Repository navigation
fix: close the tag-stripping gaps CodeQL keeps finding - #347
Merged
Merged
Conversation
All 15 open code-scanning alerts are the same 15 that were triaged and dismissed months ago. They came back because #340 renamed holding/ to www/, lwe-pipeline/ to pipelines/lwe/, and moved build.mjs and contract-tests.mjs into scripts/. CodeQL tracks an alert by file path, so every dismissal whose file moved reset to open. The alerts on files that stayed put, in scripts/lib/html-to-md.mjs and check-page-contracts.mjs, are still dismissed, which is what confirms the cause. So this fixes the 11 where fixing is cheaper than re-dismissing, and leaves 4 that no code change should touch. The security half of every old dismissal still holds: lensMarkdown returns JSON that reaches the DOM through esc(), an excerpt is stored escaped, and contentOf feeds link extraction rather than rendering. What those dismissals conceded was a FIDELITY cost, and that part is real and now fixed. `</script >` is a legal end tag and `--!>` legally closes a comment, so a stripper spelled `</script>` or `-->` hands the body through: script source lands in a markdown render, and minified JS lands in a webmention excerpt a human reads. www/_worker.js/lens.js \s* on the closer, --!> on the comment, and \b so <head> stops swallowing a page that opens with <header> and runs to the next literal </head> www/_worker.js/webmention.js same closers in excerptAround www/_worker.js/webmention-send.js same closers in contentOf pipelines/lwe/generate.mjs replaceAll on the accent's #, and --!> on the console.log marker strip scripts/build.mjs escape the backslash and the rest of the metacharacters before turning * into .* lensStripTags loops, and the comment at it says plainly that one pass of /<[^>]+>/g is already a fixpoint: the match runs from a < to the next >, so a surviving < has no > after it and a second pass cannot match. The loop is for CodeQL, which cannot see that argument, and for whoever narrows the pattern later, where the proof stops holding. Better to write that down than to claim a fix the code does not make. The contract test fails if either closer regex goes back, verified by reverting one. Left open deliberately, because the right resolution is a dismissal: #51 www/garage/horizon.html the unsafe parse IS the demo, setHTMLUnsafe beside setHTML on the visitor's own textarea #48 #49 #50 contract-tests two of these are Array.prototype.includes on a list of extracted citations, so they are not string operations at all, let alone URL checks Suite 240 pass, build, pages:check, typecheck, and 130 routes all green.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
aadhar-sh | 32baf63 | Aug 12 2026, 02:16 PM |
Contributor
Wire-size diff vs merge base
Brotli q11, the quality the edge serves. Every number here is deterministic: Client assetsNo change. 19 files, 68.07 KiB Brotli total. PagesNo change. 48 files, 490.31 KiB Brotli total. Worker bundle276.27 KiB → 276.30 KiB gzip (+0.03 KiB, +0.0%) Largest module deltas (raw bytes in bundle):
Dictionary deltas (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| s = s.replace(/<(script|style|noscript|template|svg|head|nav|footer|aside)[\s\S]*?<\/\1>/gi, ""); | ||
| s = s.replace(/<pre[^>]*>([\s\S]*?)<\/pre>/gi, (m, i) => "\n\n```\n" + lensDecode(i.replace(/<[^>]+>/g, "")).replace(/\n+$/g, "") + "\n```\n\n"); | ||
| s = s.replace(/<!--[\s\S]*?(?:-->|--!>)/g, ""); | ||
| s = s.replace(/<(script|style|noscript|template|svg|head|nav|footer|aside)\b[\s\S]*?<\/\1\s*>/gi, ""); |
Owner
Author
There was a problem hiding this comment.
Fixed. Same fixpoint loop, plus the end tag is <\/\1\b[^>]*> now so </script bar> closes the element too.
🤖 Addressed by Claude Code
Owner
Author
There was a problem hiding this comment.
Same correction as the sibling thread: the end tag fix (<\/\1\b[^>]*>) shipped and stands, but the fixpoint loop I mentioned came back out in 5de4d4e after measuring it. This one reopened as alert 69 and wants a dismissal.
🤖 Addressed by Claude Code
…t removes CodeQL re-scanned the changed lines and found two things the first pass got wrong. Both are real, and the second is the interesting one. An end tag may carry attributes. `</script bar>` closes a script element as surely as `</script>` does, so `<\/script\s*>` was still handing the body through, just for a narrower set of inputs than before. Every closer in lensMarkdown, excerptAround and contentOf is `<\/tag\b[^>]*>` now. Removing a span can RE-FORM the thing being removed out of what is left on either side. `<!-` + `<!--x-->` + `-` collapses to `<!--` the moment the middle goes, so one pass leaves a live comment opener behind. The contract test asserts that collapse directly before asserting the fix, so it demonstrates the bug rather than asserting around it. What explains the split in the alerts is the REPLACEMENT, not the pattern. The webmention strippers replace with a space, which keeps the two sides from touching and makes them immune for free; that is why CodeQL never flagged them for this and flagged lens.js twice. lensMarkdown replaces with "" to preserve markdown spacing, so it pays for that with a fixpoint loop instead. The old lensStripTags is now a general untilStable(s, re, repl) covering all five strips, and the comment at it says which of the two problems each caller has. pipelines/lwe/generate.mjs builds its console.log label by allowlisting the wordy part of the marker rather than denylisting delimiters it has to remember. Output is byte-identical on all four call sites, checked against the old form. Suite 242 pass, build, pages:check, typecheck, 130 routes all green.
The loops in the previous commit were wrong and the measurement is why. Keeping the reasoning in the file so it is not re-argued every time this moves. CodeQL wants a fixpoint loop on each strip in lensMarkdown, because removing a span can re-form what was removed out of what is left either side. That really happens here: one pass of the comment strip over `<p>a<!-<!--x-->-</p>` leaves `<p>a<!--</p>`, since the match starts at the INNER opener. It is unreachable anyway, twice over. The re-formed opener is eaten by the generic `<[^>]+>` strip further down, because `<!--</p>` matches whole. And that strip is itself already a fixpoint: its match runs from a `<` to the next `>`, so any `<` surviving a pass has no `>` after it and a second pass cannot match. The bill for looping all three, measured on one 256KB fixture, node, same V8: 6.29ms against 5.94ms median. Roughly 10% of the CPU-bound half of a scan, on the exact path LENS_PARSE_CAP exists to keep under the 10ms Workers ceiling. Real money for a bug nothing can reach, so the loops are out and the four lens.js multi-character alerts want a dismissal instead. The contract test I wrote for the loops is out with them. It passed with the loops reverted, which made it decoration, and this repo has the scar tissue to know better. The two end-tag tests stay and both fail when their fix is reverted, verified. What survives from that commit is the part that was real. An end tag may carry attributes, so `</script bar>` closes a script element as surely as `</script>` does, and `<\/script\s*>` was still handing the body through. Every closer in lensMarkdown, excerptAround and contentOf is `<\/tag\b[^>]*>` now. Suite 241 pass, build, pages:check, typecheck, 130 routes all green.
oddharsh
added a commit
that referenced
this pull request
Aug 12, 2026
… tag CodeQL, on #353: js/bad-tag-filter against httpWords(). An end tag may carry attributes and may hold whitespace, so `</script >` and `</script bar>` close a script element as surely as `</script>` does. A stripper spelled the short way hands the whole body through. Same class and same fix as #347, which closed this across the tree. The closers are `<\/tag\b[^>]*>` now, matching what lens.js already uses. Not cosmetic. The control, on `<p>keep</p><script >var leak=1;</script >tail`: OLD: ["keep","var","leak=1;","tail"] NEW: ["keep","tail"] Those three leaked words land in the HTTP side of the legible-without- JavaScript comparison, so a page carrying an inline script closed with a space reads as MORE legible than it is. A measurement tool that quietly inflates the number it exists to report is worse than one that fails. httpWords is exported and main() now runs behind `import.meta.main`, so the assertion is behavioural instead of a regex-shape match on the source. That matters here specifically: the short spelling LOOKS right, and a source-shape test would have passed on the broken version.
oddharsh
added a commit
that referenced
this pull request
Aug 12, 2026
… tag CodeQL, on #353: js/bad-tag-filter against httpWords(). An end tag may carry attributes and may hold whitespace, so `</script >` and `</script bar>` close a script element as surely as `</script>` does. A stripper spelled the short way hands the whole body through. Same class and same fix as #347, which closed this across the tree. The closers are `<\/tag\b[^>]*>` now, matching what lens.js already uses. Not cosmetic. The control, on `<p>keep</p><script >var leak=1;</script >tail`: OLD: ["keep","var","leak=1;","tail"] NEW: ["keep","tail"] Those three leaked words land in the HTTP side of the legible-without- JavaScript comparison, so a page carrying an inline script closed with a space reads as MORE legible than it is. A measurement tool that quietly inflates the number it exists to report is worse than one that fails. httpWords is exported and main() now runs behind `import.meta.main`, so the assertion is behavioural instead of a regex-shape match on the source. That matters here specifically: the short spelling LOOKS right, and a source-shape test would have passed on the broken version.
oddharsh
added a commit
that referenced
this pull request
Aug 13, 2026
… against a real agent browser (#353) * fix(nav): detect the Navigation API member, not the object Kitesurf, Cloudflare's agent browser, ships a `navigation` stub carrying entries() and currentEntry and nothing else. No canGoBack, no addEventListener. Every `if (window.navigation)` guard in the history-nav block therefore passed, and the listener line threw: TypeError: navigation.addEventListener is not a function It throws inside a requestAnimationFrame pass, so it takes out the rest of that function, which is where the maximize/restore control gets wired. Measured 2026-08-12 against the public Kitesurf CDP: the control was dead on all six pages tested (/, /writing, /terminal, /lens, /coffee, /garage) and a Chrome control wired it on all six. Verified the fix by reproducing the stub in Chrome and A/B-ing the two files against the same page: production nav.js gives maximize=DEAD plus the TypeError, this one gives maximize=WIRED and a clean console. Asking for the member each branch actually uses means a partial implementation degrades to the history fallback rather than crashing past it. * feat(agent-check): score agent readiness against a real agent browser Every agent check here reads a declaration. The route oracle sweeps routes, gen-mcp-cards diffs a card against tools/list, build.mjs counts the Markdown twins. All of them were green on 2026-08-12 while an agent browser was getting a homepage with 12 blank squares and a script that threw on every page, because the defect is in what an ENGINE does with bytes that are themselves correct. `pnpm run agent:check` drives Cloudflare's Kitesurf over the routes and scores what comes back. It is free: the public playground at kitesurf.cloudflare.app takes no account and no token, so it spends none of the 10 browser-minutes a day that /lens/shot and /lens/browser share. The same sweep through the account binding would black out the browser lenses on its first run. lib/agent-rubric.js is the scoring, kept pure for the reason lib/tui.js is pure: /lens will score arbitrary URLs out of the evidence its machine tabs already collect, and one definition has to answer a Node script, an HTTP response and `node --test` without caring which is asking. Two rules make the number defensible, both pinned in contract tests. A dimension nobody measured is unmeasured rather than zero, so max shrinks instead of the score becoming an accusation the scored site cannot answer. And the browser dimension carries the heaviest weight at 25, because the declared-surfaces layer everyone builds first scored full marks on the same run that turned up both real defects. --control is the other half. gotcha 15 and gotcha 33 are both the same lesson: an instrument reporting a browser feature as missing is usually broken itself, so a Kitesurf-only defect means nothing until the identical script passes in Chrome. Without the flag the report says the control did not run rather than implying one did. Not a CI gate. It reaches a third-party playground on every run, so as a required check it would redden PRs on somebody else's outage. * fix(agent-rubric): an empty alt is correct markup, not a missing description The rubric's own first bug, and it defamed the site it was written to check. It counted images with non-empty alt against ALL images, so /garage scored 1/10 for describability when 14 of its 16 images are taskbar sprites carrying a deliberate alt="", which is the right markup for a decorative icon. The homepage scored 5/10 on the same mistake. Three states, not two. A non-empty alt describes, an alt="" is a decision that the image owes no description, and a missing alt attribute is the only omission. Decorative images leave both sides of the ratio, which is the whole point of writing one. A page whose images are all decorative is now unmeasured on this dimension rather than failed. After the repair, measured against production: / scores 12/12 content images described with 14 decorative, /garage 2/2 with 14 decorative. Both 10/10. Also probes the Markdown twin per route, since it is the one discovery surface that is not site-wide. That immediately found four of the eight sampled routes serving no twin: /writing, /lens, /coffee and /around. * feat(lens): score what an agent browser DID, not just what the site declares /lens has scored agent readiness since it was built, across twenty checks in five categories. Every one of them is a declaration audit: does robots.txt exist, does the sitemap parse, does the MCP server card have the right shape. That layer was perfect on aadhar.sh on 2026-08-12, on a day an agent browser was getting a homepage with twelve blank squares and a script that threw on every page. A declaration cannot catch either defect, because both live in what an ENGINE does with bytes that are themselves correct. So this adds a sixth category, Execution, with two checks a declaration structurally cannot answer: agentScripts did the page's own JavaScript survive the engine? agentMedia could the engine decode the images the page served? Both ride the CDP session /lens/wire already opens, so they cost no extra browser instance and no extra minute of the 10-a-day account ceiling. Runtime and Log are enabled before the navigation, because an exception thrown during load is the interesting case, and the DOM census runs after the settle window because an image that has not finished loading yet reports naturalWidth 0 and is not broken. NEUTRAL UNTIL MEASURED is the load-bearing rule, and it is deliberately the opposite of how `unknown` is treated everywhere else in this rubric. An unknown declared check is the SITE's fact: we asked and it did not answer. An unmeasured execution check is OURS, and docking a stranger's score because our render budget was spent would make the number dishonest. Verified through the real scorer: an unrendered scan leaves `counted` at 15 and renders the category as skipped in the existing grid, a render takes it to 17. lib/agent-execution.js holds the checks and the in-page probe so the rubric and `pnpm run agent:check` cannot drift into two definitions of one claim. The CLI now publishes no overall score of its own for the same reason. Two things deliberately NOT built, both because the surface already exists. No standalone rubric module: /lens owns the score. No header band above the panes: verdictStrip() already renders a score that opens Agent-ready?, and a third copy of a number on one screen is worse than none. The honesty the band was for lands anyway, since the client already draws a zero-total category as skipped and both checks state why they are neutral. * fix(agent-check): close the script and style strip on every legal end tag CodeQL, on #353: js/bad-tag-filter against httpWords(). An end tag may carry attributes and may hold whitespace, so `</script >` and `</script bar>` close a script element as surely as `</script>` does. A stripper spelled the short way hands the whole body through. Same class and same fix as #347, which closed this across the tree. The closers are `<\/tag\b[^>]*>` now, matching what lens.js already uses. Not cosmetic. The control, on `<p>keep</p><script >var leak=1;</script >tail`: OLD: ["keep","var","leak=1;","tail"] NEW: ["keep","tail"] Those three leaked words land in the HTTP side of the legible-without- JavaScript comparison, so a page carrying an inline script closed with a space reads as MORE legible than it is. A measurement tool that quietly inflates the number it exists to report is worse than one that fails. httpWords is exported and main() now runs behind `import.meta.main`, so the assertion is behavioural instead of a regex-shape match on the source. That matters here specifically: the short spelling LOOKS right, and a source-shape test would have passed on the broken version. --------- Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What these alerts actually are
All 15 open code-scanning alerts are the same 15 that were triaged and dismissed months ago. They came back because #340 renamed
holding/towww/,lwe-pipeline/topipelines/lwe/, and movedbuild.mjsandcontract-tests.mjsintoscripts/. CodeQL tracks an alert by file path, so every dismissal whose file moved reset to open.The control is in the data: alerts on files that stayed put (
scripts/lib/html-to-md.mjs,scripts/check-page-contracts.mjs,scripts/generate-search-index.mjs) are still dismissed. Only the moved ones came back, 1:1, all 15.There is no new finding here and there was no regression. Expect it again on the next reorg, which is the argument for fixing wherever a fix is cheap and honest.
Fixed: 7 alerts
The security half of every old dismissal still holds.
lensMarkdownreturns JSON that reaches the DOM throughesc(), an excerpt is stored escaped, andcontentOffeeds link extraction rather than rendering. What those dismissals conceded was a fidelity cost, and that part was real.An end tag may carry attributes, so
</script bar>closes a script element as surely as</script>does. A stripper spelled</script>handed the whole body through, which put script source into markdown renders and minified JS into webmention excerpts a human reads. Fixing<head>also caught a latent bug where it swallowed any page opening with<header>and ran to the next literal</head>.www/_worker.js/lens.js<\/\1\b[^>]*>closers,--!>comments,\bon the element nameswww/_worker.js/webmention.jsexcerptAroundwww/_worker.js/webmention-send.jscontentOfpipelines/lwe/generate.mjsreplaceAllon the accent's#; the log label allowlists the marker's wordy part instead of denylisting delimiters (output byte-identical on all 4 call sites)scripts/build.mjs*into.*Two contract tests cover the closers. Both fail when their fix is reverted, verified.
Deliberately not fixed: 8 alerts, all wanting a dismissal
www/_worker.js/lens.js(56, 57, 58, 59) — a fixpoint loop that fixes nothing, measured.I shipped the loop CodeQL asks for and then took it back out. The re-formation it warns about is real: one pass of the comment strip over
<p>a<!-<!--x-->-</p>leaves<p>a<!--</p>, because the match starts at the inner opener. It is unreachable anyway, twice over. The re-formed opener is eaten by the generic<[^>]+>strip further down (<!--</p>matches whole), and that strip is itself already a fixpoint, since its match runs from a<to the next>, so any<surviving a pass has no>after it.Looping all three cost a measured 6.29ms against 5.94ms median on one 256KB fixture (node, same V8). That is ~10% of the CPU-bound half of a scan, on the exact path
LENS_PARSE_CAPexists to keep under the 10ms Workers ceiling. Real money for a bug nothing can reach. The reasoning is recorded at the code, along with what would change the answer: narrowing any pattern there (a literal/<script>/gre-forms and is not self-limiting), or the output ever reaching a parser instead ofpre() -> esc().The test I wrote for the loops came out with them. It passed with the loops reverted, which made it decoration.
www/garage/horizon.html(51) — the unsafe parse IS the demo.setHTMLUnsaferuns besidesetHTMLso a visitor can see what each keeps, on their own textarea in their own tab.scripts/contract-tests.mjs(48, 49, 50) — test assertions. Two areArray.prototype.includeson a list of extracted citations, so they are not string operations at all, let alone URL checks.CI
validategreen, Workers Builds green, wire size +0.11 KiB gzip on the worker bundle.The
CodeQLcheck is red on the 2 lens.js alerts above and stays red until they are dismissed. It is not a required check;validateis the only one gating merge.build,pages:check,typecheckcleanroutes:check130 routes, 0 hard failures