fix(services-bff): Disable PAR until ids resolves error #17176
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve modifications to multiple configuration files across various services, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (11)
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
|
![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 (2)
infra/src/dsl/bff.ts (2)
81-81: Add documentation for temporary PAR disable.While the change correctly disables PAR support as intended, please add an inline comment explaining why PAR is disabled and that this is a temporary measure.
+ // Temporarily disabled PAR support due to IDS not handling expired request URIs correctly + // TODO: Re-enable once IDS properly handles expired request URIs BFF_PAR_SUPPORT_ENABLED: 'false',
81-81: Create a tracking issue for re-enabling PAR.Since this is a temporary fix, we should track it to ensure PAR gets re-enabled once the IDS service properly handles expired request URIs.
Would you like me to create a GitHub issue to track the re-enabling of PAR support? The issue would include:
- Current status and reason for disabling
- Acceptance criteria for re-enabling
- Links to related PRs/issues
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
infra/src/dsl/bff.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
infra/src/dsl/bff.ts (1)
Pattern infra/src/dsl/**/*: "Confirm that the code adheres to the following:
- The clarity and expressiveness of the DSL syntax.
- Integration with Helm charts and Kubernetes resources.
- Documentation on how to use the DSL to create complex Helm values."
🔇 Additional comments (1)
infra/src/dsl/bff.ts (1)
81-81: Verify security implications of disabling PAR.
While this change addresses the immediate issue with expired request URIs, we should verify the security impact of disabling PAR authentication.
Run the following to analyze PAR usage and dependencies:
Consider implementing additional security measures while PAR is disabled:
- Enhanced request validation
- Additional logging for authentication flows
- Rate limiting if not already in place
✅ Verification successful
Let me gather more specific information about PAR usage and security implications.
Let me search for OAuth and authentication flow configurations to better understand the security implications.
Based on the gathered information, I can now provide a final assessment of the PAR security implications.
Disabling PAR is acceptable with current security controls
The codebase shows that:
- PAR (Pushed Authorization Request) is an optional security feature, as evidenced by its configuration being toggleable
- The BFF (Backend-for-Frontend) maintains secure redirect URI validation through
BFF_ALLOWED_REDIRECT_URIS - The system still enforces OAuth flow security through whitelisted redirect URIs for each environment
While PAR adds an extra security layer by pre-registering authorization requests, its absence is mitigated by:
- Strict redirect URI validation
- Environment-specific URI whitelisting
- Backend-for-Frontend architecture that handles auth flows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PAR-related configurations and dependencies
# Search for PAR-related code and configurations
echo "=== PAR Usage Analysis ==="
rg -i "PAR|pushed.authorization" --type ts
# Check for security-related comments or documentation
echo "=== Security Documentation ==="
rg -i "security|auth|oauth" "README.md" || true
Length of output: 68002
Script:
#!/bin/bash
# Search for PAR-related security configurations and OAuth flows
echo "=== PAR Security Analysis ==="
rg -i "PAR.*enabled|pushed.*authorization" -B 3 -A 3 infra/src/dsl/
# Search for OAuth/auth configuration files
echo "=== OAuth Configuration Analysis ==="
fd -e ts -e js -e json "oauth|auth" infra/src/
# Search for security documentation in markdown files
echo "=== Security Documentation ==="
fd -e md . -x rg -i "PAR|pushed.*authorization|oauth.*flow"
Length of output: 23626
Script:
#!/bin/bash
# Search for OAuth and authentication flow configurations
echo "=== OAuth Flow Analysis ==="
rg -i "oauth.*flow|authentication.*flow" -B 3 -A 3 infra/src/
# Search for BFF (Backend-for-Frontend) security configurations
echo "=== BFF Security Configuration ==="
rg -i "BFF.*security|BFF.*auth" -B 3 -A 3 infra/src/
# Check for redirect URI configurations
echo "=== Redirect URI Configuration ==="
rg "BFF_ALLOWED_REDIRECT_URIS" -B 2 -A 2 infra/src/
Length of output: 1451
* fix(services-bff): Disable PAR until ids resolves error * fix tests * chore: charts update dirty files --------- Co-authored-by: andes-it <builders@andes.is>
Disable PAR in bff servers
What
Disable PAR in bff servers.
Why
Ids is not handling expired request uris properly. This disables PAR auth on both bff servers. This will be reverted once confident on propper error handling in PAR
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
BFF_PAR_SUPPORT_ENABLEDproperty from 'true' to 'false' across multiple services.