Closed Bug 1243586 Opened 8 years ago Closed 8 years ago

Implement Upgrade-Insecure-Requests HTTP Request Header Field

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [domsecurity-active])

Attachments

(3 files, 5 obsolete files)

When implementing UIR withing Bug 1139297 we haven't implemented the header field bits. It's time to do that now, see also:
https://w3c.github.io/webappsec-upgrade-insecure-requests/#preference
Assignee: nobody → mozilla
Blocks: 1139297
Status: NEW → ASSIGNED
Richard, I think that is a good sport to add the header, but we should check with a Necko peer. For now I just wanna make sure you agree to the approach we discussed.
Attachment #8722183 - Flags: review?(rlb)
Comment on attachment 8722183 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +317,5 @@
>  
> +    // Note that we are not precisely following the spec here and send
> +    // the "Upgrade-Insecure-Requests" request header for *all* navigational
> +    // requests instead of only sending it for a subset defined in the spec.
> +    // See: https://www.w3.org/TR/upgrade-insecure-requests/#preference

I don't actually see any conflict with the spec here.  The spec says you MUST/SHOULD send the header for certain loads; it doesn't say you MUST NOT send it other times.

@@ +323,5 @@
> +                               mLoadInfo->GetExternalContentPolicyType() :
> +                               nsIContentPolicy::TYPE_OTHER;
> +
> +    if (type == nsIContentPolicy::TYPE_DOCUMENT ||
> +        type == nsIContentPolicy::TYPE_SUBDOCUMENT) {

This may actually be where the conflict is.  It doesn't look to me like there's a restriction to document loads in the spec, so this would be a subset of the behavior defined by the spec.  However, given that the algorithm in "4.2. Should insecure requests be upgraded for client?" makes this all dependent on the responsible document, I don't think there's a difference in practice.  This seems like a fine subset to start with, and we can adapt/expand it if we need to later.
Attachment #8722183 - Flags: review?(rlb) → review+
Comment on attachment 8722182 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_tests.patch

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

::: dom/security/test/unit/test_csp_upgrade_insecure_request_header.js
@@ +36,5 @@
> +    contentType: Ci.nsIContentPolicy.TYPE_IMG
> +  },
> +];
> +
> +function FooListener() {

More descriptive name, please :)
Attachment #8722182 - Flags: review?(rlb) → review+
Richard, apparently there are devtools tests which needed to be updated!
Attachment #8724868 - Flags: review?(rlb)
(In reply to Wes Kocher (:KWierso) from comment #6)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6866466b1d71 for
> apparently breaking netmonitor tests:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=22654176&repo=mozilla-
> inbound

Thanks Wes!
Flags: needinfo?(mozilla)
Attachment #8724868 - Flags: review?(rlb) → review+
Unbitrotted patches and incorporated Richard's suggestions. Carrying over r+.
Attachment #8722182 - Attachment is obsolete: true
Attachment #8722183 - Attachment is obsolete: true
Attachment #8724868 - Attachment is obsolete: true
Attachment #8725303 - Flags: review+
Unbitrotted patches and incorporated Richard's suggestions. Carrying over r+.
Attachment #8725304 - Flags: review+
Victor, it seems you have written most of those tests, so I was wondering if you can provide some help with updating those tests. When adding a new request header 'Upgrade-insecure-requests' four tests are failing total:

I was able to update:
* devtools/client/netmonitor/har/test/browser_net_har_copy_all_as_har.js, and
* devtools/client/netmonitor/test/browser_net_copy_headers.js
which should be correct now.

The other two tests:
* devtools/client/netmonitor/test/browser_net_simple-request-data.js
* devtools/client/netmonitor/test/browser_net_simple-request-details.js
seem really flaky, no matter what I change the tests are failing somewhere else and I get different tests failing within those two tests. For now I commented out the parts that are failing (no idea why those parts are even related to my changes). Obviously that is not the right solution in the end, but I don't know how to modify/update the tests to make them work again. Can you have a look and let me know what I can do?

Thanks for your help!
Flags: needinfo?(vporof)
Attachment #8725305 - Flags: feedback?(vporof)
Comment on attachment 8725305 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_test_updates.patch

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

These tests are making sure we get the appropriate values are displayed in the netmonitor's frontend. Since I'd expect none of your changes to affect the frontend itself, it just looks like we're getting different network data from the backend. The test server is implemented in sjs_simple-test-server.sjs for those particular files. I'd suggest taking a look at the requests and responses and see why they differ.

For `browser_net_simple-request-details.js` it might just be that the number of headers changed so the indexes of the entries in the UI have changed as well. However, I have no why cookies would be different for `browser_net_simple-request-data.js`.
Attachment #8725305 - Flags: feedback?(vporof)
Victor, I don't know what's up with those cookie-panels. I spent quite so much time trying to figure out what's going on, but I can't. The only thing I can tell is that I get different results if I put it some |dump| statements. Potentially some timing issue?

Unfortunately those tests are quite hard to understand for me. Maybe those tests were wrong before and now they are correct? I can't tell. Let's see if TRY is happy:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53996f79376d
Attachment #8725305 - Attachment is obsolete: true
Flags: needinfo?(vporof)
Attachment #8725973 - Flags: review?(vporof)
Comment on attachment 8725973 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_test_updates.patch

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

Generally this is fine, but I'd like us to understand *why* we're getting different results. The netmonitor simply shows what it's receiving. If we're now receiving different things, it might be a symptom of some unknown underlying problem.
Attachment #8725973 - Flags: review?(vporof) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Created attachment 8725973 [details] [diff] [review]
> bug_1243586_upgrade_insecure_header_field_test_updates.patch
> 
> Victor, I don't know what's up with those cookie-panels. I spent quite so
> much time trying to figure out what's going on, but I can't. The only thing
> I can tell is that I get different results if I put it some |dump|
> statements. Potentially some timing issue?

If there was a race somewhere I'd be incredibly surprised.

> Unfortunately those tests are quite hard to understand for me. Maybe those
> tests were wrong before and now they are correct?

Wrong in what way?
(In reply to Victor Porof [:vporof][:vp] from comment #15)
> Wrong in what way?

Victor, can you have a look at the try run again [1]?
Those try failures are also errors I am experiencing intermittently locally, but I can't explain why, e.g.
> The sixth request header value was incorrect. - Got "bob=true; tom=cool", expected "keep-alive"
Do you have any idea why? I don't know.

Or also:
> The requestHeaders attachment has an incorrect |headers| property. - Got 10, expected 9
Not all the time though, very intermittent (at least locally).

Any suggestions on how to move forward? It seems TRY is not happy and we can't land as is. Any chance you could apply the patches an have a look? You are more familiar with the test setup than I am. I already tried for some hours yesterday, but it seems I can't find the root cause of the problem.

Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=53996f79376d
Flags: needinfo?(vporof)
I'll have a look.
(In reply to Victor Porof [:vporof][:vp] from comment #17)
> I'll have a look.

Thanks, it's also worth noting that I get different results on mac and linux. We should really figure out what's going on with those tests before we land the patches in this bug.
Whiteboard: [domsecurity-active]
Depends on: 1258809
Hey Victor, as discussed I disabled the two failing netmonitor tests and filed Bug 1258809 to re-enable them. Here is the latest TRY run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c1945c9738
Attachment #8725973 - Attachment is obsolete: true
Flags: needinfo?(vporof)
Attachment #8733505 - Flags: review?(vporof)
Attachment #8733505 - Flags: review?(vporof) → review+
Depends on: 1330795
You need to log in before you can comment on or make changes to this bug.