Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a v3 planning document describing how the project will reintroduce v1’s Smart-HTTP/OCI/federation capabilities within v2’s kernel/extension architecture, along with an increment-by-increment roadmap and verification criteria.
Changes:
- Add
V3_PLAN.mddescribing v3 goals, architecture targets, increment plan, and verification checklist.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+3
| # Forgepoint v3 — Plan | ||
|
|
||
| This branch combines what v1 (the original Forgepoint at `forgepoint-dev/forgepoint`) and v2 (the spec-driven rewrite, which this repo is) each got right into a single coherent kernel. |
Member
Author
v3 architectural foundation — landedAll 8 increments planned for this initial branch are committed and pushed. The v3 architecture combining the best of v1 (pure-Rust Git Smart HTTP v2, OCI extension distribution) with v2 (kernel-vs-features split, OIDC + SpiceDB + CUE + CloudEvents + WIT/Component Model, spec discipline) is in place at the contract and crate level. What's done
Test posture
Backend flip
Deferred to follow-up branchesSee
Each is independently scoped and individually shippable on its own branch. 🤖 Generated with Claude Code |
start.sh oneshot passes end-to-end after wiring identify_wasm_op + extract_root_operation_field correctly. The GraphQL handler now consults the generated dispatch table on every request (route identified, info resolved, framework hit). However, the production path currently returns None from wasm_dispatch::dispatch for every call, because the live data is in legacy shape and routing `issues.close` to WASM mid-flow fails when WASM tries to deserialise a legacy-shape issue document as a WIT StoredIssue. The clean swap is M4's job: when issues_create_mutation / issues_close_mutation / etc. are deleted, the WASM dispatcher becomes the only writer + only reader of issues documents and the WIT shape takes over the on-disk format. Until then, dual-path coexistence isn't safe. What did land in M3: - Routing decision happens via the proper GraphQL operation field (extract_root_operation_field) — no more substring scan picking up sub-fields like `issuesClosed` inside `epicsProgress`. - Dotted legacy aliases `<interface>.<verb>` (e.g. `issues.close`) are now in the dispatch table alongside the camelCase form, so the legacy GraphQL surface routes match. - start.sh assertion updated to accept the new `platform-loaded` resolver status (legacy resolver path doesn't run for extensions declaring `platformWitVersion` + shipping a real WASM). Boxes flipped: 3 (handler consults table), the legacy fallback, and 6 (smoke passes). Boxes 4 (actual WASM routing) and 5 (browser smoke fires WASM) carry explicit deferrals to M4 — the framework is in place, the swap is M4's atomic operation.
To delete the legacy issues_*_mutation handlers, the WASM-side data
shape has to match what the existing GraphQL surface returns. This
commit aligns ext_issues to that shape without changing semantics:
- WIT package bumped from 0.1.0 to 0.1.1 (additive, no breaking
changes for any caller).
- New optional `state-reason` field on the `issue` record. The
GraphQL surface exposes it as `stateReason`; close-issue populates
it from `input.reason`, reopen-issue clears it.
- New optional `closed-by-ref` field on `close-issue-input` — lets
the kernel record an actor URI other than the calling principal
on a close (used by the auto-close-on-merge reactor).
- State is now serialised UPPERCASE ("OPEN"/"CLOSED"/"REOPENED") on
the wire and in storage, matching what every existing GraphQL
reader expects. `state_from_str` accepts either case so data
written by either path during the cutover reads cleanly.
- `reopen-issue` now sets state=OPEN (not REOPENED) — matches legacy
behaviour (legacy returns "OPEN" on reopen, never uses REOPENED).
Tests + smoke still green. The next M4 commits port each legacy
issues_*_mutation/query into the WASM component, flipping
wasm_dispatch::dispatch to route ext_issues calls to WASM as each
op lands, then delete the corresponding legacy code.
Addresses comtrya/comtrya#4 sub-item **P0-3** (guessable bearer tokens + raw token in audit log). Other sub-items of #4 (P0-1 partly done in #17, P0-2, P1-5/6, P2-5/6/9) remain open and tracked in #4. ## Summary Bearer tokens (session and credential) move from `{prefix}_{unix_seconds}_{monotonic_counter}` (effectively guessable — the counter resets on restart, so the search space is ~2^14 right after boot) to `{prefix}_{32-hex-chars}` sampled from `getrandom` (OS RNG, 128 bits of entropy). Audit log no longer writes the raw session token — it writes an 8-byte SHA-256 prefix as a non-reversible cross-reference handle. ## Acceptance (P0-3 of #4) - [x] **Bearer tokens generated from `OsRng` with ≥128 bits entropy.** New `AppStateInner::next_secure_token(prefix)` (main.rs:1374) uses `getrandom::fill(&mut [0u8; 16])`. Wired into `issue_session` (main.rs:1296) and `issue_credential` (main.rs:1341). - [x] **Audit log at `main.rs:1308` no longer contains raw token.** Now writes `{"token_sha256_prefix": token_audit_id(&token)}`. The free function `token_audit_id` (main.rs:1437) returns 16 hex chars = first 8 bytes of SHA-256 = 64 bits of preimage resistance, 32-bit birthday bound. Acceptable for a non-secret cross-reference; the full 256-bit hash would just bloat lines. ## What changed - `crates/server/Cargo.toml`: added `getrandom = "0.3"` (already transitively present in the lock). - `crates/server/src/main.rs`: - `AppStateInner::next_token` renamed to `next_id` (only two non-secret callers remained: `prn_` URI fragment, `evt_` IDs). Doc-comment explicitly says "Never use for anything that must be unguessable." - `AppStateInner::next_secure_token` added — the new primitive. - Top-level free function `token_audit_id(token)` added. - `AppStateInner::read_audit` added under `#[cfg(test)]` (mirrors the existing `read_events`). - `crates/core/src/auth.rs`: - `AuthService::exchange_token` previously built `access_token = format!("fp_{}_{}", now_ms, scope.join("_"))` — the same guessable shape this PR exists to remove, but on the public core API. Now uses a new module-private `secure_access_token()` helper that mirrors the server's `next_secure_token` (`fp_` prefix + 32 hex chars from `getrandom::fill`). - `crates/core/Cargo.toml`: added `getrandom = "0.3"`. - `crates/core/src/http.rs`: deleted `SessionToken` + `SessionTokenIssuer` (and its one local test). The server doesn't use this type — it has its own `Runtime::issue_session` path via `next_secure_token`. The dead struct still issued `format!("sess_{}_{}", now_ms, len())`, i.e. the exact guessable shape this PR exists to remove. Per CLAUDE.md "Remove dead compatibility code immediately." ## Threat model closed by this PR Before: a network observer who sees a session-issuance request (and can estimate the server's wall-clock time to ±1s) can brute-force the bearer token in under 2^14 attempts during the first hour after restart — assuming `prefix_<seconds>_<counter>` and a low-traffic counter. P1-5 rate limiting is not yet implemented, so this attack is currently viable. After: 2^128 brute-force search. Infeasible by any known computation. The audit-log redaction prevents an audit-log reader (operator with read access to `metadata/audit.jsonl`) from harvesting live session tokens. ## Out of scope - **P0-1 anonymous lockdown** — partly done in #17; full lockdown depends on OIDC callback (#16 follow-up). - **P0-2 DefaultAuthz replacement** — separate sub-PR. - **P1-5 rate limiting**, **P1-6 body size limits**, **P2-5 readyz hardening**, **P2-6 security headers**, **P2-9 operator code removal from `.envrc`** — separate sub-PRs. ## Test plan ``` cargo test --bin comtrya-server -- next_secure_token issue_session_does_not_log_raw consume_session_works_with_secure → 3 passed cargo test --workspace → 297 passed, 0 failed cargo clippy --workspace -- -D warnings → clean cargo fmt --all -- --check → clean ``` Three new tests in `crates/server/src/main.rs`: - `next_secure_token_returns_unique_tokens` — 1000 tokens, all unique (`HashSet::len() == 1000`); asserts `sess_` prefix + 32 ASCII hex chars. - `issue_session_does_not_log_raw_token_to_audit` — issues a session, reads `runtime.audit_path` via the new `read_audit()` helper, asserts the raw token does NOT appear in the serialized audit line AND that `token_sha256_prefix` does. - `consume_session_works_with_secure_tokens` — round-trip proof that the helper change didn't break the call path. ## Honest disclosures 1. **No browser verification.** Server-only change; no Vue/TS/asset files in the diff. 2. **Audit-log JSON shape changed** (`event.data.token` → `event.data.token_sha256_prefix`). No downstream audit-log consumers exist in the repo (greppable). CLAUDE.md no-back-compat posture applies. 3. **`getrandom::fill(...).expect("os rng unavailable")` panics** on OS RNG failure. On supported platforms (Linux/macOS/Windows) the OS RNG is always available; failure is a kernel-level crisis where issuing weak tokens silently would be worse than panicking the request thread. 4. **`token_audit_id` 32-bit birthday bound.** At billions of sessions per server the chance of two audit entries colliding on their cross-reference handle becomes meaningful, but the handle is not a secret and collision means "two entries look alike," not a security boundary breach.
…0-6) (#44) Addresses comtrya/comtrya#6 P0-6 — reconciles the `pull-requests.wit` `pr-state` variant with the runtime strings the rest of the system uses. Specifically: `REVIEW` exists in fixtures, main.rs's `OPEN_PR_STATES`, and a dozen tests but had no WIT case, leaving the contract and the runtime out of sync. ## Summary - **WIT** (`pull-requests.wit`): add `review` to the `pr-state` variant. Lifecycle is now `DRAFT → READY → REVIEW → MERGED | CLOSED`. - **Component** (`ext_pull_requests/component/src/lib.rs`): add Review arms to `state_to_str` ("REVIEW") and `state_from_str` ("REVIEW" → Review). - **Host** (`crates/server/src/wasm_invokers.rs`): add Review arm to `pull_state_to_graphql` ("REVIEW"). Compile failure on the non-exhaustive match was the load-bearing regression guard while wiring this PR. - **Inbox.vue**: shrink `OPEN_PR_STATES` Set from 5 entries to 3, removing `OPEN` and `REOPENED` (those are issue states that wrongly leaked into the PR-state filter). - **WASM rebuilt**: `extensions/first-party/ext_pull_requests/dist/ext_pull_requests.wasm` regenerated by the bundler. ## What changed `git diff --cached --stat`: ``` crates/server/src/wasm_invokers.rs | 24 +++++++++++ extensions/first-party/ext_pull_requests/component/src/lib.rs | 37 +++++++++++++++ extensions/first-party/ext_pull_requests/dist/...wasm | Bin +209 bytes extensions/first-party/ext_pull_requests/wit/pull-requests.wit| 2 +- frontend/src/routes/Inbox.vue | 5 ++++- ``` ## Acceptance (close of #6 P0-6) - [x] `review` added to WIT `pr-state` variant. - [x] Component-side `state_to_str` / `state_from_str` handle Review with round-trip unit tests. - [x] Host-side `pull_state_to_graphql` handles Review with a unit test + pre-existing-cases regression guard. - [x] Inbox.vue's `OPEN_PR_STATES` Set no longer leaks issue states. - [x] WIT top-of-file comment updated to reflect the new 5-state lifecycle. - [ ] **Comprehensive ext_pull_requests UI handling of REVIEW** — deferred to a sibling sub-PR. See "What's NOT in scope" + honest disclosure #1. ## Test Plan ``` # Host side (server crate): cargo test -p comtrya-server pr_state # 2/2 (new) cargo test --workspace # 321/0 total cargo clippy --workspace -- -D warnings # clean cargo fmt --all -- --check # clean # Extension component (host-native target for unit tests of # pure-Rust state helpers; the wasm target can't execute): cd extensions/first-party/ext_pull_requests/component cargo test --target $(rustc -vV | awk '/^host:/{print $2}') # 2/2 (new) RUSTFLAGS="-D warnings" cargo component check # clean # Bundle regenerated: extensions/bundler/build-extension.sh extensions/first-party/ext_pull_requests # success; wasm + manifest updated # Frontend: cd frontend && bun run typecheck # clean bun test # 32/32 bun run build # clean ``` New tests: - `wasm_invokers::tests::pr_state_review_maps_to_uppercase_review` - `wasm_invokers::tests::pr_state_pre_existing_cases_unchanged` - `ext_pull_requests_component::tests::state_round_trip_review` - `ext_pull_requests_component::tests::state_round_trip_pre_existing_cases` ## What's NOT in scope - **ext_pull_requests UI handling of REVIEW** (`types.ts`'s PrState union, `stateTone` case, `PullsOverview.vue` open filter + count, `PullsQueue.vue` Filter type + match arms, `PullsDetail.vue` can-edit predicates, plus a `pr-state-review` CSS class). All deferred to a sibling sub-PR because they're a rendered-UI change requiring Chrome MCP verification per CLAUDE.md's UI Verification Hard Rule. - Issue states (`OPEN | CLOSED | REOPENED` in `ext_issues/ui/**`) — those are correct for the issue lifecycle and not what P0-6 targets. - Storage migration for any in-flight wire data — there is no production data today. ## Honest disclosures 1. **REVIEW silently downgrades to READY in the ext_pull_requests UI until the sibling sub-PR ships.** The data path goes through TWO normalisation layers: first `normalizeState` in `api.ts` (a switch with `default: return "READY"`) which silently maps any unrecognised tag — including `"REVIEW"` — to `"READY"`; then `stateTone` in `types.ts` (which `PullsOverview/ Queue/Detail` call) renders the READY-classed PR with the green "open" badge. End-user-visible effect: a REVIEW-state PR appears in those three views as a regular open PR, not as "unknown". - Inbox.vue takes a different data path: it calls `invokeOp("ext_pull_requests", "pulls", "list-pulls", …)` from `@comtrya/sdk-core` directly (not the ext_pull_requests UI's `api.ts::listPulls`), so it bypasses `normalizeState` entirely. Whether a REVIEW PR's `state` field arrives at Inbox as `"REVIEW"` (matched by the now-shrunk `OPEN_PR_STATES` Set) or as a WIT tagged-object `{tag: "review"}` (not matched at all) depends on the WIT-bridge serializer for the `pr-state` variant — that's runtime behavior the follow-up Chrome MCP sub-PR can verify and tighten conditionally. Honest disclosure: I haven't browser- verified this path either, so I included `"REVIEW"` in the Set as a forward-compatible guard. - The sibling UI sub-PR must update BOTH `normalizeState` (add the `"review" → "REVIEW"` arm before the `default`) AND `stateTone` (add a `case "REVIEW"` with its own className and label), plus the filter sites in `PullsOverview.vue`, `PullsQueue.vue`, and `PullsDetail.vue` that pattern-match on the four states. Tracking it as a known gap rather than fixing it here per the Chrome MCP UI rule. 2. **WIT variant ordering**. `review` inserted between `ready` and `merged`. This shifts the binary discriminant for `merged` (was 2, now 3) and `closed` (was 3, now 4). The wire-format layer is JSON strings via `state_to_str`/`pull_state_to_graphql` — never raw variant tags — so storage and GraphQL responses are forward/backward compatible across the change. The internal WASM-component ABI between host and extension is self-consistent: both regenerate from the new WIT in this PR. 3. **`state_from_str` default unchanged.** Unknown strings still map to `PrState::Draft`. A typo like `"reveiw"` would silently degrade — the regression guard for THIS class of bug is the round-trip test in the component (`state_round_trip_review`), which would fail if `state_to_str(Review)` ever drifted from `state_from_str`'s recognised input. 4. **Loop-worktree artifacts (untracked, not shipped).** `PLAN_PR_STATE.md` and `PR_BODY.md` at repo root, untracked. 5. **Adversarial review trail.** - Stage 1 (planning): 2 rounds. R1 caught that the original plan missed substantial ext_pull_requests UI surface (types.ts PrState union, stateTone, PullsOverview/Queue/Detail filters) — adding REVIEW to those is a rendered-UI change requiring Chrome MCP. R2 SHIP after scoping down to backend + the safe Inbox.vue Set shrinking, with the UI work explicitly deferred and the gap honestly disclosed. - Stage 2 (implementation): 2 rounds. R1 caught two gaps: (a) WIT file's top-of-file comment still described the old 4-state lifecycle; (b) the plan promised a `state_round_trip_review` test in the component crate that wasn't written. R2 SHIP after updating the comment and adding both round-trip tests (REVIEW + pre-existing-cases regression guard) — the latter required running tests against the host-native target since `cargo test` defaults to wasm32 for the component crate. Mid-implementation discovery: the first attempt to run the full workspace test suite after the host-side WIT binding regeneration revealed 11 test failures with "failed to convert function to given type" — the host bindings had 5 variants but the shipped `ext_pull_requests.wasm` still encoded 4. Resolved by rebuilding the WASM via the bundler. Documented here so future maintainers know the host-and-WASM-must-rebuild- together invariant.
Closes one bullet of comtrya/comtrya#4 — **P1-6 explicit body size limits**. ## Summary Narrows axum's implicit 2 MiB default with per-route `tower_http::limit::RequestBodyLimitLayer`. Oversized requests fail with HTTP **413 Payload Too Large** at the network layer, before any handler runs (so the cap applies even on handlers that don't extract a body, e.g. the session-issuance endpoints). | Route | Cap | |---------------------------------------------|---------| | `/graphql` (POST) | 256 KiB | | `/api/ops/:extension/:interface/:op` | 1 MiB | | `/auth/token-exchange` | 4 KiB | | `/_extensions/session` | 4 KiB | | `/events/session` | 4 KiB | | `/git/*path` | 16 MiB | Rationale per route: - **GraphQL 256 KiB**: large enough for realistic queries with embedded fragments, well below DoS territory. - **Ops 1 MiB**: ops payloads can carry markdown bodies, attachment references, structured JSON — 1 MiB is generous without being lax. - **Session 4 KiB**: handlers only consume headers; cap exists purely as a DoS guard against a misbehaving client streaming a giant body. - **Git 16 MiB**: upload-pack negotiation is tiny in practice (<1 KiB want-lists), but allowing a comfortable headroom for future push (`receive-pack`) bring-up. Push is currently rejected as an unsupported surface, so this cap is forward-looking only. ## What's NOT in scope - Streaming/chunked-encoded bodies that defeat content-length — `RequestBodyLimitLayer` counts bytes as they arrive, so transfer encoding doesn't bypass it. - Connection-level limits (max headers, max connections per IP) — out of scope for P1-6; tracked separately under #4. - `/healthz`, `/readyz`, `/graphql/stream`, `/events`, OIDC GET routes, and asset/manifest routes — all GET or no-body-consumed, no cap added. ## Test Plan ``` cargo test -p comtrya-server payload # 7/7 new cargo test -p comtrya-server # 161/0 cargo fmt --all -- --check # clean ``` New tests (all under `mod tests`): - `graphql_post_rejects_payload_exceeding_limit` - `api_op_rejects_payload_exceeding_limit` - `token_exchange_rejects_payload_exceeding_limit` - `extension_session_rejects_payload_exceeding_limit` - `events_session_rejects_payload_exceeding_limit` - `git_endpoint_rejects_payload_exceeding_limit` - `graphql_post_accepts_payload_under_limit` (regression guard) Test pattern: bind a real `axum::serve` on `127.0.0.1:0`, hit it with `reqwest`, assert the status code. The 413 must arrive before the handler's auth/CORS checks (verified by the session-endpoint tests, which would otherwise 401). ## Acceptance criteria (subset of #4) - [x] Each request-consuming route declares an explicit byte cap. - [x] Oversized requests return 413, not 400/500. - [x] Limit fires at the network layer (verified by session-endpoint tests where the handler never reads the body). - [x] Per-route caps sized to realistic ceilings, not a one-size catch-all. ## Honest disclosures 1. **`RequestBodyLimitLayer` vs `axum::extract::DefaultBodyLimit`.** First attempt used `DefaultBodyLimit::max(N)`. It works for handlers that extract `Bytes`/`String`/`Json` but is a no-op when the handler only takes `HeaderMap` — the body is never read so the limit never fires. Switched to tower-http's layer which enforces at the transport layer regardless of handler extractors. The two session-endpoint test cases (`events_session_*`, `extension_session_*`) were specifically retained to lock that regression: they fail-fast with 413 against the new layer and would have silently fallen through to 401 under the old approach. 2. **Pre-existing clippy errors on `v3`.** Running `cargo clippy -p comtrya-server --all-targets -- -D warnings` currently surfaces 3 errors unrelated to this PR (`wasm_host.rs:1837` complex-type, `wasm_registry.rs:640` items-after-test-module, `main.rs:8260` length-comparison-to-zero inside a `payload[...].as_array().unwrap()` test assertion). Verified pre-existing by stashing this PR's diff and re-running — same 3 errors. Out of scope for #4 P1-6; should be cleaned up under a separate housekeeping PR per the clippy-hard-rule discipline. 3. **No new dep beyond `tower-http`.** Added with `default-features = false, features = ["limit"]` — only the `RequestBodyLimit*` types and tower's `Service` impl come in; no h2/rustls/cors/etc. surface. 4. **Adversarial review trail.** - **Stage 1 (planning)**: 2 rounds. R1 questioned whether `DefaultBodyLimit` was sufficient for handlers that don't extract a body — confirmed it isn't, swap to `RequestBodyLimitLayer`. R2 asked about git upload-pack realistic sizes — 16 MiB picked as forward-looking ceiling. - **Stage 2 (impl)**: 2 rounds. R1 caught that the session tests (which use `RequestBodyLimitLayer`'s no-handler-body enforcement) were initially failing under `DefaultBodyLimit`. R2 confirmed all 11 payload-named tests pass under the tower-http layer, and the regression guard (under-limit request not 413) holds. - **Stage 3 (final)**: 3 reviewers. (a) acceptance: 413 on every request-consuming route, per-route caps, before handler auth — all met. (b) regressions: full server test suite 161/0; no handler signatures changed. (c) honesty: this section. 5. **Loop-worktree artifacts (untracked, not shipped).** `.claude/worktrees/issue-4-body-limits/` left in place per the loop discipline; nothing untracked at repo root.
Closes one bullet of comtrya/comtrya#4 — **P2-6 security headers on all responses**. ## Summary Adds a global `tower-http::SetResponseHeaderLayer` stack so every response — including 4xx/5xx and unmatched fallback paths — carries: | Header | Value | Conditional? | |---|---|---| | `X-Content-Type-Options` | `nosniff` | always | | `Referrer-Policy` | `no-referrer` | always | | `Content-Security-Policy` | `frame-ancestors 'none'` | always (via `if_not_present`) | | `Strict-Transport-Security` | `max-age=63072000; includeSubDomains` | only when `StartupOptions::tls_terminated == true` | ### Design notes - **CSP `if_not_present`** — `apply_extension_asset_headers` (`crates/server/src/main.rs:3339`) installs a per-asset CSP with `frame-ancestors 'self'` so extension UIs can frame their own assets. The global layer uses `if_not_present` so the asset CSP survives on `/_extensions/.../assets/*`. Every other route inherits the strict default. - **No global `X-Frame-Options`** — would clash with the asset route's `frame-ancestors 'self'` since browsers reconcile by taking the more restrictive of the two. Pure CSP approach instead; the acceptance text reads 'X-Frame-Options/CSP frame-ancestors' so either is sufficient. - **HSTS gated on `tls_terminated`** — pinning a `localhost` dev origin to HTTPS is a well-known papercut. Production deploys with TLS at the server (or a TLS-terminating proxy that flips `tls_terminated`) emit it; plain-HTTP dev runs do not. ## What's NOT in scope - `Cross-Origin-Opener-Policy` / `Cross-Origin-Embedder-Policy` — out of scope for P2-6; would need a separate audit of any cross-origin embedding the extension shell currently uses. - `Permissions-Policy` — same. - Per-route header customization beyond what's already in `apply_extension_asset_headers`. ## Test Plan ``` cargo test -p comtrya-server -- security_headers hsts baseline extension_asset_csp # 4/4 (new) cargo test -p comtrya-server # 158/0 total cargo fmt --all -- --check # clean cargo clippy -p comtrya-server --bin comtrya-server -- -D warnings # clean ``` New tests: - `baseline_security_headers_applied_to_every_response` — 200 path carries all four baseline headers; HSTS absent without `tls_terminated`. - `security_headers_applied_to_error_responses` — 404 fallback still carries `X-Content-Type-Options` + `Referrer-Policy` (would-be MIME-sniff probe target). - \`hsts_applied_when_tls_terminated\` — \`Strict-Transport-Security\` appears when the runtime is started with \`tls_terminated: true\`. - \`extension_asset_csp_not_overridden_by_global_layer\` — asset route's per-route CSP survives; \`frame-ancestors\` clause present. ## Acceptance (subset of #4) - [x] \`X-Content-Type-Options: nosniff\` on every response. - [x] \`Referrer-Policy\` on every response. - [x] \`Strict-Transport-Security\` when TLS terminates. - [x] CSP \`frame-ancestors\` covers clickjacking equivalent to \`X-Frame-Options\` (per issue's 'X-Frame-Options/CSP frame-ancestors'). ## Honest disclosures 1. **No X-Frame-Options header.** Per the design note above. If a reviewer prefers belt-and-braces, adding \`X-Frame-Options: DENY\` only on non-asset routes (via a route-scoped layer instead of global) is the right shape — happy to add in a follow-up if asked. 2. **Dependency conflict with #45.** PR #45 (P1-6 body-size limits, open) also adds \`tower-http\` to the same \`Cargo.toml\` line. Whichever merges second needs a trivial feature-union resolution — \`features = [\"set-header\", \"limit\"]\` is the combined form. 3. **Adversarial review trail.** - **Stage 1**: 2 rounds. R1 questioned global \`X-Frame-Options\` conflicting with asset CSP \`frame-ancestors 'self'\`; switched to pure-CSP approach. R2 SHIP. - **Stage 2**: 2 rounds. R1 caught \`Runtime: !Clone\` so the tls-terminated test helper had to build a fresh runtime from a new \`StartupOptions\`. R2 SHIP after that fix; 4/4 tests pass, full server suite 158/0. - **Stage 3**: 3 reviewers. (a) acceptance — all P2-6 bullets met. (b) regressions — asset CSP test confirms the \`if_not_present\` strategy preserves the stricter per-route CSP. (c) honesty — see disclosure 1 about XFO choice. 4. **Loop-worktree artifacts (untracked, not shipped).** \`.claude/worktrees/issue-4-security-headers/\` retained per the loop discipline.
**Draft — server-side complete, browser verification pending.** Last slice of #16. Login (PR #23) and callback (PR #29) merged; this PR locks down read-paths + `/api/ops/*` and wires the frontend SDK to auto-bootstrap a session. ## Goal Every non-public surface now returns `401 Unauthenticated` without a valid session. The frontend SDK auto-exchanges `PUBLIC_COMTRYA_OPERATOR_CODE` on init (dev) or redirects to `/auth/oidc/:provider/login` (production), so a real browser load still sees content. ## Status ### Server — done - `graphql_guard` now does CORS + rate-limit + `require_authenticated_principal` in one shot; the 8 per-mutation `// TODO(oidc-#16)` blocks are removed. - `graphql_response` (generic-query fallback) routes through `graphql_guard` too, gating queries that used a hand-rolled Invalid-only check. - `api_op` rejects `PrincipalStatus::Anonymous` in addition to `Invalid`. - `user_layout_query` loses its inline `Invalid`-only check (the guard covers it). - 2 pre-existing "DEFERRED-BY-DESIGN" tests pinning the anonymous-read contract are rewritten as positive lockdown tests; 1 new `api_op` test. - **171/171 server tests green; clippy clean.** - Production-testbed smoke (`./start.sh --reset`) green end-to-end after updating 21 GraphQL curls to thread the bearer. ### Frontend SDK — done - new `sdk-core/session.ts`: tiny in-memory token cache + token-exchange bootstrap; dedupes concurrent calls; clears on 401; redirects to `/auth/oidc/google/login` when no operator-code is available. - `graphql-client` + `invokeOp` attach `Authorization: Bearer …` from `getSessionToken()` on every call. - `vite.config` `envPrefix` includes `PUBLIC_` so the operator-code env reaches client code. - **8 new session tests; 66/66 frontend tests green; vue-tsc + build clean.** ### Browser verification — blocked Per CLAUDE.md's UI Verification Hard Rule, this PR cannot be marked ready-for-review without a real-browser end-to-end check (load shell, observe `/auth/token-exchange` in network panel, confirm `Authorization: Bearer` attaches to subsequent calls, confirm content renders). **Chrome MCP unavailable in this session** (`claude-in-chrome` extension disconnected; `chrome-devtools` couldn't locate Chrome at `/Applications/Google Chrome.app/...`). Direct `curl` probes from the running smoke confirm the wire contract (`401 UNAUTHENTICATED` envelope + CORS), but JavaScript-level wiring of the SDK bootstrap is not yet verified end-to-end. Will complete browser verification once Chrome MCP reconnects, then flip the draft. ## Public (still anonymous) surfaces - `GET /healthz`, `GET /readyz` - `POST /auth/token-exchange`, `GET /auth/oidc/:provider/login`, `GET /auth/oidc/:provider/callback` - `GET /_extensions/:extension/assets/*path` - `OPTIONS /*` (CORS preflight) ## Deferred (separate follow-up tickets) - Session refresh-on-expiry (current TTL long enough not to bite in practice). - atproto/Bluesky DPoP + PAR flow. - Removing the `urn:comtrya:grant:operator-code` grant (dev flow still depends on it). Closes #16 after browser verification lands.
rawkode
enabled auto-merge
May 19, 2026 14:38
…path (#7) (#48) Follow-up to #7. The real CUE evaluator landed in PR #19 and the schema wiring in PR #27, but every GraphQL workspace query was still calling `cue_config::evaluate_repo_config` on every read — materialising the worktree, writing schemas, running cuengine, tearing down — for every repo on every request. This PR caches the evaluation per `(git_dir, commit_oid)`. Commits are immutable, schemas only change at process boundaries, so the cache is sound. First read per commit pays the cost; every subsequent read short-circuits. ## Changes - New `CueConfigCache` in `crates/server/src/cue_config.rs`: - `Mutex<HashMap<(PathBuf, String), Arc<Value>>>` - `evaluate(git_dir, ref_name, schemas)` resolves ref→commit via `git rev-parse <ref>^{commit}`, then either returns the cached Arc or evaluates fresh and inserts. - **Evaluation runs OUTSIDE the lock** — cuengine can take hundreds of ms and would otherwise serialise every CUE-touching request on a single mutex. - Unresolvable refs fall through to direct evaluation (not cached) so a transient git state can't pin a bad result. - Wired into `Runtime` as `cue_config_cache`. The three hot-path call sites (`main.rs:2676`, `:2713`, `:4470`) and `git_demo_snapshot` now go through it. - 4 new cache tests: hit returns same `Arc`, new commit invalidates, per-repo isolation, unresolvable refs don't pollute. ## Memory model Each entry is the evaluated JSON envelope (~few KB). Unbounded by request rate, bounded by reachable commit count. Production workloads will churn through tens to hundreds of commits across a process lifetime — bounded by the git GC horizon, not by HTTP traffic. If that ever pinches, swap the inner map for an LRU; the public surface is stable. ## Verification - `cargo test -p comtrya-server` — **175/175 green** (171 prior + 4 new). - `cargo clippy -p comtrya-server --bin comtrya-server -- -D warnings` — clean. - Manual: confirmed cache reuse + invalidation in unit tests; under production-testbed load the workspace query no longer triggers per-request cuengine spawns. ## Out of scope (separate follow-up) - **Pre-populating on receive-pack** so even the FIRST read after a push is free. Lazy fill is fine for now — semantics change from "evaluate on every request" to "evaluate at most once per commit per process". - Cache size eviction. Process restart clears everything; that's the only eviction. ## Memory pointer This PR honours [Cache CUE evaluation per commit] guidance: CUE evaluation never runs in the hot request path again. The receive-pack validator still runs CUE (that's the validation point, not the read path), which is correct.
The \`main\` branch ruleset requires CodeQL (\`alerts_threshold: errors\`) but no CodeQL workflow exists. Every PR targeting \`main\` is blocked indefinitely because the rule cannot evaluate without a scan. This adds the missing workflow so v3 → main (PR #1) and future merges can clear. ## Coverage - **rust** — extracts via \`cargo build --workspace\`, same stable Rust + Go toolchain as ci.yml's rust job (cuengine's Go FFI build). - **javascript-typescript** — source-only extraction. Covers \`.ts\`, \`.tsx\`, \`.js\`, and \`<script>\` blocks inside \`.vue\` files (the JS/TS extractor reads Vue SFCs natively). Query pack: \`security-and-quality\`. Ruleset only blocks on \`errors\`, so quality-pack \`warning\`/\`notice\` items surface in the Security tab without gating merges. ## Triggers - \`pull_request\` and \`push\` to \`main\`/\`v3\` — so the ruleset auto-merge condition clears immediately after a green run. - Weekly cron (\`13 6 * * 1\`) — so newly-published CodeQL queries flag regressions on existing code without a push. ## Permissions Least-privilege: \`security-events: write\` for findings upload, \`actions: read\`/\`contents: read\` for the extractor. No \`run:\` step consumes untrusted external input. ## What clears after this lands PR #1 (v3 → main) has \`autoMergeRequest\` armed for squash; once a CodeQL run posts a passing analysis on v3, the ruleset's \`code_scanning\` requirement clears and the queue picks up the merge. If the first scan returns actual \`error\`-level findings, I'll fix them in a follow-up before #1 can land — but the expectation on a Rust + TS codebase this shape is a clean scan or only \`warning\`/\`notice\` items.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function Gi(e, t, n) { | ||
| let i = e.type; | ||
| if (!e.render) { | ||
| if (!t && Ui && !i.render) { |
| function Gi(e, t, n) { | ||
| let i = e.type; | ||
| if (!e.render) { | ||
| if (!t && Ui && !i.render) { |
| N(); | ||
| let n = e.setupContext = r.length > 1 ? qi(e) : null, i = Ii(e), a = Kt(r, e, 0, [e.props, n]), o = y(a); | ||
| if (P(), i(), (o || e.sp) && !Mn(e) && Dn(e), o) { | ||
| if (a.then(Li, Li), t) return a.then((n) => { |
| return yn(e, t, n); | ||
| } | ||
| function yn(e, n, i = t) { | ||
| let { immediate: a, deep: o, flush: c, once: l } = i, u = s({}, i), d = n && a || !n && c !== "post", f; |
| function nr(e) { | ||
| let t = or(e), n = e.proxy, i = e.ctx; | ||
| tr = !1, t.beforeCreate && ir(t.beforeCreate, e, "bc"); | ||
| let { data: a, computed: o, methods: s, watch: c, provide: l, inject: u, created: f, beforeMount: p, mounted: m, beforeUpdate: g, updated: _, activated: y, deactivated: b, beforeDestroy: x, beforeUnmount: S, destroyed: C, unmounted: w, render: ee, renderTracked: te, renderTriggered: ne, errorCaptured: T, serverPrefetch: re, expose: E, inheritAttrs: ie, components: ae, directives: D, filters: oe } = t; |
| return yn(e, t, n); | ||
| } | ||
| function yn(e, n, i = t) { | ||
| let { immediate: a, deep: o, flush: c, once: l } = i, u = s({}, i), d = n && a || !n && c !== "post", f; |
| }, r), o)); | ||
| } | ||
| } | ||
| e.render = i.render || r, Wi && Wi(e); |
| function nr(e) { | ||
| let t = or(e), n = e.proxy, i = e.ctx; | ||
| tr = !1, t.beforeCreate && ir(t.beforeCreate, e, "bc"); | ||
| let { data: a, computed: o, methods: s, watch: c, provide: l, inject: u, created: f, beforeMount: p, mounted: m, beforeUpdate: g, updated: _, activated: y, deactivated: b, beforeDestroy: x, beforeUnmount: S, destroyed: C, unmounted: w, render: ee, renderTracked: te, renderTriggered: ne, errorCaptured: T, serverPrefetch: re, expose: E, inheritAttrs: ie, components: ae, directives: D, filters: oe } = t; |
| Ok((repository.to_string(), title.to_string())) | ||
| } | ||
|
|
||
| fn decode_stored_issue(id: &str, bytes: &[u8]) -> Result<StoredIssue, Error> { |
| scope.workspace_id.is_some() || scope.repository_id.is_some() | ||
| } | ||
|
|
||
| fn decode(id: &str, bytes: &[u8]) -> Result<StoredPullRequest, Error> { |
| }) | ||
| } | ||
|
|
||
| fn string_payload(input: &Value, op: &str) -> Result<String, wit_types::Error> { |
| }) | ||
| } | ||
|
|
||
| fn string_vec_payload(input: &Value, op: &str) -> Result<Vec<String>, wit_types::Error> { |
| .collect() | ||
| } | ||
|
|
||
| fn u32_field(input: &Value, field: &str, op: &str) -> Result<u32, wit_types::Error> { |
| scope.workspace_id.is_some() | ||
| } | ||
|
|
||
| fn decode(id: &str, bytes: &[u8]) -> Result<StoredCheck, Error> { |
| Ok((id.clone(), format!("comtrya://workspace/{id}"))) | ||
| } | ||
|
|
||
| fn decode(id: &str, bytes: &[u8]) -> Result<StoredEpic, Error> { |
…1) (#50) **Draft — foundation only.** First of two PRs that close #8 P1-1. This one builds the injection plumbing and migrates the **shell** sites to consume it. The extension-side \`?? DEFAULT_WORKSPACE_ID\` fallbacks stay in place — they come off in PR 2 (\"Extension fallback removal\") once the foundation is proven. ## Background #8 P1-1 was previously attempted and aborted at the 3-round Stage-1 review cap. Each review round uncovered a deeper architectural problem than the previous: 1. \`onMounted\` inside a singleton composable silently no-ops outside Vue setup. 2. \`extensionElementContext()\` doesn't currently inject \`workspaceId\`, so removing extension fallbacks first would break \`/x/issues/\`, \`/x/pulls/\`, etc. 3. Module-scope command bindings in extensions hardcode the ULID directly and are called from \`setup(host)\` BEFORE \`extensionElementContext()\` runs. The recommended fix from that abort: **split into two staged PRs**. This is PR 1 — the foundation. ## What lands here - New \`frontend/src/workspace-store.ts\`: tiny store with \`setActiveWorkspaceId\`, \`activeWorkspaceId\`, \`subscribeWorkspaceId\`. Eager — populated by \`App.vue::loadShellSummary\` on boot. Replaces the per-file ULID constant in shell code. - \`App.vue::loadShellSummary\` calls \`setActiveWorkspaceId(workspaceId)\` after the workspace query lands. - \`extension-runtime.ts::extensionElementContext()\` adds \`workspaceId: activeWorkspaceId()\` to the element context map. - New \`@comtrya/sdk-vue::useWorkspaceContext()\` Vue composable. Returns \`{ workspaceId: Ref<string | null>, workspaceUri: ComputedRef<string | null>, ready: Promise<string> }\`. Auto-subscribes; tears down on unmount. - 4 shell sites migrated to consume the composable instead of the hardcoded ULID: - \`frontend/src/routes/Inbox.vue\` - \`frontend/src/routes/ProjectHome.vue\` - \`frontend/src/components/CommandPalette.vue\` - \`frontend/packages/sdk-vue/src/use-project-counts.ts\` ## What does NOT land here - Extension-side ULID fallbacks (\`?? DEFAULT_WORKSPACE_ID\` in \`ext_issues\`, \`ext_epics\`, \`ext_pull_requests\`). - \`grep -rn ws_01HV0K4XAVE2H6R5M8KJZ8Q1A3 .\` returning 0 hits. That's PR 2's acceptance criterion. - Command-binding refactor in extensions (\`pr-commands.ts\`, \`issue-commands.ts\`, \`epic-commands.ts\`). Module-scope bindings need a live getter passed through \`setup(host)\` — addressed in PR 2. ## Plan See \`PLAN_P1-1_FOUNDATION.md\` on this branch for the full sequence, including the timing-race analysis for the \`useProjectCounts\` composable and the \`ready: Promise<string>\` design. ## Acceptance for this PR - [ ] \`workspace-store.ts\` + \`useWorkspaceContext()\` exist and are unit-tested. - [ ] \`extensionElementContext()\` injects \`workspaceId\`. - [ ] All 4 shell sites consume the composable; no hardcoded ULID in shell code (\`grep -rn ws_01HV0K4XAVE2H6R5M8KJZ8Q1A3 frontend/src frontend/packages/sdk-vue\` returns 0). - [ ] Extension sites still compile and run unchanged (fallback path). - [ ] Browser verification: shell loads, Inbox renders, ProjectHome renders, CommandPalette opens, all using the dynamic workspace ID. Closes #8 P1-1 after the follow-up PR lands.
Two CodeQL alerts at \`high\` security severity (\`js/polynomial-redos\`) were the actual blocker on PR #1 (v3 → main). Both rewritten to strictly linear scans. ## Findings | Alert | File | Old pattern | |---|---|---| | #1 | \`sdk-core/route-registry.ts:54\` | \`normalized.replace(/\/+$/, "")\` | | #2 | \`sdk-vue/parse-query.ts:54\` | \`/([A-Za-z][\w-]*):"([^"]*)"\|([A-Za-z][\w-]*):(\S*)\|(\S+)/g\` | CodeQL's reasoning: each regex contains a quantifier reachable on user-controllable input where backtracking could push runtime into polynomial (or worse) time on adversarial inputs. ## Fixes **route-registry.ts** — hand loop that walks the trailing characters once, stopping at the first non-\`/\`. Linear, never collapses the single-slash root. **parse-query.ts** — whitespace-bounded tokenizer + per-token \`indexOf\` split for \`key:value\`. Quoted runs (\`is:"with spaces"\`) handled by an inline state switch; key validation rewritten as a char-code loop. The \`ParsedQuery\` shape and \`parseQueryFilters\` signature are unchanged, so PullsQueue / IssuesList / EpicsList callers need no edits. ## Verification - \`bun test\` — **73/73 green** (existing parse-query.test.ts covers the kept behaviour). - \`bun run typecheck\` — clean. - \`bun run build\` — clean. After this lands, PR #1 (v3 → main) should clear the \`code_scanning\` merge-queue rule (\`security_alerts_threshold: high_or_higher\` — these were the only two high-sev alerts on the merge ref).
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v3 brings v1's load-bearing implementation pieces back into v2's kernel-shaped architecture:
See
V3_PLAN.mdfor the increment plan.Increment plan
crates/git-http)git http-backendshell adapterSPEC_COVERAGE.md,TODO.md,start.shEach increment ships as its own commit so the PR shows a real progression.
Test plan
🤖 Generated with Claude Code