1310493 - Add TYPE_PREFETCH constant to nsIContentPolicyBase

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Jarda Snajdr [:jsnajdr]]

Add TYPE_PREFETCH constant to nsIContentPolicyBase. Needed for bug 1306004, will be used at two places:
- when NetworkPredictor decides to prefetch a resource and creates a new HttpChannel for it
- when a <link type="prefetch"> is being fetched

Now, both requests will have TYPE_OTHER. The goal is to have the prefetch requests marked as such in devtools netmonitor.

Open questions:
- should there be internal sub-values like TYPE_INTERNAL_PREFETCH_PREDICTOR and TYPE_INTERNAL_PREFETCH_LINK? I think they're not needed
- what value to assign to TYPE_PREFETCH? The first available one, after all the TYPE_INTERNAL_*? Right now, it would be 42 :)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Jarda, is this something needed or planned for the next release or months (P2) or more a nice-to-have backlog(P3)? I guess the former, but would like to confirm with you.

Comment 2

[Jarda Snajdr [:jsnajdr]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Hi Jarda, is this something needed or planned for the next release or months
> (P2) or more a nice-to-have backlog(P3)? I guess the former, but would like
> to confirm with you.

I'd like to have it implemented in a timeframe of several weeks at most. I plan to submit a patch myself - I'm only wondering who would be an appropriate reviewer?

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #2)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > Hi Jarda, is this something needed or planned for the next release or months
> > (P2) or more a nice-to-have backlog(P3)? I guess the former, but would like
> > to confirm with you.
> 
> I'd like to have it implemented in a timeframe of several weeks at most. I
> plan to submit a patch myself - I'm only wondering who would be an
> appropriate reviewer?

Hi Tanvi and Christoph, you've reviewed several recent changes on nsIContentPolicyBase::TYPE_*. I am wondering if you are the right people to ask for review. If not, do you happen to know who could help Jarda with review here? Thank you!

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> Hi Tanvi and Christoph, you've reviewed several recent changes on
> nsIContentPolicyBase::TYPE_*. I am wondering if you are the right people to
> ask for review. If not, do you happen to know who could help Jarda with
> review here? Thank you!

We are the right people to ask :-) I would be curious to see the code changes for this bug. Generally I am OK with adding TYPE_PREFETCH, which might also be desired for other bugs, e.g. [1]. Looking at the description of this bug though, I am not entirely sure if we can also use TYPE_PREFETCH for prefetches inititaed by the NetworkPredictor. Anyway, once you have a patch ready, flag us for needinfo and we can take it from there.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1242902#c14

Comment 5

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1310493 - Add TYPE_PREFETCH constant to nsIContentPolicyBase

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

Comment 6

[Jarda Snajdr [:jsnajdr]]

Attached a patch that does the following:

1. Adds TYPE_PREFETCH to nsIContentPolicy

2. Does the necessary dependent changes:
- updates NS_CP_ContentTypeName
- updates static assert at dom/cache/DBSchema.cpp
- updates MapContentPolicyTypeToRequestContext in dom/fetch/InternalRequest
- updates dom/security/nsContentSecurityManager and nsMixedContentBlocker
- updates kTypeString array in extensions/permissions/nsContentBlocker

Questions:
- is the change in extensions/permissions/nsContentBlocker necessary? I'm not sure
- I found out that I need to update dom/security/nsContentSecurityManager and nsMixedContentBlocker only after debug version started crashing on MOZ_ASSERT. Are there any other places like this?

3. Passes TYPE_PREFETCH instead of TYPE_OTHER to channel constructor in netwerk/base/Predictor.cpp and uriloader/prefetch/nsPrefetchService.cpp.

4. Updates Netmonitor in devtools to show "prefetch" as tge cause of TYPE_PREFETCH requests.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8803880 [details]
Bug 1310493 - Add TYPE_PREFETCH constant to nsIContentPolicyBase

(In reply to Jarda Snajdr [:jsnajdr] from comment #6)
> Attached a patch that does the following:

Thanks for providing the description in addition to the patch. That all looks pretty good to me.

> Questions:
> - is the change in extensions/permissions/nsContentBlocker necessary?

I am not certain whether we necessarily need those changes, but I would rather lean towards yes than no.

> - I found out that I need to update dom/security/nsContentSecurityManager
> and nsMixedContentBlocker only after debug version started crashing on
> MOZ_ASSERT. Are there any other places like this?

Browsing through the code I found [1]. TYPE_PREFETCH should also be governed by default-src (at least for now because prefetch requests of TYPE_OTHER would also fall into that directive).


Moving forward I would suggest that we get a network peer to have a quick look at it (probably Nicholas Hurley). I can sign off on the security implications of this change. The only thing left would be the changes witin: extensions/permissions/nsContentBlocker.cpp. I don't know who could help us with that to be honest.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.cpp#251

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8803880 [details]
Bug 1310493 - Add TYPE_PREFETCH constant to nsIContentPolicyBase

https://reviewboard.mozilla.org/r/88086/#review89452

::: dom/security/nsMixedContentBlocker.cpp:476
(Diff revision 1)
>    // TYPE_XMLHTTPREQUEST: XHR requires either same origin or CORS, so most
>    // mixed-content XHR will already be blocked by that check. This will also
>    // block HTTPS-to-HTTP XHR with CORS. The same security concerns mentioned
>    // above for WebSockets apply to XHR, and XHR should have the same security
>    // properties as WebSockets w.r.t. mixed content. XHR's handling of redirects
>    // amplifies these concerns.

Please add a comment for TYPE_PREFETCH here.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information