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

feat: add request limits on accept, content-type, and content-length headers #502

Merged
merged 7 commits into from
Sep 6, 2024

Conversation

stijnmoreels
Copy link
Contributor

Adds request guards/limitation on:

  • Accept header
  • Content-Type header
  • Content-Length header
    With user-friendly error http responses.

Closes #392

@stijnmoreels
Copy link
Contributor Author

I tried to follow the style guidelines and way of working, please let me know if I did anything wrong. Happy to help! 👍

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

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

Looks good from my end. I only have some questions that other maintainers (@dustinmoris) should weigh in on.

So far Giraffe doesn't have many error messages it returns that are hard coded. I could only find one place where we do it. I'm wary of having error messages like this hard coded for localization and even preference reasons. However I'm unsure of how to resolve this easily within this library.

@stijnmoreels
Copy link
Contributor Author

stijnmoreels commented Jan 4, 2022

Looks good from my end. I only have some questions that other maintainers (@dustinmoris) should weigh in on.

So far Giraffe doesn't have many error messages it returns that are hard coded. I could only find one place where we do it. I'm wary of having error messages like this hard coded for localization and even preference reasons. However I'm unsure of how to resolve this easily within this library.

Maybe we can have a different set of functions that takes in a custom error HTTP handler. There maybe needed two, as we check both on the presence of the header and whether the header has the correct value.

let haveContentTypesCustom contentTypes errorHandler = // ...

@dustinmoris
Copy link
Member

I'd prefer to make the first parameter of each of these handlers to accept an error handler. It's not just about changing the message, but the entire response should be flexible. For example an API could return this as a JSON object or plain text response but a website might want to render a HTML page with the error displayed.

Here's an example of where this approach has already been taken:
https://github.com/giraffe-fsharp/Giraffe/blob/master/src/Giraffe/Negotiation.fs#L159

Of course in the final user application one would define that handler only once and then use the new handler for everything else, e.g.:

let someErrorHandler = ...

let validateContentType = haveAnyContentTypes someErrorHandler contentTypes

// then use validateContentType everywhere else

@stijnmoreels
Copy link
Contributor Author

stijnmoreels commented Jul 8, 2022

Ok, that's fine by me! 👍 I'll change this to your needs.
Although, in the negotiation example, the exception handling (unacceptable handler) is passed not as the first parameter.

@64J0
Copy link
Member

64J0 commented Sep 6, 2023

This feature looks pretty cool!

DOCUMENTATION.md Outdated Show resolved Hide resolved
Co-authored-by: Vinícius Gajo <50725287+64J0@users.noreply.github.com>
@64J0
Copy link
Member

64J0 commented Jul 29, 2024

I'll review this PR either this or next week.

@64J0 64J0 added the feature request Request to add new functionality label Aug 11, 2024
…, change their implementation to accept OptionalErrorHandlers so the user can configure the response, and update DOCUMENTATION due to the changes
@64J0
Copy link
Member

64J0 commented Aug 11, 2024

I changed this branch's code to accept custom handlers from the user of this feature. And added more unit tests to validate these new scenarios.

This PR is adding some helper middlewares that will filter requests based on some header values. Do you think it makes sense to add it to Giraffe? I know that we have other helper middlewares as well, like:

What do you think? Pinging especially @dbrattli and @nojaf to know their opinion.

@64J0 64J0 requested review from dbrattli and nojaf August 11, 2024 16:25
Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Sorry for the late review here. This slipped off my radar and I don't feel very qualified to say much about this. I don't use Giraffe that often and that makes it hard to have any opinion about the API.

I would merge this in and publish an alpha.
Ask people if the API works for them and take it from there.

@64J0 64J0 merged commit 54f38a4 into giraffe-fsharp:master Sep 6, 2024
6 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request to add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request - request limitation
5 participants