feat: add `apiKeyMiddlewareOptional` to `r.Use(` #21357

jakehwll · 2025-12-22T05:47:40Z

Closes #20857

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

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

Testing

Note

I've prepended (User) to the rate limit error messages in RateLimitByAuthToken() to distinguish them from the generic RateLimit() function (which shares the same error message, but to catch the error we use limit+1 on RateLimit() 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 minute
export CODER_API_RATE_LIMIT=5

# Make 10 requests - should be limited after the 5th
for i in {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 header
for i in {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.

ethanndickson

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

jakehwll · 2025-12-23T02:56:16Z

coderd/coderd_test.go

+ // Test that /api/experimental works with authentication
+ t.Run("APIExperimentalWithAuth", func(t *testing.T) {
+ t.Parallel()
+
+ ctx := testutil.Context(t, testutil.WaitLong)
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, client.URL.String()+"/api/experimental", nil)
+ require.NoError(t, err)
+ req.Header.Set(codersdk.SessionTokenHeader, client.SessionToken())
+
+ resp, err := client.HTTPClient.Do(req)
+ require.NoError(t, err)
+ defer resp.Body.Close()
+
+ require.Equal(t, http.StatusOK, resp.StatusCode, "should succeed with auth")
+	})


I wanted to add a regression test for Experimental routes other than / but I don't think there is anything that will be long-lived here (being that we either promote or remove).

Thoughts?

Makes sense, / is the only thing that won't get promoted eventually.

jakehwll · 2025-12-23T02:57:46Z

coderd/coderd.go

 apiRateLimiter,
 httpmw.ReportCLITelemetry(api.Logger, options.Telemetry),
 		)
+ r.Get("/", apiRoot)


Added an apiRoot here so we can test against something long-lived. Same as the root api/v2/ handler

$ curl http://localhost:8080/api/experimental {"message":"👋"}

spikecurtis

There are 2 other instances of the rate limit handler being used, currently lines 892 and 915 -- these are for workspace app routes and won't have the user identity available.

spikecurtis · 2025-12-23T06:41:31Z

coderd/coderd.go


 r.NotFound(func(rw http.ResponseWriter, _ *http.Request) { httpapi.RouteNotFound(rw) })
 r.Use(
+ apiKeyMiddlewareOptional,


This needs a more significant refactor than just adding apiKeyMiddlewareOptional at the top of the stack of middlewares.

The problem is that when we add apiKeyMiddleware deeper in the route tree, that middleware restarts the whole process of extracting the API key, including making another database call.

Instead we need an architecture where we always attempt to extract the API key at the top of the route tree, and then deeper in the tree, when we want to make it required, we just check that the identity has been added to the context.

feat: add apiKeyMiddlewareOptional to r.Use(

889d09c

github-actions bot assigned jakehwll Dec 22, 2025

jakehwll added 2 commits December 22, 2025 17:36

Merge branch 'main' into jakehwll/20857-user-rate-limit-remediation

bbac087

chore: commit bump (ci)

5660238

ethanndickson reviewed Dec 22, 2025

View reviewed changes

jakehwll added 4 commits December 22, 2025 11:31

feat: add regression test

e1aa97a

fix: resolve lint error

ab08b25

fix: remove useless test

2dbac5b

feat: add / route for /api/experimental + regression test

991c88f

jakehwll commented Dec 23, 2025

View reviewed changes

jakehwll marked this pull request as ready for review December 23, 2025 02:56

Merge branch 'main' into jakehwll/20857-user-rate-limit-remediation

c6e94f5

jakehwll requested review from ethanndickson, kacpersaw and spikecurtis December 23, 2025 02:56

jakehwll commented Dec 23, 2025

View reviewed changes

spikecurtis requested changes Dec 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add `apiKeyMiddlewareOptional` to `r.Use(` #21357

feat: add `apiKeyMiddlewareOptional` to `r.Use(` #21357

Uh oh!

jakehwll commented Dec 22, 2025

Uh oh!

ethanndickson left a comment

Uh oh!

jakehwll Dec 23, 2025

Uh oh!

spikecurtis Dec 23, 2025

Uh oh!

jakehwll Dec 23, 2025 •

edited

Loading

Uh oh!

spikecurtis left a comment

Uh oh!

spikecurtis Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: add apiKeyMiddlewareOptional to r.Use( #21357

Are you sure you want to change the base?

feat: add apiKeyMiddlewareOptional to r.Use( #21357

Uh oh!

Conversation

jakehwll commented Dec 22, 2025

Testing

Without bypass header (rate limited after 5 requests)

With bypass header (no rate limiting)

Uh oh!

ethanndickson left a comment

Choose a reason for hiding this comment

Uh oh!

jakehwll Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

spikecurtis Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

jakehwll Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spikecurtis left a comment

Choose a reason for hiding this comment

Uh oh!

spikecurtis Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: add `apiKeyMiddlewareOptional` to `r.Use(` #21357

feat: add `apiKeyMiddlewareOptional` to `r.Use(` #21357

jakehwll Dec 23, 2025 •

edited

Loading