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 infrastructure for testing origin policy #21705

Merged
merged 6 commits into from Feb 19, 2020
Merged

Add infrastructure for testing origin policy #21705

merged 6 commits into from Feb 19, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Feb 10, 2020

This adds 99 new subdomains for testing origin policy on, without interfering with other tests. It also configures /.well-known/origin-policy to use a Python script handler, instead of a static file handler, to allow varying responses. Closes #20773.

RFC: web-platform-tests/rfcs#44

@jgraham
Copy link
Contributor

jgraham commented Feb 11, 2020

This kind of change requires an RFC since it affects integrators.

@domenic domenic changed the title Add new subdomains for testing origin policy Add infrastructure for testing origin policy Feb 11, 2020
@domenic
Copy link
Member Author

domenic commented Feb 11, 2020

RFC made: web-platform-tests/rfcs#44

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21705 February 11, 2020 17:17 Inactive
@jgraham
Copy link
Contributor

jgraham commented Feb 11, 2020

Thanks

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21705 February 12, 2020 16:57 Inactive
@domenic
Copy link
Member Author

domenic commented Feb 18, 2020

I'll rebase this in the hopes that maybe the CI will start being green...

This adds 99 new subdomains for testing origin policy on, without interfering with other tests. It also configures `/.well-known/origin-policy` to use a Python script handler, instead of a static file handler, to allow varying responses. Closes #20773.

RFC: web-platform-tests/rfcs#44
@jgraham
Copy link
Contributor

jgraham commented Feb 18, 2020

It looks like there's a meaningful failure in the unitests:

=================================== FAILURES ===================================
___________________________ test_config_json_length ____________________________
    def test_config_json_length():
        # we serialize the config as JSON for pytestrunner and put it in an env
        # variable, which on Windows must have a length <= 0x7FFF (int16)
        with ConfigBuilder() as c:
            data = json.dumps(c.as_dict())
>       assert len(data) <= 0x7FFF
E       assert 42392 <= 32767
E        +  where 42392 = len('{"ports": {"https": [8443], "ws": [55777], "h2": [9000], "wss": [48573], "http": [8000, 38419]}, "not_domains": {"": ...w1.www2", "op9", "op8", "op3", "op2", "op1", "op7", "op6", "op5", "op4"], "doc_root": "/home/test/web-platform-tests"}')

Also I think we need to regenerate the certificate so that the domains work over https. I do not at this moment remember the right way to do that, but I'll try to.

@domenic
Copy link
Member Author

domenic commented Feb 18, 2020

Oh dang, thanks, I never know when the CI actually has errors versus sporadic failures.

I'm not sure how to fix that test... it seems like this legitimately causes a config to exceed a limit. I'll poke around, but ideas would also be welcome.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21705 February 18, 2020 17:33 Inactive
@jgraham
Copy link
Contributor

jgraham commented Feb 18, 2020

186d18f has docs on how to regenerate the certs.

The length limit here seems to be a windows limitation with environment variables. I guess we could do something mad, like allow the value to be spread over multiple environment variables and piece them back together. I don't know if there's a more sane suggestion.

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Feb 18, 2020

What's that line from BSG? "All this has happened before, and all this will happen again"?

#12461 is the last time this happened. We tried to add ~300 subdomains, and hit the same environment variable problem. At the time, the fix was to just add less.

We seem to dump the config as JSON in tools/wptrunner/wptrunner/executors/pytestrunner/runner.py, as 'WD_SERVER_CONFIG', and read it in webdriver/tests/support/fixtures.py. So any splitting up of the config would have to happen there, albeit that would be a bit of an ugly hack.

That said, https://devblogs.microsoft.com/oldnewthing/?p=15083 implies that the entire block of env variables has a shared limit, so that may not even help.

Backing up a bit however, perhaps we can just drop some of the bigger bits of data when setting WD_SERVER_CONFIG? If this is only used for webdriver tests (ohhh thats what the WD stands for), does webdriver need the expanded list of subdomains?

@domenic
Copy link
Member Author

domenic commented Feb 18, 2020

Backing up a bit however, perhaps we can just drop some of the bigger bits of data when setting WD_SERVER_CONFIG? If this is only used for webdriver tests (ohhh thats what the WD stands for), does webdriver need the expanded list of subdomains?

This is worth trying. I don't currently have any plans to use web driver stuff in origin policy tests, so hopefully it'll be OK.

Thanks for the pointer to the code; that's invaluable information for trying to make this work.

@wpt-pr-bot wpt-pr-bot added the wptrunner The automated test runner, commonly called through ./wpt run label Feb 18, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21705 February 18, 2020 20:34 Inactive
@domenic
Copy link
Member Author

domenic commented Feb 18, 2020

All checks have passed, woohoo!! Ready for review... I imagine my Python could probably use some critiquing.

tools/wptserve/wptserve/config.py Outdated Show resolved Hide resolved
tools/wptserve/wptserve/config.py Outdated Show resolved Hide resolved
tools/serve/serve.py Outdated Show resolved Hide resolved
tools/serve/serve.py Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21705 February 19, 2020 16:25 Inactive
def as_dict_for_wd_env_variable(self):
result = self.as_dict()

for key in [("subdomains",), ("domains", "alt"), ("domains", ""), ("all_domains", "alt"), ("all_domains", ""), ("domains_set",), ("all_domains_set",)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing colon at the end of this line is causing CI failures.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-21705 February 19, 2020 16:34 Inactive
@domenic domenic merged commit 644a206 into master Feb 19, 2020
@domenic domenic deleted the add-op-domains branch February 19, 2020 18:57
@Hexcles
Copy link
Member

Hexcles commented Feb 19, 2020

Woohoo! Thanks for following through with it, Domenic!

pull bot pushed a commit to FreddyZeng/chromium that referenced this pull request Feb 20, 2020
This CL cherry picks the core part of
web-platform-tests/wpt#21705 into blinkpy.

See README.chromium for more context.

Bug: 1042034
Change-Id: I9045c9e2987ef4b22e722f934d23037635000874
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2064783
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742854}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
certs infra serve wptrunner The automated test runner, commonly called through ./wpt run wptserve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to test origin policy?
5 participants