[pull] main from appwrite:main - #219
Merged
Merged
Conversation
GitHub only echoes the state parameter back when the flow finishes through the redirect URI. An organisation member requesting owner approval, and an owner approving that request from GitHub's own settings, both finish on the app's setup URL instead, which carries no state. The callback then threw a bare 400 whose message blamed the user for being an organisation member -- the check is only empty($state) and never looks at the caller's GitHub role, so an owner hit the same page with no way forward. Authorize now mirrors state into a short-lived, one-shot cookie the callback falls back to. The error redirect also used a ':' separator instead of '?', so the console received /settings/git-installations:error=... and could never read the error back, and empty success/failure values in state overrode the computed console defaults, turning the remaining failure branch into a bare 400 as well. The unused setup_action param now distinguishes a submitted request from a plain permission failure.
/v1/vcs/github/callback is public and took the projectId and both redirect URLs straight out of the state parameter, so anyone could name the project an installation gets attached to, or point the redirect anywhere. The OAuth2 providers already guard this: Authorize/Base signs [projectId, success, failure] with _APP_OPENSSL_KEY_V1 and Callback/Base fails closed on hash_equals. The GitHub App flow predates that and was never brought across. Mirror the same signature into the state Authorize builds and verify it before the projectId is read. This also covers the fallback cookie, since the cookie carries the signed state verbatim.
The repository endpoints loaded an installation by id and never checked it
belongs to the requesting project. Authorization does not backstop this: on
admin-mode routes outside /v1/projects*, shared/api.php sets the default
authorization status to false, so getDocument('installations', $installationId)
returns another tenant's document.
Apply the same guard Installations/Get.php, Delete.php and Namespaces/XList.php
already use, so a mismatched project gets INSTALLATION_NOT_FOUND rather than
another tenant's repositories, file contents, branches or detections.
…attach The cookie is set for ten minutes with SameSite=Lax, so a top-level navigation from another site still sends it. Substituting it for missing state on a request that carries an installation id let that cookie supply the project for an installation it was never bound to; the signature covers the project and redirect URLs, not the installation. Fall back to the cookie only when there is no installation id, which is the organisation-member request it exists for. Callbacks that would persist an installation now require state on the request itself.
The cookie guessed that the browser hitting the callback was the one that started the flow, which SameSite=Lax does not guarantee: any top-level navigation from another site sent it. Once the callback refused to let it attach an installation, the only branch it could still reach was the error redirect, so all it bought was a nicer message on a flow that cannot succeed anyway -- an organisation member cannot install the app, an owner has to. It also never helped the case that matters. An owner approving a member's request from GitHub's settings is a different browser with no cookie, so that flow needs reconciliation from the installation webhook rather than anything carried in the request. Missing state now reports itself plainly in every case.
Test methods are verb-first everywhere else in tests/e2e, matching the
{restVerb}{Qualifier} rule the sibling testGetInstallation follows, and e2e
suites call builtins unprefixed rather than with a leading backslash.
The project scoping on the repository endpoints and the state signature are separate concerns from the misleading installation error, and each wants its own E2E coverage. Split out so this PR carries only the callback behaviour.
The cast came in with the fallback cookie and has no reader now that state is only appended to the redirect query.
The repository endpoints loaded an installation by id and never checked it
belongs to the requesting project. Authorization does not backstop this: on
admin-mode routes outside /v1/projects*, shared/api.php sets the default
authorization status to false, so getDocument('installations', $installationId)
returns another tenant's document.
Apply the same guard Installations/Get.php, Delete.php and Namespaces/XList.php
already use, so a mismatched project gets INSTALLATION_NOT_FOUND rather than
another tenant's repositories, file contents, branches or detections.
Two console users with their own teams, projects and Gitea accounts: each connects an installation, neither can use the other's through any of the six repository endpoints even knowing its id, and the probe by the other tenant's project id dies on team membership before reaching VCS code. Inviting the second user into the first team and accepting via the emailed secret makes the same project-scoped request succeed, while the installation still resolves only under its own project. The Gitea OAuth2 helpers move from VCSGiteaConsoleClientTest into VCSGiteaBase and take the project, headers and Gitea account as parameters, so suites can run the dance as any tenant. A second Gitea user is created through the admin API with must_change_password disabled, since the default would turn the authorize redirect into the change-password page.
…e suite The missing-state message now says what happened (the installation finished on GitHub, so it could not be linked to a project) and where to go, instead of a bare parameter complaint. The request message warns that approving on GitHub is not enough, since that route drops state and dead-ends the same way. The suite signs its state the way authorize does: harmless while the callback does not verify signatures, and keeps this file green once signing lands. The state validator gets headroom (4096) because the redirect URLs it carries are not length-limited, so a server-built state can outgrow 2048 on its own.
The fix in these branches is the separator character and the request-aware message; inverting the guards on top of that buried a one-character change in a rewritten block.
GitHub sends install, update or request; install and update always carry an installation_id, so this branch treats them and anything unrecognized as the permissions case.
Present-but-empty state, the request message from the match, the project-not-found redirect, and the computed default redirect when state carries empty URLs. States are built through one signer so redirects for missing projects and empty URLs stay verifiable once signing lands.
The multi-tenant tests move into VCSGiteaConsoleClientTest and the OAuth2 dance takes its project, headers and Gitea account as parameters in place, so the suite gains the coverage without relocating helpers or adding a class.
…des only The match now names GitHub's three documented values and gives anything else its own message instead of folding it into the permissions case. Tests assert status codes and redirect targets; error prose is not a contract.
The long-state case is the one that distinguishes the widened validator from the old one; the invalid-JSON case covers the null-guarded decode through to the project lookup; the unexpected setup_action selects the match's default arm.
The callback renders the HTML error view, so the body is a string and has no type field to read.
Assigning an empty literal narrowed the cookie jar to array{}, so the reads
after each Gitea call were flagged; the Rector fix casts the email body before
the string assertion.
Without signing the decode leaves no project and the callback answers 404; with signing it fails the signature first and answers 400. The point of the case is that neither is a fatal error.
Authorize defaults success and failure to empty strings and signs them, so the shared OAuth2 callback's array_merge let those empty values win over the computed console URLs: Gitea, GitLab and Bitbucket redirect to '' on success and throw instead of redirecting on failure. Same defect the GitHub callback had, on three more providers. The state validators also rejected an empty value before the friendly message and capped at 2048, which the signature can push a server-built state past. Tests move onto Gitea, where the callback can be driven end to end without provider credentials: missing state, empty state, a state whose project was changed after signing, the failure redirect carrying its error as a query string, and empty redirect URLs falling back to the console default.
Empty state, project not found, the default redirect, invalid JSON and the widened state length all run against the shared callback on Gitea. What stays is GitHub's own file: setup_action, installation_id, and the missing-state 400 users actually hit.
The Gitea suite drives the shared callback end to end without provider credentials, so the GitHub copies of those cases are redundant.
The long-state case is the one that tells the widened validator apart from the old one; unknown project exercises the not-found redirect; invalid JSON lands on the signature check.
One process with six coroutines instead of one process with one, matching the other workers. SMTP is stream-based and coroutine-hooked, so sends overlap on one event loop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Authorize signs empty success and failure into state when none are given, and before the fallback fix the callback then redirected to an empty location. The helper's existing assertion on the callback's Location now proves the console default is used instead. Also corrects a test comment that described GitHub's separator bug, which the shared callback never had.
chore: run the mails worker with 6 coroutines
…scope fix(vcs): scope installation repository endpoints to the project
# Conflicts: # tests/e2e/Services/VCSGitea/VCSGiteaConsoleClientTest.php
…allback fix(vcs): stop the installation callback dead-ending, on every provider
…nnot be created Bucket creation writes the bucket document, then creates the bucket_<sequence> collection. The second step is DDL, cannot share a transaction with the first, and needs the document's sequence for its name. Any failure between the two (a lock wait timeout, a killed pod) left a bucket document with no collection under it: a bucket that can never hold a file, and that every later request fails on with "Collection not found". Cloud found 227 such buckets across regions. Delete the document when collection creation throws, then rethrow, so a failed create leaves nothing behind. The test asserts the buckets collection is empty after the failed request, and fails on the unpatched code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
It asserted the double's bookkeeping of a deleteDocument call rather than anything a user can observe. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
fix(storage): remove the bucket document when its files collection cannot be created
…ibution perf: cache geo and audit attribution lookups
The callback builds its fallback redirect from the project's region, but the expected URL hardcoded project-default-. Cloud CI creates its test projects in another region, so testCreateInstallationWithEmptyRedirects and testCreateInstallationWithoutRedirects fail there since #13426. Expose the created project's region from the ProjectCustom scope and build the expected URL from it, so the test reads no environment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UQPB3JWhybuoo1gHhqDBLa
test(vcs): expect the Gitea callback redirect in the project's region
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )