Skip to content

fix(deps): replace nets with Electron net.request in telemetry - #613

Merged
cwillisf merged 4 commits into
developfrom
fix/telemetry-replace-nets
May 13, 2026
Merged

fix(deps): replace nets with Electron net.request in telemetry#613
cwillisf merged 4 commits into
developfrom
fix/telemetry-replace-nets

Conversation

@cwillisf

@cwillisf cwillisf commented May 11, 2026

Copy link
Copy Markdown
Contributor

Proposed Changes

  • Replace nets with Electron's net.request in src/main/telemetry/TelemetryClient.js.
  • Remove nets from devDependencies (also drops the nets → request → form-data/tough-cookie/qs/cookie transitive chain from the runtime tree).
  • Introduce a small httpRequest(opts, callback) helper that wraps net.request in the same (err, res) => … signature nets exposed, so the two existing call sites stay structurally identical.

Reason for Changes

nets is unmaintained and pulls in a deprecated request chain that's the source of several open Dependabot alerts: critical form-data (unsafe random), and medium alerts for request (SSRF), tough-cookie (prototype pollution), qs (DoS), and cookie (OOB).

Beyond closing CVEs, this aligns telemetry with the rest of the app's networking. With nets, telemetry took its own path through Node's HTTP stack — invisible to whatever proxy or filter the rest of the app was sitting behind. With net.request, telemetry now uses the same Chromium networking stack as the editor itself: system proxy settings, OS cert handling, and Electron-session policies all apply consistently. Schools and parents who set up filters or proxies expecting all the app's network traffic to flow through them will get that behavior.

What's preserved

Everything the telemetry client does with the network stays the same:

  • Same packet shape (clientID, packet UUID, event name, platform string, timestamp, timezone).
  • Same opt-in semantics, queue limit (100), retry limit (3), and connectivity-check cadence.
  • Same callback contract inside the class ((err, {statusCode}) => …).

Changes added after initial review

Server URL. Telemetry now targets https://telemetry.scratch.mit.edu/ regardless of NODE_ENV. The previous staging endpoint has been retired; both production and staging builds use the same server going forward.

Wrapper shape. The nets-shaped callback wrapper was an intentional choice to keep the two call sites structurally identical, but net.request is event-stream-based rather than one-shot. The wrapper now listens for response-stream error and aborted events in addition to request.error, and routes every completion path through a once-guard so the callback fires at most once even if multiple events land. Promise-shaping the helper was considered but would have pulled the delivery-loop recursion into a different shape; that refactor is worth doing separately, not in this PR.

Verification

  • npm run test:lint passes with 0 errors locally on node 24.15.0 (baseline jsdoc warnings unchanged).
  • npm run compile builds both renderer and main bundles successfully.
  • Bundled dist/main/main.js contains no require('nets') calls.
  • Electron runtime smoke test pending — needs an npm start opt-in run to confirm a telemetry POST lands at the production server with status 200 and the connectivity-check GET flips _networkIsOnline correctly.

Dependabot scope note

This PR closes the path nets → request → form-data/tough-cookie/qs/cookie through the dependency tree. The same chain is also reachable via @scratch/scratch-gui → scratch-l10n → transifex, but those calls only run at scratch-l10n build time (not at scratch-desktop build or runtime), so they aren't a runtime exposure here. Dependabot's flat view may still show some of the underlying alerts as open until scratch-l10n is updated separately — that's tracked in its own repo.

Out of scope

Not addressing other unrelated Dependabot alerts (hull.js, tar, serialize-javascript) — separate work.

`nets` is unmaintained and pulls in a deprecated `request` chain
that is the source of several Dependabot alerts (critical
form-data, mediums for request/qs/tough-cookie/cookie). Replace
it with Electron's own `net.request`. The shape of what's sent,
when, and to whom is unchanged.

Beyond closing CVEs, this aligns telemetry's network behavior
with the rest of the app: telemetry now uses the same Chromium
networking stack as everything else, so it respects system proxy
settings, OS cert handling, and Electron-session policies the
same way every other request does. Previously, telemetry took its
own path through Node's HTTP stack — invisible to whatever proxy
or filter the rest of the app was sitting behind.

The swap is contained in `src/main/telemetry/TelemetryClient.js`
behind a small `httpRequest(opts, callback)` helper that mirrors
`nets`'s callback signature, so the two existing call sites
(`_attemptDelivery` POST and `_updateNetworkStatus` GET) keep
their shape and the rest of the class is untouched.

Note: removing `nets` only closes the `nets → request → ...` path
through the dependency tree. The same chain is also reachable
through `@scratch/scratch-gui → scratch-l10n → transifex`, but
those calls only run at scratch-l10n build time (not at
scratch-desktop build or runtime), so they aren't a runtime
exposure here. Dependabot's flat view may continue to show the
underlying alerts until scratch-l10n is updated separately.

Verified locally: lint passes (0 errors, 38 baseline warnings
unchanged); `npm run compile` builds both renderer and main
bundles successfully under node 24.15.0; the bundled main
process contains no `require('nets')` calls. Electron runtime
smoke test (opt-in flow -> HTTP 200 from staging server)
pending review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the unmaintained nets dependency from the telemetry client and migrates telemetry HTTP calls to Electron’s net.request, reducing the runtime dependency tree and aligning telemetry networking with Electron/Chromium’s network stack.

Changes:

  • Added a small httpRequest(opts, callback) wrapper around electron.net.request to preserve the existing nets-style callback shape.
  • Switched the telemetry POST delivery and connectivity-check GET to use httpRequest instead of nets.
  • Removed nets from package.json and pruned it from package-lock.json.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/main/telemetry/TelemetryClient.js Replaces nets with an Electron net.request wrapper and updates the two call sites to use it.
package.json Removes nets from the dependency tree.
package-lock.json Removes the nets entry from the lockfile.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/main/telemetry/TelemetryClient.js Outdated
cwillisf added 3 commits May 11, 2026 14:29
`net.request` throws if used before the Electron `app` `ready`
event fires. TelemetryClient's constructor schedules an immediate
connectivity-check via `setTimeout(_updateNetworkStatus, 0)`,
which can land before app-ready when the telemetry singleton is
constructed at module import time (as it is in
`src/main/ScratchDesktopTelemetry.js`). The previous `nets`
implementation used Node's HTTP module and had no such
constraint, so this was a regression introduced by the swap.

Wrap the helper's body in `app.whenReady().then(...)` so requests
issued pre-ready are deferred until they're allowed; ones issued
post-ready dispatch on the next microtask (no measurable
latency). `.catch(callback)` keeps any thrown errors flowing
through the existing error path.
…errors

Listen for response-stream `error` and `aborted` events so a connection
reset mid-body doesn't leave them unhandled. Route every completion
path through a once-guard so _attemptDelivery can't double-shift the
queue if both an error and a normal end fire.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Comment thread src/main/telemetry/TelemetryClient.js
@cwillisf
cwillisf merged commit 33d629c into develop May 13, 2026
8 checks passed
@cwillisf
cwillisf deleted the fix/telemetry-replace-nets branch May 13, 2026 16:16
@github-actions github-actions Bot locked and limited conversation to collaborators May 13, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants