Closed Bug 1207838 Opened 6 years ago Closed 6 years ago

Text for devtools security referrer command should reflect scheme of page

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 + wontfix
firefox44 + fixed

People

(Reporter: kmckinley, Assigned: kmckinley)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

The example.com example is hard-coded to http://example.com, but should reflect the scheme of the page.
Attachment #8666971 - Flags: review?(mozilla)
Attachment #8666971 - Flags: review?(jwalker)
Assignee: nobody → kmckinley
Status: NEW → ASSIGNED
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?
Flags: needinfo?(kmckinley)
(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.
Flags: needinfo?(kmckinley)
(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 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.
Attachment #8666971 - Flags: review?(jwalker) → review+
Duplicate of this bug: 1210916
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.
Attachment #8666971 - Flags: review?(mozilla)
Attachment #8666971 - Attachment is obsolete: true
Attachment #8669830 - Flags: review?(mozilla)
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.
Attachment #8669830 - Flags: review?(mozilla) → review?(franziskuskiefer)
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).
Attachment #8669830 - Flags: review?(franziskuskiefer)
(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.
Attachment #8669830 - Attachment is obsolete: true
Attachment #8670455 - Flags: review?(franziskuskiefer)
(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 :)
Flags: needinfo?(kmckinley)
(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.
Flags: needinfo?(kmckinley)
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.
Attachment #8670455 - Flags: review?(mozilla)
Attachment #8670455 - Flags: review?(franziskuskiefer)
Attachment #8670455 - Flags: review+
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.
Attachment #8670455 - Flags: review?(mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/592aa7647aef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Attachment #8674578 - Flags: approval-mozilla-aurora?
This looks like string changes to me, so let's see what the localization folks have to say.
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
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?
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
Tracking since this sounds like a recent regression. 

Kate, how bad is it if we ship this in 43?
Attachment #8674578 - Flags: approval-mozilla-aurora?
I think this can wait. Removing the uplift request.
You need to log in before you can comment on or make changes to this bug.