RE: [webappsec-testsuite] RE: Your submitted test

Odin,

 I made the fixes you suggested.  Thanks.  New test (as HTML+JS only) is checked into:

webappsec/tests/cors/submitted/bhill2

  The test makes a simple CORS request, gets the Origin header that was sent, then makes another request to a cross-origin host which performs a redirect back to the source origin.  The Origin header should be different from the simple request because of the redirect, otherwise the target of a CORS request can reflect/forward any request and assume the Origin "credentials" of the requesting site.

 This test confirms a clarification requested as part of the LC review:

http://www.w3.org/2011/webappsec/track/actions/46 

  Somewhere in the edits between RFC 6454 and CORS, the text specifying this behavior was lost.  RFC 6454 originally specified this behavior, but the final version omits this and leaves the choice of whether and how to modify the Origin header on a redirect to each API. CORS simply says to do what the RFC says, which creates a circular reference.  We need to put the text specifying the correct redirect behavior (append the new Origin or set it to null) in CORS.

  I would appreciate any further review of the test case and supporting files.

Thanks,

Brad Hill

> -----Original Message-----
> From: Hill, Brad [mailto:bhill@paypal-inc.com]
> Sent: Thursday, February 02, 2012 10:25 AM
> To: Odin Hørthe Omdal; public-webappsec-testsuite@w3.org
> Subject: [webappsec-testsuite] RE: Your submitted test
> 
> Copying public-webappsec-testsuite@w3.org.
> 
> Thanks, Odin, for the great review.  I'm adding the public testsuite list for the
> group here as it is a good example of what we need to be doing!
> 
> 1. Check in with the test coordinators when submitting tests 2. Code review
> 3. Better documentation
> 
> The issues you point out are from some of the first PHP I've ever written and
> I'm still learning the test framework by example, so I'm not surprised my
> code is crap! :)  I'll work on addressing the issues you point out below, follow
> up here on the list, and I hope others can learn a bit from this, as well.
> 
> -Brad
> 
> > -----Original Message-----
> > From: Odin Hørthe Omdal [mailto:odinho@opera.com]
> > Sent: Thursday, February 02, 2012 2:05 AM
> > To: Hill, Brad
> > Subject: Your submitted test
> >
> > Hey!
> >
> > I've looked through your test. As you haven't announced a call for
> > reviews of it I'll just send you some directly, if you don't mind :-)
> >
> >
> > Issue 1
> > -------
> > All the code in the beginning is unnecessary to keep in PHP. The place
> > where you make the URLs etc. You are using them two places:
> > $get_origin, and $next_script + urlencode($get_origin).
> >
> > You can make the crossdomain path in javascript like this:
> >
> > --
> > // First we need a dirname function
> > function dirname(path) {
> >        return path.replace(/\/[^\/]*$/, '/') }
> >
> > // Then substitude our address
> > var crossdomain = dirname(location.href)
> >        .replace('://www.', '://www2.')
> > --
> >
> > Then you can use:
> >    req.open('get', crossdomain + 'support/get-origin.php'); and
> >    req.open('get', 'support/redir-to-get-origin.php?url=' +
> > encodeURIComponent(crossdomain + 'support/get-origin.php'));
> >
> >
> > The benefit? It'll be easier to see what's happening from the html
> > file, and it'll also work on other setups rather than hardcoding the paths.
> >
> >
> > Issue 2
> > -------
> > You're making two tests (or actually 4), but they are sorely dependent
> > on eachother. Testing this with the wrong URL's on my local setup, the
> > first one failed, but the second one passed because obviously it
> > didn't return Origin == "http://www.w3c-test.org". The second test is
> > dependent on the first one working. It's all really one test.
> >
> > Also, you should use the async test system, not sync tests here. So
> > start
> > with:
> >
> > var t = async_test();
> >
> > and use that single test for everything. It's kinda similar to the
> > review I did of Microsofts IndexedDB tests:
> > http://lists.w3.org/Archives/Public/public-webapps-

> > testsuite/2012Jan/0002.html
> >
> >
> > Issue 3
> > -------
> > Don't use try-catch, use the framework.
> >
> > t.step(function() {
> >    might_throw()
> > })
> >
> > Does the same thing as
> >
> > try {
> >    might_throw()
> > }
> > catch(ex) {
> >    assert_true(false, ex.message)
> > }
> >
> > Only much better! :-)
> >
> > You should wrap your whole testing inside the testing framework, and
> > for new functions, should do it to them to. So:
> >
> > req2.onreadystatechange = t.step_func(function() {
> >    whatever();
> > });
> >
> > That way any exceptions caught will fail the test.
> >
> >
> >
> >
> >
> > As for what the test is testing itself, it isn't very clear to me. It
> > seems wrong according to spec. When you do:
> >
> >    local redirect to crossdomain page,
> >
> > the origin-header sent to that crossdomain page should in fact be
> > "www.w3c-tests.org", whilst you are testing that it is NOT that. Seems
> > inverted. Care to explain?
> > --
> > Odin Hørthe Omdal · Core QA, Opera Software · http://opera.com /

Received on Friday, 3 February 2012 00:40:32 UTC