oficina / home Public

Keep the way home on error pages; return Appearance only to real pages

Four nits from Fable's reviews of #80 and #81:

- A 500 for a GET replaced "Go to Oficina" with "Try again", which
  points at the URL that just failed, so the one link known to work
  was gone. "Try again" now sits beside it, in estilo-error's second
  slot.
- Appearance's return_to was the current path on every directory page,
  404s and POST-drawn pages included, so choosing a theme there 303'd
  to a page that is not there. It is now the path only for a GET that
  drew a 200, and "/" otherwise.
- errorCopy covered 404 and 500 only; any other status drew a blank
  heading and tab title. 400, 401, 403, 405, 413, 429 and 503 now have
  their own words (rastrillo's error copy where it has some), and any
  other status gets a general page.
- TestAppearanceSetsTheTheme only checked the happy path. It now posts
  hostile return_to values (//evil, /%09/evil, /%5c/evil, https://evil
  and friends) and wants "/" for each. They already passed; weakening
  the validator estilo actually calls (sessions.SafeReturn on the
  github.com/carlosframework/rastrillo path) to a bare "/" prefix check
  makes five of them fail, so the test does guard it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude pushed by paul@keymail.dev 2 weeks ago c5ed5ea2dcf3164c7c699ebe803985bf441154f7 parent 83c5d16

3 files changed, +218 −14

internal/home/chrome.go +51 −13
internal/home/render.go +1 −1
internal/home/shared_chrome_test.go +166 −0

diff --git a/internal/home/chrome.go b/internal/home/chrome.go

index beb34b0..9915db0 100644

--- a/internal/home/chrome.go

+++ b/internal/home/chrome.go

@@ -58,7 +58,7 @@ func (a *App) onCredential(r *http.Request) bool {

// sign-in, sent and signed-out pages, whose corner stays empty. hop marks

// a page that moves on by itself: its bar is the brand alone, as plain

// text, so the page still holds exactly one link, the way on.

-func (a *App) pageFor(r *http.Request, data any, door, hop bool) page {

+func (a *App) pageFor(r *http.Request, status int, data any, door, hop bool) page {

p := page{Data: data}

brand := chrome.App{Name: suiteName, Slug: "oficina", Home: "/", Icon: versioned("/static/brand/icon.svg")}

if hop {

@@ -74,7 +74,7 @@ func (a *App) pageFor(r *http.Request, data any, door, hop bool) page {

p.Theme = chrome.ThemeAttr(chrome.Appearance(r))

p.Chrome = chrome.Bar{App: brand, Labels: barLabels}

if _, ok := sessions.Current(r); ok {

- p.Chrome.Account = a.account(r)

+ p.Chrome.Account = a.account(r, status)

} else if !door {

p.Chrome.SignIn = &chrome.Link{Href: signInHref(r), Label: "Sign in"}

}

@@ -84,7 +84,7 @@ func (a *App) pageFor(r *http.Request, data any, door, hop bool) page {

// account is the directory's account menu. The directory cannot read the

// address (it holds a person ref, never an address: the one rule), so the

// identity row says only "Signed in" and the trigger is the person glyph.

-func (a *App) account(r *http.Request) *chrome.Account {

+func (a *App) account(r *http.Request, status int) *chrome.Account {

return &chrome.Account{

TriggerLabel: "Account",

Labels: accountLabels,

@@ -93,10 +93,8 @@ func (a *App) account(r *http.Request) *chrome.Account {

{Href: a.origins.Credential + "/passkey/codes", Label: "Recovery codes"},

Appearance: &chrome.AppearanceMenu{

- Action: "/appearance",

- // The path alone: no directory page reads its query, and a

- // query is where a crafted link would carry its text.

- Return: r.URL.EscapedPath(),

+ Action: "/appearance",

+ Return: appearanceReturn(r, status),

Options: chrome.AppearanceOptions(chrome.Appearance(r), "Light", "Dark", "System"),

Help: []menu.Item{{Href: "/help/open-source", Label: "Open source"}},

@@ -104,6 +102,20 @@ func (a *App) account(r *http.Request) *chrome.Account {

}

+// appearanceReturn is where choosing an appearance comes back to: this

+// page, if it is one a GET can come back to, otherwise the directory. Only

+// a GET that drew a 200 qualifies. A 404's or an error page's own path is

+// known not to work, and the path of a page drawn for a POST is POST-only,

+// so a 303 back to it would land on a 404. The path alone: no directory

+// page reads its query, and a query is where a crafted link would carry

+// its text.

+func appearanceReturn(r *http.Request, status int) string {

+ if r.Method != http.MethodGet || status != http.StatusOK {

+ return "/"

+ }

+ return r.URL.EscapedPath()

// signInHref starts the directory's sign-in and comes back to this page.

// An error page comes back to the directory instead: its own URL is the

// one thing known not to exist.

@@ -130,10 +142,34 @@ func wantsHTML(r *http.Request) bool {

return strings.Contains(r.Header.Get("Accept"), "text/html")

}

-// errorCopy is estilo spec §3.2's copy for the statuses Home answers.

-var errorCopy = map[int]struct{ page, heading, body string }{

- http.StatusNotFound: {"Page not found", "We can’t find that page", "The link may be out of date, or the page may have moved."},

- http.StatusInternalServerError: {"Something went wrong", "Something went wrong", "Try again in a moment. If it keeps happening, quote the reference below."},

+// errorWords is one status's error page: the tab title, the heading and

+// the line under it.

+type errorWords struct{ page, heading, body string }

+// errorCopy is estilo spec §3.2's copy for the statuses Home answers, and

+// rastrillo's error copy (locales/en.toml) for the rest an HTTP handler

+// can plausibly be asked to draw. Any other status gets errorFallback.

+var errorCopy = map[int]errorWords{

+ http.StatusBadRequest: {"Bad request", "That didn’t go through", "Something in what was sent wasn’t right. Go back and try again."},

+ http.StatusUnauthorized: {"Sign in needed", "You need to sign in", "This page is only for someone who is signed in. Sign in, then try again."},

+ http.StatusForbidden: {"No access", "You can’t see this", "Your account doesn’t have access here. If you think it should, ask whoever runs this site."},

+ http.StatusNotFound: {"Page not found", "We can’t find that page", "The link may be out of date, or the page may have moved."},

+ http.StatusMethodNotAllowed: {"Not available", "That can’t be done here", "This page doesn’t take that kind of request. Go back and try again from the page itself."},

+ http.StatusRequestEntityTooLarge: {"Too large", "That was too large", "What was sent is bigger than this page accepts. Try again with less."},

+ http.StatusTooManyRequests: {"Too many tries", "Too many tries", "Wait a minute, then try again."},

+ http.StatusInternalServerError: {"Something went wrong", "Something went wrong", "Try again in a moment. If it keeps happening, quote the reference below."},

+ http.StatusServiceUnavailable: {"Briefly unavailable", "We’re briefly unavailable", "Oficina is being updated or is busy. Try again in a minute."},

+// errorFallback is the error page for a status with no words of its own.

+var errorFallback = errorWords{"Something’s not right", "Something’s not right", "That request couldn’t be completed. Go back and try again."}

+// errorCopyFor is status's words, or the general ones.

+func errorCopyFor(status int) errorWords {

+ if c, ok := errorCopy[status]; ok {

+ return c

+ }

+ return errorFallback

}

// errorPage answers status as estilo's error page, under the bar, for a

@@ -145,7 +181,7 @@ func (a *App) errorPage(w http.ResponseWriter, r *http.Request, status int) {

http.Error(w, strings.ToLower(http.StatusText(status)), status)

return

}

- c := errorCopy[status]

+ c := errorCopyFor(status)

home := chrome.Link{Href: "/", Label: "Go to Oficina"}

if a.onCredential(r) {

home.Href = a.origins.Directory + "/"

@@ -155,7 +191,9 @@ func (a *App) errorPage(w http.ResponseWriter, r *http.Request, status int) {

data.Ref, data.RefLabel = rastrillo.NewRef(), "Reference:"

a.logger.Error("error page", "ref", data.Ref, "path", r.URL.Path)

if r.Method == http.MethodGet {

- data.Home = chrome.Link{Href: r.URL.RequestURI(), Label: "Try again"}

+ // Beside the way home, not instead of it: the way home is the

+ // one link known to work.

+ data.Back = &chrome.Link{Href: r.URL.RequestURI(), Label: "Try again"}

}

a.renderPage(w, r, status, "error.html", errorView{chrome.Title(c.page, suiteName), data})

diff --git a/internal/home/render.go b/internal/home/render.go

index f63ed7f..2ac21ab 100644

--- a/internal/home/render.go

+++ b/internal/home/render.go

@@ -108,7 +108,7 @@ func (a *App) renderPage(w http.ResponseWriter, r *http.Request, status int, nam

return

}

var buf bytes.Buffer

- if err := t.ExecuteTemplate(&buf, "layout", a.pageFor(r, data, doorPages[name], hopPages[name])); err != nil {

+ if err := t.ExecuteTemplate(&buf, "layout", a.pageFor(r, status, data, doorPages[name], hopPages[name])); err != nil {

a.logger.Error("render", "page", name, "err", err)

a.renderFailed(w, r, name)

return

diff --git a/internal/home/shared_chrome_test.go b/internal/home/shared_chrome_test.go

index 95feba1..8d9890a 100644

--- a/internal/home/shared_chrome_test.go

+++ b/internal/home/shared_chrome_test.go

@@ -168,6 +168,172 @@ func TestAppearanceSetsTheTheme(t *testing.T) {

if !strings.Contains(body, `value="dark" aria-pressed="true"`) {

t.Error("Dark is not shown as the current appearance")

}

+ // return_to is the attacker's half of the form: anything that would

+ // leave the directory goes to "/" instead. Raw form bodies, as a

+ // hostile page would post them: %09 and %5c decode to a tab and a

+ // backslash, which browsers strip or fold into "/" before navigating.

+ for _, hostile := range []string{"//evil", "/%09/evil", "/%5c/evil", "/%5C/evil", "https://evil", "https:%2F%2Fevil", "/%0d%0a/evil"} {

+ rec := f.request("POST", "/appearance", directoryOrigin, "mode=dark&return_to="+hostile, person)

+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/" {

+ t.Errorf("return_to=%s: %d to %q, want 303 to /", hostile, rec.Code, rec.Header().Get("Location"))

+ }

+// Appearance comes back to the page it was chosen on only if that page

+// is there to come back to: a GET that drew a 200. A 404's own path, an

+// error page's, or the path of a POST that drew a page would 303 back to

+// a page that is not there (a POST-only path answers a GET with a 404).

+func TestAppearanceReturnsOnlyToARealPage(t *testing.T) {

+ f := newMountedChooser(t)

+ person := f.session(t, chooserPerson)

+ directory := f.p.directory.Router()

+ returnTo := regexp.MustCompile(`name="return_to" value="([^"]*)"`)

+ for _, c := range []struct {

+ name, target, want string

+ }{

+ {"the directory", "/", "/"},

+ {"open source", "/help/open-source", "/help/open-source"},

+ {"a 404", "/no/such/page", "/"},

+ {"a GET to a POST-only path", "/appearance", "/"},

+ } {

+ t.Run(c.name, func(t *testing.T) {

+ body := chromeGet(directory, directoryOrigin+c.target, acceptHTML, person).Body.String()

+ m := returnTo.FindStringSubmatch(body)

+ if m == nil {

+ t.Fatalf("no Appearance return_to on %s", c.target)

+ }

+ if m[1] != c.want {

+ t.Errorf("return_to %q, want %q", m[1], c.want)

+ }

+ })

+ }

+ // A page drawn in answer to a POST, or with any status but 200.

+ for _, c := range []struct {

+ method string

+ status int

+ }{

+ {http.MethodPost, http.StatusOK},

+ {http.MethodGet, http.StatusServiceUnavailable},

+ {http.MethodGet, http.StatusInternalServerError},

+ } {

+ r := httptest.NewRequest(c.method, directoryOrigin+"/help/open-source", nil)

+ r.Header.Set("Accept", acceptHTML)

+ r.AddCookie(person)

+ var got string

+ f.p.directory.sessions.Middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

+ got = f.p.directory.account(r, c.status).Appearance.Return

+ })).ServeHTTP(httptest.NewRecorder(), r)

+ if got != "/" {

+ t.Errorf("%s drawing a %d: return_to %q, want \"/\"", c.method, c.status, got)

+ }

+// A 500 for a GET offers to try the page again beside the way home; it

+// does not replace the way home, which is the one link known to work.

+func TestServerErrorKeepsTheWayHome(t *testing.T) {

+ f := newMountedChooser(t)

+ for _, c := range []struct {

+ name string

+ app *App

+ method string

+ target string

+ home string

+ tryAgain bool

+ }{

+ {"directory, GET", f.p.directory, http.MethodGet, directoryOrigin + "/help/open-source?x=1", "/", true},

+ {"directory, POST", f.p.directory, http.MethodPost, directoryOrigin + "/appearance", "/", false},

+ {"id.*, GET", f.p.credential.App, http.MethodGet, Origin + "/passkey/codes", directoryOrigin + "/", true},

+ } {

+ t.Run(c.name, func(t *testing.T) {

+ r := httptest.NewRequest(c.method, c.target, nil)

+ r.Header.Set("Accept", acceptHTML)

+ rec := httptest.NewRecorder()

+ c.app.serverError(rec, r)

+ body := rec.Body.String()

+ if rec.Code != http.StatusInternalServerError {

+ t.Fatalf("status %d", rec.Code)

+ }

+ if !strings.Contains(body, `<a rst-btn="primary" href="`+c.home+`">Go to Oficina</a>`) {

+ t.Errorf("no Go to Oficina link to %s: %s", c.home, errorCTA(body))

+ }

+ again := `<a rst-btn href="` + strings.TrimPrefix(strings.TrimPrefix(c.target, directoryOrigin), Origin) + `">Try again</a>`

+ if got := strings.Contains(body, ">Try again</a>"); got != c.tryAgain {

+ t.Errorf("Try again shown %v, want %v: %s", got, c.tryAgain, errorCTA(body))

+ }

+ if c.tryAgain && !strings.Contains(body, again) {

+ t.Errorf("Try again does not retry the page (want %s): %s", again, errorCTA(body))

+ }

+ })

+ }

+// errorCTA is the error page's links, for a failure message.

+func errorCTA(body string) string {

+ if i := strings.Index(body, "<p rst-error-cta>"); i >= 0 {

+ end := strings.Index(body[i:], "</p>")

+ if end > 0 {

+ return body[i : i+end]

+ }

+ return "(no rst-error-cta)"

+// Every status the error page can be asked for has its own words; one

+// nobody wrote words for gets a general page, never a blank heading.

+func TestErrorPageHasCopyForEveryStatus(t *testing.T) {

+ f := newMountedChooser(t)

+ headings := map[string]int{}

+ for _, status := range []int{

+ http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,

+ http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusRequestEntityTooLarge,

+ http.StatusTooManyRequests, http.StatusInternalServerError, http.StatusServiceUnavailable,

+ } {

+ heading := checkErrorCopy(t, f.p.directory, status)

+ if other, ok := headings[heading]; ok {

+ t.Errorf("%d and %d share the heading %q", status, other, heading)

+ }

+ headings[heading] = status

+ }

+ // Statuses with no words of their own: a general page.

+ for _, status := range []int{http.StatusTeapot, http.StatusConflict, http.StatusBadGateway} {

+ checkErrorCopy(t, f.p.directory, status)

+ }

+var (

+ errorHeading = regexp.MustCompile(`<h1 rst-error-title>([^<]*)</h1>`)

+ errorBody = regexp.MustCompile(`<p rst-error-body>([^<]*)</p>`)

+// checkErrorCopy draws status's error page and fails on any part of its

+// copy left blank. It returns the heading.

+func checkErrorCopy(t *testing.T, a *App, status int) string {

+ t.Helper()

+ r := httptest.NewRequest(http.MethodGet, directoryOrigin+"/x", nil)

+ r.Header.Set("Accept", acceptHTML)

+ rec := httptest.NewRecorder()

+ a.errorPage(rec, r, status)

+ body := rec.Body.String()

+ if rec.Code != status {

+ t.Errorf("%d: drew a %d", status, rec.Code)

+ }

+ title := titleOf(body)

+ h := errorHeading.FindStringSubmatch(body)

+ b := errorBody.FindStringSubmatch(body)

+ if title == "" || strings.HasPrefix(title, " ·") || strings.HasPrefix(title, "·") {

+ t.Errorf("%d: tab title %q", status, title)

+ }

+ if h == nil || strings.TrimSpace(h[1]) == "" {

+ t.Errorf("%d: no heading", status)

+ return ""

+ }

+ if b == nil || strings.TrimSpace(b[1]) == "" {

+ t.Errorf("%d: no body copy", status)

+ }

+ return h[1]

}

// estilo's error page on both origins, for a browser; everything else