Closed Bug 1406908 Opened 2 years ago Closed 2 years ago

Implement 425 (Too Early)

Categories

(Core :: Networking: HTTP, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-triaged])

Attachments

(1 file, 4 obsolete files)

I have one question, what to do if server is misbehaving and is sending 425 for a request that is not sent using in early data?

I would suggest trying it once more and displaying an error, probably 425 with an explanation that the server is misbehaving.

Or let our 10 repeats limit control it. We retry up to 10 times before giving up (this is unrelated to this code but in general we will try up to 10 times). And after that display the error.

Knowing the Internet this will probably happen :)
Flags: needinfo?(martin.thomson)
Sorry, I will implement something.
Flags: needinfo?(martin.thomson)
if early-data is not use, we will show a error page. This also counts the case when early-data is used and we get 425 and on the repeated request(of course not in early-data) we get 425 again.
Sounds like a plan.  I think that we shouldn't try really hard to determine if the original request was in early data.  Maybe we have an intermediary that gets overzealous, or something.  425 == unconditional retry seems reasonable to me.
Attached patch bug_1406908_test_v1.patch (obsolete) — Splinter Review
Attachment #8917087 - Flags: review?(mcmanus)
Attached patch bug_1406908_v0.patch (obsolete) — Splinter Review
Attached patch bug_1406908_v2.patch (obsolete) — Splinter Review
Ok, I made the first version that is imitating 421. This is patch bug_1406908_v0.patch. 421 always resets the connection which does not need to happened for 425. So I have spent some time trying to do a restart inside nsHttpTransaction without reseting the connection, but it got too complicated, e.g. we need to read any content that may exist, it should not but ... it is the Internet, etc.

So I decided to do restart inside nsHttpChannel.
Attachment #8917093 - Flags: review?(mcmanus)
Comment on attachment 8917093 [details] [diff] [review]
bug_1406908_v2.patch

I would like Valentin to look at the patch as well.

Valentin can you take a look at nsHttpChannel part, if I got everything with RCWN correctly.
Attachment #8917093 - Flags: review?(valentin.gosu)
Comment on attachment 8917093 [details] [diff] [review]
bug_1406908_v2.patch

Review of attachment 8917093 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7125,5 @@
>  
>      LOG(("nsHttpChannel::OnStartRequest [this=%p request=%p status=%" PRIx32 "]\n",
>           this, request, static_cast<uint32_t>(static_cast<nsresult>(mStatus))));
>  
> +    if ((mTransaction->ResponseHead()->Status() == 425) &&

I'd move this block after the if (mRaceCacheWithNetwork) block.
I think mTransaction may be null here, so a check would be appropriate.
I guess the condition would be something like if (request == mTransactionPump && status == 425 && !alreadyReceived) ...
Attachment #8917093 - Flags: review?(valentin.gosu)
Comment on attachment 8917093 [details] [diff] [review]
bug_1406908_v2.patch

Review of attachment 8917093 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to ask for a pretty significant change.. can you make the retry logic happen inside nshttptransaction as 421 does via NS_ERROR_NET_RESET? the way you have it structured through nsHttpChannel brings a lot of other concerns into play.. chiefly it involves the nshttptransaction being accessed on multiple threads much more often, and I don't think that's a good idea.. the only place that does that is auth retry and that's a constant source of pain so I don't think that's the right thing to model.

I think that will also make it easier to not honor 425 in the cases where the request wasn't send in Early Data, which I think was missed.

(I'm sure that means no-early-data becomes a nsHttp::mCap)

I appreciate the telemetry changes.

please also make sure that 425 responses are not cached (in the channel code).. they could end up in the channel if 425 is erroneously returned to something that wasn't sent in early data.
Attachment #8917093 - Flags: review?(mcmanus)
Whiteboard: [necko-triaged]
Priority: -- → P2
Attached patch bug_1406908_v0.patch (obsolete) — Splinter Review
Honza, can you take a look at nsHttpChannel. We should not cache 425 responses. Is tihs the right way to do it?
Attachment #8917088 - Attachment is obsolete: true
Attachment #8917093 - Attachment is obsolete: true
Attachment #8919357 - Flags: review?(mcmanus)
Attachment #8919357 - Flags: review?(honzab.moz)
Comment on attachment 8919357 [details] [diff] [review]
bug_1406908_v0.patch

Review of attachment 8919357 [details] [diff] [review]:
-----------------------------------------------------------------

This is correct.  Thanks.
Attachment #8919357 - Flags: review?(honzab.moz) → review+
Comment on attachment 8919357 [details] [diff] [review]
bug_1406908_v0.patch

Review of attachment 8919357 [details] [diff] [review]:
-----------------------------------------------------------------

we shouldn't retry on 425 if we didn't orginally send in early data.. this doesn't check that (does it?)
Attachment #8919357 - Flags: review?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #13)
> Comment on attachment 8919357 [details] [diff] [review]
> bug_1406908_v0.patch
> 
> Review of attachment 8919357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> we shouldn't retry on 425 if we didn't orginally send in early data.. this
> doesn't check that (does it?)

I wanted to implement that in the first place, but Martin suggested to do it unconditionally - comment 4.
I think that it would be OK to check if it were inexpensive to do so.  I should have clarified: unconditional retry would be acceptable, but I wouldn't go so far as to suggest that it would be a requirement.
Attachment #8919357 - Attachment is obsolete: true
Attachment #8920021 - Flags: review?(mcmanus)
(In reply to Martin Thomson [:mt:] from comment #15)
> I think that it would be OK to check if it were inexpensive to do so.  I
> should have clarified: unconditional retry would be acceptable, but I
> wouldn't go so far as to suggest that it would be a requirement.

(this is something we already know - there's telem for it :))
Comment on attachment 8920021 [details] [diff] [review]
bug_1406908_v0.patch

Review of attachment 8920021 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8920021 - Flags: review?(mcmanus) → review+
Attachment #8917087 - Flags: review?(mcmanus) → review?(daniel)
Attachment #8917087 - Flags: review?(daniel) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/291c46b79fa2
Implement 425 (Too Early). r=mcmanus, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/6941e33afc7d
Test for response code 425. r=mcmanus
Keywords: checkin-needed
Backed out for failing own xpcshell test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a82bcbca7bf3eb2f82b5d6e7678eadfc9125c92

A push with failures (landing push also had failures from previous pushes): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=797dfb82b10deedd49faaa3229311ed9a456ad17&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139525582&repo=mozilla-inbound

[task 2017-10-25T15:25:57.427Z] 15:25:57     INFO -  TEST-START | netwerk/test/unit/test_425.js
[task 2017-10-25T15:25:57.620Z] 15:25:57  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_425.js | xpcshell return code: 0
[task 2017-10-25T15:25:57.620Z] 15:25:57     INFO -  TEST-INFO took 190ms
[task 2017-10-25T15:25:57.621Z] 15:25:57     INFO -  >>>>>>>
[task 2017-10-25T15:25:57.621Z] 15:25:57     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-10-25T15:25:57.621Z] 15:25:57     INFO -  (xpcshell/head.js) | test pending (2)
[task 2017-10-25T15:25:57.622Z] 15:25:57     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-10-25T15:25:57.622Z] 15:25:57     INFO -  running event loop
[task 2017-10-25T15:25:57.624Z] 15:25:57     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2017-10-25T15:25:57.625Z] 15:25:57     INFO -  TEST-PASS | netwerk/test/unit/test_425.js | onStopRequest - [onStopRequest : 181] 10 == 10
[task 2017-10-25T15:25:57.626Z] 15:25:57  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_425.js | checkRequestResponse - [checkRequestResponse : 52] 425 == 200
[task 2017-10-25T15:25:57.628Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_425.js:checkRequestResponse:52
[task 2017-10-25T15:25:57.645Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/head_channels.js:onStopRequest:186
[task 2017-10-25T15:25:57.645Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:221
[task 2017-10-25T15:25:57.646Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:544
[task 2017-10-25T15:25:57.646Z] 15:25:57     INFO -  -e:null:1
[task 2017-10-25T15:25:57.647Z] 15:25:57     INFO -  exiting test
[task 2017-10-25T15:25:57.648Z] 15:25:57     INFO -  Error in closure function: 2147500036
[task 2017-10-25T15:25:57.649Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/head_channels.js:onStopRequest:193
[task 2017-10-25T15:25:57.650Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_do_main:221
[task 2017-10-25T15:25:57.651Z] 15:25:57     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:544
[task 2017-10-25T15:25:57.652Z] 15:25:57     INFO -  -e:null:1
[task 2017-10-25T15:25:57.653Z] 15:25:57     INFO -  exiting test
[task 2017-10-25T15:25:57.654Z] 15:25:57     INFO -  PID 11396 | JavaScript error: /builds/worker/workspace/build/tests/xpcshell/head.js, line 221: uncaught exception: 2147500036
[task 2017-10-25T15:25:57.655Z] 15:25:57     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "uncaught exception: 2147500036" {file: "/builds/worker/workspace/build/tests/xpcshell/head.js" line: 221}]"
[task 2017-10-25T15:25:57.656Z] 15:25:57     INFO -  PID 11396 | JavaScript strict warning: resource://testing-common/httpd.js, line 800: ReferenceError: reference to undefined property "_stopCallback"
[task 2017-10-25T15:25:57.657Z] 15:25:57     INFO -  PID 11396 | !!! error running onStopped callback: TypeError: callback is not a function
[task 2017-10-25T15:25:57.658Z] 15:25:57     INFO -  <<<<<<<
Flags: needinfo?(dd.mozilla)
I forgot that I  have changed the patch so that we will retry on a 425 only if we have really send early-data. Since we do not have a way to test with early-data I could only make a pref to change behavior for tests which I do not find good having extra code only for tests. I will not push the test.
Flags: needinfo?(dd.mozilla)
(In reply to Dragana Damjanovic [:dragana] from comment #21)
> I forgot that I  have changed the patch so that we will retry on a 425 only
> if we have really send early-data. Since we do not have a way to test with
> early-data I could only make a pref to change behavior for tests which I do
> not find good having extra code only for tests. I will not push the test.

Also 421 executes the same code path so the code path is already automatically tested.
Attachment #8917087 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7df0e32add4c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I'm not sure if that's related but I started to get a 425 in some connections with my servers using firefox nightly 59.0a1 (2018-01-09) (64-bit) - build id 20180109100117.

Basically if I open the page, wait a few minutes and click in the links I get the 425 status code.

I'm using cloudflare with TLS 1.3. The connection never actually get to my servers (Nginx don't even log it).

If I disable the 'security.tls.enable_0rtt_data" the but won't happen.

I've also tested on Stable 57.0.4 (64-bit) and it works fine.
Yes, that is expected behaviour.  What you are seeing is:

1. you connect to Cloudflare (we setup 0-RTT at this point)
2. after waiting, you disconnect from Cloudflare
3. you click on a link something, which causes Firefox to reconnect to Cloudflare
4. Firefox sees that 0-RTT is possible, and sends requests in 0-RTT
5. Cloudflare sees that the requests aren't guaranteed to be safe from replay attack and sends back a 425 status
6. Firefox sends the requests again after the handshake completes

Nothing to worry about other than a tiny bit more data being exchanged.

You can avoid this and improve performance, by enabling 0-RTT for those resources.  I don't know what signals Cloudflare expects to receive from your servers before it will enable 0-RTT for a given resource, but it's probably only going to be used for something that they can cache.  Their support might be able to give you more information.

Note that this is likely to stop working for a little while.  Firefox is upgrading its TLS 1.3 support and we expect it to take a few days or weeks before servers are upgraded.
I've opened this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1429221

The problem is that when our API returns 425 our webapp treat that as an error and show an error message to our users. Also Firefox does not request again (should it be step 6?) and no data is loaded to the user.

Is that really expected? As a user it feels like a bug.
(In reply to Sergio Oliveira Campos from comment #27)
> I've opened this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1429221
> 
> The problem is that when our API returns 425 our webapp treat that as an
> error and show an error message to our users. Also Firefox does not request
> again (should it be step 6?) and no data is loaded to the user.
> 
> Is that really expected? As a user it feels like a bug.

it should send request again. It will not send it if TLS early-data is not used (only in that case), but Firefox should not get 425 if it have not sent early-data.

Can you make a http log for me:
https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging

thanks. you can also send it via e-mail.
Flags: needinfo?(seocam)
(In reply to Dragana Damjanovic [:dragana] from comment #28)

> it should send request again. It will not send it if TLS early-data is not
> used (only in that case), but Firefox should not get 425 if it have not sent
> early-data.
> 
> Can you make a http log for me:
> https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging
> 
> thanks. you can also send it via e-mail.

Just sent the logs to you by e-mail.
Flags: needinfo?(seocam)
(In reply to Sergio Oliveira Campos from comment #29)
> (In reply to Dragana Damjanovic [:dragana] from comment #28)
> 
> > it should send request again. It will not send it if TLS early-data is not
> > used (only in that case), but Firefox should not get 425 if it have not sent
> > early-data.
> > 
> > Can you make a http log for me:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging
> > 
> > thanks. you can also send it via e-mail.
> 
> Just sent the logs to you by e-mail.

Thanks. I will take a look.
(In reply to Sergio Oliveira Campos from comment #29)
> (In reply to Dragana Damjanovic [:dragana] from comment #28)
> 
> > it should send request again. It will not send it if TLS early-data is not
> > used (only in that case), but Firefox should not get 425 if it have not sent
> > early-data.
> > 
> > Can you make a http log for me:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging
> > 
> > thanks. you can also send it via e-mail.
> 
> Just sent the logs to you by e-mail.

thanks. I have opened bug 1429859 to resolve this issue.
Depends on: 1429859
Keywords: dev-doc-needed
I have documented this by:

Adding a page for the 425 status code:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/425

Adding a page for the Early-Data header:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Early-Data
https://github.com/mdn/browser-compat-data/pull/2811 (browser compat data for the header)

Adding a note to the Fx58 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/58#HTTP

Let me know if this looks OK; review appreciated. Thanks!
Flags: needinfo?(dd.mozilla)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #32)
> I have documented this by:
> 
> Adding a page for the 425 status code:
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/425
> 
> Adding a page for the Early-Data header:
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
> https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Early-Data
> https://github.com/mdn/browser-compat-data/pull/2811 (browser compat data
> for the header)
> 
> Adding a note to the Fx58 rel notes:
> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/58#HTTP
> 
> Let me know if this looks OK; review appreciated. Thanks!

Sorry for delay.
Early-Data header is not meant to be used by user agents, instead it is for intermediaries. Browsers indicates that they know early-data and code 425 by sending data using tls early-data. Early-data header is used by intermediary on a path to tell the next hop that the request is received using tls early-data.
Flags: needinfo?(dd.mozilla)
Also, conveniently, the RFC (RFC 8470) was published today.
(In reply to Dragana Damjanovic [:dragana] from comment #33)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #32)
> > I have documented this by:
> > 
> > Adding a page for the 425 status code:
> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/425
> > 
> > Adding a page for the Early-Data header:
> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
> > https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Early-Data
> > https://github.com/mdn/browser-compat-data/pull/2811 (browser compat data
> > for the header)
> > 
> > Adding a note to the Fx58 rel notes:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/58#HTTP
> > 
> > Let me know if this looks OK; review appreciated. Thanks!
> 
> Sorry for delay.
> Early-Data header is not meant to be used by user agents, instead it is for
> intermediaries. Browsers indicates that they know early-data and code 425 by
> sending data using tls early-data. Early-data header is used by intermediary
> on a path to tell the next hop that the request is received using tls
> early-data.

No worries. I've updated the spec links to link to the correct published RFC, and made some small updates, but I'm not really sure what to change to fix other issues here. Can you advise me on other problems with the content as it stands? Thanks!
All fixed now.
You need to log in before you can comment on or make changes to this bug.