Skip to content
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.

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

chore: cherry-pick 4d26949260aa from chromium #33676

Merged
merged 4 commits into from Apr 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -166,6 +166,7 @@ remove_incorrect_width_height_adjustments.patch
fix_non-client_mouse_tracking_and_message_bubbling_on_windows.patch
fixed_terminate_caused_by_binding_to_wrong_version.patch
follow_wayland_specification_regarding_xkb_keymap.patch
cherry-pick-4d26949260aa.patch
use-after-free_of_id_and_idref_attributes.patch
fix_--without-valid_build.patch
usb_fix_oob_access_with_non-sequential_interfaces.patch
154 changes: 154 additions & 0 deletions patches/chromium/cherry-pick-4d26949260aa.patch
@@ -0,0 +1,154 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yutaka Hirano <yhirano@chromium.org>
Date: Fri, 4 Feb 2022 10:16:05 +0000
Subject: Don't interpret informational response body as HTTP/0.9 response

An informational (1xx) response with body can be interpreted as an
informational response followed by an HTTP/0.9 response. It is confusing
so let's stop doing that.

Bug: 1291482
Change-Id: Ic3823838614330d761f11360a783859e5baa260e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3428433
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/main@{#967169}

diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index 8fe9a4e507aa5b081af4b48fe80ee38275dee84e..8ec7bbc1ed065b84afd22a4223e2359344876c47 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -1013,15 +1013,23 @@ int HttpStreamParser::ParseResponseHeaders(int end_offset) {
base::StringPiece(read_buf_->StartOfBuffer(), end_offset));
if (!headers)
return net::ERR_INVALID_HTTP_RESPONSE;
+ has_seen_status_line_ = true;
} else {
// Enough data was read -- there is no status line, so this is HTTP/0.9, or
// the server is broken / doesn't speak HTTP.

- // If the port is not the default for the scheme, assume it's not a real
- // HTTP/0.9 response, and fail the request.
+ if (has_seen_status_line_) {
+ // If we saw a status line previously, the server can speak HTTP/1.x so it
+ // is not reasonable to interpret the response as an HTTP/0.9 response.
+ return ERR_INVALID_HTTP_RESPONSE;
+ }
+
base::StringPiece scheme = request_->url.scheme_piece();
if (url::DefaultPortForScheme(scheme.data(), scheme.length()) !=
request_->url.EffectiveIntPort()) {
+ // If the port is not the default for the scheme, assume it's not a real
+ // HTTP/0.9 response, and fail the request.
+
// Allow Shoutcast responses over HTTP, as it's somewhat common and relies
// on HTTP/0.9 on weird ports to work.
// See
diff --git a/net/http/http_stream_parser.h b/net/http/http_stream_parser.h
index 0a415338f3396e1fce75b691911aa77c28d8bdf9..ff0e1508326c92ecd46546755c030a906c6eac28 100644
--- a/net/http/http_stream_parser.h
+++ b/net/http/http_stream_parser.h
@@ -271,6 +271,11 @@ class NET_EXPORT_PRIVATE HttpStreamParser {
// True if reading a keep-alive response. False if not, or if don't yet know.
bool response_is_keep_alive_;

+ // True if we've seen a response that has an HTTP status line. This is
+ // persistent across multiple response parsing. If we see a status line
+ // for a response, this will remain true forever.
+ bool has_seen_status_line_ = false;
+
// Keep track of the number of response body bytes read so far.
int64_t response_body_read_;

diff --git a/net/http/http_stream_parser_unittest.cc b/net/http/http_stream_parser_unittest.cc
index 545b9f11b43b37456ccab54a63eeb76034c934ef..07f94e6bb6aefe3777a71afed80b22eedea2999e 100644
--- a/net/http/http_stream_parser_unittest.cc
+++ b/net/http/http_stream_parser_unittest.cc
@@ -1395,6 +1395,25 @@ TEST(HttpStreamParser, Http09PortTests) {
}
}

+TEST(HttpStreamParser, ContinueWithBody) {
+ const std::string kResponse =
+ "HTTP/1.1 100 Continue\r\n\r\nhello\r\nworld\r\n";
+
+ SimpleGetRunner get_runner;
+ get_runner.set_url(GURL("http://foo.com/"));
+ get_runner.AddRead(kResponse);
+ get_runner.SetupParserAndSendRequest();
+
+ get_runner.ReadHeadersExpectingError(OK);
+ ASSERT_TRUE(get_runner.response_info()->headers);
+ EXPECT_EQ("HTTP/1.1 100 Continue",
+ get_runner.response_info()->headers->GetStatusLine());
+
+ // We ignore informational responses and start reading the next response in
+ // the stream. This simulates the behavior.
+ get_runner.ReadHeadersExpectingError(ERR_INVALID_HTTP_RESPONSE);
+}
+
TEST(HttpStreamParser, NullFails) {
const char kTestHeaders[] =
"HTTP/1.1 200 OK\r\n"
diff --git a/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any-expected.txt b/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any-expected.txt
new file mode 100644
index 0000000000000000000000000000000000000000..6ac068363a83816939013bbde88a7584aca4f307
--- /dev/null
+++ b/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any-expected.txt
@@ -0,0 +1,7 @@
+This is a testharness.js-based test.
+PASS Status(100) should be ignored.
+FAIL Status(101) should be accepted, with removing body. promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch"
+PASS Status(103) should be ignored.
+PASS Status(199) should be ignored.
+Harness: the test ran to completion.
+
diff --git a/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.js b/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.js
new file mode 100644
index 0000000000000000000000000000000000000000..df4dafcd80b38af93b688dcb318cfaf1978a939f
--- /dev/null
+++ b/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.js
@@ -0,0 +1,28 @@
+promise_test(async (t) => {
+ // The 100 response should be ignored, then the transaction ends, which
+ // should lead to an error.
+ await promise_rejects_js(
+ t, TypeError, fetch('/common/text-plain.txt?pipe=status(100)'));
+}, 'Status(100) should be ignored.');
+
+// This behavior is being discussed at https://github.com/whatwg/fetch/issues/1397.
+promise_test(async (t) => {
+ const res = await fetch('/common/text-plain.txt?pipe=status(101)');
+ assert_equals(res.status, 101);
+ const body = await res.text();
+ assert_equals(body, '');
+}, 'Status(101) should be accepted, with removing body.');
+
+promise_test(async (t) => {
+ // The 103 response should be ignored, then the transaction ends, which
+ // should lead to an error.
+ await promise_rejects_js(
+ t, TypeError, fetch('/common/text-plain.txt?pipe=status(103)'));
+}, 'Status(103) should be ignored.');
+
+promise_test(async (t) => {
+ // The 199 response should be ignored, then the transaction ends, which
+ // should lead to an error.
+ await promise_rejects_js(
+ t, TypeError, fetch('/common/text-plain.txt?pipe=status(199)'));
+}, 'Status(199) should be ignored.');
diff --git a/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.worker-expected.txt b/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.worker-expected.txt
new file mode 100644
index 0000000000000000000000000000000000000000..6ac068363a83816939013bbde88a7584aca4f307
--- /dev/null
+++ b/third_party/blink/web_tests/external/wpt/fetch/security/1xx-response.any.worker-expected.txt
@@ -0,0 +1,7 @@
+This is a testharness.js-based test.
+PASS Status(100) should be ignored.
+FAIL Status(101) should be accepted, with removing body. promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch"
+PASS Status(103) should be ignored.
+PASS Status(199) should be ignored.
+Harness: the test ran to completion.
+