fix(rate-limit): close an unauthenticated DoS, a permanently-stuck limiter, and two outbound backoff defects (#9469, #9493, #9494) - #9504
Merged
Conversation
…miter, and two outbound backoff defects (#9469, #9493, #9494) The inbound webhook bucket was keyed on installation.id read from the body BEFORE signature verification, with no IP component -- so anyone could pin a chosen installation's 10/60s window by POSTing {"installation":{"id":<victim>}} ten times a minute. GitHub's real deliveries then 429, and GitHub does not auto-redeliver, so the engine went deaf for that installation at a cost of one unauthenticated request every six seconds. The prior threat note correctly observed that a spoofed id grants no ACCESS; it missed that sharing the bucket IS the attack. Folding the connecting IP into the key keeps #4891's tenant separation (the installation id still splits brokered tenants behind one egress) while ensuring an attacker and GitHub never share a bucket. It also restores headroom #4891 removed by accident: GitHub delivers from many egress IPs, so a CI storm spreads across buckets again instead of contending for a single window. The Redis limiter's INCR-then-conditional-EXPIRE is two round trips: a death, dropped connection, or a failed EXPIRE between them left the key with no TTL, so the window never reset and every request on that key was denied until manual deletion -- with the -1 PTTL fallback papering over it by promising "retry in 60s" forever. One Lua script now makes the increment and its expiry atomic, and repairs an already-TTL-less key so anything stranded by the old path self-heals on next contact. Outbound, getAppInstallation recorded its App-JWT response headers under installation:{id}, but the JWT has its own 5000/hr bucket -- so refresh-installation-health wrote a healthy remaining count over a genuinely exhausted installation's newest observation every 30 minutes, re-admitting parked jobs to burn retries against 403s and re-park. It now records under a distinct app-jwt key. And a secondary-limit Retry-After (typically 60s) was truncated to the 8s inline cap and retried up to three times inside the window GitHub asked us to stay out of, which its own docs warn can extend the block; the response now surfaces so the queue's existing jittered Retry-After handling applies. The inline ladder is jittered too, so concurrent workers no longer retry in lockstep.
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-28 00:36:41 UTC
Review summary Nits — 7 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9504 +/- ##
==========================================
- Coverage 89.55% 88.65% -0.91%
==========================================
Files 843 843
Lines 110073 110082 +9
Branches 26194 26196 +2
==========================================
- Hits 98573 97590 -983
- Misses 10238 11520 +1282
+ Partials 1262 972 -290
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…9504/#9505 merges (#9509) * test(gate): cover the fail-safe marker-clear path (#9462) Mirrors the sibling visual-capture-satisfied fail-safe test: a rejecting clear must not throw out of the pass, since a failed clear only means the gate stays deferred until the head moves. (cherry picked from commit efa6472) * refactor(gate): drop an unreachable spread arm and annotate the defensive head-SHA guard (#9462) screenshotTableEvidenceUnresolved is computed as a plain boolean, so the undefined arm of its conditional spread was dead code -- pass it directly. The head-SHA guard in the exhausted branch is genuinely defensive (a recapture-preview job is only minted for a PR that had one), matching the sibling mark write, so it carries a reason rather than a contrived test. (cherry picked from commit 4f59499) * test(ai): exclude two unreachable best-effort cleanup arms from coverage (#9479) rm with force:true does not throw for a missing path, so the catch needs a filesystem-level failure no unit test can portably induce; the unset-cwd guard is only reachable when mkdtemp itself threw, i.e. no directory was ever created. Both annotated with their reason, matching the file's existing convention. (cherry picked from commit bf3efe6) * test(ai): drive the real subprocess EPIPE path and the non-Error stall guard (#9476, #9479) The EPIPE regression uses the existing real-subprocess harness rather than a stubbed spawn, so the stdin error listener is genuinely exercised: a fake CLI exits immediately without draining stdin, and the test asserts no uncaughtException reaches the process. Verified to fail without the listener (it captures an uncaught EPIPE), which is precisely the crash that took down every in-flight queue job in the container. Also covers isStalledNoOutput rejecting a non-Error throw -- a provider adapter can reject with a string, and misclassifying that as a deadline signal would silently skip the retry budget. (cherry picked from commit c770cc1) * test(rate-limit): cover the inline-retry break for an over-budget Retry-After (#9494) Asserts the attempt COUNT, not just the classification: a 60s Retry-After yields exactly one attempt instead of four, and a short one still uses the full inline budget so the break stays narrow. (cherry picked from commit 92813bf)
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
1 participant
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.
Summary
Three rate-limiting defects from the 2026-07-27 audit: one unauthenticated denial-of-service vector, one way the limiter can wedge permanently, and two ways outbound backoff misbehaves against GitHub.
Closes #9469
Closes #9493
Closes #9494
#9469 — an unauthenticated caller could silence a target installation's webhooks
/v1/github/webhookand/v1/orb/webhookkeyed their rate-limit bucket oninstallation.idread from the request body before signature verification, with no IP component. The class isstrict= 10 requests / 60 seconds.So anyone could POST
{"installation":{"id":<victim>}}ten times a minute — no credentials — and pin that installation's bucket. GitHub's genuine deliveries then receive 429 from the middleware (which runs ahead of the handler), and GitHub does not auto-redeliver, so the engine goes deaf for that installation. Cost to the attacker: one request every six seconds. Installation ids are low-entropy and discoverable.The existing threat note reasoned that a spoofed id "grants no access, since HMAC verification still gates everything downstream" — correct about access, but sharing the bucket is the attack.
The key now folds in the connecting IP. #4891's requirement is preserved: brokered tenants behind one shared egress still get independent buckets, because the installation id continues to separate them. It also restores headroom #4891 removed by accident — GitHub delivers from many egress IPs, so a CI storm spreads across per-IP buckets again instead of contending for a single 10/60s window (the second half of #9469's report).
#9493 — a TTL-less key denies its bucket forever
The Redis limiter did
INCR, thenEXPIREonly when the count came back as 1 — two round trips. A process death, a dropped connection, or anEXPIREthat merely errored between them left the key with no TTL: the fixed window never resets, the counter climbs forever, and once past the limit every request on that key is denied until someone deletes it by hand. The code even papered over the symptom — aPTTLof -1 fell back to a full window, so the 429 kept promising "retry in 60s" indefinitely.Worst case is
strict:/v1/github/webhook:installation:<id>: a stuck key means that installation's webhooks 429 forever.One Lua script now makes the increment and its expiry atomic. It also repairs a key that still has no TTL, so anything already stranded by the old path self-heals on next contact instead of needing manual intervention.
#9494 — outbound: a false-clear every 30 minutes, and a truncated Retry-After
App-JWT rows cleared real exhaustion backoffs.
getAppInstallationauthenticates with the App JWT, which has its own 5000/hr bucket — but recorded its response headers underinstallation:{id}. The admission readers take only the newest REST row per key, andrefresh-installation-healthruns unconditionally every 30 minutes. So an exhausted installation's parked jobs saw a healthyremaining≈4990, were re-admitted as a cohort, burned their retries against 403s, and re-parked — every 30 minutes for the whole exhaustion window. Now recorded under a distinctapp-jwtkey. #4506's requirement (these calls are visible to admission reads) is unchanged; only the attribution is fixed.Retry-After was truncated into the window GitHub asked us to avoid. A secondary-limit
Retry-Afteris typically 60s; the inline budget is 8s. The old code tookMath.min(retryAfter, 8s)and retried up to 3 times inside the window — exactly what GitHub's docs warn can extend or escalate a secondary block. When the instructed wait exceeds the inline budget the response now surfaces instead, so the queue's existing handling applies: it honors the fullRetry-Afterwith jitter and defers sibling jobs for the same admission target.The inline ladder is also jittered now. Up to
QUEUE_CONCURRENCY(default 8) workers can trip the same limit within milliseconds, and a fixed 500/1000/2000 ladder had them retry in lockstep.Tests
npx tsc --noEmitclean; 235 passed acrossselfhost-redis-ratelimit,auth,github-app,rate-limit,github-client.New:
Retry-Afterwithin budget is honored exactly, without jitter (a server instruction is authoritative); a 60sRetry-Afterreports as exceeding the inline budget while 3s does not, and absent/invalid headers do not; the invented backoff falls in its jittered range rather than a fixed value.Three existing tests asserted the old behaviour and were updated rather than deleted, each with the rationale inline:
#4506invariant test assertedadmissionKey: "installation:123"; it now asserts"app-jwt".The redis test's fake was upgraded from three stub methods to a TTL-aware fake that models the counter and its expiry (including the
-1"exists, no TTL" shape), so the repair arm is genuinely exercised rather than mocked away.Note
GITHUB_RATE_LIMIT_MAX_DELAY_MS(the 8s inline cap) is unchanged — the fix is to stop truncating into it, not to raise it. Long waits belong on the queue, which already has the machinery.