trevato-apps/forge
5
0
Fork 4
Code Issues Pull requests 6 Releases 498 Packages 1 Wiki Activity Actions 6

The connect flow page goes on only to the authorize URL stored on the flow, so a crafted link cannot choose the client or the callback #69

Open
tdobbertin-espo wants to merge 1 commit from tdobbertin-espo/forge:fix/connect-flow-stored-target into main
pull from: tdobbertin-espo/forge:fix/connect-flow-stored-target
merge into: trevato-apps:main
Conversation 0 Commits 1 Files changed 6 +495 -21
tdobbertin-espo commented 2026-10-23 19:10:19 +00:00
First-time contributor
Copy link

Security. The connect flow page (/settings/origins/flows/:id) took its authorize URL from its own query string (?to=). It checked only two things: that the URL was on the instance's origin, and that it had a PKCE challenge. So a crafted link to a forge's flow page could choose another OAuth application's client id, with that application's callback, on the same instance. The page then offered it as "Continue to the instance". A person who trusted the forge's page could approve an application that the forge did not start. The flow row already seals the authorize URL (target_enc, for the short link), so the query string was never necessary.

Now flowTarget (src/origins/oauth.ts) opens the URL sealed on a pending flow, and the page links to that URL and to no other. The page checks the URL again first: the instance's own /login/oauth/authorize, the flow's client_id and redirect_uri, and a code_challenge. Starting a flow redirects to the bare flow page, with then=migrate when it started on the migrate page. A to that an older link still has must be exactly the stored URL. Any other to gets "This connect cannot be continued from here". A pending flow past its ten minutes says "This connect link expired" and shows a fresh Connect for the same host. That button keeps then=migrate.

The device flow, the short link (/go/:code), the poll and the callback are unchanged. Nothing new is stored, and there is no migration.

Tests

bun 1.3.13, a scrubbed environment (no agent variables), Chromium 151:

  • test/origin-flow-page.test.ts: 5 pass, 0 fail. One of them is new here: a connect started from migrate keeps then=migrate and no to, and its expired page starts again from migrate.
  • test/oauth-connect-browser.test.ts (FORGE_CHROMIUM set, so it ran and did not skip): 1 pass, 0 fail. It clicks Authorize and Continue and checks that the consent screen is asked for with no CSP refusal.
  • With the origin, OAuth, migrate and cross suites, in one bun test --timeout 60000 call: 432 pass, 0 fail across 33 files.
  • bun scripts/gate.ts and tsc --noEmit: clean.
the 33 files

origin-flow-page, oauth-connect-browser, origins, origins-oauth, origins-operator, origins-reconnect, origins-security-auth, origins-security-provider, origins-security-push, origins-sync, origins-sync-settled, web-origins, web-origin-link, mcp-origins, mcp-origin-apps, push-through, trusted-hosts, trusted-hosts-ui, steps-lists, no-polling, refresh-meta, mirror-outbound, mirror-out, carried, migrate, migrate-after, tui-migrate, apps-feedback-mirror, and the five cross files reserved-names, studio, shards, check-list, hollow (each test/<name>.test.ts).

🤖 Generated with Claude Code

**Security.** The connect flow page (`/settings/origins/flows/:id`) took its authorize URL from its own query string (`?to=`). It checked only two things: that the URL was on the instance's origin, and that it had a PKCE challenge. So a crafted link to a forge's flow page could choose another OAuth application's client id, with that application's callback, on the same instance. The page then offered it as "Continue to the instance". A person who trusted the forge's page could approve an application that the forge did not start. The flow row already seals the authorize URL (`target_enc`, for the short link), so the query string was never necessary. Now `flowTarget` (`src/origins/oauth.ts`) opens the URL sealed on a pending flow, and the page links to that URL and to no other. The page checks the URL again first: the instance's own `/login/oauth/authorize`, the flow's `client_id` and `redirect_uri`, and a `code_challenge`. Starting a flow redirects to the bare flow page, with `then=migrate` when it started on the migrate page. A `to` that an older link still has must be exactly the stored URL. Any other `to` gets "This connect cannot be continued from here". A pending flow past its ten minutes says "This connect link expired" and shows a fresh **Connect** for the same host. That button keeps `then=migrate`. The device flow, the short link (`/go/:code`), the poll and the callback are unchanged. Nothing new is stored, and there is no migration. ### Tests bun 1.3.13, a scrubbed environment (no agent variables), Chromium 151: - `test/origin-flow-page.test.ts`: **5 pass, 0 fail**. One of them is new here: a connect started from migrate keeps `then=migrate` and no `to`, and its expired page starts again from migrate. - `test/oauth-connect-browser.test.ts` (`FORGE_CHROMIUM` set, so it ran and did not skip): **1 pass, 0 fail**. It clicks Authorize and Continue and checks that the consent screen is asked for with no CSP refusal. - With the origin, OAuth, migrate and cross suites, in one `bun test --timeout 60000` call: **432 pass, 0 fail across 33 files**. - `bun scripts/gate.ts` and `tsc --noEmit`: clean. <details><summary>the 33 files</summary> origin-flow-page, oauth-connect-browser, origins, origins-oauth, origins-operator, origins-reconnect, origins-security-auth, origins-security-provider, origins-security-push, origins-sync, origins-sync-settled, web-origins, web-origin-link, mcp-origins, mcp-origin-apps, push-through, trusted-hosts, trusted-hosts-ui, steps-lists, no-polling, refresh-meta, mirror-outbound, mirror-out, carried, migrate, migrate-after, tui-migrate, apps-feedback-mirror, and the five cross files reserved-names, studio, shards, check-list, hollow (each `test/<name>.test.ts`). </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
The page at /settings/origins/flows/:id took its authorize URL from its
own query string (`?to=`). It checked only that the URL was on the
instance's origin and had a PKCE challenge. So a crafted link could
choose another application's client id, or another callback, on the
same instance, and the page offered it as "Continue". The flow row
already seals that URL (`target_enc`, for the short link), so the query
string was never necessary.

Now `flowTarget` opens the URL sealed on a pending flow, and the page
links to that URL and to no other. It checks the URL again first: the
instance's own `/login/oauth/authorize`, the flow's `client_id` and
`redirect_uri`, and a `code_challenge`. Starting a flow redirects to
the bare flow page (with `then=migrate` when it started there). A `to`
that an older link still has must be exactly the stored URL. A pending
flow past its time says "This connect link expired" and shows a fresh
Connect, which keeps `then=migrate`.

The device flow, the short link and the callback are unchanged.

test/origin-flow-page.test.ts (5 tests). test/oauth-connect-browser.test.ts
clicks Authorize and Continue in headless Chromium and checks that no CSP
refusal occurs; it skips, and says so, where no Chromium is found. It uses
a small addition to test/helpers/chrome.ts (event listeners,
`clickAndFollow`).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Some checks are pending
preview / preview (pull_request) Blocked by required conditions
Details
This pull request has changes conflicting with the target branch.
  • src/web/pages/origins.ts
  • test/helpers/chrome.ts
  • test/oauth-connect-browser.test.ts
  • test/origin-flow-page.test.ts
Some workflows are waiting to be reviewed.
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fix/connect-flow-stored-target:tdobbertin-espo-fix/connect-flow-stored-target
git switch tdobbertin-espo-fix/connect-flow-stored-target

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff tdobbertin-espo-fix/connect-flow-stored-target
git switch tdobbertin-espo-fix/connect-flow-stored-target
git rebase main
git switch main
git merge --ff-only tdobbertin-espo-fix/connect-flow-stored-target
git switch tdobbertin-espo-fix/connect-flow-stored-target
git rebase main
git switch main
git merge --no-ff tdobbertin-espo-fix/connect-flow-stored-target
git switch main
git merge --squash tdobbertin-espo-fix/connect-flow-stored-target
git switch main
git merge --ff-only tdobbertin-espo-fix/connect-flow-stored-target
git switch main
git merge tdobbertin-espo-fix/connect-flow-stored-target
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
agent-work
Queue this for an agent: it will build it and open a PR

  
agent-working
An agent is actively working on this

  
ultracode
Operators: run a Fable ultracode orchestrator on this issue

No labels
agent-work
agent-working
ultracode
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
trevato-apps/forge!69
No description provided.