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

RESOLVED FIXED in mozilla1.9alpha1

Status

Core Graveyard
File Handling
--
minor
RESOLVED FIXED
12 years ago
10 months ago

People

(Reporter: Troy, Assigned: Caleb)

Tracking

({fixed1.8})

Trunk
mozilla1.9alpha1
x86
Windows XP
fixed1.8
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed1.8 for toolkit, URL)

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

12 years ago
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.
confirming with win2k3 and Mozilla 20050414..
Assignee: bugs → file-handling
Status: UNCONFIRMED → NEW
Component: File Handling → File Handling
Ever confirmed: true
Product: Firefox → Core
QA Contact: aebrahim-bmo → ian
Version: unspecified → Trunk

Comment 2

12 years ago
Also confirming w/ Mozilla 1.7.5 on XP Pro SP2.
Where's my free gigs ??
:o)
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

12 years ago
cofirmed with WinXPSp2 w/ firefox 1.0.2
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

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

Comment 7

12 years ago
Created attachment 181042 [details] [diff] [review]
patch to contentAreaUtils.js

Proposed fix for bug.  Should work all platforms.
Owain, want to patch the non-Firefox version of that file too, since the bug's
in Core?
(Assignee)

Comment 9

12 years ago
(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!

Updated

12 years ago
Attachment #181042 - Flags: review+
See comment 8.  If people are insisting on making this a Firefox-only bug,
please move it to the right product.
(Assignee)

Comment 11

12 years ago
(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).
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.
(Assignee)

Comment 13

12 years ago
(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 ?
(Assignee)

Updated

12 years ago
Attachment #181042 - Flags: approval-aviary1.1a2?
Comment on attachment 181042 [details] [diff] [review]
patch to contentAreaUtils.js

he's not a reviewer! :P
Attachment #181042 - Flags: review+

Updated

12 years ago
Attachment #181042 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
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

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #181042 - Flags: approval-aviary1.1a2+
(Assignee)

Comment 17

12 years ago
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)
Flags: blocking1.8b4?

Comment 18

12 years ago
Created attachment 188288 [details] [diff] [review]
patch for mozilla trunk

same fix for mozilla suite.
(Assignee)

Updated

12 years ago
Whiteboard: has patch, needs review
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

12 years ago
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;
Whiteboard: has patch, needs review → [no l10n impact] has patch, needs review
Attachment #188288 - Flags: review?(mconnor)
Comment on attachment 188288 [details] [diff] [review]
patch for mozilla trunk

see Neil's comment.
Attachment #188288 - Flags: review?(mconnor)
(Assignee)

Comment 22

12 years ago
Created attachment 190519 [details]
testcases

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

Comment 23

12 years ago
Created attachment 190520 [details] [diff] [review]
toolkit patch v1.0

Updated patch to address Neil's comments.
Assignee: file-handling → bugs.caleb
Attachment #181042 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #190520 - Flags: review?(mconnor)

Comment 24

12 years ago
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 ;-)
(Assignee)

Comment 25

12 years ago
(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 ;-)
(Assignee)

Comment 26

12 years ago
Created attachment 190527 [details] [diff] [review]
toolkit patch v1.1

Here's an updated toolkit patch.
Attachment #190520 - Attachment is obsolete: true
Attachment #190527 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #190520 - Flags: review?(mconnor)
(Assignee)

Comment 27

12 years ago
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

12 years ago
(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
the code should probably ignore all trailing spaces and dots on windows, maybe like:
  aFile = aFile.replace(/[ .]+/, "");
(Assignee)

Updated

12 years ago
Attachment #190527 - Attachment is obsolete: true
Attachment #190527 - Flags: review?(mconnor)
(Assignee)

Comment 30

12 years ago
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)
(Assignee)

Updated

12 years ago
Attachment #188288 - Attachment is obsolete: true
Attachment #191038 - Flags: review?(neil.parkwaycc.co.uk)

Comment 31

12 years ago
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;
Attachment #191038 - Flags: review?(neil.parkwaycc.co.uk) → review-
(Assignee)

Comment 32

12 years ago
(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. 
(Assignee)

Comment 33

12 years ago
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).
(Assignee)

Updated

12 years ago
Attachment #191038 - Attachment is obsolete: true
Attachment #191318 - Flags: review?(neil.parkwaycc.co.uk)

Comment 34

12 years ago
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.
Attachment #191318 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Comment 35

12 years ago
Created attachment 191332 [details] [diff] [review]
xpfe patch v1.4

Nits picked, carrying over r+.
(Assignee)

Updated

12 years ago
Attachment #191318 - Attachment is obsolete: true
Attachment #191332 - Flags: review+
Attachment #191332 - Flags: approval1.8b4?
(Assignee)

Comment 36

12 years ago
Created attachment 191333 [details] [diff] [review]
toolkit patch v1.4

A toolkit version of the patch.
Attachment #191333 - Flags: review?(mconnor)
Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

You need sr= for anything inside xpfe/
Attachment #191332 - Flags: approval1.8b4?
(Assignee)

Updated

12 years ago
Attachment #191332 - Flags: superreview?(cbiesinger)
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...
Attachment #191333 - Flags: review?(mconnor) → review-
(Assignee)

Comment 39

12 years ago
Created attachment 191336 [details] [diff] [review]
toolkit patch v1.4.99

I hate myself.
(Assignee)

Updated

12 years ago
Attachment #191333 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #191336 - Flags: review?(mconnor)
Comment on attachment 191332 [details] [diff] [review]
xpfe patch v1.4

sorry, I'm not a super-reviewer...
Attachment #191332 - Flags: superreview?(cbiesinger)
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 ;)

Updated

12 years ago
Attachment #191332 - Flags: superreview+
(Assignee)

Comment 42

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

Thanks! Requesting approval for a simple fix.
Attachment #191332 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #191332 - Flags: approval1.8b4? → approval1.8b4+

Updated

12 years ago
Flags: blocking1.8b4? → blocking1.8b4-
Comment on attachment 191336 [details] [diff] [review]
toolkit patch v1.4.99

let's get this on trunk ASAP
Attachment #191336 - Flags: review?(mconnor) → review+
(Assignee)

Comment 44

12 years ago
Created attachment 196119 [details] [diff] [review]
unbitrotted toolkit (trunk)
(Assignee)

Comment 45

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

can someome please check this patch in the trunk?
Whiteboard: [no l10n impact] has patch, needs review → [no l10n impact][checkin needed]
Trunk:
mozilla/toolkit/content/contentAreaUtils.js; new revision: 1.78;
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact][checkin needed] → [no l10n impact]
Target Milestone: --- → mozilla1.9alpha

Comment 47

12 years ago
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.
Attachment #191332 - Flags: approval1.8b4+ → approval1.8b5?
Attachment #196119 - Flags: approval1.8b5?

Updated

12 years ago
Flags: blocking1.8b5- → blocking1.8b5+

Comment 48

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

Approved for 1.8b5 per bug meeting
Attachment #196119 - Flags: approval1.8b5? → approval1.8b5+
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
Whiteboard: [no l10n impact] → fixed1.8 for toolkit

Updated

12 years ago
Keywords: fixed1.8

Updated

12 years ago
Attachment #191332 - Flags: approval1.8b5? → approval1.8b5+

Comment 50

12 years ago
...and attachment 191332 [details] [diff] [review] is now checked in on the branch too.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.