diff options
| author | Bryce Kwon <bryce@brycekwon.com> | |
|---|---|---|
| committer | Bryce Kwon <bryce@brycekwon.com> | |
| commit | ||
| parent | ||
| tree | ||
| download | ||
Say when a login fails, and line up the form
A wrong password sent the reader back to the same empty form, which
looked no different from a crash. The redirect now carries a flag the
form reads, the same for an unknown user as for a wrong password, and
the form leaves the flag out of the way back it signs.
Diffstat (limited to '')
| -rw-r--r-- | README.txt | 2 | |
| -rw-r--r-- | assets/cgit.css | 20 | |
| -rw-r--r-- | custom/extensions/auth-file.lua | 30 | |
| -rw-r--r-- | custom/extensions/auth-inline.lua | 30 | |
| -rw-r--r-- | tests/extensions/test-auth.lua | 12 |
5 files changed, 77 insertions, 17 deletions
@@ -101,6 +101,8 @@ script added through the css and js options has stable hooks to work with. A character in a file that a browser would start a new line at, though git does not, such as a carriage return with no line feed after it, is shown as an escape sequence in span.escape. +* The bundled auth filters write their login form as form.login, with + p.login-error above it after a failed login. * Colours, fonts and metrics are custom properties on div#cgit, so a theme can redefine those alone. Every rule in cgit.css starts with div#cgit, apart from the background and margins it sets on html.cgit-standalone, the class of the diff --git a/assets/cgit.css b/assets/cgit.css index 9c0cce8..cf62df8 100644 --- a/assets/cgit.css +++ b/assets/cgit.css @@ -1103,6 +1103,26 @@ div#cgit span.deletions { color: var(--del); } +div#cgit p.login-error { + color: var(--del); + font-weight: bold; +} + +div#cgit form.login table { + border-collapse: separate; + border-spacing: 0 0.5em; +} + +div#cgit form.login td:first-child { + padding-right: 0.75em; +} + +div#cgit form.login input[type="text"], +div#cgit form.login input[type="password"] { + width: 16em; + max-width: 100%; +} + div#cgit .footer { padding: 0.85rem var(--gutter); text-align: center; diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.lua index 354adac..a03e1ea 100644 --- a/custom/extensions/auth-file.lua +++ b/custom/extensions/auth-file.lua @@ -476,6 +476,18 @@ function redirect_to(url) html("Location: " .. strip_controls(url) .. "\n") end +-- A failed login lands back on the page it came from with this flag, which +-- the form reads to say so and leaves out of the way back it signs. +local failed_flag = "login=failed" + +local function flag_failure(url) + return url .. (url:find("?", 1, true) and "&" or "?") .. failed_flag +end + +local function drop_failure_flag(url) + return (url:gsub("[?&]" .. failed_flag .. "$", "")) +end + function not_found() html("Status: 404 Not Found\n") html("Cache-Control: no-cache, no-store\n\n") @@ -484,9 +496,8 @@ end -- 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. --- 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. +-- A wrong password and an unknown user answer alike, so the flag on the way +-- back says only that the login failed. function authenticate_post() local redirect = validate_value("redirect", post["redirect"]) @@ -495,8 +506,6 @@ function authenticate_post() return 0 end - redirect_to(redirect) - local username = post["username"] local password = post["password"] local session = "" @@ -514,6 +523,7 @@ function authenticate_post() end -- A failed login answers with an empty session, which clears the cookie. + redirect_to(session == "" and flag_failure(redirect) or redirect) set_cookie(session_cookie, session) html("\n") @@ -541,13 +551,17 @@ end -- 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"] + local target = drop_failure_flag(cgit["url"]) + local failed = target ~= cgit["url"] if not is_safe_redirect(target) then target = cgit["login"] end html("<h2>Authentication Required</h2>") - html("<form method='post' action='") + if failed then + html("<p class='login-error' role='alert'>Wrong username or password.</p>") + end + html("<form class='login' method='post' action='") html_attr(cgit["login"]) html("'>") html("<input type='hidden' name='redirect' value='") @@ -560,7 +574,7 @@ function body() html("<tr><td><label for='password'>Password:</label></td><td>") html("<input id='password' name='password' type='password'") html(" autocomplete='current-password'></td></tr>") - html("<tr><td colspan='2'>") + html("<tr><td></td><td>") html("<input value='Login' type='submit'></td></tr>") html("</table></form>") diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.lua index e380733..c4b87f6 100644 --- a/custom/extensions/auth-inline.lua +++ b/custom/extensions/auth-inline.lua @@ -437,6 +437,18 @@ function redirect_to(url) html("Location: " .. strip_controls(url) .. "\n") end +-- A failed login lands back on the page it came from with this flag, which +-- the form reads to say so and leaves out of the way back it signs. +local failed_flag = "login=failed" + +local function flag_failure(url) + return url .. (url:find("?", 1, true) and "&" or "?") .. failed_flag +end + +local function drop_failure_flag(url) + return (url:gsub("[?&]" .. failed_flag .. "$", "")) +end + function not_found() html("Status: 404 Not Found\n") html("Cache-Control: no-cache, no-store\n\n") @@ -445,9 +457,8 @@ end -- 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. --- 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. +-- A wrong password and an unknown user answer alike, so the flag on the way +-- back says only that the login failed. function authenticate_post() local redirect = validate_value("redirect", post["redirect"]) @@ -456,8 +467,6 @@ function authenticate_post() return 0 end - redirect_to(redirect) - local username = post["username"] local password = post["password"] local session = "" @@ -475,6 +484,7 @@ function authenticate_post() end -- A failed login answers with an empty session, which clears the cookie. + redirect_to(session == "" and flag_failure(redirect) or redirect) set_cookie(session_cookie, session) html("\n") @@ -502,13 +512,17 @@ end -- 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"] + local target = drop_failure_flag(cgit["url"]) + local failed = target ~= cgit["url"] if not is_safe_redirect(target) then target = cgit["login"] end html("<h2>Authentication Required</h2>") - html("<form method='post' action='") + if failed then + html("<p class='login-error' role='alert'>Wrong username or password.</p>") + end + html("<form class='login' method='post' action='") html_attr(cgit["login"]) html("'>") html("<input type='hidden' name='redirect' value='") @@ -521,7 +535,7 @@ function body() html("<tr><td><label for='password'>Password:</label></td><td>") html("<input id='password' name='password' type='password'") html(" autocomplete='current-password'></td></tr>") - html("<tr><td colspan='2'>") + html("<tr><td></td><td>") html("<input value='Login' type='submit'></td></tr>") html("</table></form>") diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua index 0115716..742eabe 100644 --- a/tests/extensions/test-auth.lua +++ b/tests/extensions/test-auth.lua @@ -278,7 +278,8 @@ out, ret = run_action("no-such-action", {}) h.equals("an unknown action denies rather than raising", ret, 0) out, ret = run_action("body", { url = "/repo/log/?q=x" }) -h.contains("the login form posts to the login url", out, "<form method='post' action='/?p=login'>") +h.contains("the login form posts to the login url", out, "<form class='login' method='post' action='/?p=login'>") +h.equals("a first visit says nothing failed", out:find("login-error", 1, true), nil) local token = out:match("name='redirect' value='([^']*)'") h.equals("the form carries a signed way back", token and validate_value("redirect", token), "/repo/log/?q=x") @@ -286,6 +287,13 @@ out, ret = run_action("body", { url = "//evil.example/x" }) token = out:match("name='redirect' value='([^']*)'") h.equals("an unsafe destination is swapped for the login page", token and validate_value("redirect", token), "/?p=login") +h.equals("and is not taken for a failed login", out:find("login-error", 1, true), nil) + +out, ret = run_action("body", { url = "/repo/log/?q=x&login=failed" }) +h.contains("a failed login says so", out, + "<p class='login-error' role='alert'>Wrong username or password.</p>") +token = out:match("name='redirect' value='([^']*)'") +h.equals("and leaves the flag out of the way back", token and validate_value("redirect", token), "/repo/log/?q=x") local way_back = secure_value("redirect", "/repo/", 0) @@ -303,6 +311,7 @@ out, ret = run_action("authenticate-post", { body = "username=Alice&password=wrong&redirect=" .. url_encode(way_back), }) h.contains("a bad password redirects the same way", out, "Status: 302") +h.contains("back to the page with the failure flagged", out, "Location: /repo/?login=failed\n") h.contains("but clears the session cookie", out, "Set-Cookie: " .. session_name .. "=; ") out, ret = run_action("authenticate-post", { @@ -310,6 +319,7 @@ out, ret = run_action("authenticate-post", { body = "username=nobody&password=x&redirect=" .. url_encode(way_back), }) h.contains("an unknown user is told nothing different", out, "Set-Cookie: " .. session_name .. "=; ") +h.contains("and lands on the same flagged page", out, "Location: /repo/?login=failed\n") -- A failed login for a name with no account spends crypt on a configured -- hash, the same work a real check costs, so timing tells nothing. |