fix(security): close the agent star-consent bypass found auditing the #892 merge - #917
Merged
Conversation
… mechanically go/internal/cli/config_parity.go was committed by a broad `git add` three times during the #820 campaign. Two of those were caught and reverted (2101d50, 58e8718); the third rode a86ee03 into the #892 merge, so 682 lines of the retired Go experiment are now tracked on dev — the one and only tracked file under go/, absent from main, preview, and v2.10.0. Nothing in src/, the build, the typecheck, or the test path reads from go/, so this is dead weight in every clone rather than a functional regression. The commit that introduced it says "untrack ... again" in its own message, which is how it passed review: the intent was right and the index was not. .gitignore alone cannot prevent the repeat, because an already-tracked path stops honoring the ignore rule — so the guard lives in tests/repo-hygiene.test.ts next to the .codexclaw/gitlink invariants, driven red once by re-adding the file.
…t headers The agent-consent refusal on POST /api/github/star asked whether the request carried an Origin plus the GUI-origin and CSRF headers, on the stated premise that requireManagementAuth had already matched them against a minted session. It had not. The gate accepts a raw admin token and returns BEFORE it consults the session table, so those headers were never validated for a token-authorized call, and the admin token is readable by anything running as the user, which is precisely the caller this guard exists to refuse. Three headers with arbitrary values were enough to star the repository with the user's identity. managementPrincipal() now resolves which credential passed the gate, from the same session table and the same CSRF comparison the gate uses, and the server passes it into the management dispatcher. The route asks for a gui-session principal: a session this process minted for a browser, which is only accepted for a mutation after origin and per-session CSRF both match. An unresolved principal (direct dispatch in tests, any future internal caller) is untrusted. Behavior for real users is unchanged: dashboard clicks still star, hand-typed runs still star, and the non-loopback operator dashboard on a raw admin token keeps the documented fail-closed edge. Both regressions were driven red against the old header check.
… racing
The eager producer raced every read against one never-settled abort promise.
Each completed read leaves a reaction attached to that pending promise, so a
long stream retained one callback per chunk until abort — the exact retention
class relay.ts documents avoiding at its own drain ("Deliberately NOT a shared
Promise.race companion"), reintroduced in the relay this campaign added.
Abort now cancels the reader instead, which settles the parked read on a silent
upstream the same way relay.ts's stopDrain does, and the loop checks the signal
once per iteration. The 31 eager-relay tests — cancel-drain expiry, shutdown
while paused, synthetic tails, teardown, and rewrite framing — stay green, which
is what proves the wake-up path is unchanged.
…reassembly The maxBytes option was mutation-surviving: deleting it and always using the 64 KiB default left the whole suite green, because the only oversize test used a 33 MiB body that exceeds both ceilings. Nothing proved that the one caller the option exists for — the non-streaming upstream JSON read, at a 32 MiB ceiling — can actually accept a response larger than an error body. Four cases now pin it: a body between the default and the custom cap succeeds, the exact cap succeeds, one byte past it fails closed with the prefix discarded, and a 20k-chunk fragmented body under the cap reassembles byte-exactly (the observable half of the geometric single-buffer accumulation). Driven red by re-ignoring the option.
…urviving All three guard real behavior that no test actually pinned: - antigravity canonicalization: removing MAX_CANONICAL_DEPTH left all 34 tests green. Byte and key budgets do not bound recursion — a deeply nested argument is tiny on the wire — so without the cap a replay observation throws RangeError instead of skipping. Depth 120 canonicalizes, 200 and 50k refuse with null. - scheduler settle predicate: the end-to-end unknown-SCM test sets taskInstalled and registrationHealthy true, so its final clause is already false and deleting the nativeServiceAbsent guard left it green. schedulerVerificationMaySettle is now exercised directly against a transient-looking tail, one unproven flag at a time, and goes red when that guard is removed. - ephemeral ACL memo release: the test inherited USERNAME from an earlier block's `??=`, which never restores it, so running it alone or in another order failed before reaching the memo behavior. It sets and restores its own environment now.
…fore abort Two findings from the second audit round. The star mutation required a GUI session only when isAgentDriven() was true, and that function reads the SERVER's environment rather than the caller's. A proxy running as a service — no agent markers, the normal remote setup — therefore accepted a raw-admin-token star from anyone who could read the token, which is every agent on the machine. Caller provenance is not knowable at this endpoint; the credential is. The dashboard session is now required unconditionally, and the refusal names the agent markers only when there are any. The former "hand-typed run stars over HTTP" test encoded exactly the hole, so it is replaced by its inverse plus a dashboard-click case on the same clean environment. The eager producer honored the abort signal before examining a settled read. A read can settle with a real chunk in the same tick the signal fires — the post-cancel drain does exactly this: the terminal frame arrives, then the drain deadline aborts upstream — so the terminal was discarded and the turn was accounted as a cancel. The chunk is inspected first now, and abort is honored immediately after. Driven red by restoring the old order.
…rect The audit's second round showed both resource fixes were mutation-surviving: restoring the per-chunk accumulator, or the shared Promise.race companion, left every test green. Neither property is visible through behavior — both shapes relay and reassemble identically — so each needed its own observable. bounded-body now counts buffer reallocations for tests. A geometric buffer grows a handful of times regardless of how the peer fragments the body; an exact-fit accumulator grows once per chunk, which is the retention shape the repair removed. The new test compares 20k one-byte chunks against the same body in one chunk and pins growth to a small constant. The eager relay's property is structural, so it is pinned structurally, the way this repository already pins the star-consent guard: neither relay may race a read against a shared abort promise (comments stripped first — both files describe the banned shape in prose), and the eager producer must keep the reader-cancel wake-up that replaced it. Both driven red by restoring the old implementations.
…the shape Two round-3 audit findings, one accepted as a documentation fix and one as a test-instrument fix. A local process running as the user can mint its own GUI session — the dashboard bootstrap is served to any loopback GET — and can equally run `gh api -X PUT /user/starred/...` without involving the proxy at all. No check inside this process distinguishes that caller from the browser, because both hold every local credential. Claiming the endpoint is a technical barrier would be false, so the route comment and AGENTS.md now say what it actually does: it removes the casual path and the raw-token path, and the real boundary is the normative rule that an agent must not spend the user's identity by any mechanism. The unconditional session requirement stays; it is just no longer described as more than it is. The bounded-body growth counter caught an exact-fit reallocation mutation but not the per-chunk `Uint8Array[]` it replaced, because that implementation never increments the counter at all. The retained-object shape is now pinned structurally — no per-chunk collection, one geometric buffer — the same instrument this repository already uses for the relay retention rule. Driven red by restoring the array accumulator.
📝 WalkthroughWalkthroughThe change removes the retired Go parity command, propagates authenticated management principals, requires GUI sessions for starring, improves bounded-body and eager SSE handling, and adds regression tests for authentication, canonicalization, repository hygiene, scheduler states, and environment isolation. ChangesManagement authorization
Streaming and body handling
Repository and platform maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65429705b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // account owner. Only a minted dashboard session evidences that; a caller | ||
| // holding the admin token must relay the question instead of answering it | ||
| // with an HTTP call. | ||
| if (!hasBrowserSessionEvidence(ctx)) { |
There was a problem hiding this comment.
For non-loopback dashboards, which authenticate with the raw admin token rather than a minted GUI session, this unconditional check now makes every star click return 403. However, docs-site/src/content/docs/guides/web-dashboard.md lines 132-147 and docs-site/src/content/docs/reference/management-api.md line 201 still say the refusal applies only to agent-driven callers, and the translated pages repeat that contract. Update the user documentation to explain that all admin-token calls are refused and that remote dashboard users are redirected to GitHub instead.
AGENTS.md reference: AGENTS.md:L212-L213
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/bounded-body.ts`:
- Line 121: Move the `bufferGrowthsForTests` reset to the entry of the
bounded-body function, before null-body handling and abort preflight, so every
invocation starts at zero. Add a focused regression test near the existing
bounded-body tests that performs a growing read, then reads `new
Response(null)`, and verifies `boundedBodyBufferGrowthsForTests()` returns zero.
In `@src/server/management-auth.ts`:
- Around line 251-271: Update managementPrincipal to perform the same session
credential, origin, and unsafe-method CSRF validation enforced by
requireManagementAuth before returning "gui-session"; do not rely on callers
invoking the gate first. Share the validation logic between
requireManagementAuth and managementPrincipal, or consolidate authentication and
principal resolution, while preserving admin-token handling and adding
regression coverage for mismatched origin and CSRF values.
In `@tests/repo-hygiene.test.ts`:
- Around line 92-94: Update the assertion loop over RETIRED_TRACKED_DIRS in the
repository hygiene test to remove blank lines and comment lines from the parsed
.gitignore entries before checking for each rule, so toContain evaluates only
active rules. Preserve the existing directory rule assertions, then run bun run
typecheck and bun run test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b68a3e9-7df3-44b5-82dd-15f42431042e
📒 Files selected for processing (18)
.gitignoreAGENTS.mdgo/internal/cli/config_parity.gosrc/lib/bounded-body.tssrc/server/index.tssrc/server/management-api.tssrc/server/management-auth.tssrc/server/management/context.tssrc/server/management/sidebar-routes.tssrc/server/relay-eager.tstests/bounded-body.test.tstests/google-antigravity-replay.test.tstests/relay-eager.test.tstests/repo-hygiene.test.tstests/sidebar-routes.test.tstests/startup-prompt.test.tstests/windows-elevation-spawn.test.tstests/windows-secret-acl.test.ts
💤 Files with no reviewable changes (1)
- go/internal/cli/config_parity.go
| // beyond the payload ceiling on large budgets. | ||
| let retained = new Uint8Array(Math.min(maxBytes, 64 * 1024)); | ||
| let retainedBytes = 0; | ||
| bufferGrowthsForTests = 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the test counter before every early return.
Line 121 runs after the null-body return and the abort preflight. A bodyless or pre-aborted call therefore leaves boundedBodyBufferGrowthsForTests() reporting the previous read's value.
Reset bufferGrowthsForTests at function entry. Add a regression test that performs a growing read, then reads new Response(null), and verifies that the counter is zero.
Proposed fix
export async function readBoundedResponseBody(
response: Response,
options: BoundedBodyOptions = {},
): Promise<BoundedBodyResult> {
+ bufferGrowthsForTests = 0;
const signal = options.signal;
if (signal?.aborted) throw signal.reason;
@@
let retained = new Uint8Array(Math.min(maxBytes, 64 * 1024));
let retainedBytes = 0;
- bufferGrowthsForTests = 0;As per coding guidelines, “Behavior changes in src/ require a focused regression test near the existing tests for that subsystem.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/bounded-body.ts` at line 121, Move the `bufferGrowthsForTests` reset
to the entry of the bounded-body function, before null-body handling and abort
preflight, so every invocation starts at zero. Add a focused regression test
near the existing bounded-body tests that performs a growing read, then reads
`new Response(null)`, and verifies `boundedBodyBufferGrowthsForTests()` returns
zero.
Source: Coding guidelines
Comment on lines
+251
to
+271
| /** | ||
| * The principal for a request that already passed `requireManagementAuth`. Kept as a | ||
| * separate resolution (rather than a changed return type) so every existing caller | ||
| * keeps its `Response | null` contract; the value is derived from the same session | ||
| * table and the same CSRF comparison the gate uses, so the two cannot disagree. | ||
| */ | ||
| export function managementPrincipal( | ||
| req: Request, | ||
| state: ManagementAuthState, | ||
| config?: OcxConfig, | ||
| ): ManagementPrincipal | null { | ||
| if (!state.available) return null; | ||
| const actual = req.headers.get("x-opencodex-api-key")?.trim() | ||
| || req.headers.get("authorization")?.replace(/^Bearer\s+/i, "").trim(); | ||
| if (!actual) return null; | ||
| if (equalSecret(actual, state.token)) return "admin-token"; | ||
| if (!config) return null; | ||
| removeExpiredSessions(state); | ||
| return state.sessions.has(actual) ? "gui-session" : null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every caller of managementPrincipal and confirm each is preceded by requireManagementAuth in the same code path.
rg -n -B5 -A2 '\bmanagementPrincipal\s*\(' --type=tsRepository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- management-auth.ts relevant sections ---'
sed -n '220,330p' src/server/management-auth.ts
printf '%s\n' '--- all managementPrincipal references ---'
rg -n -C 8 '\bmanagementPrincipal\b' --glob '*.ts' .
printf '%s\n' '--- all requireManagementAuth references ---'
rg -n -C 8 '\brequireManagementAuth\b' --glob '*.ts' .
printf '%s\n' '--- index.ts management-auth flow ---'
rg -n -C 12 'managementPrincipal|requireManagementAuth' src/server/index.tsRepository: lidge-jun/opencodex
Length of output: 27376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- principal consumers and consent route ---'
rg -n -C 12 'principal|github|star' src/server/management-api.ts src/server/sidebar-routes.ts
printf '%s\n' '--- focused auth tests and principal-related tests ---'
rg -n -C 10 'managementPrincipal|gui-session|csrf|origin' tests/server-management-auth.test.ts tests/management-integration-routes.test.ts
printf '%s\n' '--- read-only call-order verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
root = Path(".")
for path in sorted(root.rglob("*.ts")):
if any(part in {"node_modules", "dist", "build"} for part in path.parts):
continue
text = path.read_text(encoding="utf-8")
for match in re.finditer(r"\bmanagementPrincipal\s*\(", text):
line = text.count("\n", 0, match.start()) + 1
line_text = text.splitlines()[line - 1].strip()
if path.as_posix().endswith("management-auth.ts") and line_text.startswith("export function"):
kind = "definition"
elif line_text.startswith("import ") or line_text.endswith("managementPrincipal,"):
kind = "import"
else:
kind = "call"
print(f"{path}:{line}: {kind}: {line_text}")
if kind == "call":
before = text[:match.start()].splitlines()
window = "\n".join(before[-12:])
print(" preceding window:")
print("\n".join(f" {i+1}: {s}" for i, s in enumerate(window.splitlines())))
PYRepository: lidge-jun/opencodex
Length of output: 8080
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- sidebar route files ---'
fd -i 'sidebar' src tests || true
printf '%s\n' '--- principal and star references in existing TypeScript files ---'
rg -n -C 10 'principal|github|star' src tests --glob '*.ts' || true
printf '%s\n' '--- read-only call-order verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path(".").rglob("*.ts")):
if any(part in {"node_modules", "dist", "build"} for part in path.parts):
continue
lines = path.read_text(encoding="utf-8").splitlines()
for number, line in enumerate(lines, 1):
if re.search(r"\bmanagementPrincipal\s*\(", line):
if "export function managementPrincipal" in line:
kind = "definition"
elif line.lstrip().startswith("import ") or "managementPrincipal" in line and line.lstrip().startswith("{"):
kind = "import"
else:
kind = "call"
print(f"{path}:{number}: {kind}: {line.strip()}")
if kind == "call":
start = max(1, number - 12)
print(f" preceding lines {start}-{number - 1}:")
for index in range(start, number):
print(f" {index}: {lines[index - 1].strip()}")
PYRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sidebar file names ---'
fd -i 'sidebar' src tests
printf '%s\n' '--- exact ManagementPrincipal references ---'
rg -n -C 6 '\bManagementPrincipal\b|\bmanagementPrincipal\b' src tests --glob '*.ts'
printf '%s\n' '--- exact consent-related route references ---'
rg -n -C 8 'star|github' src/server src --glob '*.ts' | head -n 300
printf '%s\n' '--- focused call-order verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
pattern = re.compile(r"\bmanagementPrincipal\s*\(")
for path in sorted(Path("src").rglob("*.ts")):
lines = path.read_text(encoding="utf-8").splitlines()
for number, line in enumerate(lines, 1):
if not pattern.search(line):
continue
if "export function managementPrincipal" in line:
kind = "definition"
elif line.lstrip().startswith("import "):
kind = "import"
else:
kind = "call"
print(f"{path}:{number}: {kind}: {line.strip()}")
if kind == "call":
for index in range(max(1, number - 8), number):
print(f" {index}: {lines[index - 1].strip()}")
PYRepository: lidge-jun/opencodex
Length of output: 30628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sidebar route principal checks ---'
rg -n -C 12 'principal|star|github' src/server/management/sidebar-routes.ts
printf '%s\n' '--- management context contract ---'
sed -n '1,70p' src/server/management/context.ts
printf '%s\n' '--- focused route tests for admin-token versus gui-session ---'
rg -n -C 14 'admin-token|gui-session|star|github' tests/sidebar-routes.test.ts tests/sidebar-star-state.test.ts tests/management-integration-routes.test.tsRepository: lidge-jun/opencodex
Length of output: 50375
Make principal resolution enforce session validation
managementPrincipal at src/server/management-auth.ts:257-270 accepts any recorded session token without checking its origin or unsafe-method CSRF token. The current caller at src/server/index.ts:449-455 invokes requireManagementAuth first, but the exported function relies on this call order. A future dispatcher can therefore pass "gui-session" to src/server/management/sidebar-routes.ts:75 for a request that fails the session checks. Share credential, origin, and CSRF validation between both functions, or combine authentication with principal resolution. Add regression tests for mismatched origin and CSRF values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/management-auth.ts` around lines 251 - 271, Update
managementPrincipal to perform the same session credential, origin, and
unsafe-method CSRF validation enforced by requireManagementAuth before returning
"gui-session"; do not rely on callers invoking the gate first. Share the
validation logic between requireManagementAuth and managementPrincipal, or
consolidate authentication and principal resolution, while preserving
admin-token handling and adding regression coverage for mismatched origin and
CSRF values.
Source: Coding guidelines
Comment on lines
+92
to
+94
| for (const dir of RETIRED_TRACKED_DIRS) { | ||
| expect(ignore).toContain(`${dir}/`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tests/repo-hygiene.test.ts ---'
sed -n '1,125p' tests/repo-hygiene.test.ts
printf '%s\n' '--- .gitignore ---'
nl -ba .gitignore | sed -n '35,55p'
printf '%s\n' '--- active-rule probe ---'
python3 - <<'PY'
from pathlib import Path
ignore = Path(".gitignore").read_text()
raw_rules = [line.strip() for line in ignore.splitlines()]
active_rules = [
line for line in raw_rules
if line and not line.startswith("#")
]
print("substring_matches:", {
directory: f"{directory}/" in ignore
for directory in ("go",)
})
print("active_rule_matches:", {
directory: f"{directory}/" in active_rules
for directory in ("go",)
})
print("active_rules_near_go:", [
(index + 1, line)
for index, line in enumerate(raw_rules)
if "go/" in line
])
PYRepository: lidge-jun/opencodex
Length of output: 5309
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tests/repo-hygiene.test.ts ---'
sed -n '1,125p' tests/repo-hygiene.test.ts
printf '%s\n' '--- .gitignore ---'
nl -ba .gitignore | sed -n '35,55p'
printf '%s\n' '--- active-rule probe ---'
python3 - <<'PY'
from pathlib import Path
ignore = Path(".gitignore").read_text()
raw_rules = [line.strip() for line in ignore.splitlines()]
active_rules = [line for line in raw_rules if line and not line.startswith("#")]
print("substring_matches:", {"go": "go/" in ignore})
print("active_rule_matches:", {"go": "go/" in active_rules})
print("go_occurrences:", [
(index + 1, line)
for index, line in enumerate(raw_rules)
if "go/" in line
])
PYRepository: lidge-jun/opencodex
Length of output: 5303
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
ignore = Path(".gitignore").read_text()
lines = ignore.splitlines()
without_go_rule = "\n".join(
line for line in lines
if line.strip() != "go/"
)
active_rules = [
line.strip()
for line in without_go_rule.splitlines()
if line.strip() and not line.strip().startswith("#")
]
print("current_assertion_after_rule_removal:", "go/" in without_go_rule)
print("active_rule_assertion_after_rule_removal:", "go/" in active_rules)
print("active_rules_containing_go:", [rule for rule in active_rules if "go/" in rule])
PYRepository: lidge-jun/opencodex
Length of output: 279
Check only active .gitignore rules.
At tests/repo-hygiene.test.ts:92-94, toContain also matches comment text. Removing the active go/ rule still passes because comments contain go/. Filter out blank and comment lines before asserting the rule. Run bun run typecheck and bun run test after the change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/repo-hygiene.test.ts` around lines 92 - 94, Update the assertion loop
over RETIRED_TRACKED_DIRS in the repository hygiene test to remove blank lines
and comment lines from the parsed .gitignore entries before checking for each
rule, so toContain evaluates only active rules. Preserve the existing directory
rule assertions, then run bun run typecheck and bun run test.
Source: Coding guidelines
This was referenced Aug 3, 2026
This was referenced Aug 3, 2026
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
1 participant
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
Post-merge audit of the #892 campaign, now on
dev. Two independent adversarialreviewers (
gpt-5.6-sol, medium, priority) audited the 165-file merged delta ondisjoint scopes: memory/security boundaries, and provider wire semantics. The
wire audit passed in one round. The security audit took four, and found a real
consent bypass plus one relay defect. Every fix below was driven red first.
Fixes
Agent star-consent bypass (two distinct holes)
POST /api/github/starrefuses agent callers so the CLI's "ask the user"deferral cannot be answered with one
curl. Both halves of that guard werebroken:
Originplus the GUI-origin and CSRF headers, on the stated premise that
requireManagementAuthhad already matched them against a minted session. Ithad not — the gate accepts a raw admin token and returns before it consults
the session table. Anything that can read the admin token (every process
running as the user) could send three headers of its choosing and star the
repository with the user's GitHub identity.
isAgentDriven()wastrue, and that function reads the server's environment, not the caller's.
A proxy running as a service — no agent markers, the ordinary remote setup —
accepted a raw-token star from anyone. The pre-existing "a hand-typed run is
not blocked" test encoded exactly that hole.
managementPrincipal()now resolves which credential passed the gate, from thesame session table and CSRF comparison the gate uses, and the route requires a
minted GUI session unconditionally.
Honest limit, now written into the code and
AGENTS.md: a process runningas the user can mint its own session from the loopback dashboard bootstrap, and
can run
gh api -X PUT /user/starred/...without the proxy at all. No in-processcheck distinguishes it from the browser. This guard removes the casual path and
the raw-token path; the real boundary is the normative rule in
AGENTS.md. Thereviewer proposed removing server-side starring entirely and withdrew it once
that reasoning was on the record — it would not close the
ghpath and wouldbreak the dashboard button.
Eager relay
abort promise, retaining a reaction per chunk — the exact class
relay.tsdocuments avoiding at its own drain. Abort now cancels the reader instead.
examined, so a terminal frame arriving in the same tick as the drain deadline
was discarded and the turn was accounted as a cancel. The chunk is inspected
first now.
Retired Go file on
devgo/internal/cli/config_parity.go(682 lines) was committed by a broadgit addthree times during the #820 campaign. Two were caught; the third rode into the
#892 merge, so it is currently the only tracked file under
go/— absent frommain,preview, andv2.10.0. Untracked, with a mechanical guard next to the.codexclaw/gitlink invariants intests/repo-hygiene.test.ts.Tests that could not fail
Four guards were mutation-surviving and are now pinned, each driven red by
re-introducing the defect:
bounded-bodymaxBytesbounded-bodyaccumulatorUint8Array[](never touches the growth counter, so it is pinned structurally)MAX_CANONICAL_DEPTHschedulerVerificationMaySettlenativeServiceAbsentguard (the e2e fixture's final clause was already false)tests/windows-secret-acl.test.tsalso inheritedUSERNAMEfrom an earlierblock's
??=; it owns and restores its own environment now.Verification
bun run test: 7,512 pass, 8 skip, 0 fail (500 files)bun x tsc --noEmit: passbun run privacy:scan: passCarried to a follow-up unit
OPENCODEX_HOME(ownership/modevalidation, open-then-
fstat). Under the default~/.opencodexat0700theattacker who could win the race already has write access as the user; the
concrete lesser-privileged case is a same-UID sandboxed workspace.
unfixed vulnerability.
Review note
This touches the consent boundary and the management auth gate, so it wants the
security review
MAINTAINERS.mdasks for rather than a direct push todev.Summary by CodeRabbit
Security
Reliability
Maintenance