Refresh script MIME type telemetry

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
a year ago
18 days ago

People

(Reporter: evilpie, Assigned: vinoth)

Tracking

(Blocks: 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox60 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
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.
The other thing that would be useful is knowing whether the load is same-origin, CORS-same-origin, or cross-origin.
Assignee: nobody → ckerschb
Priority: -- → P1
Whiteboard: [domsecurity-active]
Hey Vino, this is also something you could take on please. thanks
Assignee: ckerschb → cegvinoth
Flags: needinfo?(cegvinoth)
Given how close we are to releasing Firefox 57 I don't think this meets the criteria for uplift. Marking wontfix for 57.
status-firefox57: affected → wontfix
status-firefox58: --- → affected
Comment hidden (mozreview-request)
(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.
Flags: needinfo?(cegvinoth)
(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 ?
For purposes of this bug, just for wrong MIME type.

Comment 8

a year ago
mozreview-review
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)
Attachment #8927799 - Flags: review?(ckerschb) → review+
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?
Attachment #8927799 - Flags: review?(francois)

Comment 10

a year ago
mozreview-review
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": ["ckerschbaumer@mozilla.com"],
> -    "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": ["ckerschbaumer@mozilla.com"],
> -    "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`)?
Attachment #8927799 - Flags: review?(francois) → review-
(Reporter)

Comment 11

a year ago
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 hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8927799 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Updated

11 months ago
Attachment #8942202 - Flags: review?(rweiss)
(Assignee)

Comment 14

11 months ago
Created attachment 8942205 [details]
Data review answers.md
Attachment #8942205 - Flags: review?(rweiss)

Comment 15

11 months ago
mozreview-review
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).
Attachment #8942202 - Flags: review?(ckerschb)

Comment 16

11 months ago
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.
Attachment #8942205 - Flags: review?(rweiss) → review+

Comment 17

11 months ago
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.
Attachment #8942202 - Flags: review?(rweiss) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8942202 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8927799 - Attachment is obsolete: true

Comment 19

11 months ago
mozreview-review
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.
Attachment #8943547 - Flags: review?(ckerschb)
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.
Attachment #8943547 - Flags: review?(honzab.moz)

Comment 21

11 months ago
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.
(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?
Flags: needinfo?(honzab.moz)
I will when you ask for it.  I don't see r? on me.
Flags: needinfo?(honzab.moz)
(Assignee)

Updated

11 months ago
Attachment #8943547 - Flags: review?(honzab.moz)

Comment 24

11 months ago
mozreview-review
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?
Attachment #8943547 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 25

11 months ago
> ::: 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.
(Assignee)

Comment 26

11 months ago
> ::: 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().
Flags: needinfo?(ckerschb)
(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);
(Assignee)

Comment 28

11 months ago
(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.
(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.
Flags: needinfo?(ckerschb)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8943547 - Attachment is obsolete: true

Comment 31

10 months ago
mozreview-review
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)
Attachment #8927799 - Flags: review?(honzab.moz) → review-
(Assignee)

Updated

10 months ago
Attachment #8927799 - Attachment is obsolete: true
Created attachment 8949746 [details]
Bug 1399990 - Files added for New Categorical telemetry SCRIPT_BLOCK_INCORRECT_MIME_2 and same origin check done
(Assignee)

Updated

10 months ago
Attachment #8949746 - Flags: review?(ckerschb)
(Assignee)

Updated

10 months ago
Attachment #8949746 - Flags: review?(honzab.moz)
(Assignee)

Comment 33

10 months ago
(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 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
Attachment #8949746 - Flags: review+
Attachment #8949746 - Flags: review?(honzab.moz)
(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
(Assignee)

Comment 36

10 months ago
(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 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
Attachment #8949746 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8949746 - Flags: review?(ckerschb)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 38

10 months ago
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed

Comment 39

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c69ec6a80fa2
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
status-firefox58: affected → wontfix
(Reporter)

Updated

18 days ago
Blocks: 1510225
You need to log in before you can comment on or make changes to this bug.