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

Is support for extended attributes necessary? #3

Closed
domenic opened this issue Mar 4, 2016 · 14 comments
Closed

Is support for extended attributes necessary? #3

domenic opened this issue Mar 4, 2016 · 14 comments

Comments

@domenic
Copy link

domenic commented Mar 4, 2016

Currently .set( allows passing arbitrary options that set extended attributes. Is this a necessary capability? I assume you did this because it's possible with document.cookie?

I might move this to a second options bag instead of mixing it up inside the first one, if it is necessary.

@bsittler
Copy link
Contributor

bsittler commented Mar 4, 2016

Yes, this is the extensibility loophole in document.cookie writes and Set-Cookie. Why separate them? They aren't separate in Set-Cookie, nor are they separate in document.cookie writes (in both cases [ignored] extended attribute/value pairs may appear intermixed with "known" bare attributes/attribute-value pairs in any order)

@domenic
Copy link
Author

domenic commented Mar 4, 2016

I think separating the options that have an effect on normative behavior vs. those that are extensibility hooks makes a lot of sense to me. Or, disallowing them entirely unless people can present a use case.

The ordering issue is interesting, though. Note that ordering isn't completely preserved by ES object iteration; numeric keys will be moved first, for example.

@bsittler
Copy link
Contributor

bsittler commented Mar 4, 2016

Good point; also, for that matter, I'm not seeing any guidance in https://tools.ietf.org/html/rfc6265 on whether attribute order matters, nor on how an attribute occurring multiple times should be handled. My suspicion is that at least ordering of extended attributes relative to each other is not important. If this is reasonable, we could allow array-valued extended attributes.

Also, the grammar in the cookie RFC seems to require extended attributes to have values, but at least https://tools.ietf.org/html/draft-west-origin-cookies-01 adds an attribute with no value. I guess that if this API allows extended attributes it should permit them to be attribute-less.

I guess another possibility would be to default to not allowing any extended attributes, but do allow them if {[' allowUnknownAttributes ']: true} is present in options.

What do you think, @mikewest ?

@domenic
Copy link
Author

domenic commented Mar 5, 2016

The other problem I see with extended attributes via the friendly multimap API is that they are write-only. set(n, p, { extAttr: "foo" }) then get(n, p) will not give you back extAttr: "foo", if I am reading the code right.

@bsittler
Copy link
Contributor

bsittler commented Mar 5, 2016

No attributes are ever readable. Edit: Not even the non-extended ones.

@domenic
Copy link
Author

domenic commented Mar 5, 2016

OK. So extended attributes are neither readable nor do they have any normative impact. I think they should definitely not be settable through this API.

@bsittler
Copy link
Contributor

bsittler commented Mar 5, 2016

I think they are needed so that you can e.g. set an Origin cookie if your browser supports those (the setting interface for Origin cookies is Set-Cookie: with an Origin extended attribute with no value, so I believe document.cookie writes or navigator.cookies writes are the correct way to set them; it's only reading them that differs from regular cookies, where there will presumably be a document.originCookies and navigator.cookies.readOriginCookie{s} parallel to HTTP Origin-Cookie:)

Extended attributes unknown to the UA are intentionally fail-soft/ignorable rather than fail-hard/MustUnderstand, and I think that policy from Set-Cookie: should be reflected here too. That's how e.g. max-age support was added to document.cookie writes without forcing web developers to wrap every attempt in try/catch-fallback to old version. Instead cookie setting attempts with max-age and no expires got silently downgraded to session-lifetime in older browsers.

On Mar 4, 2016 20:09, "Domenic Denicola" notifications@github.com wrote:

OK. So extended attributes are neither readable nor do they have any
normative impact. I think they should definitely not be settable through
this API.


Reply to this email directly or view it on GitHub
#3 (comment)
.

@domenic
Copy link
Author

domenic commented Mar 5, 2016

It'd be better to introduce actual API for features that have normative impact, like origin cookies.

Of course we wouldn't throw for entries in the object literal that are unused; no web APIs do that. They only check for the presence of known entries.

@bsittler
Copy link
Contributor

bsittler commented Mar 7, 2016

I think that what we're actually modeling here is Set-Cookie. There already exist additional cookie attributes supported by some browsers but not yet all of them, e.g. Priority=Low|Medium|High https://groups.google.com/a/chromium.org/forum/m/#!topic/chromium-dev/xK4IJ1-5oJE

@domenic
Copy link
Author

domenic commented Mar 7, 2016

I don't think we should be modeling Set-Cookie; I think we should be modeling the process of adding cookies to browsers. Nonstandard attributes that only affect some browsers shouldn't sneak through in some extensibility dictionary.

At this point I feel like we're kind of deadlocked and need a third party to weigh in. Maybe @mikewest or @annevk.

@bsittler
Copy link
Contributor

bsittler commented Mar 7, 2016

Another attribute: (Chrome seems to support it, maybe others do too?)
https://tools.ietf.org/html/draft-west-first-party-cookies-06

@annevk
Copy link
Collaborator

annevk commented Mar 8, 2016

I would prefer only exposing a subset of Set-Cookie. E.g., we should not allow access to HttpOnly here. We should probably not allow the creation of insecure cookies.

@bsittler
Copy link
Contributor

bsittler commented Jul 27, 2016

On further though and after further discussion I am convinced that the approach you both advocate is the correct one, or at the very least the correct starting point for the new API. I've attempted to reflect that in the explainer, please take a look and share your thoughts/report issues/send pull requests

@bsittler
Copy link
Contributor

Fixed explainer link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants