Closed Bug 1374114 Opened 7 years ago Closed 7 years ago

Add a pref to disable ftp://

Categories

(Core Graveyard :: Networking: FTP, defect, P3)

Product:
Core Graveyard
Component:
Networking: FTP
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jan, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, Whiteboard: [necko-backlog])

Attachments

(1 file)

Bug 1374114 - Add a pref to disable ftp.
59 bytes, text/x-review-board-request
valentin
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170618100241

Steps to reproduce:

Please consider adding a pref to disable support for ftp://
Blocks: https-everything
See Also: → ftps
Whiteboard: [necko-backlog]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
See Also: → 1404744
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: nightly-community
Version: 56 Branch → Trunk
Assignee: nobody → jkt
When the pref is set should we be failing tests like the following:
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/eventsource/eventsource-constructor-non-same-origin.htm

Try had a fair few failures here but I'm not sure if we should just be ignoring them in Nightly now or if the return code from the new uri could be improved.
Comment on attachment 8931970 [details]
Bug 1374114 - Add a pref to disable ftp.

https://reviewboard.mozilla.org/r/203022/#review208488

::: modules/libpref/init/all.js:1813
(Diff revision 1)
>  // Section 4.8 "High-Throughput Data Service Class", and 80 (0x50, or AF22)
>  // per Section 4.7 "Low-Latency Data Service Class".
>  pref("network.ftp.data.qos", 0);
>  pref("network.ftp.control.qos", 0);
> +#ifdef EARLY_BETA_OR_EARLIER
> +pref("network.ftp.enabled", false);

we cannot disable ftp without understanding its impact to the user base. I'm not sure pre-release makes that beter.

I'll take the patch with ftp enabled on all channels.

::: netwerk/protocol/ftp/nsFtpProtocolHandler.cpp:96
(Diff revision 1)
>              mIdleTimeout = 5*60; // 5 minute default
>  
>          rv = branch->AddObserver(IDLE_TIMEOUT_PREF, this, true);
>          if (NS_FAILED(rv)) return rv;
>  
> +  rv = branch->GetBoolPref(ENABLED_PREF, &mEnabled);

spacing nit?
Attachment #8931970 - Flags: review?(mcmanus) → review-
> I'll take the patch with ftp enabled on all channels.

I assume you meant disabled :)

I will do a follow up patch for telemetry in a different bug. I will find out based on our other goals and timelines what that should look like etc.

Thanks!
Comment on attachment 8931970 [details]
Bug 1374114 - Add a pref to disable ftp.

https://reviewboard.mozilla.org/r/203022/#review209954

I'll take this patch with
a] a test case showing the difference between pref settings.. it looks like you don't have to asyncopen() the channel to see the effect, so this should be easy
b] a green try run
Attachment #8931970 - Flags: review?(mcmanus) → review-
Doing this in the protocol handler means the navigation will go to the search engine.
Doing it in the FTP channel would take the user to an error page.
It's not clear to me which one is better, but we should note that there are pros and cons with both of them.
> Doing this in the protocol handler means the navigation will go to the search engine.

I'm not worried about a privacy leak if this is what you are referring to? Is there any precedence in other code on what should happen here?

My concern was there are two channel types nsFTPChannel.cpp and FTPChannelChild.cpp which from my initial testing would require two places in the code to carry this check. In terms of risk I thought this was much less.

Despite the leaking of a URL to the search provider, this already happens in a few other cases too.

Another advantage is that we get to a post protocol implementation as if it was never supported.

> but we should note that there are pros and cons with both of them.

Please let me know if I missed anything obvious here.
Flags: needinfo?(valentin.gosu)
(In reply to Jonathan Kingston [:jkt] from comment #11)
> > Doing this in the protocol handler means the navigation will go to the search engine.
> 
> I'm not worried about a privacy leak if this is what you are referring to?
> Is there any precedence in other code on what should happen here?

None that I am aware of. I was just thinking that I'd be very surprised if one day ftp: URLs simply did something else.

> Another advantage is that we get to a post protocol implementation as if it
> was never supported.

That's fair enough. I guess by the time we actually want to flip this pref we will know for sure what the intended behaviour should be. Maybe a JS implementation that just shows an error message. Or maybe something else.

Let me know if you need any help with the steps in comment 9.
Flags: needinfo?(valentin.gosu)
If ftp protocol is disabled, ftp:// links should (try to) open external apps (maybe with warnings) as all other unsupported protocols do.
Is the test I added sufficient or did you have other ideas?
Flags: needinfo?(mcmanus)
(In reply to Jonathan Kingston [:jkt] from comment #15)
> Is the test I added sufficient or did you have other ideas?

its a little weird to bundle that with search suggestions but I think its acceptable for the need.. I won't stand on principle here.
Flags: needinfo?(mcmanus)
Comment on attachment 8931970 [details]
Bug 1374114 - Add a pref to disable ftp.

https://reviewboard.mozilla.org/r/203022/#review221238

That's great! Thanks!

::: toolkit/components/places/tests/unifiedcomplete/test_search_suggestions.js:877
(Diff revision 4)
>        },
>      ],
>    });
>  
> +  // Check FTP enabled
> +  const initialFtpPref = Services.prefs.getBoolPref("network.ftp.enabled");

Instead of setting it back to the initial value it's better to registerCleanupFunction( clearUserPref(...) )

::: toolkit/components/places/tests/unifiedcomplete/test_search_suggestions.js:958
(Diff revision 4)
> +    matches: [
> +      makeSearchMatch("ftp://test", { engineName: ENGINE_NAME, heuristic: true }),
> +    ],
> +  });
> +  // Restore FTP pref
> +  Services.prefs.setBoolPref("network.ftp.enabled", initialFtpPref);

See previous comment
Attachment #8931970 - Flags: review?(valentin.gosu) → review+
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ba748f76495
Add a pref to disable ftp. r=valentin
https://hg.mozilla.org/mozilla-central/rev/4ba748f76495
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
thanks!
Thank you! Awesome! Next ones are ws:// and http://. ;-D

I noticed a bug:
1. Open ftp://ftp.de.debian.org/
2. set network.ftp.enabled;false + keyword.enabled;false
3. Click on a folder. Nothing happens. (As this component is now disabled I think it's okay to be broken like this.)
4. Open another tab of ftp://ftp.de.debian.org/ - you will be redireced to Google
5. Open a tab of ftplol://ftp.de.debian.org/ - I would expect this page also for step 4 because of keyword.enabled;false.
I don't understand your comment :darkspirit? I get exactly the same output for both 4 and 5 minus the "lol" section obviously.
I think the point is that if you set keyword.enabled;false then you get different results for ftp:// and ftplol:// but we should technically treat them the same (unknown protocols)
I don't think it is a big problem, but we might want to file a follow-up bug for when we disable ftp support :)
Or when I hover ftp://ftp.de.debian.org/ in comment 23, the url is not shown on the bottom-left. If I click on it, nothing happens.
If I edit it via devtools to <a href="ftplol:// I would see full url on the bottom-left when hovering it and would get the unknown protocol page.
You didn't have to create a new pref. Just add pref("network.protocol-handler.external.ftp", true);
> You didn't have to create a new pref. Just add pref("network.protocol-handler.external.ftp", true);

Isn't this for external apps opening the browser? Doesn't work regardless.

> I think the point is that if you set keyword.enabled;false then you get different results for ftp:// and ftplol:// but we should technically treat them the same (unknown protocols)

Yeah ok sorry I missed that bit.

> but we might want to file a follow-up bug for when we disable ftp support

Noted, yeah we are not moving to do that immediately so we can worry about that then I think.

Thanks
(In reply to Jonathan Kingston [:jkt] from comment #28)
> > You didn't have to create a new pref. Just add pref("network.protocol-handler.external.ftp", true);
> 
> Isn't this for external apps opening the browser?

Quite opposite.

> Doesn't work regardless.

What does "Doesn't work" mean? Of course ftp protocol will not work in the browser.
> What does "Doesn't work" mean? Of course ftp protocol will not work in the browser.

Has no effect for me as I realise now it's for loading external applications. We are trying to protect the internal viewing of ftp in the browser here. Perhaps setting it true and the handler to /dev/null would protect the browser all the same.
(In reply to Jonathan Kingston [:jkt] from comment #31)
> > What does "Doesn't work" mean? Of course ftp protocol will not work in the browser.
> 
> Has no effect for me as I realise now it's for loading external
> applications. We are trying to protect the internal viewing of ftp in the
> browser here. Perhaps setting it true and the handler to /dev/null would
> protect the browser all the same.

Did you restart the browser? This pref is not live and I don't think this pref have to be live. I can't view ftp in the browser with this pref flipped. Could you tell me a step-by-step STR and AR/ER?
(In reply to Masatoshi Kimura [:emk] from comment #33)
> Did you restart the browser? This pref is not live and I don't think this pref have to be live.

Oops. Everything tested after a restart:

> network.ftp.enabled;true/false + network.protocol-handler.external.ftp;true
Correct: Link preview + clickable when hovering/clicking on ftp://ftp.de.debian.org/
I selected FileZilla: Works as intended and connects to that host.
Correct: Copy ftp://ftp.de.debian.org/ into the address bar: You'll be asked if you want to open an external app.

> network.ftp.enabled;false + keyword.enabled;true (default)
Bug: No link preview, dead link when hovering/clicking on ftp://ftp.de.debian.org/, but should lead to unknown protocol page  (Compare with <a href="ftplol://test.example">).
Correct: Copy ftp://ftp.de.debian.org/ into the address bar: It will be googled

> network.ftp.enabled;false + keyword.enabled;false
Bug: No link preview, dead link when hovering/clicking on ftp://ftp.de.debian.org/, but should lead to unknown protocol page.
Bug: Copy ftp://ftp.de.debian.org/ into the address bar: should be unknown protocol but it will be googled (Compare with ftplol://test.example)
Tested in Linux Firefox 58.0:
1. Set network.protocol-handler.external.ftp-false
2. Restart browser
3. Type ftp://ftp.de.debian.org/ in tab and press enter
(In reply to Jonathan Kingston [:jkt] from comment #35)
> Tested in Linux Firefox 58.0:
> 1. Set network.protocol-handler.external.ftp-false
> 2. Restart browser
> 3. Type ftp://ftp.de.debian.org/ in tab and press enter

network.protocol-handler.external.ftp-false;true or network.protocol-handler.external.ftp;false (default:unset) do not change anything for me (as expected). (Nightly 60 x64 20180126104626 de_DE @ Debian Testing (KDE))
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: