[OLD][HYPER-109] feat: custom CSS injection for trusted OAuth clients#9
Closed
[OLD][HYPER-109] feat: custom CSS injection for trusted OAuth clients#9
Conversation
|
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 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 |
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.
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.
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
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
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 Update to this: I think it might be better to use the flow id and have it be consistent with the rest of how its done |
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
Kzoeps
force-pushed
the
css-injection-trusted-clients
branch
from
976e670 to
a3c2fff
Compare
aspiers
force-pushed
the
css-injection-trusted-clients
branch
from
a3c2fff to
d50b368
Compare
Kzoeps
force-pushed
the
css-injection-trusted-clients
branch
from
d50b368 to
7ac64ca
Compare
Kzoeps
force-pushed
the
css-injection-trusted-clients
branch
from
7ac64ca to
fa47188
Compare
…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>
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>
aspiers
force-pushed
the
css-injection-trusted-clients
branch
from
fa47188 to
fae43f7
Compare
Wire up three active scenarios in features/client-branding.feature: - Trusted client's custom CSS is present in the login page HTML - Trusted and untrusted demos produce visibly different login pages (computed body background colors differ) - Untrusted client gets no CSS injection and renders with the default background color Also drop the CSP-hash assertion from the trusted-login scenario — the auth-service login-page template doesn't wire a CSP style-src hash (only the pds-core middleware on /oauth/authorize does). The consent-page CSS scenario is marked @Manual until we have a returning-user reauth fixture to reach the upstream consent UI. Relies on E2E_DEMO_URL and E2E_DEMO_UNTRUSTED_URL pointing at two separately-deployed instances of packages/demo, only one of which is listed in PDS_OAUTH_TRUSTED_CLIENTS. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
aspiers
added a commit
that referenced
this pull request
Apr 10, 2026
Wire ePDS for cross-client session reuse so a user who has signed in once via any OAuth client is not re-prompted for email OTP by a second client in the same browser. Three changes working together, guarded by the new PDS_COOKIE_DOMAIN env var — deployments that don't set it get the current behaviour unchanged. 1. pds-core: broaden device-session cookie domain When PDS_COOKIE_DOMAIN is set, inject Domain=<parent> into outbound Set-Cookie headers for dev-id, ses-id, and their :hash siblings. Upstream @atproto/oauth-provider's DeviceManager has no domain option and scopes the cookies host-only by default; rewriting the headers makes the cookies visible to sibling subdomains like auth.pds.foo.com. 2. auth-service: skip the email form when a device session is present At the top of GET /oauth/authorize, check for a dev-id cookie on the request. If present and prompt=login is not set, 302 to pds-core's upstream /oauth/authorize with the same query string so upstream's middleware can either auto-select the matching session (flow 1, login_hint supplied) or render the account chooser (flow 2). If absent, render the email form exactly as today. 3. pds-core: enrich the upstream account chooser Reuse PR #9's response-rewrite pattern to inject a small script into /account* HTML responses. The script captures the __deviceSessions hydration payload (which already carries each account's email) via an accessor on window, then uses a MutationObserver to (a) append each account's email alongside the rendered handle row and (b) add a "Use a different account" link pointing at auth.<host>/oauth/ authorize?prompt=login — the escape hatch that sends users back to the email form when they want to sign in as someone else. Scenario coverage in features/passwordless-authentication.feature (committed earlier) exercises all four session-reuse branches: flow 1 auto-skip, flow 2 chooser-with-confirm, flow 2 pre-approved auto-authorize, and flow 2 "use a different account". Design doc at docs/design/hyper-268-session-reuse.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
aspiers
added a commit
that referenced
this pull request
Apr 10, 2026
Wire ePDS for cross-client session reuse so a user who has signed in once via any OAuth client is not re-prompted for email OTP by a second client in the same browser. Three changes working together, guarded by the new PDS_COOKIE_DOMAIN env var — deployments that don't set it get the current behaviour unchanged. 1. pds-core: broaden device-session cookie domain When PDS_COOKIE_DOMAIN is set, inject Domain=<parent> into outbound Set-Cookie headers for dev-id, ses-id, and their :hash siblings. Upstream @atproto/oauth-provider's DeviceManager has no domain option and scopes the cookies host-only by default; rewriting the headers makes the cookies visible to sibling subdomains like auth.pds.foo.com. 2. auth-service: skip the email form when a device session is present At the top of GET /oauth/authorize, check for a dev-id cookie on the request. If present and prompt=login is not set, 302 to pds-core's upstream /oauth/authorize with the same query string so upstream's middleware can either auto-select the matching session (flow 1, login_hint supplied) or render the account chooser (flow 2). If absent, render the email form exactly as today. 3. pds-core: enrich the upstream account chooser Reuse PR #9's response-rewrite pattern to inject a small script into /account* HTML responses. The script captures the __deviceSessions hydration payload (which already carries each account's email) via an accessor on window, then uses a MutationObserver to (a) append each account's email alongside the rendered handle row and (b) add a "Use a different account" link pointing at auth.<host>/oauth/ authorize?prompt=login — the escape hatch that sends users back to the email form when they want to sign in as someone else. Scenario coverage in features/passwordless-authentication.feature (committed earlier) exercises all four session-reuse branches: flow 1 auto-skip, flow 2 chooser-with-confirm, flow 2 pre-approved auto-authorize, and flow 2 "use a different account". Design doc at docs/design/hyper-268-session-reuse.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 10, 2026
Wire ePDS for cross-client session reuse so a user who has signed in once via any OAuth client is not re-prompted for email OTP by a second client in the same browser. Three changes working together, guarded by the new PDS_COOKIE_DOMAIN env var — deployments that don't set it get the current behaviour unchanged. 1. pds-core: broaden device-session cookie domain When PDS_COOKIE_DOMAIN is set, inject Domain=<parent> into outbound Set-Cookie headers for dev-id, ses-id, and their :hash siblings. Upstream @atproto/oauth-provider's DeviceManager has no domain option and scopes the cookies host-only by default; rewriting the headers makes the cookies visible to sibling subdomains like auth.pds.foo.com. 2. auth-service: skip the email form when a device session is present At the top of GET /oauth/authorize, check for a dev-id cookie on the request. If present and prompt=login is not set, 302 to pds-core's upstream /oauth/authorize with the same query string so upstream's middleware can either auto-select the matching session (flow 1, login_hint supplied) or render the account chooser (flow 2). If absent, render the email form exactly as today. 3. pds-core: enrich the upstream account chooser Reuse PR #9's response-rewrite pattern to inject a small script into /account* HTML responses. The script captures the __deviceSessions hydration payload (which already carries each account's email) via an accessor on window, then uses a MutationObserver to (a) append each account's email alongside the rendered handle row and (b) add a "Use a different account" link pointing at auth.<host>/oauth/ authorize?prompt=login — the escape hatch that sends users back to the email form when they want to sign in as someone else. Scenario coverage in features/passwordless-authentication.feature (committed earlier) exercises all four session-reuse branches: flow 1 auto-skip, flow 2 chooser-with-confirm, flow 2 pre-approved auto-authorize, and flow 2 "use a different account". Design doc at docs/design/hyper-268-session-reuse.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 10, 2026
Port the CSS injection e2e scenarios from the css-injection-trusted-clients branch (PR #9, commit ba74e45) and adapt them to the tagging and skippability infrastructure on this branch. Three active CSS-injection scenarios: 1. "Trusted client's CSS is applied to the login page" — checks that the injected <style> tag with the demo's branding CSS appears in the login page HTML when the trusted demo client initiates OAuth. 2. "Trusted and untrusted demo clients render visibly differently" — captures computed body background color from both demos and asserts they differ. Tagged @untrusted-client. 3. "Untrusted client does not get CSS injection" — asserts the injected CSS is absent and the default background color is used. Tagged @untrusted-client. The consent-page CSS scenario is marked @Manual (needs a reauth fixture to reach the upstream PDS consent UI). The 6 email-template scenarios are tagged @not-implemented individually (the email-template feature does not exist yet). The Feature-level @not-implemented tag is removed since the CSS scenarios now have step definitions. Background is enriched with the trusted demo metadata discoverability check to match consent-screen.feature's pattern. Step definitions are in e2e/step-definitions/client-branding.steps.ts. The two untrusted-client scenarios reuse the shared "the untrusted demo client initiates an OAuth login" step from consent.steps.ts, which already has the if (!testEnv.demoUntrustedUrl) return 'pending' guard from the previous commit. A capturedBgColors property is added to EpdsWorld for the computed-color comparison scenario. Dry-run: - With E2E_DEMO_UNTRUSTED_URL: 22 scenarios (19 + 3 new) - Without: 14 scenarios (13 + 1 trusted-only)
aspiers
added a commit
that referenced
this pull request
Apr 10, 2026
Port the CSS injection e2e scenarios from the css-injection-trusted-clients branch (PR #9, commit ba74e45) and adapt them to the tagging and skippability infrastructure on this branch. Three active CSS-injection scenarios: 1. "Trusted client's CSS is applied to the login page" — checks that the injected <style> tag with the demo's branding CSS appears in the login page HTML when the trusted demo client initiates OAuth. 2. "Trusted and untrusted demo clients render visibly differently" — captures computed body background color from both demos and asserts they differ. Tagged @untrusted-client. 3. "Untrusted client does not get CSS injection" — asserts the injected CSS is absent and the default background color is used. Tagged @untrusted-client. The consent-page CSS scenario is marked @Manual (needs a reauth fixture to reach the upstream PDS consent UI). The 6 email-template scenarios are tagged @not-implemented individually (the email-template feature does not exist yet). The Feature-level @not-implemented tag is removed since the CSS scenarios now have step definitions. Background is enriched with the trusted demo metadata discoverability check to match consent-screen.feature's pattern. Step definitions are in e2e/step-definitions/client-branding.steps.ts. The two untrusted-client scenarios reuse the shared "the untrusted demo client initiates an OAuth login" step from consent.steps.ts, which already has the if (!testEnv.demoUntrustedUrl) return 'pending' guard from the previous commit. A capturedBgColors property is added to EpdsWorld for the computed-color comparison scenario. Dry-run: - With E2E_DEMO_UNTRUSTED_URL: 22 scenarios (19 + 3 new) - Without: 14 scenarios (13 + 1 trusted-only)
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
After a user signs in once via any OAuth client, a second client in the same browser no longer re-prompts for email OTP. ePDS recognises the existing device session and either auto-redirects (flow 1, login_hint matches) or shows the upstream account chooser (flow 2). Three pieces working together: 1. pds-core: broaden device-session cookie domain Auto-derived from AUTH_HOSTNAME / PDS_HOSTNAME: if AUTH_HOSTNAME ends with .<PDS_HOSTNAME>, inject Domain=<PDS_HOSTNAME> into outbound Set-Cookie headers for dev-id/ses-id and their :hash sidecars. On Railway preview envs (unrelated hostnames under a public suffix) the check fails and the middleware is silently skipped. 2. auth-service: skip the email form when a device session exists GET /oauth/authorize checks for a dev-id cookie. If present and prompt=login is not set, 302s to pds-core's upstream /oauth/authorize. If absent, renders the email form as today. 3. pds-core: enrich the upstream account chooser Response-rewrite middleware (extending PR #9's pattern) injects a script into /account* HTML responses that surfaces each account's email alongside the handle and adds a "Use a different account" link pointing at auth.<host>/oauth/authorize?prompt=login. Also includes: - 4 Gherkin scenarios (tagged @session-reuse, excluded from default CI) - 30 unit tests for the extracted pure helpers - Design doc at docs/design/hyper-268-session-reuse.md - Changeset Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
After a user signs in once via any OAuth client, a second client in the same browser no longer re-prompts for email OTP. ePDS recognises the existing device session and either auto-redirects (flow 1, login_hint matches) or shows the upstream account chooser (flow 2). Three pieces working together: 1. pds-core: broaden device-session cookie domain Auto-derived from AUTH_HOSTNAME / PDS_HOSTNAME: if AUTH_HOSTNAME ends with .<PDS_HOSTNAME>, inject Domain=<PDS_HOSTNAME> into outbound Set-Cookie headers for dev-id/ses-id and their :hash sidecars. On Railway preview envs (unrelated hostnames under a public suffix) the check fails and the middleware is silently skipped. 2. auth-service: skip the email form when a device session exists GET /oauth/authorize checks for a dev-id cookie. If present and prompt=login is not set, 302s to pds-core's upstream /oauth/authorize. If absent, renders the email form as today. 3. pds-core: enrich the upstream account chooser Response-rewrite middleware (extending PR #9's pattern) injects a script into /account* HTML responses that surfaces each account's email alongside the handle and adds a "Use a different account" link pointing at auth.<host>/oauth/authorize?prompt=login. Also includes: - 4 Gherkin scenarios (tagged @session-reuse, excluded from default CI) - 30 unit tests for the extracted pure helpers - Design doc at docs/design/hyper-268-session-reuse.md - Changeset Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
…urfaces PR #9 injects trusted-client CSS into the auth-service login page and the pds-core /oauth/authorize consent page, but four other auth-service HTML pages are not yet covered: - Auth-service consent page (HYPER-296) - Choose-handle page (HYPER-297) - Recovery page (HYPER-298) - Account settings page (HYPER-299) Add @not-implemented scenarios in features/client-branding.feature for each. These document the expected behaviour and will be activated when the corresponding injection code is written.
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
After a user signs in once via any OAuth client, a second client in the same browser no longer re-prompts for email OTP. ePDS recognises the existing device session and either auto-redirects (flow 1, login_hint matches) or shows the upstream account chooser (flow 2). Three pieces working together: 1. pds-core: broaden device-session cookie domain Auto-derived from AUTH_HOSTNAME / PDS_HOSTNAME: if AUTH_HOSTNAME ends with .<PDS_HOSTNAME>, inject Domain=<PDS_HOSTNAME> into outbound Set-Cookie headers for dev-id/ses-id and their :hash sidecars. On Railway preview envs (unrelated hostnames under a public suffix) the check fails and the middleware is silently skipped. 2. auth-service: skip the email form when a device session exists GET /oauth/authorize checks for a dev-id cookie. If present and prompt=login is not set, 302s to pds-core's upstream /oauth/authorize. If absent, renders the email form as today. 3. pds-core: enrich the upstream account chooser Response-rewrite middleware (extending PR #9's pattern) injects a script into /account* HTML responses that surfaces each account's email alongside the handle and adds a "Use a different account" link pointing at auth.<host>/oauth/authorize?prompt=login. Also includes: - 4 Gherkin scenarios (tagged @session-reuse, excluded from default CI) - 30 unit tests for the extracted pure helpers - Design doc at docs/design/hyper-268-session-reuse.md - Changeset Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
After a user signs in once via any OAuth client, a second client in the same browser no longer re-prompts for email OTP. ePDS recognises the existing device session and either auto-redirects (flow 1, login_hint matches) or shows the upstream account chooser (flow 2). Three pieces working together: 1. pds-core: broaden device-session cookie domain Auto-derived from AUTH_HOSTNAME / PDS_HOSTNAME: if AUTH_HOSTNAME ends with .<PDS_HOSTNAME>, inject Domain=<PDS_HOSTNAME> into outbound Set-Cookie headers for dev-id/ses-id and their :hash sidecars. On Railway preview envs (unrelated hostnames under a public suffix) the check fails and the middleware is silently skipped. 2. auth-service: skip the email form when a device session exists GET /oauth/authorize checks for a dev-id cookie. If present and prompt=login is not set, 302s to pds-core's upstream /oauth/authorize. If absent, renders the email form as today. 3. pds-core: enrich the upstream account chooser Response-rewrite middleware (extending PR #9's pattern) injects a script into /account* HTML responses that surfaces each account's email alongside the handle and adds a "Use a different account" link pointing at auth.<host>/oauth/authorize?prompt=login. Also includes: - 4 Gherkin scenarios (tagged @session-reuse, excluded from default CI) - 30 unit tests for the extracted pure helpers - Design doc at docs/design/hyper-268-session-reuse.md - Changeset Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
After a user signs in once via any OAuth client, a second client in the same browser no longer re-prompts for email OTP. ePDS recognises the existing device session and either auto-redirects (flow 1, login_hint matches) or shows the upstream account chooser (flow 2). Three pieces working together: 1. pds-core: broaden device-session cookie domain Auto-derived from AUTH_HOSTNAME / PDS_HOSTNAME: if AUTH_HOSTNAME ends with .<PDS_HOSTNAME>, inject Domain=<PDS_HOSTNAME> into outbound Set-Cookie headers for dev-id/ses-id and their :hash sidecars. On Railway preview envs (unrelated hostnames under a public suffix) the check fails and the middleware is silently skipped. 2. auth-service: skip the email form when a device session exists GET /oauth/authorize checks for a dev-id cookie. If present and prompt=login is not set, 302s to pds-core's upstream /oauth/authorize. If absent, renders the email form as today. 3. pds-core: enrich the upstream account chooser Response-rewrite middleware (extending PR #9's pattern) injects a script into /account* HTML responses that surfaces each account's email alongside the handle and adds a "Use a different account" link pointing at auth.<host>/oauth/authorize?prompt=login. Also includes: - 4 Gherkin scenarios (tagged @session-reuse, excluded from default CI) - 30 unit tests for the extracted pure helpers - Design doc at docs/design/hyper-268-session-reuse.md - Changeset Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
aspiers
added a commit
that referenced
this pull request
Apr 13, 2026
Port the CSS injection e2e scenarios from the css-injection-trusted-clients branch (PR #9, commit ba74e45) and adapt them to the tagging and skippability infrastructure on this branch. Three active CSS-injection scenarios: 1. "Trusted client's CSS is applied to the login page" — checks that the injected <style> tag with the demo's branding CSS appears in the login page HTML when the trusted demo client initiates OAuth. 2. "Trusted and untrusted demo clients render visibly differently" — captures computed body background color from both demos and asserts they differ. Tagged @untrusted-client. 3. "Untrusted client does not get CSS injection" — asserts the injected CSS is absent and the default background color is used. Tagged @untrusted-client. The consent-page CSS scenario is marked @Manual (needs a reauth fixture to reach the upstream PDS consent UI). The 6 email-template scenarios are tagged @not-implemented individually (the email-template feature does not exist yet). The Feature-level @not-implemented tag is removed since the CSS scenarios now have step definitions. Background is enriched with the trusted demo metadata discoverability check to match consent-screen.feature's pattern. Step definitions are in e2e/step-definitions/client-branding.steps.ts. The two untrusted-client scenarios reuse the shared "the untrusted demo client initiates an OAuth login" step from consent.steps.ts, which already has the if (!testEnv.demoUntrustedUrl) return 'pending' guard from the previous commit. A capturedBgColors property is added to EpdsWorld for the computed-color comparison scenario. Dry-run: - With E2E_DEMO_UNTRUSTED_URL: 22 scenarios (19 + 3 new) - Without: 14 scenarios (13 + 1 trusted-only)
aspiers
added a commit
that referenced
this pull request
Apr 13, 2026
…urfaces PR #9 injects trusted-client CSS into the auth-service login page and the pds-core /oauth/authorize consent page, but four other auth-service HTML pages are not yet covered: - Auth-service consent page (HYPER-296) - Choose-handle page (HYPER-297) - Recovery page (HYPER-298) - Account settings page (HYPER-299) Add @not-implemented scenarios in features/client-branding.feature for each. These document the expected behaviour and will be activated when the corresponding injection code is written.
6 tasks
aspiers
changed the title
Contributor
Author
|
Superseded by #48 |
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