682762 - ftp URI with "param" component does no longer work

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Masatoshi Kimura [:emk]]

User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110828 Firefox/9.0a1

Steps to reproduce:
1. Click the URL.

Actual result:
550 Failed to change directory.

Expected result:
Download ls-lR.gz.

Reproducible: Always

IE8 and Chrome works as expected.
Per RFC 1738, ftp URIs can have a ";type=<typecode>" parameter which is not a part of the filename.

No other RFCs do not obsolete the ftp URI scheme part of RFC 1738.
RFC 4248: telnet
RFC 4266: gopher
RFC 3986: Generic syntax
RFC 6068: mailto
RFC 6196: mailserver
RFC 6270: tn3270

Comment 1

[Julian Reschke]

That's indeed a regression caused by 665706.

The FTP specific code will need to deal with the parameter on it's own (remove it from the path, and potentially set modes accordingly).

Comment 2

[Julian Reschke]

The FTP code never called GetParam(), so it always ignored the trailing ";param".

It appears a simple fix would be to truncate the file path after a semicolon, for instance in the FTPChannelChild constructor. Will try this later.

That being said: is it ok that the parameter is always ignored? Shouldn't it set the subsequent transfer mode?

Comment 3

[Julian Reschke]

The right place seems to be nsFtpConnectionThread.cpp, nsFtpState::Init.

Comment 4

[Julian Reschke]

Attachment: bugfix for path parameter not being ignored anymore

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 556529 [details] [diff] [review]
bugfix for path parameter not being ignored anymore

There are a few more places at which nsIURL::GetFilePath is called.
https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#1101
https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#1252
What about creating a method something like nsFtpState::GetFilePathWithoutParams?

Comment 6

[Julian Reschke]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> Comment on attachment 556529 [details] [diff] [review]
> bugfix for path parameter not being ignored anymore
> 
> There are a few more places at which nsIURL::GetFilePath is called.
> https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/
> nsFtpConnectionThread.cpp#1101
> https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/
> nsFtpConnectionThread.cpp#1252
> What about creating a method something like
> nsFtpState::GetFilePathWithoutParams?

Not sure it's needed, but probably would be good for consistency.

Do you want to take over?

Comment 7

[Julian Reschke]

(In reply to Julian Reschke from comment #6)
> (In reply to Masatoshi Kimura [:emk] from comment #5)
> > Comment on attachment 556529 [details] [diff] [review]
> > bugfix for path parameter not being ignored anymore
> > 
> > There are a few more places at which nsIURL::GetFilePath is called.
> > https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/
> > nsFtpConnectionThread.cpp#1101
> > https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/
> > nsFtpConnectionThread.cpp#1252
> > What about creating a method something like
> > nsFtpState::GetFilePathWithoutParams?
> 
> Not sure it's needed, but probably would be good for consistency.
> 
> Do you want to take over?

https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#1101

-> I'm not sure how this works, and the presence of the parameter doesn't seem to affect directory listings, so maybe we don't need to touch this

https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#1252

-> This appears to be code that isn't used (as far as I can tell, Firefox doesn't support FTP write operations)

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

1101:  In any case, the parameter goes with that path element, so adding '/' at the end is ok.

1252:  Any params that are in the URI should be passed along to the server, so I see nothing to do here.

Comment 9

[Julian Reschke]

(In reply to Randell Jesup [:jesup] from comment #8)
> 1101:  In any case, the parameter goes with that path element, so adding '/'
> at the end is ok.

It would be cool to understand when that code gets executed. I couldn't see a case where having a param but not a slash lead to a problem.

> 1252:  Any params that are in the URI should be passed along to the server,
> so I see nothing to do here.

Not true for FTP, I believe. As far as I recall, the type parameter defines the transfer mode, and should *modify* the FTP command sent to the server. That it was ignored before could be considered a (harmless) bug in itself.

Comment 10

[Masatoshi Kimura [:emk]]

1101 has been added due to bug 391855.

Comment 11

[Masatoshi Kimura [:emk]]

When I type <ftp://ftp.mozilla.org/pub/;type=d> into the location bar,
Firefox 8 and earlier: redirect to <ftp://ftp.mozilla.org/pub/;type=d>.
Firefox 9: 550 Failed to change directory.
Firefox 9 with patch #556529: redirect to <ftp://ftp.mozilla.org/pub;type=d/>, then I couldn't download any files. For example, when I click ls-lR.gz, directory listing is displayed and the location bar displays <ftp://ftp.mozilla.org/pub;type=d/ls-lR.gz/>.
Conclusion: At least 1101 also needs to be patched. In the first place, I don't understand what's the point of leaving URL handling inconsistent.

Comment 12

[Julian Reschke]

(In reply to Masatoshi Kimura [:emk] from comment #11)
> When I type <ftp://ftp.mozilla.org/pub/;type=d> into the location bar,
> Firefox 8 and earlier: redirect to <ftp://ftp.mozilla.org/pub/;type=d>.
> Firefox 9: 550 Failed to change directory.
> Firefox 9 with patch #556529: redirect to
> <ftp://ftp.mozilla.org/pub;type=d/>, then I couldn't download any files. For
> example, when I click ls-lR.gz, directory listing is displayed and the
> location bar displays <ftp://ftp.mozilla.org/pub;type=d/ls-lR.gz/>.

Thanks; I will check that.

> Conclusion: At least 1101 also needs to be patched. In the first place, I
> don't understand what's the point of leaving URL handling inconsistent.

My main concern is about touching code I can't test. If it's dead code, we should remove it. If it's not dead, we need to find out how to test it.

Comment 13

[Masatoshi Kimura [:emk]]

Also, when I try to download <ftp://ftp.mozilla.org/pub/ls-lR.gz;type=i>, Filename "ls-lR.gz;type=i" and extension ".gz;type=i" is suggested.
Probably GetFilenameAndExtensionFromChannel also needs to be patched.
https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#271

Comment 14

[Michal Novotny [:michal]]

(In reply to Randell Jesup [:jesup] from comment #8)
> 1101:  In any case, the parameter goes with that path element, so adding '/'
> at the end is ok.

Yes, we should add '/' here. But if the filePath contains ';' the slash must be inserted before it.


(In reply to Julian Reschke from comment #12)
> > Conclusion: At least 1101 also needs to be patched. In the first place, I
> > don't understand what's the point of leaving URL handling inconsistent.
> 
> My main concern is about touching code I can't test. If it's dead code, we
> should remove it. If it's not dead, we need to find out how to test it.

Seamonkey can upload file via FTP, so you can test it with comm-central tree.

Comment 15

[Julian Reschke]

Just summarizing how we got there in case somebody wants to help...:

1) with 665706, the generic URI handling code doesn't treat "parameters" as something special anymore; they are just treated as part of the path. This is right, because in the generic URI syntax (RFC 3986) they are not special.

2) in FTP URIs they are special; what comes after a semicolon doesn't go over the wire but affects the transfer mode (text vs binary).

3) previously, when getting the path or filePath, the FTP code simply did not see the parameter.

4) now it does, which affects various cases where the code changes the path, such as when trying to append a slash

Comment 16

[Julian Reschke]

Attachment: patch (work in progress)

This one addresses the navigation as well; remaining problems:

- navigation shouldn't lose the param (but this is the case in FF6 as well, so no regression) - this probably needs to be fixed in the code that generates the HTML view (where?)

- download still inspects the full URI including the param, and thus doesn't find the actual file extension

- the write access case isn't tested nor addressed

Comment 17

[Julian Reschke]

Attachment: proposed patch

This patch drops the param early (in the Init method). The observable difference from FF6 should be only that it disappears earlier from the address bar (it didn't have any effect before).

Note: not tested yet with authoring in SeaMonkey

Comment 18

[Julian Reschke]

(In reply to Julian Reschke from comment #17)
> Created attachment 560445 [details] [diff] [review]
> proposed patch
> 
> This patch drops the param early (in the Init method). The observable
> difference from FF6 should be only that it disappears earlier from the
> address bar (it didn't have any effect before).
> 
> Note: not tested yet with authoring in SeaMonkey

Now tested: as far as I can tell, this will fix SeaMonkey issues as well, because, at the time an upload can be done, the FTP URI has been fixed (== the param removed).

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 560445 [details] [diff] [review]
proposed patch

+void
+nsFtpState::RemoveParamsFromPath(nsCString& path)
+{
+  PRInt32 index = path.Find(";");
+  if (index >= 0) {
+    path.SetLength(index);
+  }
+}

Nothing here references the nsFtpState object.  This should be a static void helper function at the start of the file, after the includes.  That also means you don't need to modify nsFtpConnectionThread.h.

Also, please use "path.FindChar(';')" instead of "path.Find(";").

With those nits fixed, r=jesup

You probably want to post a revised patch here and set "checkin"; you could "carry forward" the review status or I could give a 5-second review.

Comment 20

[Julian Reschke]

Attachment: proposed patch

Updated patch, asking for another review to double-check with respect to coding conventions.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 561441 [details] [diff] [review]
proposed patch

r=me
Next step is to mark with the 'checkin' tag asking someone to check it in for you (I may well do it).   Thanks!

Comment 22

[Ed Morley [:emorley]]

In my queue with a few other bits that are being sent to try first and then onto inbound :-)

Comment 23

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99

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

Comment 24

[Ed Morley [:emorley]]

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

Comment 25

[Ed Morley [:emorley]]

(Clearing to stop this request showing up on the 'My Requests' page)