1399990 - Refresh script MIME type telemetry

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Tom S [:evilpie]]

The MIME type telemetry "SCRIPT_BLOCK_WRONG_MIME" added in bug 1288361 expired in Firefox 55. We should add a new categorical telemetry probe for this. (https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html#categorical)

It would also be useful to know the type of script load, i.e modules, worker, importScript, <script> etc.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The other thing that would be useful is knowing whether the load is same-origin, CORS-same-origin, or cross-origin.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Hey Vino, this is also something you could take on please. thanks

Comment 3

[u279076]

Given how close we are to releasing Firefox 57 I don't think this meets the criteria for uplift. Marking wontfix for 57.

Comment 4

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

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

Comment 5

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> The other thing that would be useful is knowing whether the load is
> same-origin, CORS-same-origin, or cross-origin.

Working on recording these values to the defined labels.

Comment 6

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Tom Schuster [:evilpie] from comment #0)

> It would also be useful to know the type of script load, i.e modules,
> worker, importScript, <script> etc.

Type of script load should be recorded generally for all script loads or only when the response is with wrong mime type ?

Comment 7

[Boris Zbarsky [:bzbarsky]]

For purposes of this bug, just for wrong MIME type.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8927799 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

https://reviewboard.mozilla.org/r/199092/#review204136

I am fine with that, but we also need a data peer to sign off (which you have to add to the commit message e.g. r=ckerschb,francois)

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8927799 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

Francois, can you do the data review? Also, can we keep the same name or should we change the name?

Comment 10

[François Marier [:francois]]

Comment on attachment 8927799 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

https://reviewboard.mozilla.org/r/199092/#review204426

::: toolkit/components/telemetry/Histograms.json:7800
(Diff revision 1)
>      "expires_in_version": "never",
>      "kind": "enumerated",
>      "n_values": 3,
>      "description": "Whether the user is in safe mode (No, Yes, Forced)"
>    },
>    "SCRIPT_BLOCK_INCORRECT_MIME": {

Since the type of probe is changing, you'll need to change the name too. The usual convention is to append `_2` to the old name: `SCRIPT_BLOCK_INCORRECT_MIME_2`.

::: toolkit/components/telemetry/Histograms.json:7803
(Diff revision 1)
>      "description": "Whether the user is in safe mode (No, Yes, Forced)"
>    },
>    "SCRIPT_BLOCK_INCORRECT_MIME": {
>      "record_in_processes": ["main", "content"],
>      "alert_emails": ["[e-mail]"],
> -    "bug_numbers": [1288361, 1299267],
> +    "bug_numbers": [1399990],

Would it be worth continuing to link to these old bugs too?

::: toolkit/components/telemetry/Histograms.json:7807
(Diff revision 1)
>      "alert_emails": ["[e-mail]"],
> -    "bug_numbers": [1288361, 1299267],
> -    "expires_in_version": "56",
> -    "kind": "enumerated",
> -    "n_values": 15,
> -    "description": "Whether the script load has a MIME type of ...?  (0=unknown, 1=js, 2=image, 3=audio, 4=video, 5=text/plain, 6=text/csv, 7=text/xml, 8=application/octet-stream, 9=application/xml, 10=text/html, 11=empty)"
> +    "bug_numbers": [1399990],
> +    "expires_in_version": "63",
> +    "kind": "categorical",
> +    "labels": ["unknown","javaScript","image", "audio", "video","text_plain","text_csv","text_xml","app_octet_stream","app_xml","text_html","empty","worker","importSript","script","same_origin","CORS_same_origin","cross_origin"],
> +    "description": "Whether the script load has a wrong MIME type"

I'm a little confused by this description. What exactly is the label? The mimetype that was specified in the HTTP headers insted of the correct one (`application/javascript`)?

Comment 11

[Tom S [:evilpie]]

It would be nice if we could tell the difference between different kinds of loads, <script>, Worker, modules etc. 
See also https://github.com/whatwg/html/issues/3255

Comment 12

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug 1399990 - SCRIPT_BLOCK_INCORRECT_MIME2 and whether same-origin or cross-origin is added;

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

Comment 13

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Tom Schuster [:evilpie] from comment #11)
> It would be nice if we could tell the difference between different kinds of
> loads, <script>, Worker, modules etc. 
> See also https://github.com/whatwg/html/issues/3255

Whether the kind of script load information is already available somewhere in LoadInfo or this information should be newly captured?

Comment 14

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Data review answers.md

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8942202 [details]
Bug 1399990 - SCRIPT_BLOCK_INCORRECT_MIME2 and whether same-origin or cross-origin is added;

https://reviewboard.mozilla.org/r/212466/#review218494

Overall this looks good. Please incorporate my suggestions and ultimately Honza (:mayhemer) should review those changes.

::: netwerk/protocol/http/nsHttpChannel.cpp:1473
(Diff revision 1)
>          // script load has type script
> -        AccumulateCategorical(Telemetry::LABELS_SCRIPT_BLOCK_INCORRECT_MIME::javaScript);
> +        AccumulateCategorical(Telemetry::LABELS_SCRIPT_BLOCK_INCORRECT_MIME_2::javaScript);
>          return NS_OK;
>      }
>  
> +    if (aLoadInfo->LoadingPrincipal()) {

Please use the URI of the triggeringPrincipal for the same origin comparision. Please note the triggeringPrincipal is never null, so you can skip the null check.

Additionally use nsScriptSecurityManager::CheckSameOriginURI() for the same origin check.

Nit: Please move variable definitions closer to their usage. e.g. move requestURI before ->GetURI(requestURI).

Comment 16

[[e-mail]]

Comment on attachment 8942205 [details]
Data review answers.md

1. Yes, this will have standard Telemetry documentation, as it is a revised Telemetry probe.
2. Yes, data preferences will cover this.
3. Currently scheduled to expire in version 63.
4. Type 1.
5. Default on for everyone.
6. No.
7. Yes.
8. Yes, if the requesters want this to be renewed or added to permanent collection.

Comment 17

[[e-mail]]

Comment on attachment 8942202 [details]
Bug 1399990 - SCRIPT_BLOCK_INCORRECT_MIME2 and whether same-origin or cross-origin is added;

Review added to the data review answers form.

Comment 18

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug 1399990 - same origin check for SCRIPT_BLOCK_INCORRECT_MIME2

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

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8943547 [details]
Bug 1399990 - same origin check for SCRIPT_BLOCK_INCORRECT_MIME2

https://reviewboard.mozilla.org/r/213890/#review219676

The code looks good, but I am not sure if we need to change the name of the probe from SCRIPT_BLOCK_INCORRECT_MIME_2 to SCRIPT_BLOCK_INCORRECT_MIME_3. I think adding/renaming labels within a probe mess with our telemtry. Maybe Honza knows, otherwise we have to find someone who knows.

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8943547 [details]
Bug 1399990 - same origin check for SCRIPT_BLOCK_INCORRECT_MIME2

Sorry, guys, but I really don't know if the labeling is just informational or not.  Maybe some of the data stewards would know?  https://wiki.mozilla.org/Firefox/Data_Collection

btw, I don't see SCRIPT_BLOCK_INCORRECT_MIME_2 in the drop down on t.m.o.

Comment 21

[Chris H-C :chutten]

Drive-by Telemetry commentary!

SCRIPT_BLOCK_INCORRECT_MIME2/SCRIPT_BLOCK_INCORRECT_MIME_2 don't yet exist, do they? I don't see them in Histograms.json, and it appears to have been introduced in the second of the three attached patches. Since we haven't yet sent or received any data for _2, you can change it however you'd like until it goes live.

From a theoretical perspective, if _2 had already landed and this were a patch to change the name and meaning of one of the categorical labels... that _should_ work just fine. Personally, if I were changing how I was measuring that category, I would add a new label to the end and start recording to that instead. This would ensure a clear separation between data recorded before the code change and data recorded after.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Chris H-C :chutten from comment #21)
> Drive-by Telemetry commentary!
> 
> SCRIPT_BLOCK_INCORRECT_MIME2/SCRIPT_BLOCK_INCORRECT_MIME_2 don't yet exist,
> do they? I don't see them in Histograms.json, and it appears to have been
> introduced in the second of the three attached patches. Since we haven't yet
> sent or received any data for _2, you can change it however you'd like until
> it goes live.

Ah, it was reviewboard that confused me. You are right, SCRIPT_BLOCK_INCORRECT_MIME_2 does not show up in Histograms.json. Thanks for clarification Chris.

Honza, given that, can you sign off on the patch?

Comment 23

[Honza Bambas (:mayhemer)]

I will when you ask for it.  I don't see r? on me.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8943547 [details]
Bug 1399990 - same origin check for SCRIPT_BLOCK_INCORRECT_MIME2

https://reviewboard.mozilla.org/r/213890/#review220216

::: netwerk/protocol/http/nsHttpChannel.cpp:1471
(Diff revision 1)
>          return NS_OK;
>      }
>  
> -    if (aLoadInfo->LoadingPrincipal()) {
> -        aLoadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(requestURI));
> +    nsCOMPtr<nsIURI> requestURI;
> +    nsAutoCString requestURIString;
> +    aLoadInfo->TriggeringPrincipal()->GetURI(getter_AddRefs(requestURI));

can you document why you are using TriggeringPrincipal instead?  (:ckerschb is probably your source of info)

::: netwerk/protocol/http/nsHttpChannel.cpp:1487
(Diff revision 1)
> -            bool cors = false;
> +        bool cors = false;
> -            nsresult rv = aResponseHead->GetHeader(nsHttp::ResolveAtom("Access-Control-Allow-Origin"), corsOrigin);
> +        nsAutoCString corsOrigin;
> +        rv = aResponseHead->GetHeader(nsHttp::ResolveAtom("Access-Control-Allow-Origin"), corsOrigin);
> -            if (NS_SUCCEEDED(rv)) {
> +        if (NS_SUCCEEDED(rv)) {
> -                //cors origin
> +            //cors origin
> -                if (corsOrigin.Equals("*") || corsOrigin.Equals(requestURIString)) {
> +            if (corsOrigin.Equals("*") || corsOrigin.Equals(requestURIString)) {

should we construct URI from the corsOrigin string and subject to ssm->CheckSameOriginURI as well?

Comment 25

[Vinothkumar Nagasayanan [:vinoth]]

> ::: netwerk/protocol/http/nsHttpChannel.cpp:1487
> (Diff revision 1)
> > -            bool cors = false;
> > +        bool cors = false;
> > -            nsresult rv = aResponseHead->GetHeader(nsHttp::ResolveAtom("Access-Control-Allow-Origin"), corsOrigin);
> > +        nsAutoCString corsOrigin;
> > +        rv = aResponseHead->GetHeader(nsHttp::ResolveAtom("Access-Control-Allow-Origin"), corsOrigin);
> > -            if (NS_SUCCEEDED(rv)) {
> > +        if (NS_SUCCEEDED(rv)) {
> > -                //cors origin
> > +            //cors origin
> > -                if (corsOrigin.Equals("*") || corsOrigin.Equals(requestURIString)) {
> > +            if (corsOrigin.Equals("*") || corsOrigin.Equals(requestURIString)) {
> 
> should we construct URI from the corsOrigin string and subject to
> ssm->CheckSameOriginURI as well?

No not needed, because a IF loop first checks whether 'requestURIString' is a Same origin using 'ssm->CheckSameOriginURI'. 
Above code comes inside the ELSE loop and this piece of code won't be executed if it is a Same origin. So again same origin check is not required here.

Comment 26

[Vinothkumar Nagasayanan [:vinoth]]

> ::: netwerk/protocol/http/nsHttpChannel.cpp:1471
> (Diff revision 1)
> >          return NS_OK;
> >      }
> >  
> > -    if (aLoadInfo->LoadingPrincipal()) {
> > -        aLoadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(requestURI));
> > +    nsCOMPtr<nsIURI> requestURI;
> > +    nsAutoCString requestURIString;
> > +    aLoadInfo->TriggeringPrincipal()->GetURI(getter_AddRefs(requestURI));
> 
> can you document why you are using TriggeringPrincipal instead?  (:ckerschb
> is probably your source of info)
> 

Chris can you please provide me some info here on the usage of TriggeringPrincipal() instead of LoadingPrincipal().

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #25)
> > > +            if (corsOrigin.Equals("*") || corsOrigin.Equals(requestURIString)) {
> > 
> > should we construct URI from the corsOrigin string and subject to
> > ssm->CheckSameOriginURI as well?
> 
> No not needed, because a IF loop first checks whether 'requestURIString' is
> a Same origin using 'ssm->CheckSameOriginURI'. 
> Above code comes inside the ELSE loop and this piece of code won't be
> executed if it is a Same origin. So again same origin check is not required
> here.

Your patch 2 of 3 adds the same origin compare as uri-no-path string compare [1].  Chris has suggested to use CheckSameOriginURI of ss manager.  So you've created the patch 3 of 3 to address that change.

The piece I refer to is just another "is-same-origin" check you do, this time on the Access-Control-Allow-Origin response header.  You, again as in the patch 2 of 3, do a plain string compare.  But I think (suggest, not enforce) this should use CheckSameOriginURI as well (you should construct an URI object from the value of the response header - NS_NewURI - and pass it to CheckSameOriginURI).  Also note that plain string compare is case-sensitive.

[1]
if (aLoadInfo->LoadingPrincipal()) {
        aLoadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(requestURI));
        if (requestURI) {
            requestURI->GetPrePath(requestURIString);
        }
        aURI->GetPrePath(responseURIString);
        if (requestURIString.Equals(responseURIString)) {
            //same origin
            AccumulateCategorical(Telemetry::LABELS_SCRIPT_BLOCK_INCORRECT_MIME_2::same_origin);

Comment 28

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Honza Bambas (:mayhemer) from comment #27)
> (In reply to Vinothkumar Nagasayanan [:vinoth] from comment #25)
> > > > +            if (corsOrigin.Equals("*") || corsOrigin.Equals(requestURIString)) {
> > > 
> > > should we construct URI from the corsOrigin string and subject to
> > > ssm->CheckSameOriginURI as well?
> > 
> > No not needed, because a IF loop first checks whether 'requestURIString' is
> > a Same origin using 'ssm->CheckSameOriginURI'. 
> > Above code comes inside the ELSE loop and this piece of code won't be
> > executed if it is a Same origin. So again same origin check is not required
> > here.
> 
> Your patch 2 of 3 adds the same origin compare as uri-no-path string compare
> [1].  Chris has suggested to use CheckSameOriginURI of ss manager.  So
> you've created the patch 3 of 3 to address that change.
> 
> The piece I refer to is just another "is-same-origin" check you do, this
> time on the Access-Control-Allow-Origin response header.  You, again as in
> the patch 2 of 3, do a plain string compare.  But I think (suggest, not
> enforce) this should use CheckSameOriginURI as well (you should construct an
> URI object from the value of the response header - NS_NewURI - and pass it
> to CheckSameOriginURI).  Also note that plain string compare is
> case-sensitive.
> 
> [1]
> if (aLoadInfo->LoadingPrincipal()) {
>         aLoadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(requestURI));
>         if (requestURI) {
>             requestURI->GetPrePath(requestURIString);
>         }
>         aURI->GetPrePath(responseURIString);
>         if (requestURIString.Equals(responseURIString)) {
>             //same origin
>            
> AccumulateCategorical(Telemetry::LABELS_SCRIPT_BLOCK_INCORRECT_MIME_2::
> same_origin);

Now I got it. I will make this change. Thanks.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #28)
> > if (aLoadInfo->LoadingPrincipal()) {
> >         aLoadInfo->LoadingPrincipal()->GetURI(getter_AddRefs(requestURI));
> >         if (requestURI) {
> >             requestURI->GetPrePath(requestURIString);
> >         }
> >         aURI->GetPrePath(responseURIString);
> >         if (requestURIString.Equals(responseURIString)) {
> >             //same origin
> >            
> > AccumulateCategorical(Telemetry::LABELS_SCRIPT_BLOCK_INCORRECT_MIME_2::
> > same_origin);

I am fine with using the LoadingPrincipal in this case. In fact it's more accurate.

Comment 30

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8927799 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199092/diff/1-2/

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8927799 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

https://reviewboard.mozilla.org/r/199092/#review224572

::: netwerk/protocol/http/nsHttpChannel.cpp:1491
(Diff revisions 1 - 2)
> +            nsCOMPtr<nsIURI> corsOriginURI;
> +            rv = NS_NewURI(getter_AddRefs(corsOriginURI), corsOrigin);
> +            if (NS_SUCCEEDED(rv)) {
> +                rv = ssm->CheckSameOriginURI(requestURI, corsOriginURI, false);
> +                //cors origin
> +                if (corsOrigin.Equals("*") || NS_SUCCEEDED(rv)) {

corsOrigin.Equals("*") then you will very likely fail to construct an URI object from it (NS_NewURI will fail)

Comment 32

[Phabricator Automation]

Attachment: Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

Comment 33

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Honza Bambas (:mayhemer) from comment #31)
> Comment on attachment 8927799 [details]
> Bug 1399990 - Files added for New Categorical telemetry
> SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done
> 
> https://reviewboard.mozilla.org/r/199092/#review224572
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp:1491
> (Diff revisions 1 - 2)
> > +            nsCOMPtr<nsIURI> corsOriginURI;
> > +            rv = NS_NewURI(getter_AddRefs(corsOriginURI), corsOrigin);
> > +            if (NS_SUCCEEDED(rv)) {
> > +                rv = ssm->CheckSameOriginURI(requestURI, corsOriginURI, false);
> > +                //cors origin
> > +                if (corsOrigin.Equals("*") || NS_SUCCEEDED(rv)) {
> 
> corsOrigin.Equals("*") then you will very likely fail to construct an URI
> object from it (NS_NewURI will fail)

Thanks for pointing out. Those changes are made now.

Comment 34

[Phabricator Automation]

Comment on attachment 8949746 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

Honza Bambas (:mayhemer) has approved the revision.

https://phabricator.services.mozilla.com/D569

Comment 35

[Honza Bambas (:mayhemer)]

(In reply to Phabricator Automation from comment #34)
> Comment on attachment 8949746 [details]
> Bug 1399990 - Files added for New Categorical telemetry
> SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done
> 
> Honza Bambas (:mayhemer) has approved the revision.
> 
> https://phabricator.services.mozilla.com/D569

please don't miss the comments https://phabricator.services.mozilla.com/D569#12682

Comment 36

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Honza Bambas (:mayhemer) from comment #35)
> (In reply to Phabricator Automation from comment #34)
> > Comment on attachment 8949746 [details]
> > Bug 1399990 - Files added for New Categorical telemetry
> > SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done
> > 
> > Honza Bambas (:mayhemer) has approved the revision.
> > 
> > https://phabricator.services.mozilla.com/D569
> 
> please don't miss the comments
> https://phabricator.services.mozilla.com/D569#12682

Yes made the changes now.

Comment 37

[Phabricator Automation]

Comment on attachment 8949746 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D569

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c69ec6a80fa2
Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done r=ckerschb,mayhemer

Comment 39

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c69ec6a80fa2