fix(desktop): stop the OAuth connect callback from failing on a bare-path callback URL - #7005
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Separately, Reviewed by Cursor Bugbot for commit e57f284. Configure here. |
…path callback URL The desktop connect launcher passed better-auth a same-origin path as its callbackURL. Better Auth stores that value verbatim in the OAuth state, and the callback's credential-draft reader parsed it with a bare `new URL()`, which rejects a path. That throw happened inside the `account.create.before` database hook, which better-auth's OAuth callback does not guard, so the provider redirect landed on a 500 after authorization had already succeeded. Send an absolute URL from the connect page, matching the workspace-scoped branch and every other connect surface, and accept a path-absolute callback URL in the draft reader so the shape can never fail the callback again. Protocol-relative and malformed values still throw, keeping an unreadable binding loud. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
94fdf84 to
fbd094d
Compare
|
bugbot run |
Greptile SummaryThe PR fixes the desktop OAuth connection callback by passing an absolute completion URL and accepting path-absolute callback state while rejecting malformed and protocol-relative values.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/desktop/connect/page.tsx | Both desktop connection branches now compose a root-scoped absolute completion URL without duplicate slashes. |
| apps/sim/app/desktop/connect/connect-launcher.tsx | The launcher consistently passes the absolute completion URL as both its success and error callback. |
| apps/sim/lib/core/utils/urls.ts | Public and internal base URLs now match the no-trailing-slash form assumed by existing URL-concatenation callers. |
| apps/sim/lib/credentials/draft-processor.ts | Credential-draft parsing now accepts documented path-absolute callbacks while continuing to reject protocol-relative and malformed values. |
| apps/sim/app/desktop/connect/page.test.tsx | New tests cover launcher and workspace callback composition, including trailing-slash base configuration. |
| apps/sim/lib/credentials/draft-processor.test.ts | Tests pin valid relative callback parsing and rejection of protocol-relative input. |
| packages/testing/src/mocks/urls.mock.ts | The shared URL mock mirrors production trailing-slash normalization. |
Sequence Diagram
sequenceDiagram
participant Desktop as Desktop app
participant Page as Connect page
participant Auth as OAuth service
participant Provider as OAuth provider
participant Complete as Completion page
Desktop->>Page: Open connect request
Page->>Auth: Start link with absolute completion URL
Auth->>Provider: Authorize account
Provider-->>Auth: OAuth callback
Auth->>Auth: Read optional credential draft binding
Auth-->>Complete: Redirect to completion URL
Complete-->>Desktop: Forward result to loopback
Reviews (5): Last reviewed commit: "chore(urls): stop claiming path-prefixed..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fbd094d. Configure here.
Concatenating `getBaseUrl()` with the completion path leaves the result dependent on how the deployment spelled `NEXT_PUBLIC_APP_URL`: the helper only adds a missing protocol, so a trailing slash produced `//desktop/connect/complete`, a pathname that matches no route. The completion page is what bounces the OAuth result to the desktop app's loopback, so that typo would have stranded the flow just past the callback it was meant to fix. Both callback URLs in the page — the launcher's and the workspace-scoped authorize redirect's — now go through one helper that resolves the path against the base with `new URL`, matching how the same function already builds the authorize URL, with coverage for a trailing-slash base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
… assume
`getBaseUrl()` returned `NEXT_PUBLIC_APP_URL` as the operator spelled it, while
almost every consumer builds `${base}/path`. A base configured with a trailing
slash therefore produced a `//path` pathname that matches no route, and broke
the `startsWith(`${base}/`)` prefix checks that decide whether a redirect target
is our own — the OAuth authorize route rejected its own completion callback and
fell back to the workspace page, so the desktop handoff never ran on those
deployments. The previous commit fixed one such URL; this fixes the reason it
was wrong, for the ~30 concatenation sites that share the assumption.
`normalizeBaseUrl` now strips trailing slashes alongside the protocol it already
added, which is the invariant SITE_URL has always documented. A path-prefixed
base keeps its path. `getInternalApiBaseUrl` gets the same treatment, since its
callers concatenate identically.
`@sim/testing`'s urls mock is a hand-written mirror of this module, so it moves
in step. `internal-api-base-url.test.ts` now unmocks the module it names —
otherwise it asserts against that mirror and any drift between the two passes
unnoticed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5e8e10b. Configure here.
The previous commit's doc and test said a path-prefixed base keeps its path. That reads as support for a deployment shape the app does not have: there is no Next `basePath`, so routes are served at the origin root and such a value could not address them however the base were normalized. Every documented example is origin-only. Says only what is true — trailing slashes are the one spelling absorbed — and reframes the test as pinning the trim's shape rather than asserting a path-prefixed deployment works. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e57f284. Configure here.
2 participants
Summary
/api/auth/oauth2/callback/<provider>after the provider had already authorized — the browser landed on "This page isn't working" and no credential was created.callbackURL. Better Auth stores that value verbatim in the OAuth state, and the callback's credential-draft reader parsed it with a barenew URL(), which rejects a path. The throw happened inside theaccount.create.beforedatabase hook, which better-auth's OAuth callback does not guard, so it escaped as a 500 instead of a failed connection./api/auth/oauth2/authorizewith an absolute callback.Type of Change
Testing
app/desktop/connect/page.test.tsxpins the launcher's complete URL as absolute (with and without a credential draft) and keeps the workspace-scoped authorize redirect covered. Verified both new tests fail against the old code with the exactTypeError: Invalid URL.lib/credentials/draft-processor.test.tsfor the path-absolute callback URL, plus the protocol-relative value that must still throw.bun run test lib/credentials app/desktop lib/auth— 662 passing;bun run type-check;bun run lint;bun run check:audits(33/33, incl.check:api-validation:strictandcheck:client-boundary).Checklist