standard: various nit and refactorings for standard stream wrappers - #22875
standard: various nit and refactorings for standard stream wrappers#22875Girgias wants to merge 3 commits into
Conversation
| @@ -1128,7 +1125,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper, | |||
| } \ | |||
| } | |||
| /* check for control characters in login, password & path */ | |||
| if (strncasecmp(new_path, "http://", sizeof("http://") - 1) || strncasecmp(new_path, "https://", sizeof("https://") - 1)) { | |||
| if (zend_string_starts_with_literal_ci(new_path, "http://") || zend_string_starts_with_literal_ci(new_path, "https://")) { | |||
There was a problem hiding this comment.
it seems the old condition was always true regardless ? I would either drop the "migrated" version of it or
if (!zend_string_starts_with_literal_ci(new_path, "http://") && !zend_string_starts_with_literal_ci(new_path, "https://"))
```. wdyt ?
There was a problem hiding this comment.
I'm... not even sure what the semantics are. Reading the rest of the code it does seem to me that it expects the path to start with an HTTP protocol schema....
As we are passing new_path to php_stream_url_wrap_http_ex
|
Seems your PR resolves a relative Location (e.g. Location: baz on /foo/bar) to /foo/baz instead of master's /foo//baz, because it no longer inserts a / separator after a path prefix that already ends in one. |
So I guess this PR fixes another pre-existing bug as well, and clearly this part of the stream wrapper is barely used and badly tested :/ |
88c225c to
766bdd8
Compare
| /* check for control characters in login, password & path */ | ||
| if (strncasecmp(new_path, "http://", sizeof("http://") - 1) || strncasecmp(new_path, "https://", sizeof("https://") - 1)) { | ||
| if (zend_string_starts_with_literal_ci(new_path, "http://") || zend_string_starts_with_literal_ci(new_path, "https://")) { | ||
| CHECK_FOR_CNTRL_CHARS(resource->user); |
There was a problem hiding this comment.
This checks for control characters in the output from php_uri_parse_to_struct. When using the URL default parser, the control characters have already been replaced by underscores when we get here (php_replace_controlchars in php_url_parse_ex2). When setting the parser to Uri\Rfc3986\Uri, the URL fails to parse. With Uri\WhatWg\Url, it is possible to get control characters in the URL.
The check used to be performed for all URLs, but now is only performed for http(s) URLs. So this makes it possible to redirect to e.g. FTP URLs that contain control characters.
But perhaps it should be up to the URL parser to decide how to handle control characters? If WhatWg thinks URLs can have control characters, should we really discard them here?
Edit: I tested this with this on the server:
<?php
header("Location: ftp://u\vser:pass@localhost:41303/");And this as the client:
<?php
$context = stream_context_create([
"http" => [
"uri_parser_class" => \Uri\WhatWg\Url::class,
]
]);
file_get_contents("http://localhost:41303/", false, $context);There was a problem hiding this comment.
I guess this makes sense, but might leave this for a follow-up PR and do this after 8.6 is branched as this might be contentious?
There was a problem hiding this comment.
Yes, I agree. There's no need to change that in this PR.
|
Looks good to me. But it indeed can use some more tests. |
766bdd8 to
bc1aa5e
Compare
|
@Sjord do you know why I get a different result for the redirection than your test? Not sure where I went wrong during the rebase. |
|
Because you accidentally fixed the bug described in #23532? My test tests for the "wrong" behavior of the double slash. Is that what you meant? |
Right, I'll update the test then :) |
- Use newer zend_string APIs - Make logic more explicit and understandable - Prevent some strlen() recomputations
bc1aa5e to
3aaadc6
Compare
3 participants
No description provided.