1221808 - Control centre may crop the important part of nsSimpleURIs (eg, about: URLs)

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

If you sign in to Sync, you end up on a page with URL about:accounts?action=signup&entrypoint=menupanel. If you view the control centre for that page, you get https://bugzilla.mozilla.org/attachment.cgi?id=8683343 - ie, the url is shown as "...on=signup&entrypoint=menupanel" - so the important part of the URL (ie, the about:accounts part) has been cropped.

MattN said this might be a regression from bug 1204616. A quick look at the code shows that because we have a simpleURI, the getEffectiveHost() call in browser.js will return an emptry string (as uri.host throws for simple URIs), so the code then falls back to uri.specIgnoringRef, which returns the entire URL.

I guess we either want to treat about: URIs as a special case and manually trim the query portion, or at least get it to crop the end rather than the start.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1221808-have-the-control-centre-crop-the-end-for.patch

Hi Richard,
  This problem seems to be related to bug 1204616, hence I'm asking you for some advice.

The simplest fix is as attached - it arranges to crop the end of the URL when the URI has no host. Another option might be to try and remove the querystring, but (a) this means doing it manually on the string representation of the URI as SimpleURIs don't know about query strings, and (b) about:averylongaboutstring might still trim the "about:" portion if we did that.

Thoughts?

Comment 2

[Richard Barnes [:rbarnes]]

Comment on attachment 8683399 [details] [diff] [review]
0001-Bug-1221808-have-the-control-centre-crop-the-end-for.patch

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

This seems like it's probably OK.  IIRC, the risks we were trying to address in Bug 1204616 were related to an attacker using a name like "facebook.com.alotofextrapaddingtogetcropped.nefarious.com" and that getting displayed as "facebook.com..." instead of "...nefarious.com".

If there's no host, then it seems like you're not going to be in a case where you have that kind of spoofing risk, so it's better to start at the beginning.  In fact, it seems kind of nice to do that, because it highlights the unusual URL scheme.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8683399 [details] [diff] [review]
0001-Bug-1221808-have-the-control-centre-crop-the-end-for.patch

Thanks for the comment. It appears there are no tests for this, so best I can see, this patch is all that is necessary?

Comment 4

[Richard Barnes [:rbarnes]]

Comment on attachment 8683399 [details] [diff] [review]
0001-Bug-1221808-have-the-control-centre-crop-the-end-for.patch

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

Upgrading f+ to r+

Comment 5

[Mark Hammond [:markh] [:mhammond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31b925e8c417 is green.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/83cb1c2445fa

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83cb1c2445fa

Comment 8

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 46.0a1 (2016-01-04) Win 7