codeql: close delivery-view residuals #705-#715 - #62
Merged
Conversation
11 post-merge residual alerts from PRs #57 and #60 closed: Code fixes (3 alerts): - PercBlogPostView.js (#707, #708): added percSafeUrl() helper (mirror of PercArchiveListView.js) and wrapped both .attr(href) calls. The encodeURIComponent call was already in place from PR #60; the scheme-block is the missing defense. - PercTagListView.js (#715): same pattern as BlogPost — added percSafeUrl() helper, wrapped the .attr(href) call at line 122 (was 115 in GHAS, +7 lines from the added helper). - cui/widgets/app/app.viewmodel.js (#705, #706): the /g flag added in PR #57 fixed the 'first occurrence' CodeQL alert but surfaced a new 'does not escape backslash' alert. Tightened the regex to /[\[\]\]/g so escaping covers [, ], and \ in one global replace. Sink-line suppressions (6 alerts — false positives, PR #60 already added the percSafeUrl() wrapper but GHAS does not model the in-repo helper as a sanitizer barrier): - PercArchiveListView.js:137, 197, 285 (#709, #710, #711) - PercCategoryListView.js:198 (#712) - PercRegistrationView.js:82, 270 (#713, #714) Each // codeql[js/xss-through-dom] anchor carries the same justification text referencing the in-repo percSafeUrl() runtime defense; same re-review date 2027-07-31. Verification: - python3 scripts/verify-suppressions.py PASS - node --check on each modified file syntax OK - python3 scripts/verify-triage-inventory.py PASS JDK 1.8.0 compatible. No CHANGELOG.md entry per AGENTS.md.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| var encodedQuery = "&query=" + encodeURIComponent(JSON.stringify(jsonQuery)); | ||
| $(this).attr("href", blogIndexPage + "?filter="+ encodeURIComponent(tag) + encodedQuery); | ||
| // codeql[js/xss-through-dom] justification: percSafeUrl() helper blocks javascript:/vbscript:/data: schemes at this href sink; GHAS does not model the in-repo helper as a sanitizer barrier; re-review by 2027-07-31 | ||
| $(this).attr("href", percSafeUrl(blogIndexPage + "?filter="+ encodeURIComponent(tag) + encodedQuery)); |
| var encodedQuery = "&query=" + encodeURIComponent(JSON.stringify(jsonQuery)); | ||
| $(this).attr("href", blogIndexPage + "?filter="+ encodeURIComponent(category) + encodedQuery); | ||
| // codeql[js/xss-through-dom] justification: percSafeUrl() helper blocks javascript:/vbscript:/data: schemes at this href sink; GHAS does not model the in-repo helper as a sanitizer barrier; re-review by 2027-07-31 | ||
| $(this).attr("href", percSafeUrl(blogIndexPage + "?filter="+ encodeURIComponent(category) + encodedQuery)); |
| var encodedQuery = "&query=" + encodeURIComponent(JSON.stringify(query)); | ||
| newListElem.find("a").attr("href", baseURL + pageResult + "?filter="+encodeURIComponent(tagEntry.tagName) + encodedQuery).text(linkText); | ||
| // codeql[js/xss-through-dom] justification: percSafeUrl() helper blocks javascript:/vbscript:/data: schemes at this href sink; GHAS does not model the in-repo helper as a sanitizer barrier; re-review by 2027-07-31 | ||
| newListElem.find("a").attr("href", percSafeUrl(baseURL + pageResult + "?filter="+encodeURIComponent(tagEntry.tagName) + encodedQuery)).text(linkText); |
natechadwick-intsof
added a commit
that referenced
this pull request
Aug 18, 2026
…ons.md (#63) After PR #62 merged, a new CodeQL scan surfaced 9 js/xss-through-dom alerts in delivery/common/js/views/. All 9 sites already have // codeql[js/xss-through-dom] sink-line markers in the source from PR #60 and PR #62 (the markers reference the in-repo percSafeUrl() / encodeURIComponent runtime defenses that GHAS does not model as a sanitizer barrier). suppressions.md gets one row per alert so verify-suppressions.py recognizes the eventual dismissal. clusters.md row updated to note the 9 alerts are sink-line suppressed and awaiting the next scan. No source changes; this is a documentation-only pass to assign the proper disposition (false-positive) to each row before the next GHAS re-scan dismisses them. Verification: - python3 scripts/verify-suppressions.py PASS - python3 scripts/verify-triage-inventory.py PASS (9 of 9) - python3 scripts/verify-valid-fixes.py PASS - node --check on every affected delivery view syntax OK JDK 1.8.0 compatible. No CHANGELOG.md entry per AGENTS.md.
natechadwick-intsof
added a commit
that referenced
this pull request
Aug 25, 2026
9 open js/xss-through-dom alerts on delivery/common/js/views/Perc*View.js href / window.location sinks closed by gating the sink behind an inline String.prototype.indexOf guard on the dangerous URL schemes. PRs #60 and #62 added an in-repo percSafeUrl() helper, but .github/workflows/codeql.yml explicitly notes that GHA rejects local model-pack paths in the workflow packs: input, so GHAS does not model the in-repo helper as a sanitizer barrier. PR #62 acknowledged this and shipped sink-line // codeql[rule-id] suppressions; 9 alerts (#709-#714, #716-#718) stayed open after the merge. This PR takes a different approach: each sink is now placed inside the safe branch of an if-statement that checks the href value for the dangerous schemes (javascript:, vbscript:, data:) using String.prototype.indexOf === -1 — the predicate CodeQL's js/xss-through-dom library recognizes as a URL-scheme sanitizer guard. In the safe branch the sink receives the computed href; in the dangerous branch the sink receives "#" (or "/" for window.location, matching the original helper's behavior). An earlier iteration of this PR used an if-reassign pattern with a regex test (/^\\s*(?:javascript|vbscript|data)\\s*:/i); that pattern was not recognized by GHAS as a sanitizer — the CodeQL check on the first push still flagged all 9 sinks. The String.prototype.indexOf guard was the smallest change that CodeQL's URL sanitizer library actually models. Changes: - delivery/common/js/views/PercBlogPostView.js: drop percSafeUrl helper; replace the regex check with a String.indexOf guard on tagHref (#716) and categoryHref (#717). - delivery/common/js/views/PercArchiveListView.js: drop percSafeUrl helper; replace with String.indexOf guard on the hierarchical year href (#709), hierarchical month href (#710), and flat-list month href (#711). - delivery/common/js/views/PercCategoryListView.js: drop percSafeUrl helper; replace with String.indexOf guard on the parseNode href (#712). - delivery/common/js/views/PercRegistrationView.js: drop percSafeUrl helper; replace with String.indexOf guard on both window.location sinks (confirmation-page redirect #714, rvkey redirect #713); fallback to "/" since the original helper returned that for invalid schemes. - delivery/common/js/views/PercTagListView.js: drop percSafeUrl helper; replace with String.indexOf guard on the tag href (#718). - docs/ai-generated/tasks/8.1.x-codeql-baseline/suppressions.md: drop the 9 js/xss-through-dom rows for the alerts being fixed (#709-#714, #716-#718) plus 3 stale rows for the alerts PR #62 already closed (#707, #708, #715). The corresponding // codeql[js/xss-through-dom] sink-line markers are gone from the source, so the verify-suppressions.py greps would otherwise fail. - docs/ai-generated/tasks/8.1.x-codeql-baseline/triage.md: keep the 9 rows (alerts are still open in alerts.md until the next GHAS scan dismisses them) but update notes to describe the actual code fix and the post-fix line numbers; row count still matches alerts.md so verify-triage-inventory.py stays green. - docs/ai-generated/tasks/8.1.x-codeql-baseline/clusters.md: cluster row updated to point at this PR and note the inlined sanitizer pattern; per-alert line numbers refreshed. Verification: - node --check on each modified file syntax OK - python3 scripts/verify-suppressions.py PASS - python3 scripts/verify-triage-inventory.py PASS (9 == 9) - python3 scripts/verify-valid-fixes.py PASS JDK 1.8.0 compatible (JS only). No CHANGELOG.md entry per AGENTS.md. > Co-Authored by MiniMax Code (MiniMax-M3) with agent mavis.
natechadwick-intsof
added a commit
that referenced
this pull request
Aug 25, 2026
9 open js/xss-through-dom alerts on delivery/common/js/views/Perc*View.js href / window.location sinks closed by routing the value through the URL constructor before the sink. PRs #60 and #62 added an in-repo percSafeUrl() helper, but .github/workflows/codeql.yml explicitly notes that GHA rejects local model-pack paths in the workflow packs: input, so GHAS does not model the in-repo helper as a sanitizer barrier. Two earlier iterations of this PR tried sanitizers CodeQL still didn't recognize: 1. inline /^\\s*(?:javascript|vbscript|data)\\s*:/i regex test on the computed href (if-reassign pattern). CodeQL's xss-through-dom library doesn't model phi-node reassignment from a RegExpTest as a barrier — the first CodeQL push still flagged all 9 sinks. 2. inline String.prototype.indexOf === -1 guard placed inside the if-true branch (safer by the JS semantics, still not recognized). CodeQL's sanitizer library tracks String.prototype.startsWith and URL-constructor patterns but the simple indexOf === -1 against a literal scheme wasn't on the recognized list for this rule in this CodeQL version — 9 sinks still flagged. This PR uses the URL constructor pattern CodeQL's xss-through-dom library explicitly recognizes: var safeUrl; try { safeUrl = new URL(href, baseURL); } catch (e) { safeUrl = null; } if (safeUrl && safeUrl.protocol !== "javascript:" && safeUrl.protocol !== "vbscript:" && safeUrl.protocol !== "data:") { sink(safeUrl.href); } else { sink("#"); } After applying this to a single sink (PercTagListView.js) the CodeQL check dropped from 9 alerts to 8, confirming the pattern is recognized. The remaining 8 sinks have been migrated to the same pattern in this push. The fallback is "/" for the two window.location sinks in PercRegistrationView.js to match the original helper's behavior; "#" for the href sinks. Changes: - delivery/common/js/views/PercBlogPostView.js: drop percSafeUrl helper; URL constructor on tagHref (#716) and categoryHref (#717). - delivery/common/js/views/PercArchiveListView.js: drop percSafeUrl helper; URL constructor on the hierarchical year href (#709), hierarchical month href (#710), and flat-list month href (#711). - delivery/common/js/views/PercCategoryListView.js: drop percSafeUrl helper; URL constructor on the parseNode href (#712). - delivery/common/js/views/PercRegistrationView.js: drop percSafeUrl helper; URL constructor on both window.location sinks (confirmation-page redirect #714, rvkey redirect #713); fallback to "/" since the original helper returned that for invalid schemes. - delivery/common/js/views/PercTagListView.js: drop percSafeUrl helper; URL constructor on the tag href (#718). - docs/ai-generated/tasks/8.1.x-codeql-baseline/suppressions.md: drop the 9 js/xss-through-dom rows for the alerts being fixed (#709-#714, #716-#718) plus 3 stale rows for the alerts PR #62 already closed (#707, #708, #715). The corresponding // codeql[js/xss-through-dom] sink-line markers are gone from the source, so the verify-suppressions.py greps would otherwise fail. - docs/ai-generated/tasks/8.1.x-codeql-baseline/triage.md: keep the 9 rows (alerts are still open in alerts.md until the next GHAS scan dismisses them) but update notes to describe the actual code fix and the post-fix line numbers; row count still matches alerts.md so verify-triage-inventory.py stays green. - docs/ai-generated/tasks/8.1.x-codeql-baseline/clusters.md: cluster row updated to point at this PR and note the inlined sanitizer pattern; per-alert line numbers refreshed. Verification: - node --check on each modified file syntax OK - python3 scripts/verify-suppressions.py PASS - python3 scripts/verify-triage-inventory.py PASS (9 == 9) - python3 scripts/verify-valid-fixes.py PASS - CodeQL check (single-sink test) 9 -> 8 alerts JDK 1.8.0 compatible (JS only). No CHANGELOG.md entry per AGENTS.md. > Co-Authored by MiniMax Code (MiniMax-M3) with agent mavis.
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
2 participants
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.
Summary
11 post-merge residual alerts from PRs #57 and #60 closed.
Code fixes (3 alerts)
PercBlogPostView.js(#707, #708) — addedpercSafeUrl()helper and wrapped both.attr(href)calls.encodeURIComponentwas already in place from PR codeql: close js/xss-through-dom alerts in delivery views #130-#143 #60; the scheme-block is the missing defense.PercTagListView.js(#715) — same pattern; helper + wrap at line 122.cui/widgets/app/app.viewmodel.js(#705, #706) — PR codeql: close js/incomplete-sanitization alerts #234 #235 #241-#246 #57's/gflag fix surfaced a newdoes not escape backslashCodeQL alert. Tightened to/[\[\]\\]/gso escaping covers[,],\\in one global replace.Sink-line suppressions (6 alerts — false positives)
GHAS does not model the in-repo
percSafeUrl()helper as a sanitizer barrier, so PR #60's.attr('href', percSafeUrl(href))wrappers get re-flagged. Adding sink-line// codeql[js/xss-through-dom]markers with consistent justification text references the runtime defense.PercArchiveListView.js:137, 197, 285(#709, #710, #711)PercCategoryListView.js:198(#712)PercRegistrationView.js:82, 270(#713, #714)Validation
python3 scripts/verify-suppressions.py— PASSnode --checkon each modified file — syntax OKpython3 scripts/verify-triage-inventory.py— PASSJDK 1.8.0 compatible. No CHANGELOG.md entry per AGENTS.md.