diff --git a/modules/mesh-mail-web/cmd/mail-page-dev/main.go b/modules/mesh-mail-web/cmd/mail-page-dev/main.go index 64b4ae62..e5580181 100644 --- a/modules/mesh-mail-web/cmd/mail-page-dev/main.go +++ b/modules/mesh-mail-web/cmd/mail-page-dev/main.go @@ -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) }) diff --git a/modules/mesh-mail-web/internal/web/acts.go b/modules/mesh-mail-web/internal/web/acts.go index 4208fc3f..a16f988e 100644 --- a/modules/mesh-mail-web/internal/web/acts.go +++ b/modules/mesh-mail-web/internal/web/acts.go @@ -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) } diff --git a/modules/mesh-mail-web/internal/web/acts_test.go b/modules/mesh-mail-web/internal/web/acts_test.go index 5bc67020..f89c380f 100644 --- a/modules/mesh-mail-web/internal/web/acts_test.go +++ b/modules/mesh-mail-web/internal/web/acts_test.go @@ -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) } } diff --git a/modules/mesh-mail-web/internal/web/list_integration_test.go b/modules/mesh-mail-web/internal/web/list_integration_test.go index dcf1ad42..821eefd6 100644 --- a/modules/mesh-mail-web/internal/web/list_integration_test.go +++ b/modules/mesh-mail-web/internal/web/list_integration_test.go @@ -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) diff --git a/modules/mesh-mail-web/internal/web/listpage.go b/modules/mesh-mail-web/internal/web/listpage.go index 1114f27e..e78e1946 100644 --- a/modules/mesh-mail-web/internal/web/listpage.go +++ b/modules/mesh-mail-web/internal/web/listpage.go @@ -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) } diff --git a/modules/mesh-mail-web/internal/web/quiet_test.go b/modules/mesh-mail-web/internal/web/quiet_test.go index 9d7b97d6..b3264bae 100644 --- a/modules/mesh-mail-web/internal/web/quiet_test.go +++ b/modules/mesh-mail-web/internal/web/quiet_test.go @@ -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 } diff --git a/modules/mesh-mail-web/internal/web/readpane.go b/modules/mesh-mail-web/internal/web/readpane.go index a0e121a7..316c196d 100644 --- a/modules/mesh-mail-web/internal/web/readpane.go +++ b/modules/mesh-mail-web/internal/web/readpane.go @@ -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 diff --git a/modules/mesh-mail-web/internal/web/readpane_test.go b/modules/mesh-mail-web/internal/web/readpane_test.go index f03d02be..c7814c37 100644 --- a/modules/mesh-mail-web/internal/web/readpane_test.go +++ b/modules/mesh-mail-web/internal/web/readpane_test.go @@ -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"} diff --git a/modules/mesh-mail-web/internal/web/static.go b/modules/mesh-mail-web/internal/web/static.go index 650a0f50..3a91f459 100644 --- a/modules/mesh-mail-web/internal/web/static.go +++ b/modules/mesh-mail-web/internal/web/static.go @@ -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"