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

refactor(server): move host filtering to tower middleware #1179

Merged
merged 11 commits into from
Aug 11, 2023

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Aug 11, 2023

This PR moves the HTTP host filter code to tower middleware and it's optional to use.

It breaks the functionality somewhat as the old code checked that host was included in the request even when host filtering were disabled and now jsonrpsee doesn't check anything in the host header or URI when host filtering is not enabled.

It breaks the current API and the host filtering can only be enabled as middleware in this PR.

Close #868

@niklasad1 niklasad1 requested a review from a team as a code owner August 11, 2023 09:04
Comment on lines +8 to +9
pub use host_filter::*;
pub use proxy_get_request::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Given that host_filter has types like Authority etc in, maybe it's best to expose each one in its own module so it's obvious which types are for what (especially if we ever add any more)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an uri mod and simplified the host_filter mod, lemme know if you are happy with it? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Def looks better now that the name Host is in all of the exposed things! Personally I liked that the Authority stuff lived here (because it's only used in this filter so keeping the code next to it felt good) but I'm easy either way :)

pub(crate) fn fetch_authority(request: &hyper::Request<hyper::Body>) -> Option<&str> {
let host_header = http_helpers::read_header_value(request.headers(), hyper::header::HOST);
let uri = request.uri().authority();
pub(crate) fn authority(request: &hyper::Request<hyper::Body>) -> Option<Authority> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this only used in the middleware now; I wonder whether it should move there?

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it to Authority::from_http_request I think that's cleaner

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks awesome!

Is it work adding an example for this new middleware to go along with the other middleware examples? :)

Copy link
Contributor

@tadeohepperle tadeohepperle 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 to me.

@niklasad1 niklasad1 merged commit 119d3ae into master Aug 11, 2023
@niklasad1 niklasad1 deleted the na-host-filtering-as-tower-middleware branch August 11, 2023 13:54
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.

Move host filtering to a tower layer
4 participants