fix(services-bff): Move params from ids par requests body to header #16761
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
snaerseljan
added
the
deprecated:automerge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/services/bff/src/app/modules/ids/ids.service.ts (1)
126-140: Consider renaming the method to be more generic.While the implementation is correct, the method name
createPARAuthorizationHeadersuggests it's specific to PAR (Pushed Authorization Requests). Since this header is used for all OAuth endpoints (tokens, revocation, etc.), consider renaming it tocreateBasicAuthorizationHeaderorcreateClientAuthorizationHeader.- createPARAuthorizationHeader() { + createBasicAuthorizationHeader() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/services/bff/src/app/modules/ids/ids.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/services/bff/src/app/modules/ids/ids.service.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (3)
apps/services/bff/src/app/modules/ids/ids.service.ts (3)
46-46: LGTM! Authorization header implementation follows OAuth 2.0 spec.
The addition of the Authorization header aligns with RFC 6749 section 2.3.1 and improves security by moving client credentials out of the request body.
46-46: Verify error handling for authentication failures.
While the error handling looks good for general API responses, we should ensure specific handling for authentication failures (HTTP 401/403).
#!/bin/bash
# Search for authentication error handling in tests
echo "Checking for authentication error test coverage..."
rg -A 5 "401|403|unauthorized|forbidden" --type specConsider adding specific error handling for authentication failures to provide clear error messages to clients:
if (response.status === 401 || response.status === 403) {
return {
type: 'error',
data: {
error: 'authentication_failed',
error_description: 'Invalid client credentials'
}
} as ErrorRes;
}Also applies to: 51-77
151-154: Verify client credentials removal from request bodies.
The implementation correctly moves authentication to headers while maintaining client_id in request parameters. Let's verify that client credentials have been removed from request bodies across the codebase.
✅ Verification successful
Client credentials have been properly removed from request bodies
Based on the comprehensive search results, all postRequest implementations across the codebase have been reviewed and show:
- In
ids.service.ts, client credentials are correctly handled throughgetLoginSearchParams - Other services (
documentClient,documentProviderClient) use different authentication mechanisms and don't contain client credentials in request bodies - No instances of
client_secretbeing passed directly in request bodies were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of client credentials in request bodies
echo "Checking for client_secret in request bodies..."
rg -l "client_secret.*postRequest|postRequest.*client_secret" --type ts
echo "Checking for explicit client_id in request bodies (excluding URL params)..."
rg -l "client_id.*postRequest|postRequest.*client_id" --type ts | \
grep -v "getLoginSearchParams"
Length of output: 534
Script:
#!/bin/bash
# Search for any remaining instances of client credentials in request bodies
echo "Checking for client_secret in request bodies..."
rg "client_secret.*postRequest|postRequest.*client_secret" --type ts
echo -e "\nChecking for explicit client_id in request bodies..."
rg "client_id.*postRequest|postRequest.*client_id" --type ts
Length of output: 359
Script:
#!/bin/bash
# Let's try a different approach by looking at postRequest usage patterns
echo "Checking postRequest implementations..."
ast-grep --pattern 'postRequest<$_>($$$)'
echo -e "\nChecking getLoginSearchParams usage..."
ast-grep --pattern 'getLoginSearchParams($$$)'
Length of output: 274
Script:
#!/bin/bash
# Search for all TypeScript files containing postRequest method calls
echo "Files containing postRequest calls:"
rg -l "postRequest" --type ts
echo -e "\nDetailed postRequest implementations:"
rg -B2 -A2 "postRequest" --type ts
Length of output: 13872
…16761) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…16761) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…16761) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…16761) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Move client_id and client_secret from ids par requests body to header
What
This PR moves all client id and secrets from request body to authorization header instead
Why
Client secret was leaking to identity server logs. Duenedes documentation alters from Auth0 2.0 documentation
https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes