issue 1859: mark the page after an act by a one-shot cookie, not its address

Review round 2 F3 (ADR 0517 §3, 1858/AC5): acted=1 stayed in the address, so a reload, a restored tab or a bookmark of it recorded no page opened or opened. An act's 303 (and a mode chosen) now sets mail_acted for 10 seconds, HttpOnly and SameSite=Strict, Secure but on 127.0.0.1; the next list or pane it reaches records nothing and clears it; the address carries no marker. The local run keeps the browser's other cookies.
2026-10-22 20:29:17 +02:00
parent af89a69670
commit d14ef647d6
9 changed files with 153 additions and 66 deletions
@@ -88,7 +88,14 @@ func start(addr string, now time.Time) (*localRun, error) {
 	}
 	page := web.NewServer(host, &web.Auth{Sessions: sessions, Pending: web.NewPendingAuths(clock)}, newBundle(w))
 	signedIn := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		// Every cookie the browser carries but the session's own (the act's, web.ActedCookie, among them).
+		kept := req.Cookies()
 		req.Header.Del("Cookie")
+		for _, c := range kept {
+			if c.Name != web.SessionCookie {
+				req.AddCookie(c)
+			}
+		}
 		req.AddCookie(&http.Cookie{Name: web.SessionCookie, Value: id})
 		page.ServeHTTP(rw, req)
 	})
@@ -10,10 +10,10 @@ package web
 import (
 	"context"
 	"encoding/json"
+	"net"
 	"net/http"
 	"net/url"
 	"strconv"
-	"strings"
 	"time"
 )

@@ -39,33 +39,44 @@ func (b *Bundle) actFailed(w http.ResponseWriter, r *http.Request, mode Mode, ca
 	renderNamed(w, http.StatusBadGateway, "list", data)
 }

-// actDone answers an act that succeeded: done to the script, or back to the page at back, marked as the page
-// after an act (ActedParam) so it records nothing.
+// actDone answers an act that succeeded: done to the script, or back to the page at back with the act's own
+// cookie (ActedCookie), so the page shown after it records nothing.
 func actDone(w http.ResponseWriter, r *http.Request, back string) {
 	if byScript(r) {
 		answerJSON(w, http.StatusOK, map[string]any{"done": true})
 		return
 	}
-	http.Redirect(w, r, afterAct(back), http.StatusSeeOther)
+	setActed(w, r, actedFor)
+	http.Redirect(w, r, back, http.StatusSeeOther)
 }

-// ActedParam marks the page a form act goes back to (review round 1 F1; ADR 0517 §5: no act writes two
-// lines): that page records neither page opened nor opened, since the act already wrote its one line. The
-// script's own refresh says the same with RefreshHeader.
-const ActedParam = "acted"
+// actedFor is how long the act's cookie lives: the one GET a browser makes at once for the act's 303, or the
+// script's navigation after a mode chosen (review round 2 F3).
+const actedFor = 10

-// afterAct is back with ActedParam added.
-func afterAct(back string) string {
-	sep := "?"
-	if strings.Contains(back, "?") {
-		sep = "&"
+// setActed sets the act's cookie for maxAge seconds, or clears it with a negative maxAge: HttpOnly,
+// SameSite=Strict, and Secure as the session cookie is, but on this machine's own 127.0.0.1 (the local run,
+// cmd/mail-page-dev, served over plain http).
+func setActed(w http.ResponseWriter, r *http.Request, maxAge int) {
+	host := r.Host
+	if h, _, err := net.SplitHostPort(host); err == nil {
+		host = h
 	}
-	return back + sep + ActedParam + "=1"
+	http.SetCookie(w, &http.Cookie{Name: ActedCookie, Value: "1", Path: "/", MaxAge: maxAge, HttpOnly: true,
+		SameSite: http.SameSiteStrictMode, Secure: host != "127.0.0.1"})
 }

-// afterAnAct says whether r asks the page shown after an act: by ActedParam, or the script's refresh.
-func afterAnAct(r *http.Request) bool {
-	return r.URL.Query().Get(ActedParam) != "" || r.Header.Get(RefreshHeader) != ""
+// afterAnAct says whether r asks the page shown after an act — by the act's cookie, or the script's refresh —
+// and clears the cookie, so it holds for that one page alone (review rounds 1 and 2, F1 and F3).
+func afterAnAct(w http.ResponseWriter, r *http.Request) bool {
+	if r.Header.Get(RefreshHeader) != "" {
+		return true
+	}
+	if _, err := r.Cookie(ActedCookie); err != nil {
+		return false
+	}
+	setActed(w, r, -1)
+	return true
 }

 // listAddress is the list page's own address for mode.
@@ -121,10 +132,12 @@ func (b *Bundle) modeHandler(w http.ResponseWriter, r *http.Request) {
 		b.actFailed(w, r, mode, "The mode chosen could not be kept: "+err.Error())
 		return
 	}
-	// The mode chosen is its own line: the list it goes to records nothing more (F1).
+	// The mode chosen is its own line: the list it goes to records nothing more (F1, F3), by the act's cookie,
+	// with or without the script.
+	setActed(w, r, actedFor)
 	if byScript(r) {
-		answerJSON(w, http.StatusOK, map[string]any{"done": true, "go": afterAct(listAddress(mode))})
+		answerJSON(w, http.StatusOK, map[string]any{"done": true, "go": listAddress(mode)})
 		return
 	}
-	http.Redirect(w, r, afterAct(listAddress(mode)), http.StatusSeeOther)
+	http.Redirect(w, r, listAddress(mode), http.StatusSeeOther)
 }
@@ -55,9 +55,9 @@ func TestTickingUndoingAndRemindingCallTheirVerbs(t *testing.T) {

 			code, _, h := post(t, s, "/item", form, cookie, false)
 			calls := reading.callsOf(act)
-			if code != http.StatusSeeOther || h.Get("Location") != "/?mode=work&acted=1" || len(calls) != 1 ||
+			if code != http.StatusSeeOther || h.Get("Location") != "/?mode=work" || len(calls) != 1 ||
 				calls[0].Args["id"] != float64(3) || calls[0].Args["item"] != "task 1" || calls[0].Args["mode"] != "work" {
-				t.Fatalf("a post of %s: status %d to %q, calls %v; want 303 to /?mode=work&acted=1 and one call", act, code, h.Get("Location"), calls)
+				t.Fatalf("a post of %s: status %d to %q, calls %v; want 303 to /?mode=work and one call", act, code, h.Get("Location"), calls)
 			}
 			code, body, _ := post(t, s, "/item", form, cookie, true)
 			if code != http.StatusOK || !strings.Contains(body, `"done":true`) || len(reading.callsOf(act)) != 2 {
@@ -98,12 +98,12 @@ func TestChoosingAModeRecordsItAndShowsIt(t *testing.T) {
 	s, cookie, token := signedIn(t, reading.bundle(time.Now()))
 	code, _, h := post(t, s, "/mode", url.Values{"mode": {"personal"}, FormTokenField: {token}}, cookie, false)
 	rec := reading.callsOf("record")
-	if code != http.StatusSeeOther || h.Get("Location") != "/?mode=personal&acted=1" || len(rec) != 1 ||
+	if code != http.StatusSeeOther || h.Get("Location") != "/?mode=personal" || len(rec) != 1 ||
 		rec[0].Args["what"] != "mode" || rec[0].Args["mode"] != "personal" {
 		t.Fatalf("choosing personal: status %d to %q, records %v", code, h.Get("Location"), rec)
 	}
 	code, body, _ := post(t, s, "/mode", url.Values{"mode": {"work"}, FormTokenField: {token}}, cookie, true)
-	if code != http.StatusOK || !strings.Contains(body, `"go":"/?mode=work\u0026acted=1"`) {
+	if code != http.StatusOK || !strings.Contains(body, `"go":"/?mode=work"`) {
 		t.Fatalf("choosing work by the script: status %d %s", code, body)
 	}
 }
@@ -378,7 +378,32 @@ func actOnPage(t *testing.T, mw *startedMailWeb, r *readerBundle, path string, f
 	if resp.StatusCode != http.StatusSeeOther {
 		t.Fatalf("POST %s %v: status %d, want 303:\n%s", path, form, resp.StatusCode, body)
 	}
-	page := pageBody(t, mw, resp.Header.Get("Location"), cookie)
+	// Followed as a browser follows it: with the session's cookie and the act's own (ActedCookie).
+	back := resp.Header.Get("Location")
+	if strings.Contains(back, "acted") {
+		t.Fatalf("POST %s went back to %s, an address with a marker in it", path, back)
+	}
+	req, err := http.NewRequest(http.MethodGet, "http://"+mw.internalHost+back, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	req.Host = mw.internalHost
+	req.AddCookie(cookie)
+	for _, c := range resp.Cookies() {
+		if c.Name == ActedCookie {
+			req.AddCookie(c)
+		}
+	}
+	shown, err := mw.client.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pageRaw, _ := io.ReadAll(shown.Body)
+	_ = shown.Body.Close()
+	if shown.StatusCode != http.StatusOK {
+		t.Fatalf("GET %s after POST %s: status %d:\n%s", back, path, shown.StatusCode, pageRaw)
+	}
+	page := string(pageRaw)
 	after := allLines(t, r)
 	if len(after) != len(before)+1 || after[0]["what"] != what {
 		t.Fatalf("POST %s and the page it went back to left the lines %v after %v; want exactly one more, %s", path, after, before, what)
@@ -371,7 +371,7 @@ func (b *Bundle) listPage(w http.ResponseWriter, r *http.Request) {
 	ctx, cancel := context.WithTimeout(r.Context(), 20*time.Second)
 	defer cancel()
 	// Opening the page records it; the page shown after an act does not (review round 1 F1).
-	data := b.listDataOf(ctx, r, modeNamed(r.URL.Query().Get("mode")), !afterAnAct(r))
+	data := b.listDataOf(ctx, r, modeNamed(r.URL.Query().Get("mode")), !afterAnAct(w, r))
 	renderNamed(w, http.StatusOK, "list", data)
 }

@@ -1,64 +1,102 @@
 package web

-// Review round 1 F1 (novox/hq ADR 0517 §3, §5: no act writes two lines): the page shown after an act — the
-// redirect a form act answers, or the script's refresh — records nothing; opening the page or a mail does.
+// Review rounds 1 and 2, F1 and F3 (novox/hq ADR 0517 §3, §5: no act writes two lines; the page opened is
+// last looked): the page shown after an act — reached once, by the act's own short-lived cookie, or by the
+// script's refresh — records nothing; any other opening of that same address records as ever, and the
+// address carries no marker.

 import (
 	"net/http"
+	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
-	"time"
 )

-// followed is GET of the address a 303 named, with cookie, as a browser follows it.
-func followed(t *testing.T, s http.Handler, location string, cookie *http.Cookie, header map[string]string) int {
-	t.Helper()
-	r := newGet(location, cookie)
+// getWith is a GET of path carrying cookies and headers, answering the recorder.
+func getWith(s http.Handler, path string, cookies []*http.Cookie, header map[string]string) *httptest.ResponseRecorder {
+	r := newGet(path, nil)
+	for _, c := range cookies {
+		r.AddCookie(c)
+	}
 	for k, v := range header {
 		r.Header.Set(k, v)
 	}
-	return serve(s, r).Code
+	return serve(s, r)
 }

-// 1859/AC5, 1858/AC4: a tick, undo, remind or correction, and the page it goes back to, record no line;
-// the act's own line is its verb's (the reader's), so the page asks record nothing at all.
-func TestThePageShownAfterAnActRecordsNothing(t *testing.T) {
-	for _, tc := range []struct {
-		path string
-		form url.Values
-	}{
-		{"/item", url.Values{"id": {"3"}, "item": {"task 1"}, "mode": {"everything"}, "act": {"handle"}}},
-		{"/item", url.Values{"id": {"3"}, "item": {"task 1"}, "mode": {"everything"}, "act": {"remind"}}},
-		{"/correct", url.Values{"id": {"3"}, "mode": {"work"}, "read-later": {"on"}}},
-	} {
-		reading, b := paneBundle(t)
-		s, cookie, token := signedIn(t, b)
-		tc.form.Set(FormTokenField, token)
-		code, _, h := post(t, s, tc.path, tc.form, cookie, false)
-		if code != http.StatusSeeOther {
-			t.Fatalf("%s: status %d", tc.path, code)
-		}
-		if code := followed(t, s, h.Get("Location"), cookie, nil); code != http.StatusOK {
-			t.Fatalf("%s: the page after it: status %d", tc.path, code)
-		}
-		if rec := reading.callsOf("record"); len(rec) != 0 {
-			t.Fatalf("%s and the page it went back to (%s) recorded %v, want nothing: the act's line is its own", tc.path, h.Get("Location"), rec)
+// postAct posts form to path with the session's cookie, answering the recorder.
+func postAct(s http.Handler, path string, form url.Values, cookie *http.Cookie) *httptest.ResponseRecorder {
+	r := httptest.NewRequest(http.MethodPost, path, strings.NewReader(form.Encode()))
+	r.Host = testHost
+	r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	r.AddCookie(cookie)
+	return serve(s, r)
+}
+
+func actedCookie(w *httptest.ResponseRecorder) *http.Cookie {
+	for _, c := range w.Result().Cookies() {
+		if c.Name == ActedCookie {
+			return c
 		}
 	}
-	// The script's own refresh after an act records nothing either.
+	return nil
+}
+
+// 1859/AC5, 1858/AC4, AC5: after a tick, a reminder, a correction or a mode chosen, the page it goes back to
+// records nothing, once; opening that same address again records page opened (or opened, for a mail).
+func TestThePageAfterAnActRecordsNothingOnceAndThenAsEver(t *testing.T) {
+	for _, tc := range []struct {
+		path, again string
+		form        url.Values
+	}{
+		{"/item", "page opened", url.Values{"id": {"3"}, "item": {"task 1"}, "mode": {"everything"}, "act": {"handle"}}},
+		{"/item", "page opened", url.Values{"id": {"3"}, "item": {"task 1"}, "mode": {"everything"}, "act": {"remind"}}},
+		{"/mode", "page opened", url.Values{"mode": {"everything"}}},
+		{"/correct", "opened", url.Values{"id": {"3"}, "mode": {"everything"}, "read-later": {"on"}}},
+	} {
+		reading, b := paneBundle(t)
+		s, session, token := signedIn(t, b)
+		tc.form.Set(FormTokenField, token)
+		w := postAct(s, tc.path, tc.form, session)
+		back := w.Header().Get("Location")
+		if w.Code != http.StatusSeeOther || strings.Contains(back, "acted") {
+			t.Fatalf("%s: status %d to %q; want 303 to an address with no marker", tc.path, w.Code, back)
+		}
+		acted := actedCookie(w)
+		if acted == nil || acted.MaxAge <= 0 || acted.MaxAge > 60 || !acted.HttpOnly || acted.SameSite != http.SameSiteStrictMode {
+			t.Fatalf("%s: the act's cookie %+v, want a short-lived, HttpOnly, SameSite=Strict one", tc.path, acted)
+		}
+		before := len(reading.callsOf("record")) // the mode chosen is its own one line
+		shown := getWith(s, back, []*http.Cookie{session, acted}, nil)
+		if shown.Code != http.StatusOK || len(reading.callsOf("record")) != before {
+			t.Fatalf("%s: the page after it (%s): status %d, records %v; want nothing recorded", tc.path, back, shown.Code, reading.callsOf("record"))
+		}
+		if cleared := actedCookie(shown); cleared == nil || cleared.MaxAge >= 0 {
+			t.Fatalf("%s: the page after it did not clear the act's cookie: %+v", tc.path, cleared)
+		}
+		// A reload, the tab restored the next morning, a bookmark: the same address, without the cookie.
+		getWith(s, back, []*http.Cookie{session}, nil)
+		rec := reading.callsOf("record")
+		if len(rec) != before+1 || rec[len(rec)-1].Args["what"] != tc.again {
+			t.Fatalf("%s: opening %s again recorded %v; want one %s more", tc.path, back, rec, tc.again)
+		}
+	}
+}
+
+// The script's own refresh after an act records nothing; opening the page and a mail record one line each.
+func TestTheScriptsRefreshRecordsNothing(t *testing.T) {
 	reading, b := paneBundle(t)
-	s, cookie, _ := signedIn(t, b)
+	s, session, _ := signedIn(t, b)
 	for _, path := range []string{"/?mode=everything", "/read?id=3&mode=everything"} {
-		followed(t, s, path, cookie, map[string]string{RefreshHeader: "1"})
+		getWith(s, path, []*http.Cookie{session}, map[string]string{RefreshHeader: "1"})
 	}
 	if rec := reading.callsOf("record"); len(rec) != 0 {
 		t.Fatalf("the script's refreshes recorded %v, want nothing", rec)
 	}
-	// Opening the page and a mail still record one line each.
-	followed(t, s, "/?mode=everything", cookie, nil)
-	followed(t, s, "/read?id=3&mode=work", cookie, nil)
+	getWith(s, "/?mode=everything", []*http.Cookie{session}, nil)
+	getWith(s, "/read?id=3&mode=everything", []*http.Cookie{session}, nil)
 	if rec := reading.callsOf("record"); len(rec) != 2 || rec[0].Args["what"] != "page opened" || rec[1].Args["what"] != "opened" {
 		t.Fatalf("opening the page and a mail recorded %v, want page opened then opened", rec)
 	}
-	_ = time.Now
 }
@@ -188,7 +188,7 @@ func (b *Bundle) readPage(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	// Opening a mail records it; the pane shown again after a correction does not (review round 1 F1).
-	b.renderRead(w, r, id, modeNamed(r.URL.Query().Get("mode")), http.StatusOK, "", !afterAnAct(r))
+	b.renderRead(w, r, id, modeNamed(r.URL.Query().Get("mode")), http.StatusOK, "", !afterAnAct(w, r))
 }

 // correctHandler is POST /correct (1859/AC7): a correction of one mail entry — mark and mail group, not a
@@ -91,7 +91,7 @@ func TestEachCorrectionCallsCorrect(t *testing.T) {
 		tc.form.Set(FormTokenField, token)
 		code, _, h := post(t, s, "/correct", tc.form, cookie, false)
 		calls := reading.callsOf("correct")
-		if code != http.StatusSeeOther || h.Get("Location") != "/read?id=3&mode=work&acted=1" || len(calls) != 1 {
+		if code != http.StatusSeeOther || h.Get("Location") != "/read?id=3&mode=work" || len(calls) != 1 {
 			t.Fatalf("correct %v: status %d to %q, calls %v", tc.form, code, h.Get("Location"), calls)
 		}
 		want := map[string]any{"id": float64(3), "mode": "work"}
@@ -39,3 +39,7 @@ func staticPage(w http.ResponseWriter, r *http.Request) {
 // RefreshHeader marks the script's own refresh of the page after an act (review round 1 F1): the page it
 // answers records nothing, since the act already wrote its one line.
 const RefreshHeader = "X-Mail-Refresh"
+
+// ActedCookie is set by an act's own 303 (review round 2 F3): the one next GET of the list or the pane sees it,
+// records nothing, and clears it — never a marker in the address, which a reload or a bookmark would keep.
+const ActedCookie = "mail_acted"