[FIX]attempts to download balances.dll file on logout of site

RESOLVED FIXED in mozilla1.5beta

Status

Core Graveyard
File Handling
P2
normal
RESOLVED FIXED
15 years ago
a year ago

People

(Reporter: Matt Ellis, Assigned: bz)

Tracking

({fixed1.4.2})

Trunk
mozilla1.5beta
x86
All
fixed1.4.2
Bug Flags:
blocking1.4.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030718
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030718

Logging out of www.scottsave.com brings up download manager wanting to download
balances.dll file during redirect to www.scottrade.com

Reproducible: Always

Steps to Reproduce:
1.log in to www.scottsave.com
2.click log out link
3.

Actual Results:  
Download manager popped up with "The file "Balances.dll" is of type
application/x-msdownload, and Mozilla does not know how to handle this file
type.  This file is located at: https://www1.scottsave.com/Scripts/
What should Mozilla do with this file?"

Expected Results:  
no download manager asking to download file from server.  This did not occur in
version 1.3.1, but can be reproduced in version 1.4 as well.
(Reporter)

Comment 1

15 years ago
Code for the page:

<frameset cols="100%,*" border=0>
  <frame src="LogoutRedirect.asp" scrolling=no>
  <frame src="/Scripts/Balances.dll?LogoutPage" scrolling=no>
</frameset>
-> bz
Assignee: blakeross → bzbarsky
not security sensitive
Group: security
The real problem page is
http://www1.scottsave.com/Scripts/Balances.dll?LogoutPage of course.
Status: UNCONFIRMED → NEW
Component: Download Manager → File Handling
Ever confirmed: true
Priority: -- → P2
Summary: attempts to download balances.dll file on logout of site → [FIX]attempts to download balances.dll file on logout of site
Target Milestone: --- → mozilla1.5beta
Created attachment 128588 [details] [diff] [review]
Patch

More sniff for the snuff.
Attachment #128588 - Flags: superreview?(darin)
Attachment #128588 - Flags: review?(cbiesinger)
Created attachment 128589 [details] [diff] [review]
even more snuffling
Attachment #128588 - Attachment is obsolete: true
Attachment #128588 - Attachment is obsolete: false
Attachment #128588 - Flags: superreview?(darin)
Attachment #128588 - Flags: review?(cbiesinger)
Attachment #128589 - Flags: superreview?(darin)
Attachment #128589 - Flags: review?(cbiesinger)
Comment on attachment 128589 [details] [diff] [review]
even more snuffling

+     Equals(nsDependentCString(_tagstr) + NS_LITERAL_CSTRING(" "),
comparator)\
[...]
+ NS_LITERAL_CSTRING(">"), comparator))

Can you use LITERAL_CSTRING instead of DependentCSTring in these two lines?

r=biesi if you use it or explain why you can't
Attachment #128589 - Flags: review?(cbiesinger) → review+
eh, looks like that second line was wrong; what I meant was the other line in
the macro that used nsDependentCString
Hmm... I suppose I could in fact use NS_LITERAL_CSTRING there.  Will change that.

Comment 10

15 years ago
for people watching this at home, the reason that page is special is that it
doesn't send a content type, so we start guessing. the problem is that the
guessing alg didn't match the content so it fell back to another system. on
linux and windows that system probably maps the url to a mime type. beos doesn't
have that, so (biesi and) i got text/plain and didn't see a real problem :).

Comment 11

15 years ago
Comment on attachment 128589 [details] [diff] [review]
even more snuffling

>Index: netwerk/streamconv/converters/nsUnknownDecoder.cpp

>+#define MATCHES_TAG(_tagstr)                                                  \
>+  (Substring(str, pos, sizeof(_tagstr)).                                      \
>+     Equals(nsDependentCString(_tagstr) + NS_LITERAL_CSTRING(" "), comparator)\
>+   ||                                                                         \
>+   Substring(str, pos, sizeof(_tagstr)).                                      \
>+     Equals(nsDependentCString(_tagstr) + NS_LITERAL_CSTRING(">"), comparator))

hmm... you could also use the C preprocessor like this:

#define MATCHES_TAG(_tagstr)
  (Substring(str, pos, sizeof(_tagstr)).				      \
     Equals(NS_LITERAL_CSTRING(_tagstr " "), comparator)		      \
   ||									      \
   Substring(str, pos, sizeof(_tagstr)).				      \
     Equals(NS_LITERAL_CSTRING(_tagstr ">"), comparator))

of course that adds some bloat since the string data will increase in size by
nearly a factor of 3, but it might be worth it to avoid the runtime
concatenation
overhead :-/


>+      MATCHES_TAG("p")        ||

hmm, so a document that looks like:

p e a c e   o n   e a r t h

would be treated as text/html?	is it really right to treat a document starting
with "p " as HTML?  i know we are only talking about non-standard pages
anyways,
but... this does seem a little bit to accepting to me, or is it required for
some real world sites?


>+      MATCHES_TAG("b")        ||

same goes for this one as well.
> hmm... you could also use the C preprocessor like this:

Good idea.  Will do that and see whether the tree is happy.

> hmm, so a document that looks like:
> p e a c e   o n   e a r t h

It would have to be "<p e a c e   o n   e a r t h", since all this is only going
on if the first non-whitespace char in the document was a '<'.

And unfortunately, I think that documents like:

<p><a href="foo"><img src="myad"></a></p>

are probably not too uncommon.

Comment 13

15 years ago
ok, the leading '<' does make a huge difference... i should have looked over the
code more closely.  in that case, i'm fine with the addition of those other tags.  

sr=darin (with or without the preprocessor-based string concatenations)
Checked in, but this made codesize 20K bigger on Linux, 5K bigger on Windows.

Just moving the Substring().Equals() stuff into a function (out of line) makes
this library 16-18K smaller in my opt tree.

Anything we can do about that, jag?  That's pretty ridiculous codesize, imo...
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 15

15 years ago
moving the checking code into a subroutine seems like the right way to go.  in
fact, i'd almost suggest rolling your own loop to check the strings.  that would
be the most efficient and probably would be less overall code.  keep in mind
that with each Substring invocation there is a virtual dtor.. same thing applies
to nsDependentCString :-(
> in fact, i'd almost suggest rolling your own loop to check the strings.

Ah... nested-if-city.  The code would be pretty much unreadable.

This is called pretty rarely, and I doubt microseconds (or even milliseconds)
matter here much.  So I'll go ahead and optimize for codesize.  Patch coming up.
Created attachment 128889 [details] [diff] [review]
Something like this?
Attachment #128588 - Attachment is obsolete: true
Attachment #128589 - Attachment is obsolete: true
Attachment #128889 - Flags: superreview?(darin)
Attachment #128889 - Flags: review?(cbiesinger)

Comment 18

15 years ago
Comment on attachment 128889 [details] [diff] [review]
Something like this?

This should be a lot better. Except you could probably speed this up a little
more by moving the " " || ">" logic into StringMatchesTag so you only have to
create the Substring once. Also, add a comment to the sizeof(_tagstr) that it's
really - 1 + 1 /* remove terminator, add " "|">" */
Created attachment 128893 [details] [diff] [review]
This is simler and makes the file about 19k smaller
Attachment #128889 - Attachment is obsolete: true
Attachment #128889 - Attachment is obsolete: false
Attachment #128889 - Flags: superreview?(darin)
Attachment #128889 - Flags: review?(cbiesinger)
Attachment #128893 - Flags: superreview?(darin)
Attachment #128893 - Flags: review?(cbiesinger)
Attachment #128893 - Flags: review?(cbiesinger) → review+

Comment 20

15 years ago
Comment on attachment 128893 [details] [diff] [review]
This is simler and makes the file about 19k smaller

sr=jag
Attachment #128893 - Flags: superreview?(darin) → superreview+
checked that in too
(Reporter)

Comment 22

15 years ago
Looks great.  Thank you all.
(Reporter)

Updated

14 years ago
Flags: blocking1.4.1?
Comment on attachment 128589 [details] [diff] [review]
even more snuffling

removing sr request on obsolete patch
Attachment #128589 - Flags: superreview?(darin)
(Reporter)

Comment 24

14 years ago
Still occurring in nightlies of Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.4.1) Gecko/20030906, so I set 1.4.2 blocking to ?.  Has been fixed for the
1.5b target, but was hoping that it would be fixed in latest 1.4.x as they went
along as well.
Flags: blocking1.4.2?
*** Bug 221767 has been marked as a duplicate of this bug. ***
If you think this should go on the 1.4 branch, please request driver approval by
setting the "mozilla1.4.2?" flag on the "even more snuffling" patch and the
"This is simler [sic] and ...." patch.  I'm quite willing to land the patches
from this bug on the 1.4 branch if drivers want them.
(Reporter)

Comment 27

14 years ago
I would hope to see them incorporated into the 1.4 branch.  The only flags I see
are the ones I set in September.  Do I clear them, resubmit the patches, then
reset the 1.4.2? flag?
You need to set the flags on the patches, not on the bug itself...
(Reporter)

Comment 29

14 years ago
Comment on attachment 128589 [details] [diff] [review]
even more snuffling

Still occurring in nightlies of Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.4.1) Gecko/20030906 and would like to see it patched on the 1.4 branch
Attachment #128589 - Flags: approval1.4.2?
(Reporter)

Comment 30

14 years ago
Comment on attachment 128893 [details] [diff] [review]
This is simler and makes the file about 19k smaller

And this one, too.
Attachment #128893 - Flags: approval1.4.2?
clearing blocking1.4.1? because it's released
Flags: blocking1.4.1?

Comment 32

14 years ago
Comment on attachment 128893 [details] [diff] [review]
This is simler and makes the file about 19k smaller

a=mkaply for 1.4.2
Attachment #128893 - Flags: approval1.4.2? → approval1.4.2+
I can't check this in (no tree), so if we want this anytime soon someone needs
to do the checkin....

Comment 34

14 years ago
fixed in 1.4.2
Flags: blocking1.4.2? → blocking1.4.2+
Keywords: fixed1.4.2

Updated

14 years ago
Attachment #128589 - Flags: approval1.4.2?
Whiteboard: [sg:nse]

Updated

11 years ago
Duplicate of this bug: 207295
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.