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}
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}
-