fix(dl-router): reach the Discord original by rewriting the proxy URL, not by reading a link that is never there - #1286
fix(dl-router): reach the Discord original by rewriting the proxy URL, not by reading a link that is never there#1286ZacxDev wants to merge 6 commits into
Conversation
…, not by reading a link that is never there preferOriginalUrl has been inert in production since it shipped. It reads `info.linkUrl`, which Chrome populates only from an ANCESTOR <a> -- and a Discord image attachment has none. MEASURED 2026-09-03 against the live client, 3 image attachments across 2 channels and 2 message shapes (single image, mosaic): <img src> host ......................... media.discordapp.net 3/3 ancestor <a> around the image .......... 0 of 3 cdn.discordapp.com anchor in the message 3/3, a SIBLING at depth 9 same pathname (unambiguous pairing) .... true ex/hm/is values across the two hosts ... byte-identical So every real right-click lands on `if (!linkUrl) return srcUrl` and downloads the resized webp thumbnail -- the exact defect #1110 set out to fix, still live for images. The unit tests all supplied a linkUrl, so both suites stayed green over a feature that could not fire: the defect lived in the seam, not in either component. The fix rewrites instead of reading the DOM: proxy host -> cdn host, resize knobs dropped, signature carried. A prior session ruled this out on the grounds that the two hosts carry different query params. They do not -- the proxy URL carries ex/hm/is PLUS the resize params. Probed from the page context of a live Discord tab, with both controls: positive control the message's own cdn anchor .............. 206 under test proxy url, host swapped, resize dropped ... 206 negative control same url, signature removed ............... network error That also closes the gap the handoff flagged as never probed: whether a cdn URL without a valid signature actually fails. It does. Chosen over a content script because the alternative needs a discord.com host permission and couples to Discord's hashed class names, which rotate on every deploy. `originalFromPreview` is pure and rewrites only what `discordChannelId` recognises as an attachment, so an avatar or emoji on the same proxy host comes back untouched -- pinned by a test that reaches that guard by a path no earlier check rejects. Two existing assertions change on purpose, both toward the clicked image's own original: the no-link case, and the mismatched-path case (the live mosaic shape, where the only anchor on offer belongs to a sibling image). Test matrix, the production path pinned at the service-worker seam: red at aba4886 downloads media.discordapp.net/...?format=webp&width=550 green at HEAD downloads cdn.discordapp.com/...?ex=1&is=2&hm=3 node: 1455 pass / 0 fail (dl-router 543, was 537). manifest 0.3.2 -> 0.3.3: 0.3.2 was never actually loaded by the browser, so the outstanding restart check becomes "brave://extensions reads 0.3.3". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
…contradicts, and keep tier-1 correlation reachable Adversarial audit of #1286 returned one deploy-blocking finding and three should-fix ones. All addressed here. 1. (blocking) `SKILL.md` carried a 🔴 "Never reach the original by rewriting a proxy URL's host" rule, whose stated reason -- that the two hosts carry different signature parameters -- is what this PR measured to be false. The skill deploys via `mkOutOfStoreSymlink`, so it is LIVE off the working tree with no `home-manager switch`: the next agent to hit a download-routing symptom would have read a false NEVER and reverted `originalFromPreview`. That is the "safety comment whose falsity would lead a maintainer to delete the guard it describes" shape, exactly. 2. Rewriting the download url made `correlateCapture` tier 1 structurally impossible for Discord attachments: the capture holds the element's src, the DownloadItem holds the rewritten url, so every such download fell to tier 3 -- the tier this repo documents as the fragile one that once "silently degraded routing to the catch-all with no error anywhere". Concretely, two images clicked in one message inside the 15 s window would bind the first download to the SECOND image's context. `onMenuClicked` now stamps the outgoing url onto the capture it came from, and tier 1 checks that third field. Same trick and the same reason as `playerDownload` synthesising a capture around its media url. The stamp matches on `srcUrl`, never on recency -- taking "the newest capture" would reproduce the very defect it removes. 3. Comments corrected against the code, per "a comment is a claim too": - the `""` resize-param entry does NOT strip a stray `&` (the urlencoded parser skips empty sequences); it covers `?=value`, which is a different and never-observed shape. Said so, and stopped claiming a measurement. - `KNOWN UNHANDLED VARIANT` is now handled and the note invited a widening that would make it worse. - "A <video> is unaffected" is false post-rewrite; video was measured at zero points and has no fallback if the rewrite is wrong for it. Tests, all shown to kill their mutant: - `?=value` empty-named param (the audit's M1, previously SURVIVED) - proxy src + non-origin link (M10, previously SURVIVED, reachable) - proxy src + non-attachment link (M11, previously SURVIVED) - tier 1 matches `downloadUrl`, and binds to the CLICKED capture not the newest -- the multi-image case stated above - the stamp is gated on the url having changed node: 550 pass / 0 fail (was 543). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
3bcb503 to
261a4cf
Compare
|
Round 1 adversarial audit complete; fixes landed in Not addressed, deliberately, with reasons: audit M7 (preview-host guard) is near-inert — the attachment guard already rejects everything it would catch. Audit M12 is genuinely unreachable dead code (both |
…ection, and it survived at two more sites Round 2 found the round-1 fix reproduced the round-1 blocker in the opposite direction, plus two sites the round-1 grep missed. F1 (my regression). The rewritten SKILL.md rule landed inside `## Player buttons / embedded video downloads`, a section documenting the TOML `media` ordered list -- whose first accessor is an attachment anchor. That list is resolved by `container.querySelectorAll`, a DESCENDANT query, so a sibling anchor resolves fine; the ancestor-only constraint belongs to `chrome.contextMenus`' `info.linkUrl`, which that section never uses. Worse, `playerDownload` never calls `originalFromPreview` at all, so for the player-button path the anchor accessor is the ONLY route to the original -- while the new prose called it futile and pointed at the rewrite instead. An agent reading it would have deleted a working accessor from live config. The context-menu path now has its own subsection, the `linkUrl` fact is scoped to it, and the player path says plainly that it does not rewrite. F2. The refuted "Do NOT reach the original by rewriting a proxy URL's host" claim survived VERBATIM in `config.example.toml` -- the file an operator copies rules from. Round 1 fixed one of its sites and called it done. A wider grep than the auditor's found a THIRD site: the comment on `service_worker.test.mjs`'s existing menu test still asserted the original sits "on the wrapping <a href>", which is why that test's supplied `linkUrl` looked sufficient for years. Corrected, and it now says why both menu tests exist. F3. The `downloadUrl` comment claimed `onMenuClicked` stamps "the url it hands to chrome.downloads.download". The stamp runs BEFORE the streaming branch, where the url goes to the sidecar's /fetch instead. Nothing misroutes; the sentence was wider than the code. F4. The preview-host guard survived round 1's sweep and was waved off as "near-inert because the attachment guard already rejects everything it would catch". That mechanism is wrong: DISCORD_CDN_HOSTS holds BOTH hosts, so `discordChannelId` accepts an origin-host attachment too, and without the host check an origin URL carrying resize knobs would have them stripped. Now pinned by a test, and the mutant dies on it (fail 1, its own assertion). node: 551 pass / 0 fail (was 550). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
|
Round 2 delta audit complete; fixes landed in Round 2's own summary of the code half: "the fix does what it claims, the mutation results are real, and I found no regression it introduced in executable code." Both its findings were prose. Not changed, and why: round 2's two 🟢 non-findings (the |
…CLAIMED and lost, and close the subsection I left open Round 3 caught a false claim in round 2's own commit message, plus the same wrong-section shape one level down. F3 was never in `54d2a654`. I made that edit, then destroyed it: the M7 mutation battery restored `route_core.js` with `git checkout --` while the comment fix was still uncommitted, and I staged and committed the reverted file. The commit message and the PR's `audit-claims round=2` block both assert it was fixed; against the tree they were false. The edit is here now, and the lesson is the ordinary one -- a fact verified before a restore is not a fact after it. Re-checking the diff immediately before committing is what would have caught it, and is what I did this time. The comment now says what the code does: the stamp runs BEFORE the streaming branch, so on the ordinary path the url does reach `chrome.downloads.download` and for a manifest it goes to the sidecar's /fetch instead, where the stamp is inert rather than wrong. The round-2 SKILL.md fix introduced the first `###` inside `## Player buttons / embedded video downloads` and nothing closed it, so 45 lines of player-button guidance -- the "Important details" block, the whole "Buttons don't appear" troubleshooting list including "ONE malformed accessor kills the WHOLE rule", and the DEPLOY ORDER note -- were filed under a heading saying they are about the context menu, a path with no rules, no accessors and no buttons. Third appearance of correct-content-wrong-section in this PR: shipped at section level in round 1, caught in round 2, reproduced at subsection level in round 2's own fix. Closed with an explicit heading that says why it exists. Also dropped an overclaim round 2 introduced in both prose sites: "sibling, inside the container" was called "exact" on the strength of a measurement that only supports the NEGATIVE half. 0 ancestor <a> of 3 says the anchor is not an ancestor; it says nothing about whether it falls inside any given `container`, and this repo's own note records it as nine levels away. Both sites now state the descendant rule and tell the reader to confirm their container encloses it. node: 551 pass / 0 fail (unchanged -- this round touches one test comment and no assertions). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
|
🔴 Retraction of a claim in the Round 3 fixes landed in Round 3's own verdict on the code: no executable code in its range, both tiers green, the M7 mutant reproduced dying Not changed: round 3's two stated non-findings ( |
…ould misdirect a specific action Round 4 found no behavioural defect (verdict: safe to merge, no 🔴, no 🟡) but four load-bearing claims that are false. All four are sentences this ladder's own earlier rounds wrote. Fixing them rather than shipping them, because each one names an action a reader would take. F1. "the stamp is inert there rather than wrong" is false, and it is the OPPOSITE error to the one it replaced. VERIFIED against the code: `startFetch` runs its own `correlateCapture` on the same url (service_worker.js:1509), and tier 1 reads `c.downloadUrl`. A stamp also implies the rewrite fired, which implies a cdn attachment target, which implies `directFile` -- so `streaming` reduces to `manifest`, and that fetch correlates at tier 1 against the capture just stamped. The stamp is DECISIVE on the streaming path, not inert. Believing "inert" is what would license moving the stamp below the branch or dropping the clause as download-only, silently demoting the fetch to tier 2/3. F2. `config.example.toml` still asserted the original sits on a sibling anchor "INSIDE the container" as bare fact, four lines above the round-3 hedge saying containment was never measured. One comment block, two contradictory claims. The bare claim is gone; SKILL.md had already been swept correctly. F3. The round-3 heading note said the context-menu path "has no rules". False: it has no PLAYER rules, but it is governed by `[site_rules."<host>".context]`, which `content_capture.js` reads on the contextmenu event and which decides where a menu download files. An operator whose menu downloads land in the catch-all would have read that line and skipped the actual fix. F4. The round-3 typo repair changed the subject's number instead of the verb's, turning a true singular claim into a false plural one: "these exercise the swap" attributes swap coverage to both menu tests, but the second supplies no `linkUrl` and therefore cannot reach the swap at all. A maintainer trimming duplication would delete the swap branch's only coverage -- the same false-coverage belief that let this feature ship inert in the first place. Also removed the emoji I reintroduced into route_core.js; dl-router extension sources are ASCII-only and the suite enforces it. Second time this round-trip. node: 551 pass / 0 fail (unchanged -- comments only, no assertions touched). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
|
Round 4 returned safe to merge, no 🔴, no 🟡 — but four 🟢 findings, each a false sentence naming an action a reader would take. Fixed rather than shipped, in Round 4's own position on the ladder, recorded because it matters: it explicitly declined to stop on the "prose could be reworded forever" basis, on the grounds that all four findings misdirect a specific concrete action rather than being stylistic. It also verified claim 1 in the tree AND the commit separately — the distinction that caught round 2's lost edit — and confirmed the round-3 heading fix held, with no fifth instance of correct-content-wrong-section. Gap it flagged rather than covered: dev-host |
…nd the menu note was under the wrong heading Round 5 verified F1 link by link and found it correct, including proving unreachable the specific exception it was asked to hunt (a stamp coexisting with `streaming` true via `mediaType === "video"` — `stampDownloadUrl`'s `!srcUrl` guard forecloses it). F2, F3's false half and the ASCII fix were clean, the latter with a negative control that watched the hygiene guard go red. Two findings remained, both mine. The round-4 comment claimed THIS test is "the only one that reaches the swap" and that deleting it "would remove the swap branch's only coverage". False: `identity.test.mjs`'s "a proxy thumbnail is swapped for the original behind it" also reaches the swap exit and kills the swap mutant on its own. Verified it is on `origin/main` (from #1110), so it predates this PR entirely. The claim was scoped in its REASON but unscoped in its CLAIM, and coverage is a suite-wide property — a maintainer deduping `identity.test.mjs` could have read this and deleted the unit-level guard. Now scoped to this file and it names the other level explicitly. That is the same species of defect the sentence was written to remove: round 4 replaced an over-attribution of coverage with an over-attribution of uniqueness. The F3 pointer — "if menu downloads land in the catch-all, add a context rule" — sat UNDER the heading "everything below is about the BUTTON, not the menu", so the operator it names was told by the heading to skip it. Fifth instance of correct-content-wrong-section in this ladder. Moved to the end of the context-menu subsection where its audience is. While moving it, softened "decides where a menu download files": a context rule contributes subject/tags as the first and most specific signal, but the sidecar's /match weighs them against title, Open Graph, link text and the url-derived signals. It is the strongest lever a config author has, not the decision. node: 551 pass / 0 fail (unchanged — comments and prose only). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
|
Round 5 fixes landed in 🔴 THE LADDER STOPS HERE, and this is the ESCAPE HATCH, not convergenceRound 5 was not a clean round — it returned one 🟡 and one 🟢, both fixed above. Under the findings-keyed rule that would mean a round 6. I am stopping instead, and recording why so the next reader knows this is open rather than absent:
What convergence DID look like, separately: round 5 verified F1 — the item most likely to be a third wrong correction of the same comment — link by link against Left open, deliberately: the two 🟢 non-findings round 5 declined to raise, and any further prose polish. If a future reader finds another false sentence in these files, it is unaudited, not cleared. |
…very round after the first foun Claude-Session-Id: e13f042e-3be9-449d-a597-4b9a7ea7d259
preferOriginalUrlhas been inert in production since it shipped. It readsinfo.linkUrl, which Chrome populates only from an ancestor<a>— and a Discord image attachment has none.Measured, against the live client
3 image attachments, 2 channels, 2 message shapes (single image + mosaic):
<img src>hostmedia.discordapp.net— 3/3<a>around the imagediv[role=button].clickableWrapper)cdn.discordapp.comanchor in the messageex/hm/isvalues across the two hostsSo every real right-click lands on
if (!linkUrl) return srcUrland downloads the resized webp thumbnail — the defect #1110 set out to fix, still live for images. Every existing unit test supplied alinkUrl, so both suites stayed green over a feature that could not fire: the defect lived in the seam, not in either component.Why a rewrite, not a DOM read
A prior session eliminated host-rewriting because "the two hosts carry different query params". They do not — the proxy URL carries
ex/hm/isplus the resize params. Probed from the page context of a live Discord tab, with both controls:That also closes the gap the handoff flagged as never probed: whether a cdn URL without a valid signature actually fails. It does.
Chosen over a content script, which would need a
discord.comhost permission and couple to Discord's hashed class names (they rotate every deploy).originalFromPreviewis pure and rewrites only whatdiscordChannelIdrecognises as an attachment, so an avatar or emoji on the same proxy host comes back untouched — pinned by a test that reaches that guard by a path no earlier check rejects.Behaviour changes, both deliberate
Two existing assertions move toward the clicked image's own original: the no-link case, and the mismatched-path case (the live mosaic shape, where the only anchor on offer belongs to a sibling image).
Test matrix
The production path is pinned at the service-worker seam, asserting which URL was downloaded (not merely that something happened):
aba48864— downloadsmedia.discordapp.net/...?format=webp&width=550cdn.discordapp.com/...?ex=1&is=2&hm=3Gate, both tiers, base
a7dac5bdnix build .#checks.x86_64-linux.nodetests— rc 0nix build .#checks.x86_64-linux.pytests— rc 0 (built one at a time)gate.shnode tier — 1455 pass / 0 fail (dl-router 543, was 537)One earlier pytests run went red on
test_six_writers_with_a_tiny_busy_timeout_still_land_every_row. Attributed to load, not to this change: the whole 1020-test dl-router target took 391 s in that run against 115 s at base — load inflates every test, an assertion inflates one — the test is itself a deliberate 5 ms busy-timeout load reproduction, this diff contains no Python, and a genuine re-run was green.The dev-host pytest tier also shows 3 failures; all 3 were confirmed to fail identically at base.
manifest0.3.2 -> 0.3.3, because 0.3.2 was never actually loaded by the browser — so the outstanding restart check becomes "brave://extensionsreads 0.3.3".🤖 Generated with Claude Code
https://claude.ai/code/session_01BePLrAzQrg983BX2FSdEjR