Closed Bug 283730 (Shortcut) Opened 19 years ago Closed 19 years ago

"Save As" dialog tries to overwrite link/shortcut (.lnk) file instead of opening the directory/folder

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Version:
1.7 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: super_egi, Assigned: emaijala+moz)

References

Details

(Keywords: fixed-aviary1.0.5, fixed1.7.9, regression)

Attachments

(2 files, 6 obsolete files)

Backout patch
680 bytes, patch
Details | Diff | Splinter Review
alternate version of YAAP
2.10 KB, patch
emaijala+moz
: review+
dougt
: superreview+
asa
: approval-aviary1.0.5+
asa
: approval1.7.9+
asa
: approval1.8b3+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050224 Firefox/1.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050224 Firefox/1.0.2

When I try to save a file and select a lnk file that links to a folder, it asks
if it should overwrite the lnk file. The other versions of Firefox go to the
folder instead.

Reproducible: Always

Steps to Reproduce:
1. File - Save file as...
2. double click a link to a folder

Actual Results:  
It tried to overwrite the link and made it unusable.

Expected Results:  
It should have gone to the folder instead of overwriting the link.
This is fallout from bug 271732.
Depends on: 271732
Keywords: regression
*** Bug 283780 has been marked as a duplicate of this bug. ***
*** Bug 283720 has been marked as a duplicate of this bug. ***
Version: unspecified → 1.0 Branch
*** Bug 283801 has been marked as a duplicate of this bug. ***
Summary: save file - folder link: tries to overwrite link → save file - folder link/shortcut: tries to overwrite link instead of opening the directory
Discussion of this bug on Mozillazine forums:
http://forums.mozillazine.org/viewtopic.php?t=228991
Is this present on the trunk? If not, then I guess there's not a great chance of
it being fixed. If it is, then this is a relatively annoying regression from 1.0
and I'm sure many would appreciate a fix for 1.0.2.
Flags: blocking-aviary1.1?
Yes, it is present in the latest trunk
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050306
Firefox/1.0+
Nominating in case there's some simple way to distinguish links that point at a
file vs links that point at a folder.

Not a Firefox bug, Core file handling
Assignee: bugs → file-handling
Component: Download Manager → File Handling
Flags: blocking-aviary1.0.2?
Product: Firefox → Core
QA Contact: aebrahim-bmo → ian
Version: 1.0 Branch → 1.7 Branch
*** Bug 285179 has been marked as a duplicate of this bug. ***
Summary: save file - folder link/shortcut: tries to overwrite link instead of opening the directory → save file - folder link/shortcut: tries to overwrite link (.lnk) instead of opening the directory
*** Bug 285280 has been marked as a duplicate of this bug. ***
*** Bug 285877 has been marked as a duplicate of this bug. ***
Why not just look at the target of the shortcut and then use GetFileAttributesEx()?
Nominating for 1.7.6 too, since it's busted there....
Flags: blocking1.8b2?
Flags: blocking1.7.6?
what platforms?  This was fixed to fix a possible security exploit 271732. 
Between the two, i would rather have this bug than the other.  Maybe it doesn't
have to be a dichotomy.
I am of the exact opposite opinion. It seems like the other bug is a Windows
issue and not a Mozilla/Firefox issue. I don't like having the usefulness of
shortcuts removed on my platform because if you save a file two times in the
same place, it might overwrite another file on *some* subversion of my platform.

BTW - What happened to mozilla.org's position that Windows security bugs are
Windows problems, and not the application's issue? See bugs related to storing
email encrypted, etc.
Summary: save file - folder link/shortcut: tries to overwrite link (.lnk) instead of opening the directory → "Save As" dialog tries to overwrite link/shortcut (.lnk) file instead of opening the directory/folder
*local* security issues are windows problems. As a browser we're responsible for
security issues we bring in from the outside world.
Does Firefox create the Save As dialog and do the navigation, or does Windows?
Seems to me you can't have your cake (hand off the file handling to the Windows
common dialog), and eat it too (take responsibility for what Windows then does
with that file).

At least try not to let people who do not use Windows on a daily basis be the
ones to decide the fate of issues affecting Windows.
(In reply to comment #18)
> Does Firefox create the Save As dialog and do the navigation, or does Windows?

windows (via GetSaveFileName); i.e. firefox tells windows "show a filepicker and
give me back a filename"
I ask because if the Windows file system chooses to overwrite the target of a
link when a link is overwritten with another, then that is a Windows file system
issue.

Although it is noble to try and protect users from malicious people, there has
to be a line somewhere. At some point everyone is going to have to realize that
no matter what you do, people will do unwise things and there is nothing you can
do to stop it. The sooner that is acknowledged, the better off we'll be. My
personal opinion is that if you're going to click OK to download an unknown file
two times, and then OK the overwrite of an existing file, then there is no
protecting you anyway. Let's not break this functionality for everyone to try
and save some poor souls who cannot be helped anyway.
*** Bug 286484 has been marked as a duplicate of this bug. ***
(In reply to comment #20)

See nsFilePicker.cpp, about line 235:
    else if (mMode == modeSave) {
      ofn.Flags |= OFN_NOREADONLYRETURN | OFN_NODEREFERENCELINKS;
      result = nsToolkit::mGetSaveFileName(&ofn);


"OFN_NODEREFERENCELINKS" !
or really see comment 1!
*** Bug 286801 has been marked as a duplicate of this bug. ***
*** Bug 286778 has been marked as a duplicate of this bug. ***
*** Bug 286928 has been marked as a duplicate of this bug. ***
Flags: blocking1.7.7?
I tested it with an older version and the IE and understand the decision.
But couldn't it be made configurable?

Unfortunately there seems to be no way on Windows for a better solution, or
escaped anything my notice?
Is there a way to update certain files of Firefox so the security holes are
fixed, but this bug is gone? I'm not going to download a pif-file anyway, so i'd
rather have <a href="show_bug.cgi?id=271732" title="VERIFIED FIXED - COMMAND.COM
is overwritten by downloading the pif file">bug 271732</a>.
Not only do you have to download a PIF file, but you have to do it on Windows
ME, you have to overwrite another PIF file with the same target, and you have to
click OK on a dialog box that asks you if you want to overwrite command.com.
Keep that in mind when assessing the security risk of the "bug" that's fix is
responsible for this bug.
You're misrepresenting the situation - it's true that for pif files the issue 
only happens with certain platforms, but for lnk files (which is the issue for 
this bug anyway) the problem exists in all Windows versions.  Nobody is arguing 
that the risk isn't low - it says "Risk: Low" right at the top of the security 
advisory.

If something is going to get fixed here, someone needs to come up with a 
solution that doesn't involve reintroducing that low risk - ranting about it 
isn't going to get anything done, and bugzilla certainly isn't the place to 
discuss where to draw the line between usability and protecting users from 
themselves.
The risk isn't any more than the risk of a user selecting an existing system
file directly to overwrite. Why not just disallow replacing any files at all?
I'm being facetious of course.

The reason for posting arguments against (or for) a bug in the bug are for other
people that wonder in later. They will not be privy to expired discussions on
newsgroups or at MozillaZine.
> expired discussions on newsgroups

news.mozilla.org does not expire anything
*** Bug 287614 has been marked as a duplicate of this bug. ***
*** Bug 287717 has been marked as a duplicate of this bug. ***
To fix bug #271732, something else was broken.  This worked with Mozilla 1.7.5
and does not work with Mozilla 1.7.6.  Thus, this bug is a regression and should
have a priority higher than Normal.  

I have an extensive directory structure spread over two physical hard drives. 
To simplify navigation, I make extensive use of shortcuts.  When I go to
download a file, I can usually reach the preferred destination for the
downloaded file by selecting a shortcut from wherever I am.  This no longer
works.  Instead, I must first try to remember on which drive the destination
resides and then navigate downward from there.  What a *#@#!! bother!  
*** Bug 279070 has been marked as a duplicate of this bug. ***
(In reply to comment #35)
...
> I have an extensive directory structure spread over two physical hard drives. 
> To simplify navigation, I make extensive use of shortcuts.  When I go to
> download a file, I can usually reach the preferred destination for the
> downloaded file by selecting a shortcut from wherever I am.  This no longer
> works.  Instead, I must first try to remember on which drive the destination
> resides and then navigate downward from there.  What a *#@#!! bother!  
...
I strongly agree! Why not give users who are willing to take responsibility for
their own security, a choice (in about:config maybe? where most windows users
never look) so we are able to take action, and make links work like links rather
than files.

I have an alternative way to fix bug 271732 that wouldn't cause this bug. I
suggest we back out the fix for bug 271732, and do the following instead:

Before opening a save dialog, we look at the default file path. If it is a
shortcut filename (pif, lnk, url), we use an empty default filename instead,
requiring the user to select a filename. This would prevent users from
accidentally saving shortcuts.

(In reply to comment #38)
I realy don't care if I save a file as an shortcut. The bigest problem is that
you can't use the shortcuts to navigate.
Not blocking 1.7.7 or 1.0.3 security releases
Flags: blocking1.7.7?
Flags: blocking1.7.7-
Flags: blocking1.7.6?
Flags: blocking-aviary1.0.3?
Flags: blocking-aviary1.0.3-
bug 288769 is a duplicate of this bug but I don't can't seem to find a way how I
can change this in the other bug report.

Is this bug being worked on? It's quite annoying and simply a wrong behaviour
which should be fixed.
*** Bug 288769 has been marked as a duplicate of this bug. ***
This is totally infuriating and needs to be fixed ASAP! Sure it isn't security,
but it's such a fundamental behavior of all GUIs that it surely will make the
list of "reasons Firefox is not ready for prime time."

Strangely, I don't think I have this problem with the Download dialog; is there
some reason they should work differently??
Attached patch Potential Fix. (obsolete) — — Splinter Review 
Dan, what do you think?  Maybe we should throw a dialog if this happens?  Right
now we just silently fail which might be okay since this case isn't going to be
hit very often.
Assignee: file-handling → dougt
Flags: blocking1.7.8?
Flags: blocking-aviary1.0.4?
*** Bug 289739 has been marked as a duplicate of this bug. ***
I hate to just add to the noise complaining about this bug, because I can't
really offer a programming solution, but since the severity is still only listed
as normal, I think perhaps another complaint is not out of order.  It seems like
poor judgement was excercised in fixing what seemed to be a small minor
potential security issue by seriously disabling a GUI feature that every other
program written manages to use without problems.  I can only vote for this bug
once, but I just want to make sure the powers that be who assign priority to
these things are paying attention.  This is an absolute make or break issue for
me and I'm sure for a lot of people.  I will be reinstalling Thunderbird 1.0
because this bug is such a major inconvenience, and if I didn't still have the
old installer, I'd switch to any other email program, OE included, without
hesitation.  

Okay...I'm done ranting.  Thanks to everyone who actually contributes code.  I
was really happy with both Firefox and Thunderbird before this bug cropped up.  
Comment on attachment 179887 [details] [diff] [review]
Potential Fix.

You should check that length > 4 and do a case-insensitive comparison. Could
you also use wchar_t instead of unsigned short? The idea is excellent and the
current situation is unbearable.
Just a user here, cant offer much expertise, but I do have a strange factoid to
report. Mostly I use 2 computers, one with WinXP home and WinXp Pro on the other
(FF 1.0.2- showing this bug). Also, I have an Win2K machine that is used mostly
as redundant storage. I am not big on Betas, but because this mis-treatment of
links is so irritating, I went to the W2K box and desperately tried one of the
FF 1.0.3 candidates I found pointers to on the Mozillazine forum (cant find the
link right now, no matter). Anyway, I tried it for a few hours, and THE .LNK
FLAW WAS FIXED! Nothing else seemed broken, so I installed the same FF 1.0.3 on
an XP machine--THE .LNK FLAW WAS BACK! 

Now curious, I put FF 1.0.2 on an NT machine. It has the LNK flaw, sort-of. It
gives no warning, just inserts the clicked folder link in the filename field.
But if you follow through, (open or save, I forget which) it does not save the
document with "xxx.lnk" as the file name, instead, it opens the "xxx" folder as
the new save-location! IE 6 has the same behavior as FF in the NT enviroment.

In WinXP if the overwrite warning is ignored, the contents of "xxx.lnk" is
replaced by the file being saved. So, here I have 3 different folder-shortcut
treatments in in 3 different windows versions, with the same FF.

Hate to be long winded, but this behavior is hugely irritating, just thought
that the Windows-version-dependent results might help track down the cause.
UNCA MICK comments about a new behavior of this bug. I have another program 
that has this new behavior. It is ImageFox from ACD Systems. This program has 
a "favourite folders" feature. It adds several new buttons to the open/save 
windows. When you click the favourites button, it takes you to a folder managed 
by ImageFox that contains links to other folders, so you can go directly to the 
folder you want. The bug is when you click on this button, you loose the 
filename you're trying to save and it gets replaced by the location of this 
favorite folder. This happens with ANY program and in both the open and save 
windows (but it really has no relevance in the open windows). the curious thing 
about this is that if you are using, lets say, Photoshop and you click on a 
link at the save window, it takes you to the refered folder as expected, but if 
you click on the ImageFox favourites button, the described bad behavior 
happens. So, the point to note here is that a feature of this add-in, when 
activated, causes a correctly working open/save window to behave incorrectly. 
It only happens in Windows XP (Both Home and Pro) so I think it could be 
somewhat related with the bug we're discussing here and so give any clue about 
its solution.
(In reply to comment #50)
The bug is still in Firefox 1.03 beta 
The bug will be in every version of Firefox henceforth until this bug is marked
as fixed. There is no reason to waste time downloading new builds just to see if
they secretly fixed it.
I got this same problem when I "upgraded" to 1.0.1 and then 1.0.2, using Win XP sp1
- really rather annoying since it (still) works as intended in IE -

In my book this BREAKS a major functionality in Firefox (saving/downloading
pages). So, no more recommending it to clients or coworkers for a while.
I experienced the bug using Mozilla 1.7.6 it's not in 1.7.5 so I revereted back
to that build.
*** Bug 289900 has been marked as a duplicate of this bug. ***
What's the status on this bug? It seems pretty major to me... must block for 1.1!
*** Bug 290447 has been marked as a duplicate of this bug. ***
I can not re-create this issue in Firefox 1.0.3 on windows 2000 SP 4.  It
traverses all symlinks perfectly fine. and as far as I cam remember it always
has traversed symlinks as I always have a set of symlinks on the desktop going
to network shares (mapped and non-mapped) and vm host-guest paths on the system.
(Vmware install of win2k).

Could this be a Windows XP specific bug?
Are you talking about "junctions"? They are the only thing that could be
considered "symlinks" on Windows 2000. WIndows XP has hard links, but I don't
believe these exist in WIndows 2000. Junctions and hard links will always work
perfectly fine because they are at the file system level. Shortcuts are simply
files that contain a path in them. When you double-click a shortcut, Windows
goes to the path that is listed in the LNK file. 
I can verify that 1.0.3 still has the bug.  I agree with Caleb that it is quite
major - breaking a fundamental ability to navigate in Windows.  

As Jerry Baker says, Windows shortcuts are files with type .lnk and a path
content - they are not fs-internal unix-style symbolic links, or Microsoft
network shares.

Since 1.01, Firefox has decided to write over these rather than traverse them. 
From some very confused-sounding talk on the related bug report, some people
think that this is a smart way to work around a very obscure security issue
which actually requires that you write over links.  

Please, please fix it - the way it is makes it very awkward to save web pages to
different places in a knowledge base.  It also makes Firefox look like it
doesn't work in Windows to anyone who is used to saving webpages.
Attached file Firefox 1.0.3 patch (obsolete) — 
This patch will change the second hax entry 10 00 8D 85 to 00 00 8D 85 and so i
hope it will fix this bug
What do i do with those attachments in order to install them?
Will this patch work for Mozilla 1.7.6? Because the bug is there too.
Will the patch work with 1.0.2 spanish version? There isn't still a spanish 
1.0.3 version. And, in case the spanish version 1.0.3 is released, would the 
patch work with it or is just for the english version?
Comment on attachment 180936 [details]
Firefox 1.0.3 patch 

this is a binary patch, not a code patch, so correcting type and flag
Attachment #180936 - Attachment is patch: false
Attachment #180936 - Attachment mime type: text/plain → application/octet-stream
This patch worked fine for me, thanks a lot, bigdady
What does the patch actually *do*?  Do you have source code?  Does it just undo
the fix for bug 271732?
Attachment #180936 - Attachment description: This patch will fix the bug (i hope) → Firefox 1.0.3 patch
Attachment #180936 - Attachment is patch: true
Attachment #180936 - Attachment mime type: application/octet-stream → text/plain
Attachment #180936 - Attachment is patch: false
Attachment #180936 - Attachment mime type: text/plain → application/octet-stream
(In reply to comment #67)

This patch does a little bit haxing, like i said it changes the second hax entry
10 00 8D 85 to 00 00 8D 85. If you want to know more 
http://forums.mozillazine.org/viewtopic.php?p=1288710#1288710



(In reply to comment #68)
> (In reply to comment #67)
> 
> This patch does a little bit haxing, like i said it changes the second hax entry
> 10 00 8D 85 to 00 00 8D 85. If you want to know more 
> http://forums.mozillazine.org/viewtopic.php?p=1288710#1288710

Based on my interpretation of the post in the thread, your patch simply
reintroduces the security hole.  Is that correct?
(In reply to comment #69)

Truly I don't know, I just made a patch using the manual in the forum so...
I belive that it doesn't reintroduce the security hole, but ain't sure.
If you view the fact that files can be overwritten by clicking on a dialog box
that asks you if you want to overwrite a file a security hole, then yes, this
patch reintroduces the "you can overwrite a file by telling the computer you
want to overwrite the file" security bug.
(In reply to comment #60)
> I can verify that 1.0.3 still has the bug.  I agree with Caleb that it is quite
> major - breaking a fundamental ability to navigate in Windows.  

How can such an obvious bug - which seems to require a simple change to a flag
to fix it - be permitted to propagate through 3 versions of Firefox (1.0.1,
1.0.2, 1.0.3)? This is starting to make Internet Explorer look good by
comparison. Could we at least see the status move from "new" to "assigned"??

This is a bug that EVERY Windows user will hit if they use shortcuts to navigate
their directory trees. It needs to be fixed - and it is not just a cosmetic bug.  
Agreed.  Can't we change the severity to Major or Critial?  This should have
been fixed long ago.
Attached patch patch v.2 (obsolete) — — Splinter Review 
including ere's comments.
Attachment #179887 - Attachment is obsolete: true
(In reply to comment #74)
> Created an attachment (id=181139) [edit]
> patch v.2
> 
> including ere's comments.

A: Does this patch ("patch v.2") fix the regression so it won't be in 1.0.4 (or
even currenly in nightlies) ??

B: Is the .exe ("Firefox 1.0.3") patch safe to run?

C: what does "ere's" mean?

D: Is there really a point to making patches for this regression, or should the
people who "fixed" Bug 271732 just make a less sloppy bugfix for their "bug"?
(In reply to comment #75)
> C: what does "ere's" mean?

It's the genitive case of "Ere"

> D: Is there really a point to making patches for this regression, or should the
> people who "fixed" Bug 271732 just make a less sloppy bugfix for their "bug"?

I would really advise you to compare the authors of the patches for this bug and
that bug.
Comment on attachment 180936 [details]
Firefox 1.0.3 patch 

Obsoleting so as not to imply that downloading and running random executables
from bugzilla attachments is a good idea.
Attachment #180936 - Attachment is obsolete: true
(In reply to comment #74)
> Created an attachment (id=181139) [edit]
> patch v.2
> 
> including ere's comments.

Hmm, Doug you did not seem to use any review flags on the patch ....
(In reply to comment #76)
> It's the genitive case of "Ere"
then  what is an / who is  "ere"?
(http://www.google.com/search?q=define:ere wasn't much help)


> I would really advise you to compare the authors 
> of the patches for this bug and that bug.

Well, then my question still stands -- shouldn't the other bug's patch be
updated so that this regression is fixed, AND the other bug gets fixed?

It seem (to me) that this bug is an error in the implementation of the other
bug's patch, therefore the fix should occur on that bug's patch instead.  In
fact, this bug (regression) is probably invalid if you look at it that way.  We
really just want a more sophisticated solution to the other bug, which still
allows users to use their .lnk shortcuts which point to folders.
(In reply to comment #79)
> (In reply to comment #76)
> > It's the genitive case of "Ere"
> then  what is an / who is  "ere"?
> (http://www.google.com/search?q=define:ere wasn't much help)
See comment 47.

I can't believe the noise in this bug.  Please read the entire thing before
commenting!
(In reply to comment #81)
> I can't believe the noise in this bug.  Please read the entire thing before
> commenting!

I did.
On second though, my answer of "I did." is not complete enough.

I brought up some really good points to think about.

It is apparent that this bug is a big deal, and it's unbelievable to me that it
even exists.

I asked my questions (A-D) in order to be able to more fully explain this bug
over in the mozillazine forums.  I wanted clarification on things that confused
me, because I figured that the other 80 people who want this bug fixed would
appreciate the clarification.  

I build websites -- that's *my* interest in Mozilla.  I do not code, so I cannot
offer that -- what I can offer is to increase communication.


Writing my questions and clarifications off as "noise" I didn't initially, but
now do take as an insult.  While I appreciate your effort in coding on this
project, your comment I found to be quite rude.  I understand the problem of
"bugspam", and write every comment focused on the question "is this post even
worth reading?" -- I apologize if *you* found them to have zero value.  

The implementation of the other bug's fix *was not* completely thought out (as
is apparent by the results of the patch).  Adding more *thinking* to this bug in
in order.
(In reply to comment #79)
> It seem (to me) that this bug is an error in the implementation of the other
> bug's patch, therefore the fix should occur on that bug's patch instead.  In
> fact, this bug (regression) is probably invalid if you look at it that way.

This is normally how we handle regressions: leave the old bug closed and track
the regression separately. The exception is when we immediately back out the
original fix, but that's not what we've done here.
Comment on attachment 181139 [details] [diff] [review]
patch v.2

r=dveditz
Attachment #181139 - Flags: review+
(In reply to comment #83)
> On second though, my answer of "I did." is not complete enough.

And apparently inaccurate since you didn't notice Ere's comment, even when you
had a question about "including Ere's comments" which should have prompted you
to look for them.

> I brought up some really good points to think about.

Not really.

A: yes, obviously we think it fixes the regression. That's why it's a patch. 

B: Of course it's not "safe", it's a binary executable from some random bugzilla
commenter. Only you know what your risk tolerance is, and judging the poster's
reliability is a personal judgement.

C: failure to read in context noted above

D: Does it matter *where* the patches are attached? Would it even make sense to
do it elsewhere since there are 100 people watching for the fix *here*?

> Writing my questions and clarifications off as "noise" [...]
> I found to be quite rude.

Your comments were neither about the problem nor the patch, they were procedural
nitpicks at best. And, frankly, you set the tone by insulting the programmer.

> Adding more *thinking* to this bug is in order.

Would love to see some.
(In reply to comment #86)
> D: Does it matter *where* the patches are attached? Would it even make sense to
> do it elsewhere since there are 100 people watching for the fix *here*?

No, where it is attached doesn't matter. The development philosophy behind
checking things in that break other things and then chasing down all the
symptoms rather than addressing the single root cause is questionable at best.
Why not just test the checkin first? If it's broke, fix it first.
 
> nitpicks at best. And, frankly, you set the tone by insulting the programmer.

If saying that a patch was not well thought out is insulting, I would suggest
that the best way to avoid such insult is not to check in patches that aren't
well thought out.
this use case was considered when that patch was created, see bug 271732 comment 34
Assignee: dougt → nobody
Comment on attachment 181139 [details] [diff] [review]
patch v.2

Ere, I made the changes that you requested.  


I think this patch is good-to-go.  Note that if you do get into the case of
download a shortcut twice, the second time will fail without any user feedback.
 I think this is okay since (a) this case is unlikely, (b) we prevent the
serious security hole cited above, and (c) getting a UI change now is going to
be difficult.
Attachment #181139 - Flags: superreview?(emaijala)
(In reply to comment #86)
> (In reply to comment #83)
> > On second though, my answer of "I did." is not complete enough.
> 
> ... you didn't notice Ere's comment, even when you
> had a question about "including Ere's comments" which should have prompted you
> to look for them.

my response (and all future ones) in the mozillazine thread
(In reply to comment #88)
> this use case was considered when that patch was created, see bug 271732
comment 34

Yes it was, and then it was dismissed with the comment, "You can still
double-click on shortcuts to folders in the dialog" (bug 271732 comment 35).
After that, nobody bothered testing it, or if they did they did not post the
results. That's what is disconcerting about this whole thing: checkins can be
made that break the fundamental features of an OS and not only is it not backed
out, but it takes a month of bickering to even have it addressed at all. There
*really* needs to be a prohibitions on platform-specific checkins by people who
do not use that platform as their primary work environment, because they do not
seem to understand the ramifications.
jerry, the position was that it was better to break some functionality to
protect the user against a security hole.  We fixed it quick to make the
release, and now we are looking at better solutions.  
That what is represented in that bug is even a security hole is up for debate. I
have been seriously contemplating filing another security bug that it is
possible to browse to any given folder and overwrite another file simply by
answering OK to one dialog asking you if you really want to overwrite that file.
... even files in the system folder :::GASP:::. I don't see how having to OK a
dialog asking you to overwrite a file represents any sort of security hole. I
think it represents a hole in something, but not security.
jerry, that is a nice thought and all, but it really doesn't help this bug, does
it?  
I was making the point that none of what anyone says was addressed was actually
addressed. It was mentioned and then summarily ignored, but not addressed. The
fix that caused this regression is a fix for a non-issue in the first place
(that was the point of my facetious bug about being able to overwrite any file).
I can't think of any place more relevant to point that out than in a bug caused
by the fallout from that fix.
Just to clarify -- the security hole which we thought was important enough to
break this function was that you can construct a file such that if you download
it twice in the same place, the second download could replace _any_ file on your
system.  The confirmation dialog would suggest that you were replacing one file,
but in fact, it would relace another.

The way this bug was reported sounds like a regression cause by our fix for the
security hole.  I found that there was an inconsistent way that windows handled
OFN_NODEREFERENCELINKS.  

I hope this is clear.  if you would like to talk about bug fixing philosophy,
security bug considerations, or anything that doesn't directly influence this
patch, please move your discussion to the mozillaZine formus or to the public
newsgroups.

Regards,
Doug

You are incorrect. The dialog specified the file that is *actually* replaced.
The whole thing is a non-issue. And yes, I'm well aware of mozilla.org's
position that you should take relevant discussions somewhere where the
developers and drivers can more easily ignore it.
No Jerry.... you are incorrect.  The security hole specied one file and replace
aonther. 

Please do not lead yourself to believe that developers don't want feedback...
They want feedback, just not incorrect, loud, or inconsiderate feedback.  
Is the patch correctly?
I think that |ofn.lpstrFile| has reference path of shortcut file. It doesn't
have shortcut file path. So, I think that |ofn.lpstrFile| doesn't have ".lnk",
".pif" and ".url" always.
*** Bug 291579 has been marked as a duplicate of this bug. ***
Comment on attachment 181139 [details] [diff] [review]
patch v.2

I'm not a super-reviewer, but let's just pretend dveditz did this and vice
versa.
Attachment #181139 - Flags: superreview?(emaijala) → superreview+
Masayuki, the problem we are trying to fix is that if a user overwrites an
existing shortcut, the overwrite will incorrectly dereference the link and
destroy the target of that shortcut.  In this case, |ofn.lpstrFile| will have
one of the extensions we are looking for.  Please confirm.
(In reply to comment #102)
> Masayuki, the problem we are trying to fix is that if a user overwrites an
> existing shortcut, the overwrite will incorrectly dereference the link and
> destroy the target of that shortcut.  In this case, |ofn.lpstrFile| will have
> one of the extensions we are looking for.  Please confirm.

That is incorrect. If you select an existing shortcut, ofn.lpstrFile will have
the path of the referenced file (not the shortcut). However, this patch will
prevent the user from creating the shortcut in the first place (when the
shortcut doesn't already exist).

yoni, go read the security bug and attempt to reproduce it.
Jerry -- Dan Veditz confirmed what you are seeing, so maybe we both are correct.
 He used XP.  I think that an issue is that the API is inconsistent between XP
and other system such as SE.  
If an website shows following step, we cannot fix bug 271732.

1. Download an zip file that is include shortcut file(e.g., "foo.lnk").
2. Unzip the zip file.
3. The zip file has old version file. Please download "foo.lnk" and overwrite it.

Current patch cannot stop to download both shortcut file.
That is correct. You cannot write anything in C, C++, JavaScript, or any other
code that I am aware of that will alter the behavior of people. There are just
some things that are beyond the control of the programmer.
(Partially in reply to comment #99)

I have verified that “patch v.2” is NOT correct.  On Windows XP, when the user
selects a link to a file in the file save dialog, the windows returns the file
pointed to by the link.  There is no way for the caller  to determine that the
file was selected by selecting a link, and not the original file.  Therefore,
the code in the patch designed to catch this will never trigger.

While I do see the potential danger of the original “security hole”, I firmly
believe that it is time to write it off, and simply return to the old behavior.
 This “security hole” exists in every windows application (including IE).  There
is no reasonable solution (One could, I suppose, do some really hairy stuff,
involving overriding the standard windows Save dialog, but even then the
solution would be partial at best, and very bug prone).

Not being able to use links for navigation (as it stands now) is clearly
unacceptable.  Speaking as a professional developer with over 10 years windows
programming experience, I can tell you that this ‘small’ issue will eventually
lead to significant numbers of users abandoning this software.

Remove the OFN_NODEREFERENCELINKS flag now.

dan -- your call.
Assignee: nobody → dveditz
We have evangelism for Web pages, but is there evangelism for operating systems?
That seems to me to be the appropriate place for the bug that caused this
regression.
If the patch doesn't work for you, open firefox.exe with a hex editor (for
example http://www.chmaas.handshake.de/delphi/freeware/xvi32/xvi32.htm ) and
search for "4D 80 00 80 10 00 8D 85", then change it to "4D 80 00 80 00 00 8D 85".
It's not me who found that out, i got it from
http://forums.mozillazine.org/viewtopic.php?p=1387539#1387539

I have no idea how to publish patches here, so all i can do is write this.
Is it feasible to compare the directory that is currently open in the "Save As
..." dialog with the path returned by the dialog? That way, if they do not
match, we could refuse to write. At least this would cover all cases except for
where the file is in the same directory as the one in which you are saving, but
then that's not much different from directly overwriting the file.
it could just be the case that we claim this inconsistency is a windows bug and
security holes based on this bug are Microsoft's fault.  Afterall, if I
overwrite a shortcut twice, i do not expect the destination/target to ever be
touched.
If it's relatively easy to check the current directory and compare it with the
path of the filename we are trying to save, I think it would be an ideal
solution. I can't think of a case where you would be saving a file to a
different directory than the currently displayed directory EXCEPT when the
security hole is in effect. The one caveat would be to make sure that the
comparison and PASS FAIL does not occur until the Save button is pressed, or
else shortcut navigation might break.
Just make sure I can type in a path into the save as dialog and hit save without
having to navigate to that directory...
I have to say that I had never thought of doing that, nor have I seen it done in
all my years in IT. It always seemed much faster to do two clicks to the root
drive than it was to even type C:\. I suspect that what you want to do would not
work were my suggestion to be implemented.
So, if you don't mind being a bit hackish, ignore the check if the filename box
contains a backslash. It's an invalid character in filenames on Windows, and so
it either has to be a directory or an invalid filename. In the former case, we
can bypass the check because I don't think the LNK/PIF file thing will prefill
the filename box. The latter case would be handled by whatever error checking
Windows usually does on the filename.
What I had in mind is something like:

if(TARGET_DIR != CURRENT_DIR)
  {
    if(strstr(TEXTBOX_VALUE,"\")
      /* Save */
    else
      /* Fail */
  }

else
  /* Save */
  
A lot of speculation, but little actual information.

(In reply to comment #112) and (In reply to comment #119) et al.

Their strategy is to compare the current directory (once the save dialog has
closed), to the file path of the file selected.  This will not work.  The
directory is set to the folder containing the item pointed to the link, not the
folder the user was in when they selected the link.  I have verified this with a
test program (on Windows XP).

On the other hand, comment #113 is on the right path.  IMHO the only way to do
this will be to install a hook in the SaveDialog, then it is relatively
straightforward to catch the CDN_FILEOK message.  At that point CDM_GETSPEC will
return the actual name of the file that the user has selected.  This will be the
link, and the extension can be checked for “.lnk” etc, as in patch v.2. Note
that this all takes place in the hook procedure.  I have verified that this
works correctly with a small test program (again on Windows XP.)

To implement this will require a reasonably experienced Windows API developer,
and a lot of care.  It is more than just a 3 line fix.  This will also need to
be checked extensively on all Windows platforms, since this is one area (File
dialog hooks and overrides) that can vary between versions of Windows.(I am
unfortunately, not able to take on this task at this time.)

However, I stick by my original statement.  The “security hole” exists in every
Windows application that I know of.  If no-one can be found to fix this reliably
and immediately, I think the correct course would be to simply remove the
OFN_NODEREFERENCELINKS and live with the minor  “security hole”  for now.  You
will loose users over not being able to follow links.  

*** Bug 292277 has been marked as a duplicate of this bug. ***
Oh my god this problem is driving me crazy!  This WASN't happening in Mozilla
1.7 and now I'm tempted to revert back just so this failure of .lnk resolution
doesn't continue to make me want to kill myself.
Supporting comment #120: It isn't necessary to make Mozilla/Firefox the only
application "fixing" this Windows bug and pay for this with a very annoying loss
of usability.

In reply to comment #116:

Of course it is possible to enter a qualified file name, including the path; I
just tried it. But this is not the same, since it requires a lot of work of the
user [s]he tried to get rid off creating the symlink.

I can't believe this bug is still marked as NEW. Please fix this bug for the
next release. Whoever feels the security threat as so important that fixing it
is worth a big usability loss, could decide so out of his own free will by
setting a non-default setting; but in my opinion, it would be sufficient to
return to the pre-"fix" state.
Ok, so what if the "fix" is coded in but such that it requires a setting in the
prefs.js to be entered by hand ?  That way the "security issue" is "fixed" out
of the box by default, but a quick hand edit of the prefs.js could allow that to
be changed.  I think that sounds like the best of both worlds.  

I shouldn't have to fix the browser after I install it.
How about this:

Check the default file name extension and if it's .lnk, .url or .pif, check that
the file name the save dialog returned has the same extension. If not, and a
file with that name already exists, warn the user or fail.

In pseudocode: 
if (extensionOf(mDefault) == '.pif' || '.url' || '.pif') 
&& (extensionOf(ofn.lpstrFile) != extensionOf(mDefault))
&& (fileExists(ofn.lpstrFile))
  return NS_ERROR_FAILURE;
(In reply to comment #125)
> I shouldn't have to fix the browser after I install it.

You'd rather it stay broken with no option to fix?  This bug has persisted
through two version upgrades.  They're not just going to revert to the original
"unsecure" handling of links from earlier versions.  If an optional setting that
isn't selected by default is considered safer simply because one has to be aware
enough of what it does to enable it and therefore more palateable, then by all
means I'm all for it.  As I understand it, the original security flaw isn't a
problem if you know not to download .pif files.  
I think Ian meant that it should be fixed in a way that doesn't require changing
a pref.

I like the suggestion in comment 126. It's limited to PIF, LNK, and URL files,
and it is the most narrow way I can think of at the moment to stop the security
bug while allowing the usefulness of shortcuts.
How about just a "Download File Blacklist" where you'd get a warning every time
you try to download files of certain types (.lnk, .pif, ect.).  

It could have certain files in the list by default, and users could add or
remove them if they want.
Attached patch Alternate patch v1 (obsolete) — — Splinter Review 
Dan, what do you think about this approach?
Attachment #182604 - Flags: review?(dveditz)
You need to put the warning string into a DTD file so that it can be translated
to the various languages Mozilla/Firefox is released under. I'm not sure which
is the correct DTD file for errors from the file handler.
I know, but this is sort of a minimal patch. Translatability can be added later.
I agree the Ere's approach.
I think that the security hole is opened again by the patch.
But the hole is very small.

If we can get last opened folder in save dialog, we can *close* the hole...
# the folder and the result path is not equal,
# the user selected the shortcut file.
*** Bug 292846 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
Attachment #182604 - Flags: superreview?(dveditz)
Attachment #182604 - Flags: review?(dveditz)
Attachment #182604 - Flags: review?(dougt)
I send out a mail to the security list and no one seam to object about backing
out the fix completely.  This was the text of the email:

*271732* COMMAND.COM is overwritten by downloading the pif file

We are considering backing out a patch that prevented the arbitrary overwriting
of any file on the users systems.  An attack would require that you "Save-As" a
shortcut file (.lnk, .pif, .url, etc.) to the same place twice.  The first file
saved will go to the expected location.  However, the second time you "Save-As"
with the expectation to replace the file, Windows dereferences the shortcut. 
Using this method, an attack can either destroy existing files or add new files
which will execute (saving to the Startup directory).

The reason for this is the following:

Internet Explorer, and all of windows, suffers from this vulnerability.  In IE 6
Running Windows XP SP2, Dan showed that a similar attack was possible.

Significant usability issues.  The fix was to disable shortcut dereferecing in
the save-as dialogs.


If Ere has a real fix that is good.

Well, I wouldn't have created that patch if I didn't believe it's good, but I
don't oppose to backing out the original patch either. Whatever the decision, it
would be about time to fix this issue in a way or another.
Ere's approach could fix the security hole while keeping the browser usable. 
That would be much better than backing out the original patch and leaving the
browser vulnerable.

Ere, does your patch prevent the attack from being used to overwrite
"C:\Documents and Settings\All Users\Start
Menu\Programs\Accessories\WordPad.lnk" or create a file in the Startup
directory?  I'm not familiar with how this code works, but the comment in the
patch makes it sound like the patch only prevents files with non-shortcut
extensions from being overwritten.
I made it so that you can overwrite a .lnk with another .lnk, but I can easily
change that too if desired.
Attached file Firefox 1.0.4 Patch (obsolete) — 
This patch will change the second (hax) entry 10 00 8D 85 in to 00 00 8D 85 in
file firefox.exe
Attachment #183364 - Attachment description: A patch to alter the "Save As" bug in the most recent version of Firefox → Firefox 1.0.4 Patch
Still broken in Firefox 1.0.4.
(In reply to comment #140)
> Still broken in Firefox 1.0.4.

Did anyone say it was fixed? "Generalissimo Francisco Franco is still dead."
No, but the latest patch here does appear to fix 1.0.4. I don't know whether it
reopens the security "hole" or not but at least the expected save as behaviour
is back. 
Comment on attachment 183364 [details]
Firefox 1.0.4 Patch

Obsoleting per comment #77
Attachment #183364 - Attachment is obsolete: true
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Flags: blocking1.7.8?
*** Bug 293536 has been marked as a duplicate of this bug. ***
I hope this isn't noise, or too redundant:

All we want is to be able to  'click-thru' shortcuts to folders. I have not seen
a comment from anyone that said they wanted to dereference any other type of
shortcut during a SAVE.

Countless other programs allow this in their Save dialogs. I dont understand why
the fix can't just allow only dereferencing of folder shortcuts, but not file
shortcuts.
Because Windows doesn't offer such a choice. You either dereference or you don't
-- the target type doesn't matter.
Since shortcut files (.lnk) are usually only meaninful on a local machine (and
I've never heard of anyone downloading one) why don't we just make them part of
a list of files Mozilla/Firefox will not download?

If someone needs to work-around it they could just zip up the shortcut for
people to download.

This is a simple and effective solution. (hopefully)
(In reply to comment #147)
> I've never heard of anyone downloading [a .lnk file])

Yeah, I didn't either. Funny idea...

I didn't hear about .pif files ("program information files") since Windows for
Workgroups, long ago (anyone left using this dinosaur? ... and if [s]he did,
would [s]he try to save a .pif file? And if [s]he did, would [s]he try to save
the same .pif file twice, without having a look at the first downloaded
version?! And if [s]he did, would [s]he do it using Mozilla? And if [s]he did,
would [s]he blame Mozilla for his/her own stupidity? And if [s]he did: Would it
bother the oak if a hog would scratch it's back? And would, finally, this very
single hog outweight the miriads of users who wonder why Mozilla doesn't behave
like any sane application, disallowing the user to follow his/her own soft links?!)

Nobody offers a .url file for download; everyone would wonder, "why would anyone
offer such a file for download; everyone would create a bookmark/'favorite' for
an ordinary link if [s]he needed it".

Thus, IF someone thinks it be necessary to take security precautions: make a
confirmation dialog pop up whenever someone tries to download a .lnk, .pif or
.url file to a Windows system. Or simply remove the default file name for those,
like suggested in comment #38. But, please: allow users to use their soft links,
and make this VERY ANNOYING BUG *block* the next non-security release.
(In reply to comment #148)
> (In reply to comment #147)
> > I've never heard of anyone downloading [a .lnk file])
> 
> Yeah, I didn't either. Funny idea...
> 
> I didn't hear about .pif files ("program information files") since Windows for
> Workgroups, long ago (anyone left using this dinosaur? ... and if [s]he did,
> would [s]he try to save a .pif file? And if [s]he did, would [s]he try to save
> the same .pif file twice, without having a look at the first downloaded
> version?! And if [s]he did, would [s]he do it using Mozilla? And if [s]he did,
> would [s]he blame Mozilla for his/her own stupidity? And if [s]he did: Would it
> bother the oak if a hog would scratch it's back? And would, finally, this very
> single hog outweight the miriads of users who wonder why Mozilla doesn't behave
> like any sane application, disallowing the user to follow his/her own soft
links?!)
> 
> Nobody offers a .url file for download; everyone would wonder, "why would anyone
> offer such a file for download; everyone would create a bookmark/'favorite' for
> an ordinary link if [s]he needed it".
> 
> Thus, IF someone thinks it be necessary to take security precautions: make a
> confirmation dialog pop up whenever someone tries to download a .lnk, .pif or
> .url file to a Windows system. Or simply remove the default file name for those,
> like suggested in comment #38. But, please: allow users to use their soft links,
> and make this VERY ANNOYING BUG *block* the next non-security release.

YES! GREAT comment! :-] *applause*

I vote for comment #38.
Please fix it at least for Firefox 1.1.
*** Bug 294345 has been marked as a duplicate of this bug. ***
Comment on attachment 182604 [details] [diff] [review]
Alternate patch v1

>+                ::MessageBox(ofn.hwndOwner, 
>+                  "Cannot overwrite a file of another type with a shortcut file.", "Error", MB_ICONERROR);

The error string needs to be localizable.

>       MessageBox(ofn.hwndOwner,
>-                 0,
>                  "The filepicker was unexpectedly closed by Windows.",
>+                 "Error",
>                  MB_ICONERROR);

Or not? Is there a lot of this kind of thing in widget-land?

This doesn't cover the hypothetical case where the first .lnk references a file
that *doesn't* exist in order to put a file into a startup location. Hm..
Do we really want to go in the direction of attempting to anticipate and code
for every possible permutation of known Windows API security issues? I don't
think it's possible. I think we're faced with an old-fashioned trade off here:
we either break functionality, or allow the security "issue" to occur to some
extent.
(In reply to comment #138)
> I made it so that you can overwrite a .lnk with another .lnk, but I can easily
> change that too if desired. 

Please do.
No longer blocks: majorbugs
*** Bug 295967 has been marked as a duplicate of this bug. ***
(In reply to comment #148)
> (In reply to comment #147)
> > I've never heard of anyone downloading [a .lnk file])
> 
> Yeah, I didn't either. Funny idea...
> 
> I didn't hear about .pif files ("program information files") since Windows for
> Workgroups, long ago (anyone left using this dinosaur? ... and if [s]he did,
> would [s]he try to save a .pif file? And if [s]he did, would [s]he try to save
> the same .pif file twice, without having a look at the first downloaded
> version?! And if [s]he did, would [s]he do it using Mozilla? And if [s]he did,
> would [s]he blame Mozilla for his/her own stupidity? And if [s]he did: Would it
> bother the oak if a hog would scratch it's back? And would, finally, this very
> single hog outweight the miriads of users who wonder why Mozilla doesn't behave
> like any sane application, disallowing the user to follow his/her own soft
links?!)
> 
> Nobody offers a .url file for download; everyone would wonder, "why would anyone
> offer such a file for download; everyone would create a bookmark/'favorite' for
> an ordinary link if [s]he needed it".
> 
> Thus, IF someone thinks it be necessary to take security precautions: make a
> confirmation dialog pop up whenever someone tries to download a .lnk, .pif or
> .url file to a Windows system. Or simply remove the default file name for those,
> like suggested in comment #38. But, please: allow users to use their soft links,
> and make this VERY ANNOYING BUG *block* the next non-security release.

I agree but do not have the skills to fix it. PLEASE fix this. It will make my
life, and others much better!
the last patch for firefox 1.04 seems to work perfect!
the last patch for firefox 1.04 seems to work perfect!
Phew, that was one annoying bug/security fix. Last patch works for me.
I cannot understand why Firefox was deployed with such an annoying limitation.

Thanks!
Rufer
Comment on attachment 182604 [details] [diff] [review]
Alternate patch v1

in comment 152, dan mentions that this patch doesn't solve a case where the
first lnk file can be pointing at a non-existent file.	We need to solve that
case as well if we hope to address this hole.

+	       nsCOMPtr<nsILocalFile>
file(do_GetService(NS_LOCAL_FILE_CONTRACTID));

localfile's are objects, and isn't a service.  we need to use createInstance
here.

Doug
Attachment #182604 - Flags: review?(dougt) → review-
I'm starting to lean to just backing out bug 271732 and calling it a windows
problem.
Flags: blocking1.8b3?
Flags: blocking1.8b3+
Flags: blocking1.7.9+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.5?
Flags: blocking-aviary1.0.5+
(In reply to comment #160)
> +	       nsCOMPtr<nsILocalFile>
> file(do_GetService(NS_LOCAL_FILE_CONTRACTID));

D'oh. I'm fairly sure I just copied that from somewhere else.. but might have
been my mistake too. Anyway, I'm all for backing out the whole thing.
dougt:  Do you have a fix for this or is dveditz's idea a better approach for
this?  Should we backout that other change?
*** Bug 297050 has been marked as a duplicate of this bug. ***
The problem is that the shortcut is not <b>followed</b>. The shortcut should be
followed not an overwrite warning shown.
The actual problem is that *if* the shortcut was followed, Firefox *might*
overwrite a file if you download a shortcut file and save it as another shortcut
file. Which is a windows bug, not a Firefox bug.
Has anyone from mozilla.org tried contacting Microsoft to see about working with
them on resolving this bug, or at least to notify them of the bug?
Dan, your final verdict?
Attached patch Backout patch — — Splinter Review 
We're going to back out bug 271732 and leave the problem up to MS as per Dan's
decision.
Assignee: dveditz → emaijala
Attachment #181139 - Attachment is obsolete: true
Attachment #182604 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186504 - Flags: superreview?(dveditz)
Attachment #186504 - Flags: review?(dveditz)
Attachment #186504 - Flags: approval1.8b3?
dveditz: let's get the reviews here and give the a= for the branches.
Dan asked me my thoughts on backing this out.  I suggested that we just add a
preference such that if you needed this behavior, you could just set a pref. 
This would fix majority of advanced users as they would have a way to enable
this functionality.
(In reply to comment #171)
> Dan asked me my thoughts on backing this out.  I suggested that we just add a
> preference such that if you needed this behavior, you could just set a pref. 
> This would fix majority of advanced users as they would have a way to enable
> this functionality.

That's unacceptable... this is not "advanced user" functionality... this is
basic Windows functionality that should be (and has been in the past) enabled by
default!
*** Bug 297964 has been marked as a duplicate of this bug. ***
*** Bug 291842 has been marked as a duplicate of this bug. ***
(In reply to comment #172)
> That's unacceptable... this is not "advanced user" functionality... this is
> basic Windows functionality that should be (and has been in the past) enabled by
> default!

I strongly agree that links should be followed by default. Otherwise this would
continue to bite legions of users who might or might not find out about the
preference.

How about this: Whenever someone attempts to save something totally insane (like
a .lnk, .url or .pif file), pop up a confirmation window which informs about the
"security threat", tells about the precautions to take (don't save something to
the same location twice Without Knowing Exactly What You Are Doing). Whoever
happens to need to frequently save files with such names might suppress this
confirmation by enabling a non-default switch, maybe using a "don't show this
message again" checkbox.
Attachment #182604 - Flags: superreview?(dveditz)
Shortcuts are not just for advanced folks and they need to work right out of
the box without any tweaking of settings (which I would consider much more
advanced). Besides, I don't want to have a setting that would hinder the
usability so much when there are multiple viable alternatives. 

Anyway, this is yet another approach I like for its simplicity. Let's not
dereference shortcuts if the user is saving one. Otherwise, no threat. Please?
The only downside I can see is the different behavior when saving a shortcut,
but this should be very rarely encountered and actually quite logical.
Attachment #186543 - Flags: superreview?(dveditz)
Attachment #186543 - Flags: review?(dougt)
I was just working on basically the same thing, and since mine fixes a couple
of additional bugs I'll attach it anyway. (I also have a working patch for the
pref alternative, but I think we're all pretty well agreed that sucks).

- Should the default filename exceed the filebuffer we correctly don't
overwrite anything, but it's unterminated and the OS will read off into space
(not a big deal).

- An evil attacker could end their filename with some combination of dots and
spaces. It'll still save as a .lnk but defeat naive extension-checking schemes.
Attachment #186546 - Flags: superreview?(dougt)
Attachment #186546 - Flags: review?(emaijala)
Comment on attachment 186543 [details] [diff] [review]
YAAP - Yet Another Alternate Patch

I like it, but it can be defeated by "foo.lnk..."
Attachment #186543 - Flags: superreview?(dveditz) → superreview-
Comment on attachment 186546 [details] [diff] [review]
alternate version of YAAP

Nice catches. r=emaijala
Attachment #186546 - Flags: review?(emaijala) → review+
Attachment #186543 - Attachment is obsolete: true
Attachment #186543 - Flags: review?(dougt)
Comment on attachment 186546 [details] [diff] [review]
alternate version of YAAP

change to the comment:

// extra space and dots fool us

To state that they do NOT fool us (since we know about them and are trimming
them out)

r=me lets do it.
Attachment #186546 - Flags: superreview?(dougt) → superreview+
Attachment #186504 - Flags: approval1.8b3?
Attachment #186546 - Flags: approval1.8b3?
Attachment #186546 - Flags: approval1.7.9?
Attachment #186546 - Flags: approval-aviary1.0.5?
Attachment #186546 - Flags: approval1.8b3?
Attachment #186546 - Flags: approval1.8b3+
Attachment #186546 - Flags: approval1.7.9?
Attachment #186546 - Flags: approval1.7.9+
Attachment #186546 - Flags: approval-aviary1.0.5?
Attachment #186546 - Flags: approval-aviary1.0.5+
Fix checked in to trunk, aviary1.0.1 and mozilla1.7 branches
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed-aviary1.0.5, fixed1.7.9
Resolution: --- → FIXED
Ere:

I think attachment 186546 [details] [diff] [review] is buggy.

> +        // Don't follow shortcuts when saving a shortcut, this can be used
> +        // to trick users (bug 271732)
> +        NS_ConvertUTF16toUTF8 ext(mDefault);
> +        ext.Trim(" .", PR_FALSE, PR_TRUE); // extra space and dots fool us

Why you convert the extension UTF16 to UTF8?
I think that we MUST convert UTF16 to Native.
You should use NS_CopyNativeToUnicode instead of NS_ConvertUTF16toUTF8.
See this patch.
https://bugzilla.mozilla.org/attachment.cgi?id=166672
Oops. Sorry.
NS_CopyNativeToUnicode -> NS_CopyUnicodeToNative
See https://bugzilla.mozilla.org/attachment.cgi?id=166569
(In reply to comment #182)
 
> I think attachment 186546 [details] [diff] [review] [edit] is buggy.

> > +        NS_ConvertUTF16toUTF8 ext(mDefault);
> > +        ext.Trim(" .", PR_FALSE, PR_TRUE); // extra space and dots fool us
> 
> Why you convert the extension UTF16 to UTF8?
> I think that we MUST convert UTF16 to Native.

No, we don't have to. |ext| is only used to check whether |mDefault| ends with
'.url/.lnk/.???' so that it doesn't matter whether it's UTF-8 or 'ANSI' string.
Actually, UTF-8 is better.
Attachment #186504 - Flags: superreview?(dveditz)
Attachment #186504 - Flags: review?(dveditz)
*** Bug 298792 has been marked as a duplicate of this bug. ***
*** Bug 292983 has been marked as a duplicate of this bug. ***
*** Bug 298812 has been marked as a duplicate of this bug. ***
*** Bug 298941 has been marked as a duplicate of this bug. ***
*** Bug 299152 has been marked as a duplicate of this bug. ***
*** Bug 299388 has been marked as a duplicate of this bug. ***
Alias: Shortcut
v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9)
Gecko/20050706 Firefox/1.0.5
*** Bug 306194 has been marked as a duplicate of this bug. ***
The actual problem is that *if* the shortcut was followed, Firefox *might*
overwrite a file if you download a shortcut file and save it as another shortcut
file. Which is a windows bug, not a Firefox bug. http://www.seslichatailesi.com
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: