The default bug view has changed. See this FAQ.

ftp URI with "param" component does no longer work

RESOLVED FIXED in mozilla9

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: emk, Assigned: Julian Reschke)

Tracking

({regression})

Trunk
mozilla9
regression
Points:
---

Firefox Tracking Flags

(firefox6 unaffected, firefox7 unaffected, firefox8 unaffected, firefox9 affected)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
status-firefox6: --- → unaffected
status-firefox7: --- → unaffected
status-firefox8: --- → unaffected
status-firefox9: --- → affected
(Assignee)

Comment 1

6 years ago
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).
(Reporter)

Updated

6 years ago
Keywords: regression
(Assignee)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
The right place seems to be nsFtpConnectionThread.cpp, nsFtpState::Init.
(Assignee)

Comment 4

6 years ago
Created attachment 556529 [details] [diff] [review]
bugfix for path parameter not being ignored anymore
Attachment #556529 - Flags: review?(rjesup)
(Reporter)

Comment 5

6 years ago
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?
(Assignee)

Comment 6

6 years ago
(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
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
(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.
(Reporter)

Comment 10

6 years ago
1101 has been added due to bug 391855.
(Reporter)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
(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.
(Reporter)

Comment 13

6 years ago
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.
(Assignee)

Comment 15

6 years ago
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
(Assignee)

Comment 16

6 years ago
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
Attachment #556529 - Attachment is obsolete: true
Attachment #556529 - Flags: review?(rjesup)
(Assignee)

Comment 17

6 years ago
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
Attachment #560334 - Attachment is obsolete: true
Attachment #560445 - Flags: review?(rjesup)
(Assignee)

Comment 18

6 years ago
(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+
(Assignee)

Comment 20

6 years ago
Created attachment 561441 [details] [diff] [review]
proposed patch

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+
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → julian.reschke
(Assignee)

Updated

6 years ago
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://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee268ef7df39
https://hg.mozilla.org/mozilla-central/rev/ee268ef7df39
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.