[HYPER-109] feat: custom CSS injection for trusted OAuth clients#9
Open
[HYPER-109] feat: custom CSS injection for trusted OAuth clients#9
Conversation
… support Move resolveClientMetadata, resolveClientName, and ClientMetadata type from auth-service into @certified-app/shared so both pds-core and auth-service can use them. Add branding.css to ClientMetadata interface, escapeCss() for safe HTML embedding, and getClientCss() trust-gated helper. Auth-service's client-metadata.ts becomes a re-export shim. Ref: bd-20x.1 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ages Add trustedClients config (from PDS_OAUTH_TRUSTED_CLIENTS env var) to AuthServiceConfig. Login page and consent page now resolve full client metadata, call getClientCss() for trusted clients, and inject a <style> tag before </head>. Auth-service CSP already allows 'unsafe-inline' so no header changes needed. Ref: bd-20x.2, bd-20x.3, bd-20x.4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For GET /oauth/authorize, intercept responses from the stock @atproto/oauth-provider to inject custom CSS from trusted clients. Wraps res.setHeader to append SHA256 hash to CSP style-src directive, and wraps res.end to inject <style> tag before </head>. Uses same Express stack insertion technique as AS metadata override. Ref: bd-20x.5 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add gradient button styling and CSS custom properties to demonstrate the custom CSS injection feature for trusted OAuth clients. Ref: bd-20x.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Document the comma-separated list of OAuth client_id URLs trusted for CSS branding injection. Must be set identically in pds-core and auth-service. Ref: bd-20x.7 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds trusted-client configuration and per-client branding plus OTP configuration: new shared client-metadata and otp-config modules; shared re-exports; auth-service and pds-core resolve client metadata and inject/inline client CSS and OTP UI attributes into login/consent/recovery pages; env examples document PDS_OAUTH_TRUSTED_CLIENTS and OTP vars. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OAuth Client
participant PDS as PDS Core
participant Shared as Shared Module
participant Auth as Auth Service
participant Browser as Browser/User
Client->>PDS: GET /oauth/authorize?client_id=...
PDS->>PDS: check client_id ∈ trustedClients
alt trusted
PDS->>Shared: resolveClientMetadata(client_id)
Shared-->>PDS: ClientMetadata (branding.css)
PDS->>Shared: getClientCss(client_id, metadata, trustedClients)
Shared-->>PDS: Escaped CSS
PDS->>PDS: compute SHA-256(CSS) and augment CSP
PDS->>PDS: inject <style> into HTML head
end
PDS-->>Browser: HTML + CSP headers
Browser->>Auth: open /login or act on consent/recovery
Auth->>Shared: resolveClientMetadata(client_id)
Shared-->>Auth: ClientMetadata
Auth->>Shared: getClientCss(client_id, metadata, trustedClients)
Shared-->>Auth: Escaped CSS
Auth-->>Browser: Branded HTML (login/consent/recovery) with OTP attrs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aspiers
force-pushed
the
css-injection-trusted-clients
branch
from
1f498b1 to
97877a3
Compare
![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
🧹 Nitpick comments (2)
packages/shared/src/client-metadata.ts (1)
118-127: Consider case-insensitive comparison for trusted client IDs.The
trustedClients.includes(clientId)check is case-sensitive. While OAuth client IDs are typically URLs and should be compared exactly, ensure that the configuration inPDS_OAUTH_TRUSTED_CLIENTSmatches the exact client_id values used in requests (including trailing slashes, protocol, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/client-metadata.ts` around lines 118 - 127, The trusted client check in getClientCss is currently case-sensitive; update the logic to perform a case-insensitive comparison (e.g., compare clientId.toLowerCase() against trustedClients entries normalized with .map(s => s.toLowerCase())) and consider normalizing trivial URL differences (trim and optionally remove a trailing slash) before comparing so trusted client matching is robust; modify getClientCss to use the normalized comparison when evaluating trustedClients.includes(...).packages/pds-core/src/index.ts (1)
348-401: Async middleware without explicit error handling fornext().The middleware is
asyncbut Express doesn't natively handle promise rejections in middleware. While there's a try-catch around the main logic that callsnext()on error, ifresolveClientMetadatathrows afternext()is somehow not reached, Express won't catch it. The current implementation looks correct, but consider wrapping the entire function body or using an async middleware wrapper for robustness.This is a minor concern given the existing try-catch structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pds-core/src/index.ts` around lines 348 - 401, The cssInjectionMiddleware is declared async but currently swallows errors by calling next() in the catch; change error handling to forward exceptions to Express by calling next(err) inside the catch so unhandled rejections reach the error handler, or wrap the middleware with an async wrapper (e.g., express-async-handler) and remove the try/catch; specifically update cssInjectionMiddleware (and where resolveClientMetadata is awaited) to call next(err) instead of next() on failure, or export/replace the async function with a wrapper that catches promise rejections and forwards them to next.
🤖 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/routes/login-page.ts`:
- Around line 228-229: The imports otpHtmlAttrs and otpDescriptionText
referenced by login-page.ts are missing because packages/shared lacks
otp-config.ts and index.ts exports ./otp-config.js point to a non-existent
module; fix by either (A) creating packages/shared/src/otp-config.ts that
exports otpHtmlAttrs (returns the OTP input HTML attributes such as
maxlength="8" and pattern="[0-9]{8}") and otpDescriptionText (returns the
8-digit OTP description string), and update the package/shared index.ts to
export these symbols, or (B) change the import in login-page.ts to the correct
existing module that provides these utilities; ensure the exported function
names otpHtmlAttrs and otpDescriptionText are used exactly as in login-page.ts.
In `@packages/pds-core/src/index.ts`:
- Around line 383-394: The overridden response end uses untyped any and a loose
rest param; update types to remove ESLint errors and match Node signatures by
typing origEnd as typeof res.end, and change the override to res.end = (chunk?:
string | Buffer | null, encoding?: BufferEncoding | ((err?: Error) => void) |
undefined, callback?: (() => void) | undefined) => { ... } (handle encoding
being a callback), check Buffer.isBuffer and typeof chunk === 'string' as
before, and call origEnd(chunk as any, encoding as any, callback as any) to
satisfy typing while preserving behavior; reference origEnd, res.end, and
styleTag when editing.
In `@packages/shared/src/index.ts`:
- Around line 27-33: The re-export block in index.ts references a non-existent
module './otp-config.js' causing build failures; either add a new module that
exports otpConfig, generateOtp, otpHtmlAttrs, otpDescriptionText and the types
OtpFormat and OtpConfig (implementing their expected signatures), or remove
these re-exports from the file if OTP functionality isn’t part of this
change—update the import paths in any callers to point to the new module name if
you create it, and ensure the exported symbol names (otpConfig, generateOtp,
otpHtmlAttrs, otpDescriptionText, OtpFormat, OtpConfig) exactly match what's
declared.
---
Nitpick comments:
In `@packages/pds-core/src/index.ts`:
- Around line 348-401: The cssInjectionMiddleware is declared async but
currently swallows errors by calling next() in the catch; change error handling
to forward exceptions to Express by calling next(err) inside the catch so
unhandled rejections reach the error handler, or wrap the middleware with an
async wrapper (e.g., express-async-handler) and remove the try/catch;
specifically update cssInjectionMiddleware (and where resolveClientMetadata is
awaited) to call next(err) instead of next() on failure, or export/replace the
async function with a wrapper that catches promise rejections and forwards them
to next.
In `@packages/shared/src/client-metadata.ts`:
- Around line 118-127: The trusted client check in getClientCss is currently
case-sensitive; update the logic to perform a case-insensitive comparison (e.g.,
compare clientId.toLowerCase() against trustedClients entries normalized with
.map(s => s.toLowerCase())) and consider normalizing trivial URL differences
(trim and optionally remove a trailing slash) before comparing so trusted client
matching is robust; modify getClientCss to use the normalized comparison when
evaluating trustedClients.includes(...).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.beads/issues.jsonl.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/context.tspackages/auth-service/src/index.tspackages/auth-service/src/lib/client-metadata.tspackages/auth-service/src/routes/consent.tspackages/auth-service/src/routes/login-page.tspackages/demo/src/app/client-metadata.json/route.tspackages/pds-core/.env.examplepackages/pds-core/src/index.tspackages/shared/src/client-metadata.tspackages/shared/src/index.ts
Comment on lines
+383
to
+394
| const origEnd = res.end.bind(res) | ||
| res.end = (chunk: any, ...args: any[]) => { | ||
| if (typeof chunk === 'string' && chunk.includes('</head>')) { | ||
| chunk = chunk.replace('</head>', `${styleTag}</head>`) | ||
| } else if (Buffer.isBuffer(chunk)) { | ||
| const str = chunk.toString('utf-8') | ||
| if (str.includes('</head>')) { | ||
| chunk = str.replace('</head>', `${styleTag}</head>`) | ||
| } | ||
| } | ||
| return origEnd(chunk, ...args) | ||
| } |
There was a problem hiding this comment.
Add type annotations to fix ESLint errors and improve type safety.
The any types on line 384 trigger ESLint errors. While this is internal Express plumbing, adding minimal type annotations satisfies the linter and documents intent.
Also, the res.end signature in Node.js can accept (chunk, encoding, callback). The current spread may work, but being explicit is safer.
Proposed fix
// Wrap res.end to inject the <style> tag before </head>
const origEnd = res.end.bind(res)
- res.end = (chunk: any, ...args: any[]) => {
+ res.end = (chunk?: Buffer | string, encoding?: BufferEncoding | (() => void), cb?: () => void) => {
if (typeof chunk === 'string' && chunk.includes('</head>')) {
chunk = chunk.replace('</head>', `${styleTag}</head>`)
} else if (Buffer.isBuffer(chunk)) {
const str = chunk.toString('utf-8')
if (str.includes('</head>')) {
chunk = str.replace('</head>', `${styleTag}</head>`)
}
}
- return origEnd(chunk, ...args)
+ return origEnd(chunk, encoding as BufferEncoding, cb)
}🧰 Tools
🪛 ESLint
[error] 384-384: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 384-384: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: lint
[failure] 384-384:
Unexpected any. Specify a different type
[failure] 384-384:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pds-core/src/index.ts` around lines 383 - 394, The overridden
response end uses untyped any and a loose rest param; update types to remove
ESLint errors and match Node signatures by typing origEnd as typeof res.end, and
change the override to res.end = (chunk?: string | Buffer | null, encoding?:
BufferEncoding | ((err?: Error) => void) | undefined, callback?: (() => void) |
undefined) => { ... } (handle encoding being a callback), check Buffer.isBuffer
and typeof chunk === 'string' as before, and call origEnd(chunk as any, encoding
as any, callback as any) to satisfy typing while preserving behavior; reference
origEnd, res.end, and styleTag when editing.
Comment on lines
+27
to
+33
| export { | ||
| otpConfig, | ||
| generateOtp, | ||
| otpHtmlAttrs, | ||
| otpDescriptionText, | ||
| } from './otp-config.js' | ||
| export type { OtpFormat, OtpConfig } from './otp-config.js' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if otp-config.ts/js exists in the shared package
echo "=== Searching for otp-config files ==="
fd -t f 'otp-config' packages/shared/
echo -e "\n=== Listing all .ts files in packages/shared/src ==="
ls -la packages/shared/src/*.ts 2>/dev/null || echo "No .ts files found"
echo -e "\n=== Checking if client-metadata.ts exists ==="
fd -t f 'client-metadata' packages/shared/
echo -e "\n=== Checking the index.ts file at lines 25-35 ==="
sed -n '25,35p' packages/shared/src/index.tsRepository: hypercerts-org/ePDS
Length of output: 1140
Build failure: Missing ./otp-config.js module.
The exports on lines 27-33 reference ./otp-config.js, which does not exist in packages/shared/src/. No otp-config.ts or compiled otp-config.js file is present. This will cause TypeScript compilation to fail for the shared package and any dependents. Either the file needs to be created with the exported functions (otpConfig, generateOtp, otpHtmlAttrs, otpDescriptionText) and types (OtpFormat, OtpConfig), or the exports must be removed if this module is not part of this PR.
🧰 Tools
🪛 GitHub Actions: CI
[error] 32-32: Cannot find module './otp-config.js' or its corresponding type declarations. (TS2307) During 'pnpm --recursive build' step.
🪛 GitHub Check: typecheck
[failure] 33-33:
Cannot find module './otp-config.js' or its corresponding type declarations.
[failure] 32-32:
Cannot find module './otp-config.js' or its corresponding type declarations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/index.ts` around lines 27 - 33, The re-export block in
index.ts references a non-existent module './otp-config.js' causing build
failures; either add a new module that exports otpConfig, generateOtp,
otpHtmlAttrs, otpDescriptionText and the types OtpFormat and OtpConfig
(implementing their expected signatures), or remove these re-exports from the
file if OTP functionality isn’t part of this change—update the import paths in
any callers to point to the new module name if you create it, and ensure the
exported symbol names (otpConfig, generateOtp, otpHtmlAttrs, otpDescriptionText,
OtpFormat, OtpConfig) exactly match what's declared.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.beads/issues.jsonl:
- Around line 19-21: The issue is that the atproto-9fa parent and its child
tasks (atproto-9fa.1, .2, etc.) have inconsistent statuses and inverted
dependency directions; fix by updating the JSON entries so the parent
atproto-9fa remains open until all child tasks are closed (set "status":"open"
for the parent if children are still open) and correct the dependency edges:
make the shared extraction task atproto-9fa.1 an upstream dependency (other
tasks should have depends_on entries pointing to "atproto-9fa.1" and/or the
parent "atproto-9fa") instead of using inverted "blocks" relationships, and
ensure each dependency object uses the proper "depends_on_id" value and "type"
(use "depends_on" for normal dependency rather than "blocks" where appropriate);
update the "dependencies" arrays for atproto-9fa, atproto-9fa.1, and
atproto-9fa.2 to reflect the correct upstream/downstream ordering and set
statuses consistently.
- Line 17: The default OTP length should remain 8 (not 6): update the shared OTP
config module to read OTP_LENGTH (default 8) and OTP_FORMAT (default "numeric"),
ensure the exported config and generateOTP logic use that default, update any
wiring in better-auth.ts (otpLength + generateOTP callback) and .env.example to
reflect OTP_LENGTH=8, and adjust any tests/HTML defaults (login-page.ts,
recovery.ts, account-login.ts) that assumed 6 so they now use the configured
default of 8.
Replace unused CSS custom properties with a full dark theme override while keeping the gradient button styling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file was created on a previous branch but never committed, causing CI typecheck failures since shared/src/index.ts re-exports it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add eslint-disable for res.end wrapper's any types (complex Node overloads). Run prettier on consent.ts and pds-core/index.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
.1 (shared extraction) now blocks .2 and .5 (not the reverse). .2 (auth config) now blocks .3 and .4 (not the reverse). Also closes .1 and .2 which were left open. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kzoeps
force-pushed
the
css-injection-trusted-clients
branch
from
c22d690 to
0356474
Compare
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 (2)
packages/auth-service/src/routes/recovery.ts (1)
287-302:⚠️ Potential issue | 🟠 MajorRecovery still ignores the shared OTP format helpers.
This template still hard-codes “8-digit code” and
[0-9]{8}, whilelogin-page.tsnow renders fromotpHtmlAttrs()/otpDescriptionText(). Any non-defaultOTP_LENGTHorOTP_FORMATwill make/auth/recoverreject valid codes client-side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 287 - 302, The recovery template hard-codes "8-digit code" and the pattern/maxlength on the code input, causing client-side rejection when OTP_LENGTH or OTP_FORMAT are non-default; update the HTML to use the shared helpers instead of literals: call otpDescriptionText(...) to render the description sentence (replace the hard-coded "8-digit code" and maskedEmail line) and use otpHtmlAttrs(...) to produce the input attributes for the code field (replace maxlength, pattern, inputmode, autocomplete, placeholder) so /auth/recover/verify accepts the same OTP formats as login-page.ts; ensure the same imports/usage as in login-page.ts and pass the same opts (or relevant values) to these helper functions and remove the hard-coded values.packages/auth-service/src/routes/login-page.ts (1)
128-139:⚠️ Potential issue | 🟠 MajorDon't make
/oauth/authorizedepend on metadata availability.
resolveClientMetadata(clientId)is awaited before any fallback, so a transient metadata fetch error aborts the page andresolveClientName()never runs. Since branding is optional, this should fall back to a generic client name/unstyled UI instead of failing the auth flow.🤖 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 128 - 139, The code currently awaits resolveClientMetadata(clientId) and will abort the flow on transient metadata errors; instead wrap the resolveClientMetadata(clientId) call in a try/catch and on error set clientMeta = {} (or fallback object) so execution continues; then compute clientName the same way (clientMeta.client_name ?? (clientId ? await resolveClientName(clientId) : 'an application')) so resolveClientName still runs as a fallback, and call getClientCss(clientId, clientMeta, ctx.config.trustedClients) with the fallback clientMeta (or null if you prefer no CSS) so CSS and branding remain optional and metadata failures no longer block /oauth/authorize.
♻️ Duplicate comments (1)
packages/auth-service/src/routes/consent.ts (1)
47-53:⚠️ Potential issue | 🟠 MajorHandle client-metadata lookup failures here too.
Same failure mode as the authorize page: both branches await
resolveClientMetadata(clientId)before fallback logic, so a transient metadata error turns the consent screen into a hard failure instead of generic branding.Also applies to: 81-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/consent.ts` around lines 47 - 53, The code calls resolveClientMetadata(clientId) directly and lets its errors bubble, which makes the consent page fail on transient metadata lookup errors; update the logic around resolveClientMetadata(clientId) (used when computing clientMeta, clientName, and customCss and similarly in the later branch at the other occurrence) to catch and handle errors by treating failures as an empty clientMeta (e.g., clientMeta = {}) and fallback to the existing resolveClientName(clientId) or 'the application' and null CSS; also log or trace the metadata error for observability but do not let it block rendering the consent page.
🧹 Nitpick comments (1)
.beads/backup/backup_state.json (1)
2-11: Keep generated backup state out of the PR.This snapshot looks tool-generated and mutable (
last_dolt_commit,timestamp, counts). If it is not required at runtime, tracking it will add noisy diffs and conflict churn on unrelated changes. Prefer ignoring.beads/backup/backup_state.jsonor generating it outside the repo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.beads/backup/backup_state.json around lines 2 - 11, The backup_state.json snapshot is generated/mutable and should not be tracked; remove it from source control and stop future commits by adding an ignore rule. Remove the tracked file from the repository index (e.g., untrack it with git rm --cached), add an appropriate .gitignore entry for backup_state.json (or the .beads/backup folder) so it isn’t committed again, commit the removal, and ensure whatever process currently produces last_dolt_commit/timestamp/counts regenerates the file outside the repo or writes it to a build/temp directory.
🤖 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/routes/recovery.ts`:
- Around line 319-326: resolveCustomCss() currently always calls
resolveClientMetadata(clientId) even for untrusted client IDs; change it to
trust-gate first and avoid the outbound fetch for untrusted IDs. Specifically,
in resolveCustomCss check whether clientId is in trustedClients (or otherwise
considered trusted) and return null immediately if not trusted, and only call
resolveClientMetadata(clientId) and then getClientCss(clientId, meta,
trustedClients) when the client is trusted.
In `@packages/pds-core/src/index.ts`:
- Around line 385-398: The override of res.end (origEnd/res.end) mutates the
response body by injecting styleTag after the upstream handler may have already
set Content-Length or ETag, causing mismatched headers; modify the res.end
wrapper so that when you detect and rewrite the HTML (when chunk/string/Buffer
includes '</head>') you also clear stale entity headers by removing
'Content-Length' and 'ETag' (e.g., call res.removeHeader('content-length') and
res.removeHeader('etag')) before calling origEnd; ensure this logic is applied
in the same branch where chunk is replaced so origEnd is invoked with the
modified chunk and without stale headers.
In `@packages/shared/src/otp-config.ts`:
- Around line 27-29: The default OTP length currently falls back to 6 when
OTP_LENGTH is unset; change the fallback to 8 so existing auth and recovery UX
remain consistent by updating the rawLength/length logic in otp-config.ts to use
8 as the default (keep the same parseInt and validation: isNaN(length) || length
< 4 || length > 12). Ensure references to OTP_LENGTH still parse/process the env
var but if undefined the computed length variable defaults to 8.
---
Outside diff comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 128-139: The code currently awaits resolveClientMetadata(clientId)
and will abort the flow on transient metadata errors; instead wrap the
resolveClientMetadata(clientId) call in a try/catch and on error set clientMeta
= {} (or fallback object) so execution continues; then compute clientName the
same way (clientMeta.client_name ?? (clientId ? await
resolveClientName(clientId) : 'an application')) so resolveClientName still runs
as a fallback, and call getClientCss(clientId, clientMeta,
ctx.config.trustedClients) with the fallback clientMeta (or null if you prefer
no CSS) so CSS and branding remain optional and metadata failures no longer
block /oauth/authorize.
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 287-302: The recovery template hard-codes "8-digit code" and the
pattern/maxlength on the code input, causing client-side rejection when
OTP_LENGTH or OTP_FORMAT are non-default; update the HTML to use the shared
helpers instead of literals: call otpDescriptionText(...) to render the
description sentence (replace the hard-coded "8-digit code" and maskedEmail
line) and use otpHtmlAttrs(...) to produce the input attributes for the code
field (replace maxlength, pattern, inputmode, autocomplete, placeholder) so
/auth/recover/verify accepts the same OTP formats as login-page.ts; ensure the
same imports/usage as in login-page.ts and pass the same opts (or relevant
values) to these helper functions and remove the hard-coded values.
---
Duplicate comments:
In `@packages/auth-service/src/routes/consent.ts`:
- Around line 47-53: The code calls resolveClientMetadata(clientId) directly and
lets its errors bubble, which makes the consent page fail on transient metadata
lookup errors; update the logic around resolveClientMetadata(clientId) (used
when computing clientMeta, clientName, and customCss and similarly in the later
branch at the other occurrence) to catch and handle errors by treating failures
as an empty clientMeta (e.g., clientMeta = {}) and fallback to the existing
resolveClientName(clientId) or 'the application' and null CSS; also log or trace
the metadata error for observability but do not let it block rendering the
consent page.
---
Nitpick comments:
In @.beads/backup/backup_state.json:
- Around line 2-11: The backup_state.json snapshot is generated/mutable and
should not be tracked; remove it from source control and stop future commits by
adding an ignore rule. Remove the tracked file from the repository index (e.g.,
untrack it with git rm --cached), add an appropriate .gitignore entry for
backup_state.json (or the .beads/backup folder) so it isn’t committed again,
commit the removal, and ensure whatever process currently produces
last_dolt_commit/timestamp/counts regenerates the file outside the repo or
writes it to a build/temp directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d33f23-9074-4e83-983d-660ae6397ae2
📒 Files selected for processing (15)
.beads/.gitignore.beads/backup/backup_state.json.beads/backup/comments.jsonl.beads/backup/config.jsonl.beads/backup/dependencies.jsonl.beads/backup/events.jsonl.beads/backup/issues.jsonl.beads/backup/labels.jsonl.beads/issues.jsonlpackages/auth-service/src/routes/consent.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/demo/src/app/client-metadata.json/route.tspackages/pds-core/src/index.tspackages/shared/src/otp-config.ts
✅ Files skipped from review due to trivial changes (1)
- .beads/backup/config.jsonl
Comment on lines
+385
to
+398
| // Wrap res.end to inject the <style> tag before </head> | ||
| const origEnd = res.end.bind(res) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- wrapping Node http.ServerResponse.end() which has complex overloads | ||
| res.end = (chunk: any, ...args: any[]) => { | ||
| if (typeof chunk === 'string' && chunk.includes('</head>')) { | ||
| chunk = chunk.replace('</head>', `${styleTag}</head>`) | ||
| } else if (Buffer.isBuffer(chunk)) { | ||
| const str = chunk.toString('utf-8') | ||
| if (str.includes('</head>')) { | ||
| chunk = str.replace('</head>', `${styleTag}</head>`) | ||
| } | ||
| } | ||
| return origEnd(chunk, ...args) | ||
| } |
There was a problem hiding this comment.
Clear stale entity headers when you rewrite the HTML.
This mutates the body after the upstream handler has likely already set Content-Length/ETag. For trusted clients, that can produce a mismatched response and broken authorize pages.
Suggested fix
// Wrap res.end to inject the <style> tag before </head>
const origEnd = res.end.bind(res)
// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- wrapping Node http.ServerResponse.end() which has complex overloads
res.end = (chunk: any, ...args: any[]) => {
+ let mutated = false
if (typeof chunk === 'string' && chunk.includes('</head>')) {
chunk = chunk.replace('</head>', `${styleTag}</head>`)
+ mutated = true
} else if (Buffer.isBuffer(chunk)) {
const str = chunk.toString('utf-8')
if (str.includes('</head>')) {
chunk = str.replace('</head>', `${styleTag}</head>`)
+ mutated = true
}
}
+ if (mutated) {
+ res.removeHeader('Content-Length')
+ res.removeHeader('ETag')
+ }
return origEnd(chunk, ...args)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pds-core/src/index.ts` around lines 385 - 398, The override of
res.end (origEnd/res.end) mutates the response body by injecting styleTag after
the upstream handler may have already set Content-Length or ETag, causing
mismatched headers; modify the res.end wrapper so that when you detect and
rewrite the HTML (when chunk/string/Buffer includes '</head>') you also clear
stale entity headers by removing 'Content-Length' and 'ETag' (e.g., call
res.removeHeader('content-length') and res.removeHeader('etag')) before calling
origEnd; ensure this logic is applied in the same branch where chunk is replaced
so origEnd is invoked with the modified chunk and without stale headers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/auth-service/src/routes/recovery.ts (1)
325-333:⚠️ Potential issue | 🟠 MajorTrust-gate before resolving client metadata.
Line 331 still resolves metadata before the allowlist check in
getClientCss(), so any public recovery request with an arbitraryclient_idcan trigger an outbound lookup. Short-circuit ontrustedClientsfirst and only fetch metadata for trusted IDs.Suggested fix
async function resolveCustomCss( clientId: string | undefined, trustedClients: string[], ): Promise<string | null> { - if (!clientId) return null + if (!clientId || !trustedClients.includes(clientId)) return null try { const meta: ClientMetadata = await resolveClientMetadata(clientId) return getClientCss(clientId, meta, trustedClients) } catch { return null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 325 - 333, resolveCustomCss currently fetches client metadata before checking the trusted allowlist, allowing untrusted public requests to trigger outbound lookups; modify resolveCustomCss to first short-circuit by returning null if clientId is not in trustedClients (e.g., if (!clientId || !trustedClients.includes(clientId)) return null) and only call resolveClientMetadata and getClientCss for trusted IDs so metadata is resolved only for allowlisted clients.
🤖 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/routes/login-page.ts`:
- Around line 141-149: The logger.debug call is exposing the full trusted-client
allowlist (ctx.config.trustedClients); update the log to omit the full array and
instead log only isTrusted and a trustedCount (e.g.,
ctx.config.trustedClients.length) alongside clientId and other safe fields; keep
the isTrusted computation (ctx.config.trustedClients.includes(clientId ?? ''))
and retain customCss/customCss length logging but remove trustedClients from the
logged object to avoid writing the allowlist to logs (modify the logger.debug
invocation near logger.debug and clientId).
- Around line 368-369: The recovery-link anchor interpolates user-controlled
opts.requestUri and opts.clientId using encodeURIComponent but does not apply
HTML escaping; update the template to pass those encoded values through
escapeHtml() from `@certified-app/shared` before embedding them in the href
attribute. Locate the fragment with id "recovery-link" (in the login-page
rendering/template) and replace encodeURIComponent(opts.requestUri) and
encodeURIComponent(opts.clientId) with
escapeHtml(encodeURIComponent(opts.requestUri)) and
escapeHtml(encodeURIComponent(opts.clientId)) respectively so the params are
both URL-encoded and HTML-escaped.
---
Duplicate comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 325-333: resolveCustomCss currently fetches client metadata before
checking the trusted allowlist, allowing untrusted public requests to trigger
outbound lookups; modify resolveCustomCss to first short-circuit by returning
null if clientId is not in trustedClients (e.g., if (!clientId ||
!trustedClients.includes(clientId)) return null) and only call
resolveClientMetadata and getClientCss for trusted IDs so metadata is resolved
only for allowlisted clients.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 431ccba0-8f89-4516-a8f2-c41944896bab
📒 Files selected for processing (11)
.beads/.gitignore.beads/backup/backup_state.json.beads/backup/comments.jsonl.beads/backup/config.jsonl.beads/backup/dependencies.jsonl.beads/backup/events.jsonl.beads/backup/issues.jsonl.beads/backup/labels.jsonl.beads/issues.jsonlpackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .beads/.gitignore
- .beads/backup/backup_state.json
- .beads/backup/config.jsonl
Instead of solely relying on the query params for the client-id we add a fallback to also use the request-uri to fetch the actual row from the oauth-flow. From which we can get the client id and then fetch the custom css.
Kzoeps
force-pushed
the
css-injection-trusted-clients
branch
from
0356474 to
976e670
Compare
Contributor
|
@aspiers updated the logic to fetch the custom css a bit (refer to this commit) we now pass the actual |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/shared/src/otp-config.ts (1)
25-42:⚠️ Potential issue | 🟠 MajorKeep the auth-service OTP contract fixed at 8 numeric digits.
resolveOtpConfig()now permits 4–12 characters andalphanumeric, andgenerateOtp()applies that config globally. Since auth-service consumes this shared module for OTP behavior/UI, a deploy-time env change can silently move login and recovery flows away from the required 8-digit numeric contract. If other packages need configurable OTPs, keep that in a separate helper instead of the path used by auth-service.Suggested change
/** Read and validate OTP config from environment variables. */ function resolveOtpConfig(): OtpConfig { - const rawLength = process.env.OTP_LENGTH - const length = rawLength ? parseInt(rawLength, 10) : 8 - if (isNaN(length) || length < 4 || length > 12) { - throw new Error( - `OTP_LENGTH must be an integer between 4 and 12, got: ${rawLength}`, - ) - } - - const rawFormat = process.env.OTP_FORMAT ?? 'numeric' - if (rawFormat !== 'numeric' && rawFormat !== 'alphanumeric') { - throw new Error( - `OTP_FORMAT must be 'numeric' or 'alphanumeric', got: ${rawFormat}`, - ) - } - - return { length, format: rawFormat } + return { length: 8, format: 'numeric' } }As per coding guidelines, "OTP codes must be 8-digit, single-use, managed by better-auth".
Also applies to: 46-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/otp-config.ts` around lines 25 - 42, The shared resolveOtpConfig currently allows variable lengths and formats which can change auth-service behavior at deploy; fix it so resolveOtpConfig() always returns { length: 8, format: 'numeric' } (remove parsing/validation of OTP_LENGTH and OTP_FORMAT) so auth-service remains fixed to 8-digit numeric OTPs, and if configurable OTPs are needed create a separate helper (e.g., resolveConfigForFlexibleOtp or generateConfigurableOtp) for other packages instead of altering resolveOtpConfig; ensure generateOtp() continues to consume resolveOtpConfig() unchanged so it emits 8-digit numeric codes for auth-service.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
23-42: Reorder this import block to match the repo convention.
node:built-ins should come first, then external/internal package imports, and local../...imports last. The new@certified-app/sharedimport currently sits after local imports.Suggested change
-import { Router, type Request, type Response } from 'express' import { randomBytes } from 'node:crypto' +import { Router, type Request, type Response } from 'express' +import { + escapeHtml, + createLogger, + otpHtmlAttrs, + otpDescriptionText, +} from '@certified-app/shared' import type { AuthServiceContext } from '../context.js' import { resolveClientMetadata, resolveClientName, getClientCss, type ClientMetadata, } from '../lib/client-metadata.js' -import { - escapeHtml, - createLogger, - otpHtmlAttrs, - otpDescriptionText, -} from '@certified-app/shared' import { socialProviders } from '../better-auth.js' import { resolveLoginHint, fetchParLoginHint,As per coding guidelines, "Order imports as: Node built-ins, external packages, internal workspace packages, local relative imports".
🤖 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 23 - 42, The import block is misordered: bring Node built-ins first (keep "import { randomBytes } from 'node:crypto'"), then external packages (move "import { Router, type Request, type Response } from 'express'" and "import { escapeHtml, createLogger, otpHtmlAttrs, otpDescriptionText } from '@certified-app/shared'" and "import { socialProviders } from '../better-auth.js'" into the external/internal group), then internal workspace packages if any, and finally local relative imports (move "import type { AuthServiceContext } from '../context.js'" and the imports from "../lib/client-metadata.js" and "../lib/resolve-login-hint.js" to the end). Ensure symbols like randomBytes, Router, escapeHtml, socialProviders, resolveClientMetadata and resolveLoginHint remain unchanged while reordering only the import statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/shared/src/otp-config.ts`:
- Around line 25-42: The shared resolveOtpConfig currently allows variable
lengths and formats which can change auth-service behavior at deploy; fix it so
resolveOtpConfig() always returns { length: 8, format: 'numeric' } (remove
parsing/validation of OTP_LENGTH and OTP_FORMAT) so auth-service remains fixed
to 8-digit numeric OTPs, and if configurable OTPs are needed create a separate
helper (e.g., resolveConfigForFlexibleOtp or generateConfigurableOtp) for other
packages instead of altering resolveOtpConfig; ensure generateOtp() continues to
consume resolveOtpConfig() unchanged so it emits 8-digit numeric codes for
auth-service.
---
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 23-42: The import block is misordered: bring Node built-ins first
(keep "import { randomBytes } from 'node:crypto'"), then external packages (move
"import { Router, type Request, type Response } from 'express'" and "import {
escapeHtml, createLogger, otpHtmlAttrs, otpDescriptionText } from
'@certified-app/shared'" and "import { socialProviders } from
'../better-auth.js'" into the external/internal group), then internal workspace
packages if any, and finally local relative imports (move "import type {
AuthServiceContext } from '../context.js'" and the imports from
"../lib/client-metadata.js" and "../lib/resolve-login-hint.js" to the end).
Ensure symbols like randomBytes, Router, escapeHtml, socialProviders,
resolveClientMetadata and resolveLoginHint remain unchanged while reordering
only the import statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d1ad56f-7082-4b60-aa6a-9decdab5dca0
📒 Files selected for processing (5)
.beads/.gitignore.beads/issues.jsonlpackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/shared/src/otp-config.ts
✅ Files skipped from review due to trivial changes (1)
- .beads/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth-service/src/routes/recovery.ts
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
2 participants
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
resolveClientMetadataandClientMetadatatype into@certified-app/sharedso both pds-core and auth-service can use thembranding.csssupport toClientMetadatawithescapeCss()(prevents</style>injection) andgetClientCss()(trust-gated helper)<style>tag (CSP already allows'unsafe-inline')/oauth/authorizeresponses — wrapsres.setHeader/res.endto inject<style>tag and append SHA256 hash to CSPstyle-srcdirectivePDS_OAUTH_TRUSTED_CLIENTSenv var (comma-separated client_id URLs) to all env examplesHow it works
client-metadata.jsonunderbranding.cssPDS_OAUTH_TRUSTED_CLIENTS(must be set identically)style-srcsince the stock oauth-provider uses strict hash-based CSP'unsafe-inline'already allowed)Test plan
PDS_OAUTH_TRUSTED_CLIENTSset to demo's client_id, verify CSS appears on login/consent pagesRef: atproto-9fa
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores