Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: addapiKeyMiddlewareOptional tor.Use(#21357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
jakehwll wants to merge6 commits intomain
base:main
Choose a base branch
Loading
fromjakehwll/20857-user-rate-limit-remediation

Conversation

@jakehwll
Copy link
Contributor

Closes#20857

This PR fixes rate limiting by user authentication. Previously, we weren't passingapiKeyMiddlewareOptional to the rate limit middleware, which meant theapiKey was never available in the rate limiter:

// Prioritize by user, but fallback to IP.apiKey,ok:=r.Context().Value(apiKeyContextKey{}).(database.APIKey)if!ok {returnhttprate.KeyByIP(r)}

Testing

Note

I've prepended(User) to the rate limit error messages inRateLimitByAuthToken() to distinguish them from the genericRateLimit() function (which shares the same error message, but to catch the error we uselimit+1 onRateLimit() whilst debugging). This ensures we're actually testing the user-based rate limiting.

Without bypass header (rate limited after 5 requests)

# Set rate limit to 5 requests per minuteexport CODER_API_RATE_LIMIT=5# Make 10 requests - should be limited after the 5thforiin {1..10};do   curl --request GET \    --url http://localhost:8080/api/v2/regions \    --header'Cookie: dev_coder_session_token=...'done
{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"message":"(User) You've been rate limited for sending more than 5 requests in 1m0s."}{"message":"(User) You've been rate limited for sending more than 5 requests in 1m0s."}{"message":"(User) You've been rate limited for sending more than 5 requests in 1m0s."}{"message":"(User) You've been rate limited for sending more than 5 requests in 1m0s."}{"message":"(User) You've been rate limited for sending more than 5 requests in 1m0s."}

Result: First 5 requests succeed, remaining 5 are rate limited with error message:"(User) You've been rate limited for sending more than 5 requests in 1m0s."

With bypass header (no rate limiting)

# Same setup with bypass headerforiin {1..10};do   curl --request GET \    --url http://localhost:8080/api/v2/regions \    --header'Cookie: dev_coder_session_token=...' \    --header'X-Coder-Bypass-Ratelimit: true'done
{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}{"regions":[{"id":"...","name":"primary","display_name":"Default","icon_url":"/emojis/1f3e1.png","healthy":true,"path_app_url":"http://127.0.0.1:3000","wildcard_hostname":""}]}

Result: All 10 requests succeed without rate limiting.

Copy link
Member

@ethanndicksonethanndickson left a comment

Choose a reason for hiding this comment

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

I know it's a draft, but we definitely should add a regression test for this

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ethanndicksonethanndicksonethanndickson left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@jakehwlljakehwll

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: rate limit by user is broken

3 participants

@jakehwll@ethanndickson

[8]ページ先頭

©2009-2025 Movatter.jp