8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields #12976

dfuch · 2023-03-10T13:58:23Z

According to RFC 9113:

A malformed request or response is one that is an otherwise valid sequence of HTTP/2 frames but is invalid due to the presence of extraneous frames, prohibited fields or pseudo-header fields, the absence of mandatory pseudo-header fields, the inclusion of uppercase field names, or invalid field names and/or values (in certain circumstances; see Section 8.2).
[...]
Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

The current behavior is to close the connection with protocol error. This change makes it reset the stream instead.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields

Reviewers

Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12976/head:pull/12976
$ git checkout pull/12976

Update a local copy of the PR:
$ git checkout pull/12976
$ git pull https://git.openjdk.org/jdk pull/12976/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12976

View PR using the GUI difftool:
$ git pr show -t 12976

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12976.diff

bridgekeeper · 2023-03-10T14:00:00Z

👋 Welcome back dfuchs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-10T14:03:22Z

@dfuch The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-10T14:07:16Z

Webrevs

dfuch · 2023-03-10T16:01:44Z

HttpClient tests are stable and tier2 passed

jaikiran · 2023-03-11T11:30:02Z

src/java.net.http/share/classes/jdk/internal/net/http/Stream.java

+                            streamid, n, v);
+                }
+            } catch (UncheckedIOException uio) {
+                // reset stream: From RFC 7540, section-8.1.2.6


Hello Daniel, should we instead refer to RFC-9113 (section 8.1.1) which obsoletes RFC-7540? In the context of this PR, the newer RFC continues to have the same expectations as that in RFC-7540.

Right. Sorry for the oversight.

jaikiran · 2023-03-11T11:44:43Z

src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java

+     * @return the initial stream created during the upgrade.
+     */
+    @SuppressWarnings("unchecked")
+    <T> Stream<T> getInitialStream() {


Given that this method returns and also updates the initial stream member field, the naming of this method is a bit odd. But I can't think of a better name, plus this is internal to the jdk.internal.net.http package and also has a comment which explains what it does, so I think this name is fine.

Maybe I could change the name to "retrieveInitialStream()"... Would that be better?

Hello Daniel, I don't have strong preference, retrieveInitialStream sounds OK. It's also OK if you would want to continue with the current name.

jaikiran · 2023-03-11T11:45:11Z

src/java.net.http/share/classes/jdk/internal/net/http/common/ValidatingHeadersConsumer.java

+import java.util.Set;
+
+/*
+ * Checks RFC 7540 rules (relaxed) compliance regarding pseudo-headers.


Same here, should we use new RFC number?

jaikiran

The change looks good to me. I just have a question about the RFC numbers in the comment, which I have asked inline.

openjdk · 2023-03-11T11:47:15Z

@dfuch This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields

Reviewed-by: jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

431e702: 8303213: Avoid AtomicReference in TextComponentPrintable

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

src/java.net.http/share/classes/jdk/internal/net/http/Stream.java

dfuch · 2023-03-13T11:00:31Z

src/java.net.http/share/classes/jdk/internal/net/http/Stream.java

+                            streamid, n, v);
+                }
+            } catch (UncheckedIOException uio) {
+                // reset stream: From RFC 7540, section-8.1.2.6


Right. Sorry for the oversight.

src/java.net.http/share/classes/jdk/internal/net/http/common/ValidatingHeadersConsumer.java

dfuch · 2023-03-13T11:01:35Z

src/java.net.http/share/classes/jdk/internal/net/http/common/ValidatingHeadersConsumer.java

+import java.util.Set;
+
+/*
+ * Checks RFC 7540 rules (relaxed) compliance regarding pseudo-headers.


Refer to RFC 9113

…alidatingHeadersConsumer.java Refer to RFC 9113

dfuch · 2023-03-13T14:24:11Z

/integrate

openjdk · 2023-03-13T14:25:08Z

Going to push as commit 466ffeb.
Since your change was applied there has been 1 commit pushed to the master branch:

431e702: 8303213: Avoid AtomicReference in TextComponentPrintable

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-13T14:25:23Z

@dfuch Pushed as commit 466ffeb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8303965

4e76e29

openjdk bot changed the title ~~8303965~~ 8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields Mar 10, 2023

openjdk bot added the rfr Pull request is ready for review label Mar 10, 2023

openjdk bot added the net net-dev@openjdk.org label Mar 10, 2023

dfuch added 2 commits March 10, 2023 14:07

Merge branch 'master' into MalformedResponse-8303965

05aca1d

add bug id to test

101dbc6

jaikiran reviewed Mar 11, 2023

View reviewed changes

jaikiran approved these changes Mar 11, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 11, 2023

dfuch commented Mar 13, 2023

View reviewed changes

dfuch and others added 4 commits March 13, 2023 11:02

Update src/java.net.http/share/classes/jdk/internal/net/http/Stream.java

b3c0816

Refer to RFC 9113

Update src/java.net.http/share/classes/jdk/internal/net/http/common/V…

92d5eb6

…alidatingHeadersConsumer.java Refer to RFC 9113

Merge branch 'master' into MalformedResponse-8303965

8b8d378

Typo

b81e05a

jaikiran approved these changes Mar 13, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 13, 2023

openjdk bot closed this Mar 13, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 13, 2023

dfuch deleted the MalformedResponse-8303965 branch March 13, 2023 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields #12976

8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields #12976

dfuch commented Mar 10, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 10, 2023

openjdk bot commented Mar 10, 2023

mlbridge bot commented Mar 10, 2023 •

edited

Loading

dfuch commented Mar 10, 2023

jaikiran Mar 11, 2023 •

edited

Loading

dfuch Mar 13, 2023

jaikiran Mar 11, 2023

dfuch Mar 13, 2023

jaikiran Mar 13, 2023

jaikiran Mar 11, 2023

dfuch Mar 13, 2023

jaikiran left a comment

openjdk bot commented Mar 11, 2023 •

edited

Loading

dfuch Mar 13, 2023

dfuch Mar 13, 2023

dfuch commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields #12976

8303965: java.net.http.HttpClient should reset the stream if response headers contain malformed header fields #12976

Conversation

dfuch commented Mar 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 10, 2023

openjdk bot commented Mar 10, 2023

mlbridge bot commented Mar 10, 2023 • edited Loading

Webrevs

dfuch commented Mar 10, 2023

jaikiran Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaikiran left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

dfuch commented Mar 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 10, 2023 •

edited

Loading

jaikiran Mar 11, 2023 •

edited

Loading

openjdk bot commented Mar 11, 2023 •

edited

Loading