Adding redirect count tests in case of cross origin redirections #4102

youennf · 2016-10-28T10:15:40Z

Splitting redirect.py in two so that the scripts are easier to understand

wpt-pr-bot · 2016-10-28T10:15:41Z

Reviewers for this pull request are: @jdm and @youennf.

annevk · 2017-02-20T17:39:31Z

Could you rebase this? I'll review.

ghost · 2017-02-20T18:26:30Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision d4b422b
Using browser at version BuildID 20170123125947; SourceStamp 36486fdc3813ef7943ae5b07b4128866d1938a6c
Starting 10 test iterations
All results were stable

All results

5 tests ran

/fetch/api/redirect/redirect-count-cross-origin.html

Subtest	Results
	OK
`Redirect 21 times, going to cross-origin after 10`	PASS
`Redirect 20 times, going to cross-origin after 10 with preflight`	FAIL
`Redirect 20 times, last redirection to cross-origin`	PASS
`Redirect 21 times, last redirection to cross-origin with preflight`	PASS
`Redirect 21 times, going to cross-origin after 10 with preflight`	PASS
`Redirect 20 times, last redirection to cross-origin with preflight`	PASS
`Redirect 20 times, going to cross-origin after 10`	PASS
`Redirect 21 times, last redirection to cross-origin`	PASS

/fetch/api/redirect/redirect-count.html

Subtest	Results
	OK
`Redirect 308 20 times`	PASS
`Redirect 308 21 times`	PASS
`Redirect 301 21 times`	PASS
`Redirect 302 21 times`	PASS
`Redirect 307 21 times`	PASS
`Redirect 307 20 times`	PASS
`Redirect 302 20 times`	PASS
`Redirect 303 21 times`	PASS
`Redirect 303 20 times`	PASS
`Redirect 301 20 times`	PASS

/fetch/api/redirect/redirect-count-cross-origin-worker.html

Subtest	Results
	OK
`Redirect 21 times, going to cross-origin after 10`	PASS
`Redirect 20 times, going to cross-origin after 10 with preflight`	FAIL
`Redirect 20 times, last redirection to cross-origin`	PASS
`Redirect 21 times, last redirection to cross-origin with preflight`	PASS
`Redirect 21 times, going to cross-origin after 10 with preflight`	PASS
`Redirect 20 times, last redirection to cross-origin with preflight`	PASS
`Redirect 20 times, going to cross-origin after 10`	PASS
`Redirect 21 times, last redirection to cross-origin`	PASS

/fetch/api/redirect/redirect-count-worker.html

Subtest	Results
	OK
`Redirect 308 20 times`	PASS
`Redirect 308 21 times`	PASS
`Redirect 301 21 times`	PASS
`Redirect 302 21 times`	PASS
`Redirect 307 21 times`	PASS
`Redirect 307 20 times`	PASS
`Redirect 302 20 times`	PASS
`Redirect 303 21 times`	PASS
`Redirect 303 20 times`	PASS
`Redirect 301 20 times`	PASS

/fetch/api/redirect/redirect-schemes.html

Subtest	Results
	OK
`Fetch: handling different schemes in redirects`	PASS
`Fetch: handling different schemes in redirects 1`	PASS
`Fetch: handling different schemes in redirects 2`	PASS
`Fetch: handling different schemes in redirects 3`	PASS
`Fetch: handling different schemes in redirects 4`	PASS
`Fetch: handling different schemes in redirects 5`	PASS

ghost · 2017-02-20T18:26:30Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision d4b422b
Using browser at version 58.0.3013.3 dev
Starting 10 test iterations
All results were stable

All results

5 tests ran

/fetch/api/redirect/redirect-count-cross-origin.html

Subtest	Results
	OK
`Redirect 21 times, going to cross-origin after 10`	FAIL
`Redirect 20 times, going to cross-origin after 10 with preflight`	PASS
`Redirect 20 times, last redirection to cross-origin`	PASS
`Redirect 21 times, last redirection to cross-origin with preflight`	FAIL
`Redirect 21 times, going to cross-origin after 10 with preflight`	FAIL
`Redirect 20 times, last redirection to cross-origin with preflight`	PASS
`Redirect 20 times, going to cross-origin after 10`	PASS
`Redirect 21 times, last redirection to cross-origin`	FAIL

/fetch/api/redirect/redirect-count.html

Subtest	Results
	OK
`Redirect 308 20 times`	PASS
`Redirect 308 21 times`	PASS
`Redirect 301 21 times`	PASS
`Redirect 302 21 times`	PASS
`Redirect 307 21 times`	PASS
`Redirect 307 20 times`	PASS
`Redirect 302 20 times`	PASS
`Redirect 303 21 times`	PASS
`Redirect 303 20 times`	PASS
`Redirect 301 20 times`	PASS

/fetch/api/redirect/redirect-count-cross-origin-worker.html

Subtest	Results
	OK
`Redirect 21 times, going to cross-origin after 10`	FAIL
`Redirect 20 times, going to cross-origin after 10 with preflight`	PASS
`Redirect 20 times, last redirection to cross-origin`	PASS
`Redirect 21 times, last redirection to cross-origin with preflight`	FAIL
`Redirect 21 times, going to cross-origin after 10 with preflight`	FAIL
`Redirect 20 times, last redirection to cross-origin with preflight`	PASS
`Redirect 20 times, going to cross-origin after 10`	PASS
`Redirect 21 times, last redirection to cross-origin`	FAIL

/fetch/api/redirect/redirect-count-worker.html

Subtest	Results
	OK
`Redirect 308 20 times`	PASS
`Redirect 308 21 times`	PASS
`Redirect 301 21 times`	PASS
`Redirect 302 21 times`	PASS
`Redirect 307 21 times`	PASS
`Redirect 307 20 times`	PASS
`Redirect 302 20 times`	PASS
`Redirect 303 21 times`	PASS
`Redirect 303 20 times`	PASS
`Redirect 301 20 times`	PASS

/fetch/api/redirect/redirect-schemes.html

Subtest	Results
	OK
`Fetch: handling different schemes in redirects`	PASS
`Fetch: handling different schemes in redirects 1`	PASS
`Fetch: handling different schemes in redirects 2`	PASS
`Fetch: handling different schemes in redirects 3`	PASS
`Fetch: handling different schemes in redirects 4`	PASS
`Fetch: handling different schemes in redirects 5`	PASS

annevk · 2017-02-21T08:56:40Z

These look good. They also provide tests for #204, right?

annevk · 2017-02-21T13:55:46Z

I guess it's not testing that since cors/redirect-preflight.htm passes in Safari TP and is obviously wrong per allowing redirects after preflights.

How does requirePreflight in your test work then?

youennf · 2017-02-22T05:11:01Z

Link (#204) is probably not the right one.
requirePreflight should add a not-safe header and trigger preflight.
I do not think these tests check redirections as reponses to preflight.

Some of these tests should trigger using preflight after redirections.
Maybe we should add a check that preflight is done like for cors/redirect-preflight.htm.

Not sure why you say cors/redirect-preflight.htm is wrong. Can you elaborate?

annevk · 2017-02-22T08:24:32Z

Sorry, I meant whatwg/fetch#204. We changed CORS to allow redirects after a successful preflight, which would typically (not if you redirect to yourself I suppose) result in another preflight for the new destination.

That's why that test is wrong. Unfortunately when we made the change in Fetch we didn't have the policy of updating tests, but I'm trying to clean all that up now.

annevk

Having looked at these tests once more I found several mistakes and I also still don't really understand the max1/max2 parameters and how they actually end up doing anything.

The other questions still stand of course.

annevk · 2017-03-08T15:05:56Z

fetch/api/redirect/redirect-count-cross-origin.js

+  urlParameters += allow_headers;
+  urlParameters += "&location=" + encodeURIComponent(redirectLocation + "?token=" + uuid_token + "&max_age=0&max_count=" + (maxCount1 + maxCount2) + allow_headers);
+
+  var maxCount = maxCount1 + maxCount2;


This should move up so you don't need to do this addition twice.

annevk · 2017-03-08T15:08:05Z

fetch/api/redirect/redirect-count-cross-origin.js

+  urlParameters += "&redirect_status=" + redirectStatus;
+  urlParameters += "&max_count=" + maxCount1;
+  urlParameters += allow_headers;
+  urlParameters += "&location=" + encodeURIComponent(redirectLocation + "?token=" + uuid_token + "&max_age=0&max_count=" + (maxCount1 + maxCount2) + allow_headers);


It seems you're adding allow_headers and max_age twice. Also max_age is never used in the Python resource.

You also set max_count twice.

They are added to the first looped URL and the second looped URL.
Might need to check though whether this is working using a web inspector.

Sorry, yeah, that probably works.

But max_age should probably still be removed.

annevk · 2017-03-08T15:09:35Z

fetch/api/redirect/redirect-count-cross-origin.js

+  var url = redirectUrl;
+  promise_test(function(test) {
+    return fetch(RESOURCES_DIR + "clean-stash.py?token=" + uuid_token).then(function(resp) {
+      assert_equals(resp.status, 200, "Clean stash response's status is 200");


It's always 200, no? The only difference seems to be whether the contents are "1" or "0".

It is just a safety check that the overall test env is correct.

annevk · 2017-03-08T15:10:29Z

fetch/api/redirect/redirect-count-cross-origin.js

+  importScripts("/common/get-host-info.sub.js");
+}
+
+function redirectCount(desc, redirectUrl, redirectLocation, redirectStatus, maxCount1, maxCount2, requirePreflight)


redirectUrl and redirectLocation could probably be removed since they're always the same.

They should not always be the same.
The principle of the test is that we will redirect macCount1 times on redirectUrl.
On the last redirect to redirectUrl, we should be redirected to redirectLocation (bad name probably)
And we again are redirected macCount2 on redirectLocation.

But you always pass the same values as arguments here. There is no method call to redirectCount where they have a different value as far as I can tell.

redirect-count.py is expected to do that (is_final_redirection check or something like that)

Maybe you're misunderstanding my feedback. My point was that they don't have to be arguments to the redirectCount() function as the redirectCount() function is never invoked with different values for them. Surely redirect-count.py is not invoking JavaScript?

annevk · 2017-03-08T15:10:56Z

fetch/api/redirect/redirect-count-cross-origin.js

+redirectCount("Redirect 21 times, last redirection to cross-origin with preflight", redirUrl, crossOriginRedirUrl, 301, 20, 1, true);
+
+redirectCount("Redirect 20 times, going to cross-origin after 10 with preflight", redirUrl, crossOriginRedirUrl, 303, 10, 10, true);
+redirectCount("Redirect 21 times, going to cross-origin after 10 with preflight", redirUrl, crossOriginRedirUrl, 303, 10, 11, true);


Should we also test 307/308?

annevk · 2017-03-08T15:11:16Z

fetch/api/redirect/redirect-count-cross-origin.js

+redirectCount("Redirect 20 times, going to cross-origin after 10 with preflight", redirUrl, crossOriginRedirUrl, 303, 10, 10, true);
+redirectCount("Redirect 21 times, going to cross-origin after 10 with preflight", redirUrl, crossOriginRedirUrl, 303, 10, 11, true);
+
+done();


Can you explain this call? I never quite understood that in the Fetch API tests.

Isn't it needed for worker-environment tests?

I think it works without.

Is it not for Single Page Tests?

annevk · 2017-03-08T15:12:31Z

fetch/api/resources/redirect-count.py

+            headers.append(("Access-Control-Allow-Headers", request.GET['allow_headers']))
+        stashed_data['preflight'] = "1"
+        #Preflight is not redirected: return 200
+        if not "redirect_preflight" in request.GET:


Is redirect_preflight ever passed?

Do not remember. I can check to remove it.

annevk · 2017-03-08T15:16:33Z

fetch/api/redirect/redirect-count-cross-origin.js

+  urlParameters += "&location=" + encodeURIComponent(redirectLocation + "?token=" + uuid_token + "&max_age=0&max_count=" + (maxCount1 + maxCount2) + allow_headers);
+
+  var maxCount = maxCount1 + maxCount2;
+  var url = redirectUrl;


This seems rather redundant renaming?

annevk · 2017-03-14T07:32:56Z

Also, let's start using the .any.js convention as per #5129.

foolip · 2018-08-16T12:00:14Z

@youennf, this PR is one of a few that's been inactive for over a year. Do you plan to take this up again?

wpt-pr-bot added the fetch label Oct 28, 2016

annevk assigned youennf Feb 20, 2017

Adding redirect count tests in case of cross origin redirections

c1f2a66

youennf force-pushed the add-redirect-count-cross-origin-tests branch from 0cff7cc to c1f2a66 Compare February 20, 2017 18:16

ferjm mentioned this pull request Mar 2, 2017

Allow for redirects after a CORS-preflight servo/servo#14519

Closed

annevk requested changes Mar 8, 2017

View reviewed changes

annevk mentioned this pull request Mar 14, 2017

Fetch: use .any.js syntax #5129

Merged

annevk added the status:needs-changes label Jan 31, 2018

wpt-pr-bot requested review from domfarolino, jdm, mnot and yutakahirano August 16, 2018 12:00

Adding redirect count tests in case of cross origin redirections #4102

Are you sure you want to change the base?

Adding redirect count tests in case of cross origin redirections #4102

Conversation

youennf commented Oct 28, 2016 • edited by wpt-pr-bot Loading

wpt-pr-bot commented Oct 28, 2016

annevk commented Feb 20, 2017

ghost commented Feb 20, 2017

Firefox (nightly channel)

All results

ghost commented Feb 20, 2017

Chrome (unstable channel)

All results

annevk commented Feb 21, 2017

annevk commented Feb 21, 2017

youennf commented Feb 22, 2017

annevk commented Feb 22, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Mar 14, 2017

foolip commented Aug 16, 2018

youennf commented Oct 28, 2016 •

edited by wpt-pr-bot

Loading