feat(auth-service): nonce-based CSP and 5 security cucumber scenarios#100
Open
feat(auth-service): nonce-based CSP and 5 security cucumber scenarios#100
Conversation
🦋 Changeset detectedLatest commit: 1da0ec6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughPer-request CSP nonces are generated and threaded into auth-service templates to replace 'unsafe-inline' for inline scripts. The /metrics endpoint now uses deny-by-default HTTP Basic auth via a new helper. Tests and e2e scenarios were added/updated for CSRF, CSP, security headers, and metrics access. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Auth as AuthService
participant MW as SecurityHeadersMiddleware
participant Router as RouteHandler
participant Template as TemplateRenderer
participant Browser
Client->>Auth: GET /auth/login
Auth->>MW: invoke security headers middleware
MW->>MW: generate nonce (randomBytes -> base64url)
MW->>Auth: set res.locals.cspNonce
Auth->>Router: route to handler
Router->>Template: render page with cspNonce
Template->>Router: return HTML with <script nonce="...">
Router->>Auth: send response
Auth->>Browser: HTTP response + CSP header (script-src 'nonce-<value>') + body
Browser->>Browser: execute inline script only if nonce matches
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
🚅 Deployed to the ePDS-pr-100 environment in ePDS
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/auth-service/src/routes/choose-handle.ts (1)
315-326:⚠️ Potential issue | 🟠 MajorInclude the nonce in this error render path.
This catch branch is the only POST error render that omits
res.locals.cspNonce, so the returned handle picker page will have a nonce-only CSP but a non-nonced inline script.🔒 Proposed fix
renderChooseHandlePage( handleDomain, 'Could not verify handle availability. Please try again.', res.locals.csrfToken, showRandomButton, customCss, + res.locals.cspNonce as string, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 315 - 326, The error branch that returns the handle picker omits the CSP nonce, causing a mismatch between the page CSP and inline scripts; update the render call in the catch block so renderChooseHandlePage receives res.locals.cspNonce (like other error paths do) — locate the catch that logs with logger.error({ err, fullHandle }, 'Failed to check handle availability') and pass res.locals.cspNonce into the renderChooseHandlePage invocation along with handleDomain, the error message, res.locals.csrfToken, showRandomButton, and customCss.packages/auth-service/src/routes/preview.ts (1)
215-243:⚠️ Potential issue | 🟠 MajorPass the CSP nonce to preview handle pages too.
/preview/choose-handleand/preview/choose-handle-pickerstill renderrenderChooseHandlePage()withoutres.locals.cspNonce, so their inline handle-checking script will be blocked by the new nonce-only CSP.🔒 Proposed fix
renderChooseHandlePage( FAKE_HANDLE_DOMAIN, queryString(req, 'error'), fakeCsrfToken(), true, css, + res.locals.cspNonce as string, ), @@ renderChooseHandlePage( FAKE_HANDLE_DOMAIN, queryString(req, 'error'), fakeCsrfToken(), false, css, + res.locals.cspNonce as string, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/preview.ts` around lines 215 - 243, The preview routes for '/preview/choose-handle' and '/preview/choose-handle-picker' call renderChooseHandlePage without including the CSP nonce, so the inline handle-checking script will be blocked; update both handlers (the async functions that call getBranding and sendHtml) to read res.locals.cspNonce and pass it into renderChooseHandlePage (alongside FAKE_HANDLE_DOMAIN, queryString(req,'error'), fakeCsrfToken(), the boolean flag, and css) so the page gets the nonce for its inline script.packages/auth-service/src/routes/login-page.ts (1)
394-413:⚠️ Potential issue | 🟠 MajorMove the OTP
oninputhandler into the nonce-stamped script.With
'unsafe-inline'removed fromscript-src, CSP blocks the inlineoninput="..."attribute on the OTP field; the nonce on line 413 authorizes only<script nonce="...">blocks, not inline event handlers. Attach this listener inside the script instead.Proposed fix
<input type="text" id="code" name="code" required maxlength="${opts.otpLength}" pattern="${inputProps.pattern}" inputmode="${inputProps.inputmode}" autocomplete="one-time-code" placeholder="${inputProps.placeholder}" class="otp-input" autocapitalize="${inputProps.autocapitalize}" - oninput="this.value=this.value.replace(/[\\s-]/g,'')" style="letter-spacing: ${Math.max(2, Math.round(32 / opts.otpLength))}px"> @@ var recoveryLink = document.getElementById('recovery-link'); + var codeInput = document.getElementById('code'); + + codeInput.addEventListener('input', function() { + this.value = this.value.replace(/[\\s-]/g, ''); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 394 - 413, Remove the inline oninput attribute from the OTP input (id="code", class="otp-input") and instead attach the same sanitizing listener inside the existing nonce-stamped script (the <script nonce="${escapeHtml(opts.cspNonce)}"> block); locate where the page writes the script and add document.getElementById('code')?.addEventListener('input', ...) to replace spaces and hyphens (same regex used inline), preserving behavior like maxlength/pattern/inputmode from opts and without adding any inline handlers so the CSP nonce authorizes the logic.packages/auth-service/src/index.ts (1)
40-50:⚠️ Potential issue | 🔴 CriticalUse timing-safe comparison for admin password authentication.
Line 76 compares the
Authorizationheader against a secret-derived value using!==. Replace withtimingSafeEqual()(available from@certified-app/shared) to prevent timing attacks:Suggested fix
import { timingSafeEqual } from '@certified-app/shared' // ... if (!authHeader || !timingSafeEqual(authHeader, expected)) { res.status(401).json({ error: 'Unauthorized' }) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/index.ts` around lines 40 - 50, The admin auth comparison currently uses a plain !== check on authHeader vs expected; import timingSafeEqual from '@certified-app/shared' and replace that direct comparison with a timing-safe check by converting both sides to Buffers (e.g., Buffer.from(authHeader) and Buffer.from(expected)) and calling timingSafeEqual(bufferA, bufferB); ensure you still handle missing authHeader or expected early (respond 401) and update the conditional that currently uses authHeader/expected !== to use the timingSafeEqual result instead.
🧹 Nitpick comments (3)
e2e/step-definitions/security.steps.ts (2)
56-64: UseHeaders.getSetCookie()for multi-valuedSet-Cookie.Node's
Headers.get('set-cookie')joins multipleSet-Cookievalues with,— but because cookieExpiresattributes themselves contain commas, the combined string is ambiguous and easy to misparse. For theepds_csrf=substring check this still works today, but it's fragile: any future check that tries to inspect the cookie value would be broken by the join. Prefer the dedicatedgetSetCookie()accessor (Node 20+/undici), which returnsstring[].Suggested change
-Then('the response sets a CSRF cookie', function (this: EpdsWorld) { - const { headers } = getCapturedResponse(this) - const setCookie = headers.get('set-cookie') ?? '' - if (!/epds_csrf=/.test(setCookie)) { +Then('the response sets a CSRF cookie', function (this: EpdsWorld) { + const { headers } = getCapturedResponse(this) + const cookies = headers.getSetCookie() + if (!cookies.some((c) => /^epds_csrf=/.test(c))) { throw new Error( - `Expected Set-Cookie to include epds_csrf=..., got: ${setCookie || '(none)'}`, + `Expected Set-Cookie to include epds_csrf=..., got: ${cookies.join(' | ') || '(none)'}`, ) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/security.steps.ts` around lines 56 - 64, Replace the fragile use of headers.get('set-cookie') in the Then step testing CSRF cookie with the multi-value accessor headers.getSetCookie(); in the step function (the Then handler using getCapturedResponse(this)), call headers.getSetCookie() to get a string[] (or empty array if undefined), then assert that some element matches /epds_csrf=/ and throw the same error message if none match, preserving getCapturedResponse(this) and the step name.
139-155:/preview/login404 fallback to/healthsilently weakens the CSP scenario.If the preview route is disabled in the environment under test, the step silently probes
/healthinstead. Since the middleware applies CSP globally, the nonce/unsafe-inlineassertions still pass — but the scenario's stated intent ("the login page is loaded") is no longer validated, and a regression where the login route specifically loses CSP would go undetected. Consider either requiring previews in the test env (fail on 404) or probing the real login route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/security.steps.ts` around lines 139 - 155, The step When('the login page is loaded', async function (this: EpdsWorld) currently falls back from previewUrl (`${testEnv.authUrl}/preview/login`) to `/health` on a 404, which weakens the intended "login page is loaded" assertion; change the behavior in this step so that a 404 on previewUrl fails the test (throw or set a failing assertion) or instead probe the real login route (e.g., `${testEnv.authUrl}/login` or the actual auth-service login endpoint) and use that response for setCapturedResponse(this, ...), ensuring the test validates the actual login page CSP rather than a generic `/health` response.packages/shared/src/preview-ui.ts (1)
491-494: Consider defensive escaping (or validation) ofcspNoncein the attribute.
cspNonceis injected raw into a double-quoted HTML attribute. In practice auth-service generates it viacrypto.randomBytes(16).toString('base64url')so it's restricted to[A-Za-z0-9_-]and safe, but this helper is exported and could be called by future code. A one-line guard (orescapeHtml) keeps the contract defensive against an accidental caller that passes through unvalidated input.Optional hardening
export function previewClientIdScriptHtml(cspNonce?: string): string { - const nonceAttr = cspNonce ? ` nonce="${cspNonce}"` : '' + if (cspNonce && !/^[A-Za-z0-9_-]+$/.test(cspNonce)) { + throw new Error('cspNonce must be base64url-safe') + } + const nonceAttr = cspNonce ? ` nonce="${cspNonce}"` : '' return `<script${nonceAttr}>\n${PREVIEW_CLIENT_ID_SCRIPT_BODY}</script>` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/preview-ui.ts` around lines 491 - 494, The previewClientIdScriptHtml helper currently injects cspNonce raw into nonceAttr; make this defensive by validating or escaping cspNonce before interpolation in previewClientIdScriptHtml (e.g., in the function sanitize the input used to build nonceAttr: either run a strict whitelist check for /^[A-Za-z0-9_-]+$/ and only use it when it passes, or pass it through an HTML-attribute escape helper like escapeHtml and fall back to empty string on failure), so update previewClientIdScriptHtml to compute a safe nonce value and then build nonceAttr from that safe value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/__tests__/security-headers.test.ts`:
- Around line 32-51: The withServer helper nests listen and close callbacks too
deeply; refactor by extracting small helpers to start and stop the server and
use async/await with try/finally to ensure cleanup: create a startServer
function that returns the server and bound port (use app.listen and AddressInfo)
and a closeServer helper that closes the server, then in withServer await
startServer(), call fn(baseUrl) inside a try block and always await
closeServer() in finally so you remove nested .then/.catch callbacks around
server.close and satisfy SonarCloud nesting rules.
In `@packages/auth-service/src/index.ts`:
- Around line 73-76: The direct string comparison authHeader !== expected must
be replaced with the constant-time helper timingSafeEqual to avoid timing
attacks: convert both values to Buffers (ensure authHeader is a string or use an
empty string if missing), check lengths equal, then call
timingSafeEqual(authBuf, expectedBuf) and branch on its boolean result instead
of !==; update the conditional around authHeader/expected (the symbols
authHeader, expected) to use this helper so secret comparisons are done in
constant time.
In `@packages/auth-service/src/middleware/security-headers.ts`:
- Around line 37-40: The code can pass a string[] to db.getAuthFlowByRequestUri
because req.query.request_uri is typed string | string[] | undefined; update the
check that sets clientId so it only calls db.getAuthFlowByRequestUri when typeof
req.query.request_uri === 'string' (e.g., guard req.query.request_uri with a
typeof check) and avoid casting with as string; reference the
req.query.request_uri usage and db.getAuthFlowByRequestUri call and ensure
clientId is only assigned from db.getAuthFlowByRequestUri(req.query.request_uri)
when the guard passes.
---
Outside diff comments:
In `@packages/auth-service/src/index.ts`:
- Around line 40-50: The admin auth comparison currently uses a plain !== check
on authHeader vs expected; import timingSafeEqual from '@certified-app/shared'
and replace that direct comparison with a timing-safe check by converting both
sides to Buffers (e.g., Buffer.from(authHeader) and Buffer.from(expected)) and
calling timingSafeEqual(bufferA, bufferB); ensure you still handle missing
authHeader or expected early (respond 401) and update the conditional that
currently uses authHeader/expected !== to use the timingSafeEqual result
instead.
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 315-326: The error branch that returns the handle picker omits the
CSP nonce, causing a mismatch between the page CSP and inline scripts; update
the render call in the catch block so renderChooseHandlePage receives
res.locals.cspNonce (like other error paths do) — locate the catch that logs
with logger.error({ err, fullHandle }, 'Failed to check handle availability')
and pass res.locals.cspNonce into the renderChooseHandlePage invocation along
with handleDomain, the error message, res.locals.csrfToken, showRandomButton,
and customCss.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 394-413: Remove the inline oninput attribute from the OTP input
(id="code", class="otp-input") and instead attach the same sanitizing listener
inside the existing nonce-stamped script (the <script
nonce="${escapeHtml(opts.cspNonce)}"> block); locate where the page writes the
script and add document.getElementById('code')?.addEventListener('input', ...)
to replace spaces and hyphens (same regex used inline), preserving behavior like
maxlength/pattern/inputmode from opts and without adding any inline handlers so
the CSP nonce authorizes the logic.
In `@packages/auth-service/src/routes/preview.ts`:
- Around line 215-243: The preview routes for '/preview/choose-handle' and
'/preview/choose-handle-picker' call renderChooseHandlePage without including
the CSP nonce, so the inline handle-checking script will be blocked; update both
handlers (the async functions that call getBranding and sendHtml) to read
res.locals.cspNonce and pass it into renderChooseHandlePage (alongside
FAKE_HANDLE_DOMAIN, queryString(req,'error'), fakeCsrfToken(), the boolean flag,
and css) so the page gets the nonce for its inline script.
---
Nitpick comments:
In `@e2e/step-definitions/security.steps.ts`:
- Around line 56-64: Replace the fragile use of headers.get('set-cookie') in the
Then step testing CSRF cookie with the multi-value accessor
headers.getSetCookie(); in the step function (the Then handler using
getCapturedResponse(this)), call headers.getSetCookie() to get a string[] (or
empty array if undefined), then assert that some element matches /epds_csrf=/
and throw the same error message if none match, preserving
getCapturedResponse(this) and the step name.
- Around line 139-155: The step When('the login page is loaded', async function
(this: EpdsWorld) currently falls back from previewUrl
(`${testEnv.authUrl}/preview/login`) to `/health` on a 404, which weakens the
intended "login page is loaded" assertion; change the behavior in this step so
that a 404 on previewUrl fails the test (throw or set a failing assertion) or
instead probe the real login route (e.g., `${testEnv.authUrl}/login` or the
actual auth-service login endpoint) and use that response for
setCapturedResponse(this, ...), ensuring the test validates the actual login
page CSP rather than a generic `/health` response.
In `@packages/shared/src/preview-ui.ts`:
- Around line 491-494: The previewClientIdScriptHtml helper currently injects
cspNonce raw into nonceAttr; make this defensive by validating or escaping
cspNonce before interpolation in previewClientIdScriptHtml (e.g., in the
function sanitize the input used to build nonceAttr: either run a strict
whitelist check for /^[A-Za-z0-9_-]+$/ and only use it when it passes, or pass
it through an HTML-attribute escape helper like escapeHtml and fall back to
empty string on failure), so update previewClientIdScriptHtml to compute a safe
nonce value and then build nonceAttr from that safe value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdf4ebf3-e701-4ff9-9c0e-bbb1ea21c749
📒 Files selected for processing (11)
.changeset/csp-nonce-and-metrics-auth.mde2e/step-definitions/security.steps.tsfeatures/security.featurepackages/auth-service/src/__tests__/security-headers.test.tspackages/auth-service/src/index.tspackages/auth-service/src/middleware/security-headers.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/shared/src/index.tspackages/shared/src/preview-ui.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
aspiers
force-pushed
the
hyper-security-scenarios
branch
from
780190d to
2755af3
Compare
Coverage Report for CI Build 25175190833Coverage increased (+0.3%) to 49.698%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR hardens the auth-service by moving inline script execution under a per-response CSP nonce, tightening access to /metrics, and enabling end-to-end security scenarios to validate these guarantees.
Changes:
- Introduces per-response CSP nonces (dropping
script-src 'unsafe-inline') and threads the nonce through auth-service HTML templates and preview pages. - Locks down the auth-service
/metricsendpoint to return 401 whenPDS_ADMIN_PASSWORDis unset, and uses a timing-safe comparison when it is set. - Enables security Cucumber scenarios and adds unit tests around the extracted security-headers middleware (including nonce behavior).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/preview-ui.ts | Adds optional CSP nonce stamping for the preview index inline script; refactors script into body + helper. |
| packages/shared/src/index.ts | Re-exports previewClientIdScriptHtml for shared consumers. |
| packages/auth-service/src/routes/preview.ts | Passes res.locals.cspNonce into preview renderers so inline scripts can be nonced. |
| packages/auth-service/src/routes/login-page.ts | Requires and stamps CSP nonce onto the login page inline <script>. |
| packages/auth-service/src/routes/choose-handle.ts | Threads optional nonce into choose-handle page and conditionally stamps <script nonce=...>. |
| packages/auth-service/src/lib/security-headers.ts | Extracts/updates security headers middleware to generate per-response nonce and build CSP accordingly. |
| packages/auth-service/src/index.ts | Deny-by-default /metrics auth; uses timingSafeEqual for header comparison. |
| packages/auth-service/src/tests/security-headers.test.ts | Adds unit tests for nonce generation/contract and CSP script-src behavior. |
| features/security.feature | Enables security scenarios (CSRF, headers table, CSP nonce, metrics 401). |
| e2e/step-definitions/security.steps.ts | Implements HTTP-based Cucumber step definitions for the enabled security scenarios. |
| .changeset/csp-nonce-and-metrics-auth.md | Documents operator-facing behavior changes for CSP nonce and /metrics auth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/lib/security-headers.ts (1)
10-12:⚠️ Potential issue | 🟡 MinorUpdate the purity note to include
res.localsmutation.The middleware now intentionally writes
res.locals.cspNonce, so the file-level comment saying there is “no res/req mutation outside the setHeader contract” is stale.📝 Proposed doc fix
- * The middleware factory + helpers are pure: no module-level state, - * no Express types, no res/req mutation outside the well-defined - * setHeader contract. That keeps them straightforward to unit-test + * The middleware factory + helpers have no module-level state and avoid + * Express types. The middleware mutates only the well-defined response + * surface: headers plus `res.locals.cspNonce`. That keeps them straightforward to unit-testAlso applies to: 167-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/lib/security-headers.ts` around lines 10 - 12, Update the purity note to reflect that the middleware intentionally mutates res.locals by writing res.locals.cspNonce: replace the phrase "no res/req mutation outside the setHeader contract" with a short note that the middleware and its helpers are pure except they intentionally set res.locals.cspNonce for CSP nonce propagation; apply the same wording to the duplicate doc block that currently repeats the stale claim so both the top-of-file purity comment and the later helper comment acknowledge the res.locals mutation.
🧹 Nitpick comments (1)
features/security.feature (1)
48-52: Also assert that inline scripts carry the advertised nonce.This scenario verifies the CSP header has a nonce, but it would still pass if the login page’s inline
<script>missed the matchingnonceattribute and got blocked by the browser. Add a step that extracts the nonce fromscript-srcand checks the page’s inline scripts use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/security.feature` around lines 48 - 52, Add a new assertion step to the "Content-Security-Policy restricts inline scripts" Scenario that parses the Content-Security-Policy header (from the existing "Content-Security-Policy header is present" step), extracts the nonce token from the script-src directive, and then inspects the loaded login page DOM (after "When the login page is loaded") to verify that each inline <script> element includes a matching nonce attribute; reference the existing scenario text ("script-src directive carries a per-response nonce" and "When the login page is loaded") and implement a step that fails if any inline script lacks a nonce or its nonce value does not equal the one parsed from the header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/security.steps.ts`:
- Around line 139-198: The test currently only checks that a nonce-shaped token
exists but not that it changes per response; update the Then step handler for
'the script-src directive carries a per-response nonce' to fetch the same
login/health endpoint twice (reuse the previewUrl/health probe logic from the
When handler), extract the script-src from each response using
getScriptSrcDirective, parse the nonce value from each script-src, and assert
the two nonces are different to prove freshness; keep the existing regex check
but add the second fetch + comparison and throw a descriptive error if nonces
match.
- Around line 56-64: Replace the unreliable headers.get('set-cookie') call in
the Then step that asserts a CSRF cookie (the step defined as Then('the response
sets a CSRF cookie' in security.steps.ts) which currently reads
getCapturedResponse(this) and uses headers.get) with the Fetch API's
Headers.getSetCookie() method; call getCapturedResponse(this) to obtain headers,
use headers.getSetCookie() (or fall back to '' if undefined) when building the
setCookie string, and then run the existing regex check (/epds_csrf=/) and error
path unchanged so multiple cookies and Node >=20 runtimes are handled correctly.
In `@packages/auth-service/src/lib/metrics-auth.ts`:
- Around line 54-57: The code calls timingSafeEqual(authHeader, expected) with
raw header text which can throw if lengths differ in bytes for non-ASCII
strings; update the check in the auth flow so you first validate and normalize
the header before calling timingSafeEqual: ensure authHeader is a string, create
Buffers for both the received header and expected value (e.g.,
Buffer.from(authHeader, 'utf8') and Buffer.from(expected, 'utf8'), check that
their byte lengths match and if not return UNAUTHORIZED, then call
timingSafeEqual on those Buffers (still returning UNAUTHORIZED on any mismatch)
— locate the logic around authHeader, expected, timingSafeEqual and UNAUTHORIZED
to implement this guard.
---
Outside diff comments:
In `@packages/auth-service/src/lib/security-headers.ts`:
- Around line 10-12: Update the purity note to reflect that the middleware
intentionally mutates res.locals by writing res.locals.cspNonce: replace the
phrase "no res/req mutation outside the setHeader contract" with a short note
that the middleware and its helpers are pure except they intentionally set
res.locals.cspNonce for CSP nonce propagation; apply the same wording to the
duplicate doc block that currently repeats the stale claim so both the
top-of-file purity comment and the later helper comment acknowledge the
res.locals mutation.
---
Nitpick comments:
In `@features/security.feature`:
- Around line 48-52: Add a new assertion step to the "Content-Security-Policy
restricts inline scripts" Scenario that parses the Content-Security-Policy
header (from the existing "Content-Security-Policy header is present" step),
extracts the nonce token from the script-src directive, and then inspects the
loaded login page DOM (after "When the login page is loaded") to verify that
each inline <script> element includes a matching nonce attribute; reference the
existing scenario text ("script-src directive carries a per-response nonce" and
"When the login page is loaded") and implement a step that fails if any inline
script lacks a nonce or its nonce value does not equal the one parsed from the
header.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8771003-b873-4413-8d19-9b3c3fe7d65b
📒 Files selected for processing (13)
.changeset/csp-nonce-and-metrics-auth.mde2e/step-definitions/security.steps.tsfeatures/security.featurepackages/auth-service/src/__tests__/metrics-auth.test.tspackages/auth-service/src/__tests__/security-headers.test.tspackages/auth-service/src/index.tspackages/auth-service/src/lib/metrics-auth.tspackages/auth-service/src/lib/security-headers.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/shared/src/index.tspackages/shared/src/preview-ui.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/csp-nonce-and-metrics-auth.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/index.ts
- packages/auth-service/src/routes/login-page.ts
- packages/auth-service/src/tests/security-headers.test.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aspiers
force-pushed
the
hyper-security-scenarios
branch
from
a1d8dc8 to
e47c2ae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/auth-service/src/routes/choose-handle.ts (1)
439-446: Optional: consider migratingrenderChooseHandlePageto a single options object.With six positional parameters (including several
string | null | undefinedvariants), the call sites are getting error-prone — it's easy to swapcustomCssandcspNonceby accident since both are strings.renderLoginPage(opts: {...})andrenderRecoveryForm(opts: {...})in this codebase already use the options-object pattern. Not a blocker for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/choose-handle.ts` around lines 439 - 446, Change renderChooseHandlePage to accept a single options object (e.g. opts: { handleDomain: string; error?: string; csrfToken?: string; showRandomButton?: boolean; customCss?: string | null; cspNonce: string; }) instead of six positional args; update the function signature and internals to read from opts and preserve existing defaults/nullable types, then update all call sites to pass an object (matching how renderLoginPage and renderRecoveryForm do) to avoid parameter order mistakes between customCss and cspNonce. Ensure the exported symbol name remains renderChooseHandlePage and keep TypeScript types consistent.e2e/step-definitions/security.steps.ts (1)
144-160: Minor: consider pinning both requests to the same endpoint.If the first fetch in
When('the login page is loaded')hits/preview/loginbut the second fetch in the nonce-freshness check 404s and falls back to/health(or vice-versa because of a mid-test config flip/cold start), the two nonces would still differ but would come from different code paths — still a green test, but the cross-endpoint comparison is slightly weaker than intended. Capture which endpoint won the first time and reuse it for the second fetch, so the assertion is "same endpoint produces a fresh nonce per request".Also applies to: 203-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/security.steps.ts` around lines 144 - 160, The test sometimes fetches /preview/login then falls back to /health which can make nonce comparisons cross-endpoint; inside the When('the login page is loaded') step capture which URL actually returned (e.g. store the resolved previewUrl or fallbackUrl in a local variable like chosenUrl) and include that same chosenUrl when the nonce-freshness check later re-fetches the page (and mirror this change in the other similar block around the nonce freshness check), so both requests hit the identical endpoint before comparing nonces (update references around setCapturedResponse and the nonce-freshness re-fetch to reuse chosenUrl).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/step-definitions/security.steps.ts`:
- Around line 144-160: The test sometimes fetches /preview/login then falls back
to /health which can make nonce comparisons cross-endpoint; inside the When('the
login page is loaded') step capture which URL actually returned (e.g. store the
resolved previewUrl or fallbackUrl in a local variable like chosenUrl) and
include that same chosenUrl when the nonce-freshness check later re-fetches the
page (and mirror this change in the other similar block around the nonce
freshness check), so both requests hit the identical endpoint before comparing
nonces (update references around setCapturedResponse and the nonce-freshness
re-fetch to reuse chosenUrl).
In `@packages/auth-service/src/routes/choose-handle.ts`:
- Around line 439-446: Change renderChooseHandlePage to accept a single options
object (e.g. opts: { handleDomain: string; error?: string; csrfToken?: string;
showRandomButton?: boolean; customCss?: string | null; cspNonce: string; })
instead of six positional args; update the function signature and internals to
read from opts and preserve existing defaults/nullable types, then update all
call sites to pass an object (matching how renderLoginPage and
renderRecoveryForm do) to avoid parameter order mistakes between customCss and
cspNonce. Ensure the exported symbol name remains renderChooseHandlePage and
keep TypeScript types consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c23f356e-5c3c-460f-8542-1981e8b4cec7
📒 Files selected for processing (15)
.changeset/csp-nonce-and-metrics-auth.mde2e/step-definitions/security.steps.tsfeatures/security.featurepackages/auth-service/src/__tests__/metrics-auth.test.tspackages/auth-service/src/__tests__/security-headers.test.tspackages/auth-service/src/index.tspackages/auth-service/src/lib/metrics-auth.tspackages/auth-service/src/lib/security-headers.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/crypto.tspackages/shared/src/index.tspackages/shared/src/preview-ui.ts
✅ Files skipped from review due to trivial changes (4)
- packages/shared/src/index.ts
- packages/shared/src/tests/crypto.test.ts
- .changeset/csp-nonce-and-metrics-auth.md
- packages/auth-service/src/tests/metrics-auth.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/auth-service/src/tests/security-headers.test.ts
- packages/auth-service/src/lib/metrics-auth.ts
This comment has been minimized.
This comment has been minimized.
Replace the auth service's Content-Security-Policy script-src 'unsafe-inline' with a per-response nonce. The security-headers middleware now generates a fresh base64url nonce on every request, stamps it into script-src, and exposes it via res.locals.cspNonce so templates can emit <script nonce="..."> for inline scripts. All inline scripts ePDS ships (login page, choose-handle page, preview index) are threaded through to read and stamp the nonce. Also tighten /metrics on the auth service: if PDS_ADMIN_PASSWORD is unset, return 401 instead of serving metrics unauthenticated, so a missing env var can't silently open the endpoint. Extract the inline security-headers middleware into its own module with dedicated unit tests (7 tests) covering the nonce contract, baseline headers, and client-origin img-src. Enable 5 previously-pending security.feature scenarios: two CSRF checks (targeting the server-rendered recovery form, which uses ePDS's own CSRF middleware rather than better-auth's), the security-headers table, the CSP check, and the metrics 401. New step definitions live in e2e/step-definitions/security.steps.ts.
- Thread cspNonce through 3 missed renderChooseHandlePage call sites (the /_internal/check-handle catch arm in choose-handle.ts, plus both /preview/choose-handle routes). Without the nonce argument the template emits a bare <script> tag which the strict CSP blocks, silently killing handle-picker interactivity on those paths. - Use timingSafeEqual() for the /metrics Basic-auth header check so the secret comparison doesn't leak timing information. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Emit `WWW-Authenticate: Basic realm="metrics"` on every 401 from /metrics so HTTP Basic tooling is told which scheme to use (per RFC 7235). - Escape the CSP nonce before embedding it in the preview index's inline <script nonce="..."> attribute. Current callers pass a base64url crypto.randomBytes(16) value which is already attribute- safe, but escapeHtml here is cheap defence-in-depth if that ever changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extract the /metrics auth-check branching into a pure
checkMetricsAuth() helper in lib/metrics-auth.ts. The route handler
in index.ts becomes a thin wrapper that maps the helper's result
onto the Express response.
Covers every branch:
- adminPassword unset or empty → 401 even with a valid-looking header
- adminPassword set, header missing/empty → 401
- wrong password, wrong username, wrong scheme → 401
- length-matched-but-different payload → 401 (regression guard for
timingSafeEqual short-circuit behaviour)
- matching Basic admin:<password> → ok
Every 401 path carries WWW-Authenticate: Basic realm="metrics".
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The existing docstring claimed pds-core "doesn't set a CSP on preview pages", which was misleading — pds-core's /preview/consent route does set a CSP, it just uses 'unsafe-inline' rather than a nonce. Reword so the no-nonce branch is explicitly described as for CSPs that allow inline scripts without a nonce. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- shared.timingSafeEqual: compare UTF-8 byte lengths, not JS code units. Previously a non-ASCII input with the same .length as the expected value would reach crypto.timingSafeEqual with different byte lengths and throw RangeError — turning a 401 into a 500. Guards every caller (metrics-auth, recovery, callback signatures). - metrics-auth: regression test for non-ASCII Authorization header with matching code-unit length. - security.steps.ts: Headers.getSetCookie() for reliable multi-cookie access; nonce step now fetches twice and asserts nonces differ, so a hardcoded constant would no longer pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- renderChooseHandlePage: promote cspNonce to a required parameter and always stamp nonce="..." on the inline <script>. All 5 call sites already pass res.locals.cspNonce, so the previous `cspNonce?` + conditional fallback only served to mask a future wiring bug where a caller forgot the nonce — the CSP is now nonce-based, so missing nonce means broken page, not graceful degradation. - preview-ui: reword renderPreviewIndexPage cspNonce docstring. pds-core does serve at least one preview page (/preview/consent) under a CSP with 'unsafe-inline', so "omit when the service doesn't set a CSP" was misleading; frame the rule around the page's CSP rather than the service. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Under Node's fetch/undici, an un-drained response body keeps the socket open and can leave open-handles after the test exits. The nonce-freshness check added a second fetch (and a 404-fallback third) without consuming either body. Drain each response via .text() before moving on, matching the pattern in captureGet(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
aspiers
force-pushed
the
hyper-security-scenarios
branch
from
4551692 to
1da0ec6
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/step-definitions/security.steps.ts`:
- Around line 154-157: When handling the fallback from fetch(previewUrl) to the
health endpoint, drain or cancel the first response's body before reassigning
res so the underlying handle isn't leaked; in the block that checks if
res.status === 404 (where res comes from fetch(previewUrl)), consume or cancel
the body (e.g., await res.text() or res.arrayBuffer() or res.body?.cancel())
before calling fetch(`${testEnv.authUrl}/health`) and reassigning res.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4902cc57-5471-480c-af70-b4811e877658
📒 Files selected for processing (16)
.changeset/csp-nonce-and-metrics-auth.mde2e/step-definitions/security.steps.tsfeatures/security.featurepackages/auth-service/src/__tests__/login-page.test.tspackages/auth-service/src/__tests__/metrics-auth.test.tspackages/auth-service/src/__tests__/security-headers.test.tspackages/auth-service/src/index.tspackages/auth-service/src/lib/metrics-auth.tspackages/auth-service/src/lib/security-headers.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/crypto.tspackages/shared/src/index.tspackages/shared/src/preview-ui.ts
✅ Files skipped from review due to trivial changes (5)
- packages/auth-service/src/tests/login-page.test.ts
- .changeset/csp-nonce-and-metrics-auth.md
- packages/auth-service/src/routes/preview.ts
- packages/auth-service/src/lib/metrics-auth.ts
- packages/auth-service/src/tests/security-headers.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/shared/src/tests/crypto.test.ts
- packages/shared/src/index.ts
- packages/auth-service/src/routes/login-page.ts
- packages/auth-service/src/index.ts
Comment on lines
+154
to
+157
| let res = await fetch(previewUrl, { redirect: 'manual' }) | ||
| if (res.status === 404) { | ||
| res = await fetch(`${testEnv.authUrl}/health`, { redirect: 'manual' }) | ||
| } |
There was a problem hiding this comment.
Drain the first response body before the 404 fallback fetch.
When /preview/login returns 404, the first response is replaced without being consumed. Drain it before the /health fetch to avoid leaked handles in this fallback path.
♻️ Suggested patch
const previewUrl = `${testEnv.authUrl}/preview/login`
let res = await fetch(previewUrl, { redirect: 'manual' })
if (res.status === 404) {
+ await res.text()
res = await fetch(`${testEnv.authUrl}/health`, { redirect: 'manual' })
}
const body = await res.text()
setCapturedResponse(this, { status: res.status, headers: res.headers, body })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/step-definitions/security.steps.ts` around lines 154 - 157, When handling
the fallback from fetch(previewUrl) to the health endpoint, drain or cancel the
first response's body before reassigning res so the underlying handle isn't
leaked; in the block that checks if res.status === 404 (where res comes from
fetch(previewUrl)), consume or cancel the body (e.g., await res.text() or
res.arrayBuffer() or res.body?.cancel()) before calling
fetch(`${testEnv.authUrl}/health`) and reassigning res.
Contributor
|
Found 9 test failures on Blacksmith runners: Failures
|
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
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security Improvements
Tests