This was detected in image.module uploads (see this bug report)
Maybe it impacts only some server configs, it involves valid file names and probably magic_quotes settings ("On", in my case)

When you upload an image that has an apostrophe ( ' ) in its name, the image doesn't get copied in the right folder.
The remaining file in temp folder has a \' sequence in it's name, that makes them inadressable (copy, delete...)
(this is due to the antislash, not the apostrophe per se)

This patch replaces the \whatever-character sequences with an undersore in the filename.
Maybe a simple striplashes would do the trick, but I'm not PHP/magic-quotes/filenames savvy enough to decide that.

This bug probably impacts cvs as well - I have no cvs install to test that (The patch is therefore against 4.7.2)

CommentFileSizeAuthor
#22 common.inc_36.patch531 bytesyched
#16 common.inc_35.patch559 bytesyched
#15 common.inc_34.patch546 bytesyched
#3 apostrophe.diff.txt697 bytesbeginner
file.inc_3.patch679 bytesyched

Comments

yched’s picture

yched commented

Oops - I forgot the link to the Image.module bug report.
The beginning of my post should be :

This was detected in image.module uploads (see this bug report)

beginner’s picture

beginner commented
Version: 4.7.2 » x.y.z

bookmarking. I'll try to review later this week.

beginner’s picture

beginner commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new apostrophe.diff.txt697 bytes

I reproduced the bug, and the patch proposed above fixes it by replacing the apostrophe with an underscore.
I attach the same patch, updated for head.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Needs work

I can't reproduce this using the upload module. The file gets copied properly, including apostrophe. There is an issue with the final link to it being incorrect, but that looks like a separate issue. This patch seemed to have no effect.

Maybe more detailed steps to reproduce are needed.

beginner’s picture

beginner commented

no, that's the same issue.
When I tested, the file was uploaded, but the ' was replaced by \' in the file system, and the link was wrong.
with the patch, the apostrophe is removed altogether.

yched’s picture

yched commented

I don't remember about upload.module uploadings, and I'm currently away from my code and test install.
I do know it happens on image.module uploads AND that the bug resides in core's upload management.

So, steps to reproduce : upload an image.module image with an apostrophe in it's name.
(seen on LAMP - PHP 4.4.0 - Magic quotes On)

As i said earlier : I'm not sure this happens on every server config (Win / Linux, PHP magic quotes settings ... might have an effect on this).

Plus, about my patch, as I also said earlier : "Maybe a simple stripslashes would do the trick, but I'm not PHP/magic-quotes/filenames savvy enough to decide that."

Yet it would be really nice to get this solved (and backported ?), since it's hard to explain to your average users to watch out for apostrophes; and when they eventually upload one, it's quite a mess to deal with.

beginner’s picture

beginner commented
Status: Needs work » Needs review

@yched,

image.module would depend on upload.module. Besides, it is not a core module, and it has probably not been updated for head.
The current issue is with the code upload.module. You provided the patch yourself, so it probably means that it fixes your problem with image.module.

Anyway, drumm didn't know how to reproduce.
Can you reproduce the steps below, and confirm that your own patch (re-rolled for head at #3) works. If so, set the status as Ready to be Committed.

1) enable upload.module
2) create new node.
3) in the node edit form, Attach a new file I'm a test.txt
4) I don't click attach, but directly hit submit.
result:
5) the link to the file attached is files/I/'m a test.txt
6) looking at the files/ directory, the file has been renamed I\'m a test.txt, i.e. a \ has been added.

After the patch:
follow the same procedures, but the results are:
5) the link to the file is correct. I can click on it and view the file.
6) the file has been renamed I_m a test.txt.

beginner’s picture

beginner commented

Before the patch, in 5) above, the links looks like : files/I/'m a test.txt.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

image.module would depend on upload.module
Not exactly. It's rather both depending on the same code in core's file.inc

Anyway. Thanks for doing the rephrasing job I should have done myself, Augustin.

So : yes, the steps you describe in comment #7 for pre- and post-patch behaviours are correct,
and the patch in comment #3 does fix things.

RTBC, then ?...

beginner’s picture

beginner commented

er, yes, the patch is patching files.inc, you are right. :)

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Needs work

Rather than a hack in file.inc, this should be merged into the fix_gpc_magic() function we already have, and operate globally on whatever fields are escaped in $_FILES.

yched’s picture

yched commented

OK, I'll look into this next week end.

beginner’s picture

beginner commented

this is related (Apostrophe returns '#039')
http://drupal.org/node/79821

beginner’s picture

beginner commented

is it possible to have a more general fix that fixes both issues?

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new common.inc_34.patch546 bytes

New patch, according to Steven's advice (fix_gpc_magic)
This is actually, er, cleaner...

Tested to work on my hosting config
(LAMP - PHP 4.4.0 - Magic quotes On)

Maybe another round of reviewing before RTBC ?

@beginner : the issue you mention is kind of related,
but it involves the output of the file name by upload.module,
rather than the handling of the file itself, so I kept this
out of this patch ATM.
But this does have to be fixed as well :-)
I'll try to have a closer look.

yched’s picture

yched commented
StatusFileSize
new common.inc_35.patch559 bytes

And here is the patch that applies to 4.7 branch

yched’s picture

yched commented

For the record, I submitted a patch here for the issue begginner mentioned.

beginner’s picture

beginner commented

I couldn't test either of your patches, because they don't apply: they don't have standard unix line endings.

Are both patches on both issues necessary? What I wanted to test how the other patch would influence the issue at hand here.

beginner’s picture

beginner commented

your other patch got committed so I tested again this issue, without the patch above.
It is already somewhat better, but a backward slash is added in the file saved in the directory: I\'m a test.txt but the link points to files/I/'m a test.txt with a forward slash.

I guess applying the patch above would still fix this by removing the apostrophe altogether.

Would it be difficult to remove the need to rename the file (i.e. keep the apostrophe in the file name)? If yes, I don't mind the use of the underscore.

yched’s picture

yched commented

In fact, the current patch leaves the filename unchanged :
If you upload I'm a test.txt, you get I'm a test.txt.

About Unix line endings : I'm on Windows, using Tortoise VCS to generate patches.
However, this is the first time I'm told that my patches don't apply.

This patch is just one line, so maybe for now you can apply it manually ?
+ any hints welcome for me to follow the rules :-)

drumm’s picture

drumm
he/him
Location NY, US
commented

I run every patch through a linebreak fixer (I use a little program called dos2unix, and there are various 1-liners in your text processor of choice) and a couple sed statements to remove end of line whitespace.

yched’s picture

yched commented
StatusFileSize
new common.inc_36.patch531 bytes

OK, here's the patch for HEAD, with unix line endings.
Thanks for the tip.

beginner’s picture

beginner commented
Status: Needs review » Reviewed & tested by the community

Tested. The apostrophe is kept and everything works with the patch. (thanks for the unix patch which makes it much easier to apply and *remove* the patch in a flash).

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

killes@www.drop.org’s picture

killes@www.drop.org commented

backported

andrewfn’s picture

andrewfn commented

Today a user uploaded a file with a name containing four apostrophes to my test site (running cvs). It uploaded perfectly and preserved the apostrophes in the file name, but this made it impossible other users to download it. The only way to fix this problem was for me to get the file from the server by FTP, rename it, and then re-upload it. The file name was: word study - 'put on' - 'hidden'.doc and the error message was:
An appropriate representation of the requested resource /files/word study - 'put on' - 'hidden'.doc could not be found on this server.
It seems to me that either apostrophes cannot be left in files that are intended to be downloaded, or some fix has to be applied to the URL generated for downloading.

beginner’s picture

beginner commented

Hello andrew, please open another issue and link back to this one.

Give precise information on how to reproduce the problem (create a test file, which browser is used to download, etc...).

Things can be fixed only if developers can reproduce the problem.

After you've created the new issue, post a link to it below.

andrewfn’s picture

andrewfn commented

I have created a test site to reproduce the problem and posted a new issue here: http://drupal.org/node/87891

andrewfn’s picture

andrewfn commented

I have created a test site to reproduce the problem and posted a new issue here: http://drupal.org/node/87891

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)