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

request.host_port is a str not an int #2512

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

dstufft
Copy link
Contributor

@dstufft dstufft commented Apr 17, 2016

This fixes a logic error in #2501 where I accidentally treated request.host_port as an int when in reality it is a str. This wasn't exposed in my own testing because I was only using non-standard ports so I expected this to evaluate to False anyways.

@dstufft
Copy link
Contributor Author

dstufft commented Apr 17, 2016

Note, this essentially means the new automatic CSRF checking is broken on any site that uses standard ports right now.

@mmerickel
Copy link
Member

Wow.. request.server_port is documented to an int and request.host_port is a string. What?

@dstufft
Copy link
Contributor Author

dstufft commented Apr 17, 2016

Yea, I don't know :(

@sontek
Copy link
Member

sontek commented Apr 17, 2016

Rather than having the list be strings could you cast host_port to an int and use that? It would make sure if host_port ever got changed to an int that the code would continue to work.

if int(request.host_port) not in {80, 443}:

@dstufft
Copy link
Contributor Author

dstufft commented Apr 17, 2016

I could do that. The reason I didn't was that I wasn't sure if it'd ever be possible for it to be something that isn't a valid int (empty string?).

Sent from my iPhone

On Apr 17, 2016, at 2:08 PM, John Anderson notifications@github.com wrote:

Rather than having the list be strings could you cast host_port to an int and use that? It would make sure if host_port ever got changed to an int that the code would continue to work.

if int(request.host_port) not in {80, 443}:

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@digitalresistor
Copy link
Member

Wondering if this is something to be fixed in WebOb...

@mmerickel
Copy link
Member

I doubt you can just switch the type without causing people unnecessary pain. It'd be nice to know if there's a reason it's a string though.

@digitalresistor
Copy link
Member

It's pulled from HTTP_HOST header, by slicing and dicing the string:

http://docs.webob.org/en/stable/api/request.html#webob.request.BaseRequest.host_port

I don't know if there is a reason for it being string over not.

@mmerickel
Copy link
Member

On topic, I'm planning to merge this PR. Just bundling it together with a larger change that hopefully I can make later tonight.

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.

4 participants