refactor(pds-core): extract OAuth client-id resolution from CSS middleware - #241
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
🚅 Deployed to the ePDS-pr-241 environment in ePDS
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change adds shared OAuth client ID resolution for direct ChangesOAuth client resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CSSInjectionMiddleware
participant resolveOAuthClientIdFromQuery
participant ResolveClientIdFromRequestUri
participant Logger
CSSInjectionMiddleware->>resolveOAuthClientIdFromQuery: Resolve client ID from query
resolveOAuthClientIdFromQuery->>ResolveClientIdFromRequestUri: Resolve request_uri when needed
ResolveClientIdFromRequestUri-->>resolveOAuthClientIdFromRequestUri: Return client ID or error
resolveOAuthClientIdFromQuery-->>CSSInjectionMiddleware: Return resolved ID or error
CSSInjectionMiddleware->>Logger: Log request URI presence metadata
CSSInjectionMiddleware->>CSSInjectionMiddleware: Continue processing
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 31088414989Coverage increased (+0.3%) to 58.36%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pds-core/src/lib/client-css-injection.ts`:
- Around line 170-174: The error path in resolveClientIdFromRequestUri currently
logs the sensitive raw query.request_uri; replace that field in the logger.error
call with non-sensitive context such as hasRequestUri. Add or retain a
regression test for this branch asserting the logged error does not contain a
raw requestUri value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1e40245-95e7-4135-9fd8-97b657f84f9d
📒 Files selected for processing (4)
packages/pds-core/src/__tests__/client-css-injection.test.tspackages/pds-core/src/__tests__/oauth-request-context.test.tspackages/pds-core/src/lib/client-css-injection.tspackages/pds-core/src/lib/oauth-request-context.ts
There was a problem hiding this comment.
Pull request overview
This PR refactors pds-core OAuth response-enrichment utilities by extracting “OAuth client_id resolution from authorize request query/PAR request_uri” into a shared helper, so multiple middlewares can reuse the same resolution logic without duplication.
Changes:
- Introduces
resolveOAuthClientIdFromQuery()in a newoauth-request-contexthelper module. - Updates the client CSS injection middleware to use the shared resolver and adjusts logging (including adding
logger.errorto the middleware’s logger contract). - Adds unit test coverage for the new resolver helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/pds-core/src/lib/oauth-request-context.ts | New shared helper to resolve OAuth client_id from either explicit query param or PAR request_uri. |
| packages/pds-core/src/lib/client-css-injection.ts | Refactors inline resolution into the shared helper; adds LoggerLike.error and changes failure logging path. |
| packages/pds-core/src/tests/oauth-request-context.test.ts | New unit tests covering resolution order, fallbacks, and error propagation. |
| packages/pds-core/src/tests/client-css-injection.test.ts | Updates logger mock shape to include error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
request_uri is a short-lived bearer reference to the PAR entry, so a log line carrying its value is replayable by anyone who can read the logs. Log whether it was present instead. Raised independently by CodeRabbit and Copilot on #241. The value was already being logged before this series; hoisting the resolution out of the request_uri guard promoted the line from warn to error, which made it more visible without making it safe. #80 fixes the same leak on the pre-refactor code. typeof rather than a hardcoded true: the log now sits outside the `if (requestUri)` guard, so presence is no longer implied by reaching it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in - { err, requestUri: query.request_uri },
+ { err, hasRequestUri: typeof query.request_uri === 'string' },
Worth being clear that I introduced the more visible half of this: the value was already being logged before this series, but promoting the line from Added a regression test that asserts the log context contains #80 fixes the same leak on the pre-refactor code. Whichever lands first, the other is a small rebase. |
|
Tip For best results, initiate chat on the files or code changes.
Using 🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
…eware OAuth authorize pages carry either an explicit client_id query parameter or only a PAR request_uri that has to be resolved through the provider's request manager. That resolution lived inline in the CSS injection middleware, but chooser enrichment needs exactly the same step, so hoist it into a shared module rather than duplicating it at the second call site. Resolver errors stay with the caller so each middleware keeps its own logging and fallback behaviour. The CSS middleware's failure log moves from warn to error, since a client_id that cannot be resolved means the page renders unbranded. Split out of #148, which bundled this refactor with the enrichment work that motivated it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
request_uri is a short-lived bearer reference to the PAR entry, so a log line carrying its value is replayable by anyone who can read the logs. Log whether it was present instead. Raised independently by CodeRabbit and Copilot on #241. The value was already being logged before this series; hoisting the resolution out of the request_uri guard promoted the line from warn to error, which made it more visible without making it safe. #80 fixes the same leak on the pre-refactor code. typeof rather than a hardcoded true: the log now sits outside the `if (requestUri)` guard, so presence is no longer implied by reaching it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ee1ec80 to
707355b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto Resolved in favour of this PR's refactored form, which preserves #80's intent: logger.error(
{ err, hasRequestUri: typeof query.request_uri === 'string' },
'CSS middleware: failed to resolve client_id from request_uri',
)#80 landed The Verified the full stack (#241 → #243 → #244, plus #242) still reproduces #148 exactly: merging all four branches and diffing against #148's head merged into current All four rebased branches pass typecheck, lint, format and the full vitest suite locally. |
|
Split 1 of 4 from #148, which bundled four independent concerns behind ~1900 lines of enrichment tests. This is the shared dependency the others build on.
What it does
OAuth authorize pages carry either an explicit
client_idquery parameter or only a PARrequest_urithat has to be resolved through the provider's request manager. That resolution lived inline in the CSS injection middleware. Chooser enrichment (split 4) needs exactly the same step, so this hoists it intopackages/pds-core/src/lib/oauth-request-context.tsrather than duplicating it at the second call site.Resolver errors stay with the caller, so each middleware keeps its own logging and fallback behaviour.
Behaviour change
One, deliberate: the CSS middleware's failed-resolution log moves from
warntoerror, andLoggerLikegains anerrormethod. Aclient_idthat cannot be resolved means the page renders unbranded, which is worth an error-level line.Otherwise this is a pure refactor — same resolution order (explicit
client_idfirst, then PAR lookup), same fallbacks.No changeset: internal refactor with no observable behaviour change for end users, client app developers, or operators.
Verification
typecheck,lint,formatclean; 74 test files / 1129 tests pass.Note on #80
This touches the same log statement that #80 redacts
requestUrifrom. Whichever lands first, the other is a one-line rebase.Series
epds_handle_modethrough the callback hopThe four branches together are byte-identical to #148's head, verified by merging them and diffing against
7fe24bb(the only difference is the original single changeset, split into three).🤖 Generated with Claude Code
Summary by CodeRabbit