Skip to content

[Security] Triage the six alerts from the first security-extended CodeQL run #1389

Description

@pathosDev

Component: .github/codeql/, plus the files each alert names
Blocks: #1297 (its AC-2 — "every alert from the first widened run is fixed or dismissed with a recorded reason, so the baseline still means something")

a2ed3301 widened the CodeQL suite to security-extended. The first widened analysis on develop completed green and produced six new open alerts (code-scanning/alerts: 6 open, 8 fixed, 1 dismissed). This is the triage #1297 deferred, kept separate because the volume was not knowable in advance.

None of the six is in shipped library code on a request path. Below is a first read of each — a starting point, not a decision. Each still needs its own verdict, and a dismissal needs its reason recorded in the GitHub alert, not only here.

js/file-system-race — 2 alerts, CodeQL severity high

Alert Location
#10 docs/scripts/scaffold.mjs:856
#11 scripts/build-devtools-ui.mjs:599

Both are genuine TOCTOU by the query's definition. scaffold.mjs checks whether a page exists (it counts skipped) and then writeFileSyncs; build-devtools-ui.mjs reads the generated module, compares, and writes only on a difference — the skip being deliberate, so tsc --watch does not churn.

Both are developer-run build scripts operating on repository-relative paths, so the attacker the query imagines is another process on the maintainer's own machine. The likely verdict is "won't fix — not attacker-reachable", but note that build-devtools-ui.mjs writes src/devtools/generated/UiAssets.ts, which is committed and ships, and #620 already made the point that the embedded payload is reviewed by nothing but eyes. Worth deciding rather than waving through.

js/missing-origin-check — 3 alerts, CodeQL severity medium

Alert Location
#13 src/worker/WorkerNode.ts:48
#12 src/testkit/internal/ParallelMultiNodeBootstrap.ts:235
#14 tests/smoke/fixtures/parallel-mns-worker-throws-after-ready.mjs:42

The query is written for browser window.postMessage, where a handler must check event.origin because any page can post to any window. These three are self.onmessage inside a worker threadworker_threads on Node/Bun — where a MessageEvent carries no origin at all and the channel is a private port between one parent and one child. The premise does not transfer, and WorkerNode.ts:48 does discriminate on data.kind === 'worker-init'.

Do not dismiss these as "false positive" without reading the next paragraph, because the neighbourhood is not clean. Three tracked issues say the worker frame path really is under-validated: #701 (WorkerBroker.onMessage dereferences env.to unvalidated, so one malformed frame throws an uncatchable TypeError and kills the host), #945 (MessageChannelTransport calls the wire handler with no validateWireFrame and no try/catch), #775 (WorkerCluster.handshake has no first-hello latch). The query is pointing at the right region for the wrong reason. Dismissing the alerts is probably right; concluding from that that the region is fine is not.

js/log-injection — 1 alert, CodeQL severity medium

Alert Location
#15 examples/voice/static/plain/index.html:497

Server-controlled data reaches console.warn('voice-target-failed', m) in the example's browser client. Passed as a second argument rather than concatenated, so the browser renders it as a structured value and there is no line to forge — which makes this weaker than the rating suggests.

It is also the one most worth actually fixing rather than dismissing: examples/** is in CodeQL's scope deliberately, and the config says why — "examples/** source itself stays in scope — it is what a reader copies from, so an injection in it propagates." A dismissal here is a decision that the pattern is safe to copy.

Acceptance criteria

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    infrastructureCI / build / live-integration testspriority: mediumUseful, not urgentsecuritySecurity-relevant — see severity label for impact tierseverity: lowMinor / informational / mitigated-by-design

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions