amadan

oficina / sheets Public

Send the Home root through the session guard, so a revoked user cannot loop

A Home session whose member lost admission was refused by identity, so
/ sent it to /signin; connect's /signin saw the live cookie and sent it
back to /, forever (Astra, #39 finding 2). / now runs homeRequire, which
ends the stale session and keeps its 503 for storage failures; a signed-
out visitor goes to /signin?return_to=%2F. The regression follows the
redirects from / and was red before the fix (/ visited twice).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev a47411d9911851b7ab4b51b0f93caa0d8b2f73c9 parent b8788ff
2 files changed, +62 −7
  • internal/sheets/chrome_signin_test.go +48 −2
  • internal/sheets/page.go +14 −5
diff --git a/internal/sheets/chrome_signin_test.go b/internal/sheets/chrome_signin_test.go
index 287df87..1e8dea5 100644
--- a/internal/sheets/chrome_signin_test.go
+++ b/internal/sheets/chrome_signin_test.go
@@ -2,6 +2,7 @@ package sheets
import (
"net/http"
+ "net/url"
"strings"
"testing"
)
@@ -35,8 +36,8 @@ func TestConnectYourOficinaAccountIsHiddenForHomeSessions(t *testing.T) {
// hop lands on the root. Signed in, it is the list, both ways.
func TestTheRootStartsSignInWhenHomeIsConfigured(t *testing.T) {
f := newHomeFixture(t)
- if rec := f.request(t, "GET", "/", nil, false); rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin" {
- t.Fatalf("signed-out / with Home = %d %q, want 303 /signin", rec.Code, rec.Header().Get("Location"))
+ if rec := f.request(t, "GET", "/", nil, false); rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/signin?return_to=%2F" {
+ t.Fatalf("signed-out / with Home = %d %q, want 303 /signin?return_to=%%2F", rec.Code, rec.Header().Get("Location"))
}
homeSignedIn(t, f)
if rec := f.request(t, "GET", "/", nil, false); rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/sheets" {
@@ -103,3 +104,48 @@ func TestSignOutLandsOnTheSignedOutPage(t *testing.T) {
}
}
}
+
+// A Home user whose admission is revoked while their session is live must
+// not bounce between "/" and "/signin" forever (Astra, #39 finding 2).
+// "/" sent them to /signin because identity refused them; connect's
+// /signin saw the still-live cookie and sent them back to "/". The root
+// now goes through the same guard as every signed-in page, which ends
+// the stale session, so the redirects terminate at a fresh sign-in.
+func TestARevokedHomeUserIsNotBouncedBetweenTheRootAndSignIn(t *testing.T) {
+ f := newHomeFixture(t)
+ member := homeSignedIn(t, f)
+ f.app.admit = func(string) bool { return false }
+ if err := f.app.db.Where("member_id = ?", member.ID).Delete(&Grant{}).Error; err != nil {
+ t.Fatal(err)
+ }
+ path, seen := "/", map[string]int{}
+ for hop := 0; ; hop++ {
+ if hop == 8 {
+ t.Fatalf("still redirecting after 8 hops: %v", seen)
+ }
+ seen[path]++
+ if seen[path] > 1 {
+ t.Fatalf("redirect loop: %s visited twice (%v)", path, seen)
+ }
+ rec := f.request(t, "GET", path, nil, false)
+ loc := rec.Header().Get("Location")
+ if rec.Code != http.StatusSeeOther || loc == "" {
+ break
+ }
+ u, err := url.Parse(loc)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if u.Host != "" {
+ // Off to id.* for a fresh sign-in: the loop is over.
+ if u.Host != "id.example.com" {
+ t.Fatalf("left for %s", loc)
+ }
+ break
+ }
+ path = u.RequestURI()
+ }
+ if _, ok := f.jar[f.app.homeSessions.CookieName()]; ok {
+ t.Fatal("the revoked session survived")
+ }
+}
diff --git a/internal/sheets/page.go b/internal/sheets/page.go
index 1800aef..a41f0e4 100644
--- a/internal/sheets/page.go
+++ b/internal/sheets/page.go
@@ -37,14 +37,23 @@ import (
// hop used to run for them. Docs and Calendar already answer their root
// this way. Standalone keeps the landing page: there is no Home to hand
// off to, and it is where a stranger learns what this instance is.
+//
+// With Home, "/" goes through homeRequire, the guard every signed-in page
+// uses, rather than a bare identity check. A Home session whose member
+// lost admission is refused by identity but is still a live cookie, and
+// connect's /signin skips the form for a live cookie: redirecting such a
+// person straight to /signin sent them back here, forever (Astra, #39).
+// The guard ends that session first, keeps its 503 for a storage failure,
+// and sends a signed-out visitor to /signin?return_to=%2F.
func (a *App) handleIndex(w http.ResponseWriter, r *http.Request) {
- if _, ok := a.identity(r); ok {
- http.Redirect(w, r, "/sheets", http.StatusSeeOther)
+ if a.home != nil {
+ a.homeRequire(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, "/sheets", http.StatusSeeOther)
+ })).ServeHTTP(w, r)
return
}
- if a.home != nil {
- w.Header().Set("Cache-Control", "no-store")
- http.Redirect(w, r, "/signin", http.StatusSeeOther)
+ if _, ok := a.identity(r); ok {
+ http.Redirect(w, r, "/sheets", http.StatusSeeOther)
return
}
a.render(w, r, "index", frame{}, http.StatusOK, nil)