feat: constant-time HMAC-SHA256 - #635
Conversation
Welcome to OSSfolio, @Aditya8369! 🎉Thank you for opening this pull request and contributing to the open-source community! 🚀 To ensure a smooth review process, please make sure you have:
We will review your PR as soon as possible. Happy coding! 💻✨ |
github-actions
Bot
added
frontend
📝 WalkthroughWalkthroughGitHub webhook verification now uses Web Crypto byte comparisons. Failed profile refreshes are stored in a Supabase dead-letter queue with retry scheduling, and a protected endpoint processes queued entries. Tests cover signature validation, webhook handling, queue insertion, processing, and retries. ChangesGitHub webhook reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GitHub
participant WebhookRoute
participant RefreshProfile
participant DeadLetterQueue
GitHub->>WebhookRoute: Send signed webhook
WebhookRoute->>RefreshProfile: Refresh contributor profile
RefreshProfile-->>WebhookRoute: Return status or throw error
WebhookRoute->>DeadLetterQueue: Enqueue failed payload
WebhookRoute-->>GitHub: Return webhook response
sequenceDiagram
participant AdminClient
participant RetryRoute
participant DeadLetterProcessor
participant RefreshProfile
AdminClient->>RetryRoute: POST retry request with batch size
RetryRoute->>DeadLetterProcessor: Process due queue entries
DeadLetterProcessor->>RefreshProfile: Retry profile refresh
RefreshProfile-->>DeadLetterProcessor: Return refresh result
DeadLetterProcessor-->>RetryRoute: Return processing statistics
RetryRoute-->>AdminClient: Return success and stats
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/webhooks/github/retry/route.ts`:
- Around line 15-21: Clamp the parsed batchSize in the retry route before
passing it to processDeadLetterQueue, treating invalid or missing values as the
default and restricting valid values to the safe 1–100 range. Update the
existing batchSize parsing/validation flow while preserving the current
processDeadLetterQueue call.
- Around line 6-13: Update the authorization check in the retry route to return
a 503 “secret missing” response when neither SUPABASE_SERVICE_ROLE_KEY nor
GITHUB_WEBHOOK_SECRET is configured, before comparing credentials or queueing
work. Then compare the provided bearer token with the configured secret using a
timing-safe comparison, while preserving the existing 401 response for invalid
credentials.
In `@src/app/api/webhooks/github/route.ts`:
- Around line 139-144: Limit the dead-letter payload in the webhook handler’s
enqueueDeadLetterPayload calls to only fields required for retry, such as owner
and parsedPayload.ref, instead of passing the full parsedPayload containing PII.
Apply this change to both the refresh-status failure path and the second enqueue
call around the referenced lines, while preserving the existing owner fallback.
- Around line 136-145: Update refreshProfile status handling to distinguish
intentional 10-minute throttling from retryable lock-contention or refresh
failures by introducing a non-retryable Throttled variant separate from
RateLimited. In the webhook flow around refreshProfile and
enqueueDeadLetterPayload, enqueue only retryable error/lock-contention outcomes,
excluding Throttled, and update related webhook/dead-letter retry checks to
preserve this distinction.
- Around line 26-57: Update timingSafeEqualBuffer to normalize both inputs into
byte-addressed Uint8Array views that preserve each source view’s byteOffset and
byteLength before any native timingSafeEqual call. Validate equal byte lengths
first, then invoke the native API with the normalized views; retain the
constant-time manual comparison fallback for environments without the native
method.
In `@src/lib/__tests__/github-webhook.test.ts`:
- Around line 183-207: Extend the GitHub webhook tests alongside the existing
“background refresh encounters error” case to cover both remaining dead-letter
branches: mock refreshProfile to return status “rate_limited” and assert
enqueueDeadLetterPayload uses the corresponding errorReason, then mock it to
throw an Error and assert the thrown message is used as errorReason. Preserve
the existing successful response and payload assertions.
In `@src/lib/__tests__/webhook-dead-letter.test.ts`:
- Around line 79-188: Add a test in the processDeadLetterQueue suite covering a
refresh failure when retry_count + 1 >= max_retries. Mock the pending item and
refreshProfile response, invoke processDeadLetterQueue, then assert the returned
stats report one processed and failed item with no retry, and verify mockUpdate
marks the item status as failed.
In `@src/lib/webhook-dead-letter.ts`:
- Around line 44-58: Sanitize and minimize the payload before the insert in the
webhook dead-letter write flow, rather than persisting input.payload verbatim.
Update the code around the webhook_dead_letter_queue insert to remove or redact
sensitive and unnecessary GitHub fields while preserving the data required for
retry and diagnosis, and ensure the stored payload remains bounded for
indefinite retention.
- Around line 1-2: Add the `server-only` import at the top of the
`webhook-dead-letter` module before its existing imports, preserving the current
`supabaseAdmin` and `refreshProfile` behavior while preventing accidental client
bundling.
- Around line 91-116: Update the row-claiming update in processDeadLetterQueue
to atomically claim only rows still in pending status by adding a status
predicate alongside the existing id filter. Ensure subsequent processing
continues only when that claim succeeds, so concurrent invocations skip rows
already claimed by another run.
- Around line 103-164: Update the loop handling in the dead-letter processing
function to isolate each item with a per-item try/catch, ensuring exceptions
from refreshProfile or status updates do not abort the batch and leave rows in
processing. Check the returned error from every webhook_dead_letter_queue
update, and route any failure through the item’s failure/recovery handling
before updating result counters; preserve the existing completed, failed, and
pending retry transitions for successful writes.
In `@supabase/migrations/20260726000001_add_webhook_dead_letters.sql`:
- Around line 5-19: The webhook_dead_letter_queue schema retains complete
payloads indefinitely; add a retention mechanism for terminal completed/failed
rows, such as an indexed scheduled cleanup with a defined retention window, and
redact sensitive webhook fields before persistence where applicable. Keep
pending and processing retry data available and preserve service-role-only
access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54234d33-03fe-41b0-b51b-b9f430e6a774
📒 Files selected for processing (6)
src/app/api/webhooks/github/retry/route.tssrc/app/api/webhooks/github/route.tssrc/lib/__tests__/github-webhook.test.tssrc/lib/__tests__/webhook-dead-letter.test.tssrc/lib/webhook-dead-letter.tssupabase/migrations/20260726000001_add_webhook_dead_letters.sql
| // Simple administrative authorization check via header or service role token | ||
| const authHeader = request.headers.get("authorization"); | ||
| const secretKey = | ||
| process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.GITHUB_WEBHOOK_SECRET; | ||
|
|
||
| if (secretKey && authHeader !== `Bearer ${secretKey}`) { | ||
| return createErrorResponse("Unauthorized", 401); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Web Crypto crypto.subtle.timingSafeEqual exist, or is timing-safe byte comparison implemented manually in Node/Edge runtimes?
💡 Result:
The Web Crypto API standard does not include crypto.subtle.timingSafeEqual [1][2][3]. Therefore, it is not available in standard web browsers or generic edge runtimes [2][3]. Regarding the runtimes: Node.js: Node.js provides a native, constant-time crypto.timingSafeEqual(a, b) function via its node:crypto module [4]. This is implemented in C++ (using CRYPTO_memcmp from OpenSSL) to ensure the comparison is resistant to timing attacks [5][6]. It is not part of the crypto.subtle (Web Crypto) namespace in Node.js [4]. Edge Runtimes: - Cloudflare Workers, which heavily utilizes the Web Crypto API, has implemented crypto.subtle.timingSafeEqual as a non-standard extension to their runtime [7][8][3]. - For environments where crypto.subtle.timingSafeEqual is unavailable (e.g., standard browsers or other edge environments), developers commonly implement a manual timing-safe comparison using a double HMAC verification pattern [1][2][3]. This approach relies on the fact that HMAC verification in Web Crypto is typically implemented in constant time across major browser engines, preventing timing side-channel attacks [1][2]. In summary, crypto.subtle.timingSafeEqual is not a standard web API, but it is available in the Cloudflare Workers runtime as an extension. Node.js provides a similar function, but it resides in the crypto module rather than crypto.subtle [4]. In other environments, manual implementation using cryptographic primitives like HMAC is the standard approach [2][3].
Citations:
- 1: timingSafeEqual functionality w3c/webcrypto#270
- 2: https://www.arun.blog/timing-safe-auth-web-crypto/
- 3: https://www.npmjs.com/package/@advena/web-timing-safe-equal
- 4: https://nodejs.org/api/crypto.html
- 5: Add crypto.timingSafeEqual() nodejs/node#8040
- 6: https://fossies.org/linux/node/src/crypto/crypto_timing.cc
- 7: https://developers.cloudflare.com/workers/examples/protect-against-timing-attacks/
- 8: https://developers.cloudflare.com/workers/runtime-apis/web-crypto/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target route and sibling webhook route references.
echo "== files =="
git ls-files | rg '(^|/)route\.ts$' | sed -n '1,120p'
echo "== target outline =="
ast-grep outline src/app/api/webhooks/github/retry/route.ts --view expanded || true
echo "== target route =="
cat -n src/app/api/webhooks/github/retry/route.ts
echo "== sibling route candidates =="
for f in $(git ls-files | rg 'src/app/api/webhooks/github/.*/route\.ts$|src/app/api/webhooks.*route\.ts$'); do
echo "-- $f --"
sed -n '1,140p' "$f"
done
echo "== timing-safe / auth headers usage =="
rg -n "timingSafe|timing_safe|GITHUB_WEBHOOK_SECRET|SUPABASE_SERVICE_ROLE_KEY|authorization|Bearer|processDeadLetterQueue|refreshProfile|revalidatePath" src/app/api/webhooks src/app || true
echo "== deterministic auth branch behavior probe =="
python3 - <<'PY'
import os
os.environ.pop("SUPABASE_SERVICE_ROLE_KEY", None)
os.environ.pop("GITHUB_WEBHOOK_SECRET", None)
class Env:
pass
process = Env()
process.GITHUB_WEBHOOK_SECRET = None
process.SUPABASE_SERVICE_ROLE_KEY = None
auth_header = "Bearer valid"
secret_key = process.SUPABASE_SERVICE_ROLE_KEY or process.GITHUB_WEBHOOK_SECRET
print("secret_key_falsy=", bool(secret_key))
blocked = secret_key and auth_header != f"Bearer {secret_key}"
print("blocked_by_if=", blocked)
print("would_continue_to_endpoint=", not blocked)
PYRepository: PRODHOSH/ossfolio
Length of output: 12852
Fail closed before queueing retry work.
If neither SUPABASE_SERVICE_ROLE_KEY nor GITHUB_WEBHOOK_SECRET is configured, this route accepts unauthenticated requests and calls processDeadLetterQueue, which can mutate profile data. Add an explicit “secret missing” 503 guard before comparing credentials, and use a timing-safe bearer-token comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/webhooks/github/retry/route.ts` around lines 6 - 13, Update the
authorization check in the retry route to return a 503 “secret missing” response
when neither SUPABASE_SERVICE_ROLE_KEY nor GITHUB_WEBHOOK_SECRET is configured,
before comparing credentials or queueing work. Then compare the provided bearer
token with the configured secret using a timing-safe comparison, while
preserving the existing 401 response for invalid credentials.
| const { searchParams } = new URL(request.url); | ||
| const batchSizeParam = searchParams.get("batchSize"); | ||
| const batchSize = batchSizeParam ? parseInt(batchSizeParam, 10) : 10; | ||
|
|
||
| const result = await processDeadLetterQueue( | ||
| isNaN(batchSize) ? 10 : batchSize, | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
batchSize isn't clamped.
Negative, zero, or very large values pass straight to processDeadLetterQueue's .limit(batchSize) with only a NaN guard. Clamp to a safe range (e.g., 1-100).
♻️ Proposed fix
- const batchSize = batchSizeParam ? parseInt(batchSizeParam, 10) : 10;
-
- const result = await processDeadLetterQueue(
- isNaN(batchSize) ? 10 : batchSize,
- );
+ const parsed = batchSizeParam ? parseInt(batchSizeParam, 10) : 10;
+ const batchSize = Number.isNaN(parsed) ? 10 : Math.min(Math.max(parsed, 1), 100);
+
+ const result = await processDeadLetterQueue(batchSize);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { searchParams } = new URL(request.url); | |
| const batchSizeParam = searchParams.get("batchSize"); | |
| const batchSize = batchSizeParam ? parseInt(batchSizeParam, 10) : 10; | |
| const result = await processDeadLetterQueue( | |
| isNaN(batchSize) ? 10 : batchSize, | |
| ); | |
| const { searchParams } = new URL(request.url); | |
| const batchSizeParam = searchParams.get("batchSize"); | |
| const parsed = batchSizeParam ? parseInt(batchSizeParam, 10) : 10; | |
| const batchSize = Number.isNaN(parsed) ? 10 : Math.min(Math.max(parsed, 1), 100); | |
| const result = await processDeadLetterQueue(batchSize); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/webhooks/github/retry/route.ts` around lines 15 - 21, Clamp the
parsed batchSize in the retry route before passing it to processDeadLetterQueue,
treating invalid or missing values as the default and restricting valid values
to the safe 1–100 range. Update the existing batchSize parsing/validation flow
while preserving the current processDeadLetterQueue call.
| /** Constant-time comparison of two ArrayBuffer or ArrayBufferView byte buffers. */ | ||
| export function timingSafeEqualBuffer( | ||
| a: ArrayBuffer | ArrayBufferView, | ||
| b: ArrayBuffer | ArrayBufferView, | ||
| ): boolean { | ||
| const subtle = crypto.subtle as unknown as { | ||
| timingSafeEqual?: ( | ||
| _a: ArrayBuffer | ArrayBufferView, | ||
| _b: ArrayBuffer | ArrayBufferView, | ||
| ) => boolean; | ||
| }; | ||
|
|
||
| if (typeof subtle.timingSafeEqual === "function") { | ||
| return subtle.timingSafeEqual(a, b); | ||
| } | ||
|
|
||
| const viewA = | ||
| a instanceof Uint8Array | ||
| ? a | ||
| : new Uint8Array("buffer" in a ? a.buffer : a); | ||
| const viewB = | ||
| b instanceof Uint8Array | ||
| ? b | ||
| : new Uint8Array("buffer" in b ? b.buffer : b); | ||
|
|
||
| if (viewA.byteLength !== viewB.byteLength) return false; | ||
| let mismatch = 0; | ||
| for (let i = 0; i < a.length; i++) { | ||
| mismatch |= a.charCodeAt(i) ^ b.charCodeAt(i); | ||
| for (let i = 0; i < viewA.byteLength; i++) { | ||
| mismatch |= viewA[i] ^ viewB[i]; | ||
| } | ||
| return mismatch === 0; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(route\.ts|.*webhook.*|.*github.*)$' || true
echo "== target snippet =="
sed -n '1,120p' src/app/api/webhooks/github/route.ts 2>/dev/null || true
echo "== timingSafeEqual usage/tests =="
rg -n "timingSafeEqualBuffer|verifySignature|different byte lengths|byteLength|DataView|sliced|sliced" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo "== relevant package/config =="
for f in package.json wrangler.toml next.config.js next.config.mjs tsconfig.json vitest.config.*; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,220p' "$f"; }
doneRepository: PRODHOSH/ossfolio
Length of output: 11128
🌐 Web query:
Cloudflare workers crypto.subtle.timingSafeEqual ArrayBufferView byteOffset byteLength equal length documentation
💡 Result:
In Cloudflare Workers, crypto.subtle.timingSafeEqual is a non-standard extension to the Web Crypto API designed to perform constant-time comparisons of binary data to prevent timing side-channel attacks [1]. Regarding the parameters: 1. Arguments: The function accepts two inputs, which must be either ArrayBuffer, TypedArray, or DataView instances [1][2]. 2. Length Requirement: Both input buffers must have the same byte length. If the lengths differ, the function will throw an exception [3][4][2]. 3. Byte-level Comparison: The function performs a constant-time comparison of the underlying bytes of the provided buffers [2][5]. 4. Security Note: While the function itself is designed for constant-time execution, using it does not guarantee that the surrounding code is timing-safe [3][4][6]. Crucially, you must avoid returning early based on length differences, as this creates a timing leak that reveals secret lengths [7][4]. Recommended Pattern: To perform a secure comparison, always compare the input length against your expected secret length without using early returns, and use a dummy comparison when lengths do not match to ensure the execution path remains consistent [3][4]. Example implementation: const userValue = encoder.encode(providedToken); const secretValue = encoder.encode(storedSecret); // Use a constant-time comparison without leaking the length const lengthsMatch = userValue.byteLength === secretValue.byteLength; const isEqual = lengthsMatch? crypto.subtle.timingSafeEqual(userValue, secretValue):!crypto.subtle.timingSafeEqual(userValue, userValue); // Dummy comparison if (!isEqual) { throw new Error("Unauthorized"); }
Citations:
- 1: https://developers.cloudflare.com/workers/runtime-apis/web-crypto/
- 2: https://bun.com/reference/node/crypto/timingSafeEqual
- 3: https://developers.cloudflare.com/workers/examples/protect-against-timing-attacks/
- 4: https://developers.cloudflare.com/workers/examples/protect-against-timing-attacks/index.md
- 5: https://docs.deno.com/api/node/crypto/~/timingSafeEqual
- 6: nodejs/node@ad75c78c32
- 7: Using
timingSafeEqualexamples are not safe? cloudflare/cloudflare-docs#23623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== github-webhook tests =="
sed -n '1,130p' src/lib/__tests__/github-webhook.test.ts
echo "== runtime probes for current helper behavior =="
node - <<'JS'
const timingSafeEqualBuffer = (a, b) => {
const subtle = crypto.subtle ?? {};
if (typeof subtle.timingSafeEqual === "function") {
return subtle.timingSafeEqual(a, b);
}
const viewA =
a instanceof Uint8Array ? a : new Uint8Array("buffer" in a ? a.buffer : a);
const viewB =
b instanceof Uint8Array ? b : new Uint8Array("buffer" in b ? b.buffer : b);
if (viewA.byteLength !== viewB.byteLength) return false;
let mismatch = 0;
for (let i = 0; i < viewA.byteLength; i++) mismatch |= viewA[i] ^ viewB[i];
return mismatch === 0;
};
for (const name of ["NodeCrypto.timingSafeEqual present", "NodeCrypto.timingSafeEqual missing"]) {
console.log("--", name);
const original = globalThis.crypto?.subtle?.timingSafeEqual;
if (name === "NodeCrypto.timingSafeEqual missing" && original) {
globalThis.crypto.subtle.timingSafeEqual = undefined;
} else if (name === "NodeCrypto.timingSafeEqual present" && !original) {
globalThis.crypto = {
subtle: {
timingSafeEqual: (a, b) =>
new Uint8Array(a.buffer || a.data, a.byteOffset || a.offset, a.byteLength || a.length).length ===
new Uint8Array(b.buffer || b.data, b.byteOffset || b.length).length ===
0,
},
};
}
const inputs = [
["same Uint8Array values"],
["sliced Dataview equal"],
["different ArrayBufferView views same backing"],
["different byte lengths"],
];
for (const input of inputs) {
let result;
try {
if (input[0] === "sliced Dataview equal") {
const backing = new ArrayBuffer(12);
const a = new DataView(backing, 0, 4);
new Uint8Array(backing).set([1,2,3,4], 0);
const b = new Uint8Array(backing, 0, 4);
result = "returns " + timingSafeEqualBuffer(a, b);
} else {
result = "no-op";
}
} catch (e) {
result = "throws " + e.name + ": " + e.message;
}
console.log(input[0], "=>", result);
}
if (name === "NodeCrypto.timingSafeEqual missing" && original) {
globalThis.crypto.subtle.timingSafeEqual = original;
}
}
JSRepository: PRODHOSH/ossfolio
Length of output: 4865
🌐 Web query:
Bun crypto.timingSafeEqual ArrayBuffer ArrayBufferView byteLength throw different length documentation
💡 Result:
In Bun, the crypto.timingSafeEqual function requires the two provided ArrayBuffer or ArrayBufferView instances to have the same byte length [1]. If the provided arguments have different byte lengths, the function will throw an error [1]. This behavior aligns with the standard Node.js crypto.timingSafeEqual implementation [2][3]. Because the function is designed to prevent timing attacks when comparing cryptographic signatures or hashes—which typically have a fixed, expected length—it does not support comparing buffers of different sizes [3]. If you need to compare buffers that may have different lengths, you should perform a manual length check first and return false if they do not match [3][4]. This pattern is considered secure because a length comparison is not sensitive to the content of the buffers and does not leak information that would aid a timing attack [3]. Example of safe usage: if (bufferA.byteLength!== bufferB.byteLength) { return false; } return crypto.timingSafeEqual(bufferA, bufferB);
Citations:
- 1: https://bun.com/reference/node/crypto/timingSafeEqual
- 2: https://nodejs.org/api/crypto.html
- 3: crypto.timingSafeEqual is not really time safe? nodejs/node#17178
- 4: https://stackoverflow.com/questions/66226092/how-to-use-buffer-from-with-crypto-timingsafeequal
Normalize views before the native timingSafeEqual call.
timingSafeEqualBuffer is exposed for ArrayBuffer | ArrayBufferView, but the current fallback normalizes non-Uint8Array views as if they span the whole backing buffer (ignoring byteOffset/byteLength), so a sliced DataView/typed view can be compared incorrectly. Call the native API only after normalizing to a byte-addressed Uint8Array, and call it with same-length buffers to avoid workerd/node timingSafeEqual length-related failures in production.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/webhooks/github/route.ts` around lines 26 - 57, Update
timingSafeEqualBuffer to normalize both inputs into byte-addressed Uint8Array
views that preserve each source view’s byteOffset and byteLength before any
native timingSafeEqual call. Validate equal byte lengths first, then invoke the
native API with the normalized views; retain the constant-time manual comparison
fallback for environments without the native method.
| try { | ||
| await refreshProfile(username); | ||
| const res = await refreshProfile(username); | ||
| if (res.status === "error" || res.status === "rate_limited") { | ||
| await enqueueDeadLetterPayload({ | ||
| eventType: event ?? "push", | ||
| username, | ||
| payload: parsedPayload ?? { owner }, | ||
| errorReason: `refresh_status_${res.status}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'route\.ts$' . | rg 'src/app/api/webhooks/github/route\.ts|refresh-profile' || true
echo "== route excerpt =="
sed -n '1,190p' src/app/api/webhooks/github/route.ts
echo "== refresh profile files =="
fd -a -i 'refresh.*profile' src | sed 's#^\./##'
for f in $(fd -i 'refresh.*profile|refresh-profile' src); do
echo "--- $f ---"
wc -l "$f"
ast-grep outline "$f" --view compact || true
doneRepository: PRODHOSH/ossfolio
Length of output: 5517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== refresh-profile.ts =="
sed -n '1,160p' src/lib/refresh-profile.ts
echo "== dead letter related files =="
fd -a 'webhook-dead-letter|dead.*letter|queue|dead' src | sed 's#^\./##'
echo "== dead-letter module excerpts =="
for f in $(fd -i 'webhook-dead-letter|dead.*letter|queue|dead' src); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
done
echo "== usages of RefreshResult/status/rate_limited =="
rg -n "RefreshResult|refresh_profile|refreshProfile|rate_limited|advisory|lock|sleep|retry" srcRepository: PRODHOSH/ossfolio
Length of output: 39086
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== refreshProfile PGRST116 handling =="
sed -n '37,86p' src/lib/refresh-profile.ts
echo "== refreshProfile RPC lock handling =="
sed -n '24,42p' src/lib/refresh-profile.ts
echo "== behavioral data from refreshProfile source =="
python3 - <<'PY'
from pathlib import Path
text = Path("src/lib/refresh-profile.ts").read_text()
print("- lock false result:", 'lockAcquired === false' in text and 'status: "rate_limited"' in text[text.find('lockAcquired === false'):text.find('lockAcquired === false')+500])
print("- RPC catch branch:", "try {" in text and "catch (err)" in text and text[text.find("catch (err)")-5:text.find("catch (err)")+120].strip())
print("- RPC catch returns error:", "catch (err)" in text and "return { status: \"error\" }" in text[text.find("catch (err)", text.find("catch (err)")):])
print("- PGRST116 update branch:", "PGRST116" in text and 'status: "rate_limited"' in text[text.find("PGRST116"):text.find("PGRST116")+500])
print("- PGRST116 non-error branch:", "PGRST116" in text and "else {" in text[text.find("PGRST116"):text.find("PGRST116")+400])
print("- update error branch:", 'return { status: "error" }' in text[text.find("select(\"username\").single()"):text.find("select(\"username\").single()")+800])
PYRepository: PRODHOSH/ossfolio
Length of output: 2719
Separate intended rate-limiting from retryable refresh failures.
refreshProfile() returns the same "rate_limited" status for both the 10-minute update window and the advisory-lock/refresh-lock contention paths, while both webhook and dead-letter retry logic enqueue these identical statuses. Split RateLimited into a distinct non-retryable Throttled variant and enqueue only the lock-contention/error cases so rapid same-user pushes don’t fill the retry queue with intentionally skipped refreshes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/webhooks/github/route.ts` around lines 136 - 145, Update
refreshProfile status handling to distinguish intentional 10-minute throttling
from retryable lock-contention or refresh failures by introducing a
non-retryable Throttled variant separate from RateLimited. In the webhook flow
around refreshProfile and enqueueDeadLetterPayload, enqueue only retryable
error/lock-contention outcomes, excluding Throttled, and update related
webhook/dead-letter retry checks to preserve this distinction.
| await enqueueDeadLetterPayload({ | ||
| eventType: event ?? "push", | ||
| username, | ||
| payload: parsedPayload ?? { owner }, | ||
| errorReason: `refresh_status_${res.status}`, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Full raw webhook payload (including PII) is persisted to the dead-letter table.
parsedPayload = JSON.parse(body) as PushPayload (line 121) is a compile-time-only type assertion; at runtime parsedPayload still holds the entire GitHub push payload, not just repository.owner. Passing it straight through as payload: parsedPayload ?? { owner } means enqueueDeadLetterPayload stores the full event — including committer/author/pusher emails and names from commits[]/pusher — as JSON in Postgres, well beyond what the narrow PushPayload type suggests is retained. This is a PII-retention/compliance risk for a queue table whose purpose is only to retry a username refresh.
Consider persisting only the minimal fields actually needed to retry (e.g. { owner, ref: parsedPayload.ref }) rather than the full payload.
Also applies to: 151-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/webhooks/github/route.ts` around lines 139 - 144, Limit the
dead-letter payload in the webhook handler’s enqueueDeadLetterPayload calls to
only fields required for retry, such as owner and parsedPayload.ref, instead of
passing the full parsedPayload containing PII. Apply this change to both the
refresh-status failure path and the second enqueue call around the referenced
lines, while preserving the existing owner fallback.
| import { supabaseAdmin } from "@/lib/supabase"; | ||
| import { refreshProfile } from "@/lib/refresh-profile"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add server-only guard.
This module builds/uses supabaseAdmin() (service-role client). Based on learnings, server-side modules under src/lib that read SUPABASE_SERVICE_ROLE_KEY via supabaseAdmin() should import server-only at the top as a build-time guard against accidental client bundling.
🛡️ Proposed fix
+import "server-only";
import { supabaseAdmin } from "`@/lib/supabase`";
import { refreshProfile } from "`@/lib/refresh-profile`";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { supabaseAdmin } from "@/lib/supabase"; | |
| import { refreshProfile } from "@/lib/refresh-profile"; | |
| import "server-only"; | |
| import { supabaseAdmin } from "`@/lib/supabase`"; | |
| import { refreshProfile } from "`@/lib/refresh-profile`"; |
🧰 Tools
🪛 GitHub Actions: Prettier Code Formatting / 0_prettier.txt
[error] 1-1: prettier --check . reported formatting issues: Code style issues found in 192 files. Run Prettier with --write to fix.
🪛 GitHub Actions: Prettier Code Formatting / prettier
[warning] 1-1: Prettier --check reported formatting issues in 192 files. Run 'npx prettier --write .' to fix.
[error] 1-1: Command 'npx prettier --check .' failed with exit code 1 due to code style issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/webhook-dead-letter.ts` around lines 1 - 2, Add the `server-only`
import at the top of the `webhook-dead-letter` module before its existing
imports, preserving the current `supabaseAdmin` and `refreshProfile` behavior
while preventing accidental client bundling.
Source: Learnings
| const { data, error } = await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .insert({ | ||
| event_type: input.eventType, | ||
| username: input.username, | ||
| payload: input.payload ?? {}, | ||
| error_reason: input.errorReason ?? "transient_write_failure", | ||
| status: "pending", | ||
| retry_count: 0, | ||
| max_retries: 5, | ||
| received_at: now, | ||
| next_retry_at: now, | ||
| created_at: now, | ||
| updated_at: now, | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Full raw webhook payload persisted unredacted (mirrors the schema-level concern).
input.payload is inserted verbatim; combined with the webhook_dead_letter_queue.payload jsonb column having no retention limit (see migration file), this indefinitely persists potentially PII-bearing GitHub payload data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/webhook-dead-letter.ts` around lines 44 - 58, Sanitize and minimize
the payload before the insert in the webhook dead-letter write flow, rather than
persisting input.payload verbatim. Update the code around the
webhook_dead_letter_queue insert to remove or redact sensitive and unnecessary
GitHub fields while preserving the data required for retry and diagnosis, and
ensure the stored payload remains bounded for indefinite retention.
| const { data: items, error: fetchError } = await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .select("*") | ||
| .eq("status", "pending") | ||
| .lte("next_retry_at", nowIso) | ||
| .order("next_retry_at", { ascending: true }) | ||
| .limit(batchSize); | ||
|
|
||
| if (fetchError || !items || items.length === 0) { | ||
| return result; | ||
| } | ||
|
|
||
| for (const item of items as WebhookDeadLetterRow[]) { | ||
| result.processed++; | ||
| const currentNow = new Date().toISOString(); | ||
|
|
||
| // Lock row to processing status | ||
| await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .update({ | ||
| status: "processing", | ||
| last_retry_at: currentNow, | ||
| updated_at: currentNow, | ||
| }) | ||
| .eq("id", item.id); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Row-claiming has a TOCTOU race across concurrent invocations.
The claiming update (108-115) filters only .eq("id", item.id), with no .eq("status", "pending") guard. Two overlapping processDeadLetterQueue runs (e.g., overlapping cron trigger + manual /retry POST) can both SELECT the same due rows before either marks them processing, causing duplicate refreshProfile calls and racing final-state writes on the same row (a later "retry_scheduled" write could clobber an earlier "completed" write, or vice versa).
🔒 Proposed fix: atomic claim
- await adminClient
- .from("webhook_dead_letter_queue")
- .update({
- status: "processing",
- last_retry_at: currentNow,
- updated_at: currentNow,
- })
- .eq("id", item.id);
+ const { data: claimed } = await adminClient
+ .from("webhook_dead_letter_queue")
+ .update({
+ status: "processing",
+ last_retry_at: currentNow,
+ updated_at: currentNow,
+ })
+ .eq("id", item.id)
+ .eq("status", "pending")
+ .select("id")
+ .maybeSingle();
+ if (!claimed) continue; // already claimed by another run📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data: items, error: fetchError } = await adminClient | |
| .from("webhook_dead_letter_queue") | |
| .select("*") | |
| .eq("status", "pending") | |
| .lte("next_retry_at", nowIso) | |
| .order("next_retry_at", { ascending: true }) | |
| .limit(batchSize); | |
| if (fetchError || !items || items.length === 0) { | |
| return result; | |
| } | |
| for (const item of items as WebhookDeadLetterRow[]) { | |
| result.processed++; | |
| const currentNow = new Date().toISOString(); | |
| // Lock row to processing status | |
| await adminClient | |
| .from("webhook_dead_letter_queue") | |
| .update({ | |
| status: "processing", | |
| last_retry_at: currentNow, | |
| updated_at: currentNow, | |
| }) | |
| .eq("id", item.id); | |
| const { data: items, error: fetchError } = await adminClient | |
| .from("webhook_dead_letter_queue") | |
| .select("*") | |
| .eq("status", "pending") | |
| .lte("next_retry_at", nowIso) | |
| .order("next_retry_at", { ascending: true }) | |
| .limit(batchSize); | |
| if (fetchError || !items || items.length === 0) { | |
| return result; | |
| } | |
| for (const item of items as WebhookDeadLetterRow[]) { | |
| result.processed++; | |
| const currentNow = new Date().toISOString(); | |
| // Lock row to processing status | |
| const { data: claimed } = await adminClient | |
| .from("webhook_dead_letter_queue") | |
| .update({ | |
| status: "processing", | |
| last_retry_at: currentNow, | |
| updated_at: currentNow, | |
| }) | |
| .eq("id", item.id) | |
| .eq("status", "pending") | |
| .select("id") | |
| .maybeSingle(); | |
| if (!claimed) continue; // already claimed by another run |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/webhook-dead-letter.ts` around lines 91 - 116, Update the
row-claiming update in processDeadLetterQueue to atomically claim only rows
still in pending status by adding a status predicate alongside the existing id
filter. Ensure subsequent processing continues only when that claim succeeds, so
concurrent invocations skip rows already claimed by another run.
| for (const item of items as WebhookDeadLetterRow[]) { | ||
| result.processed++; | ||
| const currentNow = new Date().toISOString(); | ||
|
|
||
| // Lock row to processing status | ||
| await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .update({ | ||
| status: "processing", | ||
| last_retry_at: currentNow, | ||
| updated_at: currentNow, | ||
| }) | ||
| .eq("id", item.id); | ||
|
|
||
| const refreshRes = await refreshProfile(item.username); | ||
|
|
||
| if (refreshRes.status === "refreshed") { | ||
| const finishNow = new Date().toISOString(); | ||
| await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .update({ | ||
| status: "completed", | ||
| updated_at: finishNow, | ||
| }) | ||
| .eq("id", item.id); | ||
| result.succeeded++; | ||
| } else { | ||
| const nextRetryCount = item.retry_count + 1; | ||
| const finishNow = new Date().toISOString(); | ||
|
|
||
| if (nextRetryCount >= item.max_retries) { | ||
| await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .update({ | ||
| status: "failed", | ||
| retry_count: nextRetryCount, | ||
| error_reason: `max_retries_exceeded: ${refreshRes.status}`, | ||
| updated_at: finishNow, | ||
| }) | ||
| .eq("id", item.id); | ||
| result.failed++; | ||
| } else { | ||
| // Exponential backoff: 5m, 10m, 20m, 40m... | ||
| const backoffMinutes = Math.pow(2, nextRetryCount - 1) * 5; | ||
| const nextRetryDate = new Date( | ||
| Date.now() + backoffMinutes * 60 * 1000, | ||
| ).toISOString(); | ||
|
|
||
| await adminClient | ||
| .from("webhook_dead_letter_queue") | ||
| .update({ | ||
| status: "pending", | ||
| retry_count: nextRetryCount, | ||
| next_retry_at: nextRetryDate, | ||
| error_reason: `retry_scheduled: ${refreshRes.status}`, | ||
| updated_at: finishNow, | ||
| }) | ||
| .eq("id", item.id); | ||
| result.retried++; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled per-item exceptions leave rows stuck at processing; update errors aren't checked.
The whole loop body runs under the outer try/catch (87-171) but has no per-item try/catch. If refreshProfile throws (its own DB write path isn't wrapped in try/catch), the row was already flipped to processing and the exception unwinds past the finalize step — that row is now permanently excluded from the status='pending' retry query with no reset/timeout. Separately, none of the four .update() calls in this loop (108-115, 121-127, 134-142, 151-160) inspect the returned { error }, so a failed write is silently counted as success/retry in result.
🔧 Proposed fix: per-item isolation + error checks
for (const item of items as WebhookDeadLetterRow[]) {
result.processed++;
const currentNow = new Date().toISOString();
-
- // Lock row to processing status
- await adminClient...
-
- const refreshRes = await refreshProfile(item.username);
-
- if (refreshRes.status === "refreshed") {
- ...
- } else {
- ...
- }
+ try {
+ // ... claim, refresh, and finalize as before, checking `error` on each update
+ } catch (itemErr) {
+ console.error("[webhook-dead-letter] item processing failed", { id: item.id, itemErr });
+ await adminClient
+ .from("webhook_dead_letter_queue")
+ .update({ status: "pending", updated_at: new Date().toISOString() })
+ .eq("id", item.id);
+ result.failed++;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/webhook-dead-letter.ts` around lines 103 - 164, Update the loop
handling in the dead-letter processing function to isolate each item with a
per-item try/catch, ensuring exceptions from refreshProfile or status updates do
not abort the batch and leave rows in processing. Check the returned error from
every webhook_dead_letter_queue update, and route any failure through the item’s
failure/recovery handling before updating result counters; preserve the existing
completed, failed, and pending retry transitions for successful writes.
| create table if not exists public.webhook_dead_letter_queue ( | ||
| id uuid primary key default gen_random_uuid(), | ||
| event_type text not null, | ||
| username text not null, | ||
| payload jsonb not null default '{}'::jsonb, | ||
| error_reason text, | ||
| status text not null default 'pending' check (status in ('pending', 'processing', 'completed', 'failed')), | ||
| retry_count integer not null default 0, | ||
| max_retries integer not null default 5, | ||
| received_at timestamptz not null default now(), | ||
| last_retry_at timestamptz, | ||
| next_retry_at timestamptz not null default now(), | ||
| created_at timestamptz not null default now(), | ||
| updated_at timestamptz not null default now() | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Raw webhook payload persisted indefinitely with no retention/redaction policy.
payload jsonb stores the entire GitHub webhook body (push events include committer/author emails and other PII) with no TTL or cleanup path for completed/failed rows. RLS is correctly locked to service-role-only access, but that only limits who can read it, not how long it's retained. Consider either redacting sensitive fields before insert or adding a scheduled cleanup for terminal-status rows past a retention window.
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 12-12: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
[warning] 13-13: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@supabase/migrations/20260726000001_add_webhook_dead_letters.sql` around lines
5 - 19, The webhook_dead_letter_queue schema retains complete payloads
indefinitely; add a retention mechanism for terminal completed/failed rows, such
as an indexed scheduled cleanup with a defined retention window, and redact
sensitive webhook fields before persistence where applicable. Keep pending and
processing retry data available and preserve service-role-only access.
PRODHOSH
added
ELUSOC
|
🎉 Your PR just got merged, @Aditya8369 — thank you for contributing to OSSfolio! Your work is now part of the project. Here's what to do next:
We really appreciate you taking the time. See you in the next PR! 🚀 |
2 participants
Summary
Related Issue
Closes #557
Type of Change
Changes Made
to automatically record unhandled payload events during transient lock contention and process them on schedule.
Checklist
mainconsole.logleft insrc/schema.sqland a new migration file are includedDESIGN.mdand followed the design system (colors, spacing, typography, components)feat:,fix:,docs:, etc.)Summary by CodeRabbit
New Features
Bug Fixes
Tests