1941375 - Extend test for Idempotency-Key header to be sure we include a layer of quotes

Status: RESOLVED FIXED

(Core :: Networking: HTTP, task, P3)

Description

[Daniel Holbert [:dholbert]]

Right now, we wrap the Idempotency-Key HTTP-header value in an extra layer of double-quotes, here:

https://searchfox.org/mozilla-central/rev/9f9fa5d2cb72b12ac7e168b7f6ee9820f63291e9/netwerk/protocol/http/nsHttpHandler.cpp#586-588

aOutKey.Append("\"");
aOutKey.AppendInt(hashValue);
aOutKey.Append("\"");

That results in the header awkwardly having double-quotes when no other headers do, as shown in e.g. this snippet of "copy-as-curl" output (with newlines added by me for readability):

-H 'X-Request-Time: 1736786129843' \
-H 'X-Token: anonymous' \
-H 'X-Requested-With: XMLHttpRequest' \
-H 'Content-Type: application/json' \
-H 'X-Content-Encoding: enc' \
-H 'X-Signature: 0dd575ea76f3bf04fb2cc163a38d154dd283306dc1ab139b6ec134db111017c1' \
-H 'Idempotency-Key: "13448575235753409992"' \

This seems probably unwanted, and it seems to cause breakage on Apple; see bug 1940594 and bug 1938542 (particularly bug 1940594 comment 9 where I confirmed that this is responsible for the issue at Apple). Let's remove the double-quotes, unless they're somehow needed just for this one specific header for some reason.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1941375: Don't put double-quotes around value for Idempotency-Key http header. r?#necko

We don't add quotes around the values of other HTTP headers, so it makes sense
to have this one follow that pattern. Moreover, the presence of these quotes
seems to break at least one site (Apple's "discussions" board), as noted in the
bug.

Note that we only added support for this header quite recently, and it's behind
a nightly-only pref for now (network.http.idempotencyKey.enabled).

Comment 2

[Daniel Holbert [:dholbert]]

Note that https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-idempotency-key-header-05#section-2.1 does seem to show this header with double-quotes around the value, but that seems like maybe a mistake (or maybe just a difference between the spec shows stuff vs. how HTTP headers actually get generated in the wild).

Comment 3

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

(In reply to Daniel Holbert [:dholbert] from comment #2)

Note that https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-idempotency-key-header-05#section-2.1 does seem to show this header with double-quotes around the value, but that seems like maybe a mistake (or maybe just a difference between the spec shows stuff vs. how HTTP headers actually get generated in the wild).

Idempotency-Key = sf-string

https://datatracker.ietf.org/doc/html/rfc8941#name-strings

sf-string = DQUOTE *chr DQUOTE
chr = unescaped / escaped
unescaped = %x20-21 / %x23-5B / %x5D-7E
escaped = "" ( DQUOTE / "" )

I think the problem is that draft-ietf-httpapi-idempotency-key-header-05 says we should use a structured-header sf-string, but the apple websites don't really like that.

The problem is that none of the other browsers has shipped an idempotency-key implementation.
If this is a major problem we can disable the pref while we reach out to apple regarding their server implementation.

Comment 4

[Daniel Holbert [:dholbert]]

Ah, thanks for digging in a bit further than I did on the spec syntax. Seems like the spec syntax really does require these quotes then.

I might morph this bug/patch into just adjusting the test to expect a quoted integer then; and we can do outreach to Apple to poke at whatever's busted in their server-side http-header-parsing code.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1941375 part 1: Refactor a unit test to use a shared function for validating Idempotency-Key HTTP header values. r?#necko

This doesn't change the test behavior at all; it's just refactoring, so that
we've got a common spot to add additional validation code in the next patch
in this series.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1941375 part 2: Extend a unit test to validate the format of our generated Idempotency-Key HTTP header. r?#necko

Previously we just checked that the value was nonempty (and preserved in the
response); this patch adds a bit more validation of the value itself.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Valentin Gosu [:valentin] (he/him) {{ PTO 21 Dec - 06 Jan }} from comment #3)

I think the problem is that draft-ietf-httpapi-idempotency-key-header-05 says we should use a structured-header sf-string, but the apple websites don't really like that.

The problem is that none of the other browsers has shipped an idempotency-key implementation.
If this is a major problem we can disable the pref while we reach out to apple regarding their server implementation.

I reached out to Karl Dubost to see if he can get us in touch with the right folks at Apple, in bug 1940594 comment 10.

If we're still stalled on this in a few weeks, we might want to turn off the pref (it's already nightly-only which is some comfort, but we don't want to be unnecessarily preventing Nightly users from using discussions.apple.com).

(I wonder if this is the first http header in a browser that includes a double-quote character? It feels like it might be, if this Apple site has apparently been able to reject any requests with such a header up to this point without causing breakage. Maybe this is a sign that there's some webcompat risk from shipping this header in this form; i.e. some deployed web servers might just barf when finding a double-quote character in an http header, either due to them being overly strict, or having a parsing bug, or intentionally rejecting it as some sort of perceived injection attack... Might be worth discussing with the IETF if we anticipate this being something broader than just this one Apple discussion-forum.)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b054287126e5
part 1: Refactor a unit test to use a shared function for validating Idempotency-Key HTTP header values. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/2718dce7faae
part 2: Extend a unit test to validate the format of our generated Idempotency-Key HTTP header. r=necko-reviewers,valentin

Comment 9

[Karl Dubost💡 :karlcow]

Reported to Apple at rdar://142843628

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/b054287126e5
https://hg.mozilla.org/mozilla-central/rev/2718dce7faae