Closed Bug 427089 Opened 16 years ago Closed 16 years ago

FTP shouldn't send URLs in UTF-8

Categories

(Core Graveyard :: Networking: FTP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: brutforce, Assigned: michal)

References

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

"550 Failed to change directory" message box appear if I chose any folder that contain Russian characters.  

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
An example URL would be nice
Component: File Handling → Networking: FTP
Product: Firefox → Core
QA Contact: file.handling → networking.ftp
for example
ftp://ftp.asu.ru/incoming/
Russain name folders are not available in FireFox :( This problem take place  in other places too.

 
This seems to be a char escape problem.

example:
ñêèí is escaped to ftp://ftp.asu.ru/incoming/%C3%B1%C3%AA%C3%A8%C3%AD while IE escapes this to ftp://ftp.asu.ru/incoming/%F1%EA%E8%ED/
Status: UNCONFIRMED → NEW
Ever confirmed: true
The problem is somewhere else. Directory with name "ñêèí" is actually "скин". First name is in ISO-8859-1 which is incorrectly auto-detected. The correct one is in cp1251 which can be set manually. String %C3%B1%C3%AA%C3%A8%C3%AD is escaped UTF-8 that was converter from ISO-8859-1 and is obviously wrong. Correct UTF-8 escaped string is %D1%81%D0%BA%D0%B8%D0%BD. But even URL "ftp://ftp.asu.ru/incoming/%D1%81%D0%BA%D0%B8%D0%BD/" doesn't work because that FTP server doesn't understand UTF-8. The only way to follow the link that I found is to disable UTF-8 encoding of URL (network.standard-url.encode-utf8 in about:config) and to set charset to cp1251 manually. In this case the link is "ftp://ftp.asu.ru/incoming/%F1%EA%E8%ED/".

Correct solution would be probably to convert FTP path always to original charset in nsFtpState::Init() after it is unescaped. The problem is IMHO that there is no information about current charset of the path. String obtained from nsIURI::GetPath() can be encoded either in mOriginCharset (if network.standard-url.encode-utf8 == false) and then no conversion is needed or in UTF-8 and then conversion is needed. I couldn't find a way to find out if conversion should be performed.
(In reply to comment #4)
> FTP server doesn't understand UTF-8. The only way to follow the link that I
> found is to disable UTF-8 encoding of URL (network.standard-url.encode-utf8 in
> about:config) and to set charset to cp1251 manually. In this case the link is
> "ftp://ftp.asu.ru/incoming/%F1%EA%E8%ED/".

This is not correct. Selecting cp1251 manually isn't needed and disabling "network.standard-url.encode-utf8" is enough to get it working.
(In reply to comment #4)
> in UTF-8 and then conversion is needed. I couldn't find a way to find out if
> conversion should be performed.

I think that it is safe to try to do the conversion always when IsUTF8(mPath) is true. The patch implements it. It will work in most cases.

I found following problematic scenario:

1) Assume that there is a file with URL ftp://localhost/скин/file and it is encoded in cp1521.
2) User gets listing of ftp://localhost/ and charset is badly detected as ISO-8859-1 ("ñêèí" is displayed instead of "скин").
3) User switches encoding to cp1251 and "скин" is now displayed correctly.
4) User clicks on "скин" and gets listing of ftp://localhost/скин/ and charset is again set to ISO-8859-1.
5) Because "file" isn't in cyrillic, user don't switch to cp1251 and clicks on "file".
6) Now there is a problem because path can't be converted to ISO-8859-1. UTF8 path will be used instead. FTP server can't find such file so FF will think that it is a directory and tries to get listing but this fails too. The file can't be downloaded due to bug #406787 even if user will switch now to cp1251.
Assignee: nobody → michal
Status: NEW → ASSIGNED
Attachment #328496 - Flags: review?(cbiesinger)
see also bug 297395 with similar patch
Comment on attachment 328496 [details] [diff] [review]
convert path from UTF8 to originCharset

+    	        originCharset != NS_LITERAL_CSTRING("UTF-8"))

  !originCharset.EqualsLiteral("UTF-8")

+    nsAutoString ucsPath = NS_ConvertUTF8toUTF16(mPath);

  NS_ConvertUTF8toUTF16 ucsPath(mPath);

I'd also add an NS_ASSERTION that verifies IsUTF8(mPath)

+    result = p;

can you make this result.Assign(p)? that makes it clearer that this is a string class without having to scroll up

+    rv = encoder->Finish(p, &len);

So, this code is a bit confusing and I think not quite correct (you're passing in a strange length). Why not just pass in buf and set len to sizeof(buf) - 1?

+    encoder->Reset();

don't see much point in this considering that the encoder is about to be destroyed anyway
Attachment #328496 - Flags: review?(cbiesinger) → review+
OS: Windows XP → All
Hardware: PC → All
Summary: "Failed to change directory" in FTP-client → FTP shouldn't send URLs in UTF-8
Attached patch new patch (obsolete) — Splinter Review
(In reply to comment #9)
> +    rv = encoder->Finish(p, &len);
> 
> So, this code is a bit confusing and I think not quite correct (you're passing
> in a strange length). Why not just pass in buf and set len to sizeof(buf) - 1?

You are right. I took this code from http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsStandardURL.cpp#91 so it needs to be fixed too. Is it possible to fix that code within this bug too?
Attachment #328496 - Attachment is obsolete: true
Attachment #331478 - Flags: superreview?(darin.moz)
Comment on attachment 331478 [details] [diff] [review]
new patch

+    nsCOMPtr<nsICharsetConverterManager> charsetMgr (

should remove the space before the (

darin isn't really doing reviews anymore, so sr=biesi

feel free to fix standardurl in this bug too
Attachment #331478 - Flags: superreview?(darin.moz) → superreview+
Attachment #331478 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/index.cgi/mozilla-central/rev/0c0e0cdd761a
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
When there will be a updating solving this problem?
Firefox 3.1 (not 3.01 and not 3.0.4)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: