fix(vcs): sign the GitHub App installation state - #2
Open
anurag6569201 wants to merge 1 commit into
Open
anurag6569201 wants to merge 1 commit into
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: appwrite#13447 Source head: 6a316aa
✅ Shipwright · ApproveRecommendation: approve PR #2 · Tier
Findings (4)
Fireworks usage: 12,072 input · 606 output · 12,678 total tokens · $0.0031 · 12s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
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
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
/v1/vcs/github/callbackis public and took theprojectIdand both redirect URLs straight out of thestateparameter, so anyone could name the project an installation gets attached to, or point the redirects anywhere. The OAuth2 providers already guard this —Authorize/Base.phpsigns[projectId, success, failure]with_APP_OPENSSL_KEY_V1andCallback/Base.phpfails closed onhash_equals. The GitHub App flow predates that hardening and was never brought across.This applies the same signature to the state the GitHub authorize endpoint builds, and verifies it in the callback before the
projectIdis read.While bringing it across, the shared callback turned out to have a crash the GitHub one is written to avoid: a non-string
signaturevalue reacheshash_equals, which throws aTypeErroron PHP 8.3+ and surfaces as a 500. Both callbacks now treat a non-string signature as absent.An empty
_APP_OPENSSL_KEY_V1would make the HMAC trivially forgeable, so all four signing sites (GitHub and OAuth2 authorize/callback) now fail closed withGENERAL_SERVER_ERROR, the way the module already refuses a missing provider configuration. A weak-but-present key is a deployment-wide concern — the same key backs sessions and encryption at rest — and is surfaced byappwrite doctor.Split out of appwrite#13426 per review.
Testing
On Gitea, in the existing
VCSGiteaConsoleClientTest, driving the shared callback end to end without provider credentials. Status codes only. Together with the cases already there from appwrite#13426, the signature mechanism is covered as:testCreateInstallation(full install),testCreateInstallationWithoutCode(passes to the redirect)projectIdchanged after signingtestCreateInstallationWithTamperedStatesuccess/failurechanged after signingtestCreateInstallationWithTamperedRedirectstestCreateInstallationWithUnsignedStatetestCreateInstallationWithReplayedSignaturetestCreateInstallationWithNonStringSignatureTests run on Gitea only, following the practice set in appwrite#13426: the Gitea suite drives the shared callback end to end, and the GitHub callback — a separate
extends Actioncopy of the same logic — is covered by review until the provider callbacks merge into shared code.composer analyze(full),composer lintandcomposer refactor:checkclean.Source merge-base:
b3a843f5d54b1180564d4549f3d79abfc699c769Source head:
6a316aada74c8db5a98a891c003a4dc3bdbdec0e