amadan

carlos / todomvc-bakeoff Public

fix(go-rastrillo): review round: strict Origin check, refuse unread forms, keep typing during adds, loopback only

- origin.go compares Origin with the app's origin directly, so
  Sec-Fetch-Site can no longer vouch for a foreign Origin.
- /todos posts are parsed before any handler; an oversized body is 413
  and a malformed one 400, so an unread title can't delete an item.
- app.js clears new-todo when the add is sent, and keeps anything typed
  while it's in flight.
- main.go refuses a TCP listen address not on 127.0.0.1.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude 0ab6a9fcfc9bf4b8f81e94b9995f4fc45531c2e1 parent f06ab5d
7 files changed, +124 −37
  • apps/go-rastrillo/README.md +13 −7
  • apps/go-rastrillo/cmd/todo/main.go +21 −0
  • apps/go-rastrillo/cmd/todo/main_test.go +17 −0
  • apps/go-rastrillo/internal/todo/app.go +17 −4
  • apps/go-rastrillo/internal/todo/origin.go +15 −19
  • apps/go-rastrillo/internal/todo/static/app.js +14 −7
  • apps/go-rastrillo/internal/todotest/todo_test.go +27 −0
diff --git a/apps/go-rastrillo/README.md b/apps/go-rastrillo/README.md
index 252d675..48a1b82 100644
--- a/apps/go-rastrillo/README.md
+++ b/apps/go-rastrillo/README.md
@@ -40,14 +40,20 @@ that browser gets a fresh, empty list. The cookie is
`__Host-` prefix. Lists live 400 days from the first visit instead of
sessions' default of 30.
-### Cross-site protection: the Origin check, not `csrf.Protect` alone
+### Cross-site protection: the contract's Origin check, not `csrf.Protect`
Rastrillo's `csrf.Protect` refuses any POST that carries none of
`Sec-Fetch-Site`, `Origin` or `Referer`. The bakeoff's shared tests and
-load tests post exactly like that, and the contract allows it, so
-`origin.go` wraps it: a POST with no `Origin` header goes straight
-through, and every POST that has one goes through `csrf.Protect`, so a
-foreign origin gets 403. There's no CSRF token, as the contract says.
+load tests post exactly like that, and the contract allows it. Protect
+also trusts `Sec-Fetch-Site` ahead of `Origin`, so it would let through a
+request claiming `same-origin` that carries a foreign `Origin`. So
+`origin.go` does the contract's check instead: a POST whose `Origin`
+header is not exactly this app's origin gets 403, and a POST with no
+`Origin` header is allowed. There's no CSRF token, as the contract says.
+
+Form posts are capped at 1 MiB and parsed before any handler runs. A
+body that's too big gets 413 and a malformed one gets 400, and neither
+changes anything.
### JavaScript on and off
@@ -75,8 +81,8 @@ make test # vet, gofmt, go test with Rastrillo's time budgets, migrati
make perf # every GET screen under 150ms to first byte, cold boot under 500ms
```
-Run it locally (it only ever binds what you give it, and defaults to
-`127.0.0.1:8080`):
+Run it locally. It refuses any TCP address not on `127.0.0.1`, and
+defaults to `127.0.0.1:8080`:
```sh
dist/app --listen 127.0.0.1:8080 --db "$TMPDIR/go-rastrillo.db"
diff --git a/apps/go-rastrillo/cmd/todo/main.go b/apps/go-rastrillo/cmd/todo/main.go
index b74e291..bf9b165 100644
--- a/apps/go-rastrillo/cmd/todo/main.go
+++ b/apps/go-rastrillo/cmd/todo/main.go
@@ -6,6 +6,7 @@
package main
import (
+ "fmt"
"log/slog"
"net"
"os"
@@ -42,6 +43,12 @@ func main() {
if opts.Socket == "" && opts.Addr == "" && os.Getenv("LISTEN_FDS") == "" {
opts.Addr = localAddr
}
+ if opts.Addr != "" {
+ if err := loopbackOnly(opts.Addr); err != nil {
+ logger.Error("listen address", "err", err)
+ os.Exit(1)
+ }
+ }
d, err := db.Open(opts.DBPath, logger)
if err != nil {
@@ -66,6 +73,20 @@ func main() {
}
}
+// loopbackOnly refuses a TCP address that isn't on 127.0.0.1: the app
+// never listens on another interface ("0.0.0.0:8080" or ":8080" would
+// expose it to the whole network).
+func loopbackOnly(addr string) error {
+ host, _, err := net.SplitHostPort(addr)
+ if err != nil {
+ return fmt.Errorf("%q: %w", addr, err)
+ }
+ if host != "127.0.0.1" {
+ return fmt.Errorf("%q: only 127.0.0.1 may be listened on", addr)
+ }
+ return nil
+}
+
// origin is the app's own origin: https://$CARLOS_HOST on Carloku, and
// http://<listen address> when run locally.
func origin(addr string) string {
diff --git a/apps/go-rastrillo/cmd/todo/main_test.go b/apps/go-rastrillo/cmd/todo/main_test.go
index e5e7a15..aacebac 100644
--- a/apps/go-rastrillo/cmd/todo/main_test.go
+++ b/apps/go-rastrillo/cmd/todo/main_test.go
@@ -29,3 +29,20 @@ func TestOrigin(t *testing.T) {
t.Errorf("carloku origin %q", got)
}
}
+
+func TestLoopbackOnly(t *testing.T) {
+ t.Parallel()
+ for addr, ok := range map[string]bool{
+ "127.0.0.1:8080": true,
+ "0.0.0.0:8080": false,
+ ":8080": false,
+ "[::]:8080": false,
+ "localhost:8080": false,
+ "10.0.0.5:8080": false,
+ "127.0.0.1": false,
+ } {
+ if err := loopbackOnly(addr); (err == nil) != ok {
+ t.Errorf("loopbackOnly(%q) = %v, want ok=%v", addr, err, ok)
+ }
+ }
+}
diff --git a/apps/go-rastrillo/internal/todo/app.go b/apps/go-rastrillo/internal/todo/app.go
index 29b8e8d..cd1e244 100644
--- a/apps/go-rastrillo/internal/todo/app.go
+++ b/apps/go-rastrillo/internal/todo/app.go
@@ -2,6 +2,7 @@ package todo
import (
"context"
+ "errors"
"log/slog"
"net/http"
"time"
@@ -60,7 +61,7 @@ func Router(d *db.DB, origin string, logger *slog.Logger) (chi.Router, error) {
r.Get("/active", a.page(filterActive))
r.Get("/completed", a.page(filterCompleted))
r.Route("/todos", func(r chi.Router) {
- r.Use(limitBody)
+ r.Use(parseForm)
r.Post("/", a.create)
r.Post("/toggle-all", a.toggleAll)
r.Post("/clear-completed", a.clearCompleted)
@@ -93,12 +94,24 @@ func Configure(opts *rastrillo.Options, mux *http.ServeMux, started time.Time) {
opts.Wrap = perf.Middleware(&rec, perf.Options{Started: started, Logger: opts.Logger})
}
-// limitBody caps a form post at 1 MiB: room for a title far past the
+// parseForm caps a form post at 1 MiB (room for a title far past the
// contract's 10,000 characters, and no room for a body meant to fill
-// the disk.
-func limitBody(next http.Handler) http.Handler {
+// the disk) and parses it before any handler runs, as form.Parse's own
+// docs ask. A body that is too big or malformed is refused here, so a
+// handler never mistakes an unread field for an empty one and changes
+// data on it: an over-long edit must not read as "delete".
+func parseForm(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
r.Body = http.MaxBytesReader(w, r.Body, 1<<20)
+ if err := r.ParseForm(); err != nil {
+ var tooBig *http.MaxBytesError
+ if errors.As(err, &tooBig) {
+ errorPage(w, http.StatusRequestEntityTooLarge, "That is too long to save")
+ return
+ }
+ errorPage(w, http.StatusBadRequest, "That form could not be read")
+ return
+ }
next.ServeHTTP(w, r)
})
}
diff --git a/apps/go-rastrillo/internal/todo/origin.go b/apps/go-rastrillo/internal/todo/origin.go
index 8c9abea..9a525d9 100644
--- a/apps/go-rastrillo/internal/todo/origin.go
+++ b/apps/go-rastrillo/internal/todo/origin.go
@@ -1,31 +1,27 @@
package todo
-import (
- "net/http"
-
- "amadan.net/rastrillo/rastrillo/csrf"
-)
+import "net/http"
// originCheck is the contract's cross-site protection: a POST carrying
-// an Origin header that isn't this app's own answers 403, and a POST
-// with no Origin header is allowed.
+// an Origin header that isn't exactly this app's own answers 403, and a
+// POST with no Origin header is allowed.
//
-// It is csrf.Protect with one exception. Protect refuses a POST that
-// carries none of Sec-Fetch-Site, Origin and Referer, because no current
-// browser sends a form that way. The bakeoff's shared tests and load
-// tests do post that way, and the contract allows it, so those requests
-// pass. Everything that does carry an Origin goes through Rastrillo's
-// own check (csrf.SameOrigin), Sec-Fetch-Site first.
+// It is deliberately not csrf.Protect. Protect refuses a POST that
+// carries none of Sec-Fetch-Site, Origin and Referer, which the
+// bakeoff's shared tests and load tests send and the contract allows;
+// and it trusts Sec-Fetch-Site ahead of Origin, so a request claiming
+// "same-origin" would pass with a foreign Origin. Here the Origin
+// header alone decides.
func originCheck(origin string) func(http.Handler) http.Handler {
- protect := csrf.Protect(origin)
return func(next http.Handler) http.Handler {
- protected := protect(next)
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- if r.Header.Get("Origin") == "" {
- next.ServeHTTP(w, r)
- return
+ if r.Method != http.MethodGet && r.Method != http.MethodHead && r.Method != http.MethodOptions {
+ if o := r.Header.Get("Origin"); o != "" && o != origin {
+ http.Error(w, "cross-origin form submission refused", http.StatusForbidden)
+ return
+ }
}
- protected.ServeHTTP(w, r)
+ next.ServeHTTP(w, r)
})
}
}
diff --git a/apps/go-rastrillo/internal/todo/static/app.js b/apps/go-rastrillo/internal/todo/static/app.js
index 912db0a..824f41b 100644
--- a/apps/go-rastrillo/internal/todo/static/app.js
+++ b/apps/go-rastrillo/internal/todo/static/app.js
@@ -12,7 +12,10 @@ let queue = Promise.resolve();
function send(form) {
const body = new URLSearchParams(new FormData(form));
- const addsTodo = form.querySelector('.new-todo') !== null;
+ const newTodo = form.querySelector('.new-todo');
+ // TodoMVC clears the field once a todo is added. Clear it now, as the
+ // post goes, so anything typed while it is in flight is kept.
+ if (newTodo) newTodo.value = '';
queue = queue
.then(() => fetch(form.action, {
method: 'POST',
@@ -24,21 +27,25 @@ function send(form) {
if (!res.ok) throw new Error(`${res.status} from ${form.action}`);
return res.text();
})
- .then((html) => swap(html, addsTodo))
+ .then((html) => swap(html, newTodo !== null))
.catch(() => location.reload());
}
-// swap replaces the list with the server's markup, keeping whatever the
-// visitor has half-typed into the new-todo field (unless that field was
-// what just posted, in which case it's cleared, as TodoMVC does).
+// swap replaces the list with the server's markup, keeping whatever is
+// in the new-todo field and its focus. After an add the field keeps
+// focus, as TodoMVC does.
function swap(html, addsTodo) {
const input = app.querySelector('.new-todo');
- const typing = !addsTodo && input.value;
+ const typing = input.value;
const focused = addsTodo || document.activeElement === input;
+ const [start, end] = [input.selectionStart, input.selectionEnd];
app.innerHTML = html;
const next = app.querySelector('.new-todo');
if (typing) next.value = typing;
- if (focused) next.focus();
+ if (focused) {
+ next.focus();
+ if (typing) next.setSelectionRange(start, end);
+ }
}
app.addEventListener('submit', (e) => {
diff --git a/apps/go-rastrillo/internal/todotest/todo_test.go b/apps/go-rastrillo/internal/todotest/todo_test.go
index b82f737..323847a 100644
--- a/apps/go-rastrillo/internal/todotest/todo_test.go
+++ b/apps/go-rastrillo/internal/todotest/todo_test.go
@@ -240,6 +240,15 @@ func TestOriginCheck(t *testing.T) {
if got := titles(b.get("/").Body.String()); len(got) != 0 {
t.Fatalf("refused posts added %q", got)
}
+ // Sec-Fetch-Site can't vouch for a foreign Origin.
+ for _, site := range []string{"same-origin", "none"} {
+ if rec := b.post("/todos", form, "Origin", "https://evil.example", "Sec-Fetch-Site", site); rec.Code != http.StatusForbidden {
+ t.Errorf("foreign Origin with Sec-Fetch-Site %s: %d, want 403", site, rec.Code)
+ }
+ }
+ if got := titles(b.get("/").Body.String()); len(got) != 0 {
+ t.Fatalf("refused posts added %q", got)
+ }
if rec := b.post("/todos", form, "Origin", testOrigin); rec.Code != http.StatusSeeOther {
t.Errorf("own Origin: %d, want 303", rec.Code)
}
@@ -278,3 +287,21 @@ func TestStylesheetsServedByteForByte(t *testing.T) {
}
}
}
+
+func TestOversizedEditChangesNothing(t *testing.T) {
+ t.Parallel()
+ b := newBrowser(t, newApp(t))
+ b.add("keep me")
+ id := items(b.get("/").Body.String())[0].ID
+ huge := strings.Repeat("x", 2<<20)
+ rec := b.post("/todos/"+id+"/edit", url.Values{"title": {huge}, "filter": {"all"}})
+ if rec.Code != http.StatusRequestEntityTooLarge {
+ t.Errorf("oversized edit: %d, want 413", rec.Code)
+ }
+ if got := titles(b.get("/").Body.String()); !slices.Equal(got, []string{"keep me"}) {
+ t.Errorf("after oversized edit: %q", got)
+ }
+ if rec := b.post("/todos", url.Values{"title": {huge}}); rec.Code != http.StatusRequestEntityTooLarge {
+ t.Errorf("oversized add: %d, want 413", rec.Code)
+ }
+}