amadan
Review fixes (#7): pin estilo main a264aa5; HOME_DIRECTORY_ORIGIN judged by host and port
Rebased onto e93afe3 (rastrillo at amadan.net/rastrillo/rastrillo), so account_test.go imports the new db path. estilo main a264aa5 carries feat/menu-dropup unchanged (fd9e235 is its ancestor; no chrome or menu difference). Astra's P3: directoryOrigin judged loopback by a string prefix, so it refused http://127.0.0.1, http://localhost and http://[::1] with no port, and accepted the host-less https://:443 (and an empty or out-of-range port). It now reads the parsed hostname and port apart: a non-empty host, a numeric port in 1-65535 if any, https anywhere, http only on localhost or a loopback IP. The new cases failed first (polish-audit/r3/scripts/r2-fichas-dropup-p3-red.txt). Appearance keeps the form's return_to, as Docs, Sheets, Calendar, Memoria and Slides do (spec §8.3): estilo's handler checks it with rastrillo's SafeReturn. The test now pins that off-site, scheme, backslash and control-character targets land on "/".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+72
−7
internal/fichas/config.go+45 −6internal/fichas/config_test.go+18 −1internal/fichastest/account_test.go+9 −0
CI passed — run details
CI log
=== vet === go vet ./... go vet -tags browser ./internal/fichastest/ === fmt === === test === go test ./... ? amadan.net/oficina/fichas/cmd/fichas [no test files] ok amadan.net/oficina/fichas/internal/fichas 0.632s ? amadan.net/oficina/fichas/internal/fichas/icons [no test files] ok amadan.net/oficina/fichas/internal/fichas/seal (cached) ok amadan.net/oficina/fichas/internal/fichastest 0.405s ? amadan.net/oficina/fichas/scripts/mockups [no test files] ? amadan.net/oficina/fichas/scripts/preview [no test files] ? amadan.net/oficina/fichas/scripts/writeestilo [no test files] === migration-check === go tool rastrillo migration check rastrillo migration check: models and migrations agree
diff --git a/internal/fichas/config.go b/internal/fichas/config.go| index aaebf7a..f5f198d 100644 |
| --- a/internal/fichas/config.go |
| +++ b/internal/fichas/config.go |
| @@ -4,7 +4,9 @@ import ( |
| "encoding/base64" |
| "errors" |
| "fmt" |
| + "net" |
| "net/url" |
| + "strconv" |
| "strings" |
| "amadan.net/oficina/fichas/internal/fichas/seal" |
| @@ -182,18 +184,55 @@ func isLocal(origin string) bool { |
| func (c Config) UsedDefaultOrigin() bool { return c.Origin == defaultOrigin } |
| // directoryOrigin validates HOME_DIRECTORY_ORIGIN: empty, or an origin |
| -// and nothing else — https, or http on loopback for local development — |
| -// returned without a trailing slash. It becomes a link in every page, so |
| -// anything else is refused at boot rather than drawn. |
| +// and nothing else — https, or http on a loopback host for local |
| +// development — returned without a trailing slash. It becomes a link in |
| +// every page, so anything else is refused at boot rather than drawn. |
| +// |
| +// The host and the port are judged apart, on the parsed URL: a prefix |
| +// test on the string refused loopback with no port and let a host-less |
| +// "https://:443" through (Astra, fichas #7). |
| func directoryOrigin(v string) (string, error) { |
| v = strings.TrimSuffix(strings.TrimSpace(v), "/") |
| if v == "" { |
| return "", nil |
| } |
| + refuse := fmt.Errorf("fichas: HOME_DIRECTORY_ORIGIN is %q; it is the hub directory's origin, https://host", v) |
| u, err := url.Parse(v) |
| - if err != nil || u.Host == "" || u.User != nil || u.Path != "" || u.RawQuery != "" || u.Fragment != "" || |
| - !(u.Scheme == "https" || (u.Scheme == "http" && isLocal(v+"/"))) { |
| - return "", fmt.Errorf("fichas: HOME_DIRECTORY_ORIGIN is %q; it is the hub directory's origin, https://host", v) |
| + if err != nil || u.Opaque != "" || u.User != nil || u.Path != "" || u.RawQuery != "" || u.Fragment != "" || u.ForceQuery { |
| + return "", refuse |
| + } |
| + host := u.Hostname() |
| + if host == "" { |
| + return "", refuse |
| + } |
| + // A port, if the URL has a colon after the host, is 1-65535 in digits. |
| + if strings.HasSuffix(u.Host, ":") { |
| + return "", refuse |
| + } |
| + if p := u.Port(); p != "" { |
| + n, err := strconv.Atoi(p) |
| + if err != nil || n < 1 || n > 65535 { |
| + return "", refuse |
| + } |
| + } |
| + switch u.Scheme { |
| + case "https": |
| + case "http": |
| + if !loopbackHost(host) { |
| + return "", refuse |
| + } |
| + default: |
| + return "", refuse |
| } |
| return u.Scheme + "://" + u.Host, nil |
| } |
| + |
| +// loopbackHost is a loopback hostname, exactly: localhost, or a loopback |
| +// IP address. A name that merely starts like one is not. |
| +func loopbackHost(host string) bool { |
| + if host == "localhost" { |
| + return true |
| + } |
| + ip := net.ParseIP(host) |
| + return ip != nil && ip.IsLoopback() |
| +} |
diff --git a/internal/fichas/config_test.go b/internal/fichas/config_test.go| index 9bb365b..2f2e1ec 100644 |
| --- a/internal/fichas/config_test.go |
| +++ b/internal/fichas/config_test.go |
| @@ -210,7 +210,24 @@ func TestLoadConfigTakesTheDirectoryOrigin(t *testing.T) { |
| if err != nil || c.Directory != "https://oficina.works" { |
| t.Fatalf("Directory %q, err %v; want https://oficina.works", c.Directory, err) |
| } |
| - for _, bad := range []string{"oficina.works", "http://oficina.works", "https://oficina.works/apps", "https://oficina.works?x=1", "javascript:alert(1)", "https://"} { |
| + // Loopback http is allowed for local development, with or without a |
| + // port, and the hostname is judged on its own, not by a prefix. |
| + for in, want := range map[string]string{ |
| + "http://127.0.0.1": "http://127.0.0.1", |
| + "http://localhost/": "http://localhost", |
| + "http://[::1]": "http://[::1]", |
| + "http://127.0.0.1:8746": "http://127.0.0.1:8746", |
| + "https://oficina.works:8443": "https://oficina.works:8443", |
| + } { |
| + if c, err := LoadConfig(env(map[string]string{"HOME_DIRECTORY_ORIGIN": in})); err != nil || c.Directory != want { |
| + t.Errorf("HOME_DIRECTORY_ORIGIN=%q: Directory %q, err %v; want %q", in, c.Directory, err, want) |
| + } |
| + } |
| + for _, bad := range []string{"oficina.works", "http://oficina.works", "https://oficina.works/apps", "https://oficina.works?x=1", "javascript:alert(1)", "https://", |
| + // A port with no host, and a port that is not one. |
| + "https://:443", "http://:8080", "https://oficina.works:", "https://oficina.works:x", "https://oficina.works:99999", |
| + // Loopback-looking names that are not loopback. |
| + "http://127.0.0.1.evil.example", "http://localhost.evil.example:80", "http://[::1].evil"} { |
| if _, err := LoadConfig(env(map[string]string{"HOME_DIRECTORY_ORIGIN": bad})); err == nil { |
| t.Errorf("HOME_DIRECTORY_ORIGIN=%q accepted; want a refusal", bad) |
| } |
diff --git a/internal/fichastest/account_test.go b/internal/fichastest/account_test.go| index 833aab7..afbad21 100644 |
| --- a/internal/fichastest/account_test.go |
| +++ b/internal/fichastest/account_test.go |
| @@ -106,6 +106,15 @@ func TestAppearanceIsChosenInTheAccountMenu(t *testing.T) { |
| if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/cards/new" { |
| t.Fatalf("POST /appearance: %d to %q; want 303 back to /cards/new", rec.Code, rec.Header().Get("Location")) |
| } |
| + // return_to is the page the menu was opened on, carried in the form as |
| + // in every app (spec §8.3), and only ever a path on this origin: |
| + // anything else lands on the list. |
| + for _, off := range []string{"https://evil.example/", "//evil.example/", "/\\evil.example", "/\t/evil.example", "javascript:alert(1)", ""} { |
| + r := post(t, h, "/appearance", url.Values{"mode": {"dark"}, "return_to": {off}}) |
| + if loc := r.Header().Get("Location"); r.Code != http.StatusSeeOther || loc != "/" { |
| + t.Errorf("return_to %q: %d to %q; want 303 to /", off, r.Code, loc) |
| + } |
| + } |
| cookies := rec.Result().Cookies() |
| if len(cookies) != 1 || cookies[0].Value != "dark" { |
| t.Fatalf("POST /appearance set %v; want the dark preference", cookies) |