amadan

carlos / todomvc-bakeoff Public

fix(go): replace unissued cookies; keep in-progress edits across list redraws

Review findings: a well-formed cookie the server never issued was taken
as a list identity, and a fragment arriving mid-edit discarded the draft.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude df2a5c62732b6b0c6833da86b59a48a6d5dd37c0 parent 339c05f
6 files changed, +168 −16
  • apps/go/README.md +3 −2
  • apps/go/internal/todo/store.go +21 −0
  • apps/go/internal/todo/store_test.go +40 −1
  • apps/go/web/static/app.js +31 −3
  • apps/go/web/web.go +31 −10
  • apps/go/web/web_test.go +42 −0
diff --git a/apps/go/README.md b/apps/go/README.md
index 4e962cd..57ceba0 100644
--- a/apps/go/README.md
+++ b/apps/go/README.md
@@ -44,8 +44,9 @@ rest of `go.sum` is what it pulls in.
A visitor's first request sets a `todo_list` cookie holding 32 random
bytes (`HttpOnly; SameSite=Lax; Path=/`, plus `Secure` on Carloku). The
database keys each todo by the SHA-256 of that cookie, never the cookie
-itself. A cookie the app can't parse just gets a new one and an empty
-list. An id that isn't in your list answers 404, whoever it belongs to.
+itself. Issued lists are recorded in a `todo_lists` table, and a cookie
+the app can't parse or never issued just gets a new one and an empty
+list, so nobody can choose their own list identity. An id that isn't in your list answers 404, whoever it belongs to.
## Honest trade-offs
diff --git a/apps/go/internal/todo/store.go b/apps/go/internal/todo/store.go
index 780fb43..da857f7 100644
--- a/apps/go/internal/todo/store.go
+++ b/apps/go/internal/todo/store.go
@@ -48,6 +48,14 @@ var migrations = []string{
created INTEGER NOT NULL DEFAULT (unixepoch())
);
CREATE INDEX todo_items_by_list ON todo_items (list, id);`,
+ // Lists the server has issued. A cookie whose hash isn't here is
+ // replaced, so nobody can pick their own list identity. Lists that
+ // already hold todos were issued by an earlier build, so they count.
+ `CREATE TABLE todo_lists (
+ key BLOB PRIMARY KEY,
+ created INTEGER NOT NULL DEFAULT (unixepoch())
+ ) WITHOUT ROWID;
+ INSERT INTO todo_lists (key) SELECT DISTINCT list FROM todo_items;`,
}
// Store is the todo database: one connection for writes, a small pool
@@ -125,6 +133,19 @@ func (s *Store) Close() error {
// Ping checks that the database answers.
func (s *Store) Ping(ctx context.Context) error { return s.r.PingContext(ctx) }
+// CreateList records a newly issued list.
+func (s *Store) CreateList(ctx context.Context, k ListKey) error {
+ _, err := s.w.ExecContext(ctx, `INSERT INTO todo_lists (key) VALUES (?)`, k[:])
+ return err
+}
+
+// ListExists reports whether the server issued this list.
+func (s *Store) ListExists(ctx context.Context, k ListKey) (bool, error) {
+ var n int
+ err := s.r.QueryRowContext(ctx, `SELECT count(*) FROM todo_lists WHERE key = ?`, k[:]).Scan(&n)
+ return n > 0, err
+}
+
// List returns every todo in the list, oldest first.
func (s *Store) List(ctx context.Context, k ListKey) ([]Todo, error) {
rows, err := s.r.QueryContext(ctx,
diff --git a/apps/go/internal/todo/store_test.go b/apps/go/internal/todo/store_test.go
index 3bbe1fb..fa20643 100644
--- a/apps/go/internal/todo/store_test.go
+++ b/apps/go/internal/todo/store_test.go
@@ -160,7 +160,7 @@ func TestOpensBesideForeignTables(t *testing.T) {
t.Fatal(err)
}
for _, q := range []string{
- `DROP TABLE todo_items`, `DROP TABLE todo_migrations`,
+ `DROP TABLE todo_items`, `DROP TABLE todo_lists`, `DROP TABLE todo_migrations`,
`CREATE TABLE todos (id INTEGER PRIMARY KEY, text TEXT)`,
`INSERT INTO todos (text) VALUES ('old')`,
`PRAGMA user_version = 7`,
@@ -187,3 +187,42 @@ func TestOpensBesideForeignTables(t *testing.T) {
t.Fatalf("earlier data lost: %q", old)
}
}
+
+func TestOnlyIssuedListsExist(t *testing.T) {
+ s, ctx := open(t), context.Background()
+ k := KeyFor([]byte("issued"))
+ if ok, _ := s.ListExists(ctx, k); ok {
+ t.Fatal("unissued list exists")
+ }
+ if err := s.CreateList(ctx, k); err != nil {
+ t.Fatal(err)
+ }
+ if ok, _ := s.ListExists(ctx, k); !ok {
+ t.Fatal("issued list missing")
+ }
+}
+
+// Lists that already held todos before the lists table existed keep working.
+func TestListsMigrationKeepsExistingLists(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "todo.db")
+ ctx, k := context.Background(), KeyFor([]byte("old visitor"))
+ s, err := Open(ctx, path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ s.Add(ctx, k, "kept")
+ for _, q := range []string{`DROP TABLE todo_lists`, `DELETE FROM todo_migrations WHERE version = 2`} {
+ if _, err := s.w.Exec(q); err != nil {
+ t.Fatal(err)
+ }
+ }
+ s.Close()
+ s, err = Open(ctx, path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer s.Close()
+ if ok, _ := s.ListExists(ctx, k); !ok {
+ t.Fatal("existing list not carried into todo_lists")
+ }
+}
diff --git a/apps/go/web/static/app.js b/apps/go/web/static/app.js
index 7ad8bd6..26e8590 100644
--- a/apps/go/web/static/app.js
+++ b/apps/go/web/static/app.js
@@ -12,6 +12,7 @@ const newTodo = app.querySelector('.new-todo');
// Requests run one at a time, so their answers land in the order they
// were made.
let queue = Promise.resolve();
+let swapping = false;
function send(form) {
const body = new URLSearchParams(new FormData(form));
@@ -35,14 +36,41 @@ function send(form) {
}
// Replaces section.main and footer.footer with the server's HTML, keeping
-// keyboard focus on the same control where it still exists.
+// keyboard focus on the same control where it still exists, and keeping
+// any edit still in progress (one not yet being saved) open with its
+// draft intact.
function swap(html) {
const again = focusSelector(document.activeElement);
+ const drafts = [...app.querySelectorAll('.todo-list li.editing:not([data-saving])')].map((li) => {
+ const input = li.querySelector('.edit');
+ return {
+ id: li.dataset.id,
+ value: input.value,
+ focused: input === document.activeElement,
+ start: input.selectionStart,
+ end: input.selectionEnd,
+ };
+ });
+ // Removing a focused field can fire focusout; that isn't the visitor
+ // leaving the field, so it mustn't save.
+ swapping = true;
for (const el of app.querySelectorAll(':scope > .main, :scope > .footer')) el.remove();
+ swapping = false;
const t = document.createElement('template');
t.innerHTML = html;
app.append(t.content);
- if (again) app.querySelector(again)?.focus();
+ for (const d of drafts) {
+ const li = app.querySelector(`.todo-list li[data-id="${CSS.escape(d.id)}"]`);
+ if (!li) continue; // the item is gone, so is its edit
+ const input = li.querySelector('.edit');
+ li.classList.add('editing');
+ input.value = d.value;
+ if (d.focused) {
+ input.focus();
+ input.setSelectionRange(d.start, d.end);
+ }
+ }
+ if (again && !drafts.some((d) => d.focused)) app.querySelector(again)?.focus();
}
function focusSelector(el) {
@@ -95,7 +123,7 @@ app.addEventListener('keydown', (e) => {
});
app.addEventListener('focusout', (e) => {
- if (e.target.matches('.edit')) saveEdit(e.target);
+ if (!swapping && e.target.matches('.edit')) saveEdit(e.target);
});
// Saves an edit once, however many ways it was finished (Enter, then the
diff --git a/apps/go/web/web.go b/apps/go/web/web.go
index 6786935..43b4a51 100644
--- a/apps/go/web/web.go
+++ b/apps/go/web/web.go
@@ -161,7 +161,11 @@ func (s *Server) view(ctx context.Context, k todo.ListKey, filter string) (view,
func (s *Server) page(filter string) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
- k := s.listKey(w, r, true)
+ k, err := s.listKey(w, r, true)
+ if err != nil {
+ s.fail(w, r, err)
+ return
+ }
v, err := s.view(r.Context(), k, filter)
if err != nil {
s.fail(w, r, err)
@@ -206,7 +210,11 @@ func (s *Server) action(do func(r *http.Request, k todo.ListKey) error) http.Han
http.Error(w, http.StatusText(status), status)
return
}
- k := s.listKey(w, r, false)
+ k, err := s.listKey(w, r, false)
+ if err != nil {
+ s.fail(w, r, err)
+ return
+ }
if err := do(r, k); err != nil {
if errors.Is(err, todo.ErrNotFound) {
http.NotFound(w, r)
@@ -259,23 +267,36 @@ func (s *Server) sameOrigin(r *http.Request) bool {
return origin == scheme+"://"+r.Host
}
-// listKey reads the visitor's list cookie. A missing or unrecognisable
-// cookie gets a fresh random one, which is a fresh empty list. When renew
-// is set, a valid cookie is sent again to push its expiry forward.
-func (s *Server) listKey(w http.ResponseWriter, r *http.Request, renew bool) todo.ListKey {
+// listKey reads the visitor's list cookie. A missing cookie, or one the
+// server never issued, gets a fresh random one, which is a fresh empty
+// list. When renew is set, a valid cookie is sent again to push its
+// expiry forward.
+func (s *Server) listKey(w http.ResponseWriter, r *http.Request, renew bool) (todo.ListKey, error) {
+ ctx := r.Context()
if c, err := r.Cookie(cookieName); err == nil {
secret, err := base64.RawURLEncoding.DecodeString(c.Value)
if err == nil && len(secret) == secretBytes {
- if renew {
- s.setCookie(w, c.Value)
+ k := todo.KeyFor(secret)
+ ok, err := s.store.ListExists(ctx, k)
+ if err != nil {
+ return k, err
+ }
+ if ok {
+ if renew {
+ s.setCookie(w, c.Value)
+ }
+ return k, nil
}
- return todo.KeyFor(secret)
}
}
secret := make([]byte, secretBytes)
rand.Read(secret) // never fails (crypto/rand panics rather than return an error)
+ k := todo.KeyFor(secret)
+ if err := s.store.CreateList(ctx, k); err != nil {
+ return k, err
+ }
s.setCookie(w, base64.RawURLEncoding.EncodeToString(secret))
- return todo.KeyFor(secret)
+ return k, nil
}
func (s *Server) setCookie(w http.ResponseWriter, value string) {
diff --git a/apps/go/web/web_test.go b/apps/go/web/web_test.go
index f13b2d0..2b42e55 100644
--- a/apps/go/web/web_test.go
+++ b/apps/go/web/web_test.go
@@ -265,3 +265,45 @@ func TestScriptStaysSmall(t *testing.T) {
t.Fatalf("app.js is %d lines; the cap is 300", n)
}
}
+
+// A well-formed cookie the server never issued is replaced, so two
+// visitors can't share a list by choosing the same value.
+func TestUnissuedCookieIsReplaced(t *testing.T) {
+ srv := newServer(t)
+ forged := strings.Repeat("A", 43) // decodes to 32 bytes
+ post := func() *http.Response {
+ req, _ := http.NewRequest("POST", srv.URL+"/todos", strings.NewReader("title=shared"))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ req.Header.Set("Cookie", cookieName+"="+forged)
+ res, err := http.DefaultTransport.RoundTrip(req)
+ if err != nil {
+ t.Fatal(err)
+ }
+ res.Body.Close()
+ return res
+ }
+ for range 2 {
+ res := post()
+ var got string
+ for _, c := range res.Cookies() {
+ if c.Name == cookieName {
+ got = c.Value
+ }
+ }
+ if got == "" || got == forged {
+ t.Fatalf("forged cookie kept (Set-Cookie %q)", res.Header.Get("Set-Cookie"))
+ }
+ }
+ req, _ := http.NewRequest("GET", srv.URL+"/", nil)
+ req.Header.Set("Cookie", cookieName+"="+forged)
+ res, err := http.DefaultClient.Do(req)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var b bytes.Buffer
+ b.ReadFrom(res.Body)
+ res.Body.Close()
+ if len(ids(b.String())) != 0 {
+ t.Fatal("forged cookie sees a shared list")
+ }
+}