Skip to content

Commit

Permalink
Fix GH-11274: POST/PATCH request via file_get_contents + stream_conte…
Browse files Browse the repository at this point in the history
…xt_create switches to GET after a HTTP 308 redirect

RFC 7231 states that status code 307 should keep the POST method upon
redirect. RFC 7538 does the same for code 308. Although it's not
mandated by the RFCs that PATCH is also kept (we can choose), it seems
like keeping PATCH will be the most consistent and understandable behaviour.

This patch also changes an existing test because it was testing for the
wrong behaviour.

Closes GH-11275.
  • Loading branch information
nielsdos committed May 19, 2023
1 parent aa061cd commit 1ede313
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 7 deletions.
2 changes: 2 additions & 0 deletions NEWS
Expand Up @@ -46,6 +46,8 @@ PHP NEWS
- Standard:
. Fixed bug GH-11138 (move_uploaded_file() emits open_basedir warning for
source file). (ilutov)
. Fixed bug GH-11274 (POST/PATCH request switches to GET after a HTTP 308
redirect). (nielsdos)

- Streams:
. Fixed bug GH-10031 ([Stream] STREAM_NOTIFY_PROGRESS over HTTP emitted
Expand Down
21 changes: 15 additions & 6 deletions ext/standard/http_fopen_wrapper.c
Expand Up @@ -79,6 +79,7 @@

#define HTTP_WRAPPER_HEADER_INIT 1
#define HTTP_WRAPPER_REDIRECTED 2
#define HTTP_WRAPPER_KEEP_METHOD 4

static inline void strip_header(char *header_bag, char *lc_header_bag,
const char *lc_header_name)
Expand Down Expand Up @@ -140,6 +141,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
char *user_headers = NULL;
int header_init = ((flags & HTTP_WRAPPER_HEADER_INIT) != 0);
int redirected = ((flags & HTTP_WRAPPER_REDIRECTED) != 0);
int redirect_keep_method = ((flags & HTTP_WRAPPER_KEEP_METHOD) != 0);
bool follow_location = 1;
php_stream_filter *transfer_encoding = NULL;
int response_code;
Expand Down Expand Up @@ -363,8 +365,8 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
if (context && (tmpzval = php_stream_context_get_option(context, "http", "method")) != NULL) {
if (Z_TYPE_P(tmpzval) == IS_STRING && Z_STRLEN_P(tmpzval) > 0) {
/* As per the RFC, automatically redirected requests MUST NOT use other methods than
* GET and HEAD unless it can be confirmed by the user */
if (!redirected
* GET and HEAD unless it can be confirmed by the user. */
if (!redirected || redirect_keep_method
|| zend_string_equals_literal(Z_STR_P(tmpzval), "GET")
|| zend_string_equals_literal(Z_STR_P(tmpzval), "HEAD")
) {
Expand Down Expand Up @@ -458,7 +460,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
zend_str_tolower(ZSTR_VAL(tmp), ZSTR_LEN(tmp));
t = ZSTR_VAL(tmp);

if (!header_init) {
if (!header_init && !redirect_keep_method) {
/* strip POST headers on redirect */
strip_header(user_headers, t, "content-length:");
strip_header(user_headers, t, "content-type:");
Expand Down Expand Up @@ -606,7 +608,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
* see bug #44603 for details. Since Content-Type maybe part of user's headers we need to do this check first.
*/
if (
header_init &&
(header_init || redirect_keep_method) &&
context &&
!(have_header & HTTP_HEADER_CONTENT_LENGTH) &&
(tmpzval = php_stream_context_get_option(context, "http", "content")) != NULL &&
Expand All @@ -624,7 +626,7 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
}

/* Request content, such as for POST requests */
if (header_init && context &&
if ((header_init || redirect_keep_method) && context &&
(tmpzval = php_stream_context_get_option(context, "http", "content")) != NULL &&
Z_TYPE_P(tmpzval) == IS_STRING && Z_STRLEN_P(tmpzval) > 0) {
if (!(have_header & HTTP_HEADER_CONTENT_LENGTH)) {
Expand Down Expand Up @@ -913,9 +915,16 @@ static php_stream *php_stream_url_wrap_http_ex(php_stream_wrapper *wrapper,
CHECK_FOR_CNTRL_CHARS(resource->pass);
CHECK_FOR_CNTRL_CHARS(resource->path);
}
int new_flags = HTTP_WRAPPER_REDIRECTED;
if (response_code == 307 || response_code == 308) {
/* RFC 7538 specifies that status code 308 does not allow changing the request method from POST to GET.
* RFC 7231 does the same for status code 307.
* To keep consistency between POST and PATCH requests, we'll also not change the request method from PATCH to GET, even though it's allowed it's not mandated by the RFC. */
new_flags |= HTTP_WRAPPER_KEEP_METHOD;
}
stream = php_stream_url_wrap_http_ex(
wrapper, new_path, mode, options, opened_path, context,
--redirect_max, HTTP_WRAPPER_REDIRECTED, response_header STREAMS_CC);
--redirect_max, new_flags, response_header STREAMS_CC);
} else {
php_stream_wrapper_log_error(wrapper, options, "HTTP request failed! %s", tmp_line);
}
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/tests/http/bug67430.phpt
Expand Up @@ -41,7 +41,7 @@ POST / HTTP/1.1
Host: %s:%d
Connection: close

GET /foo HTTP/1.1
POST /foo HTTP/1.1
Host: %s:%d
Connection: close

Expand Down
62 changes: 62 additions & 0 deletions ext/standard/tests/http/gh11274.phpt
@@ -0,0 +1,62 @@
--TEST--
GH-11274 (POST/PATCH request via file_get_contents + stream_context_create switches to GET after a HTTP 308 redirect)
--INI--
allow_url_fopen=1
--CONFLICTS--
server
--FILE--
<?php
$serverCode = <<<'CODE'
$uri = $_SERVER['REQUEST_URI'];
if (isset($_GET["desired_status"]) && $uri[strlen($uri) - 1] !== '/') {
$desired_status = (int) $_GET["desired_status"];
http_response_code($desired_status);
header("Location: $uri/");
exit;
}
echo "method: ", $_SERVER['REQUEST_METHOD'], "; body: ", file_get_contents('php://input'), "\n";
CODE;

include __DIR__."/../../../../sapi/cli/tests/php_cli_server.inc";
php_cli_server_start($serverCode, null, []);

foreach ([null, 301, 302, 307, 308] as $status) {
if (is_null($status)) {
echo "-- Testing unredirected request --\n";
} else {
echo "-- Testing redirect status code $status --\n";
}
$suffix = $status ? "?desired_status=$status" : "";
echo file_get_contents("http://" . PHP_CLI_SERVER_ADDRESS . "/test$suffix", false, stream_context_create(['http' => ['method' => 'POST', 'header' => 'Content-type: application/x-www-form-urlencoded', 'content' => http_build_query(['hello' => 'world'])]]));
echo file_get_contents("http://" . PHP_CLI_SERVER_ADDRESS . "/test$suffix", false, stream_context_create(['http' => ['method' => 'PATCH', 'header' => 'Content-type: application/x-www-form-urlencoded', 'content' => http_build_query(['hello' => 'world'])]]));
echo file_get_contents("http://" . PHP_CLI_SERVER_ADDRESS . "/test/$suffix", false, stream_context_create(['http' => ['method' => 'POST', 'header' => 'Content-type: application/x-www-form-urlencoded', 'content' => http_build_query(['hello' => 'world'])]]));
echo file_get_contents("http://" . PHP_CLI_SERVER_ADDRESS . "/test/$suffix", false, stream_context_create(['http' => ['method' => 'PATCH', 'header' => 'Content-type: application/x-www-form-urlencoded', 'content' => http_build_query(['hello' => 'world'])]]));
}
?>
--EXPECT--
-- Testing unredirected request --
method: POST; body: hello=world
method: PATCH; body: hello=world
method: POST; body: hello=world
method: PATCH; body: hello=world
-- Testing redirect status code 301 --
method: GET; body:
method: GET; body:
method: GET; body:
method: GET; body:
-- Testing redirect status code 302 --
method: GET; body:
method: GET; body:
method: GET; body:
method: GET; body:
-- Testing redirect status code 307 --
method: POST; body: hello=world
method: PATCH; body: hello=world
method: POST; body: hello=world
method: PATCH; body: hello=world
-- Testing redirect status code 308 --
method: POST; body: hello=world
method: PATCH; body: hello=world
method: POST; body: hello=world
method: PATCH; body: hello=world

0 comments on commit 1ede313

Please sign in to comment.