-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for Non-ASCII filenames #117
base: master
Are you sure you want to change the base?
Conversation
Apply decodeURI to filenames (received from server) before comparing them to name of file we're going to upload. Issue was that server send percent-encodes non-URL-friendly symbols, (space " " becomes "%20"), while local files might have them. As a result, you could upload file called "a file.txt" multiple times: UI shows it as several files with same filename, but actually DAV overwrites them without prompt.
Otherwise, if new filename contained non-ASCII characters, like german "ẞß", for example, error happened in HTTP.ts:39 (in `method` function, on a line which says `const request = new Request(url, ...`): TypeError: Request constructor: Cannot convert value in record<ByteString, ByteString> branch of (sequence<sequence<ByteString>> or record<ByteString, ByteString>) to ByteString because the character at index 53 has value 1099 which is greater than 255.
Otherwise, a file called "a file" is downloaded as "a%20file".
61f6185
to
db15cd3
Compare
on the other side, I wonder if, instead of finding all places where |
Hey there, thanks again for flagging this! It'd be great to have a test-case for this (that we can test against different server implementations if required perhaps?) Do you have a specific filename/server configuration that this fails with? If we could confirm this, write a test and then fix, that'd be ideal in my opinion! |
@@ -18,8 +18,11 @@ export const handleFileUpload = async ( | |||
|
|||
state.setCollection(collection); | |||
|
|||
// NOTE: we can't use entry.name === encodeURI(file.name) here, | |||
// because server and encodeURI() might use different case for | |||
// %-encoded values, like "%D0" vs "%d0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: To avoid this problem we could toUpperCase()
/toLowerCase()
too. If you think it's valuable, but I have no problem with this approach either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this won't work because toUpperCase()
/toLowerCase()
will also change case of other letters in filename. For example, if server says that it has a file called A%2dFile.txt
, and we try to upload a file called A%2DFile.txt
(different case of letter "D"), then toUpperCase()
and toLowerCase()
will turn it to A%2DFILE.TXT
and a%2dfile.txt
, respectively, which won't match filename on the server.
We could convert both filenames to upper case, but then we would assume that backend server case-insensitive, and uploading a file called FILE.TXT
overwrites a file called file.txt
. Which is actually a case only on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. Another alternative could be decoding and re-encoding so that the case (should?) be consistent as it's using the browser to perform the encode both sides. For .toUpperCase()
we could .replace(/%[0-9a-f]{2}/g, (s) => s.toUpperCase())
although I wonder if we'll encounter problems with values like %252d
/%252D
etc.
Yeah, this makes sense. I don't know if we'd ever need the "wrong" filename. Happy for this approach too! |
Hi again and sorry for the delayed response! Thanks for suggestion regarding testcase! I now found them in I tried to explain filenames in commit messages, but not clear enough :) Let me try again: First commit, Properly compare filenames before uploading:
Expected: you should be asked if you want to overwrite the file. Currently, file is uploaded (and overwritten) without warning, and UI shows several files with same filename. Second commit, URI-encode destination filename when renaming:
Expected: it should be renamed both in UI and on server. Currently, you get an error in console, like this:
Third commit, Decode percent-encoded symbols in download file name
Expected: downloaded file is called Currently, it's called But now I also found another issue, to which I don't have a fix: Percent-encoded strings in filenames are decoded during upload:
Expected: uploaded file is called Currently, it's called I suspect that it's because filename is passed as-is to the server, and when server sees percent-encoded strings like |
Note to self: also, "Download" links should %-encode square brackets |
No description provided.