feat(a11y-core): cross-origin iframe ad denylist — Type A frame-enumeration gate (AXE-3735)

[chikara1608]

What

Adds the Type A gate for the Cross-Origin Iframe Ad Denylist (COI denylist). When Type A cross-origin traversal is armed (allowCrossOriginIframe), cross-origin ad iframes whose origin is on a curated denylist are skipped at frame enumeration, so they are never traversed under allowedOrigins: '*'.

The denylist itself lives in a11y-engine-core (bundled .txt, parsed into a Set at scan start). This fork cannot import a11y-engine-core, so it:

• receives the already-built Set by reference via axe.configure({ crossOriginDenylist }) — mirroring the existing allowedOrigins plumbing exactly, and
• carries its own small suffix matcher.

Why the gate is at enumeration (not postMessage)

frame-messenger/post-message.js has only a Window, not the iframe element — "there is no way to know the origin of win, so we'll try them all" — and allowedOrigins there is an allow-list ('*' when Type A COI is on) that cannot express "all except". So the denylist must act one layer up, at Context.frames population (pushUniqueFrameSelector), the single enumeration chokepoint that feeds both the sync (collectResultsFromFrames) and async (getFrameContexts) traversal paths — where the iframe element and its readable src are still in hand.

Changes

File	Change
lib/core/public/configure.js	handle spec.crossOriginDenylist → audit.setCrossOriginDenylist (sibling to allowedOrigins)
lib/core/base/audit.js	setCrossOriginDenylist() beside setAllowedOrigins; propagate in _init so child-frame audits inherit it
lib/core/base/context/parse-selector-array.js	gate pushUniqueFrameSelector on axe._audit.crossOriginDenylist — denied frames never enter context.frames
lib/core/utils/is-denied-frame-origin.js (new)	suffix matcher (host === entry || host.endsWith('.' + entry)), fail-open; + utils barrel export
build/tasks/esbuild.js	.txt text loader so a11y-engine-core can inline its bundled denylist (JS bundling unchanged)

All fork edits are tagged // [a11y-critical] / // [a11y-core] per repo convention.

Safety

• Off ⇒ byte-identical to main. When the Type A flag is off, crossOriginDenylist is never set, the helper returns false, and enumeration is unchanged.
• Pass-by-reference, no cross-frame serialization of the list — each frame reads its own bundled copy and feeds its own local axe.configure.
• Fail-open: unparseable src / non-http(s) / srcless / about:blank → never denied.

Testing

• New unit test test/core/utils/is-denied-frame-origin.js (exact/subdomain match, no false positive on substring hosts like notdoubleclick.net, flag-off no-op, fail-open).
• Fork unit suite green for configure, audit, context, get-frame-contexts, and the new helper (233 passing / 5 skipped).

Branched off latest main.

Related: AXE-3697 (cross-origin B1), AXE-3735 (COI denylist). Paired with the a11y-engine-core PR that ships the denylist + Type B1/C gates.

Closes: AXE-3735

🤖 Generated with Claude Code

[chikara1608]

Claude Code PR Review — axe-core side (paired with a11y-engine#2628)

Reviewed as the submodule half of the AXE-3735 COI-denylist pair. The full combined review lives on a11y-engine#2628; this comment carries the findings that live in this repo. Overall the Type A gate is clean, minimal, and correctly placed at the single pushUniqueFrameSelector enumeration chokepoint — nice. Two things to address, a couple to confirm.

🔴 High — new test file missing the mandatory [a11y-*] submodule tag

test/core/utils/is-denied-frame-origin.js — .claude/rules/commit-conventions.md requires every file changed under axe-core/ to carry a // [tag]: impact comment, and the reviewer checklist treats submodule tagging as a hard-fail. The six source files here are correctly tagged ([a11y-core] / [a11y-critical]); only this test file has none.
Fix: add a top-of-file line, e.g. // [a11y-core]: COI denylist Type A gate — matcher tests.

🟡 Low — matcher duplication has already drifted from the a11y-engine-core copy

lib/core/utils/is-denied-frame-origin.js duplicates the suffix walk in a11y-engine-core/lib/commons/coi-denylist.js (isDeniedHostname). The duplication is justified and documented (the fork can't import a11y-engine-core), but the two copies already differ: this fork restricts to http:/https: and resolves relative src against window.location.href; core's hostnameOf does neither. Nothing enforces they stay in sync.
Fix: a shared input→output test-vector table run against both, or at minimum reciprocal "keep in sync with X" header pointers.

🟡 Low — confirm intent: Type A silently drops denied frames

parse-selector-array.js drops a denied frame with a bare return, so it never enters context.frames and produces no not-evaluated marker. The B1/Type C lanes in a11y-engine-core route denied frames through markFrameNotEvaluated. So the same ad frame surfaces as not-evaluated in a B1/C report but is entirely absent from Type A. Either mark it not-evaluated in Type A too, or note here that silent drop is intended for Type A and why the lanes differ.

Notes

• Spec citations unreachable: comments here cite docs/cross-origin-iframe-denylist-spec.md (spec §1/§5.1). That path is gitignored and untracked in a11y-engine, so the cited spec isn't reachable from either repo. Commit it somewhere tracked or inline the load-bearing rules.
• Merge order: a11y-engine#2628 pins the axe-core submodule to this PR's unmerged head 8e3e3ab75. Merge this first, then re-bump the pointer to the post-merge SHA (a squash SHA won't equal 8e3e3ab75).
• Tagging: [a11y-critical] on parse-selector-array.js / configure.js is the right call — frame enumeration is global blast radius.

Verdict: changes requested — blocked on the missing submodule tag; the rest are cleanups/confirmations.

[chikara1608]

Claude Code Review (automated) — 3 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[chikara1608]

[Critical] Gate is on the secondary enumeration path — the primary full-page path is ungated

This gate only runs inside pushUniqueFrameSelector (explicit frame-path selectors). The Context constructor also enumerates frames via select('frame, iframe', this).forEach(... pushUniqueFrame ...) in context.js:53-56 → createFrameContext, and that path has no denylist check (context.js references the denylist nowhere). That path is what an ordinary full-page axe.run(document) uses, so denied ad iframes are still enumerated and traversed in essentially every real scan. The :62 comment calling this "the single frame-enumeration chokepoint" is false.

Suggestion: Move the check into the shared chokepoint createFrameContext(frame, context), or also gate pushUniqueFrame in context.js (it gets frame.actualNode, the element with .src). Add a Context-level integration test proving a denied frame is absent from context.frames via the full-page path. Fix the misleading comment.

Reviewer: stack:code-reviewer

[chikara1608]

[High] Trailing-dot FQDN bypasses the denylist

new URL('https://doubleclick.net./x').hostname returns doubleclick.net. (WHATWG preserves the root-label dot). Exact .has() misses it and the suffix walk yields net. → `` → never doubleclick.net, so a denied origin served with a trailing-dot host is scanned.

Suggestion:

Suggested change

	host = url.hostname.toLowerCase();
	host = url.hostname.toLowerCase().replace(/\.$/, '');

Mirror this in the a11y-engine-core isDeniedHostname helper per the KEEP-IN-SYNC contract, and add a shared test vector.

Reviewer: stack:code-reviewer

[chikara1608]

[Medium] .txt text-loader appears to be a no-op in this repo

No .txt import exists anywhere under axe-core lib/. The bundled denylist lives in a11y-engine-core's config/coi-denylist.txt, built by a11y-engine-core's own build.

Suggestion: Confirm whether there's an undocumented cross-repo build dependency (a11y-engine-core invoking axe-core's grunt esbuild task). If so, note it in the PR; otherwise this line is dead config and belongs in the a11y-engine-core build.

Reviewer: stack:code-reviewer

[chikara1608]

Claude Code PR Review

PR: #259 • Head: 6fa3cc9 • Reviewers: stack:code-reviewer

Summary

Adds the Type A (axe-core fork) half of the AXE-3735 cross-origin-iframe ad denylist: a new isDeniedFrameOrigin matcher, a frame-enumeration gate, an audit-level crossOriginDenylist carry, and an esbuild .txt text-loader entry. Flag-gated and fail-open by design.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	—
High	Security	Authentication/authorization checks present	N/A	No auth surface
High	Security	Input validation and sanitization	Pass	new URL() in try/catch, protocol allowlist
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Fail	Gate placed on the wrong (secondary) enumeration path — see Finding 1
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Matcher fails open on parse errors (intended)
High	Correctness	No race conditions or concurrency issues	Pass	—
Medium	Testing	New code has corresponding tests	Fail	Matcher unit-tested; the actual gate has no integration test — see Finding 4
Medium	Testing	Error paths and edge cases tested	Partial	Trailing-dot / port / uppercase cases missing
Medium	Testing	Existing tests still pass (no regressions)	Pass	—
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Denylist read hoisted out of the loop; O(labels) Set walk per frame
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors allowedOrigins plumbing
Medium	Quality	Changes are focused (single concern)	Pass	—
Low	Quality	Meaningful names, no dead code	Partial	esbuild .txt loader may be dead in this repo — see Finding 3
Low	Quality	Comments explain why, not what	Fail	"single frame-enumeration chokepoint" comment is factually wrong and masked Finding 1
Low	Quality	No unnecessary dependencies added	Pass	No new Deque imports

Findings

• File: lib/core/base/context/parse-selector-array.js:74 (missing gate belongs in lib/core/base/context.js:53-56)

• Severity: Critical

• Reviewer: stack:code-reviewer

• Issue: The denylist gate was added only to pushUniqueFrameSelector, but the Context constructor enumerates frames through two paths. The primary path — select('frame, iframe', this).forEach(... pushUniqueFrame ...) at context.js:53-56 → pushUniqueFrame → createFrameContext — is used for the ordinary full-page axe.run(document) scan and has no denylist check (context.js references the denylist nowhere; verified). So denied ad iframes are still enumerated and traversed in essentially every real scan. The "single frame-enumeration chokepoint" comment at parse-selector-array.js:62 is false and is what masked this in re-review.

• Suggestion: Move the check to the true shared chokepoint createFrameContext(frame, context) (called by both push sites), or add isDeniedFrameOrigin(frame, axe._audit?.crossOriginDenylist) to pushUniqueFrame as well (it receives frame.actualNode, the element with .src). Fix the misleading comment.

• File: lib/core/utils/is-denied-frame-origin.js:43

• Severity: High

• Reviewer: stack:code-reviewer

• Issue: Trailing-dot FQDN bypass. new URL('https://doubleclick.net./x').hostname returns doubleclick.net. (WHATWG preserves the root-label dot). Exact .has() misses it and the suffix walk yields net. → `` → never doubleclick.net, so a denied origin served with a trailing-dot host is scanned.

• Suggestion: Strip a single trailing . after lowercasing: host = url.hostname.toLowerCase().replace(/\.$/, ''). Mirror the same fix in the a11y-engine-core isDeniedHostname helper per the KEEP-IN-SYNC contract, and add a shared test vector.

• File: build/tasks/esbuild.js:63

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: The .txt text-loader entry appears to be a no-op in this repo — no .txt import exists anywhere under axe-core lib/. The bundled denylist lives in a11y-engine-core's config/coi-denylist.txt, built by a11y-engine-core's own build.

• Suggestion: Confirm whether there is an undocumented cross-repo build dependency (a11y-engine-core invoking axe-core's grunt esbuild task). If so, state it in the PR; if not, this line is dead config and belongs in the a11y-engine-core build instead.

• File: test/core/utils/is-denied-frame-origin.js (file-level)

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: The matcher is well unit-tested (positive/negative/boundary), but no test asserts that a denied frame is actually dropped from context.frames during context construction. That gap is exactly why Finding 1 slipped through. Port (:8080) and uppercase-host matcher cases are also missing.

• Suggestion: Add a Context-level integration test — a fixture page with a denied + a benign iframe, denylist armed, asserting the denied one is absent from context.frames via the full-page select('frame, iframe') path (not just an explicit frame-selector array). Add :8080 and ADS.DOUBLECLICK.NET matcher cases.

Notes (non-blocking): Submodule // [tag]: impact tags verified present and correct on all changed hunks ([a11y-core] / [a11y-critical]), no sensitive info. Flag/loading gate on this side is sound and fails safe (empty Set or non-Set value → no gating). audit.js _init comment slightly overstates its role in per-frame propagation.

---

Verdict: FAIL — Finding 1 (Critical) leaves the primary full-page frame-enumeration path ungated, so the feature does not work for ordinary scans. Move the gate to createFrameContext (or cover both push sites) and add the Context-level integration test before merge.

[chikara1608]

Re-review findings addressed in ca9234ee

• 🔴 Critical — gate on wrong path: pushUniqueFrame in context.js (the select('frame, iframe') full-page path) now runs isDeniedFrameOrigin too, so denied ad iframes are dropped in ordinary axe.run(document) scans, not just on the multi-element selector path. The misleading "single chokepoint" comment is corrected to name both push sites.
• 🟠 High — trailing-dot bypass: is-denied-frame-origin.js now strips a trailing root-label dot (host.replace(/\.$/, '')), so doubleclick.net. matches.
• 🟡 Test gap: added a Context-level integration test asserting a denied frame is absent from context.frames via the tag-based path (with a benign frame kept), plus :8080 / uppercase / trailing-dot matcher cases.
• 🟡 esbuild .txt loader: not dead — a11y-engine-core/build/tasks/esbuild.js re-exports this exact task, so this loader is what bundles coi-denylist.txt. Comment updated to document the dependency (kept the line).

The trailing-dot fix is mirrored in the companion coi-denylist.js (PR #2628) per the KEEP-IN-SYNC note.

Note: eslint is clean; the full axe-core karma suite was not run locally (needs the node-22 grunt rebuild) — relying on CI.

[chikara1608]

Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[chikara1608]

Claude Code PR Review

PR: #259 • Head: 1292bca • Reviewers: stack:code-reviewer

Summary

Adds the Type A (client-side, axe-core fork) cross-origin iframe ad-denylist gate: both frame-enumeration chokepoints — the primary tag-based path (context.js) and the explicit selector path (parse-selector-array.js) — now skip frames whose origin matches the COI denylist via a new is-denied-frame-origin.js matcher, flag-gated on axe._audit.crossOriginDenylist. Enumeration is byte-identical to main when the flag is unset.

Re-review: prior thread verdicts

All three previously-unresolved threads are genuinely addressed in the pushed diff (verified, not merely claimed):

• [Critical] Primary full-page path ungated → ADDRESSED. pushUniqueFrame (context.js:91-98) now calls isDeniedFrameOrigin in its skip condition, gating the primary select('frame, iframe') path (context.js:54-58), not just the selector path. Integration test proves a denied frame is dropped on the tag path.
• [High] Trailing-dot FQDN bypass → ADDRESSED. is-denied-frame-origin.js:216 strips the root-label dot (.replace(/\.$/, '')) before both .has() and the suffix walk; covered by doubleclick.net./x and ads.doubleclick.net./x assertions. Matches the core helper's normalization.
• [Medium] .txt loader is dead config → ADDRESSED (prior premise was incorrect). The loader is load-bearing: a11y-engine-core/build/tasks/esbuild.js re-exports this task, and coi-denylist.js imports ../../config/coi-denylist.txt. Inert (harmless) only when axe-core builds standalone.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets; ~5,200-entry list stays in a11y-engine-core, only public ad domains in tests
High	Security	Authentication/authorization checks present	N/A	Client-side scan-config path, no auth surface
High	Security	Input validation and sanitization	Pass	Fail-open on parse errors / non-http(s) / srcless frames
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	Both enumeration paths gated; trailing-dot, suffix-walk, http(s)-only handled
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Fail-open is deliberate and correct (unmatched → not denied)
High	Correctness	No race conditions or concurrency issues	Pass	—
Medium	Testing	New code has corresponding tests	Fail	Matcher + tag-path covered; selector-path gate has no integration test
Medium	Testing	Error paths and edge cases tested	Pass	Trailing-dot, subdomain, non-http vectors covered
Medium	Testing	Existing tests still pass (no regressions)	Pass	Flag-off enumeration unchanged
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Per-frame Set.has, small frame counts
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Faithful mirror of core helper; valid [a11y-core]/[a11y-critical] tags on every hunk
Medium	Quality	Changes are focused (single concern)	Pass	Minor: cosmetic eslint-comment churn in context test
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Spec-referenced comments on the by-design Type A drop
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: lib/core/base/context/parse-selector-array.js:76
• Severity: Medium
• Reviewer: stack:code-reviewer
• Issue: The selector-path gate (pushUniqueFrameSelector) has no integration test. Coverage exists for the matcher (test/core/utils/is-denied-frame-origin.js) and the tag-based path (test/core/base/context.js), but not for a multi-segment frame selector where a denied frame must be dropped from context.frames. The whole point of the fix was "both paths must gate," so the selector path deserves a direct test.
• Suggestion: Add a test building a context with include: [['#frameSelector', '#inner']] targeting a denied-origin iframe and assert it never enters context.frames.

Advisory notes (non-blocking)

• IDN/punycode (is-denied-frame-origin.js:209): new URL().hostname returns punycode; matcher lowercases but doesn't unicode-normalize, so IDN ad hosts only match if the denylist stores punycode. Low-risk (ad hosts are ASCII); worth a one-line guarantee that config/coi-denylist.txt entries are punycode/ASCII.
• Single-label denylist entry (is-denied-frame-origin.js:227-233): the suffix walk also matches a bare TLD label — a stray net/com entry would blackhole an entire TLD. Consider rejecting label-count < 2 at list-build time (core-side, in buildCoiDenylistSet).
• Cross-lane denial consistency: Type A resolves frame.src here; B1/C feed resolveFrameTargetOrigin() into the core isDeniedOrigin. For post-load navigation/redirects/relative src these can diverge (a frame dropped in A but scanned in B1/C). Documented in spec §5/§5.1 — flag to QA to confirm expected with allowCrossOriginIframeAdv ON.
• Cosmetic: removing the redundant inline // eslint-disable-next-line no-new comments in test/core/base/context.js is harmless (file has a file-level /*eslint no-new:0*/) but is unrelated churn.

---

Verdict: PASS — all prior blocking items genuinely addressed; the only should-fix is the missing selector-path integration test (Medium). Resolve the three GitHub threads once that test lands.