Last Comment Bug 290381 - "Save Link As..." doubles the extension if the link contains an escaped hash/pound (#) sign encoded as %23
: "Save Link As..." doubles the extension if the link contains an escaped hash/...
Status: RESOLVED FIXED
fixed1.8 for toolkit
: fixed1.8
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: Trunk
: x86 Windows XP
: -- minor with 45 votes (vote)
: mozilla1.9alpha1
Assigned To: Caleb
: Hixie (not reading bugmail)
Mentors:
http://www.easynews.com/mozillabug/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-14 15:17 PDT by Troy
Modified: 2016-06-22 12:16 PDT (History)
10 users (show)
mtschrep: blocking1.8b5+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch to contentAreaUtils.js (1.03 KB, patch)
2005-04-18 09:34 PDT, Owain Cleaver
no flags Details | Diff | Review
patch for mozilla trunk (1.00 KB, patch)
2005-07-05 02:05 PDT, Owain Cleaver
no flags Details | Diff | Review
testcases (269 bytes, text/html)
2005-07-25 23:56 PDT, Caleb
no flags Details
toolkit patch v1.0 (1.47 KB, patch)
2005-07-25 23:58 PDT, Caleb
no flags Details | Diff | Review
toolkit patch v1.1 (1.24 KB, patch)
2005-07-26 02:13 PDT, Caleb
no flags Details | Diff | Review
xpfe patch v1.2 (1.68 KB, patch)
2005-07-30 02:55 PDT, Caleb
neil: review-
Details | Diff | Review
xpfe patch v1.3 (1.40 KB, patch)
2005-08-02 02:27 PDT, Caleb
neil: review+
Details | Diff | Review
xpfe patch v1.4 (1.49 KB, patch)
2005-08-02 07:29 PDT, Caleb
bugs.caleb: review+
neil: superreview+
asa: approval1.8b5+
Details | Diff | Review
toolkit patch v1.4 (1.38 KB, patch)
2005-08-02 07:34 PDT, Caleb
asaf: review-
Details | Diff | Review
toolkit patch v1.4.99 (1.34 KB, patch)
2005-08-02 07:52 PDT, Caleb
mconnor: review+
Details | Diff | Review
unbitrotted toolkit (trunk) (1.35 KB, patch)
2005-09-14 22:30 PDT, Caleb
mtschrep: approval1.8b5+
Details | Diff | Review

Description Troy 2005-04-14 15:17:41 PDT
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] 2005-04-14 15:22:09 PDT
confirming with win2k3 and Mozilla 20050414..
Comment 2 Mike.... 2005-04-15 00:22:12 PDT
Also confirming w/ Mozilla 1.7.5 on XP Pro SP2.
Where's my free gigs ??
:o)
Comment 3 SmallFurryThingInACaveGrooving 2005-04-15 19:48:28 PDT
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 2005-04-16 11:59:24 PDT
cofirmed with WinXPSp2 w/ firefox 1.0.2
Comment 5 SmallFurryThingInACaveGrooving 2005-04-16 13:20:42 PDT
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 2005-04-16 19:34:08 PDT
Can't open the same page in a new window either.  Both Opera and IE do that.
Comment 7 Owain Cleaver 2005-04-18 09:34:25 PDT
Created attachment 181042 [details] [diff] [review]
patch to contentAreaUtils.js

Proposed fix for bug.  Should work all platforms.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-05-15 23:35:16 PDT
Owain, want to patch the non-Firefox version of that file too, since the bug's
in Core?
Comment 9 Caleb 2005-06-10 08:45:57 PDT
(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 [:bz] (Out June 25-July 6) 2005-06-10 08:54:00 PDT
See comment 8.  If people are insisting on making this a Firefox-only bug,
please move it to the right product.
Comment 11 Caleb 2005-06-10 09:02:51 PDT
(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 [:bz] (Out June 25-July 6) 2005-06-10 09:46:24 PDT
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 2005-06-10 10:29:36 PDT
(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] 2005-06-10 10:50:51 PDT
Comment on attachment 181042 [details] [diff] [review]
patch to contentAreaUtils.js

he's not a reviewer! :P
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2005-06-10 12:36:02 PDT
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 2005-06-10 12:43:35 PDT
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 2005-06-26 23:15:29 PDT
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 2005-07-05 02:05:32 PDT
Created attachment 188288 [details] [diff] [review]
patch for mozilla trunk

same fix for mozilla suite.
Comment 19 Christian :Biesinger (don't email me, ping me on IRC) 2005-07-05 04:42:22 PDT
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 neil@parkwaycc.co.uk 2005-07-06 16:21:25 PDT
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] 2005-07-25 12:58:32 PDT
Comment on attachment 188288 [details] [diff] [review]
patch for mozilla trunk

see Neil's comment.
Comment 22 Caleb 2005-07-25 23:56:14 PDT
Created attachment 190519 [details]
testcases

Here're a bunch of links you can test the upcoming patch on.
Comment 23 Caleb 2005-07-25 23:58:36 PDT
Created attachment 190520 [details] [diff] [review]
toolkit patch v1.0

Updated patch to address Neil's comments.
Comment 24 neil@parkwaycc.co.uk 2005-07-26 01:38:37 PDT
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 2005-07-26 02:03:17 PDT
(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 2005-07-26 02:13:16 PDT
Created attachment 190527 [details] [diff] [review]
toolkit patch v1.1

Here's an updated toolkit patch.
Comment 27 Caleb 2005-07-28 07:14:48 PDT
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 neil@parkwaycc.co.uk 2005-07-29 04:42:25 PDT
(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) 2005-07-29 05:02:29 PDT
the code should probably ignore all trailing spaces and dots on windows, maybe like:
  aFile = aFile.replace(/[ .]+/, "");
Comment 30 Caleb 2005-07-30 02:55:34 PDT
Created attachment 191038 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2005-07-31 03:15:44 PDT
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 2005-07-31 04:01:55 PDT
(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 2005-08-02 02:27:24 PDT
Created attachment 191318 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2005-08-02 07:17:30 PDT
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 2005-08-02 07:29:54 PDT
Created attachment 191332 [details] [diff] [review]
xpfe patch v1.4

Nits picked, carrying over r+.
Comment 36 Caleb 2005-08-02 07:34:52 PDT
Created attachment 191333 [details] [diff] [review]
toolkit patch v1.4

A toolkit version of the patch.
Comment 37 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-08-02 07:44:54 PDT
Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

You need sr= for anything inside xpfe/
Comment 38 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-08-02 07:48:06 PDT
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 2005-08-02 07:52:47 PDT
Created attachment 191336 [details] [diff] [review]
toolkit patch v1.4.99

I hate myself.
Comment 40 Christian :Biesinger (don't email me, ping me on IRC) 2005-08-02 15:42:21 PDT
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) 2005-08-02 16:00:57 PDT
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 2005-08-02 20:09:32 PDT
Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

Thanks! Requesting approval for a simple fix.
Comment 43 Mike Connor [:mconnor] 2005-09-14 21:45:56 PDT
Comment on attachment 191336 [details] [diff] [review]
toolkit patch v1.4.99

let's get this on trunk ASAP
Comment 44 Caleb 2005-09-14 22:30:49 PDT
Created attachment 196119 [details] [diff] [review]
unbitrotted toolkit (trunk)
Comment 45 Caleb 2005-09-16 11:46:58 PDT
Comment on attachment 196119 [details] [diff] [review]
unbitrotted toolkit (trunk)

can someome please check this patch in the trunk?
Comment 46 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-09-16 19:28:45 PDT
Trunk:
mozilla/toolkit/content/contentAreaUtils.js; new revision: 1.78;
Comment 47 neil@parkwaycc.co.uk 2005-09-18 09:08:24 PDT
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 2005-09-19 15:09:54 PDT
Comment on attachment 196119 [details] [diff] [review]
unbitrotted toolkit (trunk)

Approved for 1.8b5 per bug meeting
Comment 49 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-09-19 17:22:00 PDT
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 neil@parkwaycc.co.uk 2005-09-20 16:38:21 PDT
...and attachment 191332 [details] [diff] [review] is now checked in on the branch too.

Note You need to log in before you can comment on or make changes to this bug.