fix: BFF_ALLOWED_REDIRECT_URIS as array #16748
Conversation
WalkthroughThe pull request introduces several modifications to service configurations within 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
|
![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
🧹 Outside diff range and nitpick comments (1)
infra/src/dsl/bff.ts (1)
Line range hint
1-89: Consider adding DSL documentationThe DSL configuration is well-structured but lacks documentation explaining the purpose of each environment variable and their expected formats. Consider adding JSDoc comments to improve maintainability.
Example addition:
/** * Generates BFF configuration for different environments * @param {BffInfo} config - BFF configuration parameters * @returns {Object} Environment-specific configuration including: * - IDENTITY_SERVER_* - Identity server connection settings * - BFF_* - BFF-specific settings including redirect URIs (as JSON arrays) */ export const bffConfig = ({ key, services, clientName, clientId }: BffInfo) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
charts/islandis/values.prod.yaml(1 hunks)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)
55-55: LGTM: Format consistency improvement
The change to use json(['https://island.is']) aligns the production configuration with other environments and provides proper type safety through array format.
Let's verify the usage of this configuration across the codebase:
✅ Verification successful
Change is safe: BFF already expects array format
The verification confirms this is a safe change:
- The BFF config (
bff.config.ts) usesenv.requiredJSON()to parse the value, expecting JSON array format - Test setup already uses array format:
'["http://test-client.com/testclient"]' - All environment values files (dev, staging, prod) already use JSON array string format
- The change in the DSL only standardizes the configuration generation, while maintaining the expected array format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct string references to BFF_ALLOWED_REDIRECT_URIS
# to ensure all consumers expect array format
rg -l "BFF_ALLOWED_REDIRECT_URIS" --type ts --type yaml
Length of output: 319
Script:
#!/bin/bash
# Check how BFF_ALLOWED_REDIRECT_URIS is used in the configuration and tests
rg "BFF_ALLOWED_REDIRECT_URIS" -A 5 -B 5 apps/services/bff/src/app/bff.config.ts apps/services/bff/test/setup.ts
Length of output: 2001
Script:
#!/bin/bash
# Check the values files to see how this is configured in different environments
rg "BFF_ALLOWED_REDIRECT_URIS" -A 2 -B 2 charts/islandis/values.*.yaml
Length of output: 1458
| @@ -2157,7 +2157,7 @@ services-bff-portals-admin: | |||
| enabled: true | |||
| env: | |||
| BFF_ALLOWED_EXTERNAL_API_URLS: '["https://api.island.is"]' | |||
| BFF_ALLOWED_REDIRECT_URIS: 'https://island.is' | |||
| BFF_ALLOWED_REDIRECT_URIS: '["https://island.is"]' | |||
There was a problem hiding this comment.
💡 Codebase verification
Additional redirect URIs needed for production environment
Based on the seeder files and configuration analysis, the current BFF_ALLOWED_REDIRECT_URIS setting is incomplete. The following paths should also be included:
https://island.is/umsoknir/signin-oidchttps://island.is/umsoknir/silent/signin-oidc
These URIs are used in production as evidenced by the seeder file 20210323153127-applications-redirecturis.js and are essential for the application's authentication flow to work correctly.
🔗 Analysis chain
Verify completeness of allowed redirect URIs list
The change to make BFF_ALLOWED_REDIRECT_URIS an array is correct, but please verify if "https://island.is" is the only allowed redirect URI needed for production. Common patterns often require additional URLs like:
- www subdomain variant
- Development/staging environments
- Additional valid paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for current redirect URI usage in the codebase
rg -l "redirect.*island\.is"
Length of output: 307
Script:
#!/bin/bash
# Check content of seeder files for redirect URIs
cat libs/auth-api-lib/seeders/20210426083138-add-admin-uris.js libs/auth-api-lib/seeders/20210323153127-applications-redirecturis.js
# Search for redirect URI configurations in the codebase
rg -A 5 "redirectUri|redirect_uri|REDIRECT_URI" --type yaml
Length of output: 10676
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Chores