Add inactivity-based session timeout #66

New Issue

clawbot · 2026-08-07T13:11:16+02:00

clawbot commented

2026-08-07 13:11:16 +02:00

Post-1.0 hardening (surfaced during the 1.0 review, see #33 — not a 1.0 blocker).

Sessions have only a 7-day absolute MaxAge (internal/session/session.go, sessionMaxAgeDays). An idle session stays valid for the full 7 days regardless of activity.

Definition of done:

sessions also expire after a configurable idle period (sliding expiry, refreshed on activity)
the absolute cap remains as a backstop
covered by a test

Post-1.0 hardening (surfaced during the 1.0 review, see #33 — not a 1.0 blocker). Sessions have only a 7-day absolute `MaxAge` (`internal/session/session.go`, `sessionMaxAgeDays`). An idle session stays valid for the full 7 days regardless of activity. Definition of done: - sessions also expire after a configurable idle period (sliding expiry, refreshed on activity) - the absolute cap remains as a backstop - covered by a test

clawbot referenced this issue

2026-08-07 13:12:23 +02:00

1.0/mvp #33

clawbot referenced this issue

2026-08-07 18:07:44 +02:00

1.0/mvp #33

clawbot commented

2026-08-09 07:46:12 +02:00

Implementation requirements

Baseline: main @ 4f5ecb1. This is post-1.0 hardening per #33, not a 1.0 blocker — keep it proportionate and resist redesigning session handling.

1. Sliding idle expiry alongside the absolute cap

Sessions currently have only a 7-day absolute MaxAge (internal/session/session.go, sessionMaxAgeDays). Add an idle timeout that is refreshed on activity, and keep the absolute cap as a backstop — the two are independent and both must be able to end a session. A session must expire when either the idle window lapses or the absolute age is reached, whichever comes first.

Be explicit in the code about which clock is which. The common bug here is refreshing the absolute deadline along with the idle one, which quietly turns a 7-day hard cap into an unbounded session for an active user. Guard that with a test.

2. Configuration

Add one env-configured duration for the idle window, parsed with the existing envDuration helper in internal/config — it is already fail-loud on a set-but-unparseable value (#78), which is the repo policy. Pick a sane default and document it.

Conflict warning: PR #92 (#80) is merge-ready but unmerged and rewrites internal/config, moving env loading into a new loadFromEnv. Adding a key here will conflict there. Keep the addition minimal — one constant, one Config field, one parse call — so the rebase is mechanical. Do not restructure anything else in that package. Note envDuration itself is unchanged by #92, so the parsing call site is the only overlap.

3. Refresh on activity, deliberately

Decide and document what counts as "activity". The obvious answer is any authenticated request, refreshed in the auth middleware. Two things to get right:

Write amplification. Refreshing the session store on literally every request means a write per request. If the store makes that expensive, refresh only when the remaining idle window has dropped below some fraction of the total. If you do that, say so in the PR body — a lazily-refreshed idle timeout expires slightly early, and that tradeoff should be chosen rather than stumbled into.
Do not refresh on unauthenticated requests, or a logged-out attacker could keep someone else's session alive by hitting a public endpoint with their cookie.

4. Tests

A session idle past the window is rejected.
A session kept active stays valid past the idle window but still dies at the absolute cap — this is the test that catches the refresh-the-wrong-clock bug.
Activity refreshes the idle deadline.
A set-but-unparseable idle-timeout value aborts startup (consistent with #80).
Mutation-verify: with the idle check removed, the first test must fail. State the evidence in the PR body.

Use fake or injected time rather than real sleeps — make test must stay under 20 seconds per REPO_POLICIES.md.

5. Docs

README: the new variable, its default, and the interaction between idle and absolute expiry. TODO.md in the same commit.

Definition of done

The issue's own DoD, plus items 1-5, make check green via the repo's own entrypoints, .golangci.yml untouched, single commit whose title ends with (closes #66), no attribution trailers.

## Implementation requirements Baseline: `main` @ `4f5ecb1`. This is post-1.0 hardening per #33, not a 1.0 blocker — keep it proportionate and resist redesigning session handling. ### 1. Sliding idle expiry alongside the absolute cap Sessions currently have only a 7-day absolute `MaxAge` (`internal/session/session.go`, `sessionMaxAgeDays`). Add an idle timeout that is refreshed on activity, and **keep the absolute cap as a backstop** — the two are independent and both must be able to end a session. A session must expire when *either* the idle window lapses or the absolute age is reached, whichever comes first. Be explicit in the code about which clock is which. The common bug here is refreshing the absolute deadline along with the idle one, which quietly turns a 7-day hard cap into an unbounded session for an active user. Guard that with a test. ### 2. Configuration Add one env-configured duration for the idle window, parsed with the **existing** `envDuration` helper in `internal/config` — it is already fail-loud on a set-but-unparseable value (#78), which is the repo policy. Pick a sane default and document it. **Conflict warning:** PR #92 (#80) is merge-ready but unmerged and rewrites `internal/config`, moving env loading into a new `loadFromEnv`. Adding a key here will conflict there. Keep the addition minimal — one constant, one `Config` field, one parse call — so the rebase is mechanical. Do **not** restructure anything else in that package. Note `envDuration` itself is unchanged by #92, so the parsing call site is the only overlap. ### 3. Refresh on activity, deliberately Decide and document what counts as "activity". The obvious answer is any authenticated request, refreshed in the auth middleware. Two things to get right: - **Write amplification.** Refreshing the session store on literally every request means a write per request. If the store makes that expensive, refresh only when the remaining idle window has dropped below some fraction of the total. If you do that, say so in the PR body — a lazily-refreshed idle timeout expires slightly early, and that tradeoff should be chosen rather than stumbled into. - **Do not refresh on unauthenticated requests**, or a logged-out attacker could keep someone else's session alive by hitting a public endpoint with their cookie. ### 4. Tests - A session idle past the window is rejected. - A session kept active stays valid past the idle window but **still dies at the absolute cap** — this is the test that catches the refresh-the-wrong-clock bug. - Activity refreshes the idle deadline. - A set-but-unparseable idle-timeout value aborts startup (consistent with #80). - Mutation-verify: with the idle check removed, the first test must fail. State the evidence in the PR body. Use fake or injected time rather than real sleeps — `make test` must stay under 20 seconds per `REPO_POLICIES.md`. ### 5. Docs README: the new variable, its default, and the interaction between idle and absolute expiry. `TODO.md` in the **same commit**. ### Definition of done The issue's own DoD, plus items 1-5, `make check` green via the repo's own entrypoints, `.golangci.yml` untouched, single commit whose title ends with ` (closes #66)`, no attribution trailers.

clawbot commented

2026-08-09 07:50:41 +02:00

Implementation plan

Branch issue-66-session-idle-timeout off main @ 4f5ecb1. Single commit, title ends with (closes #66).

Two independent clocks, both stored in the session

Sessions are gorilla/sessions cookie sessions, so both anchors live in the session values as Unix seconds:

created_at (CreatedAtKey) — anchors the absolute cap. Written once by SetUser at login and never rewritten. Absolute deadline = created_at + 7d (the existing sessionMaxAgeDays, expressed as sessionAbsoluteMaxAge).
last_seen (LastSeenKey) — anchors the idle window. Written by SetUser and refreshed by the new Touch. Idle deadline = last_seen + SessionIdleTimeout.

A new unexported Session.expired(sess) checks the absolute deadline first, then the idle deadline, and returns true if either has passed. IsAuthenticated gains that check, which is the single choke point every caller already goes through (auth middleware, handlers/index.go, handlers/auth.go, handlers/profile.go, source_management.go), so there is no path that honours one clock but not the other.

Fail-closed detail: an authenticated session with no created_at (a cookie minted before this change) is treated as expired. Existing logins are invalidated once on deploy.

Also worth noting: the absolute cap was previously enforced only by the cookie's MaxAge, i.e. only by the browser. created_at makes it server-side.

Config

Exactly three lines in internal/config, to keep the #92 rebase mechanical:

defaultSessionIdleTimeout = 24 * time.Hour
Config.SessionIdleTimeout time.Duration
one envDuration("SESSION_IDLE_TIMEOUT", defaultSessionIdleTimeout) call, error returned so fx aborts startup on a set-but-unparseable value

Nothing else in that package is touched. A non-positive value disables idle expiry (absolute cap still applies); documented in the README.

Activity, and where it is refreshed

Activity = a request that passes RequireAuth. Touch is called there and only there, after IsAuthenticated has succeeded, and the session is saved before the wrapped handler runs. Unauthenticated requests take the redirect branch and never reach it, so a public endpoint hit with someone else's cookie cannot extend that session. Touch also re-checks IsAuthenticated itself and returns false otherwise, so the guarantee does not depend on the call site.

Touch never writes created_at.

Write amplification

Saving on literally every authenticated request means re-encrypting and re-emitting a Set-Cookie on every response. Touch therefore only rewrites last_seen once it is older than idleTimeout/10, and returns a bool so the middleware saves only when something changed. Consequence, chosen deliberately: last_seen lags real activity by up to idleTimeout/10, so a session can expire up to 10% early relative to true last activity — never late. This will be stated in the PR body.

Tests

Injected clock (now func() time.Time on Session, threaded through NewForTest) — no real sleeps.

idle past the window -> rejected
kept active across the whole 7 days -> survives the idle window repeatedly but still dies at the absolute cap (the refresh-the-wrong-clock test)
Touch refreshes the idle deadline, and the refreshed session outlives the original deadline
Touch is a no-op below the lazy-refresh threshold, and on an unauthenticated session
missing timestamps -> rejected
SessionIdleTimeout <= 0 -> idle disabled, absolute cap still enforced
middleware: authenticated request refreshes and re-issues the cookie; idle-expired cookie redirects to login; unauthenticated request sets no refreshed cookie
config: set-but-unparseable SESSION_IDLE_TIMEOUT aborts startup (mirrors the RETENTION_SWEEP_INTERVAL table test)

Mutation check: delete the idle branch from expired, confirm the idle-expiry test fails, restore. Evidence in the PR body.

Docs

README: SESSION_IDLE_TIMEOUT row (default 24h) plus a short paragraph on how the idle and absolute clocks interact. TODO.md updated in the same commit.

Verification via make fmt, make check, script/cibuild only.

## Implementation plan Branch `issue-66-session-idle-timeout` off `main` @ `4f5ecb1`. Single commit, title ends with ` (closes #66)`. ### Two independent clocks, both stored in the session Sessions are `gorilla/sessions` cookie sessions, so both anchors live in the session values as Unix seconds: - `created_at` (`CreatedAtKey`) — anchors the **absolute** cap. Written once by `SetUser` at login and **never** rewritten. Absolute deadline = `created_at + 7d` (the existing `sessionMaxAgeDays`, expressed as `sessionAbsoluteMaxAge`). - `last_seen` (`LastSeenKey`) — anchors the **idle** window. Written by `SetUser` and refreshed by the new `Touch`. Idle deadline = `last_seen + SessionIdleTimeout`. A new unexported `Session.expired(sess)` checks the absolute deadline first, then the idle deadline, and returns true if either has passed. `IsAuthenticated` gains that check, which is the single choke point every caller already goes through (auth middleware, `handlers/index.go`, `handlers/auth.go`, `handlers/profile.go`, `source_management.go`), so there is no path that honours one clock but not the other. Fail-closed detail: an authenticated session with no `created_at` (a cookie minted before this change) is treated as expired. Existing logins are invalidated once on deploy. Also worth noting: the absolute cap was previously enforced only by the cookie's `MaxAge`, i.e. only by the browser. `created_at` makes it server-side. ### Config Exactly three lines in `internal/config`, to keep the #92 rebase mechanical: - `defaultSessionIdleTimeout = 24 * time.Hour` - `Config.SessionIdleTimeout time.Duration` - one `envDuration("SESSION_IDLE_TIMEOUT", defaultSessionIdleTimeout)` call, error returned so fx aborts startup on a set-but-unparseable value Nothing else in that package is touched. A non-positive value disables idle expiry (absolute cap still applies); documented in the README. ### Activity, and where it is refreshed Activity = a request that passes `RequireAuth`. `Touch` is called there and **only** there, after `IsAuthenticated` has succeeded, and the session is saved before the wrapped handler runs. Unauthenticated requests take the redirect branch and never reach it, so a public endpoint hit with someone else's cookie cannot extend that session. `Touch` also re-checks `IsAuthenticated` itself and returns false otherwise, so the guarantee does not depend on the call site. `Touch` never writes `created_at`. ### Write amplification Saving on literally every authenticated request means re-encrypting and re-emitting a `Set-Cookie` on every response. `Touch` therefore only rewrites `last_seen` once it is older than `idleTimeout/10`, and returns a bool so the middleware saves only when something changed. Consequence, chosen deliberately: `last_seen` lags real activity by up to `idleTimeout/10`, so a session can expire up to 10% early relative to true last activity — never late. This will be stated in the PR body. ### Tests Injected clock (`now func() time.Time` on `Session`, threaded through `NewForTest`) — no real sleeps. - idle past the window -> rejected - **kept active across the whole 7 days -> survives the idle window repeatedly but still dies at the absolute cap** (the refresh-the-wrong-clock test) - `Touch` refreshes the idle deadline, and the refreshed session outlives the original deadline - `Touch` is a no-op below the lazy-refresh threshold, and on an unauthenticated session - missing timestamps -> rejected - `SessionIdleTimeout <= 0` -> idle disabled, absolute cap still enforced - middleware: authenticated request refreshes and re-issues the cookie; idle-expired cookie redirects to login; unauthenticated request sets no refreshed cookie - config: set-but-unparseable `SESSION_IDLE_TIMEOUT` aborts startup (mirrors the `RETENTION_SWEEP_INTERVAL` table test) Mutation check: delete the idle branch from `expired`, confirm the idle-expiry test fails, restore. Evidence in the PR body. ### Docs README: `SESSION_IDLE_TIMEOUT` row (default `24h`) plus a short paragraph on how the idle and absolute clocks interact. `TODO.md` updated in the same commit. Verification via `make fmt`, `make check`, `script/cibuild` only.

clawbot referenced this issue from a commit

2026-08-09 08:04:18 +02:00

Add inactivity-based session timeout (closes #66)

clawbot referenced a pull request that will close this issue

2026-08-09 08:05:33 +02:00

Add inactivity-based session timeout (closes #66) #105

clawbot referenced this issue

2026-08-09 08:19:57 +02:00

Session codec max-age still 30 days: bring securecookie codecs in line with the 7-day cap #108

Sign in to join this conversation.