Repository navigation
Remove Team Token PR1: User session, live authorization, and aggregate all-team access - #334
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
josue-tech-94
changed the title
|
Took a deep pass through this one, split across the access-authorization core, the catalog/pagination subsystem, persistence/SQL, auth/session + edge, directory/routes/rate-limit/CodeQL, a full security sweep, and a contract-consistency trace. The security envelope is genuinely well built (fail-closed and deny-by-default hold up consistently), so the interesting stuff is concentrated in the new aggregate-catalog subsystem and one migration side effect. Most of the blocking items are cheap to fix and worth doing now precisely because PRs 2-4 build directly on this foundation. Verdict🔴 Not ready to approve
Heads up: this is my read from the review, not a formal GH approval. Must resolve before approval
Optional (non-blocking)The PR can merge without these; they're follow-ups or author's-call. The two cross-PR notes (L11, L12) are really for PR2 to build against.
What checked out clean
|
alfredolopez80
left a comment
alfredolopez80
left a comment
There was a problem hiding this comment.
Security scope consolidation — F1 and F3
This PR appears to address both F1 (stale team-admin authorization) and F3 (password changes or resets not invalidating legacy sessions).
I would keep both findings in this PR's acceptance criteria and verify them with end-to-end regression coverage before treating them as resolved. The inline comments below document the required evidence for the fix.
|
@zach88 We’ve finished the correction/synchronization rounds from your review and would appreciate a final pass on the current exact head:
Your original four must-resolve findings have been addressed:
The subsequent review rounds also closed the legacy V1/admin/password-race findings, final lifecycle revalidation, structured logging/public error findings, release-coordinate issue, and the latest T2 public-boundary CI regression. Current exact-head ordinary CI and CodeQL are green, including Real PostgreSQL and the T2 contract lane. The PR body has also been updated to current exact-head truth. Could you please do a final review of the current head and call out anything you still consider merge-/PR2-base-blocking? The PR remains intentionally an integration base and must not merge to The optional/non-blocking items from your earlier review remain tracked separately rather than being silently discarded. |
alfredolopez80
left a comment
alfredolopez80
left a comment
There was a problem hiding this comment.
Supplemental security review — F1/F3 remain partially resolved
@josue-tech-94, thank you for the exact-head F1/F3 coverage. The additional security fan-out found the residual paths documented inline below. The existing F1/F3 test comments remain valid, but the green CI result does not cover these migration, final-authority, or derived-credential interleavings. I recommend retaining High/P0 until these paths are either fenced or explicitly covered by cross-service PostgreSQL tests.
Additional residuals not attached inline because GitHub could not resolve their source lines in the current diff: downstream GFS credentials are not bound to the password/session security epoch, and sibling pending password-reset invitations are not explicitly superseded. Please cover those two capabilities with cross-service/contract tests as well.
|
@josue-tech-94 — Non-blocking follow-up observations The items below are separate hardening and regression-coverage follow-ups. They are intentionally non-blocking for the current F1/F3 review and should not be read as additional merge blockers:
Recommended follow-up: track these as separate hardening items and cover them with focused PostgreSQL interleaving or contract tests. |
|
@claude Please do one final independent review of the FULL current PR before we continue with the stacked PR2 work: Review only — do not make code changes. Current expected state:
If the live head/base have moved, review the actual current state and report that rather than relying on the SHAs above. Please review the complete PR diff for:
Please also independently verify the dispositions of the four currently
Please challenge any of those classifications if the current code/base evidence does not support them. For any changed paths under Please distinguish clearly:
Do not turn later-stack work into a PR1 blocker unless PR1 itself violates the contract it claims to establish. At the end, give a clear answer to both:
Also reiterate that "safe as PR2 base" does NOT mean PR #334 is ready to merge into |
|
Claude finished @josue-tech-94's task in 4m 31s —— View job Final independent review — PR #334 (PR1 of 4)I reviewed the actual current state, not just the quoted SHAs. Live head/base match the expected values: head 1. The four original blockers — all independently CONFIRMED closed
2. The four Alfredo threads — independently checked① ② ③ ④ 3. Other observations (all non-blocking)
Answers to your two questions1. Are there any PR1 must-fix findings remaining? 2. Is the current exact PR1 head safe to use as the PR2 stack base?
Review only — no code changes were made. This is my analysis, not a formal GitHub approval. |
Independent review — PR #334 @
|
| Prior claim | Verdict |
|---|---|
Thread ② is mitigated because "rpcDelegationV2 is off" |
Incorrect. That flag appears only in userAccessPolicy.ts; the legacy /external/rpc/token handler (auth.ts:530) never consults it. The read-then-sign window is live in PR1 today, not dormant. The deferral to PR2 is still coherent — PR1 claims live authorization, not atomic issuance binding — but the comfort offered alongside it does not hold. |
valid_after IS NULL is a structural fail-open for SSO/passwordless users |
Literally true, operationally inert. revokeAllUserSessions writes the epoch row via INSERT … ON CONFLICT DO UPDATE GREATEST(…) in the same transaction that sets password_set_at = NOW(), across all five password writers. NULL therefore means "this user has never had a security event", not "the fence was skipped". Three independent confirmations. I would not carry this forward as an open item. |
| "No pinned deterministic ordering regression — randomized coverage only" | Incorrect. catalogProducers.realPostgres.integration.test.ts:98-100 pins an explicit value set (alpha, Zeta, a_b, a-b, é, é, Ω, 😀) with fixed seed 106_026. |
The v2 branch omits lifecycle_version |
True but unreachable. lifecycle_version is incremented in exactly one statement repo-wide (users.ts:575), always together with lifecycle_state='retired'; no reactivation path exists, so the state check subsumes the version check. Worth pinning as an invariant comment — the day someone adds reactivation with a version bump, the v2 branch opens silently. |
Medium — worth tracking, not merge-blocking
502/504diverge between the two sanitizers in this same PR.app.ts:142-154special-cases only 503, so 502/504 fall through to500 internal_error retryable:false, whilegfs.ts:149-156returns502 upstream_unavailable retryable:truefor the identical error. A client honouringretryablewill not retry a transient outage on the access/members/auth surface.Verify production CodeQL source scopeasserts 3 of 14 services.git ls-files '*/src/main.ts'returns 14; the step checkscontrol-api,external-rest-api,desktop-app. The workflow's own header names rpc-proxy's JWT chain, webhook-gateway's HMAC verification and HCC's NetworkPolicy generation as the reason the gate exists — none of the three is asserted. Deriving the list fromgit ls-fileswould close it.- The PR body's CodeQL claim is incomplete. It says the modeling recognises the rate-limit middleware "while preserving the stock security-and-quality suite". In fact the suite sets
disable-default-queries: trueand excludes two stock queries —js/missing-rate-limitingandjs/user-controlled-bypass— replacing both with custom queries that reuse the stock@id. Consequence worth knowing: the Security tab cannot distinguish stock-produced from custom-produced alerts, and the 18 dismissals dated 2026-08-12 now attach to alerts evaluated by different logic. The queries themselves look sound; this is a disclosure gap, and PR-description accuracy was explicitly in scope. - Deploy window. 35
CREATE INDEXwith zeroCONCURRENTLY(24 of them on hot pre-existing tables:workflow_runs,notification_deliveries,team_members,gfs_grants,gfs_shares, the workflow-trigger tables), plus 36 trigger operations at ACCESS EXCLUSIVE and a full-table backfill — all insideinitDb's singleBEGIN…COMMIT, so locks accumulate until one final commit. The transactional runner is pre-existing (verified at base), but the volume inside it is introduced here, andCONCURRENTLYis illegal in a transaction block, so this cannot be fixed without anonTransactionalmigration path. At minimum, measure index build time against a production dump and document a maintenance window. - R1-H3's hot row narrows rather than disappears.
authorization_team_revisions(team_id)is touched by every workflow phase transition, every notification delivery and every GFS grant/share write for that team, viaINSERT … ON CONFLICT DO UPDATE SET revision = revision + 1holding a row lock to commit. On a mono-tenant or large-shared-team install this is functionally the original problem at a different granularity. Also:469-470bumpsteam_idandusage_team_idwithout dedup — two updates to the same row in one statement when they are equal. - Latent deadlock.
authorization_bump_gfs_resource_subjects(userAccessFoundationSchema.ts:587-607) iterates aUNIONwith noORDER BYwhile taking row locks on the revision tables. Two concurrent GFS resource updates with overlapping subjects in opposite plan order can deadlock. One-line fix:ORDER BY 1, 2. - Silent authority-fingerprint freeze.
environment_idis pinned once at migration time but recomputed live at runtime, andcanonicalEnvironmentId()falls back silently todevelopment/local-clusterwhen the tracing env vars are absent. If they later diverge, the LEFT JOIN never matches andCOALESCE(revision, 1)returns a constant — the fingerprint that should detect an authority change freezes, with no metric and no error. Comparing the pinned value against the runtime value atassertDbReadyand failing loudly would close it. - Fabricated payload crossing a service boundary.
external-rest-api/src/routes/members.ts:165-174replaces an over-long name with'x'.repeat(121)(and over-long teams with 51nulls) and forwards that to control-api to provokeinvalid_name/too_many_teams. It works only because both limits are 120/50 today; raise control-api's limit and an invitation gets persisted with a destroyed name and a 201 where a 400 was intended. Blame:00469ac5e,89361daa1— both commits in this PR. x-request-idecho lost on the grants path. The 429 test was relaxed fromtoBe(REQUEST_ID)totoMatch(UUID_RE)becausecorrelationIdfalls back torandomUUID(); the/me/gfs/sharestest ~80 lines later still asserts the echo. Two adjacent endpoints, two contracts, both pinned green — andUUID_REcan no longer detect the loss.- Final-transaction revalidation covers 6 of 10 privileged mutations. Missing: the four invitation mutations and — most notably —
DELETE /external/members/:userId(full user retirement). Those four functions do not accept theauthorityparameter at all. Mitigating: they do lock and revalidate operator lifecycle viagetManagerRolesForTeams(..., lock=true); what they skip is session state (revocations, security epoch,revoked_at). Mechanical to wire. - The CodeQL model certifies as rate-limited six routes whose limiter fails open.
rateLimitMiddlewareallows the request uncounted whengetBucketKeyreturns null; the query constrains only the callee, never the key. Sites:decisions.routes.ts:64,103,users.ts:129,376,notifications.routes.ts:114,364. Latent — all six currently sit behind auth middleware. Worth noting that PR1's own new helper does this correctly, returning a':missing'sentinel that is deliberately counted; the six legacy sites keep the null pattern while CodeQL is taught to trust all of them identically. deploy/base/control-plane/configmaps.yaml:21setsCONTROL_API_OPERATIONAL_ACCESS_INDEXER_ENABLED: 'true'whileconfig.ts:1012defaults itfalse. The key did not exist at base. This is not a prose violation — the indexer is deliberately absent from the disabled-by-default list and the configmap comment says so — but it means the watch-driven reconcile loop starts in every deployed environment on merge, with new cluster-widewatchRBAC. Worth stating explicitly, since a reader checkingconfig.tsalone would conclude the opposite.
Low
- Three bare
catch {}inexternalSessionAuth.ts:122,176,244(R1-M1, still open). Behaviour is fail-closed, but the 503 carriesretryable: true, so a deterministic programming bug becomes a client retry loop with zero telemetry — the file emits noreq.logat all. - R1-M4 still open:
CATALOG_FAMILIESduplicated verbatim inuserAccessPolicy.ts:5andcatalogContracts.ts:8, each with its own type. A 13th family added to one desyncs silently. - R1-L7 still open:
config.sessionJwtPrivateKeyreused as the HMAC secret in four places. - R1-L4 partially open: two of three skip paths in
accessCatalogShadow.ts:119,124still emit no comparison metric. - R1-L1 confirmed but low impact:
errorHandler.ts:252-268still returns the legacy envelope on the terminal 500 for/api/v1/external. Verified no public client observes it —grep -rn 'api/v1/external'across desktop-app, mcp-host, profile-ui and rpc-proxy returns nothing, and external-rest-api re-wraps it. isCurrentExternalSession/assertCurrentExternalSession(externalSessionAuth.ts:36-57) are now caller-less after PR1 replaced their use, and they check less than the canonical boundary (no epoch, no revocation). A future route adopting them by name would accept logged-out tokens. Safe to delete.- A vacuous assertion in the F1 suite:
expect(staleAdminToken).not.toBe('same-valid-v1-token-minted-while-admin')compares a signed JWT against prose — it cannot fail. codeql/javascript-queriespinned at2.4.3with no dependabot/renovate entry, which defeats the weekly cron's stated purpose of letting newly published stock queries reach the default branch.- The
POST /external/teams/:teamId/invitationsroute does not usenormalizeInvitationEmail, unlike its sibling — pre-existing, but inconsistent with the bounded validator this PR introduces.
What I verified as genuinely sound
Stated with evidence, not assumed:
- All four original blockers are closed, and the fixes are load-bearing.
compareCatalogKeycompares fields sequentially rather than a serialization, so it is structurally equivalent to a multi-column SQLORDER BY— the field-boundary counterexample cannot exist, and 200,000 sampled pairs produced zero divergences against an independent oracle. The R1-H2 property test was mutation-tested: revertingcompareCatalogTextto the originallocaleComparediverges on 1.59% of generated pairs, giving a ~1e-7 chance the 1000-run test misses it. That test is real coverage, not decoration. H1 and H3 likewise verified, including that no equivalent global hot row was reintroduced (the residual per-team row is item 5 above, a different granularity). - No
localeComparesurvives on the pagination boundary. I checked the residual bare.sort()calls inaccessCatalogMerge.ts:124,203andcatalogProducerHead.ts:168specifically — they sortvalidUntil(ISO-8601, ASCII), notlogicalId, which usescompareCatalogText. Not a finding. - The CodeQL recognition predicate is handler-level, not file-level. It matches a resolved import path (unspoofable by a local shadow), and the safety property is a universal quantifier:
not exists(RouteSetup | covered and not hasLimiter). Adding one rate-limited route does not silence its unprotected siblings — and theincompleteParentRouterfixture pins exactly that scenario as still flagged.paths-ignoreis a single glob matching 12 files, all fixtures, zero production paths. Detection survives the model change in both directions, andcodeql test runis now a CI step, so drift either way fails the build. - 27 alerts → 0 open. Only two results are still produced at this head, both defensible: the
js/insufficient-password-hashalert atoperationalAccessProjection.ts:143is a server-keyed HMAC change-detection fingerprint (createHmac('sha256', key)), not password storage; the other is on a test file. The ReDoS and unused-variable alerts are resolved by code — neither query is touched by the custom suite, so that attribution is sound by construction. - Tests execute and prove what is claimed.
tsc --noEmitclean on both services (exit 0). control-api: 4856 passed, 182 skipped (DSN-gated), real exit 0 — captured without a pipe. All four headline realPostgres suites are collected by thecontrol-api-migrationlane (CI job98594577414: 34 files / 181 tests / 0 skipped) and use real routers, real auth middleware and real Postgres, with deterministicpg_blocking_pids()transaction barriers rather than sleeps. A specific production line was identified whose revert would turn each one RED. - Zero-tests-is-never-success is properly guarded — the lane asserts
Test Files [1-9]…passedandTests [1-9]…passedand fails on any skipped count, which is stricter than most. - Thread ④ is pre-existing and not widened — verified four ways: the function body is character-identical at base, the same two call sites, the same 48h TTL, the same requester guard. The one new
AND purpose <> 'password_reset'is a narrowing. Leaving it open is the right call. - Alfredo's follow-up fix+docs: onboarding dry-run blockers, README rewrite, and Diátaxis docs tree #1 is already fixed —
revokeAllUserSessionstakesSELECT id FROM users … FOR UPDATEbefore computing the cutoff, andcreateUserSessiontakes the same lock, so a concurrent exchange serializes. - F1 is structurally satisfied —
requireExternalRolereadsreq.externalTeamAuth.role, populated only bygetLiveTeamMembership; at base it read the token claim.grepconfirms exactly threerequireExternalRole(['admin'])routes exist and all three are covered by the regression suite. (The suite is a hand-maintained list rather than derived fromrouter.stack, so it has no drift resistance — a follow-up, not a gap today.) - No SQL injection, correct transaction boundaries throughout (no
pool.querymixed into a client transaction), statement accounting not bypassable on the catalog path, and the operational index cannot become effective authority —requireCondition(readiness.snapshot === 'current')throws rather than degrading permissively. - The v2 no-authority claim holds — 14 forbidden claims rejected at the external boundary, and every consumer of
.teamId/.rolere-verifies against live membership. - The public boundary gate cannot produce a false green —
test-minikube-t2-public-boundary.shruns underset -euo pipefailwith no|| true, no masking pipe, no silent skip, and is mutation-verified bytest-minikube-t2-contract.sh, which asserts it fails on an unresolved base, on a secret in an untracked file, and on materialized credentials. - No dependency changes — the only manifest/lock edits are version bumps; nothing added, no new postinstall.
Method and limits
Eight agents in parallel over a dedicated worktree at the exact head: access core, auth/session, directory/teams, persistence/migrations, cross-service contract, CodeQL/CI, test quality, and an adversarial disposition auditor whose only job was to try to break the consensus. Findings that two agents reached independently by different routes are marked as such above (the sanitizer, reached once via contract reading and once via execution).
Honest limits: CodeQL was not executed locally, so the "fails on a fresh violation" half of the guard standard is established structurally from the checked-in .expected fixtures rather than by a run. No Postgres 16 was available, so the 34 realPostgres suites were judged from source plus the real CI job log — no mutation testing was performed on them, and the named revert lines are traced, not executed. Lock levels and the UNION deadlock are derived from the SQL plus documented Postgres semantics, not reproduced. Five of the seven "disabled by default" flags were not independently re-read by anyone in this round.
Review only — no code changes, and this is analysis rather than a formal GitHub approval.
Addendum to the review above — one Critical finding, reproducedThe access-core pass on [Critical] The
|
|
@alfredolopez80 done! The F3/D3 stop was handled, and the correction set is now published at exact head d3ba05b:
The correction review also found one same-round regression, R11-B2, where the real External REST client discarded the approved 413 metadata before sanitization. That is separately closed in d3ba05b with a status-aware safe extraction boundary. Fresh exact-head validation is green: The repeated collateral pass is clean, and the fresh formal round-11 review returned No objection with no PR1 must-fix findings remaining. Exact head d3ba05b is now accepted as safe as the PR2 stack base. As before, that does not mean PR #334 is ready to merge into dev; it remains the integration base until PRs 2–4 and final stack reconciliation are complete. |
Independent review — PR #334 @
|
| Closure | Commit | Verdict |
|---|---|---|
| R11-H1 — public GFS error contract | 921efe14 |
✅ CLOSED. All five statuses now produce correct typed public codes (408→request_timeout, 411→length_required, 413→payload_too_large, 425→too_early, 507→insufficient_storage), no upstream detail leaks, and an upstream-spoofed code is replaced by the canonical one in all five. 5 of 6 mutations turn real tests red. |
| R11-B2 — REST client regression | d3ba05b0 |
✅ CLOSED END-TO-END. The boundary is 413-specific, but retry-after already crosses unconditionally via SAFE_RATE_LIMIT_RESPONSE_HEADERS (controlApiClient.ts:34-39), and 411/507 carry no approved metadata. Verified by driving the real client against simulated responses for all five statuses. |
| R11-B1 — team-GFS production admission | 82de1421 |
✅ CLOSED. The 3 production callers that need it propagate the limit; the 4th (indexer) is legitimately exempt — its budget only covers k8s object paging. No path leaves null read as "budget exhausted". 4 of 5 mutations red. And "fails before rollout traffic" is literal: it is a module-load fail-fast, not a first-request error. |
| R5-L3 — accepted onboarding after expiry | 61aaa8d8 |
✅ CLOSED AND BOUNDED. Not "never expires": it requires four simultaneous conditions (accepted + accepted_at <= expires_at + accepted_user_id === userId + null password_hash, which makes it single-use) plus an authenticated session belonging to the user. The token path keeps its unconditional expiry check. |
The guardian tests that previously blessed the bug were restored: routes.gfs.test.ts:600-610 went from expect(...).toBe('internal_error') to toEqual({code:'payload_too_large', ...}).
Mutation table for R11-H1 (each restored between runs, working tree verified clean):
| Mutation | Exit | Red tests |
|---|---|---|
Remove the 5 statuses from PUBLIC_CODE_BY_STATUS |
1 | 1 |
Revert retryable to [429,502,503,504] |
1 | 3 |
Remove the status === 413 branch from the sanitizer |
1 | 3 |
Remove the 413 block from controlApiClient.ts |
1 | 3 |
Emit retry-after on every status |
1 | 1 |
Remove the 5 entries from ALLOWED_CODES_BY_STATUS |
0 | 0 ← see Low findings |
2. Corrections to my own previous review
Three things I said in the c7735cd5 round need fixing, and they are mine to fix:
- I overstated the severity of R5-L3. I claimed "user with no self-recovery". That is wrong:
POST /external/auth/password-reset/requestis unauthenticated and issues apassword_resetfor anyactiveuser without requiring a priorpassword_hash(membership.ts:1017-1043). That predates the fix. The fix is a genuine UX improvement, not the closing of an absolute dead end. - My reasoning about
valid_after IS NULLwas wrong, even though the conclusion was right. It is not inert becauserevokeAllUserSessionswrites the epoch in the same transaction aspassword_set_at—revokeAllUserSessions(userSessionService.ts:398-409) does not look atpassword_set_atat all. It is inert because the epoch is only ever born from a revocation event, so NULL means "no event yet". - Three fresh suspicions of mine this round turned out to be false, and I am recording them rather than publishing them as findings: the 26-second
external-rest-apiCI job does run all 215 tests (36/36 files, 0 skipped); the deployment does not break without the new env var (default isoff); and the*.realPostgres.integration.test.tsfiles are not orphans — they run unconditionally inci-public.yml:150under a triple fail-loud guard that also fails if anyskippedappears.
3. Highest-value new finding: part of the green CI is manufactured by retries
control-api/vitest.config.ts:14 — REPRODUCED
retry: process.env.CONTROL_API_REAL_PG_ADMIN_URL ? 0 : 2
On the untouched tree:
npm test → EXIT=0 4871 passed
npx vitest run --retry=0 → EXIT=1 2 failed | 4865 passed
npm test with a real PG DSN → EXIT=1 2 failed | 5050 passed
The CI matrix lane runs npm test, so CI never sees these two reds.
One of them is a regression introduced by this PR. test/routes.profile.test.ts:768 is byte-identical to the merge-base, but faa41635 migrated console.warn → requestLogger(req).warn in src/routes/external/users.ts:318 while leaving the twin warn in src/routes/external/teams.ts:139,182 untouched (0 lines changed there). The spy counts 1 instead of 2. The false-green mechanism is worth naming: the assertion fails before warn.mockRestore(), so the spy survives into attempt 2 with accumulated history — 1 + 1 = 2 — and passes.
The second (test/routes.externalMembers.test.ts:135, added in 2cd02c9e) is cross-test contamination: vi.clearAllMocks() clears calls but does not drain the mockResolvedValueOnce queue, so the next test consumes a stale value.
Suggested fix: drop retry to 0 on the ordinary lane and fix both tests. A harness that reports green over deterministic reds is indistinguishable from one that was deleted.
4. Other High findings
Derived RPC tokens live 300s with no downstream revocation check — middleware/rpcAccessAuth.ts:21,44 (REPRODUCED). The middleware verifies signature, typ and scopes only; it never consults external_user_sessions, the security epoch, or external_v1_session_revocations. All five exported functions are synchronous, with no pool/db access. TTL is config.rpcTokenTtlSeconds, default 300 (config.ts:713).
This reframes the open P1 thread on auth.ts:608. Atomic fencing removes the race, but the 300 seconds of post-revocation access remain in every case, race or no race. And the stated mitigation does not exist: grep -rn "rpcDelegationV2" control-api/src/routes control-api/src/middleware returns empty — the flag only lives inside userAccessPolicy.ts. If this is deferred to PR2, the accepted exposure should be documented as 300 seconds, not "the race window", and the flag mitigation should be withdrawn.
Final session authority covers 3 of 7 directory mutations — membership.ts:1995,2229,2258,2314 (structural precondition CONFIRMED; exploitation chain READ-ONLY). There are exactly three production call sites of validateExternalSessionAuthorityContext: teams.ts:114, membership.ts:2052, :2129. Meanwhile:
resendManagedInvitationForUser(managerUserId: string, invitationId: string) ← no authority param
revokeManagedInvitationForUser(managerUserId: string, invitationId: string) ← no authority param
Invite, resend, revoke-invitation and retire-user cannot revalidate the authority context because they never receive it; they only recheck lifecycle_state through the JOIN users in getManagerRolesForTeams, which revocation does not touch. Under lock contention, a request admitted before a revoke-all would block on the same users row, re-read in READ COMMITTED after the lock is released, see lifecycle_state='active', skip the epoch, and create the invitation — possibly with role admin.
Falsifiable in one line: add the team invitation mutation to mutations() in routes.externalTeamsFinalAuthority.realPostgres.integration.test.ts:213-273. denies … when source-session invalidation wins after admission (:353) should fail. That settles it either way faster than discussion.
Consumed password_reset capabilities do not supersede older ones, and nothing can enumerate them — membership.ts:1035,1389,1396-1408,1123 (READ-ONLY). The endpoint is unauthenticated, 5/min per email, TTL 48h ⇒ ceiling of ~14,400 simultaneously valid tokens against a third-party address, mintable without any credential. The "already has a password" guard is explicitly exempted for reset (if (invitation.purpose !== 'password_reset' && user.password_hash), :1389), so an old link still works after a legitimate recovery, sets the attacker's password, and revokeAllUserSessions then evicts the victim. updateUserPassword does not invalidate them either.
Two asymmetries inside this repo suggest defect rather than design: the control-admin flow does supersede (adminAuthService.ts:461-467, plus a partial unique index at db.ts:2047-2049), and all three admin listing surfaces filter purpose = 'member_invitation' (:1107, :1123, :1944), so an incident responder cannot even list a live reset link — only wait 48h.
On the "pre-existing relative to the merge base" defence: correct on the strict point — requestProfilePasswordReset is byte-identical to the merge base — but not correct about the function. This PR substantially rewrites setInvitationPasswordForEmail (adds a mandatory invitationToken, switches to getInvitationRecordByToken(…, {lock:true}), replaces the unconditional UPDATE with a status CAS, adds revokeAllUserSessions). If the argument rests on "I did not touch that function", it does not hold.
The new error envelope breaks profile-ui — external-rest-api/src/app.ts:127-154 ↔ profile-ui/lib/api.ts:130 (REPRODUCED, found independently by two agents). The shape changed from {error: "<string>", message: "<string>"} to {error: {code, message}}, but the client does String(parsed.message || parsed.error || text), which yields "[object Object]":
BASE 500 | detail = "Control API POST /external/members/invitations failed (503): member_registration_unavailable" | friendly message fired: YES
HEAD 500 | detail = "[object Object]" | friendly message fired: no
Both detail.includes(...) recovery messages at lib/api.ts:138,143 become unreachable, and the comment at :134-137 documenting the contract they depend on is now stale. profile-ui/lib/api.ts is not touched by this PR.
The sanitizer rewrites control-api's code and always discards its message — publicApiError.ts:157-159,223 + routes/access.ts:10 (REPRODUCED). ALLOWED_CODES_BY_STATUS[409] omits invalid_request, so Catalog access changed; refresh from the first page. reaches the client as conflict / The request conflicts with current state. — the actionable recovery instruction is erased, on the surface this PR introduces.
/rpc/token stops relaying desktop_requires_team, and the guardian test was rewritten to bless it — routes/rpc.ts:6,26 ↔ publicApiError.ts:77-84. The reason is absent from SAFE_DOMAIN_REASONS, so it collapses to forbidden. The deleted comment said the relay existed precisely so "the desktop app and logs can distinguish 'needs a team' from a genuine auth failure". The test that protected it was renamed from "relays the control-api denial reason instead of a generic 403" to "returns a bounded public envelope without reflecting the control-api denial body", with its fixture changed from desktop_requires_team — a legitimate, allowlistable reason — to secret-internal-reason, which reads like a leak. This is the same pattern that was a must-fix as R11-H1, in a different endpoint. Suggested fix: add desktop_requires_team and no_permitted_scopes to SAFE_DOMAIN_REASONS, and restore a test asserting that reason does reach the client, alongside the no-leak one.
426 is unexpressible, and its trigger has no producer (two agents converging from different angles): ACCESS_PUBLIC_STATUSES (routes/access.ts:10) omits 426, so a deliberate upgrade_required degrades to invalid_request (rg -n "426" external-rest-api/src → zero hits). Separately, x-evenfire-client-version has four readers in control-api and zero producers anywhere in the repo, so enabling CONTROL_API_USER_ACCESS_MINIMUM_CLIENT_ENFORCEMENT would return 426 to every external user regardless of their actual version.
members/invite fabricates a sentinel payload — external-rest-api/src/routes/members.ts:166-173 (REPRODUCED). Instead of returning 400 at the edge, it replaces the user's name with 'x'.repeat(121) and their team list with 51 nulls and forwards that upstream so control-api rejects it:
input name : "Ana Ana Ana …" len=123
SENT upstream name : "xxxxxxxx…" len=121
SENT upstream teams: 51 entries, all = null
user payload survives? false
git blame: 00469ac5e (name) and 89361daa1 (teams), both inside this PR. This is a placeholder shipped as the final implementation, and production code shaped to provoke the response a test expects. It also burns the user's member_mutation bucket for an error the edge already rejected for free, and if the two limits ever diverge (both 120/50 today) the invitation is created with the fabricated name.
Enabling the catalog rollout with one variable kills control-api at import — userAccessPolicy.ts:332,389 (REPRODUCED). configuredUserAccessIntent is a module-level const, so setting CONTROL_API_USER_ACCESS_CATALOG_MODE: 'shadow' without its partner CONTROL_API_USER_ACCESS_TEAM_GFS_MEMBERSHIP_ADMISSION_LIMIT throws during module load → CrashLoopBackOff. grep -rn 'USER_ACCESS|CATALOG_MODE|ADMISSION' deploy/ returns nothing, and the PR touches no docs/ file at all. To be clear: today's deployment does not break (default is off); the defect is an undeclared knob with a one-step trap.
Three defects make serve/shadow impractical today: everyCatalogFamilyReady (userAccessPolicy.ts:183) demands all 12 families at once, so a single non-current K8s source — a routine watch close — returns 503 across all of /external/access, including the six families that need no K8s source at all (and makes the whole partialErrors machinery unreachable in those modes); the indexer has a hard, non-configurable ceiling of 1,000 objects per family (operationalAccessIndexer.ts:145); and it starts in every replica with no leader election (main.ts:117), producing mutual relist livelock. In fairness on that last one: the fencing itself is correct — beginRelist is atomic and stageRelistPage/promoteRelist verify generation under FOR UPDATE. The defect is multi-writer, not corruption.
5. Verdicts on the open inline threads
| Thread | Your reply | Verdict |
|---|---|---|
userAccessFoundationSchema.ts:44 — legacy epoch backfill |
"closed by additive migration 0106" | ✅ Correct. The backfill is wired (db.ts:5968). A real gap remains for legacy users (password_set_at is ADD COLUMN IF NOT EXISTS, and the backfill filters IS NOT NULL), but it is inert because of the 12h v1 TTL, not because of the backfill. Worth stating that way. |
auth.ts:608 — atomic RPC fencing |
"intentionally accepted risk for PR2" | |
teams.ts:119 — revalidate operator lifecycle |
"closed under R9-H2" | ✅ Real closure, not nominal. Same transaction, same db client, SELECT … FOR UPDATE on both users and team_members, serialised against revokeAllUserSessions, which takes the same lock. Real concurrency test in CI. What remains open is scope, not mechanism — see above. |
membership.ts:1419 — supersede older resets |
"pre-existing relative to the merge base" |
6. What I verified and found clean
- Authorization core: decision precedence is correct; no fail-open anywhere in the new code (full inventory of
catchin the decision chain — all deny with 503/400 or rethrow); multi-tenant isolation correct across all 43 queries reviewed; stable ordering in the producer merge (REPRODUCED). The only production?? trueis fail-closed (event.authorityRequired ?? true). - This PR removes four pre-existing silent fallbacks in
teamService.ts(catch { return null }→ typed 403/404 handling withthrowfor everything else). Net improvement, worth recording. - Session token v2: explicit
algorithms:['RS256'], key passed as a publicKeyObjectrather than a PEM string (so the classic algorithm-confusion vector is unavailable even if the allowlist were relaxed), strict claim allowlist, disjoint audiences. No v1→v2 downgrade path exists. - Revocation epoch: the
iat > valid_afterbound is strict and fail-closed (a token stamped at the exact revocation instant dies), identical across all four readers. - Rate limiting: the limiter key is not client-controllable (
x-evenfire-client-ipis honoured only after the internal-service token is validated); login, reset and invitation are all covered; and it fails closed if the store is down. - CodeQL is not weakened.
disable-default-queries: truere-imports the full standard suite; two ids are excluded but replaced by in-repo variants with the same@idand severity, and the custom bypass query re-widens coverage with three predicates that report cases the stock early-abort guard hid.paths-ignoreonly excludes the deliberately-vulnerable test fixtures, and a new step fails if those fixtures enter the production database. - Zero placeholders, zero unrequested fallbacks, zero logging-standard violations in
control-api(the fourconsole.*are inmain.ts, bootstrap with no logger available — the caseCLAUDE.mdexplicitly permits). - The "test rewritten to bless the bug" pattern does not repeat in
control-api: systematic sweep of 781 blocks, 19 candidates, all 19 false positives. - No new dependencies and no third-party version bumps — all four manifests only change their own
versionfield. utils/auth/googleAuth.tsnow rejects an id-token whenpayload.email_verified !== true. New relative to the merge base, and a real security improvement.
7. Recommendation
This is not ready to merge into dev, which matches your own position that #334 stays the stack integration base until PRs 2–4 land. With that framing:
Before enabling CATALOG_MODE anywhere: declare both paired variables in the ConfigMap with a comment recording the pairing, gate serveCatalog per family, separate indexer limits from HTTP-request limits, and add leader election.
Worth fixing in this PR (live regressions, independent of the rollout): the retry: 2 masking plus the two red tests, the profile-ui envelope break, desktop_requires_team and its rewritten guardian test, and the members/invite sentinel payload.
Your call, not mine: whether the 300-second post-revocation window is accepted for PR2 now that it is quantified; and whether the public vocabulary gap (426, invalid_request on 409, upstream message propagation) is widened here or in PR2. For the session-authority scope finding, I would ask for the one-line test extension before debating severity — it confirms or refutes it in a single commit.
Full consolidated report with per-agent detail, reproduction commands and their output is kept locally; happy to paste any specific section into a thread.
|
@claude Please perform a complete independent re-review of the FULL current PR. Review only — do not make code changes. Current expected state:
If the live head/base have moved, review the actual current state and report the live SHAs rather than relying on the values above. This is not a delta-only pass and not a recap of prior reviews. The last Required deliverable 1 — historical-findings closure ledgerBuild an exhaustive ledger of every finding, residual, inline thread, review observation, and claimed closure that appears anywhere on this PR: conversation comments, review bodies, inline review threads, author replies, and any For each item report:
Rules for this ledger:
Reviewers whose findings must be inventoried
Known IDs that must appear in the ledgerThis list is a minimum checklist, not the complete set. Add any other finding you discover on the PR. Zach (
Alfredo / later rounds (
Claude prior review:
Required deliverable 2 — full independent audit of the current PRReview the complete current PR diff, not only historical items. Cover at least:
Report:
Be as detailed as possible. Trace dispositions to code and tests, not to comments. Prefer observable-behavior or mutation-style evidence over narrative. Read tests for what they actually assert. Review only — no code changes. This is analysis, not a formal GitHub approval. |
|
Claude finished @alfredolopez80's task in 3m 32s —— View job Independent re-review — PR #334 @
|
| ID | Sev | Disposition | Live-head evidence (claimed vs verified) |
|---|---|---|---|
| R1-B1 catalog ordering | Blocker | ✅✔ | compareCatalogKey → Buffer.compare(utf8,utf8) (catalogContracts.ts:127); SQL orders/resumes on catalog_utf8_bytes(). No localeCompare on the pagination boundary. |
| R1-H1 effectiveCapabilities union | High | ✅✔ | Derived from selectedPath only (liveAuthorizationResolver.ts:328); read-only-path test asserts gfs.write does not leak. |
| R1-H2 circular property test | High | ✅✔ | accessCatalogProperties.test.ts uses an independent TextEncoder UTF-8 oracle (not the SUT), mixed-case + -_./ + combining/emoji. Not circular. |
| R1-H3 revision hot row | High | ✅✔ | Singleton table + 18 statement triggers dropped (0104); replaced with FOR-EACH-ROW scoped per-user/team/resource bumps. See NF-5 for the narrowed residual. |
R1-M1 bare catch{} / no req.log |
Med | 🟡✔ | Still open. externalSessionAuth.ts:122,176,244 swallow the error, no req.log. Fail-closed (503/401), so observability nit. |
R1-M2 console.warn on request path |
Med | users.ts migrated to req.log ?? rootLogger. But teams.ts:139,182 still use console.warn — partial migration (NF-6). |
|
R1-M3 pin effectiveCapabilities===selectedPath |
Med | ✅✔ | Pinned in accessLiveAuthorization.test.ts:257-307. |
| R1-M4 duplicated CATALOG_FAMILIES | Med | 🟡✔ | Still two literal copies (userAccessPolicy.ts:5, catalogContracts.ts:8), no compile-time link. |
| R1-L1 terminal-500 legacy envelope | Low | 🟡✔ | errorHandler.ts still returns {error:'Internal Server Error'} on the terminal 500 for /api/v1/external; 4xx use the public envelope. Low impact (re-wrapped downstream). |
| R1-L3 accepted-expired invitation | Low | ✅✔ | See R5-L3. |
| R1-L5 Team-GFS localeCompare | Low | ✅✔ | Team-GFS uses compareCatalogText (byte order). |
| R1-L7 HMAC key reuse | Low | 🟡✔ | config.sessionJwtPrivateKey reused as cursor-HMAC + fingerprint key (accessCatalogCursor.ts:131, exactOperationalAuthorization.ts:88). Not exploitable; blast-radius isolation follow-up. |
| R1-L2/L4/L6/L8–L12 | Low | 🟡/🔵 | Tracked; L11/L12 are PR2 build-against notes. |
alfredolopez80 / later rounds + Claude prior
| ID | Sev | Disposition | Live-head evidence (claimed vs verified) |
|---|---|---|---|
| F1 live team-admin revocation | High | ✅✔ | externalSessionAuth.ts:233-242 re-derives role from getLiveTeamMembership; real-PG routes.externalTeamsLegacyAdminRevocation asserts stale admin V1 → 403 on rename/role/removal. |
| F3 password change/reset invalidates legacy | High | ✅✔ | validateLegacyUserSession fenced by epoch valid_after (userSessionService.ts:582); both flows call revokeAllUserSessions. |
| R5-L3 accepted onboarding after TTL | Med | ✅✔ | Authenticated onboarding may finish past 48h (accepted_at ≤ expires_at, single-use, null-password); token path keeps unconditional expiry. Bounded. |
| R9-H1 legacy epoch backfill | High | ✅ |
Migration 0106 idempotent, GREATEST monotonic; real-PG proof. Residual: fence is fail-open for SSO/passwordless (password_set_at NULL → valid_after NULL, skipped). Bounded (no password to invalidate). |
| R9-H2 operator-lifecycle revalidation | High | ✅✔ | validateExternalSessionAuthorityContext runs inside each mutation txn with SELECT users FOR UPDATE (lifecycle_state/version + epoch + revocation). |
| R11-H1 public GFS error contract | Blocker | ✅✔ | 408/411/413/425/507 → typed public codes; guardian tests un-blessed (921efe14 asserted internal_error) and restored to assert typed codes. |
| R11-B1 team-GFS admission throw | Critical | ✅✔ | Now a config fail-fast: userAccessPolicy.ts:332 throws if catalogMode≠off without a positive admission limit; teamGfsTopK.ts:232 null-throw unreachable when serving. Route-level regression accessCatalogConfiguredAdmission.test.ts. Over-limit still fail-closed. |
| R11-B2 REST client 413 regression | Blocker | ✅✔ | controlApiClient.ts preserves upload-length (413) + retry-after. |
| retry-masked CI greens | High | ✅✔ | vitest.config.ts:15 retry: 0 unconditional (R12-B4); both deterministic reds fixed at source (R12-B1: stale mockResolvedValueOnce removed; console-warn spy → structured req.log assert). |
| final authority covers subset of mutations | High | ✅✔ | R12-H1 (58b62e95) wires authority into invite/resend/revoke-invitation/retire; real-PG matrix now parametrizes 8 mutation classes × 3 interleavings (was 3). |
/rpc/token drops desktop_requires_team |
High | ✅✔ | R12-H3 (61cf72c9) restores desktop_requires_team+no_permitted_scopes to SAFE_DOMAIN_REASONS; test asserts the reason reaches the client and secret-internal-reason collapses to forbidden. |
| new error envelope breaks profile-ui | High | ✅✔ | R12-H2 (1e8536ff) — profile-ui/lib/api.ts reads structured publicError.code; sanitizer preserves member_registration_* on 503. No [object Object]. |
members/invite sentinel payload |
High | ✅✔ | R12-H4 (8dd209aa) removes the 'x'.repeat(121) / 51-null fabrication; real name/teams forwarded, invalid_name/too_many_teams mapped at the edge. |
| x-request-id / correlation on 503 | High | ✅ |
R13-B1/B2 (858ec766/8ffc021b) — app-level handler preserves request-derived correlation (validated, bounded) before randomUUID; test asserts echo toBe(requestCorrelationId), not relaxed. Residual: route forwarders (gfs.ts/access.ts/rpc.ts) still call the sanitizer without the fallback arg (NF-4). |
| derived RPC tokens live 300s, no revocation check | High | 🔵✔ | OPEN, PR2-scope. rpcAccessAuth.ts verifies sig/typ/scope only, no session/epoch/revocation DB read; TTL default 300s. The "rpcDelegationV2 mitigates" claim is false — that flag is never consulted by /rpc/token. PR1 doesn't regress base; credential-binding is PR2's contract. |
| consumed password_reset siblings not superseded | High | ⬜✔ | PRE-EXISTING. No one-active-reset constraint; sibling resets survive. PR hardened the flow (FOR UPDATE, token↔id binding, revoke-all) but did not change sibling semantics. Not widened. |
| V1 replacement TTL/lineage | note | 🟡✔ | OPEN (minor). exchangeLegacyExternalUserSession signs with no ttl arg → fresh 12h, no min(12h, source.exp); single logout doesn't kill lineage (epoch/revocation still fence each use). |
| revoke-all cutoff after lock | note | ✅✔ | SELECT users FOR UPDATE precedes cutoff computation (userSessionService.ts:394-396). |
| discovery-only capability union | note | 🟡✔ | accessCatalogCoordinator.ts:200 still unions candidate caps; no authority consumer reads it (only the route response + shadow parity). Rename to discoveryCapabilities before PR2. |
revision-canonicalization localeCompare |
Med | 🟡✔ | Still present — authorizationRevision.ts:46,70,97,112 feeds the ar1_ revision hash → cursor freshness + ap1_ access-path HMAC; resourceIdentity.ts:114 compareResourceIdentity is a dead export. Latent (ids constrained to UUID/[a-z0-9.-]); determinism/availability, not a bypass. See NF-1. |
| per-arm LIMIT window exhaustion | High | 🟡✔ | catalogProducerArms.ts:61-64 emits literal FALSE source_saturated for 16 unflagged arms (9 now duplicateCapable:true). Guarded-safe today (a truncated non-dup window still forces ≥take+1 candidates → hasMore truthful); becomes live only if an arm is mislabeled. See NF-2. |
426 unexpressible / no x-evenfire-client-version producer |
Med | 🔵✔ | 426 omitted from ACCESS_PUBLIC_STATUSES; minimum-client enforcement off by default. PR2 concern. |
access sanitizer rewrites code, discards message |
High | 🟡✔ | 409 invalid_request→conflict + canned message erases "Catalog access changed; refresh from the first page." Reachable only when catalog served (default off). See NF-3. |
| everyCatalogFamilyReady / 1000 ceiling / no leader election | Med | 🔵✔ | All confirmed; serve/shadow-only, default off. PR2 rollout hardening. |
| CodeQL scope / rate-limiter fail-open | Med | 🟡✔ | Custom suite swaps 2 stock queries reusing their @id; paths-ignore fixtures-only; scope check asserts 3 of ~14 services; rateLimiterService fails open on DB error and CodeQL certifies such routes as limited. |
Reviewer inventory completeness
- @jozer-rami —
NOT_FOUND_IN_PR. Searched all Remove Team Token PR1: User session, live authorization, and aggregate all-team access #334 conversation/reviews/inline threads and the branch commit log; the name appears only in unrelated merged-PR subjects synced fromdev, not as a reviewer of Remove Team Token PR1: User session, live authorization, and aggregate all-team access #334. - @zach88 / @alfredolopez80 / @josue-tech-94 / @claude — all findings/replies inventoried above; author closures independently confirmed or refuted per row.
Deliverable 2 — fresh full-diff audit (standing findings on this head)
No Blocker/Critical survives on this head. All four original blockers and both R11 blockers are genuinely closed (mutation-relevant tests read, not just green). Independent access-core scan also found no fail-open catch in the new decision chain, no SQL injection (all $n params; dynamic SQL is developer-authored arm literals only), and sound multi-tenant isolation (every arm filters by principal userId / active membership). Standing items, none a PR1 must-fix:
- NF-1 (Med, latent):
authorizationRevision.ts:46,70,97,112localeComparein the "canonical" functions that bind thear1_revision hash → cursor freshness and theap1_access-path HMAC.localeComparereturns 0 for distinct collation-ignorable strings →Array.sortfalls back to DB row order → a plan flip / Node-ICU upgrade can yield a different revision for identical state → spuriousaccess_path_stale/stale_cursor/authorization_revision_conflict409s. Determinism/availability, not a bypass; exploit path not reachable (ids constrained today). Fix: usecompareCatalogText; delete deadcompareResourceIdentity(resourceIdentity.ts:110). Fix → - NF-2 (Low→Med, latent trap): per-arm
source_saturated = FALSEon 16 unflagged arms makes "hit the work bound" indistinguishable from "reached source end". Safe today — a non-dupLIMIT take+1window holdstake+1strictly-distinct rows, so truncation still yields≥take+1candidates andhasMoreis truthful. Becomes a silent-drop only if a non-dup arm is ever mislabeled (starts emitting intra-arm duplicatelogical_ids). Fix: emitCOUNT(*) >= $limitfor every arm and drop theduplicateCapableflag. - NF-3 (Med, gated): access sanitizer 409
invalid_request→conflict+ always-canned message erases the actionable guidance; 426 unexpressible. Reachable only undercatalogMode=serve/shadow(default off). PR2 should widen the public vocabulary before enabling. - NF-4 (Low): R13 correlation fix is app-level only;
gfs.ts/access.ts/rpc.tsroute forwarders stillrandomUUIDthe bodycorrelationId(gfs response header survives). - NF-5 (Med, ops): deploy-window risk — 35
CREATE INDEX(0CONCURRENTLY, illegal in-tx) + dozens of trigger ops + 2 full-table backfills all insideinitDb's singleBEGIN…COMMIT, so locks onusers/team_members/workflow_runs/notification_deliveries/gfs_*accumulate until one final commit. Measure against a prod-size dump and document a window. Plus a UNION-without-ORDER-BY row-lock ordering inauthorization_bump_gfs_resource_subjects(:595-602) → deadlock-prone (ORDER BY subject_type, subject_idfixes it), and a same-row double-bump whenteam_id = usage_team_id. - NF-6 (Low, standing): R1-M1 bare
catch{}(noreq.log) andteams.tsconsole.warnviolate the pino logging standard; fail-closed, no secret leak. - NF-7 (Med, rollout trap): enabling
CONTROL_API_USER_ACCESS_CATALOG_MODEwithout its paired…TEAM_GFS_MEMBERSHIP_ADMISSION_LIMITthrows at module load → CrashLoopBackOff, and the pairing is undocumented (nodocs/, no configmap comment). This is the intended fail-before-traffic behavior (R11-B1) but a one-step foot-gun. Alsoconfigmaps.yaml:21shipsOPERATIONAL_ACCESS_INDEXER_ENABLED: 'true'(code defaultfalse), so the indexer + its cluster-wide watch RBAC start in every deployed environment on merge — worth stating explicitly since readingconfig.tsalone implies the opposite. - NF-8 (Low, defense-in-depth): the catalog cursor payload carries no
userId; cross-user replay is blocked only indirectly (revision equality + producer re-filtering by sessionuserId). BinduserIdinto the cursor payload/HMAC before any PR2 stabilization ofsessionRevision.
Reopened items: none. No previously-CLOSED finding reopened after the R12/R13 commits or the dev sync. The delta commits are additive corrections; the byte-order/H1/H3 fixes are untouched.
PR-description accuracy: consistent — v2 surfaces are 503-closed by default; operationalAccessIndexerEnabled defaults false in code (note the configmap override in NF-7). The CodeQL prose slightly under-discloses that the custom suite reuses stock @ids (disclosure gap, not a security gap).
Answers
1. Are there any PR1 must-fix findings remaining?
No. Every item that breaks a contract PR1 claims to establish is closed and test-backed on this head. What remains is (a) latent defects gated behind default-off rollout flags (NF-1/2/3, 426, everyCatalogFamilyReady, indexer ceiling/leader-election), (b) legitimately PR2-scope credential-binding (RPC 300s window — real, quantified, not a PR1 regression), (c) pre-existing/out-of-diff (sibling password-reset supersession), and (d) hygiene/ops follow-ups (NF-4/5/6/7/8, R1-M1/M4/L7, V1 replacement TTL, SSO null-epoch fence). None is a foundation-contract break.
2. Is the current exact head safe as the PR2 stack base?
Yes. Authorization is fail-closed (no fail-open in the new decision chain), v2 surfaces are disabled by default, and the live-authority + aggregate-discovery + byte-order + revision-scoping contracts PR2 depends on are coherent and real-PG-backed. The R12/R13 series fixed the d3ba05b0 regressions at source (verified, not masked). PR2 should carry these as explicit acceptance criteria: (a) final derived-RPC/GFS issuance fencing + the 300s post-revocation window; (b) the public-vocabulary/sanitizer gaps (NF-3, 426); (c) the rollout hardening (NF-7, everyCatalogFamilyReady, leader election) and the NF-1/NF-2 comparator/saturation fixes before enabling CATALOG_MODE/shadow anywhere — the shadow lane is not usable as rollout evidence until then.
dev. Per the PR's own merge constraint, PRs 2, 3, and 4 must be integrated into this branch and final stack reconciliation must pass first.
Review only — no code changes. This is analysis, not a formal GitHub approval. Dispositions traced to code and test assertions at 8ffc021b across five independent subsystem traces; suites were not executed here (npm sandboxed), so pass/fail is read-verified and the CI-lane fail-loud guards were inspected instead.
alfredolopez80
left a comment
alfredolopez80
left a comment
There was a problem hiding this comment.
Independent review — exact head 8ffc021b
Read-only review against dev 4862d9c2 (live origin/dev). Claude's ledger at #334 (comment) was used as a checklist only. Every disposition below was re-found in head (and compared to base when claiming introduced_by_pr).
This is not a GitHub Approve. Suites were not re-executed locally (control-api has no vitest binary here). Ordinary CI 33198807151 and CodeQL 33198807026 are green on this head.
Legend: ✅ CLOSED ·
Deliverable 1 — historical-findings closure ledger
| ID | Sev | Claude | This review | Live-head evidence |
|---|---|---|---|---|
| R1-B1 catalog ordering | Blocker | ✅ | ✅✔ | compareCatalogKey → Buffer.compare UTF-8. SQL catalog_utf8_bytes(). Team-GFS uses compareCatalogText. Independent TextEncoder oracle. |
| R1-H1 effectiveCapabilities | High | ✅ | ✅✔ | liveAuthorizationResolver.ts:328 copies selectedPath.behavior.capabilities only. |
| R1-H2 circular tests | High | ✅ | ✅✔ | Streams sorted by a standalone oracle, not the SUT. |
| R1-H3 revision hot row | High | ✅ | ✅✔ | 0104/0105 drop singleton + statement triggers. Residual is per-subject locks (RP-005), different granularity. |
R1-M1 bare catch{} |
Med | 🟡 | 🟡✔ | externalSessionAuth.ts:122,176,244. Fail-closed 503/401. No req.log. RP-006. Combined with RP-020, a 503 can be invisible. |
R1-M2 console.warn |
Med | users.ts migrated. teams.ts:139,182 still console.warn — pre-existing on base. |
||
R1-M4 CATALOG_FAMILIES |
Med | 🟡 | 🟡✔ | Two identical literals. RP-012. |
| F1 live admin revocation | High | ✅ | ✅✔ | Role from getLiveTeamMembership, not token claims. |
| F3 password invalidates V1 | High | ✅ | ✅✔ | valid_after fence + users FOR UPDATE before epoch write. |
| R9-H1 epoch backfill | High | ✅ |
valid_after NULL skips the fence (SSO/passwordless residual). |
|
| R9-H2 / R12-H1 final authority | High | ✅ | Eight HTTP classes + real-PG matrix are wired. Invite/resend activation does not re-check the session epoch (RP-013). Helper fail-open if authority omitted (RP-011). | |
| R11-H1 / B1 / B2 | Blocker | ✅ | ✅✔ | GFS public codes, admission fail-fast, 413 upload-length. |
R12-H2..H4 + retry: 0 |
High | ✅ | ✅✔ | profile-ui structured error.code; invite sentinel gone; desktop_requires_team allowlisted; retry: 0. |
| R13 correlation | High | ✅ |
App-level fallback exists. Route forwarders still two-arg → randomUUID (RP-004). |
|
| RPC 300s | High | 🔵 | 🔵⬜✔ | Pre-existing. rpcAccessAuth is signature/typ/scope only. TTL 300s. RP-009. |
| sibling password-reset | High | ⬜ | ⬜✔ | Pre-existing. Not widened. |
| jozer-rami | — | NOT_FOUND | ✔ | No comments/reviews/inline threads on #334. |
| josue-palmera | — | author is josue-tech-94 | ✔ | No GitHub login josue-palmera. |
Reopened previously-CLOSED items: none, except R12-H1 is PARTIAL (not a full reopen of the eight-class HTTP wiring — the post-commit invite/resend gap is new relative to Claude's CLOSED).
Deliverable 2 — PR-introduced findings on this head
Count: 0 Blocker · 0 Critical · 2 High introduced · 10 Medium introduced · 6 Low introduced · 5 pre-existing · 1 evidence gap (local Vitest).
Claude's last pass reported no PR1 must-fix. This review disagrees on RP-013 (live invite activation after revoke-all). The extra Medium vs the earlier “9” is RP-020 (silent sendPublicApiError), found in the logging lane.
Inline comments are attached on the 2 High and 10 Medium introduced by this PR.
Introduced High
| ID | Sev | File | Defect | Gate | Cluster |
|---|---|---|---|---|---|
| RP-013 | High | membership.ts:800-872 |
Two-phase invite: txn 1 checks epoch, email is sent outside the lock, txn 2 only re-checks team role + lifecycle_state='active'. revokeAllUserSessions does not change those columns, so draft → pending still happens and the emailed token works. Accept/getInvitationByToken require pending (draft/revoked are hidden) — the hole is activation, not a dead draft. Real-PG matrix only contends the first users FOR UPDATE. Retirement does fail txn 2. Resend is weaker (already pending). |
Live today (email I/O window). | C3 |
| RP-019 | High | liveAuthorizationResolver.ts:227-248 |
Catalog ap1_ HMACs an ar1_ over all hydrated paths. Live rebuilds ar1_ from capability-filtered capable only. Heterogeneous grants + gfs.write + catalog handle → permanent access_path_stale. Not an extra-capability grant. Live unit test uses gfs.read and live-issued ids. |
Catalog mode default off. | C1 |
Introduced Medium
| ID | Sev | File | Defect | Gate | Cluster |
|---|---|---|---|---|---|
| RP-001 | Med | authorizationRevision.ts:46 |
localeCompare (and normalizeAccessCapabilities .sort() UTF-16) still bind ar1_ / ap1_ after pagination moved to UTF-8 bytes. Spurious stale/409 across ICU/locale. UUID/ASCII ids make a concrete reorder hard today. |
Catalog serve | C1 |
| RP-002 | Med | catalogProducerArms.ts:61-64 |
Non-duplicateCapable arms emit literal FALSE for source_saturated. Silent drop if a future arm is mislabeled. |
Safe today if arms stay 1:1 | C1/C5 |
| RP-003 | Med | external-rest-api/.../publicApiError.ts:57 |
409 invalid_request not allowlisted → rewritten to canned conflict. Catalog refresh guidance erased. |
Catalog serve | C2 |
| RP-005 | Med | userAccessFoundationSchema.ts:595 |
In-tx CREATE INDEX (no CONCURRENTLY) + unordered GFS subject UNION locks + double bump when team_id = usage_team_id. |
Deploy window | C6 |
| RP-007 | Med | configmaps.yaml:21 |
Catalog-without-admission throws at module load (CrashLoop). Deploy sets indexer 'true' (code default false) with no leader election, 1000-object clamp. Live on merge. |
Merge / catalog enable | C5 |
| RP-011 | Med | membership.ts:1901 |
hasValidManagedMutationAuthority: if (!authority) return true. HTTP eight-class routes do pass it. Future/internal caller skips the fence. |
Not current HTTP | C3 |
| RP-015 | Med | external-rest-api/.../access.ts:10 |
No 426 anywhere in sanitizer tables / PROPAGATED_STATUSES. Global handler maps other 4xx to invalid_request. Minimum-client gate becomes inert for public clients. |
Enforcement default false | C2/C5 |
| RP-016 | Med | external-rest-api/.../publicApiError.ts:256 |
publicCorrelationId echoes raw x-correlation-id (sanitizer validates; this helper does not). |
Live on 4xx fallback | C2 |
| RP-017 | Med | codeql-config.yml + local .ql |
disable-default-queries: true; stock js/missing-rate-limiting and js/user-controlled-bypass excluded and replaced by local queries reusing those @ids. Bypass model only treats 5-arg sendPublicApiError. paths-ignore is fixtures-only. |
SAST only | — |
| RP-020 | Med | control-api/.../publicApiError.ts:39 |
sendPublicApiError never logs. Direct /external 401/403/503 have no audit line. clerumErrorHandler still logs before calling it on next(error). Fail-closed. |
Live observability | C2 |
Introduced Low (no inline)
| ID | File | Defect |
|---|---|---|
| RP-004 | gfs.ts / access.ts / rpc.ts |
Route sanitizers omit fallbackCorrelationId. |
| RP-006 | externalSessionAuth.ts:122,176,244 |
Bare catch {}. |
| RP-008 | accessCatalogCursor.ts |
Cursor payload has no userId (HMAC + session revision still bind). |
| RP-012 | CATALOG_FAMILIES ×2 + REQUIRED_SOURCES ×2 |
Dual source of truth; order is a wire contract. |
| RP-018 | externalSessionIssuance.ts:148 |
V1 exchange mints a fresh 12h TTL, not min(12h, source.exp). |
| RP-021 | main.ts:122 |
Indexer .completion.catch uses console.error at runtime. |
Pre-existing (not introduced)
| ID | Sev | Defect |
|---|---|---|
| RP-009 | High | RPC bearer valid 300s after parent revoke. |
| RP-010 | High | No one-active password-reset constraint. |
| — | — | POST /external/teams has no in-txn authority (createTeamForUser). Middleware admits without users FOR UPDATE. |
| — | — | teams.ts / sharedFilesystems.ts console.warn. Rate-limiter DB fail-open. |
Root-cause clusters
| Cluster | Violated invariant | Fix in this PR vs PR2 |
|---|---|---|
| C1 | One UTF-8 byte order everywhere that binds pagination, ar1_, ap1_ |
Before catalog serve (RP-001/012/019) |
| C2 | One public {status, code, message, correlation} table; every egress logs |
Before minimum-client / catalog (RP-003/004/015/016/020) |
| C3 | Every privileged directory write revalidates the session inside the txn that makes the capability live | RP-013 now (invite/resend). RP-011 fail-closed primitive. |
| C4 | Derived credentials die with the source epoch | PR2 (RP-009 300s; GFS authGeneration is v1-only) |
| C5 | Flags, deploy, and fail-fast are one contract; no single env CrashLoops a replica | Before merge / catalog enable (RP-007) |
| C6 | Production DDL does not hold write locks for the entire init txn; subject bumps lock in stable order | Before first prod migrate (RP-005) |
Answers
1. Merge into dev?
No. The PR's own constraint forbids landing PR1 before PRs 2–4. Merge would also turn the operational access indexer on in every replica (RP-007) and leave RP-013 live.
2. Any PR1 must-fix remaining?
Yes — RP-013. A revoked session must not authorize invitation activation after the authorizing txn commits. Claude marked R12-H1 CLOSED; the eight HTTP classes are wired, but the two-phase path is not epoch-fenced.
3. Safe as PR2 integration base?
Yes, with explicit acceptance items: RP-013 first; RP-019 + C1 before CATALOG_MODE=serve/shadow; RP-015 before MINIMUM_CLIENT_ENFORCEMENT; RP-007 before treating indexer-on as intentional; RP-009 quantified as 300s until PR2 binds derived RPC/GFS to the epoch (v2 sid/sv, not only v1 authGeneration).
4. What this review did not promote
CSRF dummy-Bearer: extractAuthToken prefers Bearer and fails 401 (no cookie fallback). Old-client /rpc/token 403 shape: in-tree Desktop already reads error.details.reason. resolveLiveAuthorization having one caller is PR1 stated scope. Advertised-but-unconsumed flags (rpcDelegationV2, etc.) are PR2–4 work. session_expired is declared and never emitted.
Review only — no code changes and no formal approval. Evidence is inspected-code at 8ffc021b vs 4862d9c2. Inline threads cover the 2 High and 10 Medium introduced by this PR.
Independent review — PR #334 @
|
| Commit / file | Guard | What was done |
|---|---|---|
eec63bd1 (r9-b1) |
T2 public boundary | Widened the DSN matcher by keyword instead of an exact allowlist |
1045b570 |
T2 public boundary | Moved private runtime URL to return True for source fixtures and widened the fixture-path regex |
EvenfireMissingRateLimiting.ql (new) |
CodeQL | Relaxed "the limiter dominates the expensive node" to "a limiter is reachable later on the route" |
1045b570 is labelled "preserve … after latest dev" but restores nothing — verified
that merge 3cc9fa25 lost no lines under scripts/. It adds a synthetic literal and
widens the escape hatch in the same commit, and its body claims validation from one
result. Accepting a guard change needs three: it passes on the tree, it fails on a
freshly introduced violation, and its escape hatch silences an annotated case. The second is
demonstrated nowhere.
Blockers
B1 — Canonical rate limiting runs after runtime policy work (R18-B1, author-declared, now measured)
control-api/src/routes/external/access.ts:209-212 (catalog) and :253-256 (resolve).
requireEffectiveV2Contract is composed before rateLimitMiddleware, so the 409 branch
is 100% unmetered.
REPRODUCED. Temporary test, run and deleted, with the bucket already exhausted
(checkAndIncrement → allowed:false):
npx vitest run test/zz-audit-r18b1-repro.test.ts → EXIT=1, Tests 2 failed (2)
"limiterCalls": 0,
"policyCalls": 2,
"sessionValidations": 1,
"status": 409,
limiterCalls: 0 with an exhausted bucket proves the limiter is never reached.
policyCalls: 2 quantifies that the policy is resolved twice and the first result discarded.
Honest cost, both configurations (measured statement-by-statement, not estimated):
| Configuration | Statements before the limiter |
|---|---|
Today (catalogMode='off') |
5 statements / 1 transaction. v2 is read_write with SELECT … FOR UPDATE OF s + UPDATE external_user_sessions; v1 is read_only with a plain SELECT, no row lock. The two policy resolutions short-circuit in userAccessRuntimePolicy.ts:147-149 without touching the DB. |
Rollout target (catalogMode on) |
15 statements / 3 pool checkouts, 1 of them a write holding a row lock. |
Requires a validly signed session. Measured: a garbage token costs
{ status: 401, statements: 0, poolCheckouts: 0 } — verifyUserSessionV2Token and
verifyExternalSessionToken are pure jwt.verify. Any report claiming "an unauthenticated
caller forces expensive DB work" is inaccurate for the shipped configuration. That
becomes true only once the catalog is on, because the policy resolution runs before token
verification.
The correct pattern is 20 lines above, in the same file.
/external/access/capabilities (:184-187) composes
requireAuthenticatedExternalUserRateLimitContext + rateLimitMiddleware before the handler,
and has a real guard test (test/routes.externalAccess.test.ts:186-206). Ten other external
route files mount createExternalClientRateLimiters pre-auth;
/external/access is the only external family with no per-IP ceiling at all.
Why it was never closed: 8e4b1d21 "r3-h8 route external policy through limiter
middleware" changed exactly one line of access.ts — the /capabilities one. History
confirms no commit ever moved the policy work behind the limiter.
Fix: move the limiter ahead of requireEffectiveV2Contract; reuse the policy already
resolved during authentication instead of re-resolving it (one line — see H-group 4); and
mount the pre-auth per-IP limiter this family is missing.
B2 — The sandbox_app producer drops canonical resources and reports the page as complete
catalogProducerSql.ts:370-420 (both arms lack duplicateCapable) ·
catalogProducerArms.ts:60-64 (SQL emits literal FALSE) ·
catalogProducerHead.ts:139-144.
source_saturated is generated as constant FALSE for any arm without duplicateCapable,
and listBoundedProducerKeys translates that into state.exhausted = true after the first
refill. But sandbox_app arms can return repeated logical_ids: the PK of
operational_resource_relationships includes source_id and relationship_instance_id
(userAccessFoundationSchema.ts:154-157) and the SELECT has no DISTINCT.
Failure: GET /external/access/catalog?families=sandbox_app&limit=1 for a user with two
recipes exposing app-a and one exposing app-b → app-b disappears permanently and the
response is complete: true, partialErrors: [], nextCursor: null. That is the exact
inverse of the PR's stated invariant.
node repro-minimal.mjs → EXIT=0
source rows: 3 · distinct sandbox_apps: app-a, app-b
candidates returned: sandbox_app:app-a
hasMore: false · continuation.exhausted: true
Evidence scope, stated honestly: the loss mechanism is reproduced by executing a literal
copy of listBoundedProducerKeys. The input premise — that the SQL yields duplicates with
source_saturated=FALSE — is read from the code, not executed against Postgres.
The coverage hole that let it through:
catalogDistinctPagination.realPostgres.integration.test.ts:420 is named "preserves distinct
keys for every duplicate-capable producer arm class" and exercises host, context,
workflow_recipe, mcp_server, shared_filesystem and gfs_resource — it omits
sandbox_app, the one duplicate-capable family without the flag.
Fix: add duplicateCapable: true to both arms and extend that test. Independently, make
source_saturated always (SELECT COUNT(*) FROM arm) >= $4; the flag should save the COUNT,
never be able to produce a false "not saturated".
B3 — The shadow lane can never compare gfs_resource, by arithmetic
accessCatalogShadow.ts:131-140 reserves a child budget of databaseStatements: 8. The
minimum non-empty path for gfs_resource needs 9:
| # | Step | File:line | Acc. |
|---|---|---|---|
| 1 | withAccessDatabaseTransaction → BEGIN |
accessCatalogCoordinator.ts:398 |
1 |
| 2 | SET TRANSACTION ISOLATION LEVEL REPEATABLE READ READ ONLY |
accessAuthorityTransaction.ts:9-15 |
2 |
| 3 | SELECT set_config('statement_timeout', …) |
accessAuthorityTransaction.ts:16-22 |
3 |
| 4 | principal + memberships | catalogAuthorityContext.ts:83-168 |
4 |
| 5 | operational_catalog_source_state |
catalogAuthorityContext.ts:185-194 |
5 |
| 6 | refill(null) — direct arm |
catalogProducerHead.ts:147 |
6 |
| 7 | readHeads — team arms |
teamGfsTopK.ts:200 → :168 |
7 |
| 8 | hydrateCanonicalKeys |
catalogProducers.ts:564-569 |
8 |
| 9 | COMMIT |
accessDatabaseQuery.ts:202 |
9 → throws |
runAccessDatabaseQuery charges databaseStatements unconditionally
(accessDatabaseQuery.ts:78). Condition, in terms of the variables: with M = active
memberships and R = visible GFS resources, step 7 fires ⟺ M ≥ 1 (even if those teams own
nothing) and step 8 ⟺ R ≥ 1. So M ≥ 1 ∧ R ≥ 1 ⟹ 9 statements ⟹ skipped_capacity, always.
Why this matters more than it looks: the shadow lane exists to de-risk the rollout. For
any user with a membership and a GFS resource it records skipped_capacity and compares
nothing — and the failure is silent, because skipped_capacity is the same bucket as
genuine load shedding. The metric cannot distinguish "we were busy" from "this family is
structurally impossible to compare".
Fix: size the child budget from the real per-family cost, and separate structural
exhaustion from transient exhaustion in the metric.
B4 — Every readiness degradation ends in throw: shadow crash-loops on cold start, serve can never start
userAccessRuntimePolicy.ts:64-71,147-158,190-197 · userAccessPolicy.ts:129,205,210,362-387
· main.ts:63,116.
resolveEffectiveUserAccessPolicy carefully degrades to unavailableReadiness() in its two
failure branches — but that readiness carries snapshot: 'unavailable' and
compileUserAccessPolicy rejects it on its first line
(requireCondition(readiness.snapshot === 'current', 'readiness_snapshot_unavailable')),
before it ever looks at catalogMode. Both degradation branches are dead ends that always
throw. Separately, catalogParityAccepted is a constant false that no readiness constructor
overrides, so serve fails unconditionally.
- Cold start with
catalogMode=shadow→CrashLoopBackOff, bootstrap deadlock.
main.ts:63resolves the policy before the indexer starts at:116. Without fresh rows
inoperational_catalog_source_state,runtimeReadinessadmits only the 6 DB-only
families →catalog_shadow_families_incomplete→process.exit(1). The indexer that would
populate those rows never runs. Escapable only if another replica keeps the rows fresh; a
cold start of the whole Deployment is permanently unstartable. serve/serve_and_shadow→catalog_serving_parity_not_accepted, even with perfect
operational readiness.- Hot path, already running. If an operational source moves to
relisting— the routine
watch expiry —userAccessRuntimePolicy.ts:196explicitly re-throws, and
externalSessionAuth.ts:176turns it into 503authority_unavailableon every
/external/*route: profile, teams, members, invitations, GFS. The blast radius far
exceeds the feature the flag protects, andshadowis precisely the mode meant to de-risk.
REPRODUCED, with the red validated first:
npx vitest run test/zzTempReviewPolicyRepro.test.ts
1st run (deliberately wrong expectation): EXIT=1
AssertionError: expected 'readiness_snapshot_unavailable' to be '<no-throw>'
2nd run (corrected): EXIT=0, Tests 4 passed (4)
The four assertions pin the exact codes: [off + unavailableReadiness] → readiness_snapshot_unavailable,
[shadow + unavailableReadiness] → readiness_snapshot_unavailable,
[shadow + cold start] → catalog_shadow_families_incomplete,
[serve + perfect readiness] → catalog_serving_parity_not_accepted.
Fix: have compileUserAccessPolicy return a degraded policy (everything v2 off, v1
alive) when readiness falls short, and reserve throw for invalid configuration validated at
startup, not on the hot authentication path. Move the main.ts:63 call after the indexer
starts, or give it a bootstrap grace window.
B5 — Deployment: the migration Job locks 12 hot tables in one transaction with no lock_timeout
userAccessFoundationSchema.ts:201-306 and :629-686; runner db.ts:6246-6291; Job
deploy/scripts/run-control-api-db-migration.sh.
Measured on the tree: 14 new tables, 37 indexes, 24 of them on pre-existing tables, CONCURRENTLY: 0,
plus 36 DROP/CREATE TRIGGER statements — all inside a single BEGIN … COMMIT, with no
SET LOCAL lock_timeout.
To be precise about the actor: the pod does not migrate. main.ts calls assertDbReady(),
which only verifies and throws if a migration is missing — correct fail-fast. Migrations run in
the separate migrate.ts → initDb() entrypoint, i.e. the deploy Job. The risk is that Job
holding ACCESS EXCLUSIVE (which blocks reads too) on users, teams, team_members,
workflow_runs, workflow_approval_requests, notification_deliveries, gfs_grants,
gfs_shares, gfs_resources while already-serving pods keep taking traffic. Table volume
decides how long the index build takes, not whether it blocks: the trigger DDL blocks
users and team_members even if it takes 1 ms, and no lock is released until the batch
commits.
The repo already learned this lesson. Migration 0090 (db.ts:2350, registered at
:5899) sets SET LOCAL lock_timeout = '60s' with 13 lines of comment stating exactly this
reasoning — and the Job's backoffLimit: 2 / restartPolicy: Never was added in that same
PR to accompany it. The 6 new migrations inherit the Job but not the lock_timeout: they
inherit the retry without the fail-fast that made it useful. With lock_timeout = 0 there is
no abort to retry, only an unbounded wait.
A failing deploy does not end the incident. On kubectl wait --timeout=300s expiry the
script exit 1s without deleting the Job. The migration pod stays alive, its transaction
open and its ACCESS EXCLUSIVE request queued — and in Postgres a queued lock request
blocks everyone behind it, readers included. There is no activeDeadlineSeconds, no
statement_timeout and no idle_in_transaction_session_timeout on this path.
Reversibility, precisely — two different questions, two different answers:
- Schema rollback: SAFE. Additive only — 11
CREATE TABLE IF NOT EXISTS, zero
ALTER TABLE, no newNOT NULLcolumn on an existing table; old code keeps inserting
unchanged. Job failing mid-way is also safe: one transaction, schema untouched, and old code
starts because itsCONTROL_API_MIGRATIONSdoesn't include0107–010c. - Write-cost rollback: NOT REVERSIBLE. If the Job completes and the app rollout then
fails, reverting the image leaves 8+FOR EACH ROWtriggers active on the hottest
tables. There is nodownmigration (DbMigrationhas no reversal field) and no feature
flag switches them off — they don't depend oncatalogModeorsessionV2Issuance.
Fix: SET LOCAL lock_timeout = '60s' at the head of each of the 6 new apply bodies,
mirroring 0090; activeDeadlineSeconds on the Job so a hang cannot outlive the script's
exit 1; and move the 24 indexes on pre-existing tables to an online migration outside the
startup transaction.
High — grouped by theme
Group 1 — Guards that do not guard
G1-a · The T2 public-boundary guard now lets internal DSNs through any test file.
scripts/tests/test-minikube-t2-public-boundary.sh:145 (function), :155-164 (the branch),
:161 (the keyword regex), :227 (call site). The new exemption accepts any credential-less
Postgres DSN in a test path when the line contains one of six words:
(?i)(CONTROL_API_REAL_PG_ADMIN_URL|adminUrl|sentinel|fixture|example|test).
REPRODUCED with a before/after control and two negatives, same fixture and same base:
CASE 1 PR guard, internal DSN in a test file → PUBLIC_BOUNDARY_PASS EXIT=0
CASE 2 origin/dev guard, same fixture → PUBLIC_BOUNDARY_REJECTED EXIT=1
- control-api/test/leak.realPostgres.integration.test.ts: private PostgreSQL URL
CASE 3 PR guard, same DSN under control-api/src/ → REJECTED EXIT=1
CASE 4 PR guard, DSN *with* credentials in a test → REJECTED EXIT=1
Cases 3 and 4 bound the hole: it does not apply under src/, and credentialed DSNs are still
rejected. The hole needs three simultaneous conditions, so please don't read it as "any DSN in
any test".
What makes the fix trivial: eec63bd1 is +156/−0 over the two scripts and deleted no
string. The strings that motivated it are synthetic — one literal, 11 times:
grep -hoE "postgres(ql)?://[^\"' ]+" test/*.realPostgres.integration.test.ts | sort | uniq -c
→ 11 postgresql://postgres@127.0.0.1/postgres. Loopback, user postgres, no password. The
fixtures are legitimate; the exemption mechanism is the defect. For credential assignment
the same commit used an exact literal allowlist — correct and airtight, verified. Apply
that same treatment to DSNs and drop the keyword branch.
G1-b · The new CodeQL model is, by design, blind to B1's defect class.
.github/codeql/queries/EvenfireMissingRateLimiting.ql:143-168. The queries are all new
(git diff --diff-filter=A). The first disjunct uses stock isGuardedByNode (limiter
dominates); the three added disjuncts relax it to "a limiter is reached later on the route",
and the fourth marks a parent router.use as guarded when all its HTTP children rate-limit —
without requiring the context handler itself to be cheap. access.ts:182 fits that shape
exactly, so CodeQL cannot report the blocker this PR has open.
Two more measurements make this affirmable without running CodeQL:
- The fixture never exercises the real shape.
asyncHandlerappears 0 times in
.github/codeql/queries/test/and 6 times incontrol-api/src/routes/external/access.ts.
Either branch is bad: if CodeQL modelsasyncHandleras resuming dispatch,access.ts:182is
suppressed and B1 is invisible; if it does not, the 45-line fourth disjunct is dead code in
production and only ever runs against the synthetic fixture. - The recognition of "canonical middleware" is a hand-written allowlist of two file paths and
three function names, withsameSourceFilerequired in five predicates.
requireAuthenticatedExternalUserRateLimitContext— the guard this PR introduces, used
15+ times includingaccess.ts:186— is in neither list, because it lives in a third file.
The query also replaces stockjs/missing-rate-limitingin the suite.
G1-c · The .qls verifier can be bypassed by exclusion syntax. REPRODUCED end-to-end with a
codeql stub against the real guard: excluding js/sql-injection by id → EXIT=1
(correct); excluding the same query by tags contain → the query disappears from the resolved
suite just the same and the guard returns EXIT=0. The guard is not inert — it satisfies
all three required results — but what it watches is syntactic, not semantic.
Group 2 — Tests that exist but protect the wrong thing
G2-a · The only ordering test on these routes certifies the blocker as correct.
test/routes.externalAccess.test.ts:163-173. The it.each is literally named
'keeps %s %s disabled before rate-limited work' — its name enshrines the policy gate
running before the limiter — and asserts only 409 + invalid_request. The assertion that
would catch it exists in the same file, at :186
(expect(mocks.resolvePolicy).toHaveBeenCalledTimes(1)) — applied to /capabilities, the one
route that already has the right order. And the two new policies have zero coverage:
grep -rn 'external_access_catalog\|external_access_resolve' → 4 hits, all in
src/routes/external/access.ts; 0 in control-api/test.
Practical consequence: whoever fixes B1 by swapping the order will see this test go red and
think they broke something. The bug is protected by its own guardian.
G2-b · Mutation testing: 9 mutations, each reverted with a verified-clean tree between them.
| # | Mutation | EXIT | Tests that go red |
|---|---|---|---|
| 1 | retry: 0 → retry: 2 (vitest.config.ts:15) |
0 | NONE |
| 2 | PUBLIC_CODE_BY_STATUS[413] → 'internal_error' |
0 | NONE |
| 3 | PUBLIC_CODE_BY_STATUS[422] → 'internal_error' |
0 | NONE |
| 4 | PUBLIC_CODE_BY_STATUS[429] → 'internal_error' (contrast control) |
1 | 3 |
| 5 | ALLOWED_CODES_BY_STATUS[413] → new Set([]) |
0 | NONE |
| 6 | both 413 tables broken together | 1 | 3 |
| 7 | both 422 tables broken together | 0 | NONE |
| 8 | r15-h1 reverted (membership.ts:856-863) |
— | NONE in the non-Postgres lane |
| 9 | r15-b4 reverted |
1 | 1 |
The 429 contrast control is what makes this table mean something: without it, "I found no test
protecting this" is indistinguishable from "I failed to find them". The suite can catch this
class of regression — it does so for 429, including the fallback branch.
The 413 defect is double, and rows 2/5/6 separate the two halves. The pass-through path is
tested (routes.gfs.test.ts:571-611), but those tests never reach the mapping:
// external-rest-api/src/http/publicApiError.ts:177-180
const fallbackCode = PUBLIC_CODE_BY_STATUS[error.status] || 'internal_error'
const code = ALLOWED_CODES_BY_STATUS[error.status]?.has(suppliedCode) ? suppliedCode : fallbackCode- No test walks the fallback branch for 413 (row 2: the fallback table can say anything and
nobody notices). No test emulates a 413 whose upstream supplies no valid code — which is
exactly where the historical regression lived. - Neither table is independently verifiable, because for 413 both carry the same string and
codefalls silently from one to the other (row 5: emptying the allowlist is also green, since
fallbackCodeis'payload_too_large'too). Only breaking both at once turns 3 tests red.
So: the three 413 tests do not verify the mapping, they verify the final value — they cannot
tell which table produced it, and therefore protect neither.
422 is worse: coverage is zero, not masked. Row 7 breaks both 422 tables simultaneously and
the suite still reports EXIT=0, 232 passed (232). Not one test asserts its public code.
Fix: a table-driven test over all 18 statuses exercising both branches per status —
upstream supplying a valid code, and upstream supplying none. That pattern already exists and
works for 429 (publicErrorContract.test.ts > preserves only a bounded retry delay from legacy rate-limit bodies, which emulates a legacy body with no supplied code). It just needs generalising.
G2-c · r15-h1 is covered only by a Postgres-gated suite. Its sole guard,
routes.externalTeamsFinalAuthority.realPostgres.integration.test.ts, is describe.skip on
any run without a DSN. Reverting the authority revalidation passes 5,515 tests green.
G2-d · retry: 0 has nothing pinning it. grep -rln "vitest.config" over scripts/,
.github/, control-api/test/ and Makefile returns exactly one file,
scripts/tests/test-gfs-real-pg-minikube-gate.sh:50, which checks setupFiles, not retry.
One grep -Fq 'retry: 0' in that same script closes it.
Group 3 — The public error contract is implemented three times and already diverges
REPRODUCED by importing the real modules at 8d541772: 11 of 19 statuses disagree
between external-rest-api/src/http/publicApiError.ts (the propagated table) and the ternary
chain in external-rest-api/src/app.ts:117-159 (72 lines, new in this PR), within the same
package. 408, 409, 410, 411, 412, 413, 425, 426, 502, 504, 507 all differ, and retryable
differs for 408/425/502/504.
- Two codes control-api emits are not recognised by the edge:
426 upgrade_required
(9 emitters) collapses tointernal_error, and409 invalid_requestcollapses toconflict
— soaccess.ts:117's "This access contract is not enabled for the current session" reaches
the client as a generic conflict message. upgrade_requiredhas no public representation at all, so minimum-client enforcement
is inert end to end: a stale Desktop can never be told to upgrade.- 502/504 collapse to
500 internal_errorwithretryable:falseoutside GFS, so a
transient proxy failure is presented to Desktop as permanent and is not retried. The
aggravating detail:controlApiClient.ts:141-147synthesises that 502 deliberately, and
the comment right above it (:132-136) says why — "instead of propagating a raw SyntaxError
through Express' default error handler (which masks it as an opaque 500)". The handler this
PR adds turns it back into an opaque 500. The documented intent is undone by the PR's own
code. forwardControlApiErroris duplicated 8 times; 7 copies drop the sanitizer's headers.
Latent today — those routesnext(error)and the global handler does emit headers — so it is
an armed trap, not an active failure: it fires the day someone adds 429 or 413 to a local
status set.- The envelope changed with no version negotiation, breaking both directions:
dev's
profile-ui doesString(parsed.message || parsed.error || text)→"[object Object]"for
every error, and itsmember_registration_*friendly-message branches stop firing; a new
profile-ui against an old edge shows the raw internal string
"Control API … failed (503): …". During a rolling update the same user sees both, alternating
by pod. Suggested mitigation with zero deployment coordination: emit the new envelope and
keep a flatmessageat the root during the V1 window. - The PR canonicalises ~80% of the surface and leaves the rest incompatible: 5 new
res.status(400).json({error:'<string>'})inroutes/members.ts:159-182, and
middleware/rateLimit.ts:44-51still emits429 {"error":"<string>", retryAfterSeconds}on
password reset (5/min), invitation lookup/accept and team directory. A client implementing the
contract this PR defines readsbody.error.code→undefinedand loses its backoff exactly
where the limit exists to slow repetition.
Good news on the same axis, verified end to end: the 429 + Retry-After chain on
/me/access/* is intact across all six links, with a test pinning it
(test/routes.access.test.ts:133-171). If B1 is fixed, clients will know to retry — no extra
work needed there.
Group 4 — Session and authority
G4-a · The policy is resolved 2× per request and the first result is thrown away.
authenticateExternalUserSession already resolves it and returns it in authentication.policy
(externalSessionAuthentication.ts:139-140), but validateExternalSessionToken stores only
claims and authorityContext (externalSessionAuth.ts:173-174) and drops policy. The
request field that carries it (externalSessionAuthentication) already exists in the type and
is populated by the sibling middleware 55 lines earlier. No memo, no cache. Since
validateExternalSessionToken guards all of /external/*, turning on catalogMode=shadow
— a de-risking step, not a serving step — adds a pool checkout and 5 statements to every
authenticated external request. One line (req.externalSessionAuthentication = authentication)
fixes this and removes B1's pre-limiter DB work at the same time.
G4-b · hasValidManagedMutationAuthority returns true when authority is absent.
membership.ts:1915 → if (!authority) return true, with authority? optional in all
mutation signatures. Not exploitable today — verified that all 9 production call sites pass
req.externalSessionAuthority, and the previously reported gap (resend/revoke missing it)
is closed: all 9 mutations now receive and use it, inside the same transaction as the
mutation. The cost is the next one: a mutation added in PR 2-4 that forgets the argument
compiles, passes the tests and authorises. git blame -L 1910,1918 → 58b62e953, Josué,
2026-08-28. Make the parameter required, or return false.
G4-c · Session validity is decided against two different clocks.
userSessionService.ts:170-177 compares against the Node process clock and, on the expired
branch, calls revokeLoadedSession → session_version + 1. The same two columns are evaluated
with Postgres NOW() in accessAuthorityStore.ts:116-117 and catalogAuthorityContext.ts:103
(read-only, no mutation). A pod with a fast clock evicts sessions Postgres considers perfectly
alive, and the +1 is terminal and self-unrepairing: even after clocks resync, the session
stays revoked. The skew is baked in at creation too — createUserSession:215-216 computes
expires_at with the Node clock for values later compared against NOW(), so two pods
disagreeing with each other is enough. Fix: derive now from Postgres in the same transaction.
G4-d · Every authenticated v2 request takes a row lock and writes.
validateUserSessionClaims runs with touch = true on the normal authentication path:
SELECT … FOR UPDATE OF s plus an UPDATE of last_used_at/idle_expires_at, per request.
N parallel requests on one session serialise on that row; with the action budget,
statement_timeout resolves to 1750 ms, so queued requests time out → 57014 →
AccessBudgetExceededError('deadline') → 503 authority_unavailable on legitimate traffic,
labelled as an authority outage when it was lock contention on the user's own session. Latent
today (sessionV2Issuance: false) but armed the moment issuance is enabled, because
sessionV2Acceptance is already true. Separate the read from the touch.
G4-e · password_reset capabilities are never invalidated, and no surface enumerates them.
Each request inserts a new 48 h invitation without revoking siblings, and completing a reset
marks only the row used. The 5/min limit is per email (not per IP) and the counter is a
Postgres token bucket shared across replicas — so 5 × 60 × 48 = 14,400 exactly, not an
optimistic bound. All four admin views filter purpose = 'member_invitation', so no admin can
see or revoke these. The related dead-end from an earlier round is closed, verified across
all five flows.
Group 5 — Rate limiting coverage beyond B1
G5-a · A dev merge silently deleted 11 rate-limit compositions. Verified commit by commit:
1efacc63 -> 11 (branch: d28-gfs adds them, 2026-08-13)
cf6f86af -> 11 (branch side of the merge)
5e20e560 -> 0 (dev side)
41078180 -> 0 (the merge: dev wins)
origin/dev -> 0 · HEAD -> 0
gfs_read/gfs_mutation survive in POLICIES as dead configuration, plus a table test that
blesses them (externalUserRateLimitPolicy.test.ts:31-32) — green forever, with or without
routes composing them. GFS is not unprotected: it keeps externalGfsIngressRateLimit and
friends at router level from dev, and its order (gfs.ts:408 limiter → :412 auth) is the
correct counterexample to access.ts. The defect is dead canonical configuration plus a test
that certifies it. Note: a two-dot git diff origin/dev...HEAD shows 0 added / 0 removed here —
net-zero precisely because the merge restored dev's file. Auditing intent requires walking
commits.
G5-b · /external/workflows reads and runs have no rate limit at all — 6 GET routes
(read.routes.ts:21,67,96, runs.routes.ts:49,84,105), each performing a Kubernetes LIST per
request via getAuthorizedRecipeResources. It is the most expensive external family and the
only one with no ceiling. Its sibling POST .../trigger does have one, but with authentication
before the limiter — the same inversion as B1. Also: authorisation runs inside each
handler, while the composable middleware already exists unused
(routes/workflows/shared/auth.ts:115).
G5-c · The edge applies no limiter to any of the three new /me/access/* routes, while
/me/gfs/*, /invitations/*, /me/teams and /auth/password-reset/request all do. No comment,
test or config documents the decision; the repo's own pattern contradicts it.
G5-d · The fail-open in rateLimitMiddleware was patched at the call site, not in the
mechanism. rateLimitMiddleware.ts:38-43 still does if (!key) { next(); return }. The PR
opened that file and did not touch the branch; instead the rule was implemented twice at call
sites (the 'missing' sentinel and an optional guard). 50 sites inherit the mechanism.
Group 6 — Path injection at the edge (pre-existing, amplified here)
memberManagementService.ts:57,94,101,130,138 interpolate req.params.* into the upstream path
without encodeURIComponent. REPRODUCED: a JSON body {"teamId":"../../auth/sessions/revoke-all"}
reaches .../external/users/auth/sessions/revoke-all with the service token attached, and
GET /api/v1/members/..%2F..%2F..%2Fhealth escapes above the /api/v1 prefix to /api/health.
Applies to GET, DELETE, PATCH and POST — the trailing path suffix does not protect, because ?
swallows it.
Bounded honestly. Requires a valid session (any account, no admin role) → High, not Critical.
Enumerated all reachable DELETEs: every one is already exposed by the edge with its own route and
authorises against the attacker's own x-user-session-token, so there is no privilege
escalation; the worst non-self-inflicted case checked (member retirement) additionally requires
reason and Idempotency-Key, which traversal cannot supply → 400. What the attacker gains is:
bypass of edge-only rate limiters, and an enumeration oracle over the internal surface, including
above /api/v1. Independently confirmed against control-api's route set: of 5 /external/*
routes with no public counterpart, none offers a non-self-inflicted escalation, because every
control-api route revalidates session, role and team on its own.
git blame → 5d663a49, Jose Ramirez, 2026-07-14 — pre-existing, not introduced here. Two
reasons it still belongs in this review:
- This PR amplifies it.
meService.ts:86-88changedcatch { return null }to re-throw
anything that is not a 404. The hardening is correct in itself, but it converts a blind
primitive — everything used to collapse to a generic 403 — into a response oracle: the
attacker now distinguishes 200/403/401/429/503/500 per probed path. - The asymmetry proves it is an oversight, not design. In the same file,
deleteManagedUser(:117) does callencodeURIComponenton the very same
req.params.userIdthat four sibling functions leave raw.
Medium and Low
Full detail is in the consolidated review; one line each here.
Medium (31). Shadow lane not certified on the path production actually takes (the only
caller passes no options; all 3 suites inject all 3) · live authorization judges index freshness
with a laxer criterion than policy does, and its SELECT doesn't even fetch last_success_at —
an unification left half-done, strict introduced a day after lax · budget exhaustion inside
exact validation is reported as an operational-store outage, leaving dependencyForError
inert · two catch blocks in the same /resolve flow treat budget exhaustion in opposite ways ·
deterministic conditions advertised to clients as retryable: true, so a client that honours the
flag retries forever · the resolver's final catch is mute and the file imports no logger, so a
TypeError is indistinguishable from a Postgres outage and the correlationId returned to the
user is never written to any log · CATALOG_FAMILIES declared twice with no type crossing — a
13th family would fail open at the serving readiness gate · REQUIRED_SOURCES duplicated,
which aggravates the above · withAccessDatabaseTransaction re-implements withTransaction's
state machine and breaks the "single blessed point" invariant of #375 M3 · the correlation-id
validator at the public edge is more permissive than the internal one, whose comment
explicitly says it rejects everything else "to avoid propagating attacker-controlled arbitrary
strings" · catalogProducers.ts keeps 6 per-family mappings and only 1 is exhaustive, so a new
family silently falls through to the GFS hydration SQL · the family→capabilities table is written
twice (verified not diverging today; capabilitiesForTeamRole already exists and the catalog
re-implements it by hand) · the 15 new realPostgres suites are outside CI's anti-rename
allowlist, whose own comment explains why that allowlist exists · the shadow scheduler's capacity
test never runs a deferred callback nor asserts shadowInFlight is released · let shadowInFlight
is process-wide module state reusing a per-request concurrency constant, and 2 of 3
skipped_unavailable returns don't record a metric · cursor validUntil produced with the DB
clock and judged with the app clock · /members/invite stopped bounding teams and name at the
edge and forwards the payload unbounded · the CSRF guard is mounted after the JSON parser ·
the edge normalises invalid catalog query params instead of rejecting them, turning a 400 into an
unfiltered first page · three different shapes for the same access-path handle across three
responses · the edge drops sessionContract and absoluteExpiresAt from renew · the ConfigMap
turns on a worker the code comments say to keep opt-in, depending on RBAC applied out-of-band ·
canCallerReadWorkflowRun has zero production callers and its db parameter exists only for its
tests · external_v1_session_revocations grows with no purge while the purge pattern exists twice
in the repo · formatApiError loses server detail for bodies without error/message · CodeQL
suite pinned to an exact stock version with no drift detection · codeql test run can pass having
run zero tests · isEvenfireFailClosedGuardNode doesn't require the return to leave the same
function as the sensitive action · ~85 sequential SQL round-trips per catalog page · a re-sorted
membership list per catalog item · two response serialisations per catalog response · four
canonicalAccessPathSeeds passes per item · up to 4 sequential Kubernetes GETs per /resolve.
Low (24). Dead code (compareResourceIdentity with localeCompare and zero callers,
catalogCoordinatorContractRevision, registeredCatalogFamilies, AuthorizationRequestMemo
never instantiated in production so memoEntries/memoBytes are dead budget) · 7 residual
localeCompare in the access core, each verified inert today · AccessSemaphore(4) never exceeds
1 in flight · a new file documented as a "compatibility facade" for imports that don't exist ·
mergeCandidates discards the direct candidate's validUntil · the coordinator drops the
producer's opaqueState · authorityRequired is an error field nobody reads, with three
contradicting defaults · fast-check properties without a fixed seed in the unit lane while the
realPostgres lane fixes one · the catalog performance test asserts no threshold · the
?? 'unknown' bucket key (verified unreachable, kept for contract consistency) · a
console.error for a runtime indexer death where rootLogger was available, while the twin site
was migrated in this same PR · Math.random() correlation id in control-api where the edge uses
randomUUID() · the invitation email is sent before authority is revalidated · createUserSession
locks the user row without lifecycle_state = 'active' · authGeneration weakened from required
to optional · run history disappears when a team loses a recipe grant, which contradicts the
aggregate-discovery pillar and needs a product decision · r15-b3's guard has only incidental
regression coverage · an intra-round flip-flop in the public correlation-id selector · six review
rounds and two finding IDs with no closing commit · the CodeQL source-scope check runs after
analyze, i.e. after SARIF upload · a silent boolean parser inconsistent with the fail-loud one
introduced in this same PR · profile-ui rollout skew, cosmetic.
What is clean — verified, not assumed
This carries the same weight as the findings above.
retry: 2→retry: 0with an explicit rationale. That was the mechanism painting two
deterministic reds green.- The round-1 Critical is closed in the right shape:
userAccessPolicy.ts:332-336throws at
startup if the catalog is enabled without its admission limit — a silent misconfiguration
turned into an impossible-to-ignore one. Covered byaccessCatalogConfiguredAdmission.test.ts. - Opaque access paths are genuinely signed, and better than that: HMAC-SHA256 over material
that includesauthorizationRevision— session contract and revision, user revision, all
active memberships sorted, resource/source-state/relationship revisions and the canonical
candidate set — compared withtimingSafeEqual. Stronger still: the server does not verify
the MAC of the id it receives; it recomputes the principal's current path set in-transaction and
looks the requested one up, so a perfectly signed id with no remaining authority is rejected
anyway.principalUserIdis inside the signed value, so A's id never appears in B's set. - Fail-closed budget:
BudgetCounter.takenever partially consumes,chargethrows rather
than truncating,child()rolls back every reservation if any counter fails,hydrateSelected
throws rather than emitting an incomplete page, and Postgres57014becomes
AccessBudgetExceededError('deadline')with the backend cancelled over a separate connection. - Cancellation is real, not declarative: the
AbortSignalreachespg_cancel_backend, and
statement_timeoutis derived from the remaining deadline minus 250 ms. - Zero placeholders across 16,207 added production lines: no
TODO/FIXMEas implementation,
nonot implemented, nofoo/bar/example.com, no emptycatch {}. - Zero production code shaped to pass a test: no
NODE_ENV === 'test', notestMode, no
hardcoded value matching an assertion. The threeNODE_ENVhits are log level and tracing. - Zero failure-masking in tests: no
.only,xit,xdescribe,continue-on-error,
|| true,set +e, or--passWithNoTestsanywhere in the repo. - The test-first protocol is real, and it changes how "fix without test" should be read:
146 of 173 commits carry aRepro-test:line naming the test file and, in most cases, the
exact SHA where it is red. Across 94 closed finding IDs, exactly one fix lacks its own
regression test (r15-b3, Low). A naive count of "fix commits with no test file in the diff"
would badly misrepresent this. - The merges did not lose work, with one exception. Two independent detectors over 37 merges,
16 candidate files, each audited by symbol against HEAD: 15 restored, re-implemented or
deliberately renumbered. One merge (41078180) concentrates 12 of the 16; the other 36 lost 4
benign lines between them.safeForwardableResponseHeaderscame back stricter than what was
deleted, and it is branch work, notdev's. - Migration design is one of the strongest parts of this PR: additive, idempotent, fail-fast,
withlegacyVersionsso a database from earlier review rounds converges without re-applying. - The realPostgres CI lane is exemplary:
set -euo pipefailinside therun(so the| tee
cannot mask the exit), then it requiresTest Files ≥1 passedandTests ≥1 passed, and
fails if anyskippedappears. That iszero-tests-is-never-successdone right. The
describe.skipon the 48 realPostgres suites is not a fail-open:realPostgres.requirement.ts
is a globalsetupFilesthat throws whenCONTROL_API_REAL_PG_REQUIRED=1without a DSN. - CodeQL's public/private boundary is genuinely restored (
R16-B1/R16-B2close for real):
dev's globalexclude: js/system-prompt-injectiondisappears and is not replaced; no
lgtm[...]/codeql[...]suppressions; and CI asserts the stock query resolves by id. Both
fixtures contain real negative controls, so neither query is a no-op. - Typecheck is clean and that was proven, not assumed:
npx tsc --noEmit→EXIT=0for both
services, validated with a positive control (a deliberate type error →EXIT=2with a
diagnostic), so the empty log means a clean pass and not a command that never ran. - No SQL injection on the new surface: every
${}in access-layer SQL is a server-generated
constant; the client'sfamiliesgoes through an allowlist againstCATALOG_FAMILIES; every
arm carriesLIMIT $4. - The v2 token carries no authority claims at all — normalisation forces
teamId: null,
role: 'member'. RS256 with the algorithm pinned, public key derived from the private one
(HS256 confusion impossible), issuer/audience validated twice, strict claim allowlist. - Mass revocation is atomic with no issuance race:
revokeAllUserSessionstakes
SELECT … FOR UPDATEonusersbefore writing the epoch, the same lockcreateUserSession
takes, so they serialise. Credential changes revoke sessions on all five paths. - Google login requires a verified email and cannot be bypassed: one call site, audience
pinned, throws whenemail_verified !== true. - No N+1 over memberships anywhere on the hot path: the GFS team fan-out groups
2·Mstreams
into one statement viajsonb_to_recordset+CROSS JOIN LATERAL; the principal snapshot
resolves session liveness, all memberships and all revisions in one statement; hydration is
one statement per family, keyed by arrays. Zero Kubernetes calls on the catalog path. - No V1↔V2 copy-paste: a 9-line duplicate-block detector over all 268 changed files found only
5 distinct file pairs, all investigated. The briefing's suspicion is not confirmed. - No new dependencies: manifest and lockfile diffs are version bumps only. No new package, no
new post-install script. - No secrets anywhere in the diff or in
deploy/; the only hits are inside the synthetic
fixture thatcodeql-config.ymlexcludes. compareCanonicalUtf8Textis the PR's positive counterexample: 9 consumers across 8 files,
zero inline re-implementations, and its Postgres equivalent (catalog_utf8_bytes) is correctly
IMMUTABLE STRICT PARALLEL SAFE.- Branch naming compliant; the Electron rule untouched; the minikube T0/T1/T2 contract untouched
(new fixtures use throwaway repos with their owngit init, never touching the developer's HEAD).
Verdict on the PR body's own claims
| Claim | Verdict |
|---|---|
| "fail-closed work exhaustion" | HOLDS — though in the shadow lane that same fail-closed is what produces skipped_capacity |
| "no skipped/duplicated canonical resources" | DOES NOT HOLD — B2 |
| "stable ordering / private request-local producer state" | HOLDS |
| "bounded database/Kubernetes work, deadlines, cancellation" | HOLDS, with a reservation — the mechanism is real; two paths don't receive the budget |
| "physical SQL statement accounting" | DOES NOT HOLD (partial) — liveTeamAuthorization.ts:14 queries pool directly: unaccounted, no statement_timeout, no cancellation, and it runs on every :teamId route |
| "producer-backed evidence across all twelve families" | DOES NOT HOLD — gfs_resource is always skipped_capacity, no test runs a real producer under the child budget, and no producer has ever run in a deployed environment (B4) |
| "catalog presence is discovery only; actions authorize again" | HOLDS |
| "legacy V1 compatibility remains live-authorized" | HOLDS |
| "explicit bounded rate-limit policies, canonical public retry/error behaviour" | DOES NOT HOLD — B1, G5, Group 3 |
| "later-stage capabilities remain disabled by default" | LITERALLY TRUE, SUBSTANTIVELY MISLEADING. The 7 flags are false/off — but none of them reaches deploy/, they exist only as code defaults; enabling shadow crash-loops a cold start and serve cannot start at all. It is not "disabled by default", it is un-enableable. And the claim covers only the read plane: 17 row triggers across 15 hot product tables are active from the first deploy regardless of any flag, and the one access-family setting that does reach a manifest is CONTROL_API_OPERATIONAL_ACCESS_INDEXER_ENABLED: 'true', against a code default of 'false' |
| "admission/work limits remain activation inputs" | NOT VERIFIABLE — a claim about the provenance of future values. What is checkable does fit; but the hard clamps are constants chosen with no documented measurement, and one of them is exactly what breaks the validation lane |
"exact pushed head 1ccf7f0e" |
STALE, impact bounded. Real head is 8d541772; the 5 intervening commits are all dev merges (20 files, none owned by this PR), so CI 33409641472 and CodeQL 33409641558 did not run on 8d541772 — although the PR's own diff is identical between the two SHAs. The companion claim about the merge base doesn't hold either: git merge-base origin/dev HEAD = 24d749d0 |
Suite status
| Lane | Result |
|---|---|
control-api run 1 |
1 failed / 5,273 passed / 241 skipped — EXIT=1 |
control-api run 2 |
3 failed / 5,271 passed / 241 skipped — EXIT=1 |
control-api run 3 |
0 failed / 5,274 passed / 241 skipped — EXIT=0 |
control-api run 4 |
2 failed / 241 skipped — EXIT=1 |
external-rest-api |
38 files, 232 tests, green and stable |
tsc --noEmit, both services |
EXIT=0, validated with a positive control |
| PR CI | 34/34 green on this exact head |
Four identical runs on the same head, six different failing tests, none repeated. Symptoms
(Parse Error: Expected HTTP/, 404 where a route exists, crossed 400/201) are the signature
of an ephemeral supertest port being recycled — the request lands on a server that is no longer
that test's. routes.adminWorkflows.test.ts passes 112/112 in isolation. The comment at
vitest.config.ts:7-9 claims fileParallelism: false prevents "nondeterministic HTTP parse
failures"; it does not.
Stated as an observation, not a finding, with two caveats: the machine was under 17
concurrent agents, and PR CI is green on this head. Of the 6 flaky files, this PR touches one
(workflows.shared.rateLimit.test.ts, which issues 60 sequential supertest requests per
parameter). One of the six is a genuine pre-existing test bug rather than contention:
oauth.brokerJwt.test.ts:59 does token.slice(0,-2) + 'XX' on an RS256 signature whose last
base64url character carries only 2 significant bits, so the "tampered" token is byte-identical
1 in 200 times — measured over 5,000 samples, after refuting a first hypothesis over 200,000.
git blame → 5d663a49, pre-existing; it was masked by retry: 2 (0.5%³ ≈ 1 in 8 million).
Removing retry: 2 was the right call — it exposed this rather than causing it. The two
populations need opposite fixes and should not be treated as one issue: population A is fixed
in the tests' transport (Connection: close, or one app.listen per file instead of an ephemeral
server per request), and nothing in production needs changing; population B is fixed in that one
test and nowhere else.
The 241 skipped tests reconcile exactly, and nothing vanishes silently — measured, not
inferred. 47 Postgres files run alone give 2 passed | 238 skipped (240);
network.gateway.intent + network.k8s.integration give 41 passed | 3 skipped (44).
238 + 3 = 241 and 46 + 1 = 47 skipped files, identical to the full run. The 3 non-Postgres
ones are gated on CONTROL_API_K8S_INTEGRATION and on a deploy/scripts/gcp-detect-k8s-api-ip.sh
that isn't present in the worktree. Because the totals reconcile test-by-test, no other file
contributes a single skipped test.
One extra Low from the same lane: external-rest-api/vitest.config.ts does not set
fileParallelism; the sequencing guarantee lives only in the npm script. npx vitest run invoked
directly → EXIT=1 with three failures; the same run with --no-file-parallelism → EXIT=0,
232/232. Move the setting into the config, as control-api already does.
Corrections this review made to itself
Listed because a review that doesn't declare its own errors isn't auditable.
main.ts:471→main.ts:123. The number came from diff-hunk numbering, not the file (214
lines). The substance stands.- B5's actor is the migration Job, not pod startup. The mechanism is unchanged; the wrong
actor would have let the whole finding be dismissed in one line. - B1 has two figures, not one: 15 statements is the rollout target; today it is 5. One agent
first said 17 and corrected itself by measuring. - B1 is authenticated abuse today. One agent inferred an unauthenticated primitive; another
measured a garbage token at 0 statements and corrected it. Publishing the stronger claim would
have been refutable in one sentence. /capabilitiesresolves the policy 2×, not 3× — an agent retracted its own figure after
confusing two similarly named middlewares, and also withdrew a timer-leak claim after verifying
the budget is always closed in afinallyand its timer isunref()ed.- The edge path injection was downgraded from "SSRF with escalation" to "path injection with
rate-limit bypass and enumeration" after the real damage ceiling was measured. - The duplicated capabilities table was downgraded from functional to maintainability after a
set-by-set comparison proved the two tables do not diverge today. - The 413/429 mutation results and the 413 coverage reports were reconciled, not averaged:
both were right about different code paths. - That reconciliation was then itself corrected. The first explanation — "the fallback table
has no coverage" — was only half of it. A 2×2 truth table showed the second half: for 413 both
tables carry the same string, so breaking either one alone is invisible. The proposed
"decisive" mutation (emptying the allowlist) came back green, which is what surfaced it. - A merge-loss finding was nearly discarded on a bad measurement of mine — in zsh,
"$c:control-api/…"applies the:cmodifier to the variable, soHEADresolved to
/usr/bin/HEAD. Re-run with correct quoting, the finding held. A second agent's own
measurement (git diff origin/dev...HEAD→ 0 added / 0 removed) was also correct and
compatible: net-zero precisely because the merge restoreddev's file. - Discarded after verification:
routes.externalAccess.test.ts:186does not bless a bug;
browserMutationGuard.ts:12'scatch { return new Set() }is fail-closed; a suspected
missingCREATE TRIGGERonteam_memberswas false (they are created in aDO $$block);
the client-IP forwarding hypothesis did not survive checking all five routes.
Declared limitations
- CodeQL was never executed (no CLI available). G1-b rests on the model's design and on the
author's own certified fixture, not on a run. - Real Postgres was never executed: the 48
*.realPostgres.integration.test.tssuites (15 new
here) were skipped. B2 and B3 depend on premises read from SQL that an integration test would
close. - Two risks flagged but NOT raised to findings, both needing real Postgres: the relationship
set incatalogHydrationSql.ts:163-198doesn't check the target type while
operationalAccessReader.ts:200-219does — alogical_idcollision across types would make
every catalogaccessPathIdpermanentlyaccess_path_stale, an infinite client loop; and the
relationship CTELIMITs have different scopes (global in the catalog, per-resource in the
resolver), so revisions would diverge under truncation. - One agent noted it did not read four large files end to end
(userAccessFoundationSchema.ts,accessAuthorityStore.ts,membership.ts,
catalogHydrationSql.ts) and said exactly which parts it did read.
|
@zach88 thanks for the round-3 review. Status of every item at the new head (each was checked against the code first; where the premise did not hold we say so): Fixed in code
Corrected premise
Documented in the description (accepted in writing)
Moved to issues
Later commits: Validation at head |
…ss-foundation Dev now ends at 0136_llm_catalog_dismissed_backfill (#1285), which took slots 0131 and 0132. No shared environment has applied the PR1 migrations, so they move above dev's last version with their names unchanged, as the procedure in migrationRunner.ts requires: - 0137_user_access_foundation - 0138_legacy_password_security_epoch_backfill The dev batch bound in initDb gets its own constant, LAST_DEV_BATCH_MIGRATION. PASSWORD_SUCCESS_ADMISSION_MIGRATION keeps identifying the password rollout. A branch profile that recorded the old 0131/0132 receipts is an earlier head: the migration runner refuses it until it is recreated or cleaned with docs/deploy/sql/user-access-foundation-earlier-head-cleanup.sql. Other conflicts: - ci-public.yml and password-recovery-suites.json keep both suite lists. - The runtime access contract keeps the peer-derived check and adds dev's two new relations as sentinels. workflow-recipes gets those relations as `none`, because the contract requires the same relation names in both profiles. - The upgrade guide states the new numbers and that a database below 0130 now applies 0129 through 0136 in the same transaction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The merge of dev moved the foundation migration to 0137, so the literal 0131 version in the upgrade guide no longer names a registered migration and migrationReferences.test.ts rejects it. The sentence keeps the same fact with the prefix and the migration name written separately. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The banner told a signed-in member "this session has no active team". This change starts retiring the team id from sessions, and a session without a team works in every section, so the banner reports a normal state as a problem. The E2E seed also created users without a membership: POST /admin/teams ignores userId and creates only the team. It now adds the member with a second call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A 429 from password login showed "Login failed. Check your email and password." The server did not verify the password on that path, so the text sent people to fix a password that may be correct, and it offered no wait. authClient appends the transport's status and Retry-After to the error message, because only the message crosses IPC. The sign-in screen says that there were too many attempts, that the password was not checked, and how long to wait. The check runs before the invitation checks, which match 404 and 410 anywhere in the message: a wait of 404 or 410 seconds would otherwise read as an invitation failure. A rate-limited login also skips the backend switch hint, because the backend answered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dedicated lane runs against a seeded branch profile. It stops control-api, so it stays out of the default suite and refuses any other context: - a member without a team and a team member see no team notice in any section; - an outage keeps the session, and a real session end returns to sign-in; - a rate-limited sign-in explains the wait and does not blame the password. The shared password pace moves from 8 s to 13 s. The source limit allows five attempts per clock minute and counts correct passwords, so 8 s between attempts could reach it. The helper comments now state the limits from #1197 instead of the retired five-per-identity budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ss-foundation Dev brings no new migration, so the PR1 versions stay at 0137 and 0138. The only conflict is in desktop-app/package.json, which keeps both new scripts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bounds script grepped the old 0131_ prefix: grep -c exits 1 on no match and set -e killed it with an empty log, so CI skipped every later step of that job. It now counts the plan's migrationVersion entries with awk; the version itself is enforced by the literal type in pr1OnlineIndexPlan.ts. LAST_DEV_BATCH_MIGRATION in db.ts and LAST_DEV_MIGRATION_VERSION in migrationRunner.ts stated one fact; if they disagreed the Job would exit 0 with migrations missing. db.ts now imports the runner's constant. Comments say that a dev merge moves the bound and the latestDev producer. The policy test compares the full version instead of the 0137 prefix, the shape the merge sweep missed. The earlier-head comment no longer calls the old user_access_foundation prefix "the same 0131 version". Orphans checked: LAST_DEV_BATCH_MIGRATION had one reader, db.ts's batch loop, which now reads the shared constant; nothing else referenced it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…delta The upgrade tests hand-wrote the receipts between an immutable start and a moving head, so dev's six migrations made them expect 137 receipts against 143. They now derive the pending versions from CONTROL_API_MIGRATIONS and assert the apply order equals them, with no hand-written count. The latestDev producer moves to origin/dev's tip, 31abc80, because that is where the migration starts. Its test fails with "latestDev is stale" when a dev merge brings migrations and the pin is left behind, and asserts that the PR1 migrations commit in separate transactions. observedDev stays pinned: it is the only real producer that reaches the password-login fence. Earlier-head receipts under 0131 and 0132 now have exact-message tests, with dev's own migrations under those numbers as negatives. The upgrade doc drops the relation row count and adds the 0130-0135 start. The runbook states what each pin means and how to re-point an existing latest-dev worktree. The runner's retirement clause names these tests. Not covered by a real run: a start at production's recorded 0129 and a rollback with a pre-0130 migrator. Orphans checked: DEV_POST_0106_THROUGH_0129, OBSERVED_DEV_RECEIPT_COUNT and LATEST_DEV_RECEIPT_COUNT had no reader outside the removed assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A MODELS_DEV_API_URL with userinfo passed the guard. fetch refuses such a URL with a message containing it, and the client logged that message as the reason, so the credential reached the log and the catalog stayed on vendored data. The guard now rejects userinfo before any fetch, which also stops boot, and a malformed URL is no longer echoed in the exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Desktop password seed cached on a SHA-256 of a password that was already in memory. The cache now stores the password per user and email, so a different password still reseeds and no password reaches a hash. waitForNewRun built its poll message once, before the callback could record a transient error, so that suffix could never show. The callback now returns what it saw and the matcher compares it, so the last state appears in the poll's own failure output. Also removes four unused symbols from test files. Orphans checked: createHash had no other user in workflowUi.ts; lastTransientError, rootLogger, config, SIDEBAR_COLLAPSED_KEY and the one resolveCount had no other reader in their scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arker The sign-in screen read a marker that a response body could imitate, called every 429 "too many sign-in attempts", told the user to check the password during an outage, and matched 404 and 410 anywhere in a message. AuthClient.passwordLogin now removes any marker from the message of every failure and appends the transport's own: httpStatus plus, for a 429, a strictly parsed Retry-After, or signInTransport=noResponse when there was no HTTP answer. The renderer reads it anchored at the end of the message, so body text cannot be in that position. The texts: 401 stays "Email or password is incorrect."; a 429 says sign-in is paused and the password was not checked, which holds for all its reasons; a 5xx or no answer says sign-in isn't available and keeps the backend probe; everything else keeps today's text and the probe, including a failure after a good password, which has no marker and can no longer read as incorrect. The 429 and outage toasts use the default error duration, like the generic one. Desktop setup recognizes only the invitation_config_not_found code main raises. Orphans checked: isInvitationExpiredError and isUnauthorizedError had no other caller. Their texts and checks are gone from the password path; no route on it returns invitation_expired. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hanges App outlives a session and only the close handler cleared the plugin GFS image preview, so it survived a session end, reappeared for the next user and refetched the previous user's file under that session. It now resets in the effect that resets the workspace tabs, on sign-out or another principal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… profile The lane stops control-api, yet it defaulted to and accepted the shared clerum-test context, had no opt-in, and its fresh config skipped global setup, so nothing validated the URLs. The config now extends the base one and requires E2E_SESSION_TEAM_UX=1, an explicit E2E_K8S_CONTEXT that is a branch-owned local profile, and an explicit EXTERNAL_REST_API_BASE_URL, the one variable the spec reads. The base config is imported first because global-setup reads E2E_PROD_CONTEXT from the canonical env at load. Memberships were deleted and rebuilt by a naming convention, which lost roles and timestamps on a non-unique team name. The lane now snapshots the full rows, restores them in one transaction and asserts equality. The password restore runs even if closing the app throws, and the outage restores the replica count it read. Orphans checked: setMembership had no other caller; its convention inserts at the start of two tests are gone, and the witnesses that follow them fail on an unseeded user. isLocalPortForwardContext stays, used by global-setup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /admin/teams reads only the name, yet two E2E helpers also sent userId, so those teams had no member and a later switch-team would get 403. Both helpers now send the name alone and make the membership call, as the seed script does, and the script helper's test runs in CI. Because the membership now exists, the two approval-bound tests remove the team in their finally blocks, and cleanupRecipe failing no longer skips that. The cleanup moved from the figure-b test to the shared recipes module. It deletes the membership in its own call, so a failure deleting the team cannot roll the membership back onto the shared E2E user. Whether those lanes pass is still decided by their own run. Orphans checked: the local cleanupTeam in figure-b had one caller, which now imports the shared one; sqlLiteral and runProfilesSql stay used there. The other cleanupTeam in sandbox-ui-apps-catalog-authz is a separate helper, unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nothing in the runtime deletes from external_user_session_security_epochs, and the user-deletion cascade runs as the table owner. The grant and the access profile move it to upsert now, before this migration has a receipt on a shared cluster; afterwards the change would need a new migration. The sessions table keeps DELETE, which the session service uses. The byte-for-byte pin on the migration body moves to the new file hash. A branch profile that already ran this 0137 fails the access check and needs a fresh profile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The public boundary scanner rejected two literals that look like credentials and do not say they are synthetic. The job reached them only after the migration bounds step stopped failing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The quadrant recipe wrote its instruction into a double-quoted YAML scalar with only quotes escaped, so a backslash or newline changed or broke the value. JSON.stringify writes a valid YAML double-quoted scalar for any string. The global setup's outer loginBody held a value nothing read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude Please do the closing review of this PR before it merges, and end with a clear verdict: APPROVE or CHANGES REQUIRED. Your workflow token cannot submit a GitHub approval, so state the verdict in your comment. Pinned state
Scope Focus on what changed after
The checkout has depth 1, so fetch these objects first. If you cannot obtain them, say so and state what you did not review; do not approve on a partial read. Phase context Judge this PR as a phase, not as the end state. The plan removes the selected team id from user sessions:
Code installed turned off is covered by activation gates #1222, #1223 and #1224. A gap that only matters at a later activation belongs there unless this PR makes it harder to close. What to check
Validation already run on this head, for you to weigh T0 pass; T2 Already tracked, no need to report again #1381, #1382, #1385, #1363 (fix in #1374), #1251, #1266. Output For each finding: severity, |
|
Claude finished @alfredolopez80's task in 11m 19s —— View job Closing review — PR #334 @
|
Merge dev at d02fcb9, which carries PR #334, into this branch. PR #334 removed the access catalog, the operational index and indexer, the live resolver and the authorization revision counters, which this branch builds on. Does not compile: restored by the next commit. 19 production and 28 test files in control-api import modules that dev deleted; the next commit takes over the action authorization modules from PR1. Resolutions: - dev wins on what PR #334 changed: migration runner and its guard, online index plan, user access policy and runtime policy, CI producer pins, external-rest-api error forwarding (M-10), migration tests; - the 25 files dev deleted and this branch modified are removed here and restored where the plan needs them in later commits; - this branch keeps its PR2 readiness evidence, workflow authority bindings and approval authority checks; the operational access indexer, the catalog shadow hook and the authorization prelocks go; - versions above both parents for control-api, external-rest-api, host-context-controller, mcp-host and workflow-recipes; - password recovery suite manifest: dev's suites plus PR2's five. Removed: scripts/tests/task106-r61-b1-producer-checkout-contract.cjs, which checked the literal producer pins that dev replaced with certification inputs. Orphans checked: scheduleAccessCatalogShadow, OperationalAccessIndexer, startOperationalAccessIndexer, authorization_prelock_, sanitizeControlApiPublicError, task106-r61-b1-producer-checkout-contract. Removed: all except sanitizeControlApiPublicError in external-rest-api/src/routes/rpcDelegations.ts, which the next commit replaces with the M-10 forward. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR1 — user-session and live-access foundation
PR1 of the Task 106 plan to remove the selected team id ("Team Token") from user sessions. It installs the
backend foundation for user sessions that do not depend on the selected team, and checks team membership live
on the server instead of trusting the team and role in the token. Everything that changes what a client sees
is installed off by default; supported V1 / team-switch clients keep working as they do on
dev.Stack
dev, merged up to31abc80d8(includes fix: support 40 successful password logins #1197,password_login_success_admission, feat(auth): extend V1 session defaults to seven days #1356, theseven-day V1 session default, and
dev's migrations through0136_llm_catalog_dismissed_backfill).feat/user-centric-runtime-access-paths) builds on this PR. It still carries the old PR1migration map and limits that this PR returned to
dev, so it has to be reconciled with this headbefore it is rebased (see Migrations).
control-api migration Job now stops on a database that recorded them (see Migrations).
this PR.
Merge gate
This PR must not merge into
devbeforekeyper-labs/evenfire-infra#154(the change forkeyper-labs/evenfire-infra#143) is on the infra repository'smainand a dev deploy log shows itsEDGE_PHASE applyandEDGE_PHASE settledlines: External REST and Profile UI Ready, with no pod of an olderReplicaSet left, before control-api rolls out. See Deploy and rollback.
#154merged on 2026-11-06. The lines are inthe automatic dev deploys since then, in the required order (
jwt-check,apply,settled, validators):keyper-labs/evenfire-infra runs 42144889952 (second attempt), 42257859403 and 42292068908. Those deploys did
not change the edge images. A2 in
#154checks the same lines on the first deploy of this PR, whereimagesmustlist
external-rest-api,profile-uiandcontrol-api.What this PR installs
(
CONTROL_API_USER_ACCESS_SESSION_V2_ISSUANCE=false) and cannot be turned on before PR3 (Task 106: fix before activating the access catalog or V2 session issuance #1222). V1issuance, acceptance and the legacy team switch stay on
(
CONTROL_API_USER_ACCESS_LEGACY_LIFECYCLE=issue_and_accept,CONTROL_API_USER_ACCESS_LEGACY_SWITCH_ENDPOINT=true).rpc/tokenroutes:control-api reads the current
team_membersrow on each request. No revision tables, triggers orSECURITY DEFINERfunctions are installed.(
external_v1_session_revocations, append-only for the runtime role); a password change moves asecurity epoch that invalidates earlier sessions.
Retry-After, and on a 503 control-api'sCache-Control: no-store. Ondevthe edge kept a control-api4xx status but replaced the body with the internal error message, and turned any 5xx into a 500 carrying
that message (security(external-rest-api): do not expose upstream error details in public 500 responses #190). An edge-side 4xx keeps
dev's message; any other unhandled error now answers asanitized 500 envelope.
buildUrlrejects dot segments,?and#in the path,and caller-supplied path parameters are encoded, so a crafted segment cannot reach another
control-api route.
Originor aSec-Fetch-Siteheader must come from an origin listed inEXTERNAL_REST_API_CORS_ORIGIN(behaviour change 8).appService.ts,main.ts,preload.ts,renderer.d.tsanduseAuthController.ts(behaviour change 7). The GFS grant error parser is unchanged fromdev.Earlier heads of this PR also installed revision tables with triggers, an aggregate access catalog
(
/me/access/catalog,CONTROL_API_USER_ACCESS_CATALOG_MODE), a live access-path resolver and an operationalindex. They were removed in
f0e8a49ad,64e27bfac,eb1f52f71and522049e07; the variables are ignored(see the deployment note).
Behaviour changes compared with
devThese are intended and approved by the owner:
team_id/usage_team_idstay NULL).A direct path has direct attribution under Task 106 (AD-06, "Direct path cannot inherit UI team").
Consequence: team-scoped budgets no longer cover direct-grant V1 runs. Usage is still recorded and
budgeted by user, recipe, Host and LLM secret.
in with the new password (guided re-authentication).
POST /api/v1/invitations/passwordsets only the first password; with a password already set itanswers 409
password_already_set.without creating a session. The admin Desktop-invitation completion does not overwrite a password
when its acceptance was a replay.
Profile UI gets no answer to logout, it fences the session in the browser; if the browser cannot
store that fence, the page now says the sign-out is incomplete and offers to retry.
Logout revokes only the token it receives. A V1 token issued before a team switch stays valid until its
own expiry (at most 7 days), as on
dev; to end it, sign out with that token or change the password.A logout that commits while
/external/rpc/tokenis already past session validation (during its livemembership, agent and recipe-access reads) does not stop that mint; the RPC token it returns is valid
for its TTL (300 s by default,
CONTROL_API_RPC_TOKEN_TTL_SECONDS). Ondev, logout revoked nothing.A revocation read just before the mint would narrow this window but not close it, since the logout can
still commit after the read. The GFS mint reads the revocation in its mint statement (R2-L20).
/external/auth/*and/external/rpc/token):session-token, used by team switch) requires a current user session;a revoked, retired or stale session gets 401 instead of a new token.
rpc/tokenchecks the live team membership and role instead of trusting the JWT's team androle claims.
live membership and the SharedFileSystem routes answer a typed 503 with
Retry-AfterandCache-Control: no-storeinstead of a 500 that exposed the upstream message. Other errors stay asanitized 500.
session-token,verifyandrpc/token(its bodysessionToken), a session token, or a tokenor claim field in the request body, that is not a string gets the same answer as an empty value
instead of a 500.
google-loginkeepsdev's handling: anidTokenthat cannot be converted to astring still answers the sanitized 500.
session_issuance_temporarily_unavailablewhen theuser's security epoch would reject the new token at once.
(PR3); the edge does not forward a client version yet (PR4).
The verify body and the
session-tokenrole check are the same as ondev.second time. A Google account whose email is missing or not verified is refused with
401 {"error":"Unauthorized"}, the same answer as a retired account (ondevit signed in).Invalid or expired tokens still answer 500, now with the sanitized envelope instead of the
verifier's message and token claims, and the logs no longer contain the token or its decoded
identity.
DELETE /external/teams/:teamId/members/:userId) requires the user id incanonical form; any other form gets the managed route's 400
invalid_target.4e30c600e,43fc6e2fd,1ec1fa8d7): when/merejects the storedsession (401, 403 or 410, as on
dev) Desktop removes the stored token and tells the window ("Yoursession ended", new
auth:sessionClearedIPC); Desktop asks/meafter a 401 from a team call inlistWorkflows, when the entity-changes stream expires and in the team-context lookup. Ondevthe401/403/410 rule covered only the startup restore; here it also covers these three probes. No External
REST route sends 403 or 410 on
GET /me, so only an intermediary (a WAF or CDN) could end the sessionthat way; accepted. A workflow listing fails as a whole when a
team call answers 401 or the session changes during the listing (
stale_auth_epoch) instead ofskipping that team; a failed deliberate team switch is reported to the window.
Originlisted inEXTERNAL_REST_API_CORS_ORIGIN, and aSec-Fetch-Site, when present, ofsame-originorsame-site.Profile UI reaches External REST through a same-origin rewrite, so that variable must list the public
Profile UI origin; otherwise Profile UI login, logout, password reset, invitation accept and
set-password answer 403. This is a deploy precondition (see Deploy and rollback).
channels, orders by display name,escapes
%/_and answers 400 above 100 characters;/external/invitations/pendinglists the caller'sown invitations and ignores
?email=; an inviter who is not an admin cannot invite an admin (403) andthe response no longer carries the invitation
token; renaming a team needs a live admin membership; abrowser team switch sets the session cookie and does not return the token in the body; the
workflow-recipes runtime access verifier checks every table and column privilege (7 -> 106 table rows,
0 -> 6 column rows; each file also has one header comment line).
(
7045c3fd4,2ca0f5371): the session has no team until the invitation is accepted.GET /mewhen control-api answers a bare 401 answersdev's404 {"error":"User not found"}.The old control-api sends that bare 401 for an ended session, during the rollout window and after a
rollback. This PR's control-api answers an ended session with the
invalid_sessionenvelope, whichExternal REST relays as 401. A rejected External REST service credential (for example a misconfigured service secret) also reaches
the edge as a bare 401, so
GET /meanswers the same 404 and writes no log, as ondev, where everycontrol-api error on
/mebecame a 404. The two bare 401s cannot be told apart at the edge, andExternal REST has no structured logger to record it.
GET /team, team rename, member role change, memberremoval and
POST /me/switch-team) answers control-api's 401 instead ofdev's200 {"team":null},404 or
403 {"error":"You are not a member of this team"}(3a7465283). The sessions this PR canend while the JWT is still valid (logout revocation, password change) would otherwise be reported as
"no team" or "not a member". A refused External REST service credential also reaches these five routes
as a bare 401 and now answers 401;
GET /mekeeps its 404 (behaviour change 11).Everything else is meant to match
dev. Rate limits are the same as ondevfor every route that existsthere; each new route with a
devequivalent carries that limit.session/renew,session/logoutandsessions/revoke-allhave nodevequivalent and no frequency limit (#1200).Migrations
Migrations are listed by name. Each version string keeps a numeric prefix because
schema_migrationsstores it, but the prefix is assigned when a PR merges and says nothing about which PR comes next:
later PRs take whatever prefix is free when they merge, not the next number after this one.
Base. Every
devmigration up to and including0136_llm_catalog_dismissed_backfillruns ininitDb's single transaction, as ondev, with #1197's rollout setting andsynchronous_commit. Ondevthe #1197 admission fence waited with a 60 s lock timeout under the pool's 15 s statement timeout; here
the migration session sets its own 15 s bound, so the fence transaction raises both to 60 s.
Applied by this PR, after that base, as
0137_user_access_foundationand0138_legacy_password_security_epoch_backfill:user_access_foundationexternal_user_sessions,external_user_session_security_epochsandexternal_v1_session_revocations, and grants their runtime privileges. Its five indexes on tables that already exist (team_members,user_contexts,team_contexts,user_agents,team_agents) are built online, before the migration transaction opens. An existing index is accepted only ifpg_get_indexdefmatches the expected definition exactly.legacy_password_security_epoch_backfillpassword_set_at, so sessions issued before the last password change stop being accepted.Neither installs triggers or revision functions, and neither takes
AccessExclusiveLockon an existingtable.
Removed from this PR and not carried by any later PR. Earlier heads of this PR also had
invitation_delivery_commands,catalog_utf8_orderingandcomposable_catalog_revisions. The catalog and theinvitation delivery command table were taken out of this PR, and the revision tables were removed from
every PR. The migration Job stops before
0137on a database that recorded any version written by anearlier head of this PR, or that still has its
authorization_bump_*/authorization_prelock_*functions,catalog_utf8_bytesor its revision and catalog tables, and its error names them. The deployment notegives the check query and the cleanup script
(
docs/deploy/sql/user-access-foundation-earlier-head-cleanup.sql).Left for the next PR (#594). #594 carries its own migrations:
workflow_authority_bindings,gfs_upload_authority_bindings,pr2_readiness_evidence,pr2_runtime_privileges,workflow_recipe_authority_entity,workflow_run_failure_reason,r31_runtime_behavior_sourcesandworkflow_authorization_pause. It still lists this PR's earlier migration set; when it is reconciledwith this head it drops the three removed migrations and keeps the two above as they are here.
Execution.
300 s per Job, 360 s client wait.
--timeoutnow sets only the client wait; the Job deadline is thefixed 300 s policy. The policy file is copied next to
deploy/scripts/run-control-api-db-migration.sh,because the infra repository copies only
deploy/andscripts/.client_connection_check_interval, so a killed Job pod does not leave itsbackend holding the advisory lock (PostgreSQL 14 or later).
precondition; an attached caller waits for the Job's deadline. A terminal Job left by an earlier run is
also deleted with the UID the script just read; a Job another caller created under the same name stops
the script instead of being deleted (Migration script deletes a "terminal" Job by name, which can hit a newer running Job #1226).
Adjustments made on 2026-10-27/28, and why
A five-model review of this PR and a set of parity probes against
devfound places where the PRreplaced working
devbehaviour with new routes, flows or limits that the team-id removal does notneed. The owner set one rule for this PR: match
devwhereverdevalready works, and add newmechanisms only where removing the selected team id requires them. The changes:
dev's values on the 57 routes where they differed (gaps thatdevalreadyhas are tracked in control-api / rpc-proxy: no per-subject frequency limit on Host messages and artifact reads #1199, control-api: no frequency limit on external Google login, session verify, session-token and RPC-token issuance #1200 and control-api: no per-user limit on mutations that send email (invitations, approval-medium challenges) #1201).
dev's parent path again; the/message-resolutionand/artifact-readsub-pathsand their version fence were removed.
dev: the author always sees their own runs; other users' runs need live authority.V1 approvals use
dev's caller key, so approvals created ondevreplay unchanged.dev(self-target handling on legacy team routes, theretired-invitee 401, no internal
assignment_count, the shared draft-activation helper).dev'sinvalid_credentialsbody again; theverify 401 body and the
session-tokenrole check also matchdev. The external auth routesanswer 503 only for a real database outage, and every such 503 carries
Cache-Control: no-storeandRetry-After, which the edge relays.dev'sconfiguration.
next described were later removed entirely, in
522049e07andf0e8a49ad.)the Profile UI login can be tested on a branch profile.
Second review (2026-10-29/30), and what changed
A second blind multi-model review of head
eecc120found 23 defects (0 blocker, 0 critical, 0 high;8 medium, 15 low), all introduced by this PR. Fixed here:
made in this round applied to code that was removed later (
64e27bfac,f0e8a49ad,522049e07); they arekept here as history only. This PR has no revision rows, catalog or resolver.
change 5).
(see behaviour change 4).
team needs the canonical user id.
dev.for
/meand/me/teams, and both match the requested team.The other findings are tracked: the dormant catalog and indexer defects in #1222
(comment), and ten
low-severity follow-ups in #1251.
Third review (2026-11-05), and what changed
A third review (zach88, round 2) of head
9235eeb46found 27 items and 6 more from a blast-radius pass.Fixed here, in
d79a23f4b..9edcd358f:0137, with the versions, functions and tables named anda cleanup script (R2-M2; see Migrations).
~300-line DDL normalizer, which served the removed catalog indexes, is gone (R2-L14).
canCallerReadWorkflowRun, which had no caller here or ondev, was removed (9d32a660bhad restored it as a no-op; R2-L14).
legacySecurityCutoffRejectsSqllost its last caller in64e27bfacand was removed too. The unuseddetailsparameter of bothpublicApiErrorhelpers wasremoved.
team_membersrows in one statementordered by
(team_id, user_id), the order member removal and demotion use, so two admins acting on eachother no longer deadlock. A two-client real-PostgreSQL suite covers remove, demote and two shared teams
(R2-M1, R2-L9).
GET /meanswersdev's 404 for a bare control-api 401 (R2-M6; behaviour change 11)./me, as ondev, and the GFS error parser isback to
dev(R2-M4, R2-L7; Desktop GFS public-envelope test does not detect a missing reason #1225).session-token,verifyandrpc/token, a non-string token or claim fieldis refused instead of answering 500; the tests send it through the real routers (R2-L1, R2-L2).
google-loginis unchanged fromdev(R3-L5).Retry-Afterandno-storeand write one structured warning. TheSharedFileSystem live check answers 503 only for a database outage, rethrows anything else to the
sanitized 500, and no longer uses
console.warn(R2-L4, R2-L5). The bodies keep three shapes:dev's{"error":"session_backend_unavailable","retryAfterSeconds":…}from the session middleware, flat{"error":"authority_unavailable"}(orcontext_reconciliation_unavailable) from the team middlewareand the SharedFileSystem routes, and the public error envelope from
/external/auth/*. Desktop andProfile UI read only the status, so this is accepted here; unifying the bodies is tracked in Task 106 PR1 follow-ups: ten low-severity defects from the second review of PR #334 #1251.
unavailable, so control-api rejectsCONTROL_API_USER_ACCESS_SESSION_V2_ISSUANCE=true(session_v2_issuance_unavailable) until PR3 (R2-L6).ends at the sign-in and reset links instead of showing the form again (R2-L8).
/auth/googlehandler no longer has a branch for a 403 that control-api neversends on that route; a retired account gets control-api's own 401 (R2-L11).
UID (R2-L18, Migration script deletes a "terminal" Job by name, which can hit a newer running Job #1226); the fence comment states what
devdid (R2-L17).the revocation in the same statement (R2-L20).
control-ui/next-env.d.tsandprofile-ui/next-env.d.tsare tracked again, and.gitignorematchesdev(R2-L21). Tracking them exposed minikube T2 dirties the worktree in T0 and then refuses to certify because it is dirty #733, open ondevsince 2026-09-22: thisPR changes control-ui and profile-ui, so T0 runs
next buildfor both, the build rewrites the twotracked files, and
make minikube-t2then refuses to certify the dirty tree. Without a fix this PRcannot get a T2 verdict.
1a24817fbfixes it in the harness only (scripts/minikube/t0.sh,scripts/minikube/t2-common.shand their scenario test; no product code): T0 restores the twogenerated files to HEAD after the build, and the T2 dirty-tree check fails closed when
git statusfails.
Documented here and in the deployment note instead of code: this description (R2-M3), the Profile UI
origin precondition (R2-M5), the logout scope (R2-L3, behaviour change 3), the hardening list (R2-L10,
behaviour change 9), the 10 s lock wait and the 60 s fence (R2-L16, R2-L17), the
--timeoutcontract(R2-L19), the rollout order, the grant contract, the
0130password-check gap, the lock onusers, theGoogle pending-invitation change and the team budget change. The two dormant V2 items (R2-L12
/auth/session/renew, R2-L13 the two 426 shapes) are added to #1222.Where this round differs from the review:
0137and0138): a version ruleof "anything after
0130" misses earlier heads that used prefixes0101..0129and the same-name0131; the guard uses the migration names plus a probe for the objects.dev's 404 did not sign Profile UI or Desktop out either; the defect was the retryable 503 label.the terminal screen is the safe fix.
dev's, where V1 logout revoked nothing; it was fixed anyway.dev, fixed here because it is small.Fourth review (2026-11-06), and what changed
A fourth review (zach88, round 3) of head
6ca5fd67dfound 0 blocking items and 24 optional ones(1 Medium, 23 Low; 6 carried from earlier rounds).
Fixed here:
010a_..010e_,012a_,012b_). The guard, the cleanup SQL and the check query now match^[0-9a-f]{4}_, and thereal-PostgreSQL test inserts a hex receipt and asserts that no earlier-head receipt is left after the
cleanup (R3-L1, R3-M1;
b21a3aa14).switchTeamand the four team calls rethrow control-api's 401,so a revoked session is answered 401 instead of "not a member" or "no team" (R3-L3, behaviour change
12;
3a7465283). Correction:61249463a("forward session end and live denial from team routes")made the four team calls rethrow control-api's
invalid_sessionenvelope, but control-api answers anended session on
/external/teamsand/external/userswith a bare 401(
requireValidExternalSessionToken), so those rethrows never fired against the real control-api;their tests mocked the envelope. The new tests use the bare 401 that control-api actually sends.
b21a3aa14,79d220b92): the long-transaction note now says a long transaction makes theonline build fail after the 10 s
lock_timeout, not after 120 s (R2-L16); theclient_connection_check_intervalrequirement names the supported platforms (R3-L2); theearlier-head guard states when it can be removed (R3-L13).
79d220b92): the SharedFileSystem proxy relaysno-storeon a 503 (R3-L4); the two503 warnings carry the correlation id (R3-L7); a duplicate test now pins the 403 answer (R3-L10); comments
say what the edge emits (R3-L14);
sessionTokenFromRequestis inlined (R3-L15); a fixture uses a policyshape that exists (R3-L16). Internal hostnames were removed from the repository's deployment note.
Documented here instead of code: the scope of the non-string rule (R3-L5), the
rpc/tokenmint and a concurrent logout (R3-L6, behaviour change3),
/meand a rejected service credential (R3-L9, behaviour change 11), Desktop's/meprobes(R3-L11, behaviour change 7), why
1a24817fbis here (R3-L12), and the three 503 bodies (R2-L4). Therevocation purge (R3-L8) is #1266. The R3-L17 mount-time check and the V1 half of R1-L10 are added to
#1251; the dormant V2 items R1-L8 to R1-L11 to #1222.
Where this round differs from the review:
client_connection_check_intervalon macOS and the BSDs, notonly on Linux; the local real-PostgreSQL runs use native macOS PostgreSQL 16 and pass.
devalreadysets the same variable unconditionally. Tolerating the error would hide a missing cancellation, so the
fix is the doc wording only.
isControlApiInvalidSessionrethrow would not fire, because the memberships routesends a bare 401, not the envelope; the fix keys on the status.
(
20be2fbe4) is not ondev, one pin covers one of several earlier schemas, and the check query returnedno rows on clerum-dev and production on 2026-11-06.
A focused blind review of
68d020a1e..7befa6edefound no product or security defect and four Low items, fixed inaac5388a9: a second copy of the old wait in the LLM secret editor test (#1263, first fixed in7befa6ede), andthree inaccurate comments.
Fifth review (2026-11-07), and what changed
After
devwas merged up to31abc80d8, a focused review of the merge resolutions and of the commits sincea3f017650found no production defect in the migration wiring and no High or Medium security finding. It foundthree red CI checks and two Medium items. Everything below is on this head except where stated.
dev. ThelatestDevproducer now pinsdev's tip and moves at each merge ofdev; hand-written receipt counts are gone, and the upgrade delta is derived from the code.observedDevstays, because it covers the step through
0130. One bound (LAST_DEV_MIGRATION_VERSION) now separates thedevbatch from this PR's runner. Rejection of the old0131/0132versions is pinned.the message, so a response body cannot imitate it. A 401 keeps "Email or password is incorrect." A 429 says
"Sign-in is paused for now. Your password was not checked. Please try again {wait}." A 5xx or a failure with no
HTTP answer says "Sign-in isn't available right now. Please try again in a moment." The invitation checks no
longer match
404or410anywhere in the message. Other failures keep the existing text (Desktop sign-in: "Check your email and password" shown for 426, other 4xx and local failures #1382).MODELS_DEV_API_URLwith credentials is refused, and the raw value is not echoed in the error. The variableis not set in the GCP dev or production configuration, checked read-only.
control_api_runtimeno longer hasDELETEonexternal_user_session_security_epochs(
upsertprofile). This changes0137, which no shared database has recorded.E2E_SESSION_TEAM_UX=1, an explicitE2E_K8S_CONTEXTon abranch-owned profile, and an explicit
EXTERNAL_REST_API_BASE_URL. It restores memberships exactly. The E2Ehelpers now add the member in a second call, because
POST /admin/teamsignoresuserId.Deploy and rollback
The deployment note is
docs/deploy/user-access-foundation-upgrade.md. For this release it replacesstep 3 of the general order:
dev's migrations through0136on an environment that has not reached them yet.kubectl rollout statuson both and checkthat no pod of an older ReplicaSet is left.
Rollback goes the other way: control-api first, then External REST and Profile UI. The mixed window
(new External REST against old control-api) is pinned by
external-rest-api/test/mixedVersion.baseControlApi.test.ts; the note lists what works in it.Preconditions before step 1 (read-only checks are in the deployment note):
EXTERNAL_REST_API_CORS_ORIGINlists the public Profile UI origin in the target environment (behaviourchange 8). Checked read-only on 2026-11-05: clerum-dev and production each list the
environment's Profile UI origin; re-check before each deploy.
no rows on clerum-dev or production.
keyper-labs/evenfire-infra#154implements this order and merged on 2026-11-06. A deploy through an infrarevision without it must run the three steps by hand. With a single apply,
old External REST pods meet the new control-api and team create, team switch and password reset fail.
Locks and size:
user_access_foundationbuilds five indexes online onteam_members,user_contexts,team_contexts,user_agentsand
team_agents(CREATE INDEX CONCURRENTLY,ShareUpdateExclusiveLock; 0.1–0.3 s each at measuredsizes), then creates the session tables and their indexes in its bounded transaction.
legacy_password_security_epoch_backfillcopiespassword_set_atinto the password epochs in oneINSERT … SELECToverusersunderAccessShareLock. Measured at about 6 µs per row on one laptop, it reaches the 15 s statement timeoutnear 2.4 million users and then fails without a partial write.
indexes.
Rolling back to the
devimage:dev(cookie cleared, V1 token valid until it expires, at most 7 days), becausethe
devimage does not read the revocation table.devimage runs does not revoke earlier sessions after re-deployingthis PR; those sessions end at their TTL.
Pre-release builds read control-api variables that this release ignores (indexer, catalog and V2
action-context flags); the deployment note lists them so they can be removed. Before the first deploy,
the owner reads one stored migration Job on the development and production clusters to confirm the Job
identity check matches what the apiserver stores.
Validation
Validation on
4d990b023, withdevat31abc80d8:make minikube-t2, new branch profile,full-bootstrap):MINIKUBE_T2_PASS,T2_EXIT=0.T0, T1 with 1064 tests in 132/132 real-PostgreSQL files (0 pending), NP-08, port-forwards and health all pass.
scripts/minikube/t0.sh):T0_EXIT=0, tree clean afterwards.CodeQLcheck reports no new alert in code changed by this PR; older open alertsin code this PR does not change stay as they are on
dev.session-team-ux), three tests, all passed on this head but not in one run: tests 1 and 2in one run, test 3 in the next. Two failures were seen and are recorded here:
test2@clerum.ioright after an API login withthe same password returned 200. Classified as a test defect (E2E harness: trigger lane cannot sign in, launchAndLogin reads the URL too early and can take external keystrokes #1385): the lane window takes the foreground, and
keys typed for another app land in the password field after
fill. Reproduced with a witness (field length 9,then 11, status 401). For this failure the field length is measured (10 dots) and the server's answer is
inferred, because its logs are gone. No server password code changed, and
devhas the same test order.workflowUi.ts:589readpage.url()as empty before the window navigated, before the test reachedanything it checks. That line is on
dev.Not covered on this head:
scripts/e2e/e2e-workflow-triggers.shdid not get past its login phase, so it did not reach the changed teamhelper. It signs in two users with no pause, which the 7.5 s password pace on
devrejects, and its seed givesthe outsider user no password. Its helper is covered by
test-e2e-workflow-triggers-team-helper.sh, andseed-e2e-data.shmakes the same two calls on the live profile.recipes.ts, did not run.0129and a rollback before0130are hand-measured or untested.0130holds during the LLM DDL (pre-existing) have no measurement on a production-sized copy. Measurethem before deploying.
Results on earlier heads, kept for reference (
6ca5fd67dand9235eeb46):make minikube-t2, branch profile, full bootstrap):MINIKUBE_T2_PASS,T2_EXIT=0.T0, T1 with 964 tests in 122/122 real-PostgreSQL files (0 pending), NP-08, port-forwards and the Host
message/artifact health check all pass.
9235eeb46brings the deterministic Desktop renderer test fixes from fix: make code defaults match the values deployments run #1305 (0b458b27f,e0854be98; fixes Desktop test App.deepLinks 'renders a fresh B workspace after successful A logout' is flaky #1278). Before them,App.deepLinks.test.tsxfailed 6 of 10 runs ondevand7 of 10 on this branch, and it stopped one T2 run in T0. With them, the affected suites passed 10 of
10 runs.
that are already open on
devpoint:js/user-controlled-bypass460, 461, 464–479, 1823: all open ondevsince 2026-07-20 orearlier. Each flagged condition was read on
dev: every one is a required-field check thatreturns 400 before the request continues. None lets a request skip authentication or authorization.
js/missing-rate-limiting96: open ondev, tracked with control-api / rpc-proxy: no per-subject frequency limit on Host messages and artifact reads #1199–control-api: no per-user limit on mutations that send email (invitations, approval-medium challenges) #1201.js/unused-local-variablenotes in Desktop tests, both open ondev(1746, 1922).js/missing-rate-limitingonme.tsandmembers.ts, reported on this PR) aredismissed as "won't fix": the limits match
dev, as recorded indocs/how-to/codeql-js-missing-rate-limiting.md.js/missing-rate-limitingalerts were open on the PR ref and not ondev, and are dismissed; none marks a limitremoved by this PR (dispositions in
docs/how-to/codeql-js-missing-rate-limiting.md): grok-llm-proxy: Imagine image generation and edits #914, mcp-host: Grok image generation tool, frames and failover semantics #916-mcp-host: one corrupt pending-approval row aborts rehydration of every pending approval #921, feat(desktop): upload any file from the agent chat composer as an inline attachment #932,#1981 and #1979 are routes behind the same
express-rate-limitrouter.useas ondev; #1980 is/external/auth/verify, without a limit here or ondev; #1982 and #1983 are apps built inside tests.Follow-up issues
Task 106 activation gates (must be fixed before the matching switch is turned on in a later PR):
f0e8a49ad); also the dormant/auth/session/renewand 426 shapes64e27bfac; the issue applies to the PR that adds it back)These cover code that this PR installs turned off; none of them affects the default configuration.
Introduced by this PR, outside the team-id retirement itself:
dev(it had no producer); to close when this PR merges.Obsolete after
522049e07(no revision rows exist): #1264, #1265 — proposed to close.Pre-existing in
dev, not related to Task 106 (found by the review, not caused by this PR):seed-e2e-data.sh: a rerun after a partial failure creates a duplicate team.Also related: the rate-limit gaps that
devalready has, kept at parity in this PR, are tracked in#1199, #1200 and #1201. #190 (public External REST 500 detail exposure) remains separate. Known test
flakes on
devseen while validating: #1212, #1228, #1249.Next steps
EDGE_PHASElines and theimageslist on the first dev deploy (A2 inkeyper-labs/evenfire-infra#154).Measure the
0130locks on a production-sized copy before the production deploy.sync-auth-keyon the race in sync-auth-key: gfsc-reader wait returns before the restarted revision is Ready #1363 (fix in fix(deploy): make the gfsc-reader wait prove the rollout finished #1374, still open); the attempt that hit it on 2026-11-06 passed when re-run.before it is rebased.
🤖 Generated with Claude Code