Fix bug #62524, only follow redirects in file streams for 3xx HTTP statuses #236

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
4 participants

1stvamp commented Nov 24, 2012

See https://bugs.php.net/bug.php?id=62524.

Discovered this also while debugging a problem in Composer during Github API auth token retrieval, Github return status 210 for a created token with the canonical URI for the token in the "Location" header.

This simple patch just checks the bounds of the HTTP status, when StreamContext says fopen et al should follow Location, are =>300 && <400.

Contributor

weltling commented Nov 26, 2012

That case is a bit more complicated as at least 305 status should have special handling. 300 and 304 might also deserve more attention. Also, the case with 3xx status and no location header should be handled IMHO.

1stvamp commented Nov 26, 2012

@weltling I don't think anything needs to be done if a location header isn't present for a 3xx status as the expected behaviour re the specs in that case is that you get the page body and handle the 3xx in your own application. We could throw an error of some variety, but other parts of fopen don't in similar circumstances.

You could argue the same for 300, Wikipedia suggests 305 may not even be handled by browsers:
http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection

I'm unsure about how to handle 304.

Contributor

weltling commented Nov 26, 2012

Ok, that's right, they always say SHOULD have location. So if there is none, userspace will get the original response (no redirection).

Regarding 300 - according to the specs it's completely valid to just pick the first best location. Where 305 (in the light of what they say about security) is a specific case and shouldn't be redirected as the location points to the proxy, not to any http destination.

304 as i get it is about a request made using if-modified-since and/or e-tag headers. The response even isn't supposed to contain the body. In fact here is nothing about redirection.

I'd say it'd be rather make sense to check against a white list, excluding 305 and 304. That are effective 300, 301, 302, 303 and 307 statuses. It'd be also more efficient to move the response code check to the start of the conditions, so it even don't gets to the php_stream_context_get_option() if there is no redirection.

1stvamp commented Nov 26, 2012

@weltling 👍 aye that makes sense, I'll make up a patch later.

Contributor

lstrojny commented Jan 14, 2013

@1stvamp ping

1stvamp commented Jan 15, 2013

@lstrojny pong, and patched.

Contributor

lstrojny commented Jan 15, 2013

@weltling can you verify again?

Contributor

weltling commented Jan 15, 2013

@lstrojny @1stvamp yep, imho that should be

(response_code >= 300 && response_code < 304 || 307 == response_code)

then it were complete (see my prev comment)

1stvamp commented Jan 15, 2013

Ah good point, forgot about 307 when I was trying to come up with a good test.
Will fix now.

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

|| 307 == response_code

1stvamp commented Jan 15, 2013

Done.

ping @weltling @lstrojny

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

|| 307 == response_code

Contributor

weltling commented Jan 15, 2013

@1stvamp ups, that has to be literally in the brackets

if ((response_code >= 300 && response_code < 304 || 307 == response_code) && ......)

otherwise it doesn't what you want

1stvamp commented Jan 15, 2013

@weltling doh, just me being tired and stupid, not had enough coffee yet. :)

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

@lstrojny (https://github.com/lstrojny) @1stvamp (https://github.com/1stvamp) yep, imho that should be
(response_code >= 300 && response_code < 304 || 307 == response_code)
then it were complete (see my prev comment)


Reply to this email directly or view it on GitHub (#236 (comment)).

Contributor

weltling commented Jan 15, 2013

@1stvamp yep, now it's atomic
Ah, just another small thing - to be compatible with C90 the response_code declaration has to be moved to the top of the block. In this case to the top of the function, as far as i can see.

1stvamp commented Jan 15, 2013

@weltling done :)

Wes

On Tuesday, 15 January 2013 at 9:59 AM, Wes Mason wrote:

@weltling doh, just me being tired and stupid, not had enough coffee yet. :)

Wes

On Tuesday, 15 January 2013 at 9:27 AM, Anatoliy Belsky wrote:

@lstrojny (https://github.com/lstrojny) @1stvamp (https://github.com/1stvamp) yep, imho that should be
(response_code >= 300 && response_code < 304 || 307 == response_code)
then it were complete (see my prev comment)


Reply to this email directly or view it on GitHub (#236 (comment)).

Contributor

weltling commented Jan 15, 2013

@1stvamp @lstrojny fine now for all i care :)

Comment on behalf of stas at php.net:

merged

@php-pulls php-pulls closed this Jan 29, 2013

@1stvamp 1stvamp deleted the 1stvamp:bug62524 branch Jan 29, 2013

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