location's PUNCTURE policy throws without setting an exception

RESOLVED FIXED in mozilla16

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
mozilla16
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
I don't have a testcase right now, but I ran into this working on another bug. Basically, Policy::check functions are responsible for setting their own exceptions if they deny access.
(Assignee)

Comment 1

5 years ago
Created attachment 621016 [details] [diff] [review]
Proposed fix

I think the change to AccessCheck.cpp makes the control flow a little clearer (and it nukes a couple of braces). It's a little unfortunate that this patch actually makes LocationPolicy::check less clear (since the exception for PUNCTURE is "hidden" in a larger boolean expression) but it's the easiest way to share the code that throws the exception.
Attachment #621016 - Flags: review?(bobbyholley+bmo)
Comment on attachment 621016 [details] [diff] [review]
Proposed fix


>--- a/js/xpconnect/wrappers/AccessCheck.cpp
>+++ b/js/xpconnect/wrappers/AccessCheck.cpp
>@@ -494,12 +494,10 @@ ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper:
>         perm = PermitObjectAccess;
>         return true;
>     }
>-    if (act == Wrapper::PUNCTURE) {
>-        perm = DenyAccess;
>-        return false;
>-    }
> 
>     perm = DenyAccess;
>+    if (act == Wrapper::PUNCTURE)
>+        return false;

Doesn't this have the same issue?
(Assignee)

Comment 3

5 years ago
Created attachment 621024 [details] [diff] [review]
Proposed fix v2

Oops, yes.
Attachment #621016 - Attachment is obsolete: true
Attachment #621016 - Flags: review?(bobbyholley+bmo)
Attachment #621024 - Flags: review?(bobbyholley+bmo)
Comment on attachment 621024 [details] [diff] [review]
Proposed fix v2

Can we add some tests for this? I'm about to touch this code in a couple of ways (fixing __exposedProps__, and removing universalXPConnect), and this seems like the sort of thing I might regress.
Attachment #621024 - Flags: review?(bobbyholley+bmo) → review+
So ... can we land this?
I think so, yes.
Keywords: checkin-needed
(Assignee)

Comment 7

5 years ago
Comment on attachment 621024 [details] [diff] [review]
Proposed fix v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d752d1b621b
Attachment #621024 - Flags: checkin+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/8d752d1b621b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.