Skip to content

JS-2205 USER-2389 Support Window onmessage in S2819 - #7701

Open
francois-mora-sonarsource wants to merge 12 commits into
masterfrom
fix-user-2389-s2819-onmessage
Open

JS-2205 USER-2389 Support Window onmessage in S2819#7701
francois-mora-sonarsource wants to merge 12 commits into
masterfrom
fix-user-2389-s2819-onmessage

Conversation

@francois-mora-sonarsource

@francois-mora-sonarsource francois-mora-sonarsource commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Part of USER-2389

What changed

S2819 now detects unverified Window.onmessage registrations alongside the existing addEventListener("message", ...) support.

Covered receivers are non-computed onmessage assignments on window and globalThis, including local aliases whose unique write resolves to one of them (const target = window). The receiver's onmessage property must be declared in lib.dom.d.ts, which leaves Worker and Node projects alone since they compile against lib.webworker.d.ts or no DOM lib at all.

Files that alias the global with a window = self / window = global shim are excluded: they run in a Worker, where messages come from the parent and there is no origin to verify. Types cannot tell this apart, since self and window are both Window & typeof globalThis once the DOM lib is loaded, so the shim is detected syntactically. ace/lib/ace/worker/worker_v2.js in the ruling sources is exactly this case.

Listener resolution is shared with the addEventListener path, which as a side effect also resolves handlers held in const variables (const handler = event => {...}) — previously only function declarations were followed, so addEventListener gains a few true positives too.

Out of scope

  • Computed and dynamic properties (window["onmessage"]).
  • Compound assignments (??=, ||=), which do register a handler and are a known false negative.
  • Window-typed values that do not resolve to window/globalThis: parameters, iframe.contentWindow, document.defaultView.
  • Non-Window transports such as WebSocket, MessagePort or BroadcastChannel.
  • The same Worker shim on the addEventListener path, which reports it today. Harmonising the two paths would remove issues from the ruling baseline and belongs in its own change.

Why

Assignments to Window.onmessage receive postMessage events and require the same origin verification as addEventListener("message", ...).

Validation

  • npx tsx --tsconfig packages/tsconfig.test.json --test packages/analysis/src/jsts/rules/S2819/unit.test.ts
  • npx tsc --noEmit -p packages/tsconfig.test.json
  • npm run bbf
  • git diff --check origin/master...HEAD
  • Ruling: expected results unchanged.

@francois-mora-sonarsource
francois-mora-sonarsource requested a review from a team August 3, 2026 14:00
@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title USER-2389 Support Window onmessage in S2819 JS-2205 USER-2389 Support Window onmessage in S2819 Aug 3, 2026
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 3, 2026

Copy link
Copy Markdown

JS-2205

Comment thread packages/analysis/src/jsts/rules/S2819/rule.ts
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Ruling Report

No changes to ruling expected issues in this PR

@datadog-sonarsource

This comment has been minimized.

@francois-mora-sonarsource
francois-mora-sonarsource removed the request for review from a team August 3, 2026 15:49
Comment thread packages/analysis/src/jsts/rules/S2819/rule.ts Outdated
@francois-mora-sonarsource
francois-mora-sonarsource force-pushed the fix-user-2389-s2819-onmessage branch from 497d706 to f464fd0 Compare August 3, 2026 21:07
Drop the eslint-env worker opt-out: the comment form is ignored under flat
config and becomes an error in ESLint 10, and the lib.dom.d.ts check already
suppresses worker and Node projects.

Revert the unused isWindowObject parameter, drop a no-op unique-write lookup,
and document the receiver restriction and the `through` invariant.

Cover the operator and computed-property guards, non-function right-hand
sides, and listener resolution through a const arrow function.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `window = self` Worker shim is not distinguishable by type once the DOM lib
is loaded, since `self` and `window` are both `Window & typeof globalThis`. The
scope-based detection only worked while `window` was an unresolved reference,
and it guarded the new `onmessage` path only, leaving the pre-existing
`addEventListener("message", ...)` behaviour inconsistent with it.

Report the shim on both paths instead and document the limitation. Worker and
Node projects are still left alone by the `lib.dom.d.ts` check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ruling showed the exclusion is load-bearing: its only real-world hit,
ace/lib/ace/worker/worker_v2.js, aliases `window = self` at the top of a module
factory and then registers `window.onmessage`. Messages there come from the
parent page, so there is no origin to verify.

Look the write up on the `window` variable when the configuration declares it as
a global, and among the enclosing scope's unresolved references otherwise, so
the check no longer depends on whether `window` resolves. Drop the enclosing
function comparison: a shim mutates the global, so it disqualifies every
receiver in the file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@francois-mora-sonarsource

Copy link
Copy Markdown
Contributor Author

Two known gaps, left deliberately out of this PR and recorded here rather than as tickets:

1. Compound assignments are not detected. window.onmessage ??= handler and window.onmessage ||= handler do register a handler, so they are false negatives. The guard is a single condition (assignment.operator !== '='), but widening it can add issues to the ruling baseline, so it is worth doing as its own change.

2. The addEventListener path still reports Worker shims. A file doing window = self and then window.addEventListener("message", handler) is reported, whereas the same file assigning window.onmessage is now excluded. isWindowObject accepts any identifier whose name matches /window/i, so it also reports non-Window receivers such as const workerWindow = new WebSocket(...). Routing both paths through isWindowMessageReceiver would harmonise them and remove those false positives, which means updating the ruling expected results — again its own change.

For context on why the shim exclusion exists at all: dropping it made ruling fail with exactly one new issue, ace/lib/ace/worker/worker_v2.js:71, which aliases window = self at the top of a module factory and then registers window.onmessage. Messages there come from the parent page and event.origin is "", so there is nothing to verify. Types cannot separate the two cases, since self and window are both Window & typeof globalThis once the DOM lib is loaded.

Reject on the receiver name before consulting the type checker or the scope
chain, and look the Worker shim up last, so that an `onmessage` assignment on an
unrelated transport no longer pays for a scope walk. Behaviour is unchanged; the
predicate now also returns a plain boolean instead of `boolean | undefined`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gitar-bot

gitar-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Adds S2819 support for unverified Window.onmessage registrations while refining receiver validation and alias resolution, addressing the non-Window false positives and alias suppression findings.

✅ 2 resolved
Edge Case: onmessage check may false-positive on non-Window objects

📄 packages/analysis/src/jsts/rules/S2819/rule.ts:66-70 📄 packages/analysis/src/jsts/rules/S2819/rule.ts:111-125 📄 packages/analysis/src/jsts/rules/S2819/rule.ts:403-417
isWindowObject treats any object whose reference/identifier name contains the substring "window" (via WindowNameVisitor, regex /window/i) as a Window. Applying this heuristic to the new onmessage assignment surface broadens false-positive risk, because onmessage is a common property on non-Window objects (WebSocket, Worker, EventSource, MessagePort). E.g. const eventWindowChannel = new WebSocket(...); eventWindowChannel.onmessage = fn; would be flagged even though it is not a Window. Consider requiring the type-string match for aliases here, or restricting the name heuristic to exact window/globalThis identifiers rather than substring matches.

Edge Case: window=self alias suppresses S2819 for entire file

📄 packages/analysis/src/jsts/rules/S2819/rule.ts:136 📄 packages/analysis/src/jsts/rules/S2819/rule.ts:157-167
isWindowAliasedToWorkerGlobal scans every scope in the file and returns true if any window = self/window = global write exists anywhere. Because isWindowMessageReceiver short-circuits on this, a single worker-polyfill assignment disables origin checking for all window.onmessage handlers in the file, potentially masking genuinely unverified handlers (false negative). Consider scoping the alias check to the receiver/scope of the specific assignment being analyzed rather than the whole file.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

sonarqube-next Bot commented Aug 4, 2026

Copy link
Copy Markdown

@francois-mora-sonarsource
francois-mora-sonarsource marked this pull request as ready for review August 4, 2026 13:24
@francois-mora-sonarsource
francois-mora-sonarsource requested a review from a team August 7, 2026 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant