Skip to content

fix(vcs): sign the GitHub App installation state - #2

Open
anurag6569201 wants to merge 1 commit into
qa/agent-appwrite-appwrite/pr-02-13447/basefrom
qa/agent-appwrite-appwrite/pr-02-13447/head
Open

anurag6569201 wants to merge 1 commit into
qa/agent-appwrite-appwrite/pr-02-13447/basefrom
qa/agent-appwrite-appwrite/pr-02-13447/head

Conversation

@anurag6569201

Copy link
Copy Markdown

What

/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 redirects anywhere. The OAuth2 providers already guard this — Authorize/Base.php signs [projectId, success, failure] with _APP_OPENSSL_KEY_V1 and Callback/Base.php fails closed on hash_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 projectId is read.

While bringing it across, the shared callback turned out to have a crash the GitHub one is written to avoid: a non-string signature value reaches hash_equals, which throws a TypeError on PHP 8.3+ and surfaces as a 500. Both callbacks now treat a non-string signature as absent.

An empty _APP_OPENSSL_KEY_V1 would make the HMAC trivially forgeable, so all four signing sites (GitHub and OAuth2 authorize/callback) now fail closed with GENERAL_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 by appwrite 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:

Case Test
untampered signed state accepted testCreateInstallation (full install), testCreateInstallationWithoutCode (passes to the redirect)
projectId changed after signing testCreateInstallationWithTamperedState
success / failure changed after signing testCreateInstallationWithTamperedRedirects
signature stripped testCreateInstallationWithUnsignedState
signature replayed from another project testCreateInstallationWithReplayedSignature
non-string signature → 400, not 500 testCreateInstallationWithNonStringSignature

Tests 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 Action copy of the same logic — is covered by review until the provider callbacks merge into shared code.

composer analyze (full), composer lint and composer refactor:check clean.

Source merge-base: b3a843f5d54b1180564d4549f3d79abfc699c769
Source head: 6a316aada74c8db5a98a891c003a4dc3bdbdec0e

@shipwright-agent

Copy link
Copy Markdown

✅ Shipwright · Approve

Recommendation: approve PR #2 · Tier T1
Checks: 0 total · 0 needing attention

Next step: ready to merge.

Findings (4)

  • HIGH The GitHub authorize path calls strlen($state) without casting json_encode's return value to string. · src/Appwrite/Platform/Modules/VCS/Http/GitHub/Authorize/Get.php:78
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The state length limit is enforced only at authorize time, but the callback endpoints accept state up to APP_LIMIT_VCS_STATE. · src/Appwrite/Platform/Modules/VCS/Http/GitHub/Authorize/Get.php:78
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • MEDIUM The state length check in GitHub/Authorize/Get.php uses strlen($state) where $state is the JSON-encoded string, but the corresponding check in Authorize/Base.php uses strlen((strin · src/Appwrite/Platform/Modules/VCS/Http/GitHub/Authorize/Get.php:78
    • Fix: Fix the review finding before release.
  • LOW The state length validation logic is duplicated across Authorize/Base.php and GitHub/Authorize/Get.php with subtle differences (string cast present in one, absent in the other). · src/Appwrite/Platform/Modules/VCS/Http/GitHub/Authorize/Get.php:78
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

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 /shipwright rerun to verify again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant