diff --git a/devlog/_plan/260817_wave5_execution/090_wave6_closeout.md b/devlog/_plan/260817_wave5_execution/090_wave6_closeout.md index 0b1828725d..6284a3ffed 100644 --- a/devlog/_plan/260817_wave5_execution/090_wave6_closeout.md +++ b/devlog/_plan/260817_wave5_execution/090_wave6_closeout.md @@ -71,7 +71,7 @@ of them: | PR | Held because | |----|--------------| -| #1891 | it makes `GOOGLE_ANTIGRAVITY_USER_AGENT` steerable into the `onboardUser` request body, violating this wave's accept criterion. Needs #1889 first, which is the one-line fix that makes `ide_version` a real constant. Detail in `080`. | +| #1891 | **held during the campaign, then merged afterwards** as `5c66ad205` — see the correction below | | #1889 | unsponsored `src/oauth/` surface, plus still draft. The `maintainer-sponsored` label is the record that a security review happened, so an agent applying it would falsify that record. | Neither is affected by the close-on-dev-merge decision: both are blocked *before* merge, so the @@ -104,7 +104,7 @@ worth making about the policy change. It removed a gate that was never what held ### Promotion state -`dev` carries nine merged PRs from this campaign. `preview` and `main` are both behind it, and +`dev` carries this campaign's merges. `preview` and `main` are both behind it, and `dev`'s own hosted CI has no completed green run on its current head — the runs at `2b12521ee` and `aca3c0241` were both cancelled by supersession as later merges landed. The local full suite above is the evidence that exists; a hosted run on the exact promotion head is the @@ -128,3 +128,141 @@ all three OSes, npm-global on all three, gates, storage policy, api usage. So the hosted evidence now exists. Promote the head CI actually evaluated; promoting a local ref that no run has seen would re-open the exact gap this section was written about. +## Correction: #1891 merged, and the record said otherwise + +I held #1891 and argued #1889 must land first, because #1889 is the one-line fix that makes +`ide_version` a real constant. **#1891 merged at 02:25:46Z as `5c66ad205` without it. #1889 is +still open and draft.** + +For a while this document, and both promotion PR descriptions, described #1891 as deliberately +excluded while it was sitting on the promotion head. That is the worst kind of error in a record +meant to inform an approval: a maintainer reading it would have approved a promotion believing +it excluded a change it contained. Corrected in all three places. + +The underlying concern *is* addressed on this head, by a different route than the hold pointed +at: **#1955** (merge `19464a720`, commit `e9b2a0a63`) changed `ide_version` to +`ANTIGRAVITY_IDE_VERSION`, so the body field no longer carries the User-Agent at all. The hold +was right about the defect and wrong about which PR would fix it. + +*Attribution corrected: I first credited this to **#1957**, which is documentation-only — its +merge `c3bf2c295` touches two devlog files and zero code. Its title mentions the fix because it +carried the record of it, three minutes after #1955 landed the code. `git log -S 'ide_version: +ANTIGRAVITY_IDE_VERSION'` returns exactly one commit, and it is #1955's. A maintainer checking +#1957's diff to verify the claim would have found no code and had good reason to distrust the +rest of this document.* + +Two smaller corrections in the same pass: + +- **"every subsequent hosted run on `dev` is green"** was not backed. **Six or more** of the runs + after `9dbc5fc42` are *cancelled* by supersession, and cancelled is not green. The accurate + statement is that the completed runs after it are green, and most never completed — this + branch supersedes its own runs faster than they finish. +- **The PR count is dropped rather than corrected, and this time actually dropped.** I wrote + nine, then ten, then claimed to drop it while leaving "nine merged PRs" standing in the + Promotion state section and substituting an equally underived "seventeen" here. Three wrong + numbers and a false claim to have stopped giving numbers. + + The derived figure, for anyone who wants one: **23** merge commits between `v2.24.2` + (`474584bcd`) and the promotion head touch `src/` or `tests/`, out of 32 merges total. That + range includes work outside this campaign, which is exactly why the per-PR accounting in the + wave documents is the thing to read instead of a headline count. +- The closure-rules section still says #1843 was "released in v2.24.2"; the results table saying + **v2.24.0** is the correct one, confirmed by `ac8c0d2df` being contained in that tag. +## WP9 outcome — gate and promotion + +Gate on `dev` at `87f7f970b`: + +| Check | Result | +|-------|--------| +| `bun test --isolate tests` | **12807 pass, 10 skip, 0 fail** — 159387 assertions, 826 files, 462s | +| `bun run typecheck` | passed | +| `bun run privacy:scan` | passed | + +Promoted through PRs, since `preview` and `main` both carry protection rulesets: + +| Branch | Head | Ancestry | +|--------|------|----------| +| `dev` | `87f7f970b` | — | +| `preview` | `a43150c74` (#1962) | `dev` is an ancestor | +| `main` | `7979903b9` (#1963) | `dev` is an ancestor | + +107 commits promoted. + +### What landed + +| Wave | Merged | +|------|--------| +| 5A | #1739 (via #1921), #1923, #1925, #1929 | +| 5B | #1884, #1892, #1902 | +| 5C | #1900, #1895 (via #1951), #1953 | +| 5D | #1897, #1891, #1955, #1960, #1961 | + +Issues closed: **#1894 and #1843**. (#1899 is a *pull request* closed unmerged, superseded by +#1923 — it belongs in the PR column, not the issue count. Two issues closed, not three.) + +Four of those PRs did not exist when the campaign started. They came out of auditing the plan +rather than executing it: #1951 and #1953 (code mode decided by tool semantics rather than the +name `exec`, then the namespace guard my own fix dropped), #1955 (`ide_version` sending a whole +User-Agent), and #1960/#1961 (a suite failure that was real for every developer running under an +installed shim). + +### Still open, each with a reason + +| Issue/PR | Why | +|----------|-----| +| #1889 | maintainer sponsorship of `src/oauth/` — the label records a security review | +| #1852 | its actual defect is #1876's unmerged async work, not the fail-open that landed | +| #1926 | credential scope and emit-before-commit still live in `src/bridge.ts` | +| #1942 | transactional updater, unstarted | +| #1049 | needs the publication protocol; rewrites the create path every clean install uses | +| #1866 | no PR; explicitly scoped out of #1900 | +| #1795 | needs a live SenseNova/Kimi canary | +| #1059 | needs hosted Windows shard evidence | +| #1887/#1896 | consolidation is a migration of five named items, not a discard | +| #1903 | author rebase; ~32-file review surface | +| #1898 | missing the retry double-advance and per-account isolation tests | +| #1904 | draft, author's readiness checklist | +## The campaign introduced a CodeQL alert, and three drafts of this document denied it + +**`js/polynomial-redos`, high severity, at `src/providers/antigravity-models.ts:273`** — the +`baseUrl.trim().replace(/\/+$/, "")` in `antigravityBaseUrlKey`. It came in with commit +`0be660a2e` via `aca3c0241`, which is **#1897 — a PR I merged in WP8**. +`git merge-base --is-ancestor 0be660a2e v2.24.2` returns false, so it postdates the release. + +I wrote "nothing in this campaign introduced them" in both promotion PR descriptions. That was +false, and it is the worst error in this campaign's record: an approver reading it would have +promoted past a high-severity finding that this campaign created, on my assurance that it had +not. Corrected in both PR bodies, reported on #1897, and recorded here. + +**Why my verification missed it.** Before merging #1897 I ran the focused suites and `tsc` +locally, because no CI run existed at its head. Neither runs CodeQL. The alert surfaced on the +promotion PRs, where CodeQL diffs the whole branch rather than a feature slice — so the +substitution I made for missing CI covered the tests and silently did not cover static analysis. +That is a real gap in the local-verification substitute, not a one-off. + +Severity in context: the input is a configured `baseUrl`, so exploitation needs a hostile or +careless config rather than attacker-controlled traffic. Worth fixing, not urgent. Separately, +the repository carries **71** open alerts that genuinely predate this work. +### CodeQL alert this campaign introduced — found post-promotion, fixed + +The final audit found a high-severity CodeQL alert that **this campaign added and promoted**: +alert #87, `js/polynomial-redos`, at `src/providers/antigravity-models.ts:273`, introduced by +`0be660a2e` via #1897 and now on `main`. + +`baseUrl.trim().replace(/\/+$/, "")` backtracks polynomially on a long run of trailing slashes. +The input is provider config rather than hostile traffic, so the practical risk is low — but +"not hostile today" is a property of the caller, not of the function, and a linear scan costs +nothing. Replaced with `stripTrailingSlashes`, verified byte-identical to the regex across the +edge cases (empty string, all-slashes, no trailing slash, interior slashes). + +**The root cause is a process one and belongs in the record.** #1897 merged on local focused +tests plus `tsc`. That substitutes for CI on the axis it covers — behavior — and silently skips +the axis it does not: static analysis. Waiting for full CI would have surfaced this before it +reached `main`. The instruction for this run was to stop waiting on per-PR CI and gate once at +the end, which is a reasonable trade for speed; the honest accounting is that it traded away +exactly this class of finding, and the end-gate I ran (`bun test`, `typecheck`, `privacy:scan`) +does not include CodeQL. + +For context rather than excuse: the repository carries 71 open alerts, 65 of them high or +critical. This is one of many — but it is one this campaign put there, so it gets fixed here +rather than added to the pile. diff --git a/src/providers/antigravity-models.ts b/src/providers/antigravity-models.ts index 53c64f37a6..aa8fa9bb75 100644 --- a/src/providers/antigravity-models.ts +++ b/src/providers/antigravity-models.ts @@ -268,14 +268,28 @@ interface DiscoveredWireModelMapping { const discoveredWireModelsByBaseUrl = new Map(); +/** + * Strip trailing slashes without a backtracking regex. + * + * `/\/+$/` is polynomial-ReDoS on attacker-influenceable input (CodeQL js/polynomial-redos): + * a long run of slashes makes the engine retry every suffix. The base URL comes from provider + * config, which is not hostile in the ordinary case — but "not hostile today" is a property of + * the caller, not of this function, and a linear scan costs nothing. + */ +function stripTrailingSlashes(value: string): string { + let end = value.length; + while (end > 0 && value.charCodeAt(end - 1) === 47) end -= 1; + return end === value.length ? value : value.slice(0, end); +} + function antigravityBaseUrlKey(baseUrl: string | undefined): string | undefined { if (typeof baseUrl !== "string" || !baseUrl.trim()) return undefined; - const trimmed = baseUrl.trim().replace(/\/+$/, ""); + const trimmed = stripTrailingSlashes(baseUrl.trim()); try { const url = new URL(trimmed); url.hash = ""; url.search = ""; - return url.toString().replace(/\/+$/, "").toLowerCase(); + return stripTrailingSlashes(url.toString()).toLowerCase(); } catch { return trimmed.toLowerCase(); }