Implement Upgrade-Insecure-Requests HTTP Request Header Field

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Depends on: 2 bugs, {dev-doc-complete})

unspecified
mozilla48
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → mozilla
Blocks: 1139297
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8722182 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_tests.patch
Attachment #8722182 - Flags: review?(rlb)
(Assignee)

Comment 2

2 years ago
Created attachment 8722183 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field.patch

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+

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/846e31fe5eb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a4d2d48fb2
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
Flags: needinfo?(mozilla)
(Assignee)

Comment 7

2 years ago
Created attachment 8724868 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_test_updates.patch

Richard, apparently there are devtools tests which needed to be updated!
Attachment #8724868 - Flags: review?(rlb)
(Assignee)

Comment 8

2 years ago
(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+
(Assignee)

Comment 9

2 years ago
Created attachment 8725303 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field.patch

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+
(Assignee)

Comment 10

2 years ago
Created attachment 8725304 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_tests.patch

Unbitrotted patches and incorporated Richard's suggestions. Carrying over r+.
Attachment #8725304 - Flags: review+
(Assignee)

Comment 11

2 years ago
Created attachment 8725305 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_test_updates.patch

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)
(Assignee)

Comment 13

2 years ago
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?

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?
(Assignee)

Comment 16

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Updated

2 years ago
Whiteboard: [domsecurity-active]
(Assignee)

Updated

2 years ago
Depends on: 1258809
(Assignee)

Comment 19

2 years ago
Created attachment 8733505 [details] [diff] [review]
bug_1243586_upgrade_insecure_header_field_test_updates.patch

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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e769e96e8680
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef37a752e6cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/471a58815a86
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e769e96e8680
https://hg.mozilla.org/mozilla-central/rev/ef37a752e6cd
https://hg.mozilla.org/mozilla-central/rev/471a58815a86
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Keywords: dev-doc-needed
https://developer.mozilla.org/en-US/Firefox/Releases/48#HTTP
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Upgrade-Insecure-Requests
Keywords: dev-doc-needed → dev-doc-complete

Updated

9 months ago
Depends on: 1330795
You need to log in before you can comment on or make changes to this bug.