fix(vcs): link GitHub accounts that already have the app installed by HarshMN2345 · Pull Request #13880 · appwrite/appwrite

HarshMN2345 · 2026-09-24T11:54:48Z

What does this PR do?

Connecting GitHub from a project's settings failed with This installation was completed on GitHub, so it could not be connected to a project whenever the organization already had the Appwrite app installed, for example because it was already linked to another project.

GitHub sends an already-installed account to the installation's settings page. Saving there redirects to the callback with setup_action=update and without the state we passed to installations/new, so the callback can't tell which project started the flow.

Authorize also stores the signed state in a 10-minute HttpOnly, SameSite=Lax cookie on /v1/vcs/github/callback, one per project. The callback can only read the project through the console session, so the cookie gets the same reach as that session: the registrable domain when _APP_CONSOLE_ROOT_SESSION is enabled (Cloud), else the console host and its subdomains (which covers Cloud's regional hosts either way).
Callback uses a cookie only for setup_action=update with no state, GitHub returns nothing to tell pending connections apart by, so with several it keeps those the installation could be relinked into (see below) and uses a lone one; only when several still qualify does it refuse with an explicit message and clear them rather than guess. It still checks the signature, and a finished flow clears only its own project's cookie.
Callback, cookie path only: it relinks an installation only if it's already linked to a project the signed-in user can read, or to any project in the target project's organization. That's the case GitHub drops state for. The cookie is sent on any top-level navigation, so without this rule a page on another site could attach its own installation to a signed-in user's project. The organization lookup skips authorization because members scoped to one project can't read their sibling projects; it only compares organizations and loads nothing but projectId.
Callback, every path: it links an installation only when the GitHub user behind the returned OAuth code can access it (GET /user/installations). Before this, installation_id was trusted as sent next to any valid state.

Limits

/user/installations proves the GitHub user can access the installation (at least one of its repositories), not that they own or administer it. That's GitHub's documented check, and much stricter than before.
The cookie holds only the most recent flow in a browser. If two connect flows overlap, an update links to the later project.
On the cookie path, anyone with GitHub access to an installation already linked in the organization can get it linked to another project there, for example through a member who starts a connect flow. Nothing new is exposed: the organization already trusts that installation.
The organization lookup reads at most 500 linked projects. A GitHub installation linked to more than 500 projects across Appwrite could be refused, never wrongly accepted.
With the API on a sibling host of the console (e.g. api.example.com and console.example.com), the fallback needs _APP_CONSOLE_ROOT_SESSION, like the console session itself: without it the session never reaches the callback, so its project lookup fails with or without the cookie.
If connections for several projects are pending in one browser and more than one of them could take the installation, an update without state is refused once ("start again from the project you want"); the retry then finds a single pending connection. Nothing is linked to a guessed project.
Members of orgs that enforce SAML SSO need an active SSO session. The refusal message says so.
Failures redirect to the console's failure URL with ?error=. Neither console shows that parameter yet: the Svelte console also reuses the success alert in its failure URL. That's a follow-up in the console repos.

Self-hosted: linking now requires the GitHub App's Request user authorization (OAuth) during installation setting, which the self-hosting docs already require.

Test Plan

New credential-free suite VCSGitHubCallbackConsoleClientTest (runs in the existing VCSGitHub CI job; the VCSGitHubBase suites skip there without a GitHub App): missing state, owner-approval request, installation without a code, cookie recovery and clearing, another project's pending cookie kept, two pending projects neither of which fits refused, tampered cookie, cookie ignored on a non-update callback, unlinked installation refused from the cookie. Tests take state and the cookie from the real authorize route; the VCSGitHub CI job sets _APP_VCS_GITHUB_APP_NAME so authorize can build GitHub's URL. Nothing contacts GitHub, and GitHub stays unconfigured.

Also ran against an API container built from this branch, with a console session:

authorize returns the 301 to GitHub plus Set-Cookie: a_github_state=…; Max-Age=600; path=/v1/vcs/github/callback; domain=.<console host>; HttpOnly; SameSite=Lax.
Callback with setup_action=update, the cookie and no state:
- installation not linked to any project the user can access → redirects with the refusal, clears the cookie;
- installation linked in another organization the user can't read → refused;
- installation linked in another organization whose project the user can read → passes and continues to the GitHub token exchange;
- installation linked in the user's organization, on a project the user can't read → passes and continues to the GitHub token exchange;
- tampered cookie → 400 Invalid state parameter.
Callback with setup_action=install, the cookie and no state → the cookie is ignored and left alone (400, as before).
Callback with setup_action=request and no state → 400 with the owner-approval message.
Two pending connections: only one whose organization already uses the installation → that one is picked (continues to the token exchange) and only its cookie is cleared; both qualify → 400, both cleared; linked nowhere → 400.
authorize with _APP_CONSOLE_ROOT_SESSION enabled and console host console.example.com → Domain=.example.com.
Callback with an explicit state and the cookie, but no code → refused, cookie left alone.
Pint and PHPStan pass on the changed files.

Not exercised: a successful link with a real GitHub OAuth code. That needs a live GitHub App installation.

Related PRs and Issues

None.

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

The signed state proves which project started the flow, not which installation it ended on. installation_id is a plain query parameter, so a caller holding a valid state for their own project could name any other account's installation of the app and attach its repositories to that project. The callback now requires the OAuth code GitHub returns during installation, and links an installation only when GET /user/installations shows the GitHub user behind that code can access it.

When the app is already installed on the account a user picks, GitHub sends them to that installation's settings, and saving there redirects with setup_action=update and no state. The callback stopped on the missing state, so an account with the app installed could never be connected to another project. Authorize now mirrors the signed state into a ten-minute HttpOnly cookie scoped to the callback path on the console host, since Authorize may be served from a regional host while GitHub returns to the console one. The callback falls back to it when state is missing, still verifies its signature, and clears it after one use. The installation access check makes it safe to decide the project from the cookie.

greptile-apps · 2026-09-24T12:00:04Z

Confidence Score: 5/5

[Critical risk] Modifies GitHub OAuth flow and state handling for VCS integration.

The PR appears safe to merge, though the callback tests leave two useful regression checks incomplete.

Findings

Tampering test misses signature failures ▶
Ambiguous callback test misses clearing ▶

Fix with agent prompt

### Issue 1
tests/e2e/Services/VCSGitHub/VCSGitHubConsoleClientTest.php:1139
Appending `x` makes the cookie value invalid JSON. The callback then has no project ID, so this test still gets HTTP 400 even if signature verification is removed. It no longer checks that a well-formed state with an altered signed field is rejected, allowing a signature-check regression to pass the suite.

### Issue 2
tests/e2e/Services/VCSGitHub/VCSGitHubCallbackConsoleClientTest.php:undefined-145
When two projects have pending connections, this test checks only for HTTP 400-the same result as a callback with no state. It would pass if the callback stopped clearing the pending cookies, leaving the next connection attempt blocked. Please test the observable cookie-clearing and retry behavior.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR adds a short-lived state-cookie fallback for GitHub installation updates that omit state, checks access to the installation before linking it, and adds callback tests. The latest change replaces a format-coupled tampering fixture, but no longer checks rejection of a well-formed state with an altered signed field.

_{Reviews (12) · Last reviewed commit: "test(vcs): tamper with the GitHub state ..."}

github-actions · 2026-09-24T12:07:18Z

✨ Benchmark results

Comparing main (before) → fix/vcs-github-existing-installation (after).

Metric	Before	After	Change
🚀 Requests/sec	`161.48`	`189.9`	🟢 `+17.6%`
⏱️ Latency P50	`106.79 ms`	`89.93 ms`	🟢 `-15.8%`
⏱️ Latency P95	`249.1 ms`	`213.9 ms`	🟢 `-14.1%`

Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario	P50 (ms)	P95 (ms)	Requests	RPS	Δ P95 (ms)
API total	`89.93`	`213.9`	`12,141`	`189.9`	`-35.2`
Account	`179.23`	`338.69`	`639`	`10.61`	`-44.37`
TablesDB	`87.31`	`170.81`	`6,603`	`106.5`	`-32.27`
Storage	`82.5`	`178.06`	`3,195`	`52.92`	`-34.69`
Functions	`129.29`	`257.4`	`1,704`	`28.85`	`-41.92`

Top API waits (after)

API request	Max wait (ms)
account.name.update	`528.94`
account.prefs.update	`486.03`
functions.delete	`379.45`
functions.create	`373.09`
functions.runtimes.list	`372.11`

The cookie is sent on any top-level navigation to the callback, so a page on another site could pair a signed-in user's pending state with its own code and installation_id and attach that installation to their project. The access check cannot catch this, because the attacker's code does grant access to the attacker's installation. From the cookie, the callback now only relinks an installation this user can already see linked to another project, which is the case GitHub drops state for. It falls back to the cookie only on setup_action=update and clears it only when it was used, so a callback carrying its own state no longer consumes another flow's cookie. The access refusal mentions SAML single sign-on, and the cookie constants are named for GitHub, the only provider that uses them.

…tion' into fix/vcs-github-existing-installation

The cookie path read installations with the caller's permissions. Members scoped to one project cannot read installations linked to its sibling projects, so they were refused even though the installation already belongs to their organization. The check now accepts an installation already linked to a project the user can read, or to any project in the target project's organization. The organization lookup skips authorization, since it only compares organizations, and loads just projectId from the installations. The refusal no longer tells users to uninstall the app, which would break the projects still using it, and a stateless approval request reports the owner-approval message again.

Authorize scoped the cookie to the console host, so a browser dropped it when the API was served from a sibling host such as api.example.com. The callback can only read the project through the console session, so the cookie now gets the same reach: the registrable domain with root sessions, else the console host and its subdomains. A second connection started in the same browser overwrote the first flow's cookie, so an update finishing the first flow could land in the second project. Authorize now clears a pending cookie that belongs to another project instead of replacing it, and the callback clears the cookie only when it belongs to the project whose flow just ended. The cookie path widens to /v1/vcs/github so Authorize can see it. The stateless error no longer claims the installation was completed on GitHub, which a restarted or overlapping flow also hits.

The GitHub callback does not extend the shared Callback/Base, so the Gitea suite never ran it, and every VCSGitHubBase suite skips without a GitHub App. This suite drives the real route with signed state and the state cookie: missing state, an owner-approval request, an installation without a code, cookie recovery and clearing, another project's pending cookie, a tampered cookie, a cookie on a non-update callback, and an unlinked installation from the cookie.

…tion' into fix/vcs-github-existing-installation

Clearing the state cookie when another project's connection was pending made the connection the user started last fail whenever GitHub returned without state. GitHub returns nothing the callback could tell two flows apart by, so Authorize now lets the later connection replace the pending one. The callback still clears only its own project's cookie, so a flow that carries its own state leaves another project's cookie alone. With Authorize no longer reading the cookie, its path narrows back to the callback.

The suite rebuilt the state payload and its signature itself, so a change to how Authorize encodes state would have meant changing the tests too. It now obtains state and the cookie from the real Authorize route. The VCSGitHub CI job sets an app name for that, which Authorize only uses to build GitHub's URL; nothing contacts GitHub and GitHub stays unconfigured.

…roject With one state cookie per browser, a second connection either replaced the first flow's state, so an update for the first project could land in the second, or cleared it, failing the connection started last. GitHub returns nothing the callback could tell the flows apart by. Authorize now keeps one cookie per project. A stateless update uses the pending cookie when there is exactly one; with several it refuses with an explicit message and clears them, so the next attempt finds a single one. A finished flow still clears only its own project's cookie.

The suite reads each project's state cookie from Authorize by name, and a stateless update carrying cookies for two projects must be refused instead of linked to either.

…copy The suite picked the state cookie by its name prefix and asserted error message wording, so renaming the cookie or rewording a message would have broken it while the flow still worked. It now takes whatever cookie Authorize sets and asserts status codes only; the approval-request case, which only checked wording, is dropped.

greptile-apps · 2026-09-25T06:48:00Z

+            'installation_id' => '1234567',
+        ], $this->authorizeHelper($this->getProject()['$id'])['cookie'], $this->authorizeHelper($this->getProject(true)['$id'])['cookie']);
+
+        $this->assertEquals(400, $response['headers']['status-code']);


Ambiguous callback test misses clearing When two projects have pending connections, this test checks only for HTTP 400—the same result as a callback with no state. It would pass if the callback stopped clearing the pending cookies, leaving the next connection attempt blocked. Please test the observable cookie-clearing and retry behavior.

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/e2e/Services/VCSGitHub/VCSGitHubCallbackConsoleClientTest.php Line: 145 Comment: **Ambiguous callback test misses clearing** When two projects have pending connections, this test checks only for HTTP 400—the same result as a callback with no state. It would pass if the callback stopped clearing the pending cookies, leaving the next connection attempt blocked. Please test the observable cookie-clearing and retry behavior. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Refusing every stateless update while several connections were pending meant an abandoned flow, still within its ten minutes, blocked a valid one. The installation already says which projects could take it: those in an organization that uses it, or any when this user can read a project that does. The callback now keeps the pending connections that qualify and uses a lone one, refusing and clearing only when several still qualify. The relink check reuses the same lookups.

VCSGitHubConsoleClientTest skipped every test when no GitHub App was configured, so the callback cases lived in a class of their own. The gate now sits where the suite actually reaches GitHub, in setupInstallation() and testGitHubAuthorize(), so the cases that need no GitHub App run in the existing suite and the separate class goes away.

The test rewrote a field inside the decoded cookie, so a change to how state is encoded would have broken it while tampering was still refused. It now alters the cookie as issued and expects the callback to reject it.

greptile-apps · 2026-09-25T07:36:55Z

+        $response = $this->callGitHubCallbackHelper([
+            'setup_action' => 'update',
+            'installation_id' => '1234567',
+        ], $this->authorizeHelper($this->getProject()['$id'])['cookie'] . 'x');


Tampering test misses signature failures Appending x makes the cookie value invalid JSON. The callback then has no project ID, so this test still gets HTTP 400 even if signature verification is removed. It no longer checks that a well-formed state with an altered signed field is rejected, allowing a signature-check regression to pass the suite.

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/e2e/Services/VCSGitHub/VCSGitHubConsoleClientTest.php Line: 1139 Comment: **Tampering test misses signature failures** Appending `x` makes the cookie value invalid JSON. The callback then has no project ID, so this test still gets HTTP 400 even if signature verification is removed. It no longer checks that a well-formed state with an altered signed field is rejected, allowing a signature-check regression to pass the suite. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

HarshMN2345 and others added 3 commits September 24, 2026 17:24

Merge branch 'main' into fix/vcs-github-existing-installation

e256cc2

greptile-apps Bot reviewed Sep 24, 2026

View reviewed changes

HarshMN2345 and others added 3 commits September 24, 2026 18:55

Merge branch 'main' into fix/vcs-github-existing-installation

e7aea1b

Merge remote-tracking branch 'origin/fix/vcs-github-existing-installa…

b5a79d7

…tion' into fix/vcs-github-existing-installation

greptile-apps Bot reviewed Sep 24, 2026

View reviewed changes

Comment thread src/Appwrite/Platform/Modules/VCS/Http/GitHub/Callback/Get.php Outdated

Comment thread src/Appwrite/Platform/Modules/VCS/Http/GitHub/Callback/Get.php Outdated

HarshMN2345 and others added 5 commits September 25, 2026 11:16

Merge branch 'main' into fix/vcs-github-existing-installation

5b57788

Merge remote-tracking branch 'origin/fix/vcs-github-existing-installa…

54e265c

…tion' into fix/vcs-github-existing-installation