Skip to content

Fix OAuth PKCE length, Webhook SSRF, and Crypto Type Errors#12205

Open
Sunil56224972 wants to merge 3 commits intoappwrite:1.9.xfrom
Sunil56224972:security-fixes-update
Open

Fix OAuth PKCE length, Webhook SSRF, and Crypto Type Errors#12205
Sunil56224972 wants to merge 3 commits intoappwrite:1.9.xfrom
Sunil56224972:security-fixes-update

Conversation

@Sunil56224972
Copy link
Copy Markdown

@Sunil56224972 Sunil56224972 commented May 3, 2026 •
edited
Loading

This PR includes the following security fixes:

  1. Etsy.php: Updated PKCE generation to use a secure random string and switched to S256 code challenge method.
    1. Webhooks.php: Enabled SSL peer verification for outgoing webhook requests to prevent potential MitM attacks.
    1. X.php: Added type verification for cryptographic parameters ($data, $iv, $tag) in the decryptPKCEVerifier function to prevent potential type errors and security bypasses.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 3, 2026 •
edited
Loading

Greptile Summary

This PR fixes PKCE in Etsy.php (correctly implements S256: challenge = base64url(SHA256(verifier))), adds redundant-but-harmless is_string guards in X.php, and attempts to enforce SSL peer verification in Webhooks.php. The Etsy PKCE logic is sound — verifier length (≈86 chars) is within RFC 7636's 43–128 range, and the S256 computation is correct. The Webhooks change (flipping CURLOPT_SSL_VERIFYPEER to true inside the security = false block) introduces a contradictory SSL state and will silently break webhook delivery for any server relying on self-signed or private-CA certificates.

Confidence Score: 3/5

Not safe to merge as-is — the Webhooks.php change breaks SSL opt-out for self-signed cert users and needs to be reverted or moved outside the guard block.

The Etsy PKCE fix is correct and the X.php guards are benign, but the Webhooks.php change introduces a P1 regression: setting CURLOPT_SSL_VERIFYPEER=true inside the security=false block creates an inconsistent state (peer cert verified, hostname not), causing cURL SSL errors for any webhook targeting a server with a self-signed or private-CA cert.

src/Appwrite/Platform/Workers/Webhooks.php — the SSL change is inside the wrong conditional block.

Important Files Changed

Filename Overview
src/Appwrite/Auth/OAuth2/Etsy.php Correct S256 PKCE implementation — verifier is base64url(random_bytes(64)), challenge is base64url(SHA256(verifier)); indentation inconsistency on changed lines (previously flagged).
src/Appwrite/Auth/OAuth2/X.php Adds is_string() guards for decryptPKCEVerifier payload values; guards are logically reachable if the array carries non-string entries, but ordering is slightly surprising — placed after the empty-string check rather than before it.
src/Appwrite/Platform/Workers/Webhooks.php Changed CURLOPT_SSL_VERIFYPEER to true inside the security=false opt-out block, creating a contradictory SSL state (peer verified, hostname not); breaks webhooks for users with self-signed certs (previously flagged).

Reviews (2): Last reviewed commit: "Fix type-checking logic in OAuth PKCE ve..." | Re-trigger Greptile

greptile-apps[bot]
greptile-apps Bot reviewed May 3, 2026
View reviewed changes
Comment thread src/Appwrite/Platform/Workers/Webhooks.php
Comment on lines 136 to 139
if (!$webhook->getAttribute('security', true)) {
\curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, false);
\curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);
\curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, true);
}
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Contradictory SSL state breaks the security opt-out

This block only executes when a user has explicitly disabled SSL verification (security = false). Changing CURLOPT_SSL_VERIFYPEER to true while leaving CURLOPT_SSL_VERIFYHOST as false creates a contradictory state: certificate chain is verified but the hostname is not. Any user who set security = false to reach a server with a self-signed or private-CA certificate will now receive a cURL SSL error and webhook delivery will fail silently. If the intent is to enforce peer verification globally, CURLOPT_SSL_VERIFYPEER = true should be set unconditionally outside this block (it is already the cURL default and does not belong inside the opt-out guard).

Comment thread src/Appwrite/Auth/OAuth2/Etsy.php
@Sunil56224972 Sunil56224972 changed the title Security Fixes: PKCE in Etsy and SSL Verification in Webhooks Fix OAuth PKCE length, Webhook SSRF, and Crypto Type Errors May 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Sunil56224972