feat(salesforce): PKCE support + userURL configurable#522
Open
phof wants to merge 6 commits into
Open
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSalesforce OAuth handler: adds PKCE (verifier/challenge), includes PKCE params on authorization redirect, sends ChangesSalesforce OAuth PKCE Enhancement
Sequence Diagram(s)sequenceDiagram
participant Client
participant SalesforceHandler
participant SalesforceAuthZ as Salesforce AuthZ
participant SalesforceAPI as Salesforce API
Client->>SalesforceHandler: Initiate login
SalesforceHandler->>SalesforceHandler: Generate PKCE verifier & code_challenge
SalesforceHandler->>Client: Redirect to Salesforce AuthZ (includes code_challenge)
Client->>SalesforceAuthZ: Authorize
SalesforceAuthZ-->>Client: Authorization code
Client->>SalesforceHandler: Callback with code
SalesforceHandler->>SalesforceAPI: Exchange code + code_verifier
SalesforceAPI-->>SalesforceHandler: Access token
SalesforceHandler->>SalesforceAPI: GET config.userURL
SalesforceAPI-->>SalesforceHandler: User info
SalesforceHandler-->>Client: Session created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
phof
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/server/lib/oauth/salesforce.ts (2)
98-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
authorizationParamsfrom overriding generatedstateand PKCE values.Because
...config.authorizationParamsis spread last here, a consumer can clobberstate,code_challenge, orcode_challenge_method. That either weakens the CSRF/PKCE guarantees or causes the latercode_verifierexchange to fail.Suggested fix
return sendRedirect( event, withQuery(config.authorizationURL as string, { + ...config.authorizationParams, response_type: 'code', client_id: config.clientId, redirect_uri: redirectURL, scope: config.scope.join(' '), state, code_challenge: pkce.code_challenge, code_challenge_method: pkce.code_challenge_method, - ...config.authorizationParams, }), )🤖 Prompt for 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. In `@src/runtime/server/lib/oauth/salesforce.ts` around lines 98 - 106, The authorizationParams spread currently comes last in the withQuery call and can override the generated state and PKCE values; update the call in withQuery so that config.authorizationParams is either spread before the explicit keys (response_type, client_id, redirect_uri, scope, state, code_challenge, code_challenge_method) or filter out keys ["state","code_challenge","code_challenge_method"] from config.authorizationParams before spreading, ensuring the generated state and pkce (pkce.code_challenge, pkce.code_challenge_method) remain authoritative and that the subsequent code_verifier exchange will succeed.
65-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive the default endpoints from the merged
baseURL.Line 66 only reads
config?.baseURL, so deployments that setoauth.salesforce.baseURLvia runtime config still defaultauthorizationURL,tokenURL, and the newuserURLtohttps://login.salesforce.com. That breaks My Domain setups unless callers redundantly override every URL.Suggested fix
const runtimeConfig = useRuntimeConfig(event).oauth?.salesforce - const baseURL = config?.baseURL || 'https://login.salesforce.com' + const baseURL = config?.baseURL || runtimeConfig?.baseURL || 'https://login.salesforce.com' config = defu(config, runtimeConfig, { authorizationURL: `${baseURL}/services/oauth2/authorize`, tokenURL: `${baseURL}/services/oauth2/token`, userURL: `${baseURL}/services/oauth2/userinfo`, authorizationParams: {}, }) as OAuthSalesforceConfig🤖 Prompt for 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. In `@src/runtime/server/lib/oauth/salesforce.ts` around lines 65 - 72, The current code uses only config?.baseURL to build default endpoints so runtime-provided oauth.salesforce.baseURL is ignored; fix by first merging config and runtimeConfig (ensuring a default baseURL of 'https://login.salesforce.com') and then derive authorizationURL, tokenURL and userURL from the merged baseURL. Concretely: use useRuntimeConfig to get runtimeConfig, defu(config, runtimeConfig, { baseURL: 'https://login.salesforce.com' }) to produce a merged config, read baseURL from that merged config, then defu again to set authorizationURL, tokenURL, userURL and authorizationParams using `${baseURL}/services/oauth2/...`; keep types like OAuthSalesforceConfig unchanged.
🤖 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.
Outside diff comments:
In `@src/runtime/server/lib/oauth/salesforce.ts`:
- Around line 98-106: The authorizationParams spread currently comes last in the
withQuery call and can override the generated state and PKCE values; update the
call in withQuery so that config.authorizationParams is either spread before the
explicit keys (response_type, client_id, redirect_uri, scope, state,
code_challenge, code_challenge_method) or filter out keys
["state","code_challenge","code_challenge_method"] from
config.authorizationParams before spreading, ensuring the generated state and
pkce (pkce.code_challenge, pkce.code_challenge_method) remain authoritative and
that the subsequent code_verifier exchange will succeed.
- Around line 65-72: The current code uses only config?.baseURL to build default
endpoints so runtime-provided oauth.salesforce.baseURL is ignored; fix by first
merging config and runtimeConfig (ensuring a default baseURL of
'https://login.salesforce.com') and then derive authorizationURL, tokenURL and
userURL from the merged baseURL. Concretely: use useRuntimeConfig to get
runtimeConfig, defu(config, runtimeConfig, { baseURL:
'https://login.salesforce.com' }) to produce a merged config, read baseURL from
that merged config, then defu again to set authorizationURL, tokenURL, userURL
and authorizationParams using `${baseURL}/services/oauth2/...`; keep types like
OAuthSalesforceConfig unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b50a37b5-ab10-4189-bfc9-03ba6d4cbb27
📒 Files selected for processing (1)
src/runtime/server/lib/oauth/salesforce.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/server/lib/oauth/salesforce.ts (1)
98-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let
authorizationParamsoverride core OAuth params.
authorizationParamscan still replaceclient_id,redirect_uri,response_type, orscope. Ifredirect_uriorclient_idis overridden here, the authorize request and the token exchange will use different values, and the code exchange will fail.Suggested fix
return sendRedirect( event, withQuery(config.authorizationURL as string, { + ...config.authorizationParams, response_type: 'code', client_id: config.clientId, redirect_uri: redirectURL, scope: config.scope.join(' '), - ...config.authorizationParams, code_challenge: pkce.code_challenge, code_challenge_method: pkce.code_challenge_method, state, }), )🤖 Prompt for 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. In `@src/runtime/server/lib/oauth/salesforce.ts` around lines 98 - 107, The authorize URL construction using withQuery currently spreads config.authorizationParams after the core params, allowing authorizationParams to override critical OAuth fields (response_type, client_id, redirect_uri, scope, code_challenge, code_challenge_method, state); change the order so core params (response_type, client_id, redirect_uri, scope, code_challenge, code_challenge_method, state, and any other required fields) are spread last or explicitly override any conflicting keys from config.authorizationParams to ensure withQuery and the function that performs the token exchange use the same client_id and redirect_uri; update the call site in the salesforce OAuth flow (the withQuery invocation) to either filter out these keys from config.authorizationParams or apply config.authorizationParams first and then apply the core params to guarantee they cannot be replaced.
🤖 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 `@src/runtime/server/lib/oauth/salesforce.ts`:
- Around line 42-46: The JSDoc for the userURL option is incorrect: update the
comment on userURL to state that it is derived from the resolved baseURL (not a
fixed https://login.salesforce.com value) and that it will follow any My Domain
or baseURL override; mention the relationship to baseURL and where the
resolution occurs (the code that computes userURL from baseURL) so readers know
overriding baseURL changes userURL accordingly.
---
Outside diff comments:
In `@src/runtime/server/lib/oauth/salesforce.ts`:
- Around line 98-107: The authorize URL construction using withQuery currently
spreads config.authorizationParams after the core params, allowing
authorizationParams to override critical OAuth fields (response_type, client_id,
redirect_uri, scope, code_challenge, code_challenge_method, state); change the
order so core params (response_type, client_id, redirect_uri, scope,
code_challenge, code_challenge_method, state, and any other required fields) are
spread last or explicitly override any conflicting keys from
config.authorizationParams to ensure withQuery and the function that performs
the token exchange use the same client_id and redirect_uri; update the call site
in the salesforce OAuth flow (the withQuery invocation) to either filter out
these keys from config.authorizationParams or apply config.authorizationParams
first and then apply the core params to guarantee they cannot be replaced.
🪄 Autofix (Beta)
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: f5dc8a9d-0b40-4a69-a5d0-cc9205de92d3
📒 Files selected for processing (1)
src/runtime/server/lib/oauth/salesforce.ts
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 by CodeRabbit