fix: security hardening — OAuth2, HMAC, MFA timing, cURL, PRNG#12310
fix: security hardening — OAuth2, HMAC, MFA timing, cURL, PRNG#12310Gurjit-30 wants to merge 1 commit into
Conversation
- Fix Etsy OAuth2 getUser() missing Authorization header argument - Fix Etsy PKCE verifier using weak rand() and violating RFC 7636 max length - Upgrade webhook HMAC from SHA-1 to SHA-256 - Use hash_equals() for MFA recovery code comparison (timing-safe) - Add missing Content-Type header to assistant endpoint - Reduce assistant cURL timeout from 9000s to 300s - Fix CURLOPT_SSL_VERIFYHOST false to 0 per cURL spec - Replace rand() with random_int() in worker dispatch
Greptile SummaryThis PR applies a batch of security hardening fixes across six files, addressing findings from a codebase audit. Most changes are straightforward improvements, but one requires extra care before merging.
Confidence Score: 3/5The Etsy, cURL, PRNG, and MFA changes are safe to merge, but the webhook HMAC algorithm switch will immediately break all existing consumers validating signatures and needs a migration strategy before shipping. The webhook SHA-1 → SHA-256 change has no backward-compatibility mechanism. Any deployed integration or SDK snippet that verifies X-Appwrite-Webhook-Signature will silently reject every delivery the moment this lands in production, with no indication of why. That affects every webhook user without any action on their part. src/Appwrite/Platform/Workers/Webhooks.php needs a migration plan before the SHA-256 switch goes out. Important Files Changed
Reviews (1): Last reviewed commit: "fix: security hardening — OAuth2, HMAC, ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $url = \rawurldecode($webhook->getAttribute('url')); | ||
| $signatureKey = $webhook->getAttribute('signatureKey'); | ||
| $signature = base64_encode(hash_hmac('sha1', $url . $payload, $signatureKey, true)); | ||
| $signature = base64_encode(hash_hmac('sha256', $url . $payload, $signatureKey, true)); |
There was a problem hiding this comment.
Switching from sha1 to sha256 is the right long-term direction, but it is a breaking change for every integration that is currently verifying the X-Appwrite-Webhook-Signature header. The moment this is deployed, any consumer validating with hash_hmac('sha1', …) will get a mismatched signature on every delivery with no error message or fallback. There is no version indicator in the header, no dual-emission period, and no mention of this in the changelog or migration guide. Existing SDK snippets and third-party integrations will silently reject all webhooks.
| foreach ($mfaRecoveryCodes as $code) { | ||
| if (\hash_equals($code, $otp)) { | ||
| $found = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
break partially defeats the purpose of switching to hash_equals. The string comparison for each individual code is now constant-time, but the loop exits at the first match, so a valid code at position 0 returns faster than one at position 7. An attacker who can make many rapid requests may be able to distinguish how many codes were iterated over. Removing break and letting the loop run to completion gives genuinely constant-time behavior across the full recovery code set.
| foreach ($mfaRecoveryCodes as $code) { | |
| if (\hash_equals($code, $otp)) { | |
| $found = true; | |
| break; | |
| } | |
| } | |
| foreach ($mfaRecoveryCodes as $code) { | |
| if (\hash_equals($code, $otp)) { | |
| $found = true; | |
| } | |
| } |
| curl_setopt($ch, CURLOPT_HTTPHEADER, $headers); | ||
| curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 0); | ||
| curl_setopt($ch, CURLOPT_TIMEOUT, 9000); | ||
| curl_setopt($ch, CURLOPT_TIMEOUT, 300); |
There was a problem hiding this comment.
CURLOPT_TIMEOUT may prematurely cut off SSE streaming
CURLOPT_TIMEOUT is a wall-clock limit on the entire transfer, including the streaming phase. For a Server-Sent Events connection where the assistant may produce tokens for several minutes, 300 seconds (5 minutes) can terminate a legitimately active stream. Since CURLOPT_CONNECTTIMEOUT is already set to 0 for the connection phase, consider using a transfer-specific guard (e.g., CURLOPT_LOW_SPEED_LIMIT / CURLOPT_LOW_SPEED_TIME) rather than a hard total-transfer timeout, or raise the limit to something that still caps truly stalled connections.
1 participant
What does this PR do?
This PR implements multiple security hardening improvements and bug fixes identified during a comprehensive codebase audit:
getUser()constructed an Authorization header but never passed it to the HTTP request, causing all profile API calls to fail.rand()for its length and produced verifiers up to 256 characters long, violating the RFC 7636 maximum of 128 characters. Fixed to use a deterministic cryptographically secure 64 bytes (128 hex chars).sha1hashing algorithm withsha256for webhook signatures inWebhooks.php, aligning with industry standards (GitHub, Stripe, etc.).in_array()with a timing-safehash_equals()loop for MFA recovery code comparison inUpdate.php.Content-Type: application/jsonheader to the SSE request.CURLOPT_SSL_VERIFYHOSTfrom booleanfalseto integer0per cURL specifications inWebhooks.phpandExecutor.php.rand()withrandom_int()inapp/http.phpfor uniform worker dispatch distribution.Test Plan
X-Appwrite-Webhook-Signatureheader validates correctly usinghash_hmac('sha256', ...)on the receiving end.Related PRs and Issues
Checklist