Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting headers in error response always appends #475

Open
jeremybower opened this issue Jun 10, 2024 · 2 comments
Open

Setting headers in error response always appends #475

jeremybower opened this issue Jun 10, 2024 · 2 comments
Labels
question Further information is requested

Comments

@jeremybower
Copy link

jeremybower commented Jun 10, 2024

@danielgtaylor @victoraugustolls Thanks for the prototype solution in #386 and #387 for setting headers in error responses.

While it worked well initially, I've hit a case where I don't see solution. I'm happy to open a PR, but would appreciate your thoughts on the right approach to solve this specific issue. I know there are thoughts around a much more robust approach. At this point, I'm only trying to solve this narrow problem.

I'm implementing a route-specific secondary rate limiter (after a general IP-based limiter as middleware) that exponentially increases the required time between password resets based on the POSTed email address.

The issue is that, unlike headers in output structs, headers in errors are always appended. This means that I can't override the headers set by the general limiter with the headers set by the more restrictive limiter. The response with duplicate headers looks like this:

HTTP/1.1 429 Too Many Requests
Connection: close
Content-Type: application/problem+json
Link: </api/core/v1/schemas/ErrorModel.json>; rel="describedBy"
Retry-After: Mon, 10 Jun 2024 13:37:46 UTC
Retry-After: Mon, 10 Jun 2024 13:38:47 UTC
X-Ratelimit-Limit: 100
X-Ratelimit-Limit: 1
X-Ratelimit-Remaining: 98
X-Ratelimit-Remaining: 0
X-Ratelimit-Reset: 2024-06-10T13:37:47Z
X-Ratelimit-Reset: 2024-06-11T13:37:46Z
X-Ratelimit-Retry: 2024-06-10T13:37:46Z
X-Ratelimit-Retry: 2024-06-10T13:38:47Z

Ideally, in this case, I could do something like the array approach documented for output structs -- override if a single value and append when an array. But I understand that in the general case it makes more sense to alway append error headers to preserve information that might be useful when troubleshooting.

It seems that:

  • There is no way to control appending vs overriding error headers.
  • There is no way to access headers in the handler to remove duplicates since the request isn't available.
  • A transformer can't be used to remove duplicate headers since there is no way to the read the response headers (only the request headers) from the Huma context.

Other than a framework-level enhancement, the only things I can think of doing are:

  • Move the secondary limiter into a middleware and parse the body before Huma does to get at the properties I need to configure the secondary limiter attempt. In addition to parsing the body twice, it would not benefit from Huma's validation and it would separate part of the logic from the handler into a middleware.
  • Wrap the Huma context in the middleware to override AppendHeader() with SetHeader() for specific headers.

Thoughts?

@danielgtaylor danielgtaylor added the question Further information is requested label Jun 12, 2024
@danielgtaylor
Copy link
Owner

@jeremybower I'm still wrapping my head around this problem. Maybe you can make a small example like https://go.dev/play/p/xrlGxyVZBge to reproduce it?

That said, I wonder if you can use operation metadata to just tell the middleware when a handler will set its own rate limiting info into the response, something like https://go.dev/play/p/d_dj-_UMKM1

@jeremybower
Copy link
Author

jeremybower commented Jul 4, 2024

@danielgtaylor Here's an example of the primary and secondary rate limiter use case: https://go.dev/play/p/zOulOSxaFWL

Please let me know if I'm misunderstanding the suggestion about using operation metadata, but it seems this won't work because the primary rate limiter must always be enabled so that invalid requests that don't reach the secondary rate limiter still count against the rate limiting quota (as shown in the example with an invalid email address).

Another thing I'll point out about this example, which is purely subjective, is that there are three different ways to set the same header:

  • Middleware: ctx.SetHeader("X-Ratelimit-Remaining", "0")
  • Error: http.Header{"X-Ratelimit-Remaining": []string{"0"}}
  • Output: PasswordResetOutput{HeaderXRateLimitRemaining: "0"}

I can understand the middleware operating at a lower level with the Huma context, but it seems like the handler should be consistent about how headers are set. If I continue to pull on that thread, then it leads me to errors being similar to output structs where headers can be set in the same way.

Thanks again for your thoughts on this. I'm really enjoying using Huma.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants