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

Cookie Management #117

Merged
merged 40 commits into from Aug 12, 2018
Merged

Cookie Management #117

merged 40 commits into from Aug 12, 2018

Conversation

daurnimator
Copy link
Owner

Fixes #6
Closes #79

I took a few pieces out of #79, but the initial version of this is based on code I wrote on a plane in January 2017. It tries to keep close to RFC 6265. The later revisions are cribbing from https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02

Brings in two new dependencies: lua-psl and binaryheap

http/cookies.lua Outdated
cmp_cookie.secure
-- 3. Their domain domain-matches the domain of the newly-created
-- cookie, or vice-versa.
and domain_match(cookie.domain, cmp_cookie.domain)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Move this to the outer loop.

http/cookies.lua Outdated
local now = self.time()
local list = {}
local n = 0
for domain, domain_cookies in pairs(self.domains) do
Copy link
Owner Author

Choose a reason for hiding this comment

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

Needs optimising: can index by each req_domain domain+superdomain?

@daurnimator
Copy link
Owner Author

Need to add libpsl to travis-ci.

@daurnimator

This comment has been minimized.

@daurnimator daurnimator force-pushed the cookies branch 4 times, most recently from 4a9d3a4 to f422407 Compare July 26, 2018 12:21
@daurnimator daurnimator force-pushed the cookies branch 3 times, most recently from 01da506 to 5ca16d0 Compare August 5, 2018 16:32
@coveralls
Copy link

coveralls commented Aug 5, 2018

Coverage Status

Coverage decreased (-1.03%) to 86.863% when pulling 23af6d1 on cookies into 921e04d on master.

@daurnimator daurnimator force-pushed the cookies branch 8 times, most recently from b000e2a to 1e3d101 Compare August 12, 2018 14:27
canonicalise_host = function(str)
-- fail on non-ascii chars
if str:find("[^%p%w]") then
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

add error string on why failed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sadly libpsl doesn't tell us why we failed; so can't do :(

-- If the cookie store contains a cookie with the same name,
-- domain, and path as the newly created cookie:
if old_cookie then
-- If the newly created cookie was received from a "non-HTTP"
Copy link
Contributor

Choose a reason for hiding this comment

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

what is a non-HTTP API? AFAICT this just meant that the cookie is not exposed to JavaScript.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked a bit more into it, I think it could be that "non-HTTP" means that, for example, JavaScript couldn't overwrite the cookie.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Correct. e.g. if it was set with document.cookie.
Think of it as a flag that decides if the user can change it vs only transport

http/cookie.lua Outdated
end

-- Convert the cookie-domain to lower case.
domain = domain:lower()
Copy link
Contributor

Choose a reason for hiding this comment

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

should this not be the canonicalized URL instead?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm not sure.... the specs seem quiet on the issue?

http/cookie.lua Outdated

local function cookie_match(cookie, req_domain, req_is_http, req_is_secure, req_is_safe_method, req_site_for_cookies, req_is_top_level)
if cookie.host_only then -- Either:
-- The cookie's host-only-flag is true and the canonicalized
Copy link
Contributor

Choose a reason for hiding this comment

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

req_domain in this case isn't canonicalized

Copy link
Owner Author

Choose a reason for hiding this comment

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

How so? It's canonicalized at the call site. (and cookie_match isn't public)

Copy link
Contributor

Choose a reason for hiding this comment

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

That would make sense then, but it's not obvious it's canonicalized elsewhere. Perhaps add a comment on that?

The lua compiler will turn it into a constant
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.

Cookie Management
3 participants