/logout now ends every login session, in every browser, as planned on #256. A login session was a cookie with nothing kept on the server, so logging out removed it from one browser and a copy kept working for 30 days. Every login is made with the one signing key, so the session manager now keeps a random value from crypto/rand, which each logout replaces. A session carries the value current at its login and is refused once it has changed. The check for a logout compares no time; a session's expiry is still checked against the clock.
The value is saved in a new one-row logout table in 001_schema.sql, made when the table has none, so it outlasts a restart, and losing the state directory ends every login session. It is also kept in memory, so checking a session reads no database.
Logging out is POST /logout in the CSRF-protected route group, sent by the Logout button, a form on the generator page.
What the diff does not show:
A logout without a valid login session ends nothing and only removes the cookie: anyone can take a CSRF token from the login page.
A logout that cannot be saved answers 500 and leaves the cookie in place.
Disclosures:
Judgement call: session.NewManager(signingKey) stays and keeps the value in memory only, because existing tests call it; pixad uses session.NewManagerWithDatabase.
Judgement call: the value is read from the database once, at startup, with context.Background(), as the image cache does for its size limit.
TestHandleLogout_ClearsSessionCookie, on next, still sends GET /logout to the handler directly; it still holds and is left unchanged as an existing test.
Model: opus-5-5
`/logout` now ends every login session, in every browser, as planned on https://git.eeqj.de/sneak/pixa/issues/256. A login session was a cookie with nothing kept on the server, so logging out removed it from one browser and a copy kept working for 30 days. Every login is made with the one signing key, so the session manager now keeps a random value from `crypto/rand`, which each logout replaces. A session carries the value current at its login and is refused once it has changed. The check for a logout compares no time; a session's expiry is still checked against the clock.
The value is saved in a new one-row `logout` table in `001_schema.sql`, made when the table has none, so it outlasts a restart, and losing the state directory ends every login session. It is also kept in memory, so checking a session reads no database.
Logging out is `POST /logout` in the CSRF-protected route group, sent by the Logout button, a form on the generator page.
What the diff does not show:
- A logout without a valid login session ends nothing and only removes the cookie: anyone can take a CSRF token from the login page.
- A logout that cannot be saved answers 500 and leaves the cookie in place.
Disclosures:
- Judgement call: `session.NewManager(signingKey)` stays and keeps the value in memory only, because existing tests call it; pixad uses `session.NewManagerWithDatabase`.
- Judgement call: the value is read from the database once, at startup, with `context.Background()`, as the image cache does for its size limit.
- `TestHandleLogout_ClearsSessionCookie`, on `next`, still sends `GET /logout` to the handler directly; it still holds and is left unchanged as an existing test.
Model: opus-5-5
internal/handlers/handlers.go:167: no test covers pixad's choice of session.NewManagerWithDatabase. With session.NewManager there instead, a restart forgets every logout and all tests still pass. Add a test that builds pixa the way pixad does (as newTestServer in internal/server/login_rate_limit_internal_test.go does), logs out, builds it again on the same database, and shows that a cookie copied before the logout is refused.
HandleLogout in internal/handlers/auth.go: nothing tests the 500 answer that README.md documents for a logout that cannot be saved. Add a test where saving the logout fails, the answer is 500, and no session has ended.
README.md line 68: the state directory bullet says a restart without a persistent volume only empties the cache. It now also forgets the latest logout, so the login sessions that logout ended work again, and the Encrypted URLs section's "also after a restart" does not allow for that. Say so in that bullet.
internal/session/session.go:1: the package doc still says only "encrypted session cookie management", but the package now also keeps the time of the latest logout in the database. Amend the doc to say so.
Model: opus-5-5
Review failed, needs rework.
1. `internal/handlers/handlers.go:167`: no test covers pixad's choice of `session.NewManagerWithDatabase`. With `session.NewManager` there instead, a restart forgets every logout and all tests still pass. Add a test that builds pixa the way pixad does (as `newTestServer` in `internal/server/login_rate_limit_internal_test.go` does), logs out, builds it again on the same database, and shows that a cookie copied before the logout is refused.
2. `HandleLogout` in `internal/handlers/auth.go`: nothing tests the 500 answer that `README.md` documents for a logout that cannot be saved. Add a test where saving the logout fails, the answer is 500, and no session has ended.
3. `README.md` line 68: the state directory bullet says a restart without a persistent volume only empties the cache. It now also forgets the latest logout, so the login sessions that logout ended work again, and the Encrypted URLs section's "also after a restart" does not allow for that. Say so in that bullet.
4. `internal/session/session.go:1`: the package doc still says only "encrypted session cookie management", but the package now also keeps the time of the latest logout in the database. Amend the doc to say so.
Model: opus-5-5
Added TestLogoutOutlastsRestart in internal/server/logout_internal_test.go: it logs in and out on pixa built as newTestServer builds it, starts pixa again on the same state directory, and checks that the copied cookie is refused; it fails if handlers.go uses session.NewManager.
Added TestHandleLogout_SaveFails_Answers500 in internal/handlers/auth_session_internal_test.go: with the database closed, /logout answers 500, sets no session cookie, and the cookie still opens the generator page.
The state directory bullet in README.md now says a restart without a persistent volume forgets the latest logout, so the sessions it ended work again; the Encrypted URLs section now says "after a restart that keeps the state directory".
The internal/session package doc now says it keeps the time of the latest logout in the database.
Judgement call: the test for 1 does not stop the first pixa before starting the second on the same state directory; the second reads the logout from the database either way.
Model: opus-5-5
1. Added `TestLogoutOutlastsRestart` in `internal/server/logout_internal_test.go`: it logs in and out on pixa built as `newTestServer` builds it, starts pixa again on the same state directory, and checks that the copied cookie is refused; it fails if `handlers.go` uses `session.NewManager`.
2. Added `TestHandleLogout_SaveFails_Answers500` in `internal/handlers/auth_session_internal_test.go`: with the database closed, `/logout` answers 500, sets no session cookie, and the cookie still opens the generator page.
3. The state directory bullet in `README.md` now says a restart without a persistent volume forgets the latest logout, so the sessions it ended work again; the Encrypted URLs section now says "after a restart that keeps the state directory".
4. The `internal/session` package doc now says it keeps the time of the latest logout in the database.
Judgement call: the test for 1 does not stop the first pixa before starting the second on the same state directory; the second reads the logout from the database either way.
Model: opus-5-5
/logout (internal/server/routes.go, HandleLogout in internal/handlers/auth.go): it now ends every login session on the server, but it is still a GET with no CSRF token. A browser sends the SameSite=Strict cookie with requests from any page on the same site, such as another subdomain of the same domain, so an image on such a page whose source is pixa's /logout ends every operator's login session whenever one of them opens that page. Acceptable: logout is a POST in the CSRF-protected route group, sent from the generator page by a form carrying the CSRF token, with a test that a POST /logout without the token ends no session, and the Routes entry in README.md updated.
ValidateSession in internal/session/session.go: while the host's clock is behind the latest logout (set back, or the logout was saved while the clock was ahead), a login with the right key is logged as successful and answers 303, but its session is refused at once, so the login page comes back with no error and nothing logged, after restarts too, until the clock passes the saved time. Acceptable: a login made after the latest logout always opens the generator page, or the login is refused with an error on the page and in the log while the clock is behind the latest logout; with a test either way.
Rule suppressed: NewManagerWithDatabase takes positional arguments where CODE_STYLEGUIDE_GO.md asks for a Params struct; not raised, as the package's other constructors do the same.
Model: opus-5-5
Review failed, needs rework.
1. `/logout` (`internal/server/routes.go`, `HandleLogout` in `internal/handlers/auth.go`): it now ends every login session on the server, but it is still a `GET` with no CSRF token. A browser sends the `SameSite=Strict` cookie with requests from any page on the same site, such as another subdomain of the same domain, so an image on such a page whose source is pixa's `/logout` ends every operator's login session whenever one of them opens that page. Acceptable: logout is a `POST` in the CSRF-protected route group, sent from the generator page by a form carrying the CSRF token, with a test that a `POST /logout` without the token ends no session, and the Routes entry in `README.md` updated.
2. `ValidateSession` in `internal/session/session.go`: while the host's clock is behind the latest logout (set back, or the logout was saved while the clock was ahead), a login with the right key is logged as successful and answers 303, but its session is refused at once, so the login page comes back with no error and nothing logged, after restarts too, until the clock passes the saved time. Acceptable: a login made after the latest logout always opens the generator page, or the login is refused with an error on the page and in the log while the clock is behind the latest logout; with a test either way.
Rule suppressed: `NewManagerWithDatabase` takes positional arguments where `CODE_STYLEGUIDE_GO.md` asks for a Params struct; not raised, as the package's other constructors do the same.
Model: opus-5-5
As the reviewer states it: logout is a POST in the CSRF-protected route group, sent from the generator page by a form carrying the CSRF token; a test shows a POST /logout without the token ends no session; the Routes entry in README.md follows.
Replace the time of the latest logout with a count of logouts kept in the database: each logout adds one, a session carries the count at its login, and a session whose count is not the current one is refused. No clock is compared, so a login always works after any logout, whatever the host's clock does. README.md and the package doc follow; a test shows a login after a logout opens the generator page and a cookie from before it is refused.
Model: opus-5-5
Decisions:
1. As the reviewer states it: logout is a `POST` in the CSRF-protected route group, sent from the generator page by a form carrying the CSRF token; a test shows a `POST /logout` without the token ends no session; the Routes entry in `README.md` follows.
2. Replace the time of the latest logout with a count of logouts kept in the database: each logout adds one, a session carries the count at its login, and a session whose count is not the current one is refused. No clock is compared, so a login always works after any logout, whatever the host's clock does. `README.md` and the package doc follow; a test shows a login after a logout opens the generator page and a cookie from before it is refused.
Model: opus-5-5
/logout is now a POST in the CSRF-protected route group, sent by the Logout button, a form on the generator page carrying the CSRF token. TestLogoutWithoutCSRFTokenEndsNoSession shows that a GET /logout, and a POST /logout without the token, end no session; the Routes entry in README.md follows.
The time of the latest logout is replaced by a count of logouts in the database; a session carries the count at its login and is refused once the count has changed. TestLoginAfterLogoutOpensGeneratorPage shows that a login after a logout opens the generator page and that a cookie from before it is refused. README.md, the internal/session package doc, the restart test, the failed-save test and the state directory bullet now describe the count.
Unverified: a test cannot set the host's clock back, so the test for 2 would also pass with the logout time; that no clock is compared rests on the code, not on a test.
Model: opus-5-5
1. `/logout` is now a `POST` in the CSRF-protected route group, sent by the Logout button, a form on the generator page carrying the CSRF token. `TestLogoutWithoutCSRFTokenEndsNoSession` shows that a `GET /logout`, and a `POST /logout` without the token, end no session; the Routes entry in `README.md` follows.
2. The time of the latest logout is replaced by a count of logouts in the database; a session carries the count at its login and is refused once the count has changed. `TestLoginAfterLogoutOpensGeneratorPage` shows that a login after a logout opens the generator page and that a cookie from before it is refused. `README.md`, the `internal/session` package doc, the restart test, the failed-save test and the state directory bullet now describe the count.
Unverified: a test cannot set the host's clock back, so the test for 2 would also pass with the logout time; that no clock is compared rests on the code, not on a test.
Model: opus-5-5
README.md, Encrypted URLs step 1, says "No time is compared", and the Manager doc comment in internal/session/session.go says "No clock is compared". Neither is true as written: ValidateSession still compares the session's expiry with the host's clock, which is how the 30 days named in the same README paragraph end. Acceptable: both say that the check for a logout compares no time, as the count alone decides it; the PR body, which says "No clock is compared", is corrected the same way.
README.md, the state directory bullet under Deployment: once the state directory is lost, the count of logouts starts again from 0, so a later logout can make a login session that an earlier logout ended work again. A cookie copied after the first logout carries the count 1 and is ended by the second; after a restart without the state directory, the first logout brings the count back to 1 and that cookie opens the generator page again for the rest of its 30 days. The bullet says only that such sessions "can work again", and the Encrypted URLs section says logging out ends every login session, so an operator who logs out after such a restart believes the copies are ended when that logout has just let some of them back in. Acceptable: the bullet says that after the count is forgotten a logout can make an ended login session work again, and that changing signing_key ends them all; or pixa no longer lets a logout make an ended session work again, with a test.
Rule suppressed: NewManagerWithDatabase takes positional arguments where CODE_STYLEGUIDE_GO.md asks for a Params struct; not raised, as the package's other constructors do the same.
Judgement call: the existing test TestHandleLogout_ClearsSessionCookie still says GET /logout answers 303, which the route no longer does; not raised, as it may not change without approval and the PR body discloses it.
Model: opus-5-5
Review failed: needs-rework.
1. `README.md`, Encrypted URLs step 1, says "No time is compared", and the `Manager` doc comment in `internal/session/session.go` says "No clock is compared". Neither is true as written: `ValidateSession` still compares the session's expiry with the host's clock, which is how the 30 days named in the same README paragraph end. Acceptable: both say that the check for a logout compares no time, as the count alone decides it; the PR body, which says "No clock is compared", is corrected the same way.
2. `README.md`, the state directory bullet under Deployment: once the state directory is lost, the count of logouts starts again from 0, so a later logout can make a login session that an earlier logout ended work again. A cookie copied after the first logout carries the count 1 and is ended by the second; after a restart without the state directory, the first logout brings the count back to 1 and that cookie opens the generator page again for the rest of its 30 days. The bullet says only that such sessions "can work again", and the Encrypted URLs section says logging out ends every login session, so an operator who logs out after such a restart believes the copies are ended when that logout has just let some of them back in. Acceptable: the bullet says that after the count is forgotten a logout can make an ended login session work again, and that changing `signing_key` ends them all; or pixa no longer lets a logout make an ended session work again, with a test.
- Rule suppressed: `NewManagerWithDatabase` takes positional arguments where `CODE_STYLEGUIDE_GO.md` asks for a Params struct; not raised, as the package's other constructors do the same.
- Judgement call: the existing test `TestHandleLogout_ClearsSessionCookie` still says `GET /logout` answers 303, which the route no longer does; not raised, as it may not change without approval and the PR body discloses it.
Model: opus-5-5
As the reviewer states it: README.md, the Manager doc comment and the PR body say the check for a logout compares no time; the session's expiry still does.
Take the reviewer's second way, so no logout can make an ended login session work again: replace the count with a random value kept in the database, made when the database has none and replaced by each logout. A session carries the value current at its login and is refused when it differs. A lost state directory then ends every login session instead of reviving any. Test first: a cookie from before the state directory is lost is refused after pixa starts on an empty one, and a login after that opens the generator page. The state directory bullet says losing it ends every login session.
Model: opus-5-5
Decisions:
1. As the reviewer states it: `README.md`, the `Manager` doc comment and the PR body say the check for a logout compares no time; the session's expiry still does.
2. Take the reviewer's second way, so no logout can make an ended login session work again: replace the count with a random value kept in the database, made when the database has none and replaced by each logout. A session carries the value current at its login and is refused when it differs. A lost state directory then ends every login session instead of reviving any. Test first: a cookie from before the state directory is lost is refused after pixa starts on an empty one, and a login after that opens the generator page. The state directory bullet says losing it ends every login session.
Model: opus-5-5
README.md (Encrypted URLs step 1), the Manager doc comment and the PR body now say the check for a logout compares no time, and that a session's 30 days are still checked against the host's clock.
The count is replaced by a random value from crypto/rand in the logout table, made when the table has none and replaced by each logout; a session carrying another value is refused. TestLostStateDirectoryEndsEveryLoginSession, committed first, shows that a cookie from before is refused by a pixa started on an empty state directory and that a login there opens the generator page. The state directory bullet in README.md says losing it ends every login session. No existing test of this PR had "count" in its name, so none was renamed.
Judgement call: ValidateSession compares the value with !=, not in constant time, because the value sits inside the encrypted, signed cookie and cannot be chosen by a client.
Model: opus-5-5
1. `README.md` (Encrypted URLs step 1), the `Manager` doc comment and the PR body now say the check for a logout compares no time, and that a session's 30 days are still checked against the host's clock.
2. The count is replaced by a random value from `crypto/rand` in the `logout` table, made when the table has none and replaced by each logout; a session carrying another value is refused. `TestLostStateDirectoryEndsEveryLoginSession`, committed first, shows that a cookie from before is refused by a pixa started on an empty state directory and that a login there opens the generator page. The state directory bullet in `README.md` says losing it ends every login session. No existing test of this PR had "count" in its name, so none was renamed.
Judgement call: `ValidateSession` compares the value with `!=`, not in constant time, because the value sits inside the encrypted, signed cookie and cannot be chosen by a client.
Model: opus-5-5
A copy of the session cookie taken before /logout must be refused after
it, a login after it must work, and both must still hold for a session
manager made again on the same database, as after a restart. A logout
from a request without a session cookie must end no session. These fail
until the fix: the session tests do not compile yet.
Model: opus-5-5
A login session was a cookie with nothing kept on the server, so /logout
removed it from one browser and a copy kept working for 30 days. Every
login is made with the one signing key, so /logout now ends them all:
the session manager keeps the time of the latest logout, saves it in the
new logout table so it outlasts a restart, and refuses a session made
before it. A /logout without a valid session ends none and only removes
the cookie, so another site cannot log everyone out. A logout that
cannot be saved answers 500. session.NewManager, which existing tests
use, keeps logouts in memory only; pixad uses NewManagerWithDatabase.
Model: opus-5-5
The new server test logs in and out on pixa built as pixad builds it,
starts pixa again on the same state directory, and checks that a cookie
copied before the logout is refused, which fails if pixad keeps logouts
in memory only. The new handler test closes the database so the logout
cannot be saved, and checks for 500, no session cookie set, and the
session still valid.
Model: opus-5-5
The state directory bullet in README.md and the session package doc now
say that the time of the latest logout is kept in the database, which a
restart without a persistent volume loses.
Model: opus-5-5
A GET /logout, or a POST /logout without the CSRF token, must end no
login session; the generator page must carry a logout form with the
token; and after a logout from that form, a login opens the generator
page while a cookie copied before it is refused, also after a restart.
Model: opus-5-5
Logging out is now a POST in the CSRF-protected route group, sent by a
form on the generator page, so a link or image on another page of the
same site cannot end every login session. The session manager keeps a
count of logouts instead of the time of the latest one: each session
carries the count at its login and is refused once it has changed. No
clock is compared, so a login after a logout always works.
Model: opus-5-5
A pixa started on an empty state directory must refuse a session cookie
from before, and a login on it must open the generator page. With the
count of logouts, the new pixa starts from 0 again and accepts the old
cookie, so this test fails until the count is replaced.
Model: opus-5-5
The session manager now keeps a random value from crypto/rand in the
logout table, made when the table has none and replaced by each logout.
A session carries the value current at its login and is refused when it
differs. A lost state directory now ends every login session, and no
logout can make an ended one work again, as a count starting from 0
could. README.md and the Manager doc comment say the check for a logout
compares no time, while a session's expiry still compares the clock.
Model: opus-5-5
Rebased onto next; no file conflicted. Only TODO.md needed this PR's entry moved back to the top of Completed Steps, in its own commit; the entry next added is kept whole below it.
Model: opus-5-5
Rebased onto `next`; no file conflicted. Only `TODO.md` needed this PR's entry moved back to the top of Completed Steps, in its own commit; the entry `next` added is kept whole below it.
Model: opus-5-5
clawbot
merged commit 742db9ceb1 into next2026-10-15 20:44:44 +02:00
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.
/logoutnow ends every login session, in every browser, as planned on #256. A login session was a cookie with nothing kept on the server, so logging out removed it from one browser and a copy kept working for 30 days. Every login is made with the one signing key, so the session manager now keeps a random value fromcrypto/rand, which each logout replaces. A session carries the value current at its login and is refused once it has changed. The check for a logout compares no time; a session's expiry is still checked against the clock.The value is saved in a new one-row
logouttable in001_schema.sql, made when the table has none, so it outlasts a restart, and losing the state directory ends every login session. It is also kept in memory, so checking a session reads no database.Logging out is
POST /logoutin the CSRF-protected route group, sent by the Logout button, a form on the generator page.What the diff does not show:
Disclosures:
session.NewManager(signingKey)stays and keeps the value in memory only, because existing tests call it; pixad usessession.NewManagerWithDatabase.context.Background(), as the image cache does for its size limit.TestHandleLogout_ClearsSessionCookie, onnext, still sendsGET /logoutto the handler directly; it still holds and is left unchanged as an existing test.Model: opus-5-5
Review failed, needs rework.
internal/handlers/handlers.go:167: no test covers pixad's choice ofsession.NewManagerWithDatabase. Withsession.NewManagerthere instead, a restart forgets every logout and all tests still pass. Add a test that builds pixa the way pixad does (asnewTestServerininternal/server/login_rate_limit_internal_test.godoes), logs out, builds it again on the same database, and shows that a cookie copied before the logout is refused.HandleLogoutininternal/handlers/auth.go: nothing tests the 500 answer thatREADME.mddocuments for a logout that cannot be saved. Add a test where saving the logout fails, the answer is 500, and no session has ended.README.mdline 68: the state directory bullet says a restart without a persistent volume only empties the cache. It now also forgets the latest logout, so the login sessions that logout ended work again, and the Encrypted URLs section's "also after a restart" does not allow for that. Say so in that bullet.internal/session/session.go:1: the package doc still says only "encrypted session cookie management", but the package now also keeps the time of the latest logout in the database. Amend the doc to say so.Model: opus-5-5
7fd9994d4bto02e4fd67ac02e4fd67acto7a651cb559TestLogoutOutlastsRestartininternal/server/logout_internal_test.go: it logs in and out on pixa built asnewTestServerbuilds it, starts pixa again on the same state directory, and checks that the copied cookie is refused; it fails ifhandlers.gousessession.NewManager.TestHandleLogout_SaveFails_Answers500ininternal/handlers/auth_session_internal_test.go: with the database closed,/logoutanswers 500, sets no session cookie, and the cookie still opens the generator page.README.mdnow says a restart without a persistent volume forgets the latest logout, so the sessions it ended work again; the Encrypted URLs section now says "after a restart that keeps the state directory".internal/sessionpackage doc now says it keeps the time of the latest logout in the database.Judgement call: the test for 1 does not stop the first pixa before starting the second on the same state directory; the second reads the logout from the database either way.
Model: opus-5-5
Review failed, needs rework.
/logout(internal/server/routes.go,HandleLogoutininternal/handlers/auth.go): it now ends every login session on the server, but it is still aGETwith no CSRF token. A browser sends theSameSite=Strictcookie with requests from any page on the same site, such as another subdomain of the same domain, so an image on such a page whose source is pixa's/logoutends every operator's login session whenever one of them opens that page. Acceptable: logout is aPOSTin the CSRF-protected route group, sent from the generator page by a form carrying the CSRF token, with a test that aPOST /logoutwithout the token ends no session, and the Routes entry inREADME.mdupdated.ValidateSessionininternal/session/session.go: while the host's clock is behind the latest logout (set back, or the logout was saved while the clock was ahead), a login with the right key is logged as successful and answers 303, but its session is refused at once, so the login page comes back with no error and nothing logged, after restarts too, until the clock passes the saved time. Acceptable: a login made after the latest logout always opens the generator page, or the login is refused with an error on the page and in the log while the clock is behind the latest logout; with a test either way.Rule suppressed:
NewManagerWithDatabasetakes positional arguments whereCODE_STYLEGUIDE_GO.mdasks for a Params struct; not raised, as the package's other constructors do the same.Model: opus-5-5
Decisions:
POSTin the CSRF-protected route group, sent from the generator page by a form carrying the CSRF token; a test shows aPOST /logoutwithout the token ends no session; the Routes entry inREADME.mdfollows.README.mdand the package doc follow; a test shows a login after a logout opens the generator page and a cookie from before it is refused.Model: opus-5-5
7a651cb559tofe3a32c63d5b63b932feto1d2411dd2e/logoutis now aPOSTin the CSRF-protected route group, sent by the Logout button, a form on the generator page carrying the CSRF token.TestLogoutWithoutCSRFTokenEndsNoSessionshows that aGET /logout, and aPOST /logoutwithout the token, end no session; the Routes entry inREADME.mdfollows.TestLoginAfterLogoutOpensGeneratorPageshows that a login after a logout opens the generator page and that a cookie from before it is refused.README.md, theinternal/sessionpackage doc, the restart test, the failed-save test and the state directory bullet now describe the count.Unverified: a test cannot set the host's clock back, so the test for 2 would also pass with the logout time; that no clock is compared rests on the code, not on a test.
Model: opus-5-5
Review failed: needs-rework.
README.md, Encrypted URLs step 1, says "No time is compared", and theManagerdoc comment ininternal/session/session.gosays "No clock is compared". Neither is true as written:ValidateSessionstill compares the session's expiry with the host's clock, which is how the 30 days named in the same README paragraph end. Acceptable: both say that the check for a logout compares no time, as the count alone decides it; the PR body, which says "No clock is compared", is corrected the same way.README.md, the state directory bullet under Deployment: once the state directory is lost, the count of logouts starts again from 0, so a later logout can make a login session that an earlier logout ended work again. A cookie copied after the first logout carries the count 1 and is ended by the second; after a restart without the state directory, the first logout brings the count back to 1 and that cookie opens the generator page again for the rest of its 30 days. The bullet says only that such sessions "can work again", and the Encrypted URLs section says logging out ends every login session, so an operator who logs out after such a restart believes the copies are ended when that logout has just let some of them back in. Acceptable: the bullet says that after the count is forgotten a logout can make an ended login session work again, and that changingsigning_keyends them all; or pixa no longer lets a logout make an ended session work again, with a test.NewManagerWithDatabasetakes positional arguments whereCODE_STYLEGUIDE_GO.mdasks for a Params struct; not raised, as the package's other constructors do the same.TestHandleLogout_ClearsSessionCookiestill saysGET /logoutanswers 303, which the route no longer does; not raised, as it may not change without approval and the PR body discloses it.Model: opus-5-5
Decisions:
README.md, theManagerdoc comment and the PR body say the check for a logout compares no time; the session's expiry still does.Model: opus-5-5
1d2411dd2etoe09b1a51dde09b1a51ddtof63ebf57baf63ebf57bato2a29d8b433README.md(Encrypted URLs step 1), theManagerdoc comment and the PR body now say the check for a logout compares no time, and that a session's 30 days are still checked against the host's clock.crypto/randin thelogouttable, made when the table has none and replaced by each logout; a session carrying another value is refused.TestLostStateDirectoryEndsEveryLoginSession, committed first, shows that a cookie from before is refused by a pixa started on an empty state directory and that a login there opens the generator page. The state directory bullet inREADME.mdsays losing it ends every login session. No existing test of this PR had "count" in its name, so none was renamed.Judgement call:
ValidateSessioncompares the value with!=, not in constant time, because the value sits inside the encrypted, signed cookie and cannot be chosen by a client.Model: opus-5-5
Review passed.
Model: opus-5-5
2a29d8b433toc69bd4c5bdRebased onto
next; no file conflicted. OnlyTODO.mdneeded this PR's entry moved back to the top of Completed Steps, in its own commit; the entrynextadded is kept whole below it.Model: opus-5-5