Last Comment Bug 682762 - ftp URI with "param" component does no longer work
: ftp URI with "param" component does no longer work
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Julian Reschke
:
Mentors:
ftp://ftp.mozilla.org/pub/ls-lR.gz;ty...
Depends on:
Blocks: 665706
  Show dependency treegraph
 
Reported: 2011-08-28 19:49 PDT by Masatoshi Kimura [:emk]
Modified: 2012-09-14 03:07 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
unaffected
affected


Attachments
bugfix for path parameter not being ignored anymore (853 bytes, patch)
2011-08-29 06:24 PDT, Julian Reschke
no flags Details | Diff | Splinter Review
patch (work in progress) (3.48 KB, patch)
2011-09-15 05:55 PDT, Julian Reschke
no flags Details | Diff | Splinter Review
proposed patch (1.59 KB, patch)
2011-09-15 13:41 PDT, Julian Reschke
rjesup: review+
Details | Diff | Splinter Review
proposed patch (1.22 KB, patch)
2011-09-21 04:55 PDT, Julian Reschke
rjesup: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2011-08-28 19:49:41 PDT
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 2011-08-29 01:03:41 PDT
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 2011-08-29 02:31:24 PDT
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 2011-08-29 06:01:13 PDT
The right place seems to be nsFtpConnectionThread.cpp, nsFtpState::Init.
Comment 4 Julian Reschke 2011-08-29 06:24:44 PDT
Created attachment 556529 [details] [diff] [review]
bugfix for path parameter not being ignored anymore
Comment 5 Masatoshi Kimura [:emk] 2011-08-29 07:22:22 PDT
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 2011-08-29 07:32:18 PDT
(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 2011-08-30 06:14:54 PDT
(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] 2011-08-30 23:21:37 PDT
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 2011-08-30 23:34:06 PDT
(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] 2011-09-11 14:24:45 PDT
1101 has been added due to bug 391855.
Comment 11 Masatoshi Kimura [:emk] 2011-09-12 05:00:08 PDT
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 2011-09-12 05:08:31 PDT
(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] 2011-09-12 13:25:21 PDT
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) 2011-09-13 11:47:52 PDT
(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 2011-09-14 09:25:55 PDT
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 2011-09-15 05:55:24 PDT
Created attachment 560334 [details] [diff] [review]
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 2011-09-15 13:41:07 PDT
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
Comment 18 Julian Reschke 2011-09-16 06:51:55 PDT
(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] 2011-09-21 00:36:41 PDT
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 2011-09-21 04:55:56 PDT
Created attachment 561441 [details] [diff] [review]
proposed patch

Updated patch, asking for another review to double-check with respect to coding conventions.
Comment 21 Randell Jesup [:jesup] 2011-09-21 11:06:16 PDT
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] 2011-09-22 04:31:00 PDT
In my queue with a few other bits that are being sent to try first and then onto inbound :-)
Comment 24 Ed Morley [:emorley] 2011-09-22 17:33:56 PDT
https://hg.mozilla.org/mozilla-central/rev/ee268ef7df39
Comment 25 Ed Morley [:emorley] 2012-09-14 03:07:49 PDT
(Clearing to stop this request showing up on the 'My Requests' page)

Note You need to log in before you can comment on or make changes to this bug.