[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.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds trusted-client configuration and client branding support: new shared client-metadata utilities, re-exports in shared, auth-service and pds-core updated to resolve client metadata and inject client-specific CSS into rendered OAuth/login/consent pages; OTP UI configuration options and env docs added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OAuth Client
participant PDS as PDS Core
participant Shared as Shared Metadata
participant Auth as Auth Service
participant Browser as Browser/User
Client->>PDS: GET /oauth/authorize?client_id=...
PDS->>PDS: Check client_id against trustedClients
alt trusted
PDS->>Shared: resolveClientMetadata(client_id)
Shared-->>PDS: ClientMetadata (including 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: User interacts / submits login/consent
Auth->>Shared: resolveClientMetadata(client_id)
Shared-->>Auth: ClientMetadata
Auth->>Shared: getClientCss(client_id, metadata, trustedClients)
Shared-->>Auth: Escaped CSS
Auth->>Auth: Include customCss in render output
Auth-->>Browser: Branded HTML
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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