http/cookies: New module for parsing, serializing, and storing cookies #79

RyanSquared · 2017-08-10T05:58:23Z

No description provided.

daurnimator · 2017-08-10T06:03:36Z

http/cookies.lua

+	local domain = request.headers:get("host")
+	local key, value, matched_cookie = assert(http_patts.Set_Cookie:match(
+		text_cookie))
+	-- allow custom time for specs to not error


I don't think time should default to anything.

daurnimator · 2017-08-10T06:04:19Z

http/cookies.lua

+
+local function parse_set_cookie(text_cookie, request, time)
+	local domain = request.headers:get("host")
+	local key, value, matched_cookie = assert(http_patts.Set_Cookie:match(


The lpeg pattern needs to be anchored

daurnimator · 2017-08-10T06:04:30Z

http/cookies.lua

+local function parse_set_cookie(text_cookie, request, time)
+	local domain = request.headers:get("host")
+	local key, value, matched_cookie = assert(http_patts.Set_Cookie:match(
+		text_cookie))


Please don't wrap lines

daurnimator · 2017-08-10T06:04:39Z

http/cookies.lua

+		creation = time;
+		last_access = time;
+		persistent =
+			not not (matched_cookie.expires or matched_cookie["max-age"]);


don't wrap lines

daurnimator · 2017-08-10T06:05:25Z

http/cookies.lua

+		value = value;
+		host_only = not not matched_cookie.domain
+	}
+	cookie.indexname = cookie.domain .."|".. cookie.path .."|".. cookie.key


I'd prefer this was a function.

And what stops path or key from containing a |?

Changed to a function and changed the separator to \1. However, I don't see much of a use for it and might remove it if I don't see anything that uses it.

Good idea :) you shouldn't need this at all.

daurnimator · 2017-08-10T06:06:30Z

http/cookies.lua

+		text_cookie))
+	-- allow custom time for specs to not error
+	time = time or os.time()
+	local cookie = {


One intention of http_patts.Set_Cookie was that the output would be usable as a cookie directly.

To set defaults, use a metatable with __index.

19:40 <~daurnimator> ryan: right. but you can modify the table returned by the pattern
19:42 <~daurnimator> though I guess you might not want to do that
19:42 <~daurnimator> as the pattern can return arbitrary keys
19:42 <~daurnimator> so maybe it's fine as it is...
19:44 <~ryan> another issue is, converting it just ended up as the same line count because most of the items have defaults
19:44 <~ryan> plus the fact that there's so many different naming conventions
19:45 <~ryan> httponly, ["max-age"], same_site
19:46 <~daurnimator> ryan: I was more thinking about saving the extra allocation
19:46 <~ryan> yeah.
19:46 <~daurnimator> but yeah; in this case not worth it
19:46 <~ryan> i think the way i did it is better though
19:47 <~ryan> most of the keys are new keys, so doing it by appending on the old table means reallocating a lot
19:47 <~ryan> the new table means that all but one field are preallocated
19:48 <~ryan> which i actually just fixed to make it so all fields are prealloced

daurnimator · 2017-08-10T06:12:46Z

http/cookies.lua

+end
+
+local cookiejar_methods = {}
+local cookiejar_mt = {__index = cookiejar_methods}


Put __index on new line.

Don't forget a __name

daurnimator · 2017-08-10T06:13:36Z

http/cookies.lua

+local cookiejar_mt = {__index = cookiejar_methods}
+
+local function new_cookiejar(psl_object)
+	psl_object = psl_object or psl.builtin()


set this default in cookiejar_methods.

Also use psl.latest() if available rather than builtin

daurnimator · 2017-08-10T06:14:00Z

http/cookies.lua

+	return setmetatable({cookies={}, psl = psl_object}, cookiejar_mt)
+end
+
+function cookiejar_methods:add(cookie)


Should take time as argument

Technically, the cookie is "created" when it's parsed - which is also where other values relating to the time are set. I put in a time value here; however, according to the RFC it should be automatically generated instead of able to be variable (see the line below).

daurnimator · 2017-08-10T06:17:02Z

http/cookies.lua

+		table.insert(cookies, 1, cookie)
+	end
+
+	if not cookies[domain] then


Structure this code a little differently:

local by_domain = cookies[domain] if not by_domain then by_domain = {} cookies[domain] = by_domain end local by_path = by_domain[path] if not by_path then by_path = {} by_domain[path] = by_path end by_path[key] = cookie

daurnimator · 2017-08-10T06:20:50Z

http/cookies.lua

+	return returned_cookies
+end
+
+function cookiejar_methods:get_by_indexname(name)


This is hugely expensive splitting it apart, you only need a :get(domain, path, key)

daurnimator · 2017-08-10T06:23:16Z

http/cookies.lua

+local util = require "http.util"
+local psl = require "psl"
+
+local function parse_set_cookie(text_cookie, request, time)


Instead of passing in request here, consider passing in host and path?

daurnimator · 2017-08-10T06:25:35Z

http/cookies.lua

+	end
+
+	-- check all paths and flatten into a list of sets
+	for _path, set in pairs(self.cookies[domain]) do


Avoid local vars starting with _ unless it is literally _

daurnimator · 2017-08-10T06:37:26Z

http/cookies.lua

+	else -- luacheck: ignore
+		-- ::TODO:: make use of `expires` cookie value
+	end
+	local same_site do


No need for do: you don't introduce any temporary vars to the scope

daurnimator · 2017-08-10T06:38:46Z

http/cookies.lua

+		cookie[#cookie + 1] = "SameSite=" .. data.same_site:gsub("^.",
+			string.upper)
+	end
+	return table.concat(cookie, "; ")


Instead of passing "; " to table.concat we could prefix all the keys, e.g. cookie[#cookie + 1] = "; HttpOnly"

daurnimator · 2017-08-10T06:40:06Z

http/cookies.lua

+	if data.http_only then
+		cookie[#cookie + 1] = "HttpOnly"
+	end
+	if data.same_site then


Should this check be == nil? false might be permited?

There is no case in which it can be false. Your patterns only give true:
https://github.com/daurnimator/lpeg_patterns/blob/master/lpeg_patterns/http.lua#L372

Okay. I was imagining what I user might try and call this with.
Reading https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-4.1 it doesn't look like a value of true (which is what the pattern produces when no attribute-value is present) should be coerced to Strict.

Right, so I've done a bit more looking into it - I forget exactly where I found the "boolean" version but I remember finding it at least twice. However, none of those were the RFCs and I don't really see a version of the RFC that allows for a boolean-form. Should I just drop the boolean format?

Should I just drop the boolean format?

Yep.

Add a comment pointing to https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-4.1 too so next person to come along knows where to look

Comment added and "unbooleanified".

daurnimator · 2017-08-17T09:41:10Z

http/cookies.lua

-	local domain = request.headers:get("host")
-	local key, value, matched_cookie = assert(http_patts.Set_Cookie:match(
-		text_cookie))
+	local key, value, matched_cookie = assert(http_patts.Set_Cookie:match(text_cookie, 1))


What is the 1 for?

daurnimator · 2017-08-17T09:42:32Z

http/cookies.lua

+		same_site = matched_cookie.same_site;
+	}
+	if matched_cookie["max-age"] then
+		cookie.expires = time + tonumber(matched_cookie["max-age"])


Watch out for tonumber parsing formats other than decimal.

RFC 6265 section 5.2.2 says it should only be in base10 DIGIT with an optional negative, so I'm adding some code to properly handle it that way.

daurnimator · 2017-08-17T09:43:17Z

http/cookies.lua

+local function bake_cookie(data)
+	assert(data.key, "cookie must contain a `key` field")
+	assert(type(data.key) == "string", "`key` field for cookie must be string")
+	assert(data.value, "cookie must contain a `value` field")


Remove this line now that you have the type check below

daurnimator · 2017-08-17T09:43:45Z

http/cookies.lua

+		local v
+		if data.same_site:lower() == "strict" then
+			v = "; SameSite=Strict"
+		elseif data.same_site == "Lax" then


No :lower() here?

Thanks, totally missed that.

daurnimator · 2017-08-17T09:44:05Z

http/cookies.lua

+end
+
+local function iterate_cookies(cookie)
+	return pairs(assert(http_patts.Cookie:match(cookie, 1)))


You made a comment on a similar match about anchoring, so I added it here as well.

Adding a one here is not anchoring. anchoring is adding a *lpeg.P(-1) to the pattern before use so that you can't have trailing junk.

daurnimator · 2017-08-17T09:44:51Z

http/cookies.lua

+
+local function parse_cookies(cookie)
+	local cookies = {}
+	for k, v in iterate_cookies(cookie) do


The pattern already returns a table; why are you copying it?

I'm not copying the table. The iterate_cookies function returns a pairs() iterator after asserting that the cookie does parse correctly (which I added an error message to, after reading this, in case parsing fails)

Yes. However you're effectively copying the cookie in here with the cookies[k] = v

daurnimator · 2017-08-17T09:45:39Z

http/cookies.lua

+}
+
+local function new_cookiejar(psl_object)
+	return setmetatable({cookies={}, psl_object = psl_object}, cookiejar_mt)


I don't think the constructor needs to take a psl_object

daurnimator · 2017-08-17T09:53:50Z

http/cookies.lua

+
+function cookiejar_methods:get(domain, path, key)
+	local cookies = self.cookies
+	if cookies[domain] then


Please restructure

daurnimator · 2017-08-17T10:03:36Z

http/cookies.lua

+			local cookie = cookies[i]
+			cookies[i] = nil
+			cookies[cookie.domain][cookie.path][cookie.key] = nil
+			if not next(cookies[cookie.domain][cookie.path]) then


Cache these indexing operations in a local

daurnimator · 2017-08-17T10:16:38Z

http/cookies.lua

+	for _, cookie in pairs(sets) do
+		if not cookie.host_only then
+			if self.psl_object:is_cookie_domain_acceptable(domain,
+				cookie.domain) then


daurnimator · 2017-08-17T10:17:45Z

http/cookies.lua

+function cookiejar_methods:serialize_cookies_for(domain, path, secure)
+	-- explicitly check for secure; the other two will fail if given bad args
+	assert(type(secure) == "boolean", "expected boolean for `secure`")
+	local cookies = {}


Don't initialise before it's used

…xing

daurnimator · 2017-08-18T00:09:47Z

http/cookies.lua

+			-- representable expiration time.
+			cookie.expires = 0
+		else
+			cookie.expires = time + tonumber(age)


be careful with tonumber.

daurnimator · 2017-08-18T00:10:09Z

http/cookies.lua

+	local age = matched_cookie["max-age"]
+	if age then
+		local negative = age:match("^-")
+		if negative then


Should probably validate it as a number first?

daurnimator · 2017-08-18T00:12:55Z

http/cookies.lua

+
+	-- check all paths and flatten into a list of sets
+	local sets = {}
+	for stored_, set in pairs(self.cookies[domain]) do


Why stored_ with an underscore?

I think I might've messed up refactoring something on that.

daurnimator · 2017-08-28T00:39:21Z

http/cookies.lua

+	end
+	local by_path = by_domain[path]
+	if not by_path then
+		return


return nil vs nothing

daurnimator · 2017-08-28T00:39:32Z

http/cookies.lua

+end
+
+function cookiejar_methods:get(domain, path, key)
+	local cookies = self.cookies


No need to localise cookies

daurnimator · 2017-08-28T00:40:25Z

http/cookies.lua

+end
+
+function cookiejar_methods:remove_expired(time)
+	self:remove_cookies(get_expired(self, time))


Could inline :remove_cookies into here.

I'd rather not, given you should still be able to remove cookies from other methods - such as if they're not persistent and a session is closed.

daurnimator · 2017-08-28T00:42:52Z

http/cookies.lua

+			cookie.expires = time + tonumber(match, 10)
+		end
+	else -- luacheck: ignore
+		-- ::TODO:: make use of `expires` cookie value


Plans to implement this TODO?

Yes, after I've got all the other bugs done and I think of a decent way to do it. I plan on getting rid of every 'TODO' before it's merged.

Sorry, didn't see "plans", saw "plan". I'm thinking just use an RFC pattern to match the values and then use os.time() on the values, but I need to look on what common implementations support for patterns.

(as a note, while it's on my mind, there's several various patterns that could match the "time", which makes it difficult to match - it should be mentioned somewhere)

@daurnimator if you plan to build off of this one, this should be the only thing missing outside of optimizing the sequence for a bheap. more documentation would be nice but i can add that later

daurnimator · 2017-08-28T00:48:13Z

http/cookies.lua

+	local n = #cookies
+	-- remove cookies requiring secure connections on insecure connections
+	for index, cookie in pairs(cookies) do
+		if cookie.secure and not secure then


Do this check when adding; don't go through making holes that you later need to fixup...

….cookies once

daurnimator · 2017-10-19T02:54:03Z

You'll also need to install libpsl in the travis file.

daurnimator · 2017-10-19T03:40:10Z

:( looks like trusty and precise (ubuntu versions supported by travis) don't have libpsl.
https://packages.ubuntu.com/search?suite=default&section=all&arch=any&keywords=libpsl&searchon=names

RyanSquared · 2017-10-19T14:08:07Z

Yeah, found that out after spending what feels like forever pulling down the Docker image. Would any other GitHub-supported CI have something we can use for it? Alternatively, can we get Travis CI working on a later Ubuntu version?

RyanSquared · 2018-02-06T14:31:15Z

http/cookies.lua

+		end
+	end
+
+	local old_cookie = self:get(cookie.domain, cookie.path, cookie.key)


TODO: return index with self:get() to optimize self:remove_cookie()?

not reasonable because :get() doesn't iterate

RyanSquared · 2018-02-06T14:32:09Z

http/cookies.lua

+		local cookie_exp_time = cookie.expires
+		local inserted = false
+		for i=1, #cookies do
+			-- insert into first spot where cookie expires after


TODO: In-depth review and add more comments

RyanSquared · 2018-02-06T14:33:15Z

http/cookies.lua

+		end
+	else
+		cookie.expires = math.huge
+		table.insert(cookies, 1, cookie)


TODO: Add explanation; the "older" a nonpermanent cookie is, the more likely it should be nuked, hence why it's put in position 1

RyanSquared · 2018-02-06T14:43:16Z

http/cookies.lua

+
+function cookiejar_methods:remove_cookie(cookie)
+	local cookies = self.cookies
+	for i=1, #cookies do


TODO: Implement faster search; since cookies are sorted using time, it's possible to optimize by "leaping" through until the time is - either less than or greater than, can't recall ATM - and then go back one "leap" and going from there.

RyanSquared · 2018-02-06T15:02:02Z

http/cookies.lua

+local function serialize_cookies(cookies)
+	local out_values = {}
+	for _, cookie in pairs(cookies) do
+		out_values[#out_values + 1] = cookie.key .. "=" .. cookie.value


TODO: Use a value n to avoid getting length every time, then increment every time

See #79

http/cookies: Initial commit

0735c90

RyanSquared force-pushed the master branch from 253a4b3 to 0735c90 Compare August 10, 2017 06:01

daurnimator reviewed Aug 10, 2017

View reviewed changes

RyanSquared added 2 commits August 10, 2017 01:17

rockspec: Add psl to required modules

90bba9a

http/cookies: Only use times passed to parser

6eb208c

daurnimator requested changes Aug 10, 2017

View reviewed changes

daurnimator reviewed Aug 10, 2017

View reviewed changes

RyanSquared added 2 commits August 11, 2017 16:42

http/cookies: Optimize and fix issues

c277d33

spec/cookies_spec: Fix to be compatible with latest http.cookies module

f0cdf81

daurnimator reviewed Aug 17, 2017

View reviewed changes

RyanSquared added 3 commits August 17, 2017 19:03

rockspec: Add http.cookies module

2e53462

http/cookies: More optimizing and tweaks

0451752

http/cookies: Change structure to use locals instead of repeated inde…

7f7ebf9

…xing

daurnimator reviewed Aug 18, 2017

View reviewed changes

daurnimator reviewed Aug 28, 2017

View reviewed changes

http/cookies.lua Outdated

end

local by_path = by_domain[path]

if not by_path then

return

Copy link

Owner

daurnimator Aug 28, 2017

Choose a reason for hiding this comment

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

return nil vs nothing

daurnimator reviewed Aug 28, 2017

View reviewed changes

http/cookies: Preallocate expires field

334db6c

daurnimator added the WIP label Aug 28, 2017

RyanSquared added 2 commits August 27, 2017 20:05

http/cookies: fix issues

c3239b9

http/cookies: Optimize :remove_cookies() by only passing through self…

aeac218

….cookies once

RyanSquared added 2 commits October 18, 2017 22:18

.travis.yml: Add libpsl-dev as installed apt package

e6abaa6

.travis.yml: Fix dependency libpsl-dev

29bda47

RyanSquared commented Feb 6, 2018

View reviewed changes

daurnimator added a commit that referenced this pull request Jul 15, 2018

http/cookies.lua: Add :bake method based on @RyanSquared's code

16dece8

See #79

daurnimator mentioned this pull request Jul 15, 2018

Cookie Management #117

Merged

daurnimator added a commit that referenced this pull request Jul 23, 2018

http/cookie: Add :bake method based on @RyanSquared's code

fda2a40

See #79

daurnimator added a commit that referenced this pull request Aug 6, 2018

http/cookie: Add :bake method based on @RyanSquared's code

ef181c7

See #79

daurnimator added a commit that referenced this pull request Aug 6, 2018

http/cookie: Add :bake method based on @RyanSquared's code

0bedb34

See #79

daurnimator added a commit that referenced this pull request Aug 6, 2018

http/cookie: Add :bake method based on @RyanSquared's code

cbf685e

See #79

daurnimator added a commit that referenced this pull request Aug 12, 2018

http/cookie: Add bake function based on @RyanSquared's code

8df5bc7

See #79

daurnimator added a commit that referenced this pull request Aug 12, 2018

http/cookie: Add bake function based on @RyanSquared's code

5fd008b

See #79

daurnimator added a commit that referenced this pull request Aug 12, 2018

http/cookie: Add bake function based on @RyanSquared's code

1a0ae39

See #79

daurnimator added a commit that referenced this pull request Aug 12, 2018

http/cookie: Add bake function based on @RyanSquared's code

f611bf0

See #79

daurnimator closed this in #117 Aug 12, 2018

Uh oh!

http/cookies: New module for parsing, serializing, and storing cookies #79

http/cookies: New module for parsing, serializing, and storing cookies #79

Uh oh!

Conversation

RyanSquared commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daurnimator Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daurnimator Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daurnimator Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

daurnimator Aug 10, 2017 •

edited

Loading

daurnimator Aug 10, 2017 •

edited

Loading

daurnimator Aug 17, 2017 •

edited

Loading