"Change Value" context menu item is always disabled in Local Variables window of Venkman (JavaScript Debugger)

NEW
Unassigned

Status

Other Applications
Venkman JS Debugger
8 years ago
8 years ago

People

(Reporter: Lo, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729)
Build Identifier: 0.9.87.4

Observed beginning with version 0.9.87.1

Reproducible: Always

Steps to Reproduce:
1. Set a breakpoint in a loaded script.
2. When the breakpoint is hit, the local variables window is populated. Right-click on any of the variables for the context menu.
3. Observe that the Change Value context menu item is always grayed out no matter what variable you click on.
Actual Results:  
The 1st item in the context menu, "Change Value...", is disabled.

Expected Results:  
The "Change Value..." option should be enabled.
(Reporter)

Comment 1

8 years ago
Created attachment 388344 [details] [diff] [review]
Proposed patch, requesting r/sr attention
Attachment #388344 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 388344 [details] [diff] [review]
Proposed patch, requesting r/sr attention

This code was removed in a security bug. I am not comfortable with blanket-reinstating it without being very, very sure that doing so is safe. I should add that if you doubleclick a value, the dialog comes up normally, so it doesn't seem.

In particular, a safer fix may be to use the same test as is used for the qualified name menu entry (in venkman-views.js), ie has('expression'). I think that is, regardless of everything else, a more proper fix (as we need a complete expression anyway). That said, looks like cmdChangeValue could do with some fixing... Meh, that is not for this bug.
Attachment #388344 - Flags: review?(gijskruitbosch+bugs) → review-
Egh. Last sentence of the first paragraph should end "so it doesn't seem like the check is proper anyway".
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

8 years ago
The actual line being added by the patch is safe in and of itself (it's a wrapped jsdValue), however, cmdChangeValue is unsafe from start to finish. :(
(In reply to comment #4)
> The actual line being added by the patch is safe in and of itself (it's a
> wrapped jsdValue), however, cmdChangeValue is unsafe from start to finish. :(

Heh. There's a bug for all these cases (which I presume includes this particular one, although I should check). As for the actual line - I wasn't sure, and for some reason although I can look at the relevant bug, I can't actually see the attachment (testcase)? (I used to be able to!) Blah. :-\
(Reporter)

Comment 6

8 years ago
Created attachment 389708 [details] [diff] [review]
Updated patch to test for "has('expression')" instead.
Attachment #388344 - Attachment is obsolete: true
Attachment #389708 - Flags: review?(gijskruitbosch+bugs)
(In reply to comment #6)
> Created an attachment (id=389708) [details]
> Updated patch to test for "has('expression')" instead.

Why do we still need the .parentRecord assignment? Or was leaving that in the patch unintentional?
(Reporter)

Comment 8

8 years ago
> (In reply to comment #6)
> > Created an attachment (id=389708) [details] [details]
> > Updated patch to test for "has('expression')" instead.
> 
> Why do we still need the .parentRecord assignment? Or was leaving that in the
> patch unintentional?

Yes, intentional. It is specified as one of the params used by cmdChangeValue(), which won't work without it.
Comment on attachment 389708 [details] [diff] [review]
Updated patch to test for "has('expression')" instead.

OK, so, basically, I still don't want to take this patch. The reason is that cmdChangeValue, as James said, is a catastrophe in terms of security. I'm fixing it in bug 396142, but it involves rewriting it from scratch, also removing the need for parentValue to be around. So I'm going to r- this, sorry! :-(
Attachment #389708 - Flags: review?(gijskruitbosch+bugs) → review-
You need to log in before you can comment on or make changes to this bug.