fix: drop incorrect redirect_uri from Basecamp3 token refresh#29670
Open
apoorva-01 wants to merge 2 commits into
Open
fix: drop incorrect redirect_uri from Basecamp3 token refresh#29670apoorva-01 wants to merge 2 commits into
apoorva-01 wants to merge 2 commits into
Conversation
The Basecamp3 refresh request appended redirect_uri set to the app root URL (WEBAPP_URL), which differs from the callback URL used during the initial OAuth flow. Basecamp's token endpoint only expects type, refresh_token, client_id and client_secret on a refresh, so sending an unexpected redirect_uri can make the refresh fail once the access token expires. Send only the documented refresh parameters and add a unit test around refreshAccessToken covering the request shape.
Contributor
|
Welcome to Cal.diy, @apoorva-01! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
Contributor
📝 WalkthroughWalkthroughThe Basecamp3 helper's token refresh logic no longer includes a Changes
Sequence Diagram(s)sequenceDiagram
participant refreshAccessToken
participant BasecampOAuth
participant Prisma
refreshAccessToken->>BasecampOAuth: fetch token endpoint with refresh_token, client_id, client_secret
BasecampOAuth-->>refreshAccessToken: new access_token
refreshAccessToken->>Prisma: credential.update with new key
Related PRs: None identified. Suggested labels: app-store, tests, bugfix Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-store/basecamp3/lib/helpers.test.ts (1)
69-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact fetch contract, not just substrings.
These checks still pass if another unexpected query param gets appended later. Parsing the URL and asserting the full
searchParams(and request options) would pin the regression much more tightly.Possible tightening
expect(fetchMock).toHaveBeenCalledOnce(); - const requestedUrl = fetchMock.mock.calls[0][0] as string; + const [requestedUrl, requestInit] = fetchMock.mock.calls[0]; + const url = new URL(requestedUrl as string); - expect(requestedUrl).toContain("https://launchpad.37signals.com/authorization/token"); - expect(requestedUrl).toContain("type=refresh"); - expect(requestedUrl).toContain(`refresh_token=${basecampKey.refresh_token}`); - expect(requestedUrl).toContain("client_id=test-client-id"); - expect(requestedUrl).toContain("client_secret=test-client-secret"); - expect(requestedUrl).not.toContain("redirect_uri"); + expect(url.origin + url.pathname).toBe("https://launchpad.37signals.com/authorization/token"); + expect(Object.fromEntries(url.searchParams.entries())).toEqual({ + type: "refresh", + refresh_token: basecampKey.refresh_token, + client_id: "test-client-id", + client_secret: "test-client-secret", + }); + expect(requestInit).toMatchObject({ + method: "POST", + headers: { "User-Agent": "Cal.com (test@example.com)" }, + });🤖 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 `@packages/app-store/basecamp3/lib/helpers.test.ts` around lines 69 - 77, The token refresh test in helpers.test.ts is too loose because it only checks URL substrings, so unexpected query params or request options could slip through. Update the assertion around fetchMock/requestedUrl to parse the called URL from the helpers test and verify the full searchParams exactly for the refresh flow, using the relevant token-refresh helper path and request setup. Also assert the request options contract (method/body/headers as applicable) so the fetch call from the refresh-token logic is pinned down by the helper’s expected behavior.
🤖 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/app-store/basecamp3/lib/helpers.ts`:
- Around line 9-10: The refresh request in the Basecamp 3 helper is building the
token URL by string interpolation, which can break when credential values
contain reserved characters. Update the fetch call in helpers.ts to construct
the query string with URLSearchParams for the refresh flow, using the existing
refresh_token, client_id, and client_secret values so the request is encoded
correctly regardless of credential contents.
---
Nitpick comments:
In `@packages/app-store/basecamp3/lib/helpers.test.ts`:
- Around line 69-77: The token refresh test in helpers.test.ts is too loose
because it only checks URL substrings, so unexpected query params or request
options could slip through. Update the assertion around fetchMock/requestedUrl
to parse the called URL from the helpers test and verify the full searchParams
exactly for the refresh flow, using the relevant token-refresh helper path and
request setup. Also assert the request options contract (method/body/headers as
applicable) so the fetch call from the refresh-token logic is pinned down by the
helper’s expected behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df013015-94af-46f6-b117-db89df15d8a1
📒 Files selected for processing (2)
packages/app-store/basecamp3/lib/helpers.test.tspackages/app-store/basecamp3/lib/helpers.ts
Comment on lines
9
to
+10
| const tokenInfo = await fetch( | ||
| `https://launchpad.37signals.com/authorization/token?type=refresh&refresh_token=${credentialKey.refresh_token}&client_id=${clientId}&redirect_uri=${WEBAPP_URL}&client_secret=${clientSecret}`, | ||
| `https://launchpad.37signals.com/authorization/token?type=refresh&refresh_token=${credentialKey.refresh_token}&client_id=${clientId}&client_secret=${clientSecret}`, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Encode the refresh params instead of interpolating them into the URL.
Line 10 builds the query string by hand, so valid refresh_token / client_secret values containing +, =, or & will be parsed incorrectly and the refresh can still fail. Use URLSearchParams here so the request shape stays correct for every credential.
Proposed fix
- const tokenInfo = await fetch(
- `https://launchpad.37signals.com/authorization/token?type=refresh&refresh_token=${credentialKey.refresh_token}&client_id=${clientId}&client_secret=${clientSecret}`,
+ const params = new URLSearchParams({
+ type: "refresh",
+ refresh_token: credentialKey.refresh_token,
+ client_id: clientId,
+ client_secret: clientSecret,
+ });
+ const tokenInfo = await fetch(
+ `https://launchpad.37signals.com/authorization/token?${params.toString()}`,
{ method: "POST", headers: { "User-Agent": userAgent } }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const tokenInfo = await fetch( | |
| `https://launchpad.37signals.com/authorization/token?type=refresh&refresh_token=${credentialKey.refresh_token}&client_id=${clientId}&redirect_uri=${WEBAPP_URL}&client_secret=${clientSecret}`, | |
| `https://launchpad.37signals.com/authorization/token?type=refresh&refresh_token=${credentialKey.refresh_token}&client_id=${clientId}&client_secret=${clientSecret}`, | |
| const params = new URLSearchParams({ | |
| type: "refresh", | |
| refresh_token: credentialKey.refresh_token, | |
| client_id: clientId, | |
| client_secret: clientSecret, | |
| }); | |
| const tokenInfo = await fetch( | |
| `https://launchpad.37signals.com/authorization/token?${params.toString()}`, |
🤖 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 `@packages/app-store/basecamp3/lib/helpers.ts` around lines 9 - 10, The refresh
request in the Basecamp 3 helper is building the token URL by string
interpolation, which can break when credential values contain reserved
characters. Update the fetch call in helpers.ts to construct the query string
with URLSearchParams for the refresh flow, using the existing refresh_token,
client_id, and client_secret values so the request is encoded correctly
regardless of credential contents.
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.
What does this PR do?
The Basecamp3 token-refresh request appended a
redirect_uriparameter set to the app root URL (WEBAPP_URL). That's not the/api/integrations/basecamp3/callbackURL used during the initial OAuth authorization, and Basecamp's documented refresh request only expectstype=refresh,refresh_token,client_idandclient_secret. Sending an unexpectedredirect_urican make the refresh fail once the stored access token expires.This drops the
redirect_uriparameter (and the now-unusedWEBAPP_URLimport) so the refresh request matches Basecamp's documented parameters. The initial OAuth exchange, which legitimately uses the callbackredirect_uri, is left alone. Reference: https://github.com/basecamp/api/blob/master/sections/authentication.mdVisual Demo (For contributors especially)
N/A. This is a backend-only change to the Basecamp3 token-refresh helper, so there's nothing visual to show.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
No env vars or test data needed. The helper is unit-tested in isolation.
The new
helpers.test.tsmocksgetBasecampKeys,fetch, and Prisma, then asserts the refresh request:https://launchpad.37signals.com/authorization/tokentype=refresh,refresh_token,client_id,client_secretredirect_uriRestore the
redirect_uriline and the first test fails on theredirect_uriassertion; with the fix it passes.Checklist
I've read the contributing guide, the code follows the style guidelines, and there are no new warnings. This is a small PR (2 files).