Skip to content

Fix GH-11274: POST/PATCH request via file_get_contents + stream_context_create switches to GET after a HTTP 308 redirect #11275

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix GH-11274: POST/PATCH request via file_get_contents + stream_context_create switches to GET after a HTTP 308 redirect #11275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions ext/standard/http_fopen_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question here is which RFC this code was implemented for. If it was https://datatracker.ietf.org/doc/html/rfc2616#section-10.3.8, then it's not exactly clear (at least to me). Anyway I think it should be right thing to prohibit changing of the method even for 8.1 so I'm fine with treating this as a bug. This is just a side note that we don't always treat things as a bug if they change in the later RFC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It gets even more unclear when you read the note below "10.3.3 302 found" in the RFC you linked:

Note: [RFC 1945](https://datatracker.ietf.org/doc/html/rfc1945) and
      [RFC 2068](https://datatracker.ietf.org/doc/html/rfc2068) specify that the client is not allowed
      to change the method on the redirected request.  However, most
      existing user agent implementations treat 302 as if it were a 303
      response, performing a GET on the Location field-value regardless
      of the original request method. The status codes 303 and 307 have
      been added for servers that wish to make unambiguously clear which
      kind of reaction is expected of the client.

So it sounds like the behaviour even for 302 got changed later on (as is clear from the table in RFC 7538) because lots of clients implemented it in another way than originally intended back in the day.
Fortunately, those rules are better defined nowadays in current RFCs.

* 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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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