Closed Bug 805929 Opened 12 years ago Closed 10 years ago

(CSP) when setTimeout is blocked by CSP, it should return zero

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: geekboy, Assigned: geekboy)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [CSP 1.0])

Attachments

(1 file)

According to the CSP 1.0 Spec, when setTimeout/setInterval are blocked due to "eval" restrictions, they should not register a timeout and return zero.  Right now these blocked calls return nonzero values.

See http://www.w3.org/TR/CSP/#processing-model (4.2 - unsafe-eval).
Severity: normal → minor
Priority: -- → P2
Whiteboard: [CSP 1.0]
I augmented the eval tests (includes setTimeout) to check the return value and this seems to workforme.

We don't currently have test coverage for this behavior, so it may be good to land the tests anyway.

Garrett: since you're currently hacking on CSP tests, I'd like a gut check from you.  Please review my patch if you think we should land some additional test coverage here (or clear the flag and close this worksforme if you don't think it's necessary).
Assignee: nobody → sstamm
Status: NEW → UNCONFIRMED
Ever confirmed: false
Attachment #8431893 - Flags: review?(grobinson)
Comment on attachment 8431893 [details] [diff] [review]
test setInterval and return values

Review of attachment 8431893 [details] [diff] [review]:
-----------------------------------------------------------------

> Please review my patch if you think we should land some additional test coverage here (or clear the flag and close this worksforme if you don't think it's necessary).

I'm always in favor of additional test coverage :) Especially when it's for a behavior made explicit in the spec. Do we have any idea what caused the issue to be fixed in the interim?

::: content/base/test/csp/file_CSP_evalscript_main.js
@@ +14,5 @@
> +var verifyZeroRetVal = (function(window) {
> +  return function(val, details) {
> +    logResult((val === 0 ? "PASS: " : "FAIL: ") + "Blocked interval/timeout should have zero return value; " + details, val === 0);
> +    window.parent.verifyZeroRetVal(val, details);
> +  };})(window);

Nit: I'd put a newline after the semicolon, but this is fine.

Why do you do a closure with window? Are you afraid someone's going to reassign it later, at the call site?
Attachment #8431893 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #2)
> Comment on attachment 8431893 [details] [diff] [review]
> test setInterval and return values
> 
> Review of attachment 8431893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Please review my patch if you think we should land some additional test coverage here (or clear the flag and close this worksforme if you don't think it's necessary).
> 
> I'm always in favor of additional test coverage :) Especially when it's for
> a behavior made explicit in the spec. Do we have any idea what caused the
> issue to be fixed in the interim?

Ok, we'll get this test in there.  I suspect it was Bug 918345, which changed return from NS_ERROR_DOM_TYPE_ERR [0] to a success condition with proper error handling [1].

[0] http://hg.mozilla.org/mozilla-central/diff/508288a2b62c/dom/base/nsJSTimeoutHandler.cpp#l1.325
[1] http://hg.mozilla.org/mozilla-central/diff/508288a2b62c/dom/base/nsJSTimeoutHandler.cpp#l1.163

> ::: content/base/test/csp/file_CSP_evalscript_main.js
> Why do you do a closure with window? Are you afraid someone's going to
> reassign it later, at the call site?

I was naively following a pattern from elsewhere in the file, but yes.  That's the concern (bindings with setTimeout/setInterval can change).
We should probably wait to land this until after bug 988616 lands (since that's a lot more work and I'm reluctant to bitrot it).
Depends on: 988616
Figured I'd run this through try with bug 846978:
https://tbpl.mozilla.org/?tree=Try&rev=23d220cee0d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8404699d2c58
Flags: in-testsuite+
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/8404699d2c58
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: