Skip to content

Return response when header validation fails #3346

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

0xTim
Copy link
Contributor

@0xTim 0xTim commented Aug 13, 2025

Allow the NIOHTTPResponseHeadersValidator to be configured to return a response if the header validation fails. This allows something to be returned to client and can fix issues where proxies might mark a node as bad if a RST is returned. This has to be done in the validator since all future writes are dropped so any error handler won't be able to catch the error and write out a fallback response.

I haven't touched trailer validation, not sure if you want me to look at that. I don't know if an empty end would be valid as other data would have been already written to the channel.

@0xTim 0xTim marked this pull request as ready for review August 13, 2025 11:40
self.sendResponseOnInvalidHeader = false
}

public init(sendResponseOnInvalidHeader: Bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should create a configuration structure. That would let us extend this again in future without needing to provide more initializers and more pipeline builders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to add that in. Would you envisage a config type with an initialiser that takes no arguments and each one can be set individually?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that seems sensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok updated - I've only added the new configuration option to avoid an explosion of deprecations and confusing APIs. And I'm sure we need to refine the naming

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should pass the new config field to this initializer.

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