RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Julia Boes]

Hi,

Please find below a patch for:

8185898: setRequestProperty(key, null) results in HTTP header without 
colon in request


https://bugs.openjdk.java.net/browse/JDK-8185898

According to RFC 2616 , 
message headers of a HTTP message must adhere to the format:


field-name ":" [ field-value ]

Previously, the request line was handled like a message header and 
MessageHeader::print would omit ":" for message headers with value == 
null to account for the request line. However, this can result in a 
regular message header missing the colon if its value is null. To fix 
this, MessageHeader.requestLine was added, which is used only for the 
status line of a request. Any use of MessageHeader.set(0) and 
MessageHeader.prepend() in HttpURLConnection was replaced by 
MessageHeader::setRequestLine.


Webrev:

http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/


Cheers,

Julia

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Michael McMahon]

Hi Julia,

I will sponsor this for you.

Trivially, there are a couple of unnecessary blank lines inserted at the 
top of MessageHeader.java.
I think the @summary in the new test might be better just having the 
original bug synopsis in this case.

Otherwise, looks fine.

Thanks,
Michael.

On 31/07/2019, 11:56, Julia Boes wrote:

Hi,

Please find below a patch for:

8185898: setRequestProperty(key, null) results in HTTP header without 
colon in request


https://bugs.openjdk.java.net/browse/JDK-8185898

According to RFC 2616 , 
message headers of a HTTP message must adhere to the format:


field-name ":" [ field-value ]

Previously, the request line was handled like a message header and 
MessageHeader::print would omit ":" for message headers with value == 
null to account for the request line. However, this can result in a 
regular message header missing the colon if its value is null. To fix 
this, MessageHeader.requestLine was added, which is used only for the  
status line of a request. Any use of MessageHeader.set(0) and 
MessageHeader.prepend() in HttpURLConnection was replaced by 
MessageHeader::setRequestLine.


Webrev:

http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/ 




Cheers,

Julia

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Daniel Fuchs]

Hi Julia,

Could you verify that `HttpURLConnection::getHeaderField(0)` and
`HttpURLConnection::getHeaderFieldKey(0)` return the same thing
before and after the fix, for all implementations of
`HttpURLConnection` in the JDK?

I believe your test is missing that.

More specifically, I'm referring to this:
https://download.java.net/java/early_access/jdk13/docs/api/java.base/java/net/HttpURLConnection.html#getHeaderFieldKey(int)

"Some implementations may treat the 0th header field as special,
i.e. as the status line returned by the HTTP server. In this case,
getHeaderField(0) returns the status line, but getHeaderFieldKey(0)
returns null."

I wonder if the JDK itself was one of those implementations before
the fix. We do not want to change the behaviour of these methods.

best regards,

-- daniel

On 31/07/2019 11:56, Julia Boes wrote:

Hi,

Please find below a patch for:

8185898: setRequestProperty(key, null) results in HTTP header without 
colon in request


https://bugs.openjdk.java.net/browse/JDK-8185898

According to RFC 2616 , 
message headers of a HTTP message must adhere to the format:


field-name ":" [ field-value ]

Previously, the request line was handled like a message header and 
MessageHeader::print would omit ":" for message headers with value == 
null to account for the request line. However, this can result in a 
regular message header missing the colon if its value is null. To fix 
this, MessageHeader.requestLine was added, which is used only for the 
status line of a request. Any use of MessageHeader.set(0) and 
MessageHeader.prepend() in HttpURLConnection was replaced by 
MessageHeader::setRequestLine.


Webrev:

http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/


Cheers,

Julia

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Michael McMahon]

Daniel

I don't think the change affects the usage of response headers,
but it might be useful to include in the test a call to getHeaderField(0)
to verify that.

Michael.

On 31/07/2019, 12:30, Daniel Fuchs wrote:

Hi Julia,

Could you verify that `HttpURLConnection::getHeaderField(0)` and
`HttpURLConnection::getHeaderFieldKey(0)` return the same thing
before and after the fix, for all implementations of
`HttpURLConnection` in the JDK?

I believe your test is missing that.

More specifically, I'm referring to this:
https://download.java.net/java/early_access/jdk13/docs/api/java.base/java/net/HttpURLConnection.html#getHeaderFieldKey(int) 



"Some implementations may treat the 0th header field as special,
i.e. as the status line returned by the HTTP server. In this case,
getHeaderField(0) returns the status line, but getHeaderFieldKey(0)
returns null."

I wonder if the JDK itself was one of those implementations before
the fix. We do not want to change the behaviour of these methods.

best regards,

-- daniel

On 31/07/2019 11:56, Julia Boes wrote:

Hi,

Please find below a patch for:

8185898: setRequestProperty(key, null) results in HTTP header without 
colon in request


https://bugs.openjdk.java.net/browse/JDK-8185898

According to RFC 2616 
, message headers of a 
HTTP message must adhere to the format:


field-name ":" [ field-value ]

Previously, the request line was handled like a message header and 
MessageHeader::print would omit ":" for message headers with value == 
null to account for the request line. However, this can result in a 
regular message header missing the colon if its value is null. To fix 
this, MessageHeader.requestLine was added, which is used only for the 
status line of a request. Any use of MessageHeader.set(0) and 
MessageHeader.prepend() in HttpURLConnection was replaced by 
MessageHeader::setRequestLine.


Webrev:

http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/


Cheers,

Julia

Re: RFR: 8185898: setRequestProperty(key, null) results in HTTP header without colon in request

[Daniel Fuchs]

Hi Michael,

On 31/07/2019 14:47, Michael McMahon wrote:

Daniel

I don't think the change affects the usage of response headers,
but it might be useful to include in the test a call to getHeaderField(0)
to verify that.


Oh - right - but it will/might affect the toString() of the
response headers, if that is used for logging, isn't it?
Wouldn't the status line now be printed with an additional colon
at the end?

best,

-- daniel



Michael.

On 31/07/2019, 12:30, Daniel Fuchs wrote:

Hi Julia,

Could you verify that `HttpURLConnection::getHeaderField(0)` and
`HttpURLConnection::getHeaderFieldKey(0)` return the same thing
before and after the fix, for all implementations of
`HttpURLConnection` in the JDK?

I believe your test is missing that.

More specifically, I'm referring to this:
https://download.java.net/java/early_access/jdk13/docs/api/java.base/java/net/HttpURLConnection.html#getHeaderFieldKey(int) 



"Some implementations may treat the 0th header field as special,
i.e. as the status line returned by the HTTP server. In this case,
getHeaderField(0) returns the status line, but getHeaderFieldKey(0)
returns null."

I wonder if the JDK itself was one of those implementations before
the fix. We do not want to change the behaviour of these methods.

best regards,

-- daniel

On 31/07/2019 11:56, Julia Boes wrote:

Hi,

Please find below a patch for:

8185898: setRequestProperty(key, null) results in HTTP header without 
colon in request


https://bugs.openjdk.java.net/browse/JDK-8185898

According to RFC 2616 
, message headers of a 
HTTP message must adhere to the format:


field-name ":" [ field-value ]

Previously, the request line was handled like a message header and 
MessageHeader::print would omit ":" for message headers with value == 
null to account for the request line. However, this can result in a 
regular message header missing the colon if its value is null. To fix 
this, MessageHeader.requestLine was added, which is used only for the 
status line of a request. Any use of MessageHeader.set(0) and 
MessageHeader.prepend() in HttpURLConnection was replaced by 
MessageHeader::setRequestLine.


Webrev:

http://cr.openjdk.java.net/~michaelm/jboes/8185898/webrev.1/


Cheers,

Julia