Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add referrer policy tests for loading the various subresources via a srcdoc iframe. #2851

Closed

Conversation

bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Apr 20, 2016

No description provided.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/6407

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @kristijanburnik.

@bzbarsky
Copy link
Contributor Author

@kristijanburnik could you take a look, please? I wasn't sure what the best way to add this was; whether we want more subresource types or something else.

Note that ideally we would also add tests for resources in srcdoc in srcdoc in normal document, to check that UAs walk up the srcdoc chain....

And we probably also need tests for sandboxed iframes; while that's out of the scope of what I'm trying to do here, it might affect how we approach setting this up. Adding three more subresource types for every existing type seems a bit weird to me, but right now the "how to load" functions are just keyed off subresource type...

@bzbarsky
Copy link
Contributor Author

Oh, and the CI failures are for path length; we can sort that out once we know whether we're doing a new subresource type or not, since that will affect filenames.

@bzbarsky
Copy link
Contributor Author

@mikewest I'm told @kristijanburnik is no longer active on this stuff, so you own these tests. Can you take a look at the above questions, please? And update OWNERS? ;)

@mikewest mikewest self-assigned this Apr 26, 2016
@mikewest
Copy link
Member

Hooray. I own things. Yes, I'll look at these in the morning and try to remember how all of it works.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 4, 2016

@mikewest It's been a week. Ping?

@hillbrad
Copy link
Contributor

Looking at this - sorry for the delay - there are still a number of these tests that use the CSP delivery method, which I believe is no longer current?

https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-delivery

@bzbarsky
Copy link
Contributor Author

I have no idea. Given my experience with the similarly written mixed-content tests, there are likely all sorts of bugs in the tests themselves, even if we ignore spec changes...

mikewest added a commit that referenced this pull request Sep 9, 2016
This patch teaches the 'img-tag'-style tests to load an image both
in the top-level document, and inside a 'srcdoc' frame. The referrer
should be the same in both places.

Addresses #2851.
@mikewest
Copy link
Member

mikewest commented Sep 9, 2016

I apologize; I didn't get back to this right away, and then it fell off my radar completely.

I think the approach is reasonable in the short term, but, as you note, adds a lot of tests. Since we should probably be doing srcdoc tests (and iframe@sandbox tests, etc) for resource types other than images, perhaps it makes sense to extend the general test framework to coalesce these new tests into the existing subresource tests without adding a few hundred new tests.

I don't think it's fair to ask you to do that refactoring after ~4 months, though, so I've uploaded a pass at it to https://github.com/w3c/web-platform-tests/tree/referrer-policy. WDYT?

@kristijanburnik
Copy link
Contributor

One does not necessarily need to add a lot of tests. Even a single test can
be generated by a single spec.json rule.

The generating framework allows for suppressing (excluded_tests) or
specifying explicit scenario values (i.e. other than * expansion).

It's up to you if you choose to be more general/specific or prefer
whitelisting to blacklisting. :-)

This is all documented here:
https://github.com/w3c/web-platform-tests/blob/master/referrer-policy/README.md

On Sep 9, 2016 3:20 PM, "Mike West" notifications@github.com wrote:

I apologize; I didn't get back to this right away, and then it fell off my
radar completely.

I think the approach is reasonable in the short term, but, as you note,
adds a lot of tests. Since we should probably be doing srcdoc tests (and
iframe@sandbox tests, etc) for resource types other than images, perhaps
it makes sense to extend the general test framework to coalesce these new
tests into the existing subresource tests without adding a few hundred new
tests.

I don't think it's fair to ask you to do that refactoring after ~4 months,
though, so I've uploaded a pass at it to https://github.com/w3c/web-
platform-tests/tree/referrer-policy. WDYT?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2851 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHmrGawHtd6UwUCLrVw9N3xwCwWzoDm5ks5qoV0HgaJpZM4ILVdx
.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Sep 9, 2016

WDYT?

I have a hard time telling whether it works or not, because I have vague memories of the test harness mutating the server state, such that you couldn't do two loads from a single test. But maybe that was the mixed-content tests instead... Have you tested the patch?

@mikewest
Copy link
Member

Have you tested the patch?

I have. It works for me locally. I'll turn it into a real PR.

mikewest added a commit that referenced this pull request Oct 14, 2016
* Referrer Policy: Test image loads inside srcdoc frames.

This patch teaches the 'img-tag'-style tests to load an image both
in the top-level document, and inside a 'srcdoc' frame. The referrer
should be the same in both places.

Addresses #2851.

* fixup whitespace

* fixup console.log
@foolip
Copy link
Member

foolip commented Aug 16, 2018

This PR has been sitting without activity for over a year. @mikewest, #3700 says it addresses this PR, but there are lots of changes in this PR and only small changes in the other?

@bzbarsky, are there things in this PR that still aren't covered in master?

Copy link
Contributor

@jeisinger jeisinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with nits

function loadImage(src, callback, attributes) {
var image = new Image();

function loadImage(src, callback, attributes, isSrcDoc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. other arguments use_this_style, so I'd change this to is_src_doc

@@ -73,14 +90,14 @@ function decodeImageData(rgba) {
return JSON.parse(string_data);
}

function decodeImage(url, callback, referrer_policy) {
function decodeImage(url, callback, referrer_policy, isSrcDoc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@bzbarsky
Copy link
Contributor Author

@bzbarsky, are there things in this PR that still aren't covered in master?

@foolip, I don't know. It's been a long time (even longer since you asked the question, I know) and at this point I'd have to go and reverse-engineer this harness yet again to figure out what's going on.

@foolip
Copy link
Member

foolip commented Jul 31, 2019

Alright, I guess practically speaking the choice is between taking all of these tests and accepting that there's some or a lot of overlap, or throwing them away to avoid possible overlap. I don't know the existing tests or what's in this PR, so @bzbarsky whatever you prefer I can rubberstamp and merge :)

@bzbarsky
Copy link
Contributor Author

I think we should just throw out this PR at this point. It's not clear to me that it can even be merged as-is...

@foolip foolip closed this Jul 31, 2019
@foolip
Copy link
Member

foolip commented Jul 31, 2019

It had merge conflicts, so it couldn't have been merged without some work. I've closed it now.

@bzbarsky bzbarsky deleted the add-srcdoc-tests branch February 6, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants