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

Update cookie-related OWNERS and READMEs #7531

Merged
merged 2 commits into from
Nov 16, 2017
Merged

Update cookie-related OWNERS and READMEs #7531

merged 2 commits into from
Nov 16, 2017

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 1, 2017

No description provided.

@wpt-pr-bot
Copy link
Collaborator

There are no owners for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

@foolip
Copy link
Member Author

foolip commented Oct 1, 2017

@mikewest, what would be some other good owners for this?

Can you add a README.md explaining which specs this covers? Is it just https://w3c.github.io/webappsec-csp/cookies/, or also https://tools.ietf.org/html/rfc6265?

Finally, I noticed that #4423 deleted some of your tests. Did you know, and is that OK? If it's not OK, then @Hexcles or I should do some kind of audit for tests that we've exported and then had removed.

@ghost
Copy link

ghost commented Oct 1, 2017

Build PASSED

Started: 2017-11-16 22:53:25
Finished: 2017-11-16 22:57:02

View more information about this build on:

@Hexcles
Copy link
Member

Hexcles commented Oct 2, 2017

#4423 is a Gecko export of https://bugzilla.mozilla.org/show_bug.cgi?id=976073.

Looks like the tests were deleted unintentionally and later restored in https://bugzilla.mozilla.org/show_bug.cgi?id=1316532, which was not upstreamed. Could you take a look @jgraham ?

@foolip
Copy link
Member Author

foolip commented Oct 5, 2017

@Hexcles, @jgraham is away during October, so I guess we'll need to fix this. Maybe noticing this kind of thing could be part of the audit script, where if files we've exported are deleted by something other than a new export are flagged for review. Most will be fine, but some might be accidents.

For now I'll revert the files back into existence.

foolip added a commit that referenced this pull request Oct 5, 2017
This reverts commit 05e8454.

This appears to have been an accident, see
#7531 (comment)
Hexcles pushed a commit that referenced this pull request Oct 5, 2017
This reverts commit 05e8454.

This appears to have been an accident, see
#7531 (comment)
@foolip
Copy link
Member Author

foolip commented Nov 15, 2017

Now I'm also wondering about the relationship between this and cookie-store/. @bsittler, do you know?

@bsittler
Copy link
Contributor

These appear to be tests (originally by @tefn3849 I believe - are you a suitable owner?) verifying behavior specified in https://datatracker.ietf.org/doc/draft-ietf-httpbis-cookie-alone/ by @mikewest

It bears only tangential relationship to the https://github.com/WICG/cookie-store proposal is that the latter (a proposed new JS API for cookie operations) mentions enforcing the restrictions of the former too, in that case with promise rejections when non-conforming operations are attempted.

@bsittler
Copy link
Contributor

This is a wild guess, but perhaps @annevk could knows of a suitable owner for these tests?

@annevk
Copy link
Member

annevk commented Nov 15, 2017

I would have suggested @bsittler and @mikewest...

At some point @inikulin was interested (see whatwg/html#804), but not sure if that's still the case.

@inikulin
Copy link
Member

I always willing to help. What are duties with this one?

@bsittler
Copy link
Contributor

FWIW I am also willing to help, but like @inikulin I would like to understand what's involved. I actually misunderstood earlier and thought you were asking about who wrote the tests

@bsittler
Copy link
Contributor

/cc @pwnall in case interested

@pwnall
Copy link
Contributor

pwnall commented Nov 16, 2017

@foolip I can help, sign me up.

I skimmed through the tests. I'm reasonably familiar with the matter, having read the relevant RFCs in order to implement Chrome's Async Cookies API. I also had to / will have to write or review similar tests for Async Cookies.

@annevk
Copy link
Member

annevk commented Nov 16, 2017

@inikulin @bsittler ideally OWNERS review incoming tests for the directory they're owner for and address outstanding issues with those tests. As part of this you'll get automatically pinged for each new PR against that directory.

@foolip foolip changed the title Create cookies/OWNERS Update cookie-related OWNERS and READMEs Nov 16, 2017
@foolip
Copy link
Member Author

foolip commented Nov 16, 2017

OK, I've tried to write sensible OWNERS and README.md for both cookie-related directories now. I added just the people I could see in git log. @inikulin, should I add you somewhere as well?

Can @bsittler, @pwnall or @mikewest review this now? As long as it's an improvement that's good enough.

@foolip foolip requested a review from pwnall November 16, 2017 10:04
Copy link
Contributor

@pwnall pwnall left a comment

Choose a reason for hiding this comment

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

Thank you very much, especially for the update to cookie-store/README.md

@@ -0,0 +1,2 @@
This directory contains tests for
[Leave Secure Cookies Alone](https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01).
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like https://github.com/w3c/web-platform-tests/blob/master/cookies/path/match.html covers a bit more than the IETF draft you linked. I think this is a reasonable first step and we can rely on @mikewest's guidance for the next step 😸

@pwnall
Copy link
Contributor

pwnall commented Nov 16, 2017

I'll wait for a response from @inikulin for a bit, and then merge. @foolip - feel free to merge if you want to get this off your cache.

@mikewest
Copy link
Member

I was imagining that we'd eventually migrate the IETF test suite for the cookies RFC into this repository as part of putting together RFC6265bis. I am astoundingly slow at that (and it's currently impossible to test SameSite because of #2669), but it seems like a reasonable long-term goal.

@inikulin
Copy link
Member

@pwnall @foolip You can count me in. I'd like to work on IETF suite port at some point.

jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
This reverts commit 05e8454.

This appears to have been an accident, see
web-platform-tests#7531 (comment)
@foolip
Copy link
Member Author

foolip commented Nov 16, 2017

@mikewest, since you approved I take it you think the README is OK even though it'll hopefully be made inaccurate. If there's a list of RFCs that should be covered by this directory, please update :)

@inikulin, I'll add you to cookies/OWNERS.

@foolip foolip merged commit 11abef7 into master Nov 16, 2017
@foolip foolip deleted the cookies-owners branch November 16, 2017 22:58
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.

8 participants