standard: various nit and refactorings for standard stream wrappers - #22875
standard: various nit and refactorings for standard stream wrappers#22875Girgias wants to merge 4 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 :/ |
- Use newer zend_string APIs - Make logic more explicit and understandable - Prevent some strlen() recomputations
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);
|
Looks good to me. But it indeed can use some more tests. |
3 participants
No description provided.