Closed Bug 1186308 Opened 9 years ago Closed 9 years ago

Implement ReferrerPolicy inpsection tool using GCLI

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ckerschb, Assigned: kmckinley)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

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
Blocks: 1129999
Depends on: 704320
Assignee: nobody → kmckinley
Status: NEW → ASSIGNED
Attachment #8648820 - Flags: feedback?(mozilla)
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.
Attachment #8648820 - Flags: feedback?(mozilla) → feedback+
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.
Attachment #8648988 - Flags: feedback?(mozilla)
Attachment #8648988 - Flags: feedback?(jwalker)
Attachment #8648820 - Attachment is obsolete: true
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({...
Attachment #8648988 - Flags: feedback?(mozilla) → feedback+
Attachment #8648988 - Flags: feedback?(jwalker) → feedback+
Looked at the clear background for UI, didn't see anything. Will see if :jwalker has any suggestions.
Attachment #8649536 - Flags: review?(mozilla)
Attachment #8648988 - Attachment is obsolete: true
Attachment #8649566 - Flags: review?(mozilla)
Attachment #8649566 - Flags: review?(jwalker)
Attachment #8649536 - Attachment is obsolete: true
Attachment #8649536 - Flags: review?(mozilla)
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
Attachment #8649536 - Attachment is obsolete: false
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...
Attachment #8649566 - Flags: review?(mozilla)
Got rid of transparency by removing align:left, updated l10n to handle long URIs a little better in header.
Attachment #8650023 - Flags: review?(mozilla)
Attachment #8650023 - Flags: review?(jwalker)
Attachment #8649536 - Attachment is obsolete: true
Attachment #8649566 - Attachment is obsolete: true
Attachment #8649566 - Flags: review?(jwalker)
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?
Attachment #8650023 - Flags: review?(mozilla) → review+
Attachment #8650023 - Attachment is obsolete: true
Attachment #8650023 - Flags: review?(jwalker)
Comment on attachment 8650090 [details] [diff] [review]
Implement ReferrerPolicy inpsection tool using GCLI

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

Thanks Kate.
Attachment #8650090 - Flags: review?(jwalker) → review+
Attachment #8650090 - Attachment is obsolete: true
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
Attachment #8650692 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d9ac2969f3bb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1198229
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.
Flags: needinfo?(mozilla)
(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!
Flags: needinfo?(mozilla)
Thanks! I've renamed the page to "Display security and privacy policies", hope that works for you.
You need to log in before you can comment on or make changes to this bug.