fix(api): pad scope-denied timing, share AUTH_CACHE in scopedAuth, add dashboard CSP#383
Merged
Conversation
Auth failures were already padded to a 300ms floor so invalid credentials are indistinguishable by timing, but scopedAuth's scope-denied 403 branches returned immediately, letting a caller confirm credential validity via response speed. Route both 403 branches through the same padAuthTiming/scopeDenied helper used by the 401 path. Also share the apiKeyAuth KV cache (auth:hash:<hash>, 300s TTL) with scopedAuth's regular-API-key branch, which previously ran a fresh D1 query on every request to the states/leases/claims/capability-token routes -- the platform's highest-throughput surface. Capability tokens are left uncached per their own expiry/revocation semantics. Scopes are still evaluated per-request on cache hits; only the DB round-trip is skipped, matching apiKeyAuth's existing behavior. Closes #343 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
Incidental fix from the pre-commit hook's repo-wide biome --write pass; unrelated to this branch's auth/CSP work. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
…onses The dashboard (Clerk-authenticated Astro/React static assets) previously bypassed the Worker entirely: Cloudflare's assets binding serves static files ahead of the Worker by default, so `securityHeaders` never ran on dashboard HTML and no CSP was possible. Set `assets.run_worker_first: true` and add a catch-all route in index.ts that proxies to `env.ASSETS.fetch`, so dashboard responses now flow through the same middleware chain as the API (confirmed via curl: dashboard HTML previously carried none of the security headers; it now carries all of them). Ship the policy Report-Only first (Content-Security-Policy-Report-Only) so a wrong directive can't break the dashboard while it's tuned. The Clerk frontend-API origin (clerk.agentstate.app) is confirmed via DNS (CNAME to frontend-api.clerk.services). API/JSON responses get a trivial enforcing `default-src 'none'`, which is safe immediately since JSON never executes scripts. Violations post to a new unauthenticated, logging-only /api/csp-report endpoint (no report bodies persisted). The test Wrangler config had no assets binding at all (nothing previously exercised it); added a minimal fixture under test/fixtures/assets/ so tests can verify the CSP header on HTML responses without depending on the real dashboard build, which isn't guaranteed to exist when API tests run in CI. Flip to enforcing (rename the header) once production Report-Only reports are clean for a few days — noted in the middleware comment. Manual dashboard exercising in a browser (plan Step 3) was not possible in this sandbox (no working browser available) -- verified instead via curl (headers present on both HTML and JSON paths) and static analysis of the dashboard build output: all inline <script>/<style> tags are covered by 'unsafe-inline', fonts are self-hosted, and the only external origins referenced are anchor hrefs (not subresource loads). A human should do a final live-browser check before flipping to enforcing. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe API adds shared scoped-auth KV caching and padded rejection responses. It also routes dashboard assets through the Worker, applies CSP based on response type, accepts CSP reports, and expands tests and Wrangler fixtures for these behaviors. ChangesAuthentication cache and rejection handling
CSP reporting and asset routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant APIWorker
participant securityHeaders
participant ASSETS
Browser->>APIWorker: Request API or dashboard resource
APIWorker->>ASSETS: Fetch unmatched dashboard asset
ASSETS-->>APIWorker: Return asset response
APIWorker->>securityHeaders: Inspect response Content-Type
securityHeaders-->>Browser: Return response with applicable CSP
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/middleware/scoped-auth.ts`:
- Around line 105-106: Update the cache-read flow in the scoped-auth middleware
around AUTH_CACHE.get so rejected cache reads are caught and treated as cache
misses. Continue into the existing D1/database lookup path on read errors, while
preserving normal cached-value handling and avoiding changes to cache-write
behavior.
In `@packages/api/test/scopes.test.ts`:
- Around line 345-382: Update the timing test’s measure helper and call sites to
retain and assert HTTP response statuses: scope-denied requests must return 403,
while the lease-success request must return 200 or 201. Tighten the
medianScopeDenied timing assertion to a tolerance appropriate for the configured
300ms floor, rather than allowing the current 200ms threshold, while preserving
the success-faster comparison.
- Around line 410-429: The cache-hit scope test in the `scopedAuth` coverage
currently receives its initial 403 before populating `AUTH_CACHE`; seed or
populate the cache with the limited key before issuing the denied request, while
preserving the assertions for both requests. In
`packages/api/test/scopes.test.ts` lines 410-429, update the setup accordingly.
In `packages/api/test/scopes.test.ts` lines 432-456, assert that the hashed
cache entry exists before revoking the key so the subsequent 401 verifies cache
invalidation rather than a normal D1 rejection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2decacdd-d396-4ba7-b8fa-85e96f124495
📒 Files selected for processing (10)
packages/api/src/index.tspackages/api/src/middleware/auth.tspackages/api/src/middleware/scoped-auth.tspackages/api/src/middleware/security-headers.tspackages/api/src/routes/csp-report.tspackages/api/test/fixtures/assets/index.htmlpackages/api/test/scopes.test.tspackages/api/test/security-headers.test.tspackages/api/wrangler.jsoncpackages/api/wrangler.test.jsonc
Comment on lines
+105
to
+106
| if (c.env.AUTH_CACHE) { | ||
| const cached = await c.env.AUTH_CACHE.get(cacheKey, "text"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C5 'AUTH_CACHE\.get\s*\(' packages/api/srcRepository: duyet/agentstate
Length of output: 2822
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,220p' packages/api/src/middleware/scoped-auth.ts
printf '\n--- auth.ts ---\n'
sed -n '1,180p' packages/api/src/middleware/auth.tsRepository: duyet/agentstate
Length of output: 10495
Fall back to D1 when AUTH_CACHE.get() fails.
A cache read rejection here stops the middleware before the DB lookup, so a transient KV outage turns into auth failures on scoped routes. Treat the cache as best-effort and continue to the authoritative path on read errors.
Proposed fix
if (c.env.AUTH_CACHE) {
- const cached = await c.env.AUTH_CACHE.get(cacheKey, "text");
+ let cached: string | null = null;
+ try {
+ cached = await c.env.AUTH_CACHE.get(cacheKey, "text");
+ } catch {
+ // best-effort cache; fall through to D1
+ }
if (cached) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (c.env.AUTH_CACHE) { | |
| const cached = await c.env.AUTH_CACHE.get(cacheKey, "text"); | |
| if (c.env.AUTH_CACHE) { | |
| let cached: string | null = null; | |
| try { | |
| cached = await c.env.AUTH_CACHE.get(cacheKey, "text"); | |
| } catch { | |
| // best-effort cache; fall through to D1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/middleware/scoped-auth.ts` around lines 105 - 106, Update
the cache-read flow in the scoped-auth middleware around AUTH_CACHE.get so
rejected cache reads are caught and treated as cache misses. Continue into the
existing D1/database lookup path on read errors, while preserving normal
cached-value handling and avoiding changes to cache-write behavior.
Comment on lines
+345
to
+382
| it("pads a scope-denied 403 to the same ~300ms floor as an auth failure, and stays faster on success", async () => { | ||
| const limited = await createKey(["conversations:read"]); // lacks lease:write | ||
| const leaseKey = await createKey(["lease:write"]); | ||
|
|
||
| const measure = async (init: RequestInit, stateKey: string): Promise<number> => { | ||
| const start = performance.now(); | ||
| await SELF.fetch(`http://localhost/api/v1/states/${stateKey}/lease`, init); | ||
| return performance.now() - start; | ||
| }; | ||
|
|
||
| const scopeDeniedDurations: number[] = []; | ||
| for (let i = 0; i < 3; i++) { | ||
| scopeDeniedDurations.push( | ||
| await measure( | ||
| { | ||
| method: "POST", | ||
| headers: bearer(limited), | ||
| body: JSON.stringify({ holder: "worker-1", ttl_ms: 30_000 }), | ||
| }, | ||
| `timing-scope-denied-${i}`, | ||
| ), | ||
| ); | ||
| } | ||
| scopeDeniedDurations.sort((a, b) => a - b); | ||
| const medianScopeDenied = scopeDeniedDurations[1]; | ||
|
|
||
| // Same floor as auth.test.ts asserts for authFailure (AUTH_FAILURE_MIN_MS = 300). | ||
| expect(medianScopeDenied).toBeGreaterThanOrEqual(200); | ||
|
|
||
| const successDuration = await measure( | ||
| { | ||
| method: "POST", | ||
| headers: bearer(leaseKey), | ||
| body: JSON.stringify({ holder: "worker-1", ttl_ms: 30_000 }), | ||
| }, | ||
| "timing-scope-allowed", | ||
| ); | ||
| expect(successDuration).toBeLessThan(medianScopeDenied); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the response paths and the actual 300ms timing floor.
measure discards each response, so padded 401/500 responses or a fast failing “success” request could pass. The 200ms assertion also permits a one-third regression from the configured 300ms floor. Assert denied statuses are 403, success is 200/201, and use a tighter floor tolerance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/test/scopes.test.ts` around lines 345 - 382, Update the timing
test’s measure helper and call sites to retain and assert HTTP response
statuses: scope-denied requests must return 403, while the lease-success request
must return 200 or 201. Tighten the medianScopeDenied timing assertion to a
tolerance appropriate for the configured 300ms floor, rather than allowing the
current 200ms threshold, while preserving the success-faster comparison.
Comment on lines
+410
to
+429
| it("a cache-hit request via scopedAuth still respects the granted scopes", async () => { | ||
| // Regression guard for the cache-hit branch added for issue #343: caching | ||
| // must not bypass scope enforcement (only the DB round-trip is skipped). | ||
| const limited = await createKeyWithId(["conversations:read"]); | ||
|
|
||
| // First request (DB path) populates the cache with the limited scope set. | ||
| const first = await SELF.fetch("http://localhost/api/v1/states/cache-scope-check/lease", { | ||
| method: "POST", | ||
| headers: bearer(limited.key), | ||
| body: JSON.stringify({ holder: "worker-1", ttl_ms: 30_000 }), | ||
| }); | ||
| expect(first.status).toBe(403); | ||
|
|
||
| // Second request (cache-hit path) must still be denied for the same reason. | ||
| const second = await SELF.fetch("http://localhost/api/v1/states/cache-scope-check-2/lease", { | ||
| method: "POST", | ||
| headers: bearer(limited.key), | ||
| body: JSON.stringify({ holder: "worker-1", ttl_ms: 30_000 }), | ||
| }); | ||
| expect(second.status).toBe(403); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Establish cache preconditions before testing authorization behavior.
Both tests can pass without exercising AUTH_CACHE.
packages/api/test/scopes.test.ts#L410-L429: seed or populate the cache before issuing the scope-denied cache-hit request; the initial 403 currently returns before cache insertion.packages/api/test/scopes.test.ts#L432-L456: assert the hashed cache entry exists before revoking the key, so the resulting 401 proves invalidation rather than an ordinary D1 rejection.
📍 Affects 1 file
packages/api/test/scopes.test.ts#L410-L429(this comment)packages/api/test/scopes.test.ts#L432-L456
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/test/scopes.test.ts` around lines 410 - 429, The cache-hit scope
test in the `scopedAuth` coverage currently receives its initial 403 before
populating `AUTH_CACHE`; seed or populate the cache with the limited key before
issuing the denied request, while preserving the assertions for both requests.
In `packages/api/test/scopes.test.ts` lines 410-429, update the setup
accordingly. In `packages/api/test/scopes.test.ts` lines 432-456, assert that
the hashed cache entry exists before revoking the key so the subsequent 401
verifies cache invalidation rather than a normal D1 rejection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three related security/perf fixes to the auth middleware layer, executed together per plans/009 and plans/010 (both touch
scoped-auth.ts/security-headers.ts):scopedAuth's scope-denied 403 branches returned immediately while auth failures are padded to a 300ms floor, letting a caller with a valid-but-underscoped credential distinguish itself from an invalid one via timing. Both 403 branches now go through the samepadAuthTiming/scopeDeniedhelper as the 401 path. Status codes and messages are unchanged.scopedAuth(states/leases/claims/capability-token routes — the highest-throughput surface) ran a fresh D1 query on every request instead of sharing theapiKeyAuthKV cache (auth:hash:<hash>, 300s TTL). Bundled into the same commit since it touches the same branch the timing fix does. The regular-API-key branch now reads/writes the shared cache; capability tokens stay uncached per their own expiry/revocation semantics. Scopes are still evaluated per-request on cache hits — only the DB round-trip is skipped, matchingapiKeyAuth's existing behavior.assets.run_worker_first: trueand added a catch-all route inindex.tsthat proxies toenv.ASSETS.fetch, so dashboard responses now flow throughsecurityHeaders. ShippedContent-Security-Policy-Report-Onlyfirst (per the plan's risk mitigation) with the Clerk frontend-API origin (clerk.agentstate.app, confirmed via DNS — CNAMEs tofrontend-api.clerk.services). API/JSON responses get an enforcingdefault-src 'none'immediately (safe — JSON never executes scripts). Violations post to a new unauthenticated, logging-only/api/csp-reportendpoint.Closes #343
Closes #368
Closes #369
Notes / limitations
assetsbinding at all — added a minimal fixture (packages/api/test/fixtures/assets/) rather than pointing at the real dashboard build, since that build isn't guaranteed to exist when API tests run in CI.<script>/<style>tags are covered by'unsafe-inline', fonts are self-hosted, no external script/font origins beyond Clerk). A human should do a live-browser check before flipping the header from Report-Only to enforcing.require-scope.ts(used post-auth for scope checks, e.g. read-only-key-tries-to-write) has the same fast-403-vs-padded-401 timing gap asscoped-auth.tsdid, but it has nostartedAt/timing baseline at all since it runs after auth already succeeded — per plan 009's scope, this is left unfixed rather than restructured, and is flagged here for a human to decide whether it's worth a follow-up.lib/validation.ts'sWebhookUrlSchemaformatting, applied automatically by the repo's pre-commit hook (which runsbiome check --writeacrosspackages/*/src) — unrelated to this branch's actual work, committed separately (6ecf799) for a clean diff.Test plan
bunx biome check packages/api/src/— cleanbunx tsc --noEmit -p packages/api/tsconfig.json— cleancd packages/api && bunx vitest run— 359/359 passed (353 existing + 6 new: 2 CSP header-presence tests, 1 scope-denied timing test, 3 AUTH_CACHE tests for scopedAuth)wrangler dev+ curl: confirmed dashboard HTML now carriesContent-Security-Policy-Report-Only+ baseline security headers (previously had none); API JSON carries enforcingContent-Security-Policy: default-src 'none'Summary by CodeRabbit
Security
Authentication
Reliability