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

webob port validation #271

Closed
faassen opened this Issue Nov 24, 2014 · 17 comments

Comments

Projects
None yet
2 participants
@faassen
Copy link
Member

faassen commented Nov 24, 2014

We should guard against non-number port numbers in the Host header. I'm hoping to implement this in WebOb itself, otherwise another middleware (this one, always on).

@faassen faassen added this to the 0.9 milestone Nov 24, 2014

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Nov 24, 2014

Split off from #260

@faassen faassen modified the milestones: 0.10, 0.9 Nov 25, 2014

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Nov 25, 2014

@faassen faassen modified the milestones: 0.11, 0.10 Apr 9, 2015

@faassen faassen modified the milestones: 0.12, 0.11 Jun 29, 2015

@faassen faassen modified the milestones: 0.13, 0.12 Jan 27, 2016

@faassen faassen modified the milestones: 0.14, 0.13 Apr 6, 2016

@faassen faassen removed this from the 0.14 milestone Apr 12, 2016

@href

This comment has been minimized.

Copy link
Member

href commented Apr 15, 2016

I'm interested in implementing this until WebOb has something. I think we can just add a tween under excview_tween_factory and be done with it. What do you think?

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 15, 2016

Should it be under or over excview_tween_factory?

Along the way could you also poke the webob issue? Perhaps they'll accept a pull request, but since that may take a while we can move forward here too.

@href

This comment has been minimized.

Copy link
Member

href commented Apr 15, 2016

Should it be under or over excview_tween_factory?

My first impulse was to put it under to make sure that the resulting exception can be caught in a view. But since it's a bad request we really shouldn't be touching it anyhow, so I'm now changing my vote to 'over' :)

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 15, 2016

Yeah, I think a 400 bad request that doesn't get an exception view is the right way to go. This really only happens if there are bad client bugs or deliberate exploitation.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 15, 2016

And we shouldn't raise the 400 bad request but just return it as the response in the tween.

@href

This comment has been minimized.

Copy link
Member

href commented Apr 15, 2016

I think we might just want to do what Django does and match the host header against a regex:
django/django@77b06e4

It's well tested and it doesn't come with a real performance downside as most of the time we would just check the same host over and over again, which is fine since Python caches regex matches internally up to a point.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 15, 2016

That sounds reasonable.

href added a commit that referenced this issue Apr 15, 2016

@href

This comment has been minimized.

Copy link
Member

href commented Apr 15, 2016

See my commit above. This should do it. Note that more.forwarded overrides the host later and is therefore potentially vulnerable. That's outside of Morepath's control.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 15, 2016

Thanks, won't have time anymore today, but I'll try to review this on monday.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 18, 2016

Okay, looks good. One thing we should also do is include this in the public API (init.py and documented in api.rst), so that a policy like more.forwarded could choose to insert itself over this one and benefit from it. Once you do this we can merge it.

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 18, 2016

(that is: you can merge it :) thanks!

@href

This comment has been minimized.

Copy link
Member

href commented Apr 18, 2016

Done, this can now be implemented in more.forwarded:

from morepath import HOST_HEADER_PROTECTION

@App.tween_factory(over=HOST_HEADER_PROTECTION)
...
@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 18, 2016

Yay, thank you very much!

@faassen faassen closed this Apr 18, 2016

@href

This comment has been minimized.

Copy link
Member

href commented Apr 18, 2016

You're welcome :)

@faassen

This comment has been minimized.

Copy link
Member

faassen commented Apr 18, 2016

I've adjusted more.forwarded to work with the new tween.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment