Skip to content

Scope authenticated rate limits by credential - #1635

Merged
skyfallwastaken merged 6 commits into
mainfrom
fix/credential-rate-limit
Aug 27, 2026
Merged

Scope authenticated rate limits by credential#1635
skyfallwastaken merged 6 commits into
mainfrom
fix/credential-rate-limit

Conversation

@skyfallwastaken

@skyfallwastaken skyfallwastaken commented Aug 27, 2026

Copy link
Copy Markdown
Member

Summary of the problem

OAuth integrations sharing an IP address also share the general rate limit bucket, allowing one integration to rate limit requests made for unrelated Hackatime users.

Describe your changes

Give each authenticated Hackatime user under the OAuth API namespace a general throttle bucket shared across their OAuth tokens. Invalid, anonymous and all other requests remain scoped by IP address.

Screenshots / Media

Not applicable.

Copilot AI lite review requested due to automatic review settings August 27, 2026 11:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR changes the general Rack Attack discriminator so accessible OAuth bearer tokens under the authenticated API namespace are grouped by resource owner while invalid and anonymous requests remain grouped by IP.

  • Adds OAuth token resolution to the general throttle.
  • Adds discriminator tests for same-user and different-user tokens, invalid credentials, anonymous traffic and assets.

Confidence Score: 2/5

The PR is not safe to merge because wrong-scope credentials receive authenticated buckets and previously reported credential-isolation failures remain.

The current HEAD still maps every accessible OAuth token to its owner's bucket without checking endpoint scope, maps separate tokens for one user to the same bucket and leaves supported query credentials on shared IP buckets despite replies claiming those isolation issues were fixed.

Files Needing Attention: config/initializers/rack_attack.rb

Important Files Changed

Filename Overview
config/initializers/rack_attack.rb Adds OAuth-aware throttle bucketing, but wrong-scope tokens still receive authenticated buckets and previously reported credential-isolation failures remain.
test/lib/rack_attack_test.rb Adds direct discriminator coverage, including an assertion that same-user OAuth tokens share one bucket.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  R[Authenticated API request] --> B{Bearer token present?}
  B -- No --> I[IP throttle bucket]
  B -- Yes --> A{Token accessible?}
  A -- No --> I
  A -- Yes --> U[User throttle bucket]
  U --> C[Controller scope authorization]
  C -- Scope accepted --> O[Serve request]
  C -- Scope rejected --> F[Return insufficient-scope response]
Loading
Prompt To Fix All With AI
### Issue 1
config/initializers/rack_attack.rb:34-35
**Wrong-scope tokens bypass throttling**

When an accessible token lacks the scope required by an authenticated endpoint, this discriminator still assigns it to the resource owner's bucket before the controller rejects it, allowing callers with multiple unauthorized credentials to avoid the shared IP throttle.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (6): Last reviewed commit: "Share OAuth rate limits by user" | Re-trigger Greptile

Comment thread config/initializers/rack_attack.rb Outdated
Comment thread config/initializers/rack_attack.rb Outdated
Comment thread config/initializers/rack_attack.rb Outdated
Comment thread config/initializers/rack_attack.rb Outdated
Comment thread config/initializers/rack_attack.rb Outdated
Comment thread config/initializers/rack_attack.rb Outdated
Comment on lines +52 to +62
case normalized_path
when "/api/v1/authenticated/me"
[ [ :bearer ], false, true, [ "profile" ] ]
when "/api/v1/authenticated/hours", "/api/v1/authenticated/streak",
"/api/v1/authenticated/projects", "/api/v1/authenticated/heartbeats/latest"
[ [ :bearer ], false, true, [ "read" ] ]
when "/api/v1/authenticated/api_keys"
[ [ :bearer ], false, true, nil ]
when %r{\A/api/hackatime/v1/}
[ %i[bearer basic query], true, false, nil ]
when "/api/v1/my/heartbeats", "/api/v1/my/heartbeats/most_recent"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Format suffixes share buckets

When clients with distinct valid bearer credentials request an authenticated route such as /api/v1/authenticated/me.json from the same IP, the exact path policy does not recognize the Rails format suffix and falls back to the shared IP bucket, causing one client to exhaust another client's allowance and produce unrelated 429 responses.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: config/initializers/rack_attack.rb
Line: 52-62

Comment:
**Format suffixes share buckets**

When clients with distinct valid bearer credentials request an authenticated route such as `/api/v1/authenticated/me.json` from the same IP, the exact path policy does not recognize the Rails format suffix and falls back to the shared IP bucket, causing one client to exhaust another client's allowance and produce unrelated 429 responses.

**Knowledge Base Used:**
- [Developer API and authentication](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/developer-api-and-authentication.md)
- [API key management](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-key-management.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Member Author

Narrowed the change after maintainer feedback. The generic credential policy duplicated controller authentication and was too complex for this initializer. This now handles only the observed OAuth integration case under /api/v1/authenticated/; every other request retains the existing IP discriminator.

Copy link
Copy Markdown
Member Author

Updated the primary discriminator to the authenticated Hackatime user, matching GitHub’s user-token model. Multiple OAuth tokens for one user now share the allowance while different users behind the same integration host remain isolated.

Comment on lines +34 to +35
oauth_token = Doorkeeper::AccessToken.by_token(token)
"user:#{oauth_token.resource_owner_id}" if oauth_token&.accessible? && oauth_token.resource_owner_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Wrong-scope tokens bypass throttling

When an accessible token lacks the scope required by an authenticated endpoint, this discriminator still assigns it to the resource owner's bucket before the controller rejects it, allowing callers with multiple unauthorized credentials to avoid the shared IP throttle.

Knowledge Base Used: Developer API and authentication

Prompt To Fix With AI
This is a comment left during a code review.
Path: config/initializers/rack_attack.rb
Line: 34-35

Comment:
**Wrong-scope tokens bypass throttling**

When an accessible token lacks the scope required by an authenticated endpoint, this discriminator still assigns it to the resource owner's bucket before the controller rejects it, allowing callers with multiple unauthorized credentials to avoid the shared IP throttle.

**Knowledge Base Used:** [Developer API and authentication](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/developer-api-and-authentication.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We are intentionally not applying endpoint scopes to the primary rate-limit identity. An accessible OAuth token establishes the user identity; endpoint scope remains an authorisation concern in the controller. All accessible tokens for one resource owner share the same user bucket, so issuing multiple wrong-scope tokens does not multiply that user’s allowance. Different users receiving separate primary buckets is the intended GitHub-style model, with the existing API-wide IP throttle retained as the secondary ceiling.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're right — given the intended two-tier model, endpoint scopes should not participate in the primary identity. An accessible token, regardless of its endpoint scopes, maps to its resource owner, so issuing multiple wrong-scope tokens does not increase that user's 300/minute allowance. The separate IP-based API throttle remains the secondary ceiling. This invalidates my concern; I withdraw the finding.

@skyfallwastaken
skyfallwastaken merged commit c6708c8 into main Aug 27, 2026
17 of 19 checks passed
@skyfallwastaken
skyfallwastaken deleted the fix/credential-rate-limit branch August 27, 2026 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants