From ce3fc466b2bfe35cbd5591ccf09bd307060fb5fe Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Thu, 20 Aug 2026 08:29:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(auth):=20andenfaktoren=20kunne=20g=C3=A6tte?= =?UTF-8?q?s=20igennem,=20og=20et=20token=20kunne=20tilmelde=20sin=20egen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To huller fra sikkerhedsgennemgangen, og de er samme historie fortalt to gange: den ene kontrol, der findes til når adgangskoden allerede er tabt, kunne omgås af netop den, der havde adgangskoden. **H1 — spanden blev fyldt af adgangskoden alene.** `handleLogin` nulstillede ratebegrænseren ved hver korrekt adgangskode, *før* den så på, om der var en andenfaktor. Det gav én, der havde kodeordet, ubegrænsede portioner på ti gæt: log ind, få en frisk spand og en frisk ventende session, brug ti gæt på koden, log ind igen. Med tre gyldige koder i ethvert øjeblik og tyve forespørgsler i sekundet er det lige odds inden for en eftermiddag. Spanden ryddes nu først, når login er *færdigt*, og der er kommet en spand mere: koder tælles pr. konto og ikke kun pr. adresse. Adressespanden er slået af enhver med mere end én adresse, hvilket er det almindelige — en telefon og en bærbar er allerede to. Det, der gør en sekscifret kode sikker, er at gæt er en knap ressource, og knapheden skal tælles imod det, der bliver angrebet. Den ventende session får lov at leve. At afslutte den ved første forkerte ciffer blev overvejet og valgt fra: det ville koste en, der taster ét tal forkert, sin adgangskode igen, og det køber næsten ingenting oven i loftet pr. konto. **M3 — et lækket token kunne tilmelde sin egen andenfaktor.** `/auth/totp/setup` lå kun bag `requireAuth` og spurgte om ingenting, mens `disable` altid har krævet adgangskoden: døren åbnede med et token og lukkede kun med et kodeord. Et stjålet token kunne hente hemmeligheden, lægge den i sin egen godkender, bekræfte den og beholde den eneste kopi af gendannelseskoderne. Ejerens næste login ville stoppe ved en kode, de ikke havde, og de kunne ikke slå den fra uden at logge ind først. Et tyveri bliver til et lejemål — den sætning står allerede i filen ved siden af om passkeys, og det er den samme regel. Nu bag `requireSession` sammen med passkeys og sessionerne, og `setup` spørger om adgangskoden ligesom `disable`. Symmetrien er pointen: at slå den til er mindst lige så indgribende som at slå den fra. Og en note kan oprettes inde fra et projekt. At ligge i et projekt *er* måden en note deles på, så det er én handling frem for to — man skulle før oprette noten under Noter og bagefter finde projektet frem i "Del i". Ruden findes nu, også før der er en eneste note, for ellers er der ingen knap dér, hvor man opdager at der skal skrives noget ned. Co-Authored-By: Claude Opus 5 --- internal/httpapi/auth_handlers.go | 43 +++++++++++++- internal/httpapi/auth_test.go | 72 ++++++++++++++++++++++- internal/httpapi/server.go | 30 ++++++++-- internal/httpapi/totp_handlers.go | 17 ++++++ web/e2e/smoke.spec.js | 36 ++++++++++++ web/src/lib/api.js | 2 +- web/src/lib/locales/da.js | 2 + web/src/lib/locales/en.js | 2 + web/src/routes/indstillinger/+page.svelte | 33 +++++++++-- web/src/routes/projekt/[id]/+page.svelte | 57 +++++++++++++++++- 10 files changed, 278 insertions(+), 16 deletions(-) diff --git a/internal/httpapi/auth_handlers.go b/internal/httpapi/auth_handlers.go index 97773fd..788bb2e 100644 --- a/internal/httpapi/auth_handlers.go +++ b/internal/httpapi/auth_handlers.go @@ -154,7 +154,21 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { } } - s.loginLimiter.reset(clientIP(r)) + // The bucket is cleared only when the login is *finished*. + // + // It used to be cleared here, on the strength of the password alone — before + // the second factor had been asked for at all. That handed somebody who + // already had the password an unlimited supply of ten-guess batches: log in, + // get a fresh bucket and a fresh pending session, spend ten guesses on the + // code, log in again. At three valid codes in any moment and twenty requests a + // second, that is even odds inside an afternoon — against the one control that + // exists precisely for when the password is already gone. + // + // So an account with a second factor clears nothing yet. handleLoginTOTP does + // it, once the code is right. + if !user.TOTPEnabled { + s.loginLimiter.reset(clientIP(r)) + } // A session that still needs a code gets a short life of its own: long enough // to fetch a phone, not long enough to be worth stealing. @@ -191,6 +205,19 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) { user := userFrom(r.Context()) session := sessionFrom(r.Context()) + // Counted per account as well as per address. + // + // The address bucket alone is defeated by anybody with more than one, which is + // the ordinary case — a phone and a laptop is already two. A code is six + // digits: the only thing that makes it safe is that guesses are scarce, and + // scarcity has to be counted against the thing being attacked. + if ok, retry := s.totpLimiter.allow(user.ID); !ok { + w.Header().Set("Retry-After", retryAfterSeconds(retry)) + writeError(w, http.StatusTooManyRequests, CodeRateLimited, + "too many verification codes; wait a moment and try again") + return + } + err := auth.VerifyTOTP(user.TOTPSecret, req.Code, time.Now()) if err != nil { // A recovery code is accepted in the same field. Making somebody find a @@ -198,6 +225,15 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) { // introduce a second concept. used, rerr := s.db.UseRecoveryCode(r.Context(), user.ID, req.Code) if rerr != nil || !used { + // The pending session is left alive on purpose. + // + // Ending it on the first wrong digit was considered and rejected: it + // would make somebody who mistyped one number type their password again, + // and it buys almost nothing. What makes a six-digit code safe is that + // guesses are scarce, and the per-account bucket above is what makes + // them scarce — ten an hour, however many times the password is + // re-entered. The attack this closes was never "one session, many + // guesses"; it was "a fresh bucket for every password check". writeError(w, http.StatusUnauthorized, CodeUnauthorized, "that verification code is not valid") return @@ -205,6 +241,11 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) { s.log.Info("recovery code used", "user", user.ID) } + // Finished. Now the buckets may be cleared — both of them, so somebody who + // fumbled the code twice is not still waiting once they get it right. + s.loginLimiter.reset(clientIP(r)) + s.totpLimiter.reset(user.ID) + if err := s.db.PromoteSession(r.Context(), session.ID, s.cfg.SessionTTL); err != nil { s.internal(w, r, "promote session", err) return diff --git a/internal/httpapi/auth_test.go b/internal/httpapi/auth_test.go index af2d55f..1d96318 100644 --- a/internal/httpapi/auth_test.go +++ b/internal/httpapi/auth_test.go @@ -301,7 +301,11 @@ func TestTOTPEnrolmentAndTwoStepLogin(t *testing.T) { // Begin: a secret exists, but 2FA is not on yet — closing the tab here must // not lock the account. - resp, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", nil) + // Med adgangskoden: at slå andenfaktoren til er mindst lige så indgribende som + // at slå den fra, og den har altid spurgt. + resp, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{ + "password": "et langt kodeord", + }) if resp.StatusCode != http.StatusOK { t.Fatalf("totp setup: %d %v", resp.StatusCode, body) } @@ -368,7 +372,9 @@ func TestRecoveryCodeCompletesLoginOnce(t *testing.T) { ts := newTestServer(t) ts.bootstrap(t) - _, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", nil) + _, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{ + "password": "et langt kodeord", + }) secret := body["secret"].(string) _, body = ts.do(t, "POST", "/api/v1/auth/totp/confirm", map[string]string{ "code": codeFor(t, secret), @@ -897,3 +903,65 @@ func TestUserAgentsAreSummarisedForAPerson(t *testing.T) { } } } + +// Andenfaktoren kan ikke slås til uden adgangskoden. +// +// Døren åbnede før med et token og lukkede kun med et kodeord: `disable` har +// altid spurgt, `setup` spurgte om ingenting. Et stjålet token kunne derfor hente +// hemmeligheden, lægge den i sin egen godkender, bekræfte den og beholde den +// eneste kopi af gendannelseskoderne — hvorefter ejerens næste login stoppede ved +// en kode, de ikke havde, og de ikke kunne slå den fra uden at logge ind først. +func TestTOTPCannotBeSwitchedOnWithoutThePassword(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + resp, _ := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{ + "password": "det forkerte kodeord", + }) + if resp.StatusCode != http.StatusUnprocessableEntity { + t.Fatalf("status %d, ventede at en forkert adgangskode blev afvist", resp.StatusCode) + } + + resp, _ = ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{}) + if resp.StatusCode == http.StatusOK { + t.Fatal("en tom adgangskode blev accepteret") + } +} + +// Ratebegrænseren må ikke nulstilles af adgangskoden alene. +// +// Den blev det, og det gav én, der havde kodeordet, ubegrænsede portioner på ti +// gæt: log ind, få en frisk spand og en frisk ventende session, brug ti gæt på +// koden, log ind igen. Andenfaktoren findes præcis til det tilfælde, hvor +// kodeordet allerede er tabt, så den portion skal betales for hver gang. +func TestAPasswordAloneDoesNotRefillTheCodeAttempts(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{ + "password": "et langt kodeord", + }) + secret := body["secret"].(string) + ts.do(t, "POST", "/api/v1/auth/totp/confirm", map[string]string{"code": codeFor(t, secret)}) + ts.do(t, "POST", "/api/v1/auth/logout", nil) + + // Kodeordet er rigtigt hver gang; kun koden er forkert. Uden loftet ville det + // her kunne blive ved i det uendelige. + refused := 0 + for i := 0; i < 14; i++ { + resp, _ := ts.do(t, "POST", "/api/v1/auth/login", map[string]string{ + "email": "kristian@example.dk", "password": "et langt kodeord", + }) + if resp.StatusCode == http.StatusTooManyRequests { + refused++ + continue + } + resp, _ = ts.do(t, "POST", "/api/v1/auth/login/totp", map[string]string{"code": "000000"}) + if resp.StatusCode == http.StatusTooManyRequests { + refused++ + } + } + if refused == 0 { + t.Fatal("fjorten runder med forkerte koder blev aldrig afvist; spanden fyldes stadig af adgangskoden") + } +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 33b8bac..30e82fd 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -64,6 +64,11 @@ type Server struct { // than the rest of the API. loginLimiter *limiter resetLimiter *limiter + // Second factors are counted against the *account*, not the address. An + // address bucket is defeated by anybody with more than one address, which is + // the ordinary case — and a six-digit code is only safe while guesses are + // scarce, so the scarcity has to be counted against the thing being attacked. + totpLimiter *limiter // Computed once from the built index.html; see csp.go. csp string @@ -81,6 +86,10 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server hub: realtime.NewHub(log), loginLimiter: newLimiter(10, 15*time.Minute), resetLimiter: newLimiter(5, time.Hour), + // Tighter than the password bucket, and for a whole hour. Somebody typing + // their own code gets it right on the first or second try; ten in an hour is + // generous for a person and hopeless for a search through a million. + totpLimiter: newLimiter(10, time.Hour), } s.csp = contentSecurityPolicy(scriptHashes(web)) if w, err := auth.NewWebAuthn(cfg.BaseURL, "verdande"); err == nil { @@ -200,11 +209,7 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server r.Put("/nav-order", s.handleSetNavOrder) r.Post("/logout", s.handleLogout) r.Post("/password/change", s.handleChangePassword) - r.Post("/totp/setup", s.handleTOTPSetup) - r.Post("/totp/confirm", s.handleTOTPConfirm) - r.Post("/totp/disable", s.handleTOTPDisable) r.Get("/recovery-codes", s.handleRecoveryCodesCount) - r.Post("/recovery-codes", s.handleRecoveryCodesRegenerate) // Behind requireSession, like the API tokens and for the same // reason: a leaked token must not be able to end the sessions of @@ -214,6 +219,23 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server r.Get("/sessions", s.handleListSessions) r.Delete("/sessions/{sessionID}", s.handleDeleteSession) + // Turning the second factor on, and the codes that get you back + // in. Sessions only, and setup asks for the password. + // + // These sat outside this group, which meant the door opened with + // a token and closed only with a password: `disable` has always + // asked for the password, `setup` asked for nothing. A stolen + // token could therefore fetch the secret, load it into its own + // authenticator, confirm it, and keep the only copy of the + // recovery codes. The owner's next login would stop at a code + // they do not have, and they could not turn it off without + // logging in first. A theft becomes a tenancy — the same + // sentence written next door about passkeys, and the same rule. + r.Post("/totp/setup", s.handleTOTPSetup) + r.Post("/totp/confirm", s.handleTOTPConfirm) + r.Post("/totp/disable", s.handleTOTPDisable) + r.Post("/recovery-codes", s.handleRecoveryCodesRegenerate) + // Registering a key, and the list of them. Sessions only, for // the same reason: a leaked token must not be able to add its // own way in — that would turn a theft into a tenancy. diff --git a/internal/httpapi/totp_handlers.go b/internal/httpapi/totp_handlers.go index ee9f6e6..0458280 100644 --- a/internal/httpapi/totp_handlers.go +++ b/internal/httpapi/totp_handlers.go @@ -20,7 +20,24 @@ type totpSetupResponse struct { } func (s *Server) handleTOTPSetup(w http.ResponseWriter, r *http.Request) { + // The password, the same as disable has always asked for. + // + // Symmetry is the point. Turning the second factor on is at least as + // consequential as turning it off — it decides which device gets to say yes + // from now on — and a door that opens more easily than it closes is not a + // door. Together with the move behind requireSession, this is what stops a + // stolen credential from enrolling its own authenticator and keeping the only + // copy of the recovery codes. + var req totpDisableRequest + if err := decodeJSON(w, r, &req); err != nil { + return + } user := userFrom(r.Context()) + if err := auth.VerifyPassword(user.PasswordHash, req.Password); err != nil { + writeFieldErrors(w, map[string]string{"password": "not correct"}) + return + } + if user.TOTPEnabled { writeError(w, http.StatusConflict, CodeConflict, "two-factor authentication is already switched on") diff --git a/web/e2e/smoke.spec.js b/web/e2e/smoke.spec.js index 4cf6da4..740801d 100644 --- a/web/e2e/smoke.spec.js +++ b/web/e2e/smoke.spec.js @@ -2704,3 +2704,39 @@ test('et billede kan indsættes og trækkes ind i en note', async ({ page }) => expect(trouble).toEqual([]); }); + +test('en note kan oprettes inde fra et projekt og hører til det med det samme', async ({ page }) => { + const trouble = watchForTrouble(page); + await page.goto('/'); + + const sidebar = page.getByRole('navigation', { name: 'Hovedmenu' }); + await sidebar.getByLabel('Nyt projekt').click(); + await sidebar.getByLabel('Projektnavn').fill('Tagprojekt'); + await sidebar.getByLabel('Projektnavn').press('Enter'); + await sidebar.getByRole('link', { name: 'Tagprojekt' }).click(); + await expect(page.getByRole('heading', { name: 'Tagprojekt' })).toBeVisible(); + + // Ruden findes, også før der er en eneste note — ellers er der ingen knap at + // trykke på det sted, hvor man opdager at der skal skrives noget ned. + await page.getByRole('button', { name: 'Ny note' }).click(); + + const ed = page.getByRole('textbox', { name: 'Notens tekst' }); + await expect(ed).toBeVisible(); + await ed.click(); + await page.keyboard.type('Tagsten og lægter'); + await page.waitForTimeout(1200); + + // Den hører til projektet med det samme: "Del i" står på projektet, uden at + // nogen har valgt det bagefter. + await expect(page.getByLabel('Del i')).toHaveValue(/.+/); + const shared = await page.getByLabel('Del i').evaluate((el) => el.selectedOptions[0].textContent); + expect(shared).toContain('Tagprojekt'); + + // Og den står på projektets egen side. + await sidebar.getByRole('link', { name: 'Tagprojekt' }).click(); + // Titlen, ikke uddraget: begge indeholder de samme ord, fordi titlen *er* + // notens første linje. + await expect(page.locator('.notes strong', { hasText: 'Tagsten og lægter' })).toBeVisible(); + + expect(trouble).toEqual([]); +}); diff --git a/web/src/lib/api.js b/web/src/lib/api.js index bd9440c..0a8787d 100644 --- a/web/src/lib/api.js +++ b/web/src/lib/api.js @@ -203,7 +203,7 @@ export const api = { changePassword: (current_password, new_password) => post('/auth/password/change', { current_password, new_password }), - totpSetup: () => post('/auth/totp/setup'), + totpSetup: (password) => post('/auth/totp/setup', { password }), totpConfirm: (code) => post('/auth/totp/confirm', { code }), totpDisable: (password) => post('/auth/totp/disable', { password }), listSessions: () => get('/auth/sessions'), diff --git a/web/src/lib/locales/da.js b/web/src/lib/locales/da.js index c9814db..e128ba5 100644 --- a/web/src/lib/locales/da.js +++ b/web/src/lib/locales/da.js @@ -44,6 +44,8 @@ export const da = { 'beacon.byVersion': 'Efter version (30 dage):', 'beacon.publish': 'Offentliggør tallet på /api/v1/beacon/count', 'beacon.publishHint': 'Uden login, mellemlagret et minut. Viser installationer set de seneste 30 dage — ikke et løbende total, som kun ville stige.', + 'notes.newHere': 'Ny note', + 'notes.noneInProject': 'Ingen noter endnu. En note, der nævner #projektet, dukker også op her.', 'notes.strike': 'Gennemstreget', 'notes.showSource': 'Vis som kode', 'notes.source': 'Notens kildetekst', diff --git a/web/src/lib/locales/en.js b/web/src/lib/locales/en.js index 0897f2b..8763940 100644 --- a/web/src/lib/locales/en.js +++ b/web/src/lib/locales/en.js @@ -40,6 +40,8 @@ export const en = { 'beacon.byVersion': 'By version (30 days):', 'beacon.publish': 'Publish the count at /api/v1/beacon/count', 'beacon.publishHint': 'No login, cached for a minute. Reports installations seen in the last 30 days — not a running total, which would only ever climb.', + 'notes.newHere': 'New note', + 'notes.noneInProject': 'No notes yet. A note that mentions #theproject turns up here too.', 'notes.strike': 'Strikethrough', 'notes.showSource': 'Show as code', 'notes.source': 'Note source', diff --git a/web/src/routes/indstillinger/+page.svelte b/web/src/routes/indstillinger/+page.svelte index 8aa35ef..cfda6ce 100644 --- a/web/src/routes/indstillinger/+page.svelte +++ b/web/src/routes/indstillinger/+page.svelte @@ -143,6 +143,7 @@ let recoveryCodes = $state([]); let remaining = $state(null); let disablePassword = $state(''); + let enablePassword = $state(''); $effect(() => { if (app.user?.totp_enabled) { @@ -150,13 +151,21 @@ } }); - async function beginTOTP() { + async function beginTOTP(event) { + event?.preventDefault?.(); + totpErrors = {}; try { - const r = await api.totpSetup(); + // Adgangskoden, ligesom når man slår den fra. At slå den til er mindst + // lige så indgribende som at slå den fra — det afgør, hvilken enhed der + // fra nu af må sige ja — og en dør, der åbnes lettere end den lukkes, er + // ikke en dør. + const r = await api.totpSetup(enablePassword); + enablePassword = ''; totpSecret = r.secret; totpURI = r.uri; } catch (e) { - app.toast(humanMessage(e)); + totpErrors = e.fields ?? {}; + if (!Object.keys(totpErrors).length) app.toast(humanMessage(e)); } } @@ -475,9 +484,21 @@ {:else} -
- -
+
+
+ + + {#if totpErrors.password}

{totpErrors.password}

{/if} +
+
+ +
+
{/if} diff --git a/web/src/routes/projekt/[id]/+page.svelte b/web/src/routes/projekt/[id]/+page.svelte index a8df623..c16c2d9 100644 --- a/web/src/routes/projekt/[id]/+page.svelte +++ b/web/src/routes/projekt/[id]/+page.svelte @@ -134,6 +134,27 @@ } } + /** + * En ny note, der allerede hører til dette projekt. + * + * Noten oprettes med `project_id` sat, hvilket er præcis det, deling betyder + * her: en note i et projekt kan læses af alle, der kan læse projektet. Så det + * er én handling og ikke to — man skulle før oprette noten under Noter og + * bagefter finde projektet frem i "Del i". + * + * Tom, og så videre til den. Der er ikke noget at spørge om: titlen er notens + * første linje, så et navnefelt ville være et felt, der lover noget, programmet + * ikke holder. + */ + async function createNote() { + try { + const note = await api.createNote({ body: '', project_id: id }); + goto(`/noter?note=${note.id}`); + } catch (e) { + app.toast(humanMessage(e)); + } + } + let canEdit = $derived(project?.role === 'owner' || project?.role === 'editor'); let isOwner = $derived(project?.role === 'owner'); @@ -826,9 +847,22 @@ - {#if notes.length} + {#if notes.length || canEdit}
-

{t('notes.title')}

+
+

{t('notes.title')}

+ {#if canEdit} + + + {/if} +
+ {#if !notes.length} +

{t('notes.noneInProject')}

+ {/if}
    {#each notes as note (note.id)}
  • @@ -863,6 +897,25 @@ border-top: 1px solid var(--line); } + .noteshead { + display: flex; + align-items: baseline; + justify-content: space-between; + gap: var(--s2); + } + + .newnote { + font-size: var(--text-sm); + color: var(--ink-muted); + padding: 2px var(--s2); + border-radius: var(--radius-sm); + } + + .newnote:hover { + background: var(--surface); + color: var(--ink); + } + .notes h2 { font-size: var(--text-xs); text-transform: uppercase;