Skip to content

fix: enforce rate limit on all webhook events regardless of installation ID#409

Merged
Ayush-Patel-56 merged 2 commits into
Coder-s-OG-s:mainfrom
diksha78dev:fix/issue-391
Jun 24, 2026
Merged

fix: enforce rate limit on all webhook events regardless of installation ID#409
Ayush-Patel-56 merged 2 commits into
Coder-s-OG-s:mainfrom
diksha78dev:fix/issue-391

Conversation

@diksha78dev

Copy link
Copy Markdown
Contributor

Summary

Fixed a rate limiting bypass vulnerability where GitHub webhooks lacking an installation.id (such as meta or security_advisory events) completely skipped the rate limiter.

Type of Change

  • Bug fix
  • New feature
  • UI / UX improvement
  • Refactor
  • Documentation
  • Other

Related Issue

Closes #391

What was changed?

  • Removed the conditional check if (installationId) that skipped rate-limiting.
  • Added a fallback rate limit key using the x-forwarded-for header for events that do not contain an installation.id.
  • Webhook endpoints are now unconditionally rate-limited.

Screenshots

N/A

Checklist

  • My code follows the project structure and conventions
  • I tested this locally (npm run dev & npm run test)
  • No hardcoded secrets or credentials
  • I have updated documentation if needed

@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@diksha78dev is attempting to deploy a commit to the codersogs-3057's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Ayush-Patel-56 Ayush-Patel-56 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

x-forwarded-for is taken as the raw header value, but that's attacker-controlled in this exact threat model (leaked secret, forged requests) and can have multiple comma-separated hops. Need to confirm Vercel doesn't just append to it, and parse out the trusted IP rather than using the raw string. Also no test for the new fallback path.

@Ayush-Patel-56 Ayush-Patel-56 added the Needs author reply Author need to reply label Jun 24, 2026
@diksha78dev

Copy link
Copy Markdown
Contributor Author

@Ayush-Patel-56 Thanks for review.Relying on x-forwarded-for as a fallback leaves us vulnerable to IP spoofing since attackers can manipulate those headers.
I've pushed a commit that completely removes the IP-based rate limiting fallback. Instead, the rate limit now falls back to a global bucket scoped to the specific eventType (e.g., global:meta). Since uninstalled events happen very infrequently, a global limit of 100/minute is generous enough for normal operation but completely immune to IP spoofing during an attack. I also added a test for this fallback path

@Ayush-Patel-56 Ayush-Patel-56 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Solid fix. LGTM. Thanks!

@Ayush-Patel-56 Ayush-Patel-56 added level:critical Critical level difficulty quality:exceptional Exceptional quality contribution type:bug Bug fix gssoc:approved Approved by GSSOC admin mentor:Ayush-Patel-56 Replace Ayush-Patel-56 with mentor's GitHub handle to credit them nsoc26 SSoC26 level3 Hard and removed Needs author reply Author need to reply labels Jun 24, 2026
@Ayush-Patel-56 Ayush-Patel-56 merged commit 0f2d321 into Coder-s-OG-s:main Jun 24, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gssoc:approved Approved by GSSOC admin Hard level:critical Critical level difficulty level3 mentor:Ayush-Patel-56 Replace Ayush-Patel-56 with mentor's GitHub handle to credit them nsoc26 quality:exceptional Exceptional quality contribution SSoC26 type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Webhook rate limiting is skipped for event types without an installation field

2 participants