Skip to content

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

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

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0735c90
http/cookies: Initial commit
RyanSquared Aug 10, 2017
90bba9a
rockspec: Add psl to required modules
RyanSquared Aug 10, 2017
6eb208c
http/cookies: Only use times passed to parser
RyanSquared Aug 10, 2017
c277d33
http/cookies: Optimize and fix issues
RyanSquared Aug 11, 2017
f0cdf81
spec/cookies_spec: Fix to be compatible with latest http.cookies module
RyanSquared Aug 11, 2017
2e53462
rockspec: Add `http.cookies` module
RyanSquared Aug 18, 2017
0451752
http/cookies: More optimizing and tweaks
RyanSquared Aug 18, 2017
7f7ebf9
http/cookies: Change structure to use locals instead of repeated inde…
RyanSquared Aug 18, 2017
d4b7408
http/cookies: add matches to tonumber(), tweak iterate_cookies() -> m…
RyanSquared Aug 18, 2017
c432b54
spec/cookies_spec: rename iterate_cookies to match_cookies
RyanSquared Aug 18, 2017
a31a95c
http/cookies: Only call assert() once
RyanSquared Aug 18, 2017
99fd1a8
README: Add lua-psl to dependencies list
RyanSquared Aug 18, 2017
10624a0
http/cookies: Soft error instead of throw, one call to match(), make …
RyanSquared Aug 28, 2017
d1678f8
http/cookies: Fix undefined behavior with adding to table during pairs()
RyanSquared Aug 28, 2017
c87a96b
http/cookies: Pass `, 10` to tonumber()
RyanSquared Aug 28, 2017
334db6c
http/cookies: Preallocate `expires` field
RyanSquared Aug 28, 2017
c3239b9
http/cookies: fix issues
RyanSquared Aug 28, 2017
aeac218
http/cookies: Optimize :remove_cookies() by only passing through self…
RyanSquared Oct 18, 2017
e6abaa6
.travis.yml: Add libpsl-dev as installed `apt` package
RyanSquared Oct 19, 2017
29bda47
.travis.yml: Fix dependency libpsl-dev
RyanSquared Oct 19, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ This will automatically install run-time lua dependencies for you.
- [lpeg](http://www.inf.puc-rio.br/~roberto/lpeg/lpeg.html)
- [lpeg_patterns](https://github.com/daurnimator/lpeg_patterns) >= 0.3
- [fifo](https://github.com/daurnimator/fifo.lua)
- [lua-psl](https://github.com/daurnimator/lua-psl)

To use gzip compression you need **one** of:

Expand Down
2 changes: 2 additions & 0 deletions http-scm-0.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ dependencies = {
"lpeg";
"lpeg_patterns >= 0.3";
"fifo";
"psl";
}

build = {
type = "builtin";
modules = {
["http.bit"] = "http/bit.lua";
["http.client"] = "http/client.lua";
["http.cookies"] = "http/cookies.lua";
["http.connection_common"] = "http/connection_common.lua";
["http.h1_connection"] = "http/h1_connection.lua";
["http.h1_reason_phrases"] = "http/h1_reason_phrases.lua";
Expand Down
356 changes: 356 additions & 0 deletions http/cookies.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,356 @@
local http_patts = require "lpeg_patterns.http"
local util = require "http.util"
local lpeg = require "lpeg"
local psl = require "psl"

local Set_Cookie_anchored = http_patts.Set_Cookie * lpeg.P(-1)

local function parse_set_cookie(text_cookie, host, path, time)
assert(time, "missing time value for cookie parsing")
local key, value, matched_cookie = Set_Cookie_anchored:match(text_cookie)
if not key then
return nil, "cookie did not properly parse"
end
local cookie = {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

creation = time;
last_access = time;
persistent = not not (matched_cookie.expires or matched_cookie["max-age"]);
domain = matched_cookie.domain or host;
path = matched_cookie.path or path;
secure = matched_cookie.secure or false;
http_only = matched_cookie.httponly or false;
key = key;
value = value;
host_only = not not matched_cookie.domain;
same_site = matched_cookie.same_site;
}
local age = matched_cookie["max-age"]
if age then
local is_negative, match = age:match("^(-?)(%d+)$")
Copy link
Owner

Choose a reason for hiding this comment

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

- needs to be escaped in a pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for anchoring, ^ is not a valid character in the RFC.

Copy link
Owner

Choose a reason for hiding this comment

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

huh? what RFC?

if is_negative then
-- RFC 6265 section 5.2.2 - if the value when converted to an
-- integer is negative, the expiration should be the earliest
-- representable expiration time.
cookie.expires = 0
elseif not match then
return nil, "expected [-]DIGIT* for max-age field"
else
cookie.expires = time + tonumber(match, 10)
end
else -- luacheck: ignore
-- ::TODO:: make use of `expires` cookie value
Copy link
Owner

Choose a reason for hiding this comment

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

Plans to implement this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

end
return cookie
end

local function bake_cookie(data)
assert(type(data.key) == "string", "`key` field for cookie must be string")
assert(type(data.value) == "string", "`value` field for cookie must be string")
local cookie = {data.key .. "=" .. data.value}
if data.expires then
cookie[#cookie + 1] = "; Expires=" .. util.imf_date(data.expires)
end
if data.max_age then
cookie[#cookie + 1] = "; Max-Age=" .. string.format("%d", data.max_age)
end
if data.domain then
cookie[#cookie + 1] = "; Domain=" .. data.domain
end
if data.path then
cookie[#cookie + 1] = "; Path=" .. util.encodeURI(data.path)
end
if data.secure then
cookie[#cookie + 1] = "; Secure"
end
if data.http_only then
cookie[#cookie + 1] = "; HttpOnly"
end
-- This component is not a part of the RFC 6265 specification for the
-- headers, but is instead from a draft of another RFC that builds on the
-- original one.
-- https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-4.1
if data.same_site then
Copy link
Owner

@daurnimator daurnimator Aug 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

@daurnimator daurnimator Aug 10, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added and "unbooleanified".

local v
if data.same_site:lower() == "strict" then
v = "; SameSite=Strict"
elseif data.same_site:lower() == "lax" then
v = "; SameSite=Lax"
else
error('invalid value for same_site, expected "Strict" or "Lax"')
end
cookie[#cookie + 1] = v
end
return table.concat(cookie)
end

local Cookie_anchored = http_patts.Cookie * lpeg.P(-1)

local function match_cookies(cookie)
local match = Cookie_anchored:match(cookie)
if match then
return match
else
return nil, "improper Cookie header format"
end
end

local function parse_cookies(cookie)
local cookies = match_cookies(cookie)
local to_add = {}
for k, v in pairs(cookies) do
Copy link
Owner

Choose a reason for hiding this comment

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

Iterating over something while adding to it is undefined behaviour.

to_add[#to_add + 1] = {k, v}
end
for _, v in ipairs(to_add) do
cookies[#cookies + 1] = v
Copy link
Owner

Choose a reason for hiding this comment

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

Use the index given by ipairs to cut down on length operations

end
table.sort(cookies, function(t1, t2)
return t1[1] < t2[1]
end)
return cookies
end

local cookiejar_methods = {}
if psl.latest then
cookiejar_methods.psl_object = psl.latest()
else
Copy link
Owner

Choose a reason for hiding this comment

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

Add comment about compat with old libpsl?

-- older versions of libpsl do not offer a `latest` list
cookiejar_methods.psl_object = psl.builtin()
end
local cookiejar_mt = {
__name = "http.cookies.cookiejar";
__index = cookiejar_methods;
}

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

function cookiejar_methods:add(cookie, time)
cookie.last_access = time or os.time()
local domain, path, key = cookie.domain, cookie.path, cookie.key
local cookies = self.cookies
if cookies[domain] and cookies[domain][path] then
local old_cookie = cookies[domain][path][key]
if old_cookie then
cookie.creation = old_cookie.creation
end
end

local old_cookie = self:get(cookie.domain, cookie.path, cookie.key)
Copy link
Contributor Author

@RyanSquared RyanSquared Feb 6, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not reasonable because :get() doesn't iterate

if old_cookie then
self:remove_cookie(old_cookie)
end
if cookie.persistent then
local cookie_exp_time = cookie.expires
local inserted = false
for i=1, #cookies do
-- insert into first spot where cookie expires after
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: In-depth review and add more comments

if cookies[i].expires < cookie_exp_time then
inserted = true
table.insert(cookies, i, cookie)
end
end
if not inserted then
cookies[#cookies + 1] = cookie
end
else
cookie.expires = math.huge
table.insert(cookies, 1, cookie)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

end

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
end
Copy link
Owner

Choose a reason for hiding this comment

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

return true


function cookiejar_methods:get(domain, path, key)
local cookies = self.cookies
Copy link
Owner

Choose a reason for hiding this comment

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

No need to localise cookies

local by_domain = cookies[domain]
if not by_domain then
return
end
local by_path = by_domain[path]
if not by_path then
return
Copy link
Owner

Choose a reason for hiding this comment

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

return nil vs nothing

end
return by_path[key]
end

local function clear_holes(tbl, n)
local start_hole = 0
for i=1, n do
if tbl[i] and start_hole ~= 0 then
tbl[start_hole] = tbl[i]
tbl[i] = nil
start_hole = start_hole + 1
elseif not tbl[i] and start_hole == 0 then
start_hole = i
end
end
end

function cookiejar_methods:remove_cookie(cookie)
local cookies = self.cookies
for i=1, #cookies do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if cookie == cookies[i] then
table.remove(cookies, i)
cookies[cookie.domain][cookie.path][cookie.key] = nil
return true
end
end
return false
end

function cookiejar_methods:remove_cookies(cookies)
local cookie_hashes = {}
for _, key in pairs(cookies) do
cookie_hashes[key] = true
end
local s_cookies = self.cookies
local n = #s_cookies
for index, value in pairs(s_cookies) do
if cookie_hashes[value] then
s_cookies[index] = nil
local by_domain = s_cookies[value.domain]
local by_path = by_domain[value.path]
by_path[value.key] = nil
if not next(by_path) then
by_domain[value.path] = nil
if not next(by_domain) then
s_cookies[value.domain] = nil
end
end
end
end

clear_holes(s_cookies, n)
end

local function get_expired(jar, time)
time = time or os.time()
local cookies = jar.cookies
local returned_cookies = {}
for i=#cookies, 1, -1 do
local cookie = cookies[i]
if cookie.expires > time then
break
end
returned_cookies[#returned_cookies + 1] = cookie
end
return returned_cookies
end

function cookiejar_methods:remove_expired(time)
self:remove_cookies(get_expired(self, time))
Copy link
Owner

Choose a reason for hiding this comment

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

Could inline :remove_cookies into here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end

function cookiejar_methods:trim(size)
self:remove_expired()
local cookies = self.cookies
if #cookies > size then
for i=#cookies, size + 1, -1 do
local cookie = cookies[i]
cookies[i] = nil
local by_domain = cookies[cookie.domain]
local by_path = by_domain[cookie.path]
by_path[cookie.key] = nil
if not next(by_path) then
by_domain[cookie.path] = nil
if not next(by_domain) then
cookies[cookie.domain] = nil
end
end
end
end
end

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

Choose a reason for hiding this comment

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

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

end
return table.concat(out_values, "; ")
end

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`")

-- clear out expired cookies
self:remove_expired()

-- return empty table if no cookies are found
if not self.cookies[domain] then
return {}
end

-- check all paths and flatten into a list of sets
local sets = {}
for stored, set in pairs(self.cookies[domain]) do
if stored:sub(1, #path) == path then
for _, cookie in pairs(set) do
sets[#sets + 1] = cookie
end
end
end

-- sort as per RFC 6265 section 5.4 part 2; while it's not needed, it will
-- help with tests where values need to be reproducible
table.sort(sets, function(x, y)
if #x.path == #y.path then
return x.creation < y.creation
else
return #x.path > #y.path
end
end)

-- populate cookie list
local cookies = {}
for _, cookie in pairs(sets) do
if not cookie.host_only then
if self.psl_object:is_cookie_domain_acceptable(domain, cookie.domain) then
cookies[#cookies + 1] = cookie
end
elseif cookie.domain == domain then
cookies[#cookies + 1] = cookie
end
end

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
Copy link
Owner

Choose a reason for hiding this comment

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

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

cookies[index] = nil
end
end

-- update access time for each cookie
local time = os.time()
for _, cookie in pairs(cookies) do
cookie.last_access = time
end

clear_holes(cookies, n)
return serialize_cookies(cookies)
end

return {
match_cookies = match_cookies;
parse_set_cookie = parse_set_cookie;
bake_cookie = bake_cookie;
parse_cookies = parse_cookies;
serialize_cookies = serialize_cookies;
cookiejar = {
new = new_cookiejar;
methods = cookiejar_methods;
mt = cookiejar_mt;
};
}
Loading