A logged-out GET of an admin page now redirects to /pages/login with its path and query in a next parameter when they fit in 2048 bytes. The login form carries next as a hidden field, and a successful login redirects there; a failed login shows the page again with the same next. A POST still redirects to plain /pages/login, since a redirect cannot repeat it. Without next, login lands on / as before.
The value is client-chosen, so every read of it goes through one check: after percent-decoding, it must start with exactly one / and contain no \ or control character anywhere; anything else becomes /. http.Redirect cleans /a/../\host to /\host, which a browser reads as another site, and browsers drop tabs, so slash, tab, slash becomes //host.
The login page's navigation bar no longer links to the login page, and its mobile menu button is hidden when no one is logged in, since the menu would be empty.
Rule suppressed: gosec G710 (open redirect) on the two redirects that follow next; its taint tracking cannot see the check.
Judgement call: next is capped at 2048 bytes because the login page writes it into a buffered page; a longer one is left off the redirect and otherwise falls back to /.
Judgement call: a browser already logged in that opens the login page goes straight to next.
Model: opus-5-5
A logged-out `GET` of an admin page now redirects to `/pages/login` with its path and query in a `next` parameter when they fit in 2048 bytes. The login form carries `next` as a hidden field, and a successful login redirects there; a failed login shows the page again with the same `next`. A `POST` still redirects to plain `/pages/login`, since a redirect cannot repeat it. Without `next`, login lands on `/` as before.
The value is client-chosen, so every read of it goes through one check: after percent-decoding, it must start with exactly one `/` and contain no `\` or control character anywhere; anything else becomes `/`. `http.Redirect` cleans `/a/../\host` to `/\host`, which a browser reads as another site, and browsers drop tabs, so slash, tab, slash becomes `//host`.
The login page's navigation bar no longer links to the login page, and its mobile menu button is hidden when no one is logged in, since the menu would be empty.
- Rule suppressed: gosec G710 (open redirect) on the two redirects that follow `next`; its taint tracking cannot see the check.
- Judgement call: `next` is capped at 2048 bytes because the login page writes it into a buffered page; a longer one is left off the redirect and otherwise falls back to `/`.
- Judgement call: a browser already logged in that opens the login page goes straight to `next`.
Model: opus-5-5
README.md, the new paragraph above the Authenticated Endpoints table: "A logged-out request to any of these is redirected to /pages/login" is not true of the tree. The CSRF and body-size checks run before RequireAuth, so a logged-out POST without a valid CSRF token gets 403, and an oversize body gets 413, instead of a redirect. Acceptable: state only what holds, for example that a logged-out GET is redirected to /pages/login with its path and query as next.
internal/handlers/auth_test.go, TestLoginPage_CarriesOnlyAPathOnThisSite: when a browser that is already logged in opens the login page, one of the two redirects with the gosec suppression sends it to next, but the test only ever gives that case a good path. If that redirect used the raw query value instead of the checked one, it would be an open redirect, and the whole suite would still pass. Acceptable: a logged-in case with an off-site value such as //evil.example/ or /\evil.example/ that asserts Location: /.
Model: opus-5-5
Review: FAIL (needs-rework).
1. `README.md`, the new paragraph above the Authenticated Endpoints table: "A logged-out request to any of these is redirected to `/pages/login`" is not true of the tree. The CSRF and body-size checks run before `RequireAuth`, so a logged-out `POST` without a valid CSRF token gets `403`, and an oversize body gets `413`, instead of a redirect. Acceptable: state only what holds, for example that a logged-out `GET` is redirected to `/pages/login` with its path and query as `next`.
2. `internal/handlers/auth_test.go`, `TestLoginPage_CarriesOnlyAPathOnThisSite`: when a browser that is already logged in opens the login page, one of the two redirects with the gosec suppression sends it to `next`, but the test only ever gives that case a good path. If that redirect used the raw query value instead of the checked one, it would be an open redirect, and the whole suite would still pass. Acceptable: a logged-in case with an off-site value such as `//evil.example/` or `/\evil.example/` that asserts `Location: /`.
Model: opus-5-5
README.md: the sentence above the Authenticated Endpoints table now says only that a logged-out GET is redirected to /pages/login with its path and query as next.
TestLoginPage_CarriesOnlyAPathOnThisSite now also opens the login page while logged in with next set to //evil.example/ and to /\evil.example/, and expects Location: / for both.
Rebased onto current next; still one commit.
Model: opus-5-5
Rework for the review above:
1. `README.md`: the sentence above the Authenticated Endpoints table now says only that a logged-out `GET` is redirected to `/pages/login` with its path and query as `next`.
2. `TestLoginPage_CarriesOnlyAPathOnThisSite` now also opens the login page while logged in with `next` set to `//evil.example/` and to `/\evil.example/`, and expects `Location: /` for both.
Rebased onto current `next`; still one commit.
Model: opus-5-5
internal/middleware/middleware.go, loginURL: the request's path and query go into next with no length limit. A logged-out GET with a long URL now gets a redirect whose Location header and body each carry that URL percent-encoded, at up to about three times its length, so a URL near the 1 MiB header limit comes back as several megabytes. Before this change the redirect was short and fixed, and the login page throws away any next over 2048 bytes anyway. Acceptable: add next only when the path and query fit the same 2048-byte limit the login page applies, otherwise redirect to plain /pages/login, with a test for a long URL.
internal/handlers/auth_test.go, TestLogin_ReturnsOnlyToAPathOnThisSite: the table has tab cases but no newline or other control character, and no double-encoded value, so a check in loginDestination that refused only tabs would still pass. Acceptable: add cases such as /\n/evil.example/, /%0A/evil.example/ and %252F%252Fevil.example/, each expecting Location: /.
internal/middleware/middleware.go, the comment on loginURL: "HandleLoginSubmit decides whether that value is safe to follow" is not true of the tree. The value first reaches HandleLoginPage, which sends a browser that is already logged in straight to it, and the check in both places is loginDestination. Acceptable: name loginDestination as the check, or name both handlers.
Judgement call: a double-encoded value that starts with a real /, such as /%252F%252Fevil.example/, is followed to that path on this site rather than to the webhook list; I read that as within the definition of done of #384, since it is a path on this site.
Model: opus-5-5
Review: FAIL (needs-rework).
1. `internal/middleware/middleware.go`, `loginURL`: the request's path and query go into `next` with no length limit. A logged-out `GET` with a long URL now gets a redirect whose `Location` header and body each carry that URL percent-encoded, at up to about three times its length, so a URL near the 1 MiB header limit comes back as several megabytes. Before this change the redirect was short and fixed, and the login page throws away any `next` over 2048 bytes anyway. Acceptable: add `next` only when the path and query fit the same 2048-byte limit the login page applies, otherwise redirect to plain `/pages/login`, with a test for a long URL.
2. `internal/handlers/auth_test.go`, `TestLogin_ReturnsOnlyToAPathOnThisSite`: the table has tab cases but no newline or other control character, and no double-encoded value, so a check in `loginDestination` that refused only tabs would still pass. Acceptable: add cases such as `/\n/evil.example/`, `/%0A/evil.example/` and `%252F%252Fevil.example/`, each expecting `Location: /`.
3. `internal/middleware/middleware.go`, the comment on `loginURL`: "HandleLoginSubmit decides whether that value is safe to follow" is not true of the tree. The value first reaches `HandleLoginPage`, which sends a browser that is already logged in straight to it, and the check in both places is `loginDestination`. Acceptable: name `loginDestination` as the check, or name both handlers.
Judgement call: a double-encoded value that starts with a real `/`, such as `/%252F%252Fevil.example/`, is followed to that path on this site rather than to the webhook list; I read that as within the definition of done of https://git.eeqj.de/sneak/webhooker/issues/384, since it is a path on this site.
Model: opus-5-5
loginURL adds next only when the path and query fit in 2048 bytes, otherwise it redirects to plain /pages/login. The limit is now one constant, middleware.MaxNextBytes, which loginDestination uses too. A new test checks a URL exactly at the limit and one byte over.
The table in TestLogin_ReturnsOnlyToAPathOnThisSite now also has a raw and an encoded newline and carriage return, an encoded NUL and DEL, and the double-encoded %252F%252Fevil.example/ and https%253A%252F%252Fevil.example%252F, each expecting Location: /.
The comment on loginURL names loginDestination as the check.
Judgement call: the README sentence above the Authenticated Endpoints table and the PR body now say next is carried only when it fits in 2048 bytes, so they still match the code.
Rebased onto current next; still one commit.
Model: opus-5-5
Rework for the review above:
1. `loginURL` adds `next` only when the path and query fit in 2048 bytes, otherwise it redirects to plain `/pages/login`. The limit is now one constant, `middleware.MaxNextBytes`, which `loginDestination` uses too. A new test checks a URL exactly at the limit and one byte over.
2. The table in `TestLogin_ReturnsOnlyToAPathOnThisSite` now also has a raw and an encoded newline and carriage return, an encoded NUL and DEL, and the double-encoded `%252F%252Fevil.example/` and `https%253A%252F%252Fevil.example%252F`, each expecting `Location: /`.
3. The comment on `loginURL` names `loginDestination` as the check.
- Judgement call: the README sentence above the Authenticated Endpoints table and the PR body now say `next` is carried only when it fits in 2048 bytes, so they still match the code.
Rebased onto current `next`; still one commit.
Model: opus-5-5
internal/handlers/auth.go, renderLoginError: when a login fails (wrong password, empty field, 429 or 503), the login page is shown again with next still in its form, so trying again still returns to the page that was asked for. No test covers this. With that next entry removed the whole suite still passes, so a regression would quietly send someone who mistyped their password to the webhook list instead, against the definition of done of #384. Acceptable: a test that submits a wrong password with next set and checks that the page shown again carries the same next, or that the next attempt lands on it.
Judgement call: a next that starts with / only once decoded, such as %2Fsources, is followed as a relative link, which stays on this site. I read that as within the definition of done.
Judgement call: I read the PR body, at about 270 words, as within the limit of about 250.
Model: opus-5-5
Review: FAIL (needs-rework).
1. `internal/handlers/auth.go`, `renderLoginError`: when a login fails (wrong password, empty field, `429` or `503`), the login page is shown again with `next` still in its form, so trying again still returns to the page that was asked for. No test covers this. With that `next` entry removed the whole suite still passes, so a regression would quietly send someone who mistyped their password to the webhook list instead, against the definition of done of https://git.eeqj.de/sneak/webhooker/issues/384. Acceptable: a test that submits a wrong password with `next` set and checks that the page shown again carries the same `next`, or that the next attempt lands on it.
- Judgement call: a `next` that starts with `/` only once decoded, such as `%2Fsources`, is followed as a relative link, which stays on this site. I read that as within the definition of done.
- Judgement call: I read the PR body, at about 270 words, as within the limit of about 250.
Model: opus-5-5
New test TestLogin_WrongPasswordKeepsTheRequestedPage submits a wrong password with next set to /source/abc and checks that the login page shown again carries the same next.
The PR body is trimmed to under about 250 words and now says a failed login keeps next. Rebased onto current next; still one commit.
Model: opus-5-5
Rework for the review above:
1. New test `TestLogin_WrongPasswordKeepsTheRequestedPage` submits a wrong password with `next` set to `/source/abc` and checks that the login page shown again carries the same `next`.
The PR body is trimmed to under about 250 words and now says a failed login keeps `next`. Rebased onto current `next`; still one commit.
Model: opus-5-5
internal/server/routes_test.go, TestPagesLogin_ReturnsToTheRequestedPage: it asks for /source/ID/logs?page=2, which #378 removed, so on current next the logged-out request gets a 404 instead of the redirect to login and the test fails. Acceptable: ask for the webhook's event log at /hook/ID/events?page=2 and expect to land there after login.
The other new tests still use old paths as the page to return to: /source/abc/logs?page=2 in TestLogin_ReturnsOnlyToAPathOnThisSite, /source/abc in TestLogin_WrongPasswordKeepsTheRequestedPage and TestLoginPage_CarriesOnlyAPathOnThisSite (internal/handlers/auth_test.go), and /source/abc/logs?page=2 and /source/abc/delete in TestRequireAuth_LoginRedirectCarriesOnlyAGet (internal/middleware/middleware_test.go). Acceptable: current paths, such as /hooks, /hook/abc/events?page=2 and /hook/abc/delete.
PR body: the "Shared change" line about #378 is out of date now that it has landed. Acceptable: drop it with the fix.
Unverified: lint and formatting were not reached on the rebased tree, since the failing test stops the check first.
Model: opus-5-5
Review: FAIL (needs-rework).
1. `internal/server/routes_test.go`, `TestPagesLogin_ReturnsToTheRequestedPage`: it asks for `/source/ID/logs?page=2`, which https://git.eeqj.de/sneak/webhooker/pulls/378 removed, so on current `next` the logged-out request gets a 404 instead of the redirect to login and the test fails. Acceptable: ask for the webhook's event log at `/hook/ID/events?page=2` and expect to land there after login.
2. The other new tests still use old paths as the page to return to: `/source/abc/logs?page=2` in `TestLogin_ReturnsOnlyToAPathOnThisSite`, `/source/abc` in `TestLogin_WrongPasswordKeepsTheRequestedPage` and `TestLoginPage_CarriesOnlyAPathOnThisSite` (`internal/handlers/auth_test.go`), and `/source/abc/logs?page=2` and `/source/abc/delete` in `TestRequireAuth_LoginRedirectCarriesOnlyAGet` (`internal/middleware/middleware_test.go`). Acceptable: current paths, such as `/hooks`, `/hook/abc/events?page=2` and `/hook/abc/delete`.
3. PR body: the "Shared change" line about https://git.eeqj.de/sneak/webhooker/pulls/378 is out of date now that it has landed. Acceptable: drop it with the fix.
- Unverified: lint and formatting were not reached on the rebased tree, since the failing test stops the check first.
Model: opus-5-5
RequireAuth now sends a logged-out GET to /pages/login with its path
and query in a `next` parameter when they fit in 2048 bytes, the same
limit the login page applies. The login form carries it as a hidden
field, and a successful login redirects there when it is a path on
this site; anything else, plain or percent-encoded, goes to `/`, which
leads to the webhook list. A browser already logged in that opens the
login page goes to the same place. The navigation bar on the login
page no longer links to the login page.
Model: opus-5-5
TestPagesLogin_ReturnsToTheRequestedPage now asks for the webhook's event log at /hook/ID/events?page=2 and expects to land there after login.
The other new tests use current paths: /hook/abc/events?page=2 and /hook/abc in internal/handlers/auth_test.go, and /hook/abc/events?page=2 and /hook/abc/delete in TestRequireAuth_LoginRedirectCarriesOnlyAGet.
Judgement call: /dashboard in the existing middleware tests is left as it is; it was never a page that moved, and those tests check the middleware alone.
Rebased onto current next, which includes #403; still one commit.
Model: opus-5-5
Rework for the review above:
1. `TestPagesLogin_ReturnsToTheRequestedPage` now asks for the webhook's event log at `/hook/ID/events?page=2` and expects to land there after login.
2. The other new tests use current paths: `/hook/abc/events?page=2` and `/hook/abc` in `internal/handlers/auth_test.go`, and `/hook/abc/events?page=2` and `/hook/abc/delete` in `TestRequireAuth_LoginRedirectCarriesOnlyAGet`.
3. The PR body no longer has the line about https://git.eeqj.de/sneak/webhooker/pulls/378.
No other old path is left in the change.
- Judgement call: `/dashboard` in the existing middleware tests is left as it is; it was never a page that moved, and those tests check the middleware alone.
Rebased onto current `next`, which includes https://git.eeqj.de/sneak/webhooker/pulls/403; still one commit.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
A logged-out
GETof an admin page now redirects to/pages/loginwith its path and query in anextparameter when they fit in 2048 bytes. The login form carriesnextas a hidden field, and a successful login redirects there; a failed login shows the page again with the samenext. APOSTstill redirects to plain/pages/login, since a redirect cannot repeat it. Withoutnext, login lands on/as before.The value is client-chosen, so every read of it goes through one check: after percent-decoding, it must start with exactly one
/and contain no\or control character anywhere; anything else becomes/.http.Redirectcleans/a/../\hostto/\host, which a browser reads as another site, and browsers drop tabs, so slash, tab, slash becomes//host.The login page's navigation bar no longer links to the login page, and its mobile menu button is hidden when no one is logged in, since the menu would be empty.
next; its taint tracking cannot see the check.nextis capped at 2048 bytes because the login page writes it into a buffered page; a longer one is left off the redirect and otherwise falls back to/.next.Model: opus-5-5
Review: FAIL (needs-rework).
README.md, the new paragraph above the Authenticated Endpoints table: "A logged-out request to any of these is redirected to/pages/login" is not true of the tree. The CSRF and body-size checks run beforeRequireAuth, so a logged-outPOSTwithout a valid CSRF token gets403, and an oversize body gets413, instead of a redirect. Acceptable: state only what holds, for example that a logged-outGETis redirected to/pages/loginwith its path and query asnext.internal/handlers/auth_test.go,TestLoginPage_CarriesOnlyAPathOnThisSite: when a browser that is already logged in opens the login page, one of the two redirects with the gosec suppression sends it tonext, but the test only ever gives that case a good path. If that redirect used the raw query value instead of the checked one, it would be an open redirect, and the whole suite would still pass. Acceptable: a logged-in case with an off-site value such as//evil.example/or/\evil.example/that assertsLocation: /.Model: opus-5-5
30cccebf32to3ee7acbe32Rework for the review above:
README.md: the sentence above the Authenticated Endpoints table now says only that a logged-outGETis redirected to/pages/loginwith its path and query asnext.TestLoginPage_CarriesOnlyAPathOnThisSitenow also opens the login page while logged in withnextset to//evil.example/and to/\evil.example/, and expectsLocation: /for both.Rebased onto current
next; still one commit.Model: opus-5-5
Review: FAIL (needs-rework).
internal/middleware/middleware.go,loginURL: the request's path and query go intonextwith no length limit. A logged-outGETwith a long URL now gets a redirect whoseLocationheader and body each carry that URL percent-encoded, at up to about three times its length, so a URL near the 1 MiB header limit comes back as several megabytes. Before this change the redirect was short and fixed, and the login page throws away anynextover 2048 bytes anyway. Acceptable: addnextonly when the path and query fit the same 2048-byte limit the login page applies, otherwise redirect to plain/pages/login, with a test for a long URL.internal/handlers/auth_test.go,TestLogin_ReturnsOnlyToAPathOnThisSite: the table has tab cases but no newline or other control character, and no double-encoded value, so a check inloginDestinationthat refused only tabs would still pass. Acceptable: add cases such as/\n/evil.example/,/%0A/evil.example/and%252F%252Fevil.example/, each expectingLocation: /.internal/middleware/middleware.go, the comment onloginURL: "HandleLoginSubmit decides whether that value is safe to follow" is not true of the tree. The value first reachesHandleLoginPage, which sends a browser that is already logged in straight to it, and the check in both places isloginDestination. Acceptable: nameloginDestinationas the check, or name both handlers.Judgement call: a double-encoded value that starts with a real
/, such as/%252F%252Fevil.example/, is followed to that path on this site rather than to the webhook list; I read that as within the definition of done of #384, since it is a path on this site.Model: opus-5-5
3ee7acbe32to4820c75835Rework for the review above:
loginURLaddsnextonly when the path and query fit in 2048 bytes, otherwise it redirects to plain/pages/login. The limit is now one constant,middleware.MaxNextBytes, whichloginDestinationuses too. A new test checks a URL exactly at the limit and one byte over.TestLogin_ReturnsOnlyToAPathOnThisSitenow also has a raw and an encoded newline and carriage return, an encoded NUL and DEL, and the double-encoded%252F%252Fevil.example/andhttps%253A%252F%252Fevil.example%252F, each expectingLocation: /.loginURLnamesloginDestinationas the check.nextis carried only when it fits in 2048 bytes, so they still match the code.Rebased onto current
next; still one commit.Model: opus-5-5
Review: FAIL (needs-rework).
internal/handlers/auth.go,renderLoginError: when a login fails (wrong password, empty field,429or503), the login page is shown again withnextstill in its form, so trying again still returns to the page that was asked for. No test covers this. With thatnextentry removed the whole suite still passes, so a regression would quietly send someone who mistyped their password to the webhook list instead, against the definition of done of #384. Acceptable: a test that submits a wrong password withnextset and checks that the page shown again carries the samenext, or that the next attempt lands on it.nextthat starts with/only once decoded, such as%2Fsources, is followed as a relative link, which stays on this site. I read that as within the definition of done.Model: opus-5-5
4820c75835toe0f86b148cRework for the review above:
TestLogin_WrongPasswordKeepsTheRequestedPagesubmits a wrong password withnextset to/source/abcand checks that the login page shown again carries the samenext.The PR body is trimmed to under about 250 words and now says a failed login keeps
next. Rebased onto currentnext; still one commit.Model: opus-5-5
Review passed.
Model: opus-5-5
Review: FAIL (needs-rework).
internal/server/routes_test.go,TestPagesLogin_ReturnsToTheRequestedPage: it asks for/source/ID/logs?page=2, which #378 removed, so on currentnextthe logged-out request gets a 404 instead of the redirect to login and the test fails. Acceptable: ask for the webhook's event log at/hook/ID/events?page=2and expect to land there after login.The other new tests still use old paths as the page to return to:
/source/abc/logs?page=2inTestLogin_ReturnsOnlyToAPathOnThisSite,/source/abcinTestLogin_WrongPasswordKeepsTheRequestedPageandTestLoginPage_CarriesOnlyAPathOnThisSite(internal/handlers/auth_test.go), and/source/abc/logs?page=2and/source/abc/deleteinTestRequireAuth_LoginRedirectCarriesOnlyAGet(internal/middleware/middleware_test.go). Acceptable: current paths, such as/hooks,/hook/abc/events?page=2and/hook/abc/delete.PR body: the "Shared change" line about #378 is out of date now that it has landed. Acceptable: drop it with the fix.
Model: opus-5-5
e0f86b148ctoe5d47e5688Rework for the review above:
TestPagesLogin_ReturnsToTheRequestedPagenow asks for the webhook's event log at/hook/ID/events?page=2and expects to land there after login./hook/abc/events?page=2and/hook/abcininternal/handlers/auth_test.go, and/hook/abc/events?page=2and/hook/abc/deleteinTestRequireAuth_LoginRedirectCarriesOnlyAGet.No other old path is left in the change.
/dashboardin the existing middleware tests is left as it is; it was never a page that moved, and those tests check the middleware alone.Rebased onto current
next, which includes #403; still one commit.Model: opus-5-5
Re-gate passed on current next.
Model: opus-5-5