1207838 - Text for devtools security referrer command should reflect scheme of page

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Kate McKinley [:kmckinley, :Kate]]

The example.com example is hard-coded to http://example.com, but should reflect the scheme of the page.

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Use the scheme of the page URI for the external URI

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Hey Kate, two questions, do we need to keep the 'example.com' part at all, or would it make more sense to complete that part completely? The other thing I was wondering, should we add some additional information if the page does not use any referrer policy? At the moment it just says 'None when downgrade' but I as user don't know that that is the default. What do you think?

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Hey Kate, two questions, do we need to keep the 'example.com' part at all,
> or would it make more sense to complete that part completely? The other
> thing I was wondering, should we add some additional information if the page
> does not use any referrer policy? At the moment it just says 'None when
> downgrade' but I as user don't know that that is the default. What do you
> think?

The code uses example.com as an "other" origin, which may behave differently from the same-origin. I'm not sure what else we should use to indicate an arbitrary other origin. Do you have suggestions?

One option is to change them all to words, so it would be something like:

When visiting               | the Referrer would be
Same origin                 | <same origin referrer>
Other origin                | <other origin referrer>
Same origin, http downgrade | <same origin http downgrade>

We have no way to tell the user whether a referrer policy was not specified and we are using the default. We could say that 'None When Downgrade' is the default.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kate McKinley [:kmckinley] from comment #3)
> One option is to change them all to words, so it would be something like:
> 
> When visiting               | the Referrer would be
> Same origin                 | <same origin referrer>
> Other origin                | <other origin referrer>
> Same origin, http downgrade | <same origin http downgrade>

I like that better, but we can surely also include the URL right next to same origin, other origin, same origin downgrade.

> We have no way to tell the user whether a referrer policy was not specified
> and we are using the default. We could say that 'None When Downgrade' is the
> default.

Yes, we should definitely mention that that is the default, are you sure we can't figure out whether it was explicitly set or not? There must be a way :-)

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 8666971 [details] [diff] [review]
Use the scheme of the page URI for the external URI

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

r+ from me. Thanks.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8666971 [details] [diff] [review]
Use the scheme of the page URI for the external URI

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

Hey Kate, please address my concerns from comment 4 and flag me again for review. Clearing r? for now.

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Update text used for the security referrer command

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8669830 [details] [diff] [review]
Update text used for the security referrer command

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

You know what - Franziskus did a lot of referrer work over the summer. I would like to hear his input. Once he is fine with it I will do the final review and sign off on it.

Comment 10

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8669830 [details] [diff] [review]
Update text used for the security referrer command

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

I like the description better now. But I think we should add a note that "None When Downgrade" is the default (see comment 4). (I don't think we can actually check at the moment if a policy has been set or not as we set it to the default one if we don't find any. We would have to pass around a flag whether we actually found a referrer policy or not.) I'm not sure what's the best way to do this. Maybe just adding "(default)" behind the policy.

But the description is not ideal yet. If I read "Other Origin HTTP" I think "what's with other origin HTTPS?". So I would suggest adding the protocol only if it actually makes a difference if it is HTTP or HTTPS.

The same origin URL has a trailing slash while the others don't have (on github for example with Origin When Cross-Origin). Not sure if that's correct and where it's coming from (there was something, but would have to read up on it again).

Otherwise it looks good to me, flag me again with those changes.

::: devtools/shared/gcli/commands/security.js
@@ +290,5 @@
>            "  <table class='gcli-referrer-policy-detail' cellspacing='10' >" +
>            "    <tr><th> " + NEXT_URI_HEADER + " </th><th> " + CALCULATED_REFERRER_HEADER + " </th></tr>" +
>            // iterate all policies
>            "    <tr foreach='nextURI in ${rpi.urls}' >" +
> +          "      <td> ${nextURI.description} ( i.e., ${nextURI.uri}) </td>" +

the space after the bracket looks a bit odd and this means that we also get "Other Origin HTTP ( i.e., http://example.com/)" where example.com is only an example (would expect e.g. here).

Comment 11

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10)
> Comment on attachment 8669830 [details] [diff] [review]
> Update text used for the security referrer command
> 
> Review of attachment 8669830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the description better now. But I think we should add a note that
> "None When Downgrade" is the default (see comment 4). (I don't think we can
> actually check at the moment if a policy has been set or not as we set it to
> the default one if we don't find any. We would have to pass around a flag
> whether we actually found a referrer policy or not.) I'm not sure what's the
> best way to do this. Maybe just adding "(default)" behind the policy.

We don't currently curry whether it was set or not.

> But the description is not ideal yet. If I read "Other Origin HTTP" I think
> "what's with other origin HTTPS?". So I would suggest adding the protocol
> only if it actually makes a difference if it is HTTP or HTTPS.

It should be the case that HTTP is only added when the origin is HTTPS. If the origin is HTTP, it should only show Same Origin and Other Origin.

> 
> The same origin URL has a trailing slash while the others don't have (on
> github for example with Origin When Cross-Origin). Not sure if that's
> correct and where it's coming from (there was something, but would have to
> read up on it again).

Github's policy is Origin When Cross-Origin, which means we send the full referrer to the same origin, so the path is included. This is more apparent if you visit another page.

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Update text used for the security referrer command

Comment 13

[Franziskus Kiefer [:franziskus]]

(In reply to Kate McKinley [:kmckinley] from comment #11)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10)
> > Comment on attachment 8669830 [details] [diff] [review]
> > Update text used for the security referrer command
> > 
> > Review of attachment 8669830 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > But the description is not ideal yet. If I read "Other Origin HTTP" I think
> > "what's with other origin HTTPS?". So I would suggest adding the protocol
> > only if it actually makes a difference if it is HTTP or HTTPS.
> 
> It should be the case that HTTP is only added when the origin is HTTPS. If
> the origin is HTTP, it should only show Same Origin and Other Origin.

yep that's right, but my question was more along the line why adding HTTP there at all? if we have for example Origin When Cross-Origin on a HTTPS site it makes no difference if the visiting site is HTTP or HTTPS as long is it's a different origin it will be only the origin. That's why I think adding HTTP there is confusing. It also gives no information about what happens when you visit a different origin with HTTPS. So how about omitting the HTTP in this case?

> > The same origin URL has a trailing slash while the others don't have (on
> > github for example with Origin When Cross-Origin). Not sure if that's
> > correct and where it's coming from (there was something, but would have to
> > read up on it again).
> 
> Github's policy is Origin When Cross-Origin, which means we send the full
> referrer to the same origin, so the path is included. This is more apparent
> if you visit another page.

right :)

Comment 14

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> (In reply to Kate McKinley [:kmckinley] from comment #11)
> > (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10)
> > > Comment on attachment 8669830 [details] [diff] [review]
> > > Update text used for the security referrer command
> > > 
> > > Review of attachment 8669830 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > But the description is not ideal yet. If I read "Other Origin HTTP" I think
> > > "what's with other origin HTTPS?". So I would suggest adding the protocol
> > > only if it actually makes a difference if it is HTTP or HTTPS.
> > 
> > It should be the case that HTTP is only added when the origin is HTTPS. If
> > the origin is HTTP, it should only show Same Origin and Other Origin.
> 
> yep that's right, but my question was more along the line why adding HTTP
> there at all? if we have for example Origin When Cross-Origin on a HTTPS
> site it makes no difference if the visiting site is HTTP or HTTPS as long is
> it's a different origin it will be only the origin. That's why I think
> adding HTTP there is confusing. It also gives no information about what
> happens when you visit a different origin with HTTPS. So how about omitting
> the HTTP in this case?

Ah, the included patch does this by checking if the two would be the same and only including the "HTTP" if it would be different.

Comment 15

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8670455 [details] [diff] [review]
Update text used for the security referrer command

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

looks good now, Chris you can have look at it.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8670455 [details] [diff] [review]
Update text used for the security referrer command

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

Looks good. Just some styling issues. r=me and please also request the uplift right after you landed this on mc.

::: devtools/shared/gcli/commands/security.js
@@ +260,5 @@
> +          referrer: otherDomainReferrer,
> +          description: l10n.lookup('securityReferrerPolicyOtherDomain')},
> +        {uri: sameDomainUri,
> +          referrer: sameDomainReferrer,
> +          description: l10n.lookup('securityReferrerPolicySameDomain')}

nit: we have to clean that up a little bit and have the same indentation. Preferrably we have:

{
  uri: pageURI.scheme+'://example.com/',
  referrer: otherDomainReferrer,
  description: l10n.lookup('securityReferrerPolicyOtherDomain')
},
...

@@ +270,5 @@
> +          referrerUrls.push({uri: "http://"+pageURI.hostPort+"/*", referrer: downgradeReferrer, description: l10n.lookup('securityReferrerPolicySameDomainDowngrade')});
> +        }
> +        if (otherDomainReferrer != otherDowngradeReferrer) {
> +          referrerUrls.push({uri: "http://example.com/", referrer: otherDowngradeReferrer, description: l10n.lookup('securityReferrerPolicyOtherDomainDowngrade')});
> +        }

Also here, please align the properties of the object underneath each other and keep the 80 column lines.

Comment 17

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1207838 - Update text used for the security referrer command r=ckerschb,r=jwalker,r=fkiefer

Updated formatting

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/592aa7647aef

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/592aa7647aef

Comment 21

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8674578 [details] [diff] [review]
Bug 1207838 - Update text used for the security referrer command r=ckerschb,r=jwalker,r=fkiefer

Approval Request Comment
[Feature/regressing bug #]: Bug 1186308
[User impact if declined]: Possibly incorrect values will be presented to developers using the gcli tool to test what referrer will be sent.
[Describe test coverage new/current, TreeHerder]: no tests in the gcli component
[Risks and why]: Low. The changes made were to better discriminate between referrers which would be sent to different domains and correct a case where incorrect information would be displayed.
[String/UUID change made/needed]: None

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

This looks like string changes to me, so let's see what the localization folks have to say.

Comment 23

[Axel Hecht [:Pike]]

Yeah, this seems mostly about changing strings.

I'd let this ride the trains.

Note, I didn't try in depth, but it looks like "(no referrer)" and friends are hard-coded English strings? Can you file a follow-up for that if that's actually the case?

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this sounds like a recent regression. 

Kate, how bad is it if we ship this in 43?

Comment 25

[Kate McKinley [:kmckinley, :Kate]]

I think this can wait. Removing the uplift request.