Closed Bug 1490868 Opened 6 years ago Closed 6 years ago

'Security wrapper denied access to property "supportedNetworks"' of BasicCardRequest but access to the containing object is allowed

Categories

(Firefox :: WebPayments UI, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: MattN, Unassigned)

References

Details

Attachments

(1 file)

(Marking as a security bug as this is about an inconsistency in the security wrapper which potentially affects other code)

If I create a PaymentRequest using a methodData[1] argument which was created in privileged code (e.g. in a test using ContentTask.spawn), there is an security wrapper error only for accessing properties of the BasicCardRequest dictionary[2], not for accessing the other parts of the `PaymentMethodData` dictionary that it is contained in. Perhaps this is because `PaymentMethodData.data` is defined as `object` in the .webidl[1]?

> Security wrapper denied access to property "supportedNetworks" on privileged Javascript object.…

See the attached testcase and STR and the stack trace in bug 1429181 comment 20.

The impact is that it's confusing in a test to have certain objects assigned to properties disappear due to security wrappers when the parent object and all descendant properties/objects were all created in the same scope with the same principal.

Is this a bug specific to BasicCardRequest[3][4][5] or is this expected behaviour for `object` in webidl? 

If it's the latter, should we perhaps always cloneInto with the task arguments in content-task.js[6] to avoid this footgun?

[1] https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/dom/webidl/PaymentRequest.webidl#10-13,106
[2] https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/dom/webidl/BasicCardPayment.webidl#15-18
[3] https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/dom/payments/PaymentRequest.cpp#275-278
[4] https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/dom/payments/BasicCardPayment.cpp#149,156,158-159
[5] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/BasicCardPaymentBinding.cpp#67,96
[6] https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/testing/mochitest/BrowserTestUtils/content/content-task.js#60
Flags: needinfo?(bobbyholley)
Group: core-security → dom-core-security
Component: General → XPConnect
> content.rq = new content.PaymentRequest(md, details);
> content.addEventListener("click", () => content.rq.show());

So with this bit, I think your intention is to put the object into the content scope, and then call it from content script, right? If so, that's not what's actually happening. |content| is going to be an XrayWrapper, so you'll be putting an expando property on the wrapper that's visible to the Xray caller but not to the content. This is the only reason the event listener works, because the function you're defining is being defined in the privileged scope as well (you're defining a function literal, not passing any sort of string that would get evaluated in the content scope).

So you could just as easily do |(new content.PaymentRequest(md, details)).show()|.

In terms of the specific issue: yes, it's the result of using |object| here. The code appears to funnel down to some manual parsing of the thing:

https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/payments/PaymentRequest.cpp#536
https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/payments/PaymentRequest.cpp#254
https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/payments/BasicCardPayment.cpp#149

which then tries to parse the thing into a dictionary. But since we're already past the binding layer, you don't benefit from the Xray layer, and just get stuck on the security wrapper.

Ideally, you would not use |object| here, and find a way to express what you want in webidl. Barring that, yes, you should use cloneInto in the test. I would not recommend doing that in the test harness, because that is likely to cause confusing behavior for unrelated things.
Flags: needinfo?(bobbyholley)
It sounds like this isn't really an XPConnect bug. Can we close and unhide it? Or move it back to a Firefox component and make it be about fixing the error.
Flags: needinfo?(bobbyholley)
Group: dom-core-security → firefox-core-security
Component: XPConnect → WebPayments UI
Flags: needinfo?(bobbyholley)
Product: Core → Firefox
(In reply to Bobby Holley (:bholley) from comment #1)
> > content.rq = new content.PaymentRequest(md, details);
> > content.addEventListener("click", () => content.rq.show());
> 
> So with this bit, I think your intention is to put the object into the
> content scope, and then call it from content script, right? If so, that's
> not what's actually happening. |content| is going to be an XrayWrapper, so
> you'll be putting an expando property on the wrapper that's visible to the
> Xray caller but not to the content. This is the only reason the event
> listener works, because the function you're defining is being defined in the
> privileged scope as well (you're defining a function literal, not passing
> any sort of string that would get evaluated in the content scope).
> 
> So you could just as easily do |(new content.PaymentRequest(md,
> details)).show()|.

Stashing the request on `content` was just copied from test code where we just need it available for a future ContentTask so it didn't actually need to be exposed to content.

> In terms of the specific issue: yes, it's the result of using |object| here.
> The code appears to funnel down to some manual parsing of the thing:
> 
> https://searchfox.org/mozilla-central/rev/
> 99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/payments/PaymentRequest.cpp#536
> https://searchfox.org/mozilla-central/rev/
> 99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/payments/PaymentRequest.cpp#254
> https://searchfox.org/mozilla-central/rev/
> 99cbc0aec3e1c0b65ff9052523fb5c181b248f57/dom/payments/BasicCardPayment.
> cpp#149
> 
> which then tries to parse the thing into a dictionary. But since we're
> already past the binding layer, you don't benefit from the Xray layer, and
> just get stuck on the security wrapper.

OK, thanks for the explanation! 

> Ideally, you would not use |object| here, and find a way to express what you
> want in webidl. Barring that, yes, you should use cloneInto in the test. I
> would not recommend doing that in the test harness, because that is likely
> to cause confusing behavior for unrelated things.

OK, the `object` comes from the spec and I don't think we can change it so we're using cloneInto now.
Group: firefox-core-security
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: