1436517 - Restrict server-timing support to HTTPS

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[u408661]

When we added support for server-timing trailers in bug 1413999, there was no limiting of that support to HTTPS. We need to have that limited to HTTPS-only.

Comment 1

[Patrick McManus [:mcmanus]]

whole feature really - not just trailers.

Comment 2

[u408661]

Right :)

Comment 3

[u408661]

Attachment: Bug 1436517 - Limit access to the server-timing header to HTTPS contexts.

This also introduces a hidden pref to allow server-timing access from
HTTP contexts for the purposes of our xpcshell tests. We'll remove that
once we get h2 (and therefore tls test) support for server-timing
trailers (https://bugzilla.mozilla.org/show_bug.cgi?id=1436601).

This does not reject or otherwise error when receiving server-timing
headers or trailers on non-HTTPS contexts, it just makes it unavailable
outside the channel.

Review commit: https://reviewboard.mozilla.org/r/218610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218610/

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8949243 [details]
Bug 1436517 - Limit access to the server-timing header to HTTPS contexts.

https://reviewboard.mozilla.org/r/218610/#review224428

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/583dac82f2b5
Limit access to the server-timing header to HTTPS contexts. r=mcmanus

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/583dac82f2b5

Comment 7

[u408661]

Comment on attachment 8949243 [details]
Bug 1436517 - Limit access to the server-timing header to HTTPS contexts.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1413999
[User impact if declined]: Firefox 59 will allow server timing over HTTP, and then when 60 is released, it will be limited to HTTPS-only. Kind of surprising if you're not expecting it.
[Is this code covered by automated tests?]: No - we had to whitelist the tests for HTTP contexts until we can get an HTTP/2 implementation locked in (in progress, bug 1429973)
[Has the fix been verified in Nightly?]: Sort of - running the tests without the pref whitelist causes them to fail, so that's good :)
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not particularly
[Why is the change risky/not risky?]: Just a check for an https scheme on the code that makes headers available outside necko. Everything else is as it was.
[String changes made/needed]: None

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8949243 [details]
Bug 1436517 - Limit access to the server-timing header to HTTPS contexts.

Sounds like we need this in 59. This should land for beta 10. 
I'll follow up in the related bug about whitelisted tests.

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8507a552a358

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing modified xpcshell test netwerk/test/unit/test_header_Server_Timing.js:

https://hg.mozilla.org/releases/mozilla-beta/rev/7590a5f25ea3eab7a8bf1ed118f394f6a01ea335

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=30d165841f3da737624edb48dbfe87442c916f96&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161956218&repo=mozilla-beta

[task 2018-02-13T14:50:31.436Z] 14:50:31     INFO -  TEST-START | netwerk/test/unit/test_header_Server_Timing.js
[task 2018-02-13T14:50:31.744Z] 14:50:31  WARNING -  TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_header_Server_Timing.js | xpcshell return code: 0
[task 2018-02-13T14:50:31.745Z] 14:50:31     INFO -  TEST-INFO took 307ms
[task 2018-02-13T14:50:31.745Z] 14:50:31     INFO -  >>>>>>>
[task 2018-02-13T14:50:31.745Z] 14:50:31     INFO -  PID 12521 | [12521, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2734
[task 2018-02-13T14:50:31.747Z] 14:50:31     INFO -  PID 12521 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2018-02-13T14:50:31.748Z] 14:50:31     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-02-13T14:50:31.750Z] 14:50:31     INFO -  ReferenceError: Services is not defined at /builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_header_Server_Timing.js:57
[task 2018-02-13T14:50:31.752Z] 14:50:31     INFO -  run_test@/builds/worker/workspace/build/tests/xpcshell/tests/netwerk/test/unit/test_header_Server_Timing.js:57:3
[task 2018-02-13T14:50:31.753Z] 14:50:31     INFO -  _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:540

Comment 11

[u408661]

Ah, lovely. Guess central has |Services| already available, but beta doesn't. I'll fix up the patch, make sure I'm right by testing locally, and upload a new version for landing on beta.

Comment 12

[u408661]

Attachment: beta.patch

Approval Request Comment: See previous approval request comment. This version is purely to fix a test issue that is unique to beta (where Services.jsm is not automatically available, as it apparently is on central).

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Aryx, this is a follow up beta59 patch that needs to be landed. Thanks!

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4e1f1641976c