97529 – REGRESSION (r129456): http/tests/security/xss-eval.html is failing on JSC platforms

RESOLVED FIXED 97529

REGRESSION (r129456): http/tests/security/xss-eval.html is failing on JSC platforms

https://bugs.webkit.org/show_bug.cgi?id=97529

Summary REGRESSION (r129456): http/tests/security/xss-eval.html is failing on JSC pla...

Zan Dobersek

Reported 2012-09-24 23:24:44 PDT

http/tests/security/xss-eval.html started failing on all JavaScriptCore platforms after r129456: http://trac.webkit.org/changeset/129456 http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fsecurity%2Fxss-eval.html Here's the diff: --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/xss-eval-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/http/tests/security/xss-eval-actual.txt @@ -5,7 +5,7 @@ If the test passes, you'll see a pass message below. PASS: eval.call(frames[0], 'document') should be EvalError and is. -PASS: childEval.call(frames[0], 'document') should be EvalError and is. +FAIL: childEval.call(frames[0], 'document') should be EvalError but instead is [object HTMLDocument]. PASS: childEvalCaller('document') should be TypeError and is. -PASS: childLocalEvalCaller('document') should be EvalError and is. +FAIL: childLocalEvalCaller('document') should be EvalError but instead is [object HTMLDocument].

Attachments
Fix (15.04 KB, patch) 2012-09-25 17:00 PDT, Gavin Barraclough	buildbot: commit-queue-	Details Formatted Diff Diff
Fixed cross-frame-access-call (16.73 KB, patch) 2012-09-25 21:02 PDT, Gavin Barraclough	fpizlo: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Gavin Barraclough

Comment 1 2012-09-24 23:35:30 PDT

Ooops, I'll look into this! - we may be changing this behaviour anyway, I don't think these EvalErrors match the spec, going to test against other browsers.

Csaba Osztrogonác

Comment 2 2012-09-25 02:51:18 PDT

Skipped on Qt by https://trac.webkit.org/changeset/129481, on GTK by https://trac.webkit.org/changeset/129460

Raphael Kubo da Costa (:rakuco)

Comment 3 2012-09-25 04:47:22 PDT

And on EFL in http://trac.webkit.org/changeset/129483 :-)

Gavin Barraclough

Comment 4 2012-09-25 17:00:52 PDT

Created attachment 165702 [details] Fix

Build Bot

Comment 5 2012-09-25 17:43:42 PDT

Comment on attachment 165702 [details] Fix Attachment 165702 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14033227 New failing tests: http/tests/security/cross-frame-access-call.html

WebKit Review Bot

Comment 6 2012-09-25 19:01:29 PDT

Comment on attachment 165702 [details] Fix Attachment 165702 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14002958 New failing tests: http/tests/security/xss-eval.html fast/js/eval-cross-window.html

Gavin Barraclough

Comment 7 2012-09-25 21:02:46 PDT

Created attachment 165722 [details] Fixed cross-frame-access-call

Gavin Barraclough

Comment 8 2012-09-25 21:45:23 PDT

Fixed in r129592

Zan Dobersek

Comment 9 2012-09-26 00:00:40 PDT

(In reply to comment #8) > Fixed in r129592 Thanks, the changes seem to work well, albeit there have been some failures on the Chromium ports: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showAllRuns=true&tests=fast%2Fjs%2Feval-cross-window.html%2Chttp%2Ftests%2Fsecurity%2Fcross-frame-access-call.html%2Chttp%2Ftests%2Fsecurity%2Fxss-eval.html Two of these seem more like a progression though http/tests/security/xss-eval.html looks more of a failure.

Gavin Barraclough

Comment 10 2012-09-26 00:07:42 PDT

(In reply to comment #9) > (In reply to comment #8) > > Fixed in r129592 > > Thanks, the changes seem to work well, albeit there have been some failures on the Chromium ports: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showAllRuns=true&tests=fast%2Fjs%2Feval-cross-window.html%2Chttp%2Ftests%2Fsecurity%2Fcross-frame-access-call.html%2Chttp%2Ftests%2Fsecurity%2Fxss-eval.html > > Two of these seem more like a progression though http/tests/security/xss-eval.html looks more of a failure. We don't thing the xss-eval change diminishes security. This change does not allow the parent frame to inject new code that gets to run in the navigated frame's environment - the eval takes place in the context the eval function was extracted from, which has the same security origin (and if it did not, we would not have had access to eval in the first place). The test does cover a case where the eval does try to access the new document global object, and this is correctly inhibited. cheers, G.

Csaba Osztrogonác

Comment 11 2012-09-26 01:34:34 PDT

Unskipped on Qt - r129608.

WebKit Review Bot

Comment 12 2012-09-26 06:12:09 PDT

Re-opened since this is blocked by 97670

Stephen Chenney

Comment 13 2012-09-26 06:17:40 PDT

Adding Chromium security people to verify the security assertions. We also need to know why this is causing chromium tests to fail.

Adam Barth

Comment 14 2012-09-26 09:03:25 PDT

> Adding Chromium security people to verify the security assertions. We also need to know why this is causing chromium tests to fail. I'm sorry, I'm not sure what you're asking. Ignoring the |this| value passed to eval sounds safe. If that's what other browsers do in this case, that seems like a good thing to do. It's certainly more useful than throwing an error if |this| doesn't match what you expect.

Csaba Osztrogonác

Comment 15 2012-09-26 09:05:49 PDT

(In reply to comment #12) > Re-opened since this is blocked by 97670 Skipped on Qt again after the rollout - https://trac.webkit.org/changeset/129639 Please unskip it with the proper fix.

Stephen Chenney

Comment 16 2012-09-26 09:38:42 PDT

(In reply to comment #14) > > Adding Chromium security people to verify the security assertions. We also need to know why this is causing chromium tests to fail. > > I'm sorry, I'm not sure what you're asking. Ignoring the |this| value passed to eval sounds safe. If that's what other browsers do in this case, that seems like a good thing to do. It's certainly more useful than throwing an error if |this| doesn't match what you expect. Sorry, should not have used the word "assertions". I simply wanted to be sure that you agree with the analysis in comment #10, before we put this patch back in and punt the errors to the v8 team (or whomever is the right group to fix the failures).

Adam Barth

Comment 17 2012-09-26 10:12:59 PDT

> I simply wanted to be sure that you agree with the analysis in comment #10. Yes.

Beth Dakin

Comment 18 2012-09-26 13:50:09 PDT

(In reply to comment #8) > Fixed in r129592 This test still seems to be failing on the Mac bots.

Beth Dakin

Comment 19 2012-09-26 13:52:48 PDT

(In reply to comment #18) > (In reply to comment #8) > > Fixed in r129592 > > This test still seems to be failing on the Mac bots. Oh, I see the patch was rolled out. Sorry for the noise!

Gavin Barraclough

Comment 20 2012-09-26 16:21:20 PDT

Hi, I didn't realize this was rolled out – relanding – please file bugs against V8 to match behaviour or skip tests as you see fit.

Gavin Barraclough

Comment 21 2012-09-26 16:25:22 PDT

Relanded in r129712.

Csaba Osztrogonác

Comment 22 2012-09-27 03:07:15 PDT

(In reply to comment #21) > Relanded in r129712. and unskipped on Qt again - r129747

Csaba Osztrogonác

Comment 23 2012-09-27 03:24:35 PDT

I digged a little bit the history of this bug: https://trac.webkit.org/changeset/129456 - original patch https://trac.webkit.org/changeset/129460 - skip on GTK https://trac.webkit.org/changeset/129481 - skip on Qt https://trac.webkit.org/changeset/129483 - skip on EFL https://trac.webkit.org/changeset/129592 - fix landed https://trac.webkit.org/changeset/129599 - unskip on EFL https://trac.webkit.org/changeset/129608 - unskip on Qt https://trac.webkit.org/changeset/129615 - unskip on GTK https://trac.webkit.org/changeset/129629 - fix rolled out https://trac.webkit.org/changeset/129635 - skip on EFL https://trac.webkit.org/changeset/129639 - skip on Qt https://trac.webkit.org/changeset/129680 - skip on GTK https://trac.webkit.org/changeset/129712 - fix relanded https://trac.webkit.org/changeset/129740 - unskip on EFL https://trac.webkit.org/changeset/129747 - unskip on Qt ... and it it still skipped on GTK I think we should reduce the number of the commits in the future ... In my opinion if you fix a bug, you should unskip the test on all platform _with_ the proper fix. And if you rollout a patch, you should rollout its dependency too. If we had followed this suggestion, we would have spare many gardening patches. - r129592 should have contained r129599, r129608, r129615 - r129629 should have contained r129635, r129639, r129680 - r129712 should have contained r129740, r129747, r......

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Tools / Tests

Assignee

Gavin Barraclough

Reported

2012-09-24 23:24 PDT

Modified

2012-09-27 03:24 PDT History

CC List

13 users Show

URL

Keywords

Depends on

97670

Blocks

79666 97519

Dependencies

tree graph