290381 - "Save Link As..." doubles the extension if the link contains an escaped hash/pound (#) sign encoded as %23

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Troy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2

There is a proof of concept at http://www.easynews.com/mozillabug/ . 
Right-click and save link as on the second link.  The dialog shows a doubled
extension on the jpg file as you're about to save it.

Reproducible: Always

Steps to Reproduce:
1. Create an HTML page with a %23 in a link target.
2. Do a Save Link As... on that link.

Actual Results:  
Filename extension is doubled (.jpg.jpg), but can be manually corrected before
saving.  It seems that the extension may be being guessed based off the mime
type since some part of the code may be dropping the part after the decoded #
sign before validating the extension.

Expected Results:  
Should have presented the link as-is and not doubled the extension.

Comment 1

[Matthias Versen [:Matti]]

confirming with win2k3 and Mozilla 20050414..

Comment 2

[Mike....]

Also confirming w/ Mozilla 1.7.5 on XP Pro SP2.
Where's my free gigs ??
:o)

Comment 3

[SmallFurryThingInACaveGrooving]

also confirmed on linux kernel 2.6 kde 3.3 firefox 1.01

I could use some free gigs too as about a gig was wasted yesterday by a bug in
pan :(

Comment 4

[alanv72]

cofirmed with WinXPSp2 w/ firefox 1.0.2

Comment 5

[SmallFurryThingInACaveGrooving]

f the extention is being guessed off of the MIME type, try the proof of concept
by naming an mpeg file as .jpg given the above example. If the sample tries to
save as file.jpg.mpg or file.mpg.jpg, then that's probably the case

Comment 6

[Sam Lindhorst]

Can't open the same page in a new window either.  Both Opera and IE do that.

Comment 7

[Owain Cleaver]

Attachment: patch to contentAreaUtils.js

Proposed fix for bug.  Should work all platforms.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Owain, want to patch the non-Firefox version of that file too, since the bug's
in Core?

Comment 9

[Caleb]

(In reply to comment #7)
> Created an attachment (id=181042) [edit]
> patch to contentAreaUtils.js
> 
> Proposed fix for bug.  Should work all platforms.

Can you please set review flags on your patch? The sooner it gets a review the
more chances that it gets into 1.1!

Comment 10

[Boris Zbarsky [:bzbarsky]]

See comment 8.  If people are insisting on making this a Firefox-only bug,
please move it to the right product.

Comment 11

[Caleb]

(In reply to comment #10)
> See comment 8.  If people are insisting on making this a Firefox-only bug,
> please move it to the right product.

OK. I suppose that it's better to fix the problem at the core rather to band-aid
it in Firefox.

But if no such solution is available in a timely manner, this bug will sneak
into another release I suppose.

Btw Owain, by setting the review flags, I meant requesting for review, and not
setting it to + (unless, of course, you have the permissions to do so).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Caleb, I have no idea what you're talking about.  Please read comment 8 and the
patch again.  It's the right approach; it just needs to be done in two places.

Comment 13

[Caleb]

(In reply to comment #12)
> Caleb, I have no idea what you're talking about.  Please read comment 8 and the
> patch again.  It's the right approach; it just needs to be done in two places.

Sorry, I've misunderstood :)

So this patch can go as is and the bug can remain open for work to be done on
the other part of it then ?

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 181042 [details] [diff] [review]
patch to contentAreaUtils.js

he's not a reviewer! :P

Comment 15

[Boris Zbarsky [:bzbarsky]]

IF the patch gets reviewed by someone competent who decides that they really
don't care about keeping the two files in sync (which is probably about nobody,
since we're trying to sync them up), yeah.

Comment 16

[Owain Cleaver]

Mike: i might be ;-)

Caleb: Sorry, i'm new to bugzilla and that was the only "review" option i could
find.

Boris/all: i'm quite willing to fix the mozilla trunk but it won't be until
monday.  Only reason i didn't was because i thought the moz equivalent was in
xpfe C++ code and i don't have the build tools.

So um, wait if you can.

Comment 17

[Caleb]

I'm requesting blocking for 1.8b4 since this bug could really piss off people
downloading lots of images that contain these urlencoded chars. (Like, say, from
a gallery)

Comment 18

[Owain Cleaver]

Attachment: patch for mozilla trunk

same fix for mozilla suite.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 188288 [details] [diff] [review]
patch for mozilla trunk

could you diff with more context, like 6 or 8?

also, I'm not sure there's need to keep historical information like this in
comments ("no longer use nsIURL")

Does this do the right thing for "foo.exe.."?

Comment 20

[[e-mail]]

Comment on attachment 188288 [details] [diff] [review]
patch for mozilla trunk

>+  var i = aFile.lastIndexOf(".");
>+  if(i < 0)
>+    return aFile;
This isn't quite correct; we want to treat ".foo" as having no extension.

>-  if (url.fileExtension != aDefaultExtension) {
>+  if (aFile.substr(i+1) != aDefaultExtension) {
Nit: spaces around +
>     return aFile + "." + aDefaultExtension;

I was trying to think of some way of doing "ends with aDefaultExtension" in
JavaScript but the best I could come up with was
aDefaultExtension = "." + aDefaultExtension;
if (aFile.slice(1).slice(-aDefaultExtension.length) != aDefaultExtension)
  return aFile + aDefaultExtension;

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 188288 [details] [diff] [review]
patch for mozilla trunk

see Neil's comment.

Comment 22

[Caleb]

Attachment: testcases

Here're a bunch of links you can test the upcoming patch on.

Comment 23

[Caleb]

Attachment: toolkit patch v1.0

Updated patch to address Neil's comments.

Comment 24

[[e-mail]]

Comment on attachment 190520 [details] [diff] [review]
toolkit patch v1.0

>+  if(i <= 0 || i == aFile.length-1)
Nit: space before ( and before and after -
(although I wonder why you added that second test)

>+    aDefaultExtension = "." + aDefaultExtension;
>+    if (aFile.slice(1).slice(-aDefaultExtension.length) != aDefaultExtension)
>+      return aFile + aDefaultExtension;
I didn't mean you to make the change like this.
Hopefully mconnor will decide it's unreadable anyway ;-)

Comment 25

[Caleb]

(In reply to comment #24)
> (From update of attachment 190520 [details] [diff] [review] [edit])
> >+  if(i <= 0 || i == aFile.length-1)
> Nit: space before ( and before and after -
> (although I wonder why you added that second test)

I added the second test so that files such as "bleh." are also considered not to
have an extension.

If you check out the testcase, without the patch ".bleh" and "bleh." get
appended with ".html", with this patch they don't get any extra extension at all.

> >+    aDefaultExtension = "." + aDefaultExtension;
> >+    if (aFile.slice(1).slice(-aDefaultExtension.length) != aDefaultExtension)
> >+      return aFile + aDefaultExtension;
> I didn't mean you to make the change like this.
> Hopefully mconnor will decide it's unreadable anyway ;-)

The previous "method" worked perfectly, and so does this one, I've only changed
it because I thought you wanted it this way ;-)

Comment 26

[Caleb]

Attachment: toolkit patch v1.1

Here's an updated toolkit patch.

Comment 27

[Caleb]

Neil says he doesn't know (yet) whether he wants "blah." to mean "this file has
no extensions".

If you compare the testcase (attachment 190519 [details]) with and without the patch, you
get this:

With:
bleh.jpg -> bleh.jpg
bleh#3.jpg -> bleh#3.jpg
bleh. -> bleh.
.bleh - > .bleh
bleh.exe.. -> bleh.exe..
bleh -> bleh (this test isn't included in the testcase)

Without
bleh.jpg -> bleh.jpg
bleh#3.jpg -> bleh#3.jpg.jpg
bleh. -> bleh..htm
.bleh - > .bleh.htm
bleh.exe.. -> bleh.exe...htm
bleh -> bleh.htm (this test isn't included in the testcase)

Comment 28

[[e-mail]]

(In reply to comment #27)
>bleh.exe.. -> bleh.exe..
This is bad, because Windows ignores the trailing .s and saves it as bleh.exe so
you need to preferably save bleh.exe.htm although I would accept bleh.exe...htm

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

the code should probably ignore all trailing spaces and dots on windows, maybe like:
  aFile = aFile.replace(/[ .]+/, "");

Comment 30

[Caleb]

Attachment: xpfe patch v1.2

This patch removes trailing spaces and dots on windows, and appends the default
extension for invalid files.

With (On win32, other platforms do different things):
bleh.jpg -> bleh.jpg
bleh#3.jpg -> bleh#3.jpg
bleh. -> bleh
.bleh - > .bleh.htm (Win32 Only - The only reason I append the default
extension in this case is because ".filename" is invalid on win32)
bleh.exe.. -> bleh.exe.htm
bleh -> bleh (this test isn't included in the testcase)

Without
bleh.jpg -> bleh.jpg
bleh#3.jpg -> bleh#3.jpg.jpg
bleh. -> bleh..htm
.bleh - > .bleh.htm
bleh.exe.. -> bleh.exe...htm
bleh -> bleh.htm (this test isn't included in the testcase)

Comment 31

[[e-mail]]

Comment on attachment 191038 [details] [diff] [review]
xpfe patch v1.2

>+#ifdef XP_WIN
In XPFE you need to use
if (/Win/.test(navigator.platform))

>+  if (i <= 0 || i == aFile.length - 1)
>+    return aFile;
>+#endif
>+  if (aFile.substr(i + 1) != aDefaultExtension)
Thinking about this, I'm not sure this is the right test. On all platforms I
think you should use
if (i <= 0 || aFile.substr(i + 1) != aDefaultExtension)
  return aFile + "." + aDefaultExtension;

Comment 32

[Caleb]

(In reply to comment #31)
> (From update of attachment 191038 [details] [diff] [review] [edit])
> >+  if (i <= 0 || i == aFile.length - 1)
> >+    return aFile;
> >+#endif
> >+  if (aFile.substr(i + 1) != aDefaultExtension)
> Thinking about this, I'm not sure this is the right test. On all platforms I
> think you should use
> if (i <= 0 || aFile.substr(i + 1) != aDefaultExtension)
>   return aFile + "." + aDefaultExtension;

You're probably right. I tried to butcher the filename as least as possible, but
it seems that "htm" is passed as the default file extension to the file picker
when you Right Click > Save As (since we have no content-type/disposition), so
even if I leave "bleh" as it is, it will still come out as "bleh.htm", and same
goes for every other file that has an "invalid" extension.

Does the same thing happen for Linux/etc..? Because I think it would suck to
just append .htm to every file not ending with .htm. 

Comment 33

[Caleb]

Attachment: xpfe patch v1.3

OK Neil, this patch should do what you want (always append the default
extension... just as it did before).

Comment 34

[[e-mail]]

Comment on attachment 191318 [details] [diff] [review]
xpfe patch v1.3

Nit: inconsitent bracing styles, your new block includes them but you removed
them from the block you changed. r=me with that fixed.

Comment 35

[Caleb]

Attachment: xpfe patch v1.4

Nits picked, carrying over r+.

Comment 36

[Caleb]

Attachment: toolkit patch v1.4

A toolkit version of the patch.

Comment 37

[Asaf Romano (gone)]

Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

You need sr= for anything inside xpfe/

Comment 38

[Asaf Romano (gone)]

Comment on attachment 191333 [details] [diff] [review]
toolkit patch v1.4

>Index: contentAreaUtils.js
>===================================================================

>+#ifdef XP_WIN
>+  // Remove trailing dots and spaces on windows
>+  if (/Win/.test(navigator.platform))
>+    aFile = aFile.replace(/[\s.]+$/, "");
>+#endif
>+

No reason to check navigator.platform here...

Comment 39

[Caleb]

Attachment: toolkit patch v1.4.99

I hate myself.

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

sorry, I'm not a super-reviewer...

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

however, the patch looks right to me, so r=biesi; maybe neil can upgrade his r
to an sr ;)

Comment 42

[Caleb]

Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

Thanks! Requesting approval for a simple fix.

Comment 43

[Mike Connor [:mconnor]]

Comment on attachment 191336 [details] [diff] [review]
toolkit patch v1.4.99

let's get this on trunk ASAP

Comment 44

[Caleb]

Attachment: unbitrotted toolkit (trunk)

Comment 45

[Caleb]

Comment on attachment 196119 [details] [diff] [review]
unbitrotted toolkit (trunk)

can someome please check this patch in the trunk?

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Trunk:
mozilla/toolkit/content/contentAreaUtils.js; new revision: 1.78;

Comment 47

[[e-mail]]

Nobody seems to have checked attachment 191332 [details] [diff] [review] in anywhere (I guess its branch
approval is now out of date?) so I checked it in on the trunk at least.

Comment 48

[Mike Schroepfer]

Comment on attachment 196119 [details] [diff] [review]
unbitrotted toolkit (trunk)

Approved for 1.8b5 per bug meeting

Comment 49

[Asaf Romano (gone)]

1.8 Branch:

Checking in contentAreaUtils.js;
/cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.77.2.1; previous revision: 1.77
done

Comment 50

[[e-mail]]

...and attachment 191332 [details] [diff] [review] is now checked in on the branch too.