Restyle the filter extension headers and markup

1 file changed, 183 insertions, 158 deletions

diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.lua
index 2553233..8c731a3 100644
--- a/custom/extensions/auth-file.lua
+++ b/custom/extensions/auth-file.lua
@@ -1,40 +1,28 @@
--- cgit auth-filter that gates repositories behind a login form and a signed
--- session cookie.
---
--- This is the FILE-BACKED variant. The user accounts, the groups, and the
--- per-repository access lists are read from files on disk, whose paths are set
--- among the configuration values below. Edit those files without touching this
--- script. This suits larger or externally managed user sets.
---
--- The companion auth-inline.lua behaves identically but keeps its accounts and
--- access lists inline in the script itself, which suits a small fixed set of
--- users.
---
--- Enable it in cgitrc with
---     auth-filter=lua:/path/to/auth-file.lua
---
--- SUPPORTED LUA
---
--- Lua 5.1, 5.2, 5.3, 5.4 and LuaJIT. Lua 5.5 is not supported, because luaossl
--- has no 5.5 build. Match the runtime to the Lua that cgit is built against.
---
--- HTTPS IS RECOMMENDED
---
--- Serve cgit over HTTPS. Terminate TLS at the web server in front of cgit. The
--- session cookie is marked Secure by default, so a browser only sends it back
--- over HTTPS. If you genuinely run cgit over plain HTTP with no TLS anywhere,
--- set cookie_insecure below, otherwise the cookie is never returned and login
--- appears to loop.
---
--- DEPENDENCIES
+-- A cgit auth filter that puts chosen repositories behind a login form and a
+-- signed session cookie. cgit consults it on every request once cgitrc names
+-- it with auth-filter=lua:/path/to/auth-file.lua, and it answers the
+-- authenticate-cookie, authenticate-post and body actions the filter API
+-- defines. This variant keeps the accounts, the groups and the per-repository
+-- access lists in files on disk, which suits a user set that is large or
+-- maintained by something else. The companion auth-inline.lua behaves
+-- identically but carries the same lists inside the script.
+
+-- The script runs on Lua 5.1, 5.2, 5.3, 5.4 and LuaJIT, and the runtime has to
+-- be the same Lua that cgit was built against. Lua 5.5 will not do, because
+-- luaossl has no 5.5 build.
 --
---     luaossl    OpenSSL binding, provides openssl.rand and openssl.hmac
---                <https://github.com/wahern/luaossl>
---     luaposix   POSIX binding, provides posix.sys.stat and posix.unistd
---                <https://github.com/luaposix/luaposix>
+-- Serve cgit over HTTPS and terminate TLS in the web server in front of it.
+-- The session cookie is marked Secure by default, so a browser only sends it
+-- back over HTTPS, and on an instance served over plain HTTP with no TLS
+-- anywhere the cookie never comes back and login appears to loop until
+-- cookie_insecure below is set.
 --
--- The reliable cross-platform install is LuaRocks, matched to your Lua
--- version. luaossl also needs the OpenSSL development headers present.
+-- Two libraries are needed. luaossl provides openssl.rand and openssl.hmac and
+-- lives at <https://github.com/wahern/luaossl>, and luaposix provides
+-- posix.sys.stat and posix.unistd and lives at
+-- <https://github.com/luaposix/luaposix>. The reliable cross-platform way to
+-- install them is LuaRocks matched to your Lua version, and luaossl also needs
+-- the OpenSSL development headers present.
 --
 --     # Debian and Ubuntu
 --     sudo apt install luarocks libssl-dev
@@ -54,152 +42,158 @@
 --     luarocks install luaossl OPENSSL_DIR="$(brew --prefix openssl)"
 --     luarocks install luaposix
 --
--- Some distributions also package these, for example lua-luaossl and lua-posix
--- on Debian. If you use a distribution package, make sure it is built for the
--- same Lua version as cgit.
+-- Some distributions package both as well, for example lua-luaossl and
+-- lua-posix on Debian, and such a package has to be built for the same Lua
+-- version as cgit.
 --
--- SECURITY NOTES
---
--- The cookie carries only a username with no server-side session store, so
--- deleting an account does not revoke a cookie already issued until it
--- expires, and instances that share a secret file accept each other's cookies.
--- The login form carries no CSRF token. Both are acceptable for gating read
--- access to a git browser. Weigh them before guarding anything more sensitive.
+-- The cookie carries only a user name and there is no server-side session
+-- store, so deleting an account does not revoke a cookie already issued until
+-- it expires, and instances that share a secret file accept each other's
+-- cookies. The login form carries no CSRF token. Both are acceptable for
+-- gating read access to a git browser, so weigh them before guarding anything
+-- more sensitive.
 
 local sysstat = require("posix.sys.stat")
 local unistd = require("posix.unistd")
 local rand = require("openssl.rand")
 local hmac = require("openssl.hmac")
 
--- Configuration, edit these values. Nothing below them needs changing for
--- ordinary use.
+-- The values that follow are the configuration and are meant to be edited.
+-- Nothing below them needs changing for ordinary use.
 
--- Accounts, one per line, as username:hash. Generate a hash with
+-- Accounts live one per line as username:hash. Generate a hash with
 --     mkpasswd -m sha-512 -R 300000
 -- This file should not be world-readable.
 local users_filename = "/etc/cgit-auth/users"
 
--- Group membership, one per line, as groupname:user1,user2,user3,...
+-- Group membership lives one per line as groupname:user1,user2,user3 and so
+-- on.
 local groups_filename = "/etc/cgit-auth/groups"
 
--- Per-repository access, one per line, as reponame:group1,group2,...
--- A repository listed here is protected. One not listed is public.
+-- Per-repository access lives one per line as reponame:group1,group2 and so
+-- on. A repository named here is protected and one that is not named is
+-- public. The repository name has to match exactly, while group and user names
+-- match whatever their case.
 local repos_filename = "/etc/cgit-auth/repos"
 
--- Where the cookie-signing secret is stored. It is created on first use. It
--- must be persistent and writable by cgit. Prefer a path OUTSIDE the cache
+-- Where the cookie-signing secret is stored, created on first use. It must be
+-- persistent and writable by cgit, and it is worth keeping outside the cache
 -- root, because pruning the cache would delete a secret kept inside it and
 -- invalidate every live session. This file should not be world-readable.
 local secret_filename = "/var/lib/cgit/auth-secret"
 
--- How long a login stays valid, in seconds. Default one week.
+-- How long a login stays valid, in seconds. The default is one week.
 local session_seconds = 7 * 24 * 60 * 60
 
--- Name of the session cookie.
 local cookie_name = "cgitauth"
 
--- Path the cookie is scoped to. "/" covers the whole host. Set it to the cgit
--- root to scope the cookie more tightly.
+-- "/" covers the whole host. Set this to the cgit root to scope the cookie
+-- more tightly.
 local cookie_path = "/"
 
--- Leave false so the cookie is marked Secure and only travels over HTTPS. Set
--- it true ONLY if cgit is served over plain HTTP with no TLS anywhere, see the
--- HTTPS note in the header.
+-- Leave this false so the cookie is marked Secure and only travels over HTTPS.
+-- Set it true only if cgit is served over plain HTTP with no TLS anywhere.
 local cookie_insecure = false
 
 -- A throwaway hash of the documented shape, used only to spend the same work
 -- on a missing account as on a present one, so a failed login does not reveal
--- by timing whether the username exists.
+-- by timing whether the user name exists.
 local dummy_hash = "$6$rounds=300000$0000000000000000$"
 
--- Module state shared across the open, write and close calls of one request.
+-- One request reaches this script as an open, a write and a close, so what the
+-- open decodes is kept here for the calls that follow.
 local action, http, cgit, post
 
--- Account and access-list storage. This is the ONLY part that differs from
--- auth-inline.lua. Swap these two functions to change where accounts live.
+-- The two lookups below, account_hash and repo_userset, are the only part of
+-- this script that differs from auth-inline.lua. Replacing them is all it
+-- takes to keep accounts somewhere else.
 
 local function trim(s)
 	return (string.gsub(s, "^%s*(.-)%s*$", "%1"))
 end
 
--- Return the stored password hash for a user, or nil. Reads the users file
--- fresh each call. A missing or unreadable file, and any unparsable line, are
--- skipped rather than fatal.
+local function add_names(list, set)
+	for name in string.gmatch(list, "([^,]+)") do
+		set[trim(name):lower()] = true
+	end
+end
+
+-- A missing or unreadable users file is not fatal, and neither is a line that
+-- does not parse, so a broken file turns every login down rather than failing
+-- the request outright.
 --
--- The hash is trimmed as well as the name. f:lines() strips the newline but
--- not a carriage return, so a users file saved with CRLF endings would
--- otherwise hand crypt a hash with a trailing \r and fail every login with
--- nothing in the log to say why.
+-- The hash is trimmed as well as the name because reading by line strips the
+-- newline but not a carriage return, so a users file saved with CRLF endings
+-- would otherwise hand crypt a hash with a trailing \r and fail every login
+-- with nothing in the log to say why.
 function account_hash(user)
 	if user == nil then
 		return nil
 	end
 	local wanted = user:lower()
-	local f = io.open(users_filename, "r")
-	if f == nil then
+	local users_file = io.open(users_filename, "r")
+	if users_file == nil then
 		return nil
 	end
-	for line in f:lines() do
-		local u, h = string.match(line, "(.-):(.+)")
-		if u ~= nil and trim(u):lower() == wanted then
-			f:close()
-			return trim(h)
+	for line in users_file:lines() do
+		local name, hash = string.match(line, "(.-):(.+)")
+		if name ~= nil and trim(name):lower() == wanted then
+			users_file:close()
+			return trim(hash)
 		end
 	end
-	f:close()
+	users_file:close()
 	return nil
 end
 
--- Return the set of users allowed to access a repository, keyed by lowercased
--- username, or nil if the repository is not protected. A protected repository
--- whose groups resolve to no users returns an EMPTY table, which denies
--- everyone rather than falling through to public.
+-- The users allowed into a repository come back keyed by lowercased name, and
+-- a repository the repos file does not name comes back as nil. A protected
+-- repository whose groups resolve to nobody comes back as an empty table
+-- instead, so it denies everyone rather than falling through to public.
 function repo_userset(repo)
 	if repo == nil then
 		return nil
 	end
 	local groups = nil
-	local f = io.open(repos_filename, "r")
-	if f ~= nil then
-		for line in f:lines() do
-			local r, g = string.match(line, "(.-):(.+)")
-			if r ~= nil and trim(r) == repo then
+	local repos_file = io.open(repos_filename, "r")
+	if repos_file ~= nil then
+		for line in repos_file:lines() do
+			local name, list = string.match(line, "(.-):(.+)")
+			if name ~= nil and trim(name) == repo then
 				groups = {}
-				for group in string.gmatch(g, "([^,]+)") do
-					groups[trim(group):lower()] = true
-				end
+				add_names(list, groups)
 				break
 			end
 		end
-		f:close()
+		repos_file:close()
 	end
 	if groups == nil then
 		return nil
 	end
 	local users = {}
-	local gf = io.open(groups_filename, "r")
-	if gf ~= nil then
-		for line in gf:lines() do
-			local g, u = string.match(line, "(.-):(.+)")
-			if g ~= nil and groups[trim(g):lower()] then
-				for user in string.gmatch(u, "([^,]+)") do
-					users[trim(user):lower()] = true
-				end
+	local groups_file = io.open(groups_filename, "r")
+	if groups_file ~= nil then
+		for line in groups_file:lines() do
+			local name, list = string.match(line, "(.-):(.+)")
+			if name ~= nil and groups[trim(name):lower()] then
+				add_names(list, users)
 			end
 		end
-		gf:close()
+		groups_file:close()
 	end
 	return users
 end
 
--- Utility functions based on keplerproject/wsapi.
+-- The URL helpers below are adapted from keplerproject/wsapi.
 
 function url_decode(str)
 	if not str then
 		return ""
 	end
 	str = string.gsub(str, "+", " ")
-	str = string.gsub(str, "%%(%x%x)", function(h) return string.char(tonumber(h, 16)) end)
+	str = string.gsub(str, "%%(%x%x)", function(hex)
+		return string.char(tonumber(hex, 16))
+	end)
 	str = string.gsub(str, "\r\n", "\n")
 	return str
 end
@@ -209,30 +203,31 @@ function url_encode(str)
 		return ""
 	end
 	str = string.gsub(str, "\n", "\r\n")
-	str = string.gsub(str, "([^%w ])", function(c) return string.format("%%%02X", string.byte(c)) end)
+	str = string.gsub(str, "([^%w ])", function(char)
+		return string.format("%%%02X", string.byte(char))
+	end)
 	str = string.gsub(str, " ", "+")
 	return str
 end
 
 -- Parse an application/x-www-form-urlencoded body. A value may itself contain
 -- '=', for example a base64 password, so the value runs to the next '&'.
-function parse_qs(qs)
-	local tab = {}
-	for key, val in string.gmatch(qs or "", "([^&=]+)=([^&]*)") do
-		tab[url_decode(key)] = url_decode(val)
+function parse_query(query)
+	local params = {}
+	for key, value in string.gmatch(query or "", "([^&=]+)=([^&]*)") do
+		params[url_decode(key)] = url_decode(value)
 	end
-	return tab
+	return params
 end
 
--- Escape the Lua pattern magic characters, so a name is matched literally.
 local function pattern_escape(s)
 	return (string.gsub(s, "([%^%$%(%)%%%.%[%]%*%+%-%?])", "%%%1"))
 end
 
--- Return the value of the named cookie, or nil. The stored token was already
--- url-encoded by secure_value, so it is returned verbatim, which keeps the
--- write path (set_cookie) and the read path symmetric. Decoding it here would
--- break the signature check for any value carrying a percent escape.
+-- The stored token was already URL encoded by secure_value, so it comes back
+-- verbatim and the write path in set_cookie stays symmetric with this read
+-- path. Decoding it here would break the signature check for any value
+-- carrying a percent escape.
 --
 -- The name is escaped because it lands in a pattern. A cookie_name holding a
 -- magic character, say "cgit-auth", would otherwise read as a pattern and stop
@@ -242,16 +237,14 @@ function get_cookie(cookies, name)
 	return string.match(cookies, ";" .. pattern_escape(name) .. "=(.-);")
 end
 
-function tohex(b)
-	local x = ""
-	for i = 1, #b do
-		x = x .. string.format("%.2x", string.byte(b, i))
+function tohex(bytes)
+	local hex = ""
+	for i = 1, #bytes do
+		hex = hex .. string.format("%.2x", string.byte(bytes, i))
 	end
-	return x
+	return hex
 end
 
--- Cookie construction and validation helpers.
-
 local secret = nil
 
 -- Load the cookie-signing secret, creating it on first use. Failures raise,
@@ -263,21 +256,30 @@ function get_secret()
 	end
 	local secret_file = io.open(secret_filename, "r")
 	if secret_file == nil then
+		-- The secret is written under a tightened mask so it is not
+		-- created readable by anyone but the user cgit runs as, and
+		-- the old mask goes back on every way out.
 		local old_umask = sysstat.umask(63)
-		local temporary_filename = secret_filename .. ".tmp." .. tohex(rand.bytes(16))
+		local temporary_filename = secret_filename .. ".tmp." ..
+			tohex(rand.bytes(16))
 		local temporary_file = io.open(temporary_filename, "w")
 		if temporary_file == nil then
 			sysstat.umask(old_umask)
-			error("cgit auth: cannot create secret file " .. secret_filename)
+			error("cgit auth: cannot create secret file " ..
+				secret_filename)
 		end
 		local wrote = temporary_file:write(tohex(rand.bytes(32)))
 		local closed = temporary_file:close()
 		if not wrote or not closed then
 			os.remove(temporary_filename)
 			sysstat.umask(old_umask)
-			error("cgit auth: failed writing secret file " .. secret_filename)
+			error("cgit auth: failed writing secret file " ..
+				secret_filename)
 		end
-		unistd.link(temporary_filename, secret_filename) -- Intentionally fails if another worker won the race.
+		-- The link is meant to fail when another worker won the race,
+		-- which leaves that worker's secret in place rather than
+		-- replacing it and invalidating the sessions it just signed.
+		unistd.link(temporary_filename, secret_filename)
 		unistd.unlink(temporary_filename)
 		sysstat.umask(old_umask)
 		secret_file = io.open(secret_filename, "r")
@@ -289,7 +291,8 @@ function get_secret()
 	secret_file:close()
 	if secret == nil or secret:len() ~= 64 then
 		secret = nil
-		error("cgit auth: secret file " .. secret_filename .. " is malformed, expected 64 hex characters")
+		error("cgit auth: secret file " .. secret_filename ..
+			" is malformed, expected 64 hex characters")
 	end
 	return secret
 end
@@ -315,7 +318,7 @@ function validate_value(expected_field, cookie)
 	local field = ""
 	local expiration = 0
 	local salt = ""
-	local chmac = ""
+	local signature = ""
 
 	if cookie == nil or cookie:len() < 3 or cookie:sub(1, 1) == "|" then
 		return nil
@@ -327,9 +330,10 @@ function validate_value(expected_field, cookie)
 		elseif i == 1 then
 			value = component
 		elseif i == 2 then
-			-- The expiration must be a plain integer. Rejecting other forms
-			-- keeps the signed bytes canonical, since tonumber and tostring of
-			-- "1e9" or "100.0" differ across Lua versions.
+			-- The expiration must be a plain integer, since
+			-- tonumber and tostring of "1e9" or "100.0" differ
+			-- across Lua versions and the signed bytes have to
+			-- come back byte for byte.
 			if not string.match(component, "^%d+$") then
 				return nil
 			end
@@ -337,19 +341,22 @@ function validate_value(expected_field, cookie)
 		elseif i == 3 then
 			salt = component
 		elseif i == 4 then
-			chmac = component
+			signature = component
 		else
 			break
 		end
 		i = i + 1
 	end
 
-	if chmac == nil or chmac:len() == 0 then
+	if signature == nil or signature:len() == 0 then
 		return nil
 	end
 
-	-- Compare the HMAC without short-circuiting on the first mismatch.
-	if not constant_equals(chmac, tohex(hmac.new(get_secret(), "sha256"):final(field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt))) then
+	local payload = field .. "|" .. value .. "|" ..
+		tostring(expiration) .. "|" .. salt
+	local expected_signature =
+		tohex(hmac.new(get_secret(), "sha256"):final(payload))
+	if not constant_equals(signature, expected_signature) then
 		return nil
 	end
 
@@ -371,6 +378,8 @@ function validate_value(expected_field, cookie)
 	return decoded
 end
 
+-- The layout built here is what validate_value takes apart again, so the two
+-- have to move together.
 function secure_value(field, value, expiration)
 	if value == nil or value:len() <= 0 then
 		return ""
@@ -379,13 +388,15 @@ function secure_value(field, value, expiration)
 	local salt = tohex(rand.bytes(16))
 	value = url_encode(value)
 	field = url_encode(field)
-	local authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
-	authstr = authstr .. "|" .. tohex(hmac.new(get_secret(), "sha256"):final(authstr))
-	return authstr
+	local payload = field .. "|" .. value .. "|" ..
+		tostring(expiration) .. "|" .. salt
+	local signature = tohex(hmac.new(get_secret(), "sha256"):final(payload))
+	return payload .. "|" .. signature
 end
 
--- Strip control characters that could split an HTTP response header.
-function strip_ctl(s)
+-- A control character in a header value would let that value split the
+-- response, so it is dropped rather than escaped.
+function strip_controls(s)
 	return (string.gsub(s or "", "%c", ""))
 end
 
@@ -397,29 +408,33 @@ function constant_equals(a, b)
 	end
 	local diff = 0
 	for i = 1, #a do
-		local d = a:byte(i) - b:byte(i)
-		diff = diff + d * d
+		local delta = a:byte(i) - b:byte(i)
+		diff = diff + delta * delta
 	end
 	return diff == 0
 end
 
+-- An empty value is how a login is cleared, since Max-Age=0 tells the browser
+-- to drop the cookie it already holds.
 function set_cookie(cookie, value)
-	local attrs = "; HttpOnly; SameSite=Lax; Path=" .. cookie_path
+	local attributes = "; HttpOnly; SameSite=Lax; Path=" .. cookie_path
 	if not cookie_insecure then
-		attrs = attrs .. "; Secure"
+		attributes = attributes .. "; Secure"
 	end
 	if value == "" then
-		attrs = attrs .. "; Max-Age=0"
+		attributes = attributes .. "; Max-Age=0"
 	elseif session_seconds > 0 then
-		attrs = attrs .. "; Max-Age=" .. tostring(session_seconds)
+		attributes = attributes ..
+			"; Max-Age=" .. tostring(session_seconds)
 	end
-	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. attrs .. "\n")
+	html("Set-Cookie: " .. cookie .. "=" ..
+		strip_controls(value) .. attributes .. "\n")
 end
 
 function redirect_to(url)
 	html("Status: 302 Redirect\n")
 	html("Cache-Control: no-cache, no-store\n")
-	html("Location: " .. strip_ctl(url) .. "\n")
+	html("Location: " .. strip_controls(url) .. "\n")
 end
 
 function not_found()
@@ -427,9 +442,12 @@ function not_found()
 	html("Cache-Control: no-cache, no-store\n\n")
 end
 
--- Authentication actions. Identical to auth-inline.lua from here down.
+-- The three actions cgit can ask for follow, and they are the same in both
+-- variants, so a change to one belongs in the other.
 
--- Sets HTTP cookie headers based on post and sets up redirection.
+-- The redirect goes out before the password is checked, so a wrong password
+-- and a right one answer with the same status and location and differ only in
+-- the cookie.
 function authenticate_post()
 	local redirect = validate_value("redirect", post["redirect"])
 
@@ -454,7 +472,8 @@ function authenticate_post()
 	end
 
 	if ok then
-		set_cookie(cookie_name, secure_value("username", username, os.time() + session_seconds))
+		set_cookie(cookie_name, secure_value("username", username,
+			os.time() + session_seconds))
 	else
 		set_cookie(cookie_name, "")
 	end
@@ -463,22 +482,26 @@ function authenticate_post()
 	return 0
 end
 
--- Returns 1 if the cookie is valid and 0 if it is not.
+-- cgit reads the answer, where 1 lets the request through and 0 sends it to
+-- the login form.
 function authenticate_cookie()
 	local accepted_users = repo_userset(cgit["repo"])
 	if accepted_users == nil then
-		-- The repository is not protected.
+		-- A repository nothing lists is public.
 		return 1
 	end
 
-	local username = validate_value("username", get_cookie(http["cookie"], cookie_name))
+	local username = validate_value("username",
+		get_cookie(http["cookie"], cookie_name))
 	if username == nil or not accepted_users[username:lower()] then
 		return 0
 	end
 	return 1
 end
 
--- Prints the html for the login form.
+-- cgit calls this to fill the page body once the cookie has been turned down.
+-- The form carries a signed redirect token, so the browser lands back on the
+-- page that was asked for.
 function body()
 	local target = cgit["url"]
 	if not is_safe_redirect(target) then
@@ -501,8 +524,10 @@ function body()
 	return 0
 end
 
--- Wrapper around the filter API, exposing the http, cgit and post tables to
--- the functions above.
+-- cgit calls filter_open with the action name followed by the request fields
+-- in a fixed order, so they are unpacked here into the tables the functions
+-- above read. Only a post reaches filter_write, carrying the form body, and
+-- filter_close is where the action finally runs and answers cgit.
 
 local actions = {}
 actions["authenticate-post"] = authenticate_post
@@ -532,12 +557,12 @@ end
 
 function filter_close()
 	if action == nil then
-		-- Unknown action, deny rather than raise.
+		-- An unknown action denies rather than raising.
 		return 0
 	end
 	return action()
 end
 
 function filter_write(str)
-	post = parse_qs(str)
+	post = parse_query(str)
 end