1174254 - Disable FTP protocol guessing

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jeremy Orem [:oremj]]

If the hostname begins with "ftp.", "ftp://" will be inserted in to the URI. Let's remove that behavior, so we don't create hangs on domains which aren't listening on port 21, but begin with ftp.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> so we don't create hangs on domains which aren't listening on port 21, but begin with
> ftp.

How common are such domains compared to ones that do listen on port 21?

Comment 2

[Jeremy Orem [:oremj]]

Attachment: bug-1174254.patch

I don't have access to the try, so this is untested.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 8621755 [details] [diff] [review]
bug-1174254.patch

i would like this, but I think we should just rm -rf ftp://

Comment 4

[Patrick McManus [:mcmanus]]

doug, let's set aside how much ftp is used (we can figure that out) and just assume that it is "once in a while".

What's your vision for what happens when ftp:// is clicked on? A front-end driven helper pointing at add-ons? How about a subresource? What about other products? I'm concerned we trade one piece of kinda-scary but in practice ok code for a matrix of usability problems on different products.

Or are we only talking about doing it if ftp is super rare? We can gather that data, but my intuition says its much more "uncommon" than "doesn't really impact people.

Comment 5

[Patrick McManus [:mcmanus]]

remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=012443c0b59f

Comment 6

[Patrick McManus [:mcmanus]]

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


409 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_canonizeURL.js | entering 'ftp.example.bar' loads expected URL - Got http://ftp.example.bar/, expected ftp://ftp.example.bar/
527 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_plainTextLinks.js | ftp.example.com should be preceeded with ftp:// - Got http://ftp.example.com/, expected ftp://ftp.example.com/
665 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlHighlight.js | Correct part of the urlbar contents is highlighted - Got <ftp.>mozilla.org, expected <http://ftp.>mozilla.org
865 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_urlbarTrimURLs.js | url bar value set - Got ftp.mozilla.org, expected http://ftp.mozilla.org 

jeremy can you get access to try and work out the tests that need to be updated?

I'll accept the patch concept.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8621755 [details] [diff] [review]
bug-1174254.patch

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

tests need updating

Comment 8

[Jesse Ruderman]

I think we should make this change:

* HTTP can show nicer UI.
* HTTP can redirect to HTTPS (and use HSTS).
* The FTP protocol on ftp.mozilla.org is being turned off soon (August 15).

Doug, please file a new bug for removing FTP support. That needs broader discussion and data.

Comment 9

[Patrick McManus [:mcmanus]]

Jeremy, if you fixup comment 6 we can land this

Comment 10

[Jeremy Orem [:oremj]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> 409 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_canonizeURL.js | entering
> 'ftp.example.bar' loads expected URL - Got http://ftp.example.bar/, expected
> ftp://ftp.example.bar/
> 527 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_plainTextLinks.js |
> ftp.example.com should be preceeded with ftp:// - Got
> http://ftp.example.com/, expected ftp://ftp.example.com/
> 665 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_urlHighlight.js | Correct part of
> the urlbar contents is highlighted - Got <ftp.>mozilla.org, expected
> <http://ftp.>mozilla.org
> 865 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_urlbarTrimURLs.js | url bar value
> set - Got ftp.mozilla.org, expected http://ftp.mozilla.org 

The first two failures make sense. Any idea why this would affect highlighting and trimming?

Comment 11

[Patrick McManus [:mcmanus]]

no.. you probably need to find someone on the frontend team for the insight

Comment 12

[Jeremy Orem [:oremj]]

Attachment: bug-1174254.patch

This should fix up the broken tests. I looked in to this further. They were breaking, because trimURL trims off http://, unless the "fixed up" URL is different from the given URL. Before this patch http://ftp.mozilla.org, would test "http://ftp.mozilla.org" == "ftp://ftp.mozilla.org" and fail, so it would not strip http://.

Relevant code in browser/base/content/utilityOverlay.js:trimURL

>  try {
>    fixedUpURL = Services.uriFixup.createFixupURI(urlWithoutProtocol, flags);
>    expectedURLSpec = makeURI(aURL).spec;
>  } catch (ex) {
>    return url;
>  }
>  if (fixedUpURL.spec == expectedURLSpec) {
>    return urlWithoutProtocol;
>  }
>  return url;

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8746874 [details] [diff] [review]
bug-1174254.patch

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

I've pushed this to try for you - please don't checkin unless that passes.

Comment 14

[Patrick McManus [:mcmanus]]

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

Comment 15

[Jeremy Orem [:oremj]]

Looks like the tests passed, what's the next step?

Comment 16

[Patrick McManus [:mcmanus]]

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 17

[Josh Matthews [:jdm]]

Specifically, see the use of the checkin-needed keyword described in "Committing the patch".

Comment 18

[Pulsebot]

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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Andrew Overholt [:overholt]]

Is this worth relnoting?

Comment 21

[Marcia Knous [:marcia]]

(In reply to Andrew Overholt [:overholt] from comment #20)
> Is this worth relnoting?

Sure, please nominated for relnote and craft some verbiage and we can include it in the changed section.

Comment 22

[Andrew Overholt [:overholt]]

(Sorry I'm late here. On second thought, it's probably not worth rel-noting this.)