Closed Bug 682762 Opened 9 years ago Closed 9 years ago

ftp URI with "param" component does no longer work

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9
Tracking Status
firefox6 --- unaffected
firefox7 --- unaffected
firefox8 --- unaffected
firefox9 --- affected

People

(Reporter: emk, Assigned: julian.reschke)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
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).
Keywords: regression
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?
The right place seems to be nsFtpConnectionThread.cpp, nsFtpState::Init.
Attachment #556529 - Flags: review?(rjesup)
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?
(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?
Blocks: 665706
No longer depends on: 665706
(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)
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.
(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.
1101 has been added due to bug 391855.
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.
(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.
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
(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.
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
Attached patch patch (work in progress) (obsolete) — Splinter Review
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
Attachment #556529 - Attachment is obsolete: true
Attachment #556529 - Flags: review?(rjesup)
Attached patch proposed patch (obsolete) — Splinter Review
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
Attachment #560334 - Attachment is obsolete: true
Attachment #560445 - Flags: review?(rjesup)
(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 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.
Attachment #560445 - Flags: review?(rjesup) → review+
Attached patch proposed patchSplinter Review
Updated patch, asking for another review to double-check with respect to coding conventions.
Attachment #560445 - Attachment is obsolete: true
Attachment #561441 - Flags: review?(rjesup)
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!
Attachment #561441 - Flags: review?(rjesup) → review+
Assignee: nobody → julian.reschke
Target Milestone: --- → mozilla9
In my queue with a few other bits that are being sent to try first and then onto inbound :-)
Status: NEW → ASSIGNED
Flags: in-testsuite?
Keywords: checkin-needed
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/ee268ef7df39
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(Clearing to stop this request showing up on the 'My Requests' page)
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.