38215 - js 'with' statement does not work for 'location'

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[sean echevarria]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.01; Windows NT 5.0)
BuildID:    2000042708

Two js functions that do the same thing in 2 different ways.  One succeeds, one 
fails (but should succeed).


<script LANGUAGE=JavaScript>
// calls to this work successfully
function showBaseURL()
{	
  baseURL = location.href.substring (0, location.href.lastIndexOf ("/") + 1);
  alert(baseURL);
}

// calls to this fail with 'href is not defined'
function showBaseURLwith()
{	
  with (location)
  {
    baseURL = href.substring (0, href.lastIndexOf ("/") + 1);
    alert(baseURL);
  }
}
</script>

Will attach test case.


Reproducible: Always
Steps to Reproduce:
1. load test case
2. press second form button


Actual Results:  see stdout js error 'href is not defined'

Expected Results:  should be same as pressing first form button

I did a pull today May 4.  Don't know why the build id says 20000427.

Comment 1

[sean echevarria]

Attachment: test case

Comment 2

[Brendan Eich [:brendan]]

This is a bug in nsLocation.cpp's use of the JS API.  It fails to implement a 
resolve hook to define "href" lazily, instead doing an inefficient strcmp on 
every property get or set, in order to call a special security check function.  
Two things:

1.  The JS engine asks, when a name is being sought in an object on the scope 
chain, by first searching the object's scope; if not found, it then tries the 
resolve hook; if resolve defines the property, the scope re-lookup finds it; 
else it's truly not-found and the search continues up the proto and (when each 
scope's proto chain is exhausted) parent chains.

Therefore implementations must predefine properties (possibly with "tiny-ids" 
for fast switch-based shared get and setProperty implementations), or define 
them lazily in their class resolve hooks.  By checking for a property id such as 
"href" only in the getter or setter, an implementation breaks with and other 
scope chain searches, and answers with the property's value only for fully 
qualified (o.p, not with(o) ...p...) references.

2.  Why the security check for href only?  Aren't the other substrings of the 
URL vulnerable to attack?

Reassigning to jst, cc'ing mstoltz.

/be

Comment 3

[Mitchell Stoltz (not reading bugmail)]

It's quite possible the other substrings of the URL are vulnerable to attack - 
none should be visible to cross-site scripts. I will look at these security 
checks when I can.

Comment 4

[Jesse Ruderman]

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

Comment 5

[ekrock's old account (dead)]

Nom. nsbeta2, recc. nsbeta2+ [some lenient date-], falling through to nsbeta3 
hard stop if not fixed during nsbeta2. with was in Nav4 JS. Don't want it to 
start occasionally breaking in Moz.

Comment 6

[leger]

Are top sites breaking.  Is this presenting a security problem?  Not clear on 
what fix is.

Comment 7

[sean echevarria]

I didn't present this as a security problem and can't address whether it is or 
not.  Mitchell, have you had a chance to look into the question raised by 
Brendan?  Perhaps the security implications raised as a result of this bug 
should be opened as a new bug.

Can't say that I've noticed this particular problem at a top100, but haven't 
look too hard.

That said - it is a masking bug.  Errors like this cause js exceptions that 
prevent more code from being exercised.

Comment 8

[Brendan Eich [:brendan]]

jst, may I relieve you of this bug?  I've already got it half-fixed in my tree.

/be

Comment 9

[Johnny Stenback (:jst)]

Brendan, be my guest :) I'm reassigning it to you.

Jan, AFAIK no top sites are breaking due to this problem.

Comment 10

[Brendan Eich [:brendan]]

This'll be fixed, along with other subtle DOM level 0 bugs related to it.

/be

Comment 11

[leger]

Putting on [nsbeta2-] radar.  We can fix in beta3.

Comment 12

[Brendan Eich [:brendan]]

Removing nsbeta2 keyword.

/be

Comment 13

[Daniel Veditz [:dveditz]]

Adding nsbeta2 keyword to bugs with nsbeta2 triage value in status field so the 
queries don't get screwed up

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Brendan Eich [:brendan]]

Same problem for image.src (document.images[i].src), see bug 49213.

/be

Comment 16

[ekrock's old account (dead)]

This bug report might be a DUP of bug 49112 "Referencing document.images in a JS 
with statement, does not works" which jst has acccepted as P2 nsbeta3+ for 9/21. 

Comment 17

[ekrock's old account (dead)]

Copying over comment by brendan from bug 49112:

>------- Additional Comments From Brendan Eich 2000-09-15 00:15 -------
> Not really [a DUP], they're fraternal twins.  The bugs describe separate 
> causes in the code, with separate yet similar symptoms.

Increasing severity to P2 as this is high priority backward compatibility (with 
statement and location were both in JS 1.0 IIRC, so this is DOM0 that if broken 
might possibly break a lot of pages). This would be eligible for check-in as 
late as 9/21 w/ mozilla reviewer approval. Brendan, do you think you have the 
time to do this? If so, please accept as [nsbeta3+]. If not, I wonder if someone 
else can take this on ...

Comment 18

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 19

[Johnny Stenback (:jst)]

This is the same problem as in bug 49112, only with different object and
different property. I just attached a fix (more or less the same fix that I'm
about to check in for bug 49112) and there's no reason to not fix this, this is
a backwards compatibility problem, we have a low risk fix. Setting to P2 to
match bug 49112. nsbeta3+ and r/a= please?

Brendan, feel free to hand this over to me unless you wanto land the fix
(assuming you approve of the patch).

Comment 20

[Brendan Eich [:brendan]]

r,a=[e-mail] on that patch -- thanks a bunch for fixing this.

/be

Comment 21

[ekrock's old account (dead)]

Marking [nsbeta3+], acting in Nisheeth's place in his absence per our previous 
discussions. This qualifies for check-in through 9/21 because it is a P2 high 
priority backward compatibility, JavaScript 1.0 language syntax and JavaScript 
1.0 DOM0. Please check-in immediately. Thanks!

Comment 22

[Phil Peterson]

PDT agrees P2

Comment 23

[Johnny Stenback (:jst)]

Fixed.

Comment 24

[sean echevarria]

vrfy