Allow FTP requests by Chromium extensions

Allow FTP requests by Chromium extensions

When an extension specifies a ftp://*/* or <all_urls> permission, then
the extension should be allowed to fetch data from a FTP site.

Contributed by Rob Wu <rob@robwu.nl>;

BUG=75248
TEST=ExtensionApiTest.CrossOriginXHRBackgroundPage:ExtensionApiTest.CrossOriginXHRContentScript

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256810

[robwu, 2014-02-16 17:34:30]

Please review!

2 lines for the actual feature, a 84 lines for the unit tests...

[not at google - send to devlin, 2014-02-18 19:24:03]

Giving FTP support to extensions sounds fine, I think?

However I have this vague recollection of FTP support being deprecated in Chrome
at some point. Will could you help put me in touch with the right people to chat
to about this?

[willchan no longer on Chromium, 2014-02-18 20:10:40]

News to me. Maybe Chris knows about it.
On Feb 18, 2014 11:24 AM, <kalman@chromium.org> wrote:

Giving FTP support to extensions sounds fine, I think?

However I have this vague recollection of FTP support being deprecated in
Chrome
at some point. Will could you help put me in touch with the right people to
chat
to about this?

https://codereview.chromium.org/166793003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-02-18 20:11:29]

News to me. Maybe Chris knows about it.
On Feb 18, 2014 11:24 AM, <kalman@chromium.org> wrote:

Giving FTP support to extensions sounds fine, I think?

However I have this vague recollection of FTP support being deprecated in
Chrome
at some point. Will could you help put me in touch with the right people to
chat
to about this?

https://codereview.chromium.org/166793003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cbentzel, 2014-02-18 20:13:33]

+ellyjones,davidben

They are looking at a summer intern project for an FTP client app
(with the raw FTP support moving out to JS on top of the sockets API).
I think this will also need to allow the app/extension handle
subresource FTP loads but I'm not sure how to make that work. The
longer term hope is that we could get rid of FTP from Chrome itself,
as a pretty small percentage of users access it over a 1 week period.

On Tue, Feb 18, 2014 at 3:09 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> News to me. Maybe Chris knows about it.
>
> On Feb 18, 2014 11:24 AM, <kalman@chromium.org> wrote:
>
> Giving FTP support to extensions sounds fine, I think?
>
> However I have this vague recollection of FTP support being deprecated in
> Chrome
> at some point. Will could you help put me in touch with the right people to
> chat
> to about this?
>
> https://codereview.chromium.org/166793003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robwu, 2014-02-18 21:30:52]

On 2014/02/18 20:13:33, cbentzel wrote:
> +ellyjones,davidben
> 
> They are looking at a summer intern project for an FTP client app

Indeed, see http://crbug.com/333943.

> (with the raw FTP support moving out to JS on top of the sockets API).

The sockets API is only available to Chrome Apps, not extensions.

> I think this will also need to allow the app/extension handle
> subresource FTP loads but I'm not sure how to make that work. The
> longer term hope is that we could get rid of FTP from Chrome itself,
> as a pretty small percentage of users access it over a 1 week period.

I can't find any public discussion on deprecating FTP from Chromium, but if
you're really
going to remove it, I hope that you provide a good solution for handling
content.
FTP does not only serve directory listings or "downloadable" content, but also
files.
If the future solution lacks support for delegating the content stream to
another
extension or app (chosen by the user), then this future solution is useless.
I expect that the implementation and testing of this new feature is non-trivial,
so even
when the consensus of removing FTP support has reached, it will probably take
many more
months before a suitable alternative exists.

[ motivation for CL ]
During research, I occasionally encounter PDFs from universities on ftp sites,
or software documentation (e.g. for LaTeX packages) on FTP sites.
I have submitted this CL because I want to view these PDF files in the browser
with the PDF.js Chrome extension.

[ argument for keeping FTP support ]
Most modern content is only available on HTTP, so I'm not surprised that only
0.1 - 0.2% of the users end up at a FTP site. Regardless, 0.1-0.2% is far from
insignificant: According to Google I/O 2013, Chrome has 750M monthly users.
That means that there are approximately 75-150M users who visit FTP sites in
a given week (not bad for a protocol that was declared dead a long time ago).

[not at google - send to devlin, 2014-02-18 21:38:27]

On 2014/02/18 21:30:52, robwu wrote:
> On 2014/02/18 20:13:33, cbentzel wrote:
> > +ellyjones,davidben
> > 
> > They are looking at a summer intern project for an FTP client app
> 
> Indeed, see http://crbug.com/333943.
> 
> > (with the raw FTP support moving out to JS on top of the sockets API).
> 
> The sockets API is only available to Chrome Apps, not extensions.
> 
> > I think this will also need to allow the app/extension handle
> > subresource FTP loads but I'm not sure how to make that work. The
> > longer term hope is that we could get rid of FTP from Chrome itself,
> > as a pretty small percentage of users access it over a 1 week period.
> 
> I can't find any public discussion on deprecating FTP from Chromium, but if
> you're really
> going to remove it, I hope that you provide a good solution for handling
> content.
> FTP does not only serve directory listings or "downloadable" content, but also
> files.
> If the future solution lacks support for delegating the content stream to
> another
> extension or app (chosen by the user), then this future solution is useless.
> I expect that the implementation and testing of this new feature is
non-trivial,
> so even
> when the consensus of removing FTP support has reached, it will probably take
> many more
> months before a suitable alternative exists.
> 
> [ motivation for CL ]
> During research, I occasionally encounter PDFs from universities on ftp sites,
> or software documentation (e.g. for LaTeX packages) on FTP sites.
> I have submitted this CL because I want to view these PDF files in the browser
> with the PDF.js Chrome extension.
> 
> [ argument for keeping FTP support ]
> Most modern content is only available on HTTP, so I'm not surprised that only
> 0.1 - 0.2% of the users end up at a FTP site. Regardless, 0.1-0.2% is far from
> insignificant: According to Google I/O 2013, Chrome has 750M monthly users.
> That means that there are approximately 75-150M users who visit FTP sites in
> a given week (not bad for a protocol that was declared dead a long time ago).

0.1% is a thousandth, so we're talking more like 1M; but that's still roughly
the population of San Francisco.

As for the code: I'm surprised you only need to change those 2 lines to add FTP
support. I guess we already support parsing FTP at the extension level.

[not at google - send to devlin, 2014-02-19 21:56:15]

+meacer, WDYT?

It sounds like we're ok to go forward on the network front?

However there is still a security decision. I don't know what the effect of
allowing cross-origin FTP would be, but it's because I'm not that familiar with
FTP. A lot of work has obviously gone into hardening cross-origin HTTP but I
don't know about FTP. Same could go for HTTP vs FTP servers, and allowing
extensions that capability... hm.

[robwu, 2014-02-19 22:20:18]

On 2014/02/19 21:56:15, kalman wrote:
> +meacer, WDYT?
> 
> It sounds like we're ok to go forward on the network front?
> 
> However there is still a security decision. I don't know what the effect of
> allowing cross-origin FTP would be, but it's because I'm not that familiar
with
> FTP. A lot of work has obviously gone into hardening cross-origin HTTP but I
> don't know about FTP. Same could go for HTTP vs FTP servers, and allowing
> extensions that capability... hm.

FYI, it is technically already possible to make FTP requests from extensions.
1. Declare a content script for a FTP site.
2. Load that FTP site in a frame.
3. Perform XHR in a content script on that FTP site and pass the result back to
the parent frame using window.postMessage.

This hacky method adds a significant latency (a few seconds to initialize the
trampoline at step 2), so I'd rather have a direct way to send XHR to websites
within an extension.

[cbentzel, 2014-02-19 23:35:19]

OK to go forward on the network front.

On Wed, Feb 19, 2014 at 4:56 PM,  <kalman@chromium.org> wrote:
> +meacer, WDYT?
>
> It sounds like we're ok to go forward on the network front?
>
> However there is still a security decision. I don't know what the effect of
> allowing cross-origin FTP would be, but it's because I'm not that familiar
> with
> FTP. A lot of work has obviously gone into hardening cross-origin HTTP but I
> don't know about FTP. Same could go for HTTP vs FTP servers, and allowing
> extensions that capability... hm.
>
> https://codereview.chromium.org/166793003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[not at google - send to devlin, 2014-02-21 17:39:44]

some small comments, but ping @ meacer re security questions.

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js
(right):

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:23:
if (/^https?:/i.test(url)) chrome.test.assertEq(200, req.status);
what response is ftp expected to give?

if you expose isFtp as a function rather than a local variable in rewriteURL
then you could use if (!isFtp(url)) ...;

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:46:
// FTP request fails. This is handled by req.onerror.
can you assert properties about the error

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/content_script.js
(right):

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/content_script.js:30:
// FTP request fails. This is handled by req.onerror.
ditto

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/test.js
(right):

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/test.js:22:
if (/^https?:/i.test(url)) chrome.test.assertEq(200, response.status);
ditto throughout file

[robwu, 2014-02-21 19:05:23]

On 2014/02/21 17:39:44, kalman wrote:
> some small comments, but ping @ meacer re security questions.
> 
>
https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
> File
> chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js
> (right):
> 
>
https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
>
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:23:
> if (/^https?:/i.test(url)) chrome.test.assertEq(200, req.status);
> what response is ftp expected to give?

It happens to be zero (just like file:), but I'd say that it is undefined
behavior, because FTP does not support HTTP status codes.

> 
> if you expose isFtp as a function rather than a local variable in rewriteURL
> then you could use if (!isFtp(url)) ...;

I had deliberately tested against the http(s) scheme to avoid having to add
negations for future schemes. I guess that if a new protocol is added, it is
more likely a non-http protocol than a http protocol.

> 
>
https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
>
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:46:
> // FTP request fails. This is handled by req.onerror.
> can you assert properties about the error

It is currently a DOMException with the following properties:
  code: 19 ( = DOMException.NETWORK_ERR)
  message: "A network error occurred."
  name: "NetworkError"
I did not check the properties because:

1. It is too much of an implementation detail.
2. The onerror is always triggered when an exception is thrown, which handles
the error.

I have updated the test though, to check whether the "onerror" event is really
triggered when an exception is thrown.

[not at google - send to devlin, 2014-02-21 19:49:22]

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js
(right):

https://codereview.chromium.org/166793003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:23:
if (/^https?:/i.test(url)) chrome.test.assertEq(200, req.status);
On 2014/02/21 17:39:44, kalman wrote:
> what response is ftp expected to give?
> 
> if you expose isFtp as a function rather than a local variable in rewriteURL
> then you could use if (!isFtp(url)) ...;

Fair point, and this makes me wonder; do we have tests for the file: scheme
anywhere?

https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js
(right):

https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:50:
setTimeout(function() {
why does this need to be in a setTimeout?

I actually just meant that only FTP requests should have errors here, right? So
you could assert that only an FTP request should throw this.

https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/test.js
(right):

https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/test.js:22:
}
no logging if possible

[robwu, 2014-02-21 21:04:09]

On 2014/02/21 19:49:22, kalman wrote:
> Fair point, and this makes me wonder; do we have tests for the file: scheme
> anywhere?

Yes,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/e...

> 
>
https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
> File
> chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js
> (right):
> 
>
https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:50:
> setTimeout(function() {
> why does this need to be in a setTimeout?

In case onerror becomes asynchronous. Since that will probably not happen, I
have removed it.

> 
> I actually just meant that only FTP requests should have errors here, right?
So
> you could assert that only an FTP request should throw this.

Added check.

> 
>
https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
> File
> chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/test.js
> (right):
> 
>
https://codereview.chromium.org/166793003/diff/200001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/test.js:22:
> }
> no logging if possible

Changed to chrome.test.fail and return.

[robwu, 2014-03-05 23:53:52]

Ping. Still waiting for security review?

Just rebased to solve merge conflict from
https://codereview.chromium.org/169053005/.
Also noticed that the Side-by-side diff in Rietveld is broken, but the raw diff
works as expected
(https://codereview.chromium.org/download/issue166793003_380001.diff).

[not at google - send to devlin, 2014-03-05 23:55:27]

If you delete your last patchset and try re-uploading the diffs should be there.
If they don't... try again :)

[robwu, 2014-03-06 00:03:29]

On 2014/03/05 23:55:27, kalman wrote:
> If you delete your last patchset and try re-uploading the diffs should be
there.
> If they don't... try again :)

Thanks, that did solve the problem for the side-by-side diffs. (not for the
patch set though, though that might be harder to fix because the inconsistency
was caused by a failure of "git cl upload" during patch set 3)

[robwu, 2014-03-12 15:13:04]

Ben, did you actually cc meacer when you said ping @meacer for security review
(https://codereview.chromium.org/166793003/#msg11)?

Apparently the only remaining question is whether there are any security issues
with allowing extensions to make FTP requests.
Extensions can already issue FTP requests by loading a ftp page in a frame, then
use the content script to request the FTP resource (from the same origin), so I
think that the added security risk is minimal.

[not at google - send to devlin, 2014-03-12 16:20:41]

lgtm

https://codereview.chromium.org/166793003/diff/400001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js
(right):

https://codereview.chromium.org/166793003/diff/400001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:24:
if (/^https?:/i.test(url)) chrome.test.assertEq(200, req.status);
nit: please write this (and all other similar occurrences) like

if (/^https?:/i.test(url))
  chrome.test.assertEq(200, req.status);

incidentally it seems like you don't need the ignore-case in the regex.

[robwu, 2014-03-12 16:32:48]

On 2014/03/12 16:20:41, kalman wrote:
>
https://codereview.chromium.org/166793003/diff/400001/chrome/test/data/extens...
>
chrome/test/data/extensions/api_test/cross_origin_xhr/background_page/test.js:24:
> if (/^https?:/i.test(url)) chrome.test.assertEq(200, req.status);
> nit: please write this (and all other similar occurrences) like

Fixed.

> 
> if (/^https?:/i.test(url))
>   chrome.test.assertEq(200, req.status);
> 
> incidentally it seems like you don't need the ignore-case in the regex.

Schemes are supposed to be case-insensitive, so I made the check
case-insensitive even though all URLs are currently lowercased.

[robwu, 2014-03-12 23:09:55]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-03-12 23:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob@robwu.nl/166793003/440001

[commit-bot: I haz the power, 2014-03-13 03:29:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 03:29:38]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[robwu, 2014-03-13 11:31:13]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-03-13 11:31:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob@robwu.nl/166793003/440001

[commit-bot: I haz the power, 2014-03-13 13:12:21]

Change committed as 256810