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

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Matt Ellis]

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.

Comment 1

[Matt Ellis]

Code for the page:

<frameset cols="100%,*" border=0>
  <frame src="LogoutRedirect.asp" scrolling=no>
  <frame src="/Scripts/Balances.dll?LogoutPage" scrolling=no>
</frameset>

Comment 2

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

-> bz

Comment 3

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

not security sensitive

Comment 4

[Boris Zbarsky [:bzbarsky]]

The real problem page is
http://www1.scottsave.com/Scripts/Balances.dll?LogoutPage of course.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

More sniff for the snuff.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: even more snuffling

Comment 7

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

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

Comment 8

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

eh, looks like that second line was wrong; what I meant was the other line in
the macro that used nsDependentCString

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm... I suppose I could in fact use NS_LITERAL_CSTRING there.  Will change that.

Comment 10

[timeless]

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

[Darin Fisher]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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

[Darin Fisher]

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)

Comment 14

[Boris Zbarsky [:bzbarsky]]

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...

Comment 15

[Darin Fisher]

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 :-(

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this?

Comment 18

[jag (Peter Annema)]

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 " "|">" */

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: This is simler and makes the file about 19k smaller

Comment 20

[jag (Peter Annema)]

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

sr=jag

Comment 21

[Boris Zbarsky [:bzbarsky]]

checked that in too

Comment 22

[Matt Ellis]

Looks great.  Thank you all.

Comment 23

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

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

removing sr request on obsolete patch

Comment 24

[Matt Ellis]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

*** Bug 221767 has been marked as a duplicate of this bug. ***

Comment 26

[Boris Zbarsky [:bzbarsky]]

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.

Comment 27

[Matt Ellis]

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?

Comment 28

[Boris Zbarsky [:bzbarsky]]

You need to set the flags on the patches, not on the bug itself...

Comment 29

[Matt Ellis]

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

Comment 30

[Matt Ellis]

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

And this one, too.

Comment 31

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

clearing blocking1.4.1? because it's released

Comment 32

[Mike Kaply [:mkaply]]

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

a=mkaply for 1.4.2

Comment 33

[Boris Zbarsky [:bzbarsky]]

I can't check this in (no tree), so if we want this anytime soon someone needs
to do the checkin....

Comment 34

[Mike Kaply [:mkaply]]

fixed in 1.4.2