Skip to content

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.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually in hindsight I disagree with this partially - in Vapor we'll be setting up the validator ourselves so will need to set up a pipeline configuration option just to create this. Should we offer both APIs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, perhaps I'm not following where the issue is there. Can you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in Vapor when we construct our pipeline, we just create the handlers ourselves rather than using NIO's helpers - https://github.com/vapor/vapor/blob/main/Sources/Vapor/HTTP/Server/HTTPServer.swift#L605.

If the validator was to take a ChannelPipeline.SynchronousOperations.Configuration type, we'd need to construct the entire configuration object when we just want to be able to set the options for that specific handler directly. This might cause confusion if we created a configuration type that then wasn't used for another handler which would be misconfigured even though a dev would be setting the configuration object. So IMO it makes sense to have a configuration object for configureHTTPServerPipeline, but each handler should be able to be set up independent of that if desired - does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I was suggesting a configuration object only for this handler. That would then replace the mystery-meat boolean in the configureHTTPServerPipeline.

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