1186308 - Implement ReferrerPolicy inpsection tool using GCLI

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Since we have the CSP policy inspection tool in GCLI (Bug 1129999) [1] we could/should also provide an inspection tool for Referrer Policies.


[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/security.js#33

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8648820 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

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

Hey Kate, from what I am seeing you are listing all referrers, that would be send out, using all the potential policies. Looks pretty good, the only thing I am slightly worried is, that we don't have any 'default' case in the switch statement, what happens to our devtool in case policies are extended or maybe some of them will be removed. I am worried that someone will just update the policies in the actual code that is needed for policies, but not in the devtool. Would be great if we can reuse some of the gecko code, e.g. reuse the same string constants. Otherwise this looks great!

::: toolkit/devtools/gcli/commands/security.js
@@ +48,5 @@
> +/*   same as default, but reduced to ORIGIN when cross-origin.      */
> +const REFERRER_POLICY_ORIGIN_WHEN_XORIGIN        = 3;
> +/*   always sends the referrer, even on downgrade.                  */
> +const REFERRER_POLICY_UNSAFE_URL                 = 4;
> +

I am not completely sure, but I think you should be able to query those values from the *.idl. I was hoping we could get away without redefining those constants, something like:
Ci.nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE

@@ +50,5 @@
> +/*   always sends the referrer, even on downgrade.                  */
> +const REFERRER_POLICY_UNSAFE_URL                 = 4;
> +
> +/* The official names from the W3C Referrer Policy Draft http://www.w3.org/TR/referrer-policy/ */
> +const REFERRER_POLICY_NAMES = [ "None When Downgrade", "None", "Origin Only", "Origin When Cross-Origin", "Unsafe URL" ];

A quick mxr search didn't give me any results, but aren't those strings also defined somewhere already? Maybe we can reuse them.

@@ +203,5 @@
> +    manual: "", // TODO l10n.lookup("securityReferrerPolicyManual"),
> +    returnType: "securityReferrerPolicyInfo",
> +    exec: function(args, context) {
> +
> +      var referrerPolicy = context.environment.document.referrerPolicy;

maybe even store the document in it's on variable, then you can query the referrerPolicy or also the documentURI.

var doc = context.environment.doc;
var referrerPolicy = doc.referrerPolicy;
var uri = doc.documentURI;

@@ +218,5 @@
> +      const unsigned long REFERRER_POLICY_ORIGIN_WHEN_XORIGIN        = 3;
> +      /*   always sends the referrer, even on downgrade.                  * /
> +      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> +      */
> +      console.log("csp: "+referrerPolicy);

please remove those constants from within the function body.

@@ +220,5 @@
> +      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> +      */
> +      console.log("csp: "+referrerPolicy);
> +
> +      var currentPageUri = NetUtil.newURI(context.environment.document.location);

as mentioned above, you can use doc.documentURI, then you don't have to create a new URI here.

@@ +226,5 @@
> +      console.log("host:"+currentPageUri.host);
> +      var sameDomainReferrer = "";
> +      var otherDomainReferrer = "";
> +      var downgradeReferrer = "";
> +      var origin = currentPageUri.scheme+"://"+currentPageUri.hostPort;

wouldn't 'prePath' work here? see
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIURI.idl#97

@@ +259,5 @@
> +      var sameDomainUri = currentPageUri.scheme+"://"+currentPageUri.hostPort+"/*";
> +
> +      var referrerUrls = [];
> +      referrerUrls.push({uri: 'http://example.com/', referrer: otherDomainReferrer});
> +      referrerUrls.push({uri: sameDomainUri, referrer: sameDomainReferrer});

Please add a little documentation what those two lines are doing.

@@ +264,5 @@
> +
> +      console.log('http://example.com/'+": "+otherDomainReferrer);
> +      console.log(sameDomainUri+": "+sameDomainReferrer);
> +
> +      if (currentPageUri.scheme == 'https') {

Please use schemeis, since it's more robust, please see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIURI.idl#170

@@ +291,5 @@
> +          "<table class='gcli-referrer-policy-detail' cellspacing='10' valign='top' align='left'>" +
> +          "  <tr><th>Referrer Policy</th><th>${rpi.policyName}</th></tr>" +
> +          // iterate all policies
> +          "  <tr foreach='uri in ${rpi.urls}' >" +
> +          "    <td> Visiting ${uri.uri} </td>" +

maybe use a different name for the running variable, uri.uri seems confusing.

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

Thanks for the feedback! A couple of comments inline...

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Comment on attachment 8648820 [details] [diff] [review]
> Implement ReferrerPolicy inpsection tool using GCLI
> 
> Review of attachment 8648820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Kate, from what I am seeing you are listing all referrers, that would be
> send out, using all the potential policies. Looks pretty good, the only
> thing I am slightly worried is, that we don't have any 'default' case in the
> switch statement, what happens to our devtool in case policies are extended
> or maybe some of them will be removed. I am worried that someone will just
> update the policies in the actual code that is needed for policies, but not
> in the devtool. Would be great if we can reuse some of the gecko code, e.g.
> reuse the same string constants. Otherwise this looks great!

I changed to include an explicit default case that would be shown if a new, unknown referrer policy is added, which basically says we don't know it.

> @@ +50,5 @@
> > +/*   always sends the referrer, even on downgrade.                  */
> > +const REFERRER_POLICY_UNSAFE_URL                 = 4;
> > +
> > +/* The official names from the W3C Referrer Policy Draft http://www.w3.org/TR/referrer-policy/ */
> > +const REFERRER_POLICY_NAMES = [ "None When Downgrade", "None", "Origin Only", "Origin When Cross-Origin", "Unsafe URL" ];
> 
> A quick mxr search didn't give me any results, but aren't those strings also
> defined somewhere already? Maybe we can reuse them.

The only place I could find the strings was in tests, so I didn't want to rely on them.

> @@ +220,5 @@
> > +      const unsigned long REFERRER_POLICY_UNSAFE_URL                 = 4;
> > +      */
> > +      console.log("csp: "+referrerPolicy);
> > +
> > +      var currentPageUri = NetUtil.newURI(context.environment.document.location);
> 
> as mentioned above, you can use doc.documentURI, then you don't have to
> create a new URI here.

For some reason, I am not getting a URI, but a string when I use documentURI.

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8648988 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

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

Great, looks pretty good, looking forward to see a demo tomorrow :-)

::: toolkit/devtools/gcli/commands/security.js
@@ +37,5 @@
>  const NO_CSP_ON_PAGE_MSG = l10n.lookup("securityCSPNoCSPOnPage");
>  const CONTENT_SECURITY_POLICY_MSG = l10n.lookup("securityCSPHeaderOnPage");
>  const CONTENT_SECURITY_POLICY_REPORT_ONLY_MSG = l10n.lookup("securityCSPROHeaderOnPage");
>  
> +const NEXT_URI_MSG = "When Visiting (Translate me)"; //l10n.lookup("securityReferrerNextURL");

You can add your string locals here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties#1589

@@ +40,5 @@
>  
> +const NEXT_URI_MSG = "When Visiting (Translate me)"; //l10n.lookup("securityReferrerNextURL");
> +const CALCULATED_REFERRER_MSG = "Referrer Will Be (Translate me)"; //l10n.lookup("securityReferrerCalculatedReferrer");
> +/* The official names from the W3C Referrer Policy Draft http://www.w3.org/TR/referrer-policy/ */
> +const REFERRER_POLICY_NAMES = [ "None When Downgrade", "None", "Origin Only", "Origin When Cross-Origin", "Unsafe URL" ];

nit, maybe align them underneath (but totally up to you)
const REFERRER_POLICY_NAMES = [
  "None When Downgrade",
  "None",
  "Origin Only",
  "Origin When Cross-Origin",
  "Unsafe URL"
  "error bla bla" // see comment further down
];

@@ +189,5 @@
> +    item: "command",
> +    runAt: "server",
> +    name: "security referrer",
> +    description: "", // TODO l10n.lookup("securityReferrerPolicyDesc"),
> +    manual: "", // TODO l10n.lookup("securityReferrerPolicyManual"),

as above; please define them in gclicommands.properties. I just went through the trouble of using hardcoded strings, you don't wanna go down that road :-)

@@ +196,5 @@
> +      var doc = context.environment.document;
> +
> +      var referrerPolicy = doc.referrerPolicy;
> +
> +      var pageURI = NetUtil.newURI(doc.documentURI);

Is it possible that it is document.documentURIObject, maybe that does the trick, not sure what the difference between the two is by only looking at the implementation.

@@ +228,5 @@
> +          downgradeReferrer = "(no referrer)";
> +          break;
> +        default:
> +          sameDomainReferrer = otherDomainReferrer = downgradeReferrer = "(unknown Referrer Policy)";
> +          break;

nice, that's a better default case. thanks, but should we also set referrerPolicy? Otherwise you might index out of bounds underneath when we hit:
policyName: REFERRER_POLICY_NAMES[referrerPolicy],

Maybe add yet another string to  REFERRER_POLICY_NAMES and set referrerPolicy to that index for the default case. At least that makes the problem easy to localize in case the implementation ever changes.

@@ +241,5 @@
> +
> +      if (pageURI.schemeIs('https')) {
> +        // add the referrer we would send on downgrading http->https
> +        var downgradeUri = "http://"+pageURI.hostPort+"/*";
> +        referrerUrls.push({uri: downgradeUri, referrer: downgradeReferrer});

nit you could remove the tmp variable and directly inline within push({...

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI

Looked at the clear background for UI, didn't see anything. Will see if :jwalker has any suggestions.

Comment 7

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8649536 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

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

Cool, looks great. Let me see it one more time and then we can hand it off to jwalker for a final review.

::: toolkit/devtools/gcli/commands/security.js
@@ +196,5 @@
> +      var doc = context.environment.document;
> +
> +      var referrerPolicy = doc.referrerPolicy;
> +
> +      var pageURI = NetUtil.newURI(doc.documentURI);

doc.documentURIObject also didn't return an nsIURI?

@@ +215,5 @@
> +        case Ci.nsIHttpChannel.REFERRER_POLICY_ORIGIN_WHEN_XORIGIN:
> +          // same as default, but reduced to ORIGIN when cross-origin.
> +          sameDomainReferrer = pageURI.spec;
> +          otherDomainReferrer = origin;
> +          downgradeReferrer = "(no referrer)"

nit, missing semicolon

@@ +216,5 @@
> +          // same as default, but reduced to ORIGIN when cross-origin.
> +          sameDomainReferrer = pageURI.spec;
> +          otherDomainReferrer = origin;
> +          downgradeReferrer = "(no referrer)"
> +            break;

nit, indentation

@@ +237,5 @@
> +
> +      var referrerUrls = [];
> +      // add the referrer uri 'referrer' we would send when visiting 'uri'
> +      referrerUrls.push({uri: 'http://example.com/', referrer: otherDomainReferrer});
> +      referrerUrls.push({uri: sameDomainUri, referrer: sameDomainReferrer});

nit you can inline those instead of using push right after.

var referrerUrls = [
  {uri: 'http://example.com/', referrer: otherDomainReferrer},
  {uri: sameDomainUri, referrer: sameDomainReferrer}
];

@@ +241,5 @@
> +      referrerUrls.push({uri: sameDomainUri, referrer: sameDomainReferrer});
> +
> +      if (pageURI.schemeIs('https')) {
> +        // add the referrer we would send on downgrading http->https
> +        var downgradeUri = "http://"+pageURI.hostPort+"/*";

nit: remove the tmp variable here.

@@ +260,5 @@
> +      return context.createView({
> +          html:
> +          "<div class='gcli-referrer-policy'>" +
> +          "  <strong> Referrer Policy: </strong> ${rpi.policyName} <br />" +
> +          "  <table class='gcli-referrer-policy-detail' cellspacing='10' valign='top' align='left'>" +

you mentioned that align='left' makes it transparent. whatever you end up doing please do the same for CSP above so we are consistent. either both transparent, or none of them transparent. thanks.

@@ +261,5 @@
> +          html:
> +          "<div class='gcli-referrer-policy'>" +
> +          "  <strong> Referrer Policy: </strong> ${rpi.policyName} <br />" +
> +          "  <table class='gcli-referrer-policy-detail' cellspacing='10' valign='top' align='left'>" +
> +          "    <tr><th> " + NEXT_URI_MSG + " </th><th> " + CALCULATED_REFERRER_MSG + " </th></tr>" +

this line becomes a little easier to read once you make the local string allow to process and argument.

::: toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties
@@ +1588,5 @@
>  securityCSPHeaderOnPage=Content-Security-Policy for
>  securityCSPROHeaderOnPage=Content-Security-Policy-Report-Only for
> +# Referrer Policy specific
> +securityReferrerPolicyDesc=Display the current Referrer Policy
> +securityReferrerPolicyManual=Display the Referrer Policy of the current page with exmample referrers for different URIs.

Nit: since we are using 'Content-Security-Policy for' maybe we should also use
'Referrer Policy for' + uri + ' including example referrer for different URIs.

I think from a UX perspective, that would be nice.

Also, you misspelled 'exmample'.

@@ +1589,5 @@
>  securityCSPROHeaderOnPage=Content-Security-Policy-Report-Only for
> +# Referrer Policy specific
> +securityReferrerPolicyDesc=Display the current Referrer Policy
> +securityReferrerPolicyManual=Display the Referrer Policy of the current page with exmample referrers for different URIs.
> +securityReferrerNextURI=When Visiting

Actually maybe we should let that take an argument, see e.g: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/screenshot.js#172

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8649566 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

Huh, why didn't it clear my review request. I thought I cleared the flag...

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI

Got rid of transparency by removing align:left, updated l10n to handle long URIs a little better in header.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8650023 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

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

Ship it!!

::: toolkit/devtools/gcli/commands/security.js
@@ +235,5 @@
> +
> +      var sameDomainUri = origin + "/*";
> +
> +      var referrerUrls = [
> +      // add the referrer uri 'referrer' we would send when visiting 'uri'

nit: indentation

::: toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties
@@ +1566,1 @@
>  

Since this is more a privacy feature than a security feature we should update:
securityDesc=Display supported security and privacy features

@@ +1570,5 @@
>  
>  # LOCALIZATION NOTE (folderOpenDirResult) A very short string used to
>  # describe the result of the 'folder open' command.
>  # The argument (%1$S) is the folder path.
> +ForfolderOpenDirResult=Opened %1$S

Did you change that accidentally? Or why did you change that?

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI

Comment 13

[Joe Walker [:jwalker]]

Comment on attachment 8650090 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

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

Thanks Kate.

Comment 14

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Implement ReferrerPolicy inpsection tool using GCLI (

Comment 15

[Kate McKinley [:kmckinley, :Kate]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f0350d6d447

Comment 16

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8650692 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI (

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

Carrying over r+ from jwalker and ckerschb

Comment 17

[Kate McKinley [:kmckinley, :Kate]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91274c7195ec

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ac2969f3bb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Will Bamberg [:wbamberg]]

I've updated this page: https://developer.mozilla.org/en-US/docs/Tools/GCLI/Display_security_information to talk about this command. Let me know if it looks OK.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Will Bamberg [:wbamberg] from comment #20)
> I've updated this page:
> https://developer.mozilla.org/en-US/docs/Tools/GCLI/
> Display_security_information to talk about this command. Let me know if it
> looks OK.

Thanks Will. From a technical perspective everything looks fine to me. Probably we could replace the headline 'Display security information' with 'Display Security and Privacy settings of a Website' since referrer-policy is rather privacy related than security related. Other than that the page looks great!

Comment 22

[Will Bamberg [:wbamberg]]

Thanks! I've renamed the page to "Display security and privacy policies", hope that works for you.