amadan

oficina / home Public

id.* sign-out: confirm a preview's cross-site hop; judge site by hostname

A preview's hop is cross-site, and the SameSite=Lax id.* cookie does not
ride it, so answering with a sign-out cleared the cookie in the browser
but left the session row alive behind any copy of it. The hop now gets
the "Finish signing out" page (200) and keeps the cookie; its same-origin
button carries the cookie and revokes the row (#74 review, finding 1).

onSuiteSite compared the host, port included, against the directory's
host, so a registered suite origin on a non-default port was refused. It
now compares the parsed hostname; the redirect keeps the full origin
(finding 2).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 2545ef588beb1cbf98668e4194724bdf84060572 parent c783513
2 files changed, +120 −21
  • internal/home/signout.go +52 −18
  • internal/home/signout_test.go +68 −3
diff --git a/internal/home/signout.go b/internal/home/signout.go
index 1c50306..63bf16c 100644
--- a/internal/home/signout.go
+++ b/internal/home/signout.go
@@ -51,10 +51,18 @@ const signoutPath = "/signout"
// have a way through.
func (a *App) handleCredentialSignout(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Cache-Control", "no-store")
- back, ok := a.signoutReturn(r)
- if !ok {
+ back, verdict := a.signoutReturn(r)
+ switch verdict {
+ case signoutRefused:
a.render(w, http.StatusForbidden, "signout_confirm.html", nil)
return
+ case signoutConfirm:
+ // Admitted, but the id.* cookie did not ride this hop, so there is
+ // no row to revoke yet. Clearing the cookie now would leave the
+ // row alive behind it; keep it, and let the same-origin button
+ // (which does carry the cookie) end the session.
+ a.render(w, http.StatusOK, "signout_confirm.html", nil)
+ return
}
a.sessions.SignOut(w, r)
http.Redirect(w, r, back+"/signed-out", http.StatusSeeOther)
@@ -99,41 +107,60 @@ func (a *App) handleCredentialSignoutPage(w http.ResponseWriter, r *http.Request
// preview), or absent. A suite origin arriving cross-site passed
// through another site on the way, and a preview is never same-site.
//
+// A preview is admitted but not signed out on the spot (signoutConfirm).
+// Its hop is cross-site, and the id.* session cookie is SameSite=Lax, so
+// the POST arrives without it: there is no row to revoke, and clearing
+// the cookie in the browser would leave a live row behind a copy of it.
+// It gets the "Finish signing out" page, whose same-origin POST carries
+// the cookie and revokes the row (the #74 review, finding 1).
+//
// A page elsewhere cannot set Referer to a consumer's origin. What is
// left, and accepted (spec §9.5), is logout CSRF from a page ON a
// consumer origin: any of them can post here. That needs a user-injected
// form on a page the victim visits, and a consumer that renders one must
// not do it on a registered origin.
-func (a *App) signoutReturn(r *http.Request) (string, bool) {
+type signoutVerdict int
+
+const (
+ signoutRefused signoutVerdict = iota
+ signoutAdmitted
+ signoutConfirm
+)
+
+func (a *App) signoutReturn(r *http.Request) (string, signoutVerdict) {
if csrf.SameOrigin(r, a.origins.Credential) {
- return a.origins.Directory, true
+ return a.origins.Directory, signoutAdmitted
}
claimed := r.Header.Get("Origin")
if claimed == "" || claimed == "null" {
ref, err := url.Parse(r.Header.Get("Referer"))
if err != nil || ref.User != nil || ref.Scheme == "" || ref.Host == "" {
- return "", false
+ return "", signoutRefused
}
claimed = ref.Scheme + "://" + ref.Host
}
matched, ok := a.consumerOrigin(claimed)
if !ok {
- return "", false
+ return "", signoutRefused
}
+ suite := a.onSuiteSite(matched)
switch r.Header.Get("Sec-Fetch-Site") {
case "":
case "same-site":
- if !a.onSuiteSite(matched) {
- return "", false
+ if !suite {
+ return "", signoutRefused
}
case "cross-site":
- if a.onSuiteSite(matched) {
- return "", false
+ if suite {
+ return "", signoutRefused
}
default:
- return "", false
+ return "", signoutRefused
}
- return matched, true
+ if !suite {
+ return matched, signoutConfirm
+ }
+ return matched, signoutAdmitted
}
// consumerOrigin returns the registered consumer origin exactly equal to
@@ -151,11 +178,18 @@ func (a *App) consumerOrigin(origin string) (string, bool) {
return "", false
}
-// onSuiteSite reports whether origin is the directory or under its host.
-// The directory is the suite's apex (oficina.works), and the credential
-// origin and every team instance sit under it, so this is the site a
-// browser's Sec-Fetch-Site compares against.
+// onSuiteSite reports whether origin's hostname is the directory's host
+// or under it. The directory is the suite's apex (oficina.works), and the
+// credential origin and every team instance sit under it, so this is the
+// site a browser's Sec-Fetch-Site compares against. A site has no port,
+// so the hostname is compared, never the host: a registered team on :8443
+// is still same-site (the #74 review, finding 2). The caller keeps the
+// full origin, port included, for the redirect.
func (a *App) onSuiteSite(origin string) bool {
- host := strings.TrimPrefix(origin, "https://")
- return origin == a.origins.Directory || strings.HasSuffix(host, "."+a.directoryHost)
+ u, err := url.Parse(origin)
+ if err != nil {
+ return false
+ }
+ host := u.Hostname()
+ return host == a.directoryHost || strings.HasSuffix(host, "."+a.directoryHost)
}
diff --git a/internal/home/signout_test.go b/internal/home/signout_test.go
index 3aad60d..434724d 100644
--- a/internal/home/signout_test.go
+++ b/internal/home/signout_test.go
@@ -241,9 +241,6 @@ func TestCredentialSignOutAdmitsRegisteredConsumers(t *testing.T) {
{"null", "same-site", teamOrigin + "/d/42?view=compact", teamOrigin},
{"", "", teamOrigin + "/", teamOrigin},
{teamOrigin, "", "", teamOrigin},
- {previewOrigin, "cross-site", "", previewOrigin},
- {"null", "cross-site", previewOrigin + "/", previewOrigin},
- {"", "", previewOrigin + "/", previewOrigin},
{"null", "same-site", directoryOrigin + "/", directoryOrigin},
{directoryOrigin, "same-site", directoryOrigin + "/", directoryOrigin},
} {
@@ -355,3 +352,71 @@ func TestCredentialSignOutRefusalIsBrandedAndRecoverable(t *testing.T) {
t.Fatal("directory served the credential's sign-out page", rec.Code)
}
}
+
+// TestCredentialSignOutFromAPreviewConfirmsFirst: a preview is registered
+// but off the suite's site, so its hop is cross-site, and the id.* cookie
+// (SameSite=Lax) does not ride a cross-site POST. Answering it with a
+// sign-out would clear the cookie in the browser but leave the session
+// row alive, so a copy of the cookie would still sign in. So the hop gets
+// the "Finish signing out" page, the cookie is kept, and its same-origin
+// button, which does carry the cookie, revokes the row.
+func TestCredentialSignOutFromAPreviewConfirmsFirst(t *testing.T) {
+ for _, c := range []struct{ origin, site, referer string }{
+ {previewOrigin, "cross-site", ""},
+ {"null", "cross-site", previewOrigin + "/"},
+ {"", "", previewOrigin + "/"},
+ } {
+ p := withPreviewConsumer(newExchangePair(t))
+ credential, _ := signedInPair(t, p)
+ // The browser sends no cookie on this hop; send none.
+ rec := signoutHop(p, c.origin, c.site, c.referer)
+ if rec.Code != http.StatusOK || !isFinishSigningOut(rec) || clearsCookie(rec, credential.Name) {
+ t.Fatalf("%+v: %d, branded=%v, cleared=%v", c, rec.Code, isFinishSigningOut(rec), clearsCookie(rec, credential.Name))
+ }
+ if !resumeMints(t, p, credential) {
+ t.Fatalf("%+v: the session ended before it was confirmed", c)
+ }
+ r := httptest.NewRequest("POST", Origin+"/signout", nil)
+ r.Header.Set("Origin", Origin)
+ r.Header.Set("Sec-Fetch-Site", "same-origin")
+ r.AddCookie(credential)
+ confirm := httptest.NewRecorder()
+ p.credential.Router().ServeHTTP(confirm, r)
+ if confirm.Code != http.StatusSeeOther || confirm.Header().Get("Location") != directoryOrigin+"/signed-out" || !clearsCookie(confirm, credential.Name) {
+ t.Fatalf("%+v: confirm %d %q", c, confirm.Code, confirm.Header().Get("Location"))
+ }
+ // The row is revoked, so a copy of the old cookie is refused.
+ if resumeMints(t, p, credential) {
+ t.Fatalf("%+v: a copy of the cookie still signs in after confirming", c)
+ }
+ }
+}
+
+// TestCredentialSignOutKeepsASuiteOriginsPort: a registered suite origin
+// on a non-default port is same-site. The site is judged by hostname,
+// and the browser goes back to the full origin, port included.
+func TestCredentialSignOutKeepsASuiteOriginsPort(t *testing.T) {
+ const ported = "https://blue.sheets.oficina.works:8443"
+ for _, c := range []struct{ origin, site, referer string }{
+ {ported, "same-site", ""},
+ {"null", "same-site", ported + "/"},
+ } {
+ p := newExchangePair(t)
+ p.credential.runtime.consumers["team_sheets_ported"] = ported
+ credential, _ := signedInPair(t, p)
+ rec := signoutHop(p, c.origin, c.site, c.referer, credential)
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != ported+"/signed-out" {
+ t.Fatalf("%+v: %d %q", c, rec.Code, rec.Header().Get("Location"))
+ }
+ if resumeMints(t, p, credential) {
+ t.Fatalf("%+v: session survived", c)
+ }
+ }
+ // Still the site rule: that ported suite origin arriving cross-site is refused.
+ p := newExchangePair(t)
+ p.credential.runtime.consumers["team_sheets_ported"] = ported
+ credential, _ := signedInPair(t, p)
+ if rec := signoutHop(p, ported, "cross-site", "", credential); rec.Code != http.StatusForbidden {
+ t.Fatal("ported suite origin admitted cross-site", rec.Code)
+ }
+}