amadan

oficina / docs Public

The branch review's findings: a guest kind, no editor for readers, hashed tokens

Fable's whole-branch review refused this branch and was right to. Two
blockers and ten smaller findings; all of the blockers and all but one of
the rest are fixed here. Five mutations run against the new guards and all
five are caught.

**A reader was handed a live editor (blocker).** `show` used GetFor —
correct, a reader may read — and then rendered the owner's page verbatim:
ProseMirror mounted, the toolbar enabled, the document went
contenteditable. The server refused every write, which was never the
point: by the time the 404 arrives the person has typed a paragraph, and
save-queue.mjs classifies a 4xx as permanent, drops the batch, and keeps
taking keystrokes. They lose the work, having been shown an editor. "It
fails safely on the server" is not a defence when the cost is somebody's
writing. The editor is now absent rather than disabled — a disabled editor
is one CSS rule from being an editor again.

**A grant made a full member (blocker).** Suite §1 ruling 1 and Docs §8.2
both say an outside guest is a *guest* member. There was no guest kind, so
one reader grant was an unbounded transitive key to the front door: Bob is
admitted by Alice's grant, creates documents, grants Mallory, who repeats
it — and $DOCS_ALLOWED_ADDRESSES had quietly stopped being the membership
list the operator configured. A guest now reads and writes what they were
granted and cannot create or share.

**Revocation ended nothing.** allows() ran only at sign-in, so revoking a
guest's last grant left them a valid session for thirty days. Admission is
re-asked per request now. Members pay nothing for it — allows() answers
from the configured set before touching the database — and a guest is
exactly who the answer can have changed for.

**The token is hashed (suite ruling 16).** IsPublished returns a bool
rather than a link, because the store genuinely cannot produce a URL any
more; that is the ruling expressed as a signature rather than as a
convention somebody has to keep. The URL reaches exactly one render
through a 30-second one-shot, so the 303 survives without a reload
re-posting and rotating the link mid-copy.

**Three of my own tests were proving nothing, and finding them is most of
what this commit is.**

TestSignIn read the last mail sent to anyone. A refused address gets the
same 303 ?sent=1 a real one does — deliberately, so the endpoint is not a
membership oracle — so it returned somebody else's link and reported
success for an address the allowlist had refused. I added a recipient
check and it was STILL wrong: a second refusal of an address that once
succeeded passes, because the recorder keeps the earlier mail. It now
requires a new send.

NewForTest wires no BaseCatalog, so catalog strings render as their keys.
An assertion I had just written, Contains(body, "published"), passed on
the key "docs.share.public.published" no matter what the page said.

And the test written to catch the reader-editor bug first banned the bare
string "docs-editor", which appears a dozen times in the page's own CSS.
It would have failed on a correct page while telling me nothing about the
element.

Also: created_by restored to grants (the spec's §4 model has it; it was
dropped while the nullable doc_id beside it was argued at length),
no-store and X-Robots-Tag on /p/, real <title> blocks on the two pages
rendering "Hello", address validation and a refusal to grant to yourself,
HasGrant scoped so a grant on a deleted document stops holding the door
open, a "Shared with you" row marker, no sample library for guests, and
the dozen comments still claiming "owner-scoped" about reader-scoped code.

Not done, deliberately: caching the CRDT replay behind /p/. The review
marked it non-blocking; it is a performance change to a hot path and
belongs in its own reviewed branch, not in the tail of this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RVcEQ8NgrPCpRTb6JtBz1Q
Paul Campbell pushed by paul@keymail.dev c8415ceb4753c409d0df9375e6437d363dbd375f parent 137cfd8
25 files changed, +1257 −113
  • internal/docs/app.go +21 −5
  • internal/docs/auth.go +79 −5
  • internal/docs/grants.go +89 −11
  • internal/docs/grants_edges_test.go +64 −0
  • internal/docs/guest_test.go +128 −0
  • internal/docs/handlers.go +109 −20
  • internal/docs/links.go +52 −26
  • internal/docs/migrations/0005_grants.sql +11 −0
  • internal/docs/migrations/0006_links.sql +25 −7
  • internal/docs/oneshot.go +84 −0
  • internal/docs/publiclink_test.go +143 −9
  • internal/docs/publish.go +28 −6
  • internal/docs/reader_noeditor_test.go +81 −0
  • internal/docs/render.go +19 −1
  • internal/docs/search.go +1 −1
  • internal/docs/share.go +53 −9
  • internal/docs/shared_list_test.go +40 −0
  • internal/docs/store.go +19 −2
  • internal/docs/stream.go +1 −1
  • internal/docs/testing.go +123 −5
  • internal/docs/views/list.html +2 −2
  • internal/docs/views/public.html +1 −0
  • internal/docs/views/share.html +14 −0
  • internal/docs/views/show.html +9 −3
  • internal/docs/writer_http_test.go +61 −0

CI passed — run details

Intent

full ledger: sharing

diff --git a/internal/docs/app.go b/internal/docs/app.go
index b74468e..664648a 100644
--- a/internal/docs/app.go
+++ b/internal/docs/app.go
@@ -18,6 +18,7 @@ import (
"net/http"
"net/url"
"strings"
+ "time"
"github.com/carlosframework/rastrillo/csrf"
"github.com/carlosframework/rastrillo/db"
@@ -385,8 +386,19 @@ func New(cfg Config) (*App, error) {
// exists — deliberately, so a misconfigured allowlist refuses to boot
// before anything else is built. See allowlist.granted for why an
// unredeemable invitation is the failure worth avoiding.
+ // The sample library is for members. A guest admitted by a grant is
+ // visiting, not moving in.
+ store.seedOnlyMembers = list.configured
+
list.granted = func(address string) bool {
- return store.HasGrant(context.Background(), address)
+ // A short deadline rather than context.Background(): this runs on
+ // the sign-in path and now on every request (requireAdmission), so
+ // a stalled query must fail rather than hold a request open. A
+ // failure answers false, which refuses admission — the safe
+ // direction, and the configured allowlist still admits members.
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+ defer cancel()
+ return store.HasGrant(ctx, address)
}
render, err := newRenderer()
@@ -410,7 +422,7 @@ func New(cfg Config) (*App, error) {
return nil, fmt.Errorf("docs: build auth: %w", err)
}
- h := &handlers{render: render, logger: logger, store: store, hub: newHub(), previews: newPreviews(), origin: cfg.Origin}
+ h := &handlers{render: render, logger: logger, store: store, hub: newHub(), previews: newPreviews(), origin: cfg.Origin, isMember: list.configured, oneShots: newOneShots()}
r := chi.NewRouter()
// Mounted above every route, per rastrillo SKILL.md §5: refuses a
@@ -456,10 +468,14 @@ func New(cfg Config) (*App, error) {
// GET/HEAD redirects to /signin; anything else 403s.
r.Group(func(r chi.Router) {
r.Use(authPlugin.RequireSession)
+ // Admission is re-asked per request, not only at sign-in, so
+ // revoking a guest's last grant ends their access now rather
+ // than whenever their 30-day session happens to expire.
+ r.Use(requireAdmission(list, "/signin"))
r.Get("/", h.list)
r.Post("/documents", h.create)
// Title search for the jump-to palette (search.go). Inside
- // this group like every other document route, and owner-scoped
+ // this group like every other document route, and reader-scoped
// through Store.Search → Store.List rather than through a
// check of its own — a person's document titles are theirs.
r.Get("/documents/search", h.searchJSON)
@@ -482,7 +498,7 @@ func New(cfg Config) (*App, error) {
r.Post("/d/{sqid}/updates", h.updatesPost)
// Export (export.go). Inside this group, so a signed-out
// request gets the same redirect every other document route
- // gives it, and owner-scoped through the same Store.Get, so a
+ // gives it, and reader-scoped through the same Store.GetFor, so a
// document someone else owns 404s identically to one that does
// not exist. Extensions in the path rather than a ?format=
// query: the URL is what a person sees when they save the
@@ -491,7 +507,7 @@ func New(cfg Config) (*App, error) {
r.Get("/d/{sqid}/export.md", h.exportMarkdown)
r.Get("/d/{sqid}/export.pdf", h.exportPDF)
// Plan 5: the live-update stream (stream.go). A GET, so
- // csrf.Protect passes it through untouched, and owner-scoped
+ // csrf.Protect passes it through untouched, and reader-scoped
// through the same getDocumentForUpdates helper as the two
// routes above — a stranger gets 404, never 403.
r.Get("/d/{sqid}/stream", h.stream)
diff --git a/internal/docs/auth.go b/internal/docs/auth.go
index 0c5c744..254660b 100644
--- a/internal/docs/auth.go
+++ b/internal/docs/auth.go
@@ -107,14 +107,29 @@ func buildMailer(cfg Config, logger *slog.Logger) (mail.Sender, *recordingMailer
// dev-mode behavior (the link lands in the log, clearly marked) is
// unchanged; this only adds a second, in-memory place to find it.
type recordingMailer struct {
- mu sync.Mutex
- next mail.Sender
+ mu sync.Mutex
+ next mail.Sender
+ // lastTo is recorded alongside lastBody, and it is not decoration.
+ // gateBegin answers a refused address with exactly the 303 ?sent=1 a
+ // real one gets — deliberately, so the endpoint is not a membership
+ // oracle — so a caller that reads only lastBody cannot tell "a link
+ // was mailed to this address" from "somebody else's link is still
+ // sitting here". TestSignIn used to make that mistake and reported
+ // success for an address the allowlist had refused.
+ lastTo string
lastBody string
+ // sends counts mails, and it is the field that makes the recipient
+ // check above actually work. Comparing only lastTo answers "was the
+ // most recent mail addressed here", which stays true for every later
+ // REFUSAL of the same address — the recorder simply keeps the earlier
+ // success. A caller has to know a NEW mail was sent, not that an old
+ // one is still lying there.
+ sends int
}
func (m *recordingMailer) Send(ctx context.Context, to, subject, body string) error {
m.mu.Lock()
- m.lastBody = body
+ m.lastTo, m.lastBody, m.sends = to, body, m.sends+1
m.mu.Unlock()
return m.next.Send(ctx, to, subject, body)
}
@@ -168,6 +183,32 @@ type allowlist struct {
granted func(address string) bool
}
+// configured reports whether address is a MEMBER of this instance — on
+// $DOCS_ALLOWED_ADDRESSES, rather than admitted by a grant.
+//
+// The distinction the branch review found missing (finding 1). Suite §1
+// ruling 1 and Docs §8.2 both say an outside guest is a *guest* member;
+// the branch had no guest kind, so a grant was a full membership and
+// admission became transitive — Alice grants Bob, Bob creates and grants
+// Mallory, and the operator's allowlist has quietly stopped being the
+// membership list.
+//
+// A guest reads and writes what they were granted. Creating documents and
+// changing who reaches one are member powers, and this is the predicate
+// that says so.
+//
+// addrs == nil (development's admit-everyone) makes everybody a member,
+// which is the same latitude allows() already gives and the same reason:
+// a dev instance with no configuration should not need a membership list
+// to be usable.
+func (a allowlist) configured(address string) bool {
+ if a.addrs == nil {
+ return true
+ }
+ _, ok := a.addrs[normalizeAddress(address)]
+ return ok
+}
+
// allows reports whether address may sign in.
func (a allowlist) allows(address string) bool {
if a.addrs == nil {
@@ -177,8 +218,10 @@ func (a allowlist) allows(address string) bool {
if _, ok := a.addrs[normalized]; ok {
return true
}
- // A grant admits its holder, and nobody else. This only ever widens: an
- // addrs == nil allowlist has already returned true above.
+ // A grant admits its holder, and nobody else — as a guest, not as a
+ // member: see configured() above for the powers that stay behind the
+ // configured list, and requireAdmission for why this is now re-asked on
+ // every request rather than only at sign-in.
return a.granted != nil && a.granted(normalized)
}
@@ -303,6 +346,37 @@ func gateBegin(next http.HandlerFunc, list allowlist, signinPath string) http.Ha
}
}
+// requireAdmission re-asks the front-door question on every request, and
+// answers a person who no longer passes it by ending them at /signin.
+//
+// It exists because allows() used to run only at sign-in. A session lasts
+// 30 days (rastrillo/auth.DefaultSessionTTL), so revoking somebody's last
+// grant took their document away and left them a working session for a
+// month — during which, before the guest rule above, they went on creating
+// documents and granting them to other people. "Stop sharing" was complete
+// for the document and silent about the instance. (Branch review, finding
+// 3.)
+//
+// Cheap for members: allows() checks the configured set first and returns
+// before touching the database. Only a guest pays a query, and a guest is
+// exactly who the answer can have changed for.
+//
+// It sits after RequireSession so it can rely on there being an identity;
+// with no identity it does nothing and lets RequireSession's own redirect
+// stand.
+func requireAdmission(list allowlist, signinPath string) func(http.Handler) http.Handler {
+ return func(next http.Handler) http.Handler {
+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ id, ok := rauth.From(r)
+ if ok && !list.allows(id.Address) {
+ http.Redirect(w, r, signinPath, http.StatusSeeOther)
+ return
+ }
+ next.ServeHTTP(w, r)
+ })
+ }
+}
+
// newAuthPlugin wires rastrillo/auth per its SKILL.md contract: one
// *Auth built at boot (New — SKILL.md's own warning against building
// a Flow per request, since the rate limiter's budget lives on the
diff --git a/internal/docs/grants.go b/internal/docs/grants.go
index e663637..8723c45 100644
--- a/internal/docs/grants.go
+++ b/internal/docs/grants.go
@@ -31,10 +31,15 @@ func (r Role) valid() bool { return r == RoleReader || r == RoleWriter }
// sample library, so minting a Person row to hold a grant would hand an
// invitee six documents they never asked for before they had accepted.
type Grant struct {
- ID int64 `gorm:"column:id;primaryKey"`
- DocID int64 `gorm:"column:doc_id;index"`
- Subject string `gorm:"column:subject;index"`
- Role Role `gorm:"column:role"`
+ ID int64 `gorm:"column:id;primaryKey"`
+ DocID int64 `gorm:"column:doc_id;index"`
+ Subject string `gorm:"column:subject;index"`
+ Role Role `gorm:"column:role"`
+ // CreatedBy is the person who made this grant — the spec's §4 data
+ // model has it and this table shipped without it. It is what lets an
+ // operator answer "who let this address in", which is a question with
+ // no other source once a grant exists.
+ CreatedBy int64 `gorm:"column:created_by"`
CreatedAt time.Time `gorm:"column:created_at"`
}
@@ -84,8 +89,37 @@ func writable(g *gorm.DB, a Actor) *gorm.DB {
// subjectOf normalises an address the way EnsurePerson stores one, so a grant
// made to "Bob@Example.com " is the grant Bob holds when he signs in.
+//
+// It defers to normalizeAddress rather than repeating it. The two were the
+// same two lines in two files — the two halves of one key, free to drift
+// into comparing differently-normalised forms of the same address, which
+// would have shown up as a grant that silently admits nobody.
func subjectOf(address string) string {
- return strings.ToLower(strings.TrimSpace(address))
+ return normalizeAddress(address)
+}
+
+// addressOf maps a person id back to the address they signed in with, so
+// Grant can refuse a grant somebody makes to themselves.
+func (s *Store) addressOf(ctx context.Context, person int64) (string, error) {
+ var p Person
+ if err := s.db.WithContext(ctx).First(&p, person).Error; err != nil {
+ return "", err
+ }
+ return subjectOf(p.Email), nil
+}
+
+// looksLikeAnAddress is the cheapest check that keeps nonsense out of the
+// front door.
+//
+// Not a validator: RFC 5322 is not worth implementing here, and rauth owns
+// the real parsing downstream. This exists because Grant accepted any
+// non-empty string, so "not-an-email" became a row, appeared on the share
+// page, and made HasGrant answer true — widening admission to a string
+// that can never sign in, and leaving the owner a share list with a typo
+// on it they will read as a person. (Branch review, finding 10.)
+func looksLikeAnAddress(subject string) bool {
+ at := strings.Index(subject, "@")
+ return at > 0 && at < len(subject)-1 && !strings.ContainsAny(subject, " \t")
}
// readable scopes a query to the documents one person may read: the ones they
@@ -128,7 +162,7 @@ func (s *Store) Grant(ctx context.Context, owner int64, sqid, address string, ro
return fmt.Errorf("docs: unknown role %q", role)
}
subject := subjectOf(address)
- if subject == "" {
+ if subject == "" || !looksLikeAnAddress(subject) {
return errEmptyAddress
}
@@ -140,10 +174,23 @@ func (s *Store) Grant(ctx context.Context, owner int64, sqid, address string, ro
return err
}
- grant := Grant{DocID: doc.ID, Subject: subject, Role: role, CreatedAt: time.Now().UTC()}
+ // Granting to yourself is refused: you already reach it by owning it,
+ // and the row would put the owner on their own share list as though
+ // somebody had been invited.
+ if ownerAddress, err := s.addressOf(ctx, owner); err == nil && ownerAddress == subject {
+ return errEmptyAddress
+ }
+
+ grant := Grant{
+ DocID: doc.ID,
+ Subject: subject,
+ Role: role,
+ CreatedBy: owner,
+ CreatedAt: time.Now().UTC(),
+ }
return s.db.WithContext(ctx).Clauses(clause.OnConflict{
Columns: []clause.Column{{Name: "doc_id"}, {Name: "subject"}},
- DoUpdates: clause.AssignmentColumns([]string{"role"}),
+ DoUpdates: clause.AssignmentColumns([]string{"role", "created_by"}),
}).Create(&grant).Error
}
@@ -161,9 +208,18 @@ func (s *Store) HasGrant(ctx context.Context, address string) bool {
if subject == "" {
return false
}
+ // Scoped to grants on documents that still exist. A grant on a
+ // soft-deleted or orphaned (owner_id = 0) document opens nothing —
+ // readable hides those rows — so counting it here would hold the front
+ // door open for somebody who can reach nothing at all, and would keep
+ // holding it after every document they were given had been deleted.
+ // (Branch review, finding 12.)
var count int64
if err := s.db.WithContext(ctx).Model(&Grant{}).
- Where("subject = ?", subject).Count(&count).Error; err != nil {
+ Joins("JOIN documents ON documents.id = grants.doc_id").
+ Where("grants.subject = ? AND documents.deleted_at IS NULL AND documents.owner_id > 0",
+ subject).
+ Count(&count).Error; err != nil {
s.logger.Error("docs: checking grants for admission", "err", err)
return false
}
@@ -224,6 +280,28 @@ func (s *Store) GetFor(ctx context.Context, a Actor, sqid string) (*Document, er
return &doc, nil
}
+// CanWrite reports whether the acting person may change a document.
+//
+// The same EXISTS clause writable builds, asked as a question rather than
+// used as a scope, because the document page needs the answer before it
+// renders: a reader must not be handed an editor (branch review, #2). The
+// one predicate, asked twice, so the page and the update route cannot
+// disagree about who may write.
+//
+// A database error answers false. Rendering a read-only page to somebody
+// who may write costs them a reload; rendering an editor to somebody who
+// may not costs them their work.
+func (s *Store) CanWrite(ctx context.Context, a Actor, docID int64) bool {
+ var count int64
+ if err := writable(s.db.WithContext(ctx).Model(&Document{}), a).
+ Where("documents.id = ?", docID).
+ Count(&count).Error; err != nil {
+ s.logger.Error("docs: checking write access", "doc", docID, "err", err)
+ return false
+ }
+ return count > 0
+}
+
// ListFor returns every non-deleted document the acting person may read —
// the ones they own, plus the ones they hold a grant on — newest first, on
// the same UpdatedAt clock List uses so this list and handlers.go's recency
@@ -236,8 +314,8 @@ func (s *Store) GetFor(ctx context.Context, a Actor, sqid string) (*Document, er
// The order deliberately does not separate "mine" from "shared with me".
// Recency is the sort the list page already teaches, and a shared document
// somebody is working in today belongs above one of yours you have not
-// opened since spring. Which are yours is a question the row can answer
-// without reordering the list.
+// opened since spring. Which are yours the row answers itself, through
+// docRow.SharedNote, without reordering the list.
func (s *Store) ListFor(ctx context.Context, a Actor) ([]*Document, error) {
var docs []*Document
err := readable(s.db.WithContext(ctx), a.PersonID, a.Subject).
diff --git a/internal/docs/grants_edges_test.go b/internal/docs/grants_edges_test.go
new file mode 100644
index 0000000..2417ed2
--- /dev/null
+++ b/internal/docs/grants_edges_test.go
@@ -0,0 +1,64 @@
+package docs_test
+
+import (
+ "net/http"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/docs/internal/docs"
+)
+
+// Grant refuses what is not an address.
+//
+// Before this, Grant accepted any non-empty string: "not-an-email" became
+// a row, appeared on the share page as though it were a person, and made
+// HasGrant answer true — widening the instance's front door to a string
+// that can never sign in. (Branch review, finding 10.)
+func TestGrantRefusesNonsenseAddresses(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+
+ for _, bad := range []string{"not-an-email", "@example.com", "bob@", "bob example@x.com"} {
+ rec := post(t, app, alice, "/d/"+sqid+"/share", "address="+strings.ReplaceAll(bad, " ", "+"))
+ if rec.Code == http.StatusSeeOther {
+ t.Errorf("granted to %q", bad)
+ }
+ if body := get(t, app, alice, "/d/"+sqid+"/share").Body.String(); strings.Contains(body, bad) {
+ t.Errorf("the share page lists %q", bad)
+ }
+ }
+}
+
+// An owner cannot grant to themselves. They already reach it by owning it,
+// and the row would put them on their own share list looking like an
+// invitation somebody made.
+func TestGrantRefusesTheOwner(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+
+ if rec := post(t, app, alice, "/d/"+sqid+"/share", "address=alice@example.com&role=writer"); rec.Code == http.StatusSeeOther {
+ t.Error("the owner granted a document to herself")
+ }
+}
+
+// A grant on a document that no longer exists does not hold the front door
+// open. Admission is meant to follow from reaching something; a grant on a
+// deleted document reaches nothing, because readable hides it.
+func TestAGrantOnADeletedDocumentAdmitsNobody(t *testing.T) {
+ app := members(t, "alice@example.com")
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader")
+
+ if _, err := app.TestSignIn(t, "bob@example.com"); err != nil {
+ t.Fatalf("precondition: the grantee cannot sign in: %v", err)
+ }
+
+ app.TestSoftDelete(t, sqid)
+
+ if _, err := app.TestSignIn(t, "bob@example.com"); err == nil {
+ t.Error("a grant on a deleted document still admits its holder")
+ }
+}
diff --git a/internal/docs/guest_test.go b/internal/docs/guest_test.go
new file mode 100644
index 0000000..dfa1a1b
--- /dev/null
+++ b/internal/docs/guest_test.go
@@ -0,0 +1,128 @@
+package docs_test
+
+import (
+ "net/http"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/docs/internal/docs"
+)
+
+// members builds an app whose front door is a real allowlist, so the
+// difference between a configured member and somebody admitted only by a
+// grant actually exists. NewForTest's empty allowlist means "admit
+// everyone" in development, which is why none of this was reachable before.
+func members(t *testing.T, addresses string) *docs.App {
+ t.Helper()
+ app, err := docs.NewForTestWith(t, docs.Config{AllowedAddresses: addresses})
+ if err != nil {
+ t.Fatalf("NewForTestWith: %v", err)
+ }
+ return app
+}
+
+// A grant admits its holder as a GUEST, not as a member.
+//
+// Suite §1 ruling 1: "Sharing is membership; an outside guest is a guest
+// member." Docs §8.2: "Outside guests are granted a specific document and
+// become guest members." The branch had no guest kind at all, so a single
+// reader grant was an unbounded, transitive key to the instance's front
+// door: Bob is admitted by Alice's grant, creates his own documents, shares
+// them to Mallory, who is now admitted and can repeat it. Alice cannot
+// undo any of it, and $DOCS_ALLOWED_ADDRESSES silently stopped being the
+// membership list the operator configured. (Branch review, finding 1.)
+//
+// A guest reads and writes what they were granted, and nothing else.
+func TestAGuestCannotCreateOrShare(t *testing.T) {
+ app := members(t, "alice@example.com")
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer")
+
+ // Bob gets in — the invitation is redeemable, which is the whole
+ // reason grant-admission exists.
+ bob := signInCookies(t, app, "bob@example.com")
+ if rec := get(t, app, bob, "/d/"+sqid); rec.Code != http.StatusOK {
+ t.Fatalf("the guest cannot read the document he was granted: %d", rec.Code)
+ }
+
+ // But he is a guest. He cannot mint documents of his own...
+ if rec := post(t, app, bob, "/documents", "title=Bob%27s+own"); rec.Code == http.StatusSeeOther {
+ t.Error("a guest created a document; a grant is not a membership")
+ }
+ // ...and he cannot admit anybody else, which is the transitive half
+ // and the one that makes the first unbounded.
+ if rec := post(t, app, bob, "/d/"+sqid+"/share", "address=mallory@example.com&role=writer"); rec.Code == http.StatusSeeOther {
+ t.Error("a guest shared a document; admission would be transitive")
+ }
+}
+
+// A configured member keeps everything. Without this the rule above passes
+// on an app where nobody can create anything.
+func TestAMemberStillCreatesAndShares(t *testing.T) {
+ app := members(t, "alice@example.com,carol@example.com")
+ carol := signInCookies(t, app, "carol@example.com")
+
+ sqid := createDoc(t, app, carol, "Carol's")
+ if rec := post(t, app, carol, "/d/"+sqid+"/share", "address=dave@example.com&role=reader"); rec.Code != http.StatusSeeOther {
+ t.Errorf("a configured member could not share: status %d", rec.Code)
+ }
+}
+
+// Revoking the last grant ends admission, not just the document.
+//
+// Before this, `allows()` ran only at sign-in, so a revoked guest kept a
+// valid session for up to 30 days — and, per the finding above, kept
+// creating and sharing with it. "Stop sharing" was complete for the
+// document and said nothing about the instance. (Branch review, finding 3.)
+func TestRevokingTheLastGrantEndsAdmission(t *testing.T) {
+ app := members(t, "alice@example.com")
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader")
+
+ bob := signInCookies(t, app, "bob@example.com")
+ if rec := get(t, app, bob, "/d/"+sqid); rec.Code != http.StatusOK {
+ t.Fatalf("precondition: the guest cannot read (%d)", rec.Code)
+ }
+
+ post(t, app, alice, "/d/"+sqid+"/share/revoke", "address=bob@example.com")
+
+ // The same cookies, still cryptographically valid, now reach nothing.
+ for _, path := range []string{"/", "/d/" + sqid} {
+ if rec := get(t, app, bob, path); rec.Code == http.StatusOK {
+ t.Errorf("a revoked guest still reads %s with a live session", path)
+ }
+ }
+
+ // A member is not touched by any of this: the recheck must key on
+ // admission, not on holding a grant.
+ if rec := get(t, app, alice, "/"); rec.Code != http.StatusOK {
+ t.Errorf("the owner lost access to her own list: %d", rec.Code)
+ }
+}
+
+// The wiring, end to end through the real sign-in, rather than allows()
+// and HasGrant each tested against a stand-in for the other.
+func TestAGrantAdmitsThroughTheRealSignIn(t *testing.T) {
+ app := members(t, "alice@example.com")
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+
+ // Before any grant, Bob is refused — and refused invisibly, since
+ // gateBegin answers a refused address exactly as it answers a real
+ // one so the endpoint is not a membership oracle.
+ if _, err := app.TestSignIn(t, "bob@example.com"); err == nil {
+ t.Fatal("an address on no list and holding no grant was mailed a link")
+ }
+
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader")
+
+ link, err := app.TestSignIn(t, "bob@example.com")
+ if err != nil {
+ t.Fatalf("a granted address was not mailed a link: %v", err)
+ }
+ if !strings.Contains(link, "/auth/verify") {
+ t.Fatalf("not a verify link: %q", link)
+ }
+}
diff --git a/internal/docs/handlers.go b/internal/docs/handlers.go
index b0d4285..67a7cd9 100644
--- a/internal/docs/handlers.go
+++ b/internal/docs/handlers.go
@@ -58,6 +58,23 @@ type handlers struct {
streamHeartbeat time.Duration
streamTTL time.Duration
+ // isMember answers "is this address on the configured allowlist",
+ // as opposed to admitted by a grant. It gates the member-only powers
+ // — creating a document, and changing who reaches one — so that a
+ // guest reads and writes what they were granted and nothing else
+ // (suite §1 ruling 1, Docs §8.2, branch review finding 1).
+ //
+ // A func rather than the allowlist value because handlers has no
+ // business with the rest of that type, and because app.go must be the
+ // one place the two halves of admission are wired together.
+ isMember func(address string) bool
+
+ // oneShots parks a freshly minted public URL for exactly one render
+ // (oneshot.go). Here rather than on Store for the reason hub and
+ // previews are: Store is persistence, and this value is deliberately
+ // never persisted.
+ oneShots *oneShots
+
// origin is cfg.Origin, the app's own validated base URL (app.go's
// New refuses to boot without one outside development). The share
// page needs it to print a public link somebody can copy into a
@@ -147,6 +164,16 @@ type docRow struct {
// could not be read, or one past a cold render's budget — and
// renders as the empty box the row has always shown.
Thumb thumbView
+
+ // SharedNote is set on a row the reader does not own — "Shared with
+ // you" — and empty on their own.
+ //
+ // The list used to be able to say nothing about whose a document was,
+ // because every row was the reader's; the comment on `list` said so
+ // and gave that as the reason. Sharing made it false, and a row that
+ // looks like yours and is not is the setup for opening somebody's
+ // document expecting to edit it. (Branch review, finding 9.)
+ SharedNote string
}
// owner resolves the signed-in person's Store.Person id for r —
@@ -185,6 +212,23 @@ func (h *handlers) actor(r *http.Request) (Actor, error) {
return Actor{PersonID: person, Subject: id.Address}, nil
}
+// member reports whether the person on this request is a full member of
+// the instance rather than a guest admitted by a grant.
+//
+// A guest reaching a member-only route is answered 403 rather than 404.
+// The app answers 404 for documents, and for a good reason (§4.2: "not
+// yours" must not be distinguishable from "does not exist"), but there is
+// no document here to hide and nothing secret about the instance having
+// members — the guest was invited by one. Pretending POST /documents does
+// not exist would be a lie told for no gain.
+func (h *handlers) member(r *http.Request) bool {
+ if h.isMember == nil {
+ return true
+ }
+ id, ok := rauth.From(r)
+ return ok && h.isMember(id.Address)
+}
+
// list serves GET /: the document list, spec §6.4 and Paul's four
// rulings against it — newest first, grouped by recency, one row per
// document. Cut, deliberately, not by oversight: no template
@@ -252,10 +296,11 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) {
thumb := h.thumbnailer(r.Context())
for _, d := range hits {
data.Results = append(data.Results, docRow{
- Href: "/d/" + d.Sqid,
- Title: d.Title,
- When: formatWhen(now, d.UpdatedAt),
- Thumb: thumb(d),
+ SharedNote: sharedNote(r, d, who.PersonID),
+ Href: "/d/" + d.Sqid,
+ Title: d.Title,
+ When: formatWhen(now, d.UpdatedAt),
+ Thumb: thumb(d),
})
}
if err := h.render.page(w, r, "list", data); err != nil {
@@ -271,7 +316,7 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) {
http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)
return
}
- data.Groups = groupByRecency(r, time.Now().UTC(), docs, h.thumbnailer(r.Context()))
+ data.Groups = groupByRecency(r, time.Now().UTC(), docs, h.thumbnailer(r.Context()), who.PersonID)
if err := h.render.page(w, r, "list", data); err != nil {
h.logger.Error("docs: render list", "err", err)
http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)
@@ -288,6 +333,14 @@ func (h *handlers) list(w http.ResponseWriter, r *http.Request) {
// navigation, replays the GET the redirect points at, never the POST
// that created the document.
func (h *handlers) create(w http.ResponseWriter, r *http.Request) {
+ // Member-only: a guest reads and writes what they were granted, and
+ // minting documents of their own is not that. Without this, one grant
+ // is a transitive key to the front door — the guest creates, shares,
+ // and admits the next person (branch review, finding 1).
+ if !h.member(r) {
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusForbidden)
+ return
+ }
owner, err := h.owner(r)
if err != nil {
h.logger.Error("docs: resolve signed-in person", "err", err)
@@ -368,8 +421,27 @@ type showPage struct {
// as nothing at all — so {{icon "share"}} would have shipped an
// invisible control that no test of ours would have caught. A
// thirteenth icon is a rastrillo change, not a docs one.
- CanShare bool
- ShareLabel string
+ CanShare bool
+ ShareLabel string
+
+ // CanWrite gates the editor: the ProseMirror mount point, its
+ // data-sqid, and the script that mounts it (views/show.html).
+ //
+ // A reader grantee reaches this page — GetFor admits them, and that is
+ // correct, they may read it. What was not correct was rendering them
+ // the owner's page: the bundle mounted, the toolbar enabled, and the
+ // document became contenteditable for somebody every write of whose
+ // the server refuses. The refusal worked; that is not the point. By
+ // the time the 404 arrives the person has typed a paragraph, and
+ // save-queue.mjs classifies a 4xx as permanent, drops the batch and
+ // keeps taking keystrokes. They lose the work, having been shown an
+ // editor. "It fails safely on the server" is not a defence when the
+ // cost is a person's work. (Branch review, finding 2.)
+ //
+ // So the editor is absent rather than disabled. A disabled editor is
+ // one CSS rule from being an editor again; an absent one has nothing
+ // to re-enable. views/public.html already demonstrates the shape.
+ CanWrite bool
EditorScript string // views/show.html's <script src>, from editorScriptPath() — never a literal, so it can never drift from what staticHandler actually serves
// SaveRetrying/SaveFailed are the editor's own save-status copy
@@ -479,7 +551,7 @@ func newDialogView(r *http.Request) dialogView {
// not plain text — inside a page-shaped canvas (views/show.html). An
// unknown sqid and a sqid someone else owns 404 identically — spec
// §4.2's contract that a document you cannot see is indistinguishable
-// from one that does not exist — because Store.Get's owner-scoped
+// from one that does not exist — because Store.GetFor's reader-scoped
// query never distinguishes "no such row" from "not yours" in the
// first place; there is no separate check here that could 403 by
// mistake.
@@ -520,6 +592,7 @@ func (h *handlers) show(w http.ResponseWriter, r *http.Request) {
BackLabel: rastrillo.T(r, "docs.list.title"),
Sqid: doc.Sqid,
CanShare: doc.OwnerID == actor.PersonID,
+ CanWrite: h.store.CanWrite(r.Context(), actor, doc.ID),
ShareLabel: rastrillo.T(r, "docs.share.title"),
EditorScript: editorScriptPath(),
SaveRetrying: rastrillo.T(r, "docs.editor.status.retrying"),
@@ -548,10 +621,10 @@ func (h *handlers) show(w http.ResponseWriter, r *http.Request) {
// corrupt document that looks intact.
const maxLogBytes = 8 << 20
-// getDocumentForUpdates is the owner-scoped lookup both
+// getDocumentForUpdates is the reader-scoped lookup both
// updatesGet and updatesPost share: it resolves the signed-in
// person, then loads the document through Store.Get — already
-// owner-scoped and already 404-not-403 (its own doc comment) — so a
+// reader-scoped and already 404-not-403 (its own doc comment) — so a
// stranger's sqid returns exactly the same outcome as one that does
// not exist. ok is false once a response has already been written
// (404 or 500) and the caller must return immediately without doing
@@ -590,7 +663,7 @@ func (h *handlers) getDocumentForUpdates(w http.ResponseWriter, r *http.Request)
// will not decode; the server cannot detect that corruption itself
// (it never calls into the core here), so the browser is the only
// reader that can, and raw=1 is what lets it fall back to the one
-// source it can always trust. Owner-scoped exactly like show
+// source it can always trust. Reader-scoped exactly like show
// (getDocumentForUpdates), and capped at maxLogBytes above.
//
// Why snapshot-then-tail loses nothing: a snapshot IS an ordinary Yjs
@@ -747,11 +820,16 @@ func (h *handlers) updatesPost(w http.ResponseWriter, r *http.Request) {
seq, err := h.store.ApplyUpdate(r.Context(), doc, actor, update)
if err != nil {
- // A reader posting an update gets the same 404 a stranger gets.
- // Spec §4.2: a document you may not write does not announce itself
- // as one you may not write.
+ // A reader posting an update gets the same 404 a stranger gets —
+ // the same BODY too, which it did not used to be. This branch
+ // answered http.NotFound ("404 page not found") while a stranger
+ // got http.Error with the catalog's text, so the sentence above
+ // and store.go's "refused identically" were both false. Harmless
+ // as an oracle (a reader already knows the document exists) and
+ // still worth fixing, because the comment was the thing being
+ // relied on. (Branch review, finding 7f.)
if errors.Is(err, ErrNotWritable) {
- http.NotFound(w, r)
+ http.Error(w, rastrillo.T(r, "docs.error.notfound"), http.StatusNotFound)
return
}
if updateIsClientFault(err) {
@@ -833,6 +911,16 @@ func updateIsClientFault(err error) bool {
return core.Recovered(err) || core.Rejected(err) || errors.Is(err, core.ErrUpdateLogFull)
}
+// sharedNote labels a row the reader does not own. Empty for their own
+// documents: a list where every row carries a badge is a list where none
+// of them reads as information.
+func sharedNote(r *http.Request, d *Document, mine int64) string {
+ if d.OwnerID == mine {
+ return ""
+ }
+ return rastrillo.T(r, "docs.list.shared")
+}
+
// groupByRecency buckets docs — already newest-first, per Store.List
// — into recency groups without re-sorting: consecutive documents
// that share a bucket join the same group, and a new bucket starts a
@@ -849,7 +937,7 @@ func updateIsClientFault(err error) bool {
// group labels ("Today … Yesterday … Today") the instant an older
// document got a new update. See store_test.go's
// TestListOrdersByUpdatedAt.
-func groupByRecency(r *http.Request, now time.Time, docs []*Document, thumb func(*Document) thumbView) []docGroup {
+func groupByRecency(r *http.Request, now time.Time, docs []*Document, thumb func(*Document) thumbView, mine int64) []docGroup {
var groups []docGroup
var key string
for _, d := range docs {
@@ -860,10 +948,11 @@ func groupByRecency(r *http.Request, now time.Time, docs []*Document, thumb func
}
g := &groups[len(groups)-1]
g.Rows = append(g.Rows, docRow{
- Href: "/d/" + d.Sqid,
- Title: d.Title,
- When: formatWhen(now, d.UpdatedAt),
- Thumb: thumb(d),
+ SharedNote: sharedNote(r, d, mine),
+ Href: "/d/" + d.Sqid,
+ Title: d.Title,
+ When: formatWhen(now, d.UpdatedAt),
+ Thumb: thumb(d),
})
}
return groups
diff --git a/internal/docs/links.go b/internal/docs/links.go
index 3f86f9f..d6ef71f 100644
--- a/internal/docs/links.go
+++ b/internal/docs/links.go
@@ -3,8 +3,9 @@ package docs
import (
"context"
"crypto/rand"
+ "crypto/sha256"
"encoding/base64"
- "errors"
+ "encoding/hex"
"time"
"gorm.io/gorm"
@@ -17,9 +18,13 @@ import (
// no Role field is the ruling's other half — every write has a named
// actor, and a link holds no name. See 0006_links.sql.
type Link struct {
- ID int64 `gorm:"column:id;primaryKey"`
- DocID int64 `gorm:"column:doc_id;uniqueIndex"`
- Token string `gorm:"column:token;uniqueIndex"`
+ ID int64 `gorm:"column:id;primaryKey"`
+ DocID int64 `gorm:"column:doc_id;uniqueIndex"`
+ // TokenHash, never the token (suite doctrine ruling 16). There is no
+ // field on this struct that can yield a working URL, which is the
+ // point: nothing that reads a Link out of the database — a query, a
+ // log line, a debugger — is holding a capability.
+ TokenHash string `gorm:"column:token_hash;uniqueIndex"`
CreatedAt time.Time `gorm:"column:created_at"`
}
@@ -49,6 +54,24 @@ func newToken() (string, error) {
return base64.RawURLEncoding.EncodeToString(buf), nil
}
+// hashToken is the one-way form stored in the database.
+//
+// SHA-256, unsalted, and both halves of that are deliberate. A salt
+// defends against precomputation over a small input space; there is no
+// small input space here, because the token is 128 bits from crypto/rand
+// and is its own salt. A password hash (bcrypt, argon2) would buy the same
+// nothing while making every load of a published page do key-stretching
+// work. The property that matters — the stored form does not yield the
+// token — a fast hash gives just as completely.
+//
+// Hex rather than base64 for the stored form: it is fixed-width, it is
+// what a person comparing rows by eye expects of a SHA-256, and the column
+// is never parsed by anything but this function.
+func hashToken(token string) string {
+ sum := sha256.Sum256([]byte(token))
+ return hex.EncodeToString(sum[:])
+}
+
// Publish turns a document's public link on and returns it, minting a
// fresh token every time.
//
@@ -60,17 +83,17 @@ func newToken() (string, error) {
// Owner-only through Get, so a writer grantee is answered exactly as a
// stranger is — no grant carries re-sharing, and publishing to everyone is
// the largest re-share there is.
-func (s *Store) Publish(ctx context.Context, owner int64, sqid string) (*Link, error) {
+func (s *Store) Publish(ctx context.Context, owner int64, sqid string) (string, error) {
doc, err := s.Get(ctx, owner, sqid)
if err != nil {
- return nil, err
+ return "", err
}
token, err := newToken()
if err != nil {
- return nil, err
+ return "", err
}
- link := Link{DocID: doc.ID, Token: token, CreatedAt: time.Now().UTC()}
+ link := Link{DocID: doc.ID, TokenHash: hashToken(token), CreatedAt: time.Now().UTC()}
// Delete-then-insert rather than an upsert on doc_id: the old token
// must stop working, and a row whose token changed and a row that was
// replaced are the same outcome only if nothing else ever holds a
@@ -83,9 +106,11 @@ func (s *Store) Publish(ctx context.Context, owner int64, sqid string) (*Link, e
return tx.Create(&link).Error
})
if err != nil {
- return nil, err
+ return "", err
}
- return &link, nil
+ // The plaintext token is returned here and never again: this is the
+ // only moment it exists outside the caller's hand (ruling 16).
+ return token, nil
}
// Unpublish turns it off. Like Revoke, unpublishing something that is not
@@ -99,27 +124,27 @@ func (s *Store) Unpublish(ctx context.Context, owner int64, sqid string) error {
return s.db.WithContext(ctx).Where("doc_id = ?", doc.ID).Delete(&Link{}).Error
}
-// LinkOf returns a document's live link, or nil if it is not published.
+// IsPublished reports whether a document currently has a public link.
+//
+// It answers a bool and not a link, and that is ruling 16 showing up in a
+// signature: there is no way to ask this store for a published document's
+// URL, because the store does not have it. The share page can say "this is
+// published" and offer to replace or stop it; it cannot show the URL
+// again, and no future caller can be tempted to make it.
+//
// Owner-scoped through Get: whether a document is published is part of the
// share page, which is the owner's alone.
-//
-// A missing link is (nil, nil) rather than an error, because "not
-// published" is an ordinary state of an ordinary document and the share
-// page renders it as a switch that is off.
-func (s *Store) LinkOf(ctx context.Context, owner int64, sqid string) (*Link, error) {
+func (s *Store) IsPublished(ctx context.Context, owner int64, sqid string) (bool, error) {
doc, err := s.Get(ctx, owner, sqid)
if err != nil {
- return nil, err
- }
- var link Link
- err = s.db.WithContext(ctx).Where("doc_id = ?", doc.ID).First(&link).Error
- if errors.Is(err, gorm.ErrRecordNotFound) {
- return nil, nil
+ return false, err
}
- if err != nil {
- return nil, err
+ var count int64
+ if err := s.db.WithContext(ctx).Model(&Link{}).
+ Where("doc_id = ?", doc.ID).Count(&count).Error; err != nil {
+ return false, err
}
- return &link, nil
+ return count > 0, nil
}
// ByToken resolves a public link to its document, for the one route in
@@ -141,7 +166,8 @@ func (s *Store) ByToken(ctx context.Context, token string) (*Document, error) {
var doc Document
err := s.db.WithContext(ctx).
Joins("JOIN links ON links.doc_id = documents.id").
- Where("links.token = ? AND documents.deleted_at IS NULL", token).
+ Where("links.token_hash = ? AND documents.deleted_at IS NULL AND documents.owner_id > 0",
+ hashToken(token)).
First(&doc).Error
if err != nil {
return nil, err
diff --git a/internal/docs/migrations/0005_grants.sql b/internal/docs/migrations/0005_grants.sql
index c6a684f..200481b 100644
--- a/internal/docs/migrations/0005_grants.sql
+++ b/internal/docs/migrations/0005_grants.sql
@@ -25,11 +25,22 @@
-- The UNIQUE index is the whole conflict story: granting twice to the same
-- address on the same document updates the role rather than stacking two rows
-- whose precedence nobody has decided.
+-- `created_by` is the person who made the grant, and it is in the spec's own
+-- data model (§4) — this table shipped without it, and the omission was never
+-- argued, only overlooked while the nullable `doc_id` above was argued at
+-- length. It is the one column that answers "who let this address in".
+--
+-- That question has teeth because admission is transitive in shape even where
+-- it is closed in policy: a guest is admitted by somebody's grant, and if a
+-- guest ever gains the power to grant, the chain of who admitted whom is only
+-- reconstructable if each link recorded its author. Adding it now, while the
+-- table is new, costs a line; adding it after a leak costs the history.
CREATE TABLE IF NOT EXISTS grants (
id INTEGER PRIMARY KEY AUTOINCREMENT,
doc_id INTEGER NOT NULL,
subject TEXT NOT NULL,
role TEXT NOT NULL,
+ created_by INTEGER NOT NULL DEFAULT 0,
created_at DATETIME
);
diff --git a/internal/docs/migrations/0006_links.sql b/internal/docs/migrations/0006_links.sql
index a117339..518bd1e 100644
--- a/internal/docs/migrations/0006_links.sql
+++ b/internal/docs/migrations/0006_links.sql
@@ -9,10 +9,27 @@
-- edit" is already no (Paul's own Sheets ruling 12). The column that cannot
-- exist is the cheapest way to keep that decision.
--
--- `token` is the capability itself: 128 bits from crypto/rand, base64url, and
--- never derived from doc_id, sqid, title or clock. A guessable token is the
--- whole feature failing silently, and a derived one is guessable by anybody
--- who reads this file.
+-- `token_hash` is a SHA-256 of the capability, never the capability itself
+-- (suite doctrine ruling 16, following Sheets spec §8.3). The token is 128
+-- bits from crypto/rand, base64url, never derived from doc_id, sqid, title or
+-- clock — a guessable token is the whole feature failing silently, and a
+-- derived one is guessable by anybody who reads this file.
+--
+-- Stored as a hash because a token is a STANDING KEY where a document is a
+-- snapshot. Leaked content is what it was on the day it leaked; a leaked token
+-- keeps opening the live document, every later revision, until somebody
+-- notices. Hashing turns a backup or a replica from an ongoing capability into
+-- a one-time disclosure. That argument holds here even though this app stores
+-- its content in plaintext, which is the tempting reason to skip it.
+--
+-- The consequence is that the app cannot show the URL twice. It is shown once,
+-- in the response to the publish that minted it, and after that the share page
+-- can say only whether a link exists. An owner who loses it replaces it.
+--
+-- Unsalted, deliberately: a salt defends a small input space and there is not
+-- one here — the token's own 128 bits are the salt. SHA-256 rather than a
+-- password hash for the same reason, and because a public page load should not
+-- do key-stretching work.
--
-- UNIQUE(doc_id): one live link per document, so "the link" is never an
-- ambiguous phrase in the UI or in an incident. Turning publishing off DELETEs
@@ -26,12 +43,13 @@
CREATE TABLE IF NOT EXISTS links (
id INTEGER PRIMARY KEY AUTOINCREMENT,
doc_id INTEGER NOT NULL,
- token TEXT NOT NULL,
+ token_hash TEXT NOT NULL,
created_at DATETIME
);
CREATE UNIQUE INDEX IF NOT EXISTS idx_links_doc ON links (doc_id);
-- The public read path has only a token to go on, so the lookup that has to be
--- fast — and single-row — leads on it.
-CREATE UNIQUE INDEX IF NOT EXISTS idx_links_token ON links (token);
+-- fast — and single-row — leads on its hash. Byte-equality on an indexed
+-- column is not a timing concern at 128 bits of entropy.
+CREATE UNIQUE INDEX IF NOT EXISTS idx_links_token_hash ON links (token_hash);
diff --git a/internal/docs/oneshot.go b/internal/docs/oneshot.go
new file mode 100644
index 0000000..04e4331
--- /dev/null
+++ b/internal/docs/oneshot.go
@@ -0,0 +1,84 @@
+package docs
+
+import (
+ "fmt"
+ "sync"
+ "time"
+)
+
+// oneShotTTL is how long a minted public URL waits to be collected. Long
+// enough to survive a 303 and a slow render, short enough that a URL left
+// behind by a browser that never followed the redirect is gone before
+// anybody could come back to the machine and find it.
+const oneShotTTL = 30 * time.Second
+
+// oneShots holds each freshly minted public URL for exactly one read.
+//
+// It exists because of ruling 16 and the redirect, together. The token is
+// shown once and the database cannot produce it again, so the share page
+// has to be handed it — but sharePost answers 303 (a POST that renders its
+// own page is a POST a reload re-submits, and here a re-submit would
+// rotate the link the person is in the middle of copying). So the URL has
+// to survive one redirect and no longer.
+//
+// In memory, and that is consistent rather than lazy: this app is one
+// process on one SQLite file, which is the same assumption hub and
+// previews already make. A restart between the POST and the GET loses the
+// URL; the owner republishes, which is the same recovery as losing it any
+// other way, and no worse than the alternative of writing the plaintext
+// token to the database — which is the whole thing being avoided.
+type oneShots struct {
+ mu sync.Mutex
+ items map[string]oneShot
+}
+
+type oneShot struct {
+ value string
+ expires time.Time
+}
+
+func newOneShots() *oneShots { return &oneShots{items: map[string]oneShot{}} }
+
+// key scopes a pending URL to one person and one document, so a second
+// person on the same document — or the same person on another — can never
+// collect it.
+func oneShotKey(person int64, sqid string) string {
+ return fmt.Sprintf("%d:%s", person, sqid)
+}
+
+// put stores value under key, replacing anything already there. Publishing
+// twice in a row leaves the newer URL, which is the one that works.
+func (o *oneShots) put(key, value string) {
+ o.mu.Lock()
+ defer o.mu.Unlock()
+ o.sweepLocked()
+ o.items[key] = oneShot{value: value, expires: time.Now().Add(oneShotTTL)}
+}
+
+// take returns the value and removes it. A second read returns "", which
+// is what makes this show-once rather than show-until-expiry: a reload of
+// the share page after collecting the URL shows the published state and no
+// URL, exactly as a visit tomorrow would.
+func (o *oneShots) take(key string) string {
+ o.mu.Lock()
+ defer o.mu.Unlock()
+ item, ok := o.items[key]
+ delete(o.items, key)
+ if !ok || time.Now().After(item.expires) {
+ return ""
+ }
+ return item.value
+}
+
+// sweepLocked drops expired entries. Called on put rather than on a timer:
+// the map only grows when somebody publishes, so that is the only moment
+// it can need clearing, and a background goroutine for a map this size
+// would be more machinery than the problem.
+func (o *oneShots) sweepLocked() {
+ now := time.Now()
+ for k, v := range o.items {
+ if now.After(v.expires) {
+ delete(o.items, k)
+ }
+ }
+}
diff --git a/internal/docs/publiclink_test.go b/internal/docs/publiclink_test.go
index 65d46ae..07bdb76 100644
--- a/internal/docs/publiclink_test.go
+++ b/internal/docs/publiclink_test.go
@@ -17,15 +17,22 @@ func anon(t *testing.T, app *docs.App, path string) *http.Response {
return rec.Result()
}
-// linkFor turns the link on for a document and returns its public path,
-// read back off the share page rather than out of the store — so these
-// tests exercise the same route a person would copy out of the UI.
+// linkFor publishes a document and returns its public path, collected the
+// one way the app offers it: the share page rendered immediately after the
+// publish. Under ruling 16 the database holds a SHA-256 and cannot produce
+// a URL, so there is no second chance and no store-level shortcut — the
+// test has to take the same one shot a person does.
func linkFor(t *testing.T, app *docs.App, cookies []*http.Cookie, sqid string) string {
t.Helper()
if rec := post(t, app, cookies, "/d/"+sqid+"/share/link", "on=1"); rec.Code != http.StatusSeeOther {
t.Fatalf("POST link: status %d, want 303", rec.Code)
}
- body := get(t, app, cookies, "/d/"+sqid+"/share").Body.String()
+ return readLink(t, get(t, app, cookies, "/d/"+sqid+"/share").Body.String())
+}
+
+// readLink pulls a /p/... path out of a rendered share page.
+func readLink(t *testing.T, body string) string {
+ t.Helper()
i := strings.Index(body, "/p/")
if i < 0 {
t.Fatal("the share page does not show the public link it just created")
@@ -80,8 +87,12 @@ func TestAPublicLinkCarriesNoWrite(t *testing.T) {
}
// And the public page offers no editor to drive one with.
+ //
+ // "editor-entry" used to be in this list and appears on no page in
+ // this app — a ban that could never fail. The mount element and the
+ // real served script path are what actually matter.
body := get(t, app, nil, linkFor(t, app, alice, sqid)).Body.String()
- for _, banned := range []string{"docs-editor", "editor-entry", "/updates"} {
+ for _, banned := range []string{"docs-text docs-editor", "data-sqid", docs.TestEditorScriptPath(), "/updates"} {
if strings.Contains(body, banned) {
t.Errorf("the public page ships %q; it is a reading surface", banned)
}
@@ -98,12 +109,128 @@ func TestAWrongTokenIsANotFound(t *testing.T) {
path := linkFor(t, app, alice, sqid)
for _, bad := range []string{"/p/", "/p/nope", "/p/" + strings.Repeat("a", 43), path + "x"} {
- if res := anon(t, app, bad); res.StatusCode == http.StatusOK {
- t.Errorf("GET %s answered 200", bad)
+ if res := anon(t, app, bad); res.StatusCode != http.StatusNotFound {
+ t.Errorf("GET %s answered %d, want exactly 404", bad, res.StatusCode)
}
}
}
+// The refusals are byte-identical, which is the invariant the branch
+// claims and nothing previously checked. The old test asserted only
+// "!= 200", so a 500 from a database error would have passed it while
+// telling a stranger that this token means something.
+//
+// Unknown, revoked, and pointing-at-a-deleted-document must be one answer.
+// The differences between them are exactly the oracle §4.2 exists to deny:
+// which documents were ever published, and which were withdrawn.
+func TestPublicRefusalsAreIndistinguishable(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+
+ type answer struct{ status, body, ctype string }
+ capture := func(path string) answer {
+ rec := get(t, app, nil, path)
+ return answer{
+ status: rec.Result().Status,
+ body: rec.Body.String(),
+ ctype: rec.Header().Get("Content-Type"),
+ }
+ }
+
+ // (i) a syntactically valid token that was never minted.
+ unknown := capture("/p/" + strings.Repeat("A", 22))
+
+ // (ii) a real token, revoked.
+ revokedSqid := createDoc(t, app, alice, "Revoked")
+ revokedPath := linkFor(t, app, alice, revokedSqid)
+ post(t, app, alice, "/d/"+revokedSqid+"/share/link", "on=0")
+ revoked := capture(revokedPath)
+
+ // (iii) a live token whose document has been deleted underneath it.
+ deletedSqid := createDoc(t, app, alice, "Deleted")
+ deletedPath := linkFor(t, app, alice, deletedSqid)
+ app.TestSoftDelete(t, deletedSqid)
+ deleted := capture(deletedPath)
+
+ if unknown.status != "404 Not Found" {
+ t.Fatalf("an unknown token answered %q, want 404", unknown.status)
+ }
+ for name, got := range map[string]answer{"revoked": revoked, "deleted-document": deleted} {
+ if got != unknown {
+ t.Errorf("%s answers differently from an unknown token:\n got: %+v\nwant: %+v", name, got, unknown)
+ }
+ }
+}
+
+// The published page shows the document's actual content.
+//
+// Every other test here publishes an EMPTY document, so if ByToken's join
+// ever scanned links.id into Document.ID the wrong document would be
+// replayed and every one of them would still pass with a 200. GORM emits
+// SELECT documents.* here today; nothing held it.
+func TestAPublicLinkShowsTheDocumentsContent(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ app.TestWrite(t, alice, sqid, "hello")
+
+ body := get(t, app, nil, linkFor(t, app, alice, sqid)).Body.String()
+ if !strings.Contains(body, "hello") {
+ t.Error("the published page does not contain the document's text")
+ }
+ if strings.Contains(body, sqid) {
+ t.Error("the published page names the private sqid")
+ }
+}
+
+// A published URL is shown once and never again — ruling 16 as a
+// behaviour rather than as a column type. Reloading the share page after
+// collecting it must show the document as published WITHOUT the URL.
+func TestThePublicURLIsShownExactlyOnce(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+
+ path := linkFor(t, app, alice, sqid) // consumes the one showing
+
+ again := get(t, app, alice, "/d/"+sqid+"/share").Body.String()
+ if strings.Contains(again, path) {
+ t.Error("the share page showed the public URL a second time")
+ }
+ if strings.Contains(again, "/p/") {
+ t.Error("the share page still prints a /p/ URL after the one showing")
+ }
+ // But it still says the document is published, or the owner cannot
+ // tell a published document from an unpublished one.
+ //
+ // The catalog key, not the word: NewForTest wires no BaseCatalog, so
+ // strings render as their keys — and "published" matched the key
+ // "docs.share.public.published" whatever the page actually said,
+ // which made this assertion unfalsifiable.
+ if !strings.Contains(again, "docs.share.public.published") {
+ t.Error("the share page does not say the document is published")
+ }
+ // And the link itself still works — shown once, not valid once.
+ if res := anon(t, app, path); res.StatusCode != http.StatusOK {
+ t.Errorf("the link stopped working after being shown: %d", res.StatusCode)
+ }
+}
+
+// Nothing anywhere stores the plaintext token. The point of ruling 16,
+// asserted against the database rather than against the code that writes
+// to it.
+func TestTheTokenIsNotStoredInPlaintext(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ path := linkFor(t, app, alice, sqid)
+ token := strings.TrimPrefix(path, "/p/")
+
+ if app.TestRowContains(t, "links", token) {
+ t.Error("the plaintext token is in the links table")
+ }
+}
+
// Turning it off is the revoke, and it has to be immediate: a link that
// keeps working after the owner switched it off is the failure that makes
// people distrust the whole control.
@@ -172,8 +299,15 @@ func TestOnlyTheOwnerMayPublish(t *testing.T) {
bob := signInCookies(t, app, "bob@example.com")
post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer")
- if rec := post(t, app, bob, "/d/"+sqid+"/share/link", "on=1"); rec.Code != http.StatusNotFound {
- t.Errorf("a writer grantee published the document: status %d, want 404", rec.Code)
+ for _, c := range []struct{ path, body string }{
+ {"/d/" + sqid + "/share/link", "on=1"},
+ {"/d/" + sqid + "/share/link", "on=0"},
+ {"/d/" + sqid + "/share/revoke", "address=alice@example.com"},
+ {"/d/" + sqid + "/share", "address=eve@example.com&role=reader"},
+ } {
+ if rec := post(t, app, bob, c.path, c.body); rec.Code != http.StatusNotFound {
+ t.Errorf("a writer grantee reached POST %s (%s): status %d, want 404", c.path, c.body, rec.Code)
+ }
}
}
diff --git a/internal/docs/publish.go b/internal/docs/publish.go
index 9a3e2dc..48562dd 100644
--- a/internal/docs/publish.go
+++ b/internal/docs/publish.go
@@ -61,11 +61,20 @@ func (h *handlers) publicShow(w http.ResponseWriter, r *http.Request) {
return
}
- // No cache header beyond the default: a published document is meant
- // to be read, and a token in the path already makes the URL its own
- // cache key. What must not happen is a shared cache keeping a copy
- // after the link is revoked, which is why nothing here marks it
- // public or sets a max-age.
+ // no-store, said rather than implied. The first version of this
+ // handler set no header at all and its comment claimed that kept a
+ // shared cache from retaining a revoked copy — which is not what an
+ // absent Cache-Control means. Absent leaves it to heuristics; a
+ // revoked link's page could sit in an intermediary. stream.go and
+ // search.go already say no-store for exactly this reason.
+ //
+ // X-Robots-Tag because "anyone with the link" is not "anyone": a
+ // capability that leaks into a crawlable place should not also become
+ // a search result. It is not a security control — the link is the
+ // authorization and a crawler that has it can read the page — it is a
+ // refusal to help the leak spread.
+ w.Header().Set("Cache-Control", "no-store")
+ w.Header().Set("X-Robots-Tag", "noindex, nofollow")
if err := h.render.page(w, r, "public", publicPage{
Title: doc.Title,
HTML: html,
@@ -87,6 +96,12 @@ func (h *handlers) publicShow(w http.ResponseWriter, r *http.Request) {
// answered 404: publishing to everyone is the largest re-share there is,
// and no grant carries re-sharing.
func (h *handlers) shareLink(w http.ResponseWriter, r *http.Request) {
+ // Member-only, as the other two share routes are. Publishing to the
+ // whole internet is the largest re-share there is.
+ if !h.member(r) {
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusForbidden)
+ return
+ }
owner, err := h.owner(r)
if err != nil {
h.logger.Error("docs: resolve owner for publish", "err", err)
@@ -97,7 +112,14 @@ func (h *handlers) shareLink(w http.ResponseWriter, r *http.Request) {
on := r.PostFormValue("on") == "1"
if on {
- _, err = h.store.Publish(r.Context(), owner, sqid)
+ var token string
+ token, err = h.store.Publish(r.Context(), owner, sqid)
+ if err == nil {
+ // The only moment this URL exists outside the token itself.
+ // Parked for exactly one render (oneshot.go) and never
+ // written down, because the database holds only a hash.
+ h.oneShots.put(oneShotKey(owner, sqid), h.origin+"/p/"+token)
+ }
} else {
err = h.store.Unpublish(r.Context(), owner, sqid)
}
diff --git a/internal/docs/reader_noeditor_test.go b/internal/docs/reader_noeditor_test.go
new file mode 100644
index 0000000..f39abe7
--- /dev/null
+++ b/internal/docs/reader_noeditor_test.go
@@ -0,0 +1,81 @@
+package docs_test
+
+import (
+ "net/http"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/docs/internal/docs"
+)
+
+// A reader grantee must not be handed an editor. Found by the branch
+// review, and the worst defect on this branch: `show` used GetFor
+// (correct — a reader may read) and then rendered the owner's page
+// verbatim, so ProseMirror mounted, the toolbar enabled, and the document
+// became contenteditable for somebody the server will refuse.
+//
+// The server refusal was never in doubt: ApplyUpdate returns ErrNotWritable
+// and updatesPost answers 404. The damage is upstream of that. The browser
+// has already taken the keystrokes; the save queue classifies the 404 as
+// permanent, drops the batch, and keeps accepting typing. The person is
+// told "save failed" after writing a paragraph into a page that told them
+// they could.
+//
+// "It fails safely on the server" is not a defence when the cost is a
+// person's work.
+func TestAReaderIsNotGivenAnEditor(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ bob := signInCookies(t, app, "bob@example.com")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader")
+
+ rec := get(t, app, bob, "/d/"+sqid)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("GET /d/%s as a reader: status %d, want 200", sqid, rec.Code)
+ }
+ body := rec.Body.String()
+
+ // The mount ELEMENT, not the string: `[docs-editor]` also appears a
+ // dozen times in this page's own <style> block, so banning the bare
+ // word would fail on a page that is perfectly correct — and would
+ // have hidden whether the element itself was gone. `docs-text
+ // docs-editor` is the attribute pair the mount point carries and the
+ // stylesheet never does.
+ //
+ // data-sqid is the bundle's only way to learn where to post, and the
+ // editor script is checked by its real served path rather than a
+ // guessed substring: the review found this test's earlier sibling
+ // banning "editor-entry", a string that appears on no page in this
+ // app, which is a ban that could never fail.
+ for _, banned := range []string{"docs-text docs-editor", "data-sqid", docs.TestEditorScriptPath()} {
+ if strings.Contains(body, banned) {
+ t.Errorf("a reader's document page ships %q", banned)
+ }
+ }
+
+ // The content is still there. A read-only page that renders nothing is
+ // not the fix.
+ if !strings.Contains(body, "Minutes") {
+ t.Error("the reader's page does not show the document")
+ }
+}
+
+// The writer keeps everything. Without this the fix above passes on a
+// page that gives nobody an editor.
+func TestAWriterKeepsTheEditor(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ bob := signInCookies(t, app, "bob@example.com")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer")
+
+ for who, cookies := range map[string][]*http.Cookie{"writer": bob, "owner": alice} {
+ body := get(t, app, cookies, "/d/"+sqid).Body.String()
+ for _, needed := range []string{"docs-text docs-editor", "data-sqid", docs.TestEditorScriptPath()} {
+ if !strings.Contains(body, needed) {
+ t.Errorf("the %s's page is missing %q", who, needed)
+ }
+ }
+ }
+}
diff --git a/internal/docs/render.go b/internal/docs/render.go
index fc96104..a581b5e 100644
--- a/internal/docs/render.go
+++ b/internal/docs/render.go
@@ -94,7 +94,19 @@ var Catalog = rastrillo.Catalog{
"docs.share.public.body": "Publish a link that anyone can open to read this document. They will not be able to change it, and they will not need to sign in.",
"docs.share.public.on": "Publish a link",
"docs.share.public.off": "Stop publishing",
- "docs.share.public.hint": "Copy this link to share it. Publishing again later makes a new link, and this one stops working.",
+ "docs.share.public.hint": "Publishing again makes a new link, and the old one stops working.",
+
+ // docs.share.public.once is the one-time notice, and the copy has to
+ // carry a real instruction because the app genuinely cannot repeat
+ // itself: it stores a hash of the token, not the token (suite ruling
+ // 16). "You will not see it again" is a fact here, not a scare.
+ //
+ // docs.share.public.published is what the section says on every later
+ // visit — published, without the URL, which is the shape of the whole
+ // ruling in one line of copy.
+ "docs.share.public.once": "Copy this link now. This is the only time it will be shown.",
+ "docs.share.public.published": "This document is published. The link was shown once when it was created and cannot be shown again — replace it if you no longer have it.",
+ "docs.share.public.replace": "Replace the link",
// docs.public.readonly is the standing notice on the published page
// itself (views/public.html). It is there because a person arriving
@@ -222,6 +234,12 @@ var Catalog = rastrillo.Catalog{
// bar's account menu, which list.html and show.html each fill with
// an "account" block; it was in their two page headers until the
// chrome moved to the topbar shell (see shellLayout above).
+ // docs.list.shared marks a row on the list that belongs to somebody
+ // else. It exists because the list stopped being "your documents" the
+ // day sharing landed, and a row that looks like yours and is not is
+ // how a person opens a document expecting to be able to edit it.
+ "docs.list.shared": "Shared with you",
+
"docs.signout.label": "Sign out",
// docs.editor.status.* is the save-status indicator fix round 1's
diff --git a/internal/docs/search.go b/internal/docs/search.go
index e06ca09..f1a0f83 100644
--- a/internal/docs/search.go
+++ b/internal/docs/search.go
@@ -204,7 +204,7 @@ type searchResult struct {
// cannot drift from the one the list page uses (the plan's D3). It also
// means no bulk export of a person's titles to the browser.
//
-// Inside app.go's RequireSession group and owner-scoped through
+// Inside app.go's RequireSession group and reader-scoped through
// Search → List, so a document belonging to someone else cannot appear
// here — the same property every other document route has, by the same
// mechanism rather than by a check of its own.
diff --git a/internal/docs/share.go b/internal/docs/share.go
index a4d8a93..7abc56e 100644
--- a/internal/docs/share.go
+++ b/internal/docs/share.go
@@ -32,10 +32,23 @@ type sharePage struct {
// could disagree with it.
PublicHeading string
PublicBody string
+
+ // Published is whether a link exists; PublicURL is the link itself and
+ // is set only in the render immediately after publishing.
+ //
+ // Two fields rather than one because they are two different facts now:
+ // under ruling 16 the app knows a document is published without being
+ // able to say what its URL is. Collapsing them would make "published"
+ // unrepresentable whenever the URL is unavailable — which is almost
+ // always.
+ Published bool
PublicURL string
PublishButton string
UnpublishBtn string
+ ReplaceButton string
CopyHint string
+ OnceWarning string
+ PublishedBody string
}
// shareRow is one person who reaches this document.
@@ -76,17 +89,21 @@ func (h *handlers) shareGet(w http.ResponseWriter, r *http.Request) {
}
func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Document, grants []*Grant, errKey string) {
- // A failure to read the link is logged and then rendered as "not
- // published" rather than failing the page: the grant list above it is
- // the part somebody came here for, and a 500 over the switch would
+ // A failure to read the published state is logged and then rendered as
+ // "not published" rather than failing the page: the grant list above it
+ // is the part somebody came here for, and a 500 over the switch would
// take the working half of the screen down with it.
- var publicURL string
- if link, err := h.store.LinkOf(r.Context(), doc.OwnerID, doc.Sqid); err != nil {
- h.logger.Error("docs: read public link", "sqid", doc.Sqid, "err", err)
- } else if link != nil {
- publicURL = h.origin + "/p/" + link.Token
+ published, err := h.store.IsPublished(r.Context(), doc.OwnerID, doc.Sqid)
+ if err != nil {
+ h.logger.Error("docs: read published state", "sqid", doc.Sqid, "err", err)
}
+ // The URL appears only in the render that follows the publish that
+ // minted it (ruling 16: the database holds a hash and cannot produce a
+ // URL). Collected here, once; a reload shows the published state
+ // without it, exactly as a visit tomorrow would.
+ publicURL := h.oneShots.take(oneShotKey(doc.OwnerID, doc.Sqid))
+
rows := make([]shareRow, 0, len(grants))
for _, g := range grants {
label := "docs.share.role.reader"
@@ -114,16 +131,23 @@ func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Docu
PublicHeading: rastrillo.T(r, "docs.share.public.heading"),
PublicBody: rastrillo.T(r, "docs.share.public.body"),
+ Published: published,
PublicURL: publicURL,
PublishButton: rastrillo.T(r, "docs.share.public.on"),
UnpublishBtn: rastrillo.T(r, "docs.share.public.off"),
+ ReplaceButton: rastrillo.T(r, "docs.share.public.replace"),
CopyHint: rastrillo.T(r, "docs.share.public.hint"),
+ OnceWarning: rastrillo.T(r, "docs.share.public.once"),
+ PublishedBody: rastrillo.T(r, "docs.share.public.published"),
}
if errKey != "" {
page.Error = rastrillo.T(r, errKey)
}
- h.render.page(w, r, "share", page)
+ if err := h.render.page(w, r, "share", page); err != nil {
+ h.logger.Error("docs: render share", "sqid", doc.Sqid, "err", err)
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)
+ }
}
// sharePost serves POST /d/{sqid}/share: grant an address access.
@@ -131,6 +155,14 @@ func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Docu
// Scriptless: a plain form with a named address field and a role radio, and a
// 303 back to this page afterwards so a reload does not re-post.
func (h *handlers) sharePost(w http.ResponseWriter, r *http.Request) {
+ // Member-only, on top of the owner check below: changing who reaches
+ // a document is a member power. A guest owns no documents (create is
+ // closed to them), so this is belt and braces — and it is the brace
+ // that holds if a guest ever acquires one.
+ if !h.member(r) {
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusForbidden)
+ return
+ }
owner, err := h.owner(r)
if err != nil {
h.logger.Error("docs: resolve owner for share", "err", err)
@@ -163,6 +195,10 @@ func (h *handlers) sharePost(w http.ResponseWriter, r *http.Request) {
http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusInternalServerError)
return
}
+ // Content-Type before WriteHeader, or the header render.page
+ // sets afterwards is dropped and the response works only by
+ // sniffing.
+ w.Header().Set("Content-Type", "text/html; charset=utf-8")
w.WriteHeader(http.StatusBadRequest)
h.renderShare(w, r, doc, grants, "docs.share.error.address")
default:
@@ -173,6 +209,14 @@ func (h *handlers) sharePost(w http.ResponseWriter, r *http.Request) {
// shareRevoke serves POST /d/{sqid}/share/revoke.
func (h *handlers) shareRevoke(w http.ResponseWriter, r *http.Request) {
+ // Member-only, on top of the owner check below: changing who reaches
+ // a document is a member power. A guest owns no documents (create is
+ // closed to them), so this is belt and braces — and it is the brace
+ // that holds if a guest ever acquires one.
+ if !h.member(r) {
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusForbidden)
+ return
+ }
owner, err := h.owner(r)
if err != nil {
h.logger.Error("docs: resolve owner for revoke", "err", err)
diff --git a/internal/docs/shared_list_test.go b/internal/docs/shared_list_test.go
index 2813afa..7ad59c4 100644
--- a/internal/docs/shared_list_test.go
+++ b/internal/docs/shared_list_test.go
@@ -50,6 +50,46 @@ func TestASharedDocumentAppearsInTheGranteesList(t *testing.T) {
}
}
+// A shared row says it is shared, and the reader's own rows do not.
+//
+// The list stopped being "your documents" the day sharing landed. A row
+// that looks like yours and is not is how somebody opens a document
+// expecting to edit it — and with a reader grant they now get a page with
+// no editor, which is correct and needs explaining before the click, not
+// after it. (Branch review, finding 9.)
+func TestASharedRowSaysSo(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Alices")
+ bob := signInCookies(t, app, "bob@example.com")
+ createDoc(t, app, bob, "Bobs")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=reader")
+
+ // The catalog KEY, not the English. NewForTest's app never wires
+ // rastrillo.Options.BaseCatalog (only cmd/docs/main.go does), so every
+ // catalog string renders as its own key here. Asserting the prose
+ // would be asserting a string that never appears — and, worse, a
+ // prose fragment can accidentally match a key: a nearby test checked
+ // for "published" and passed on the key "docs.share.public.published"
+ // rather than on anything being published.
+ const marker = "docs.list.shared"
+
+ body := get(t, app, bob, "/").Body.String()
+ if !strings.Contains(body, marker) {
+ t.Error("the shared row carries no marker")
+ }
+ // Exactly one row is marked: Bob's own must not be. A list where
+ // every row is badged says nothing.
+ if n := strings.Count(body, marker); n != 1 {
+ t.Errorf("%d rows marked shared, want exactly 1", n)
+ }
+
+ // And Alice, who owns hers, sees no marker at all.
+ if strings.Contains(get(t, app, alice, "/").Body.String(), marker) {
+ t.Error("the owner's own document is marked as shared with her")
+ }
+}
+
// Revoking takes it back out of the list, not just out of the document. A
// revoke that left the row on the grantee's list would leave them clicking
// into a 404 and reading it as a bug.
diff --git a/internal/docs/store.go b/internal/docs/store.go
index 52cba00..5ef83f9 100644
--- a/internal/docs/store.go
+++ b/internal/docs/store.go
@@ -29,6 +29,20 @@ type Store struct {
// gate lives here rather than at the HTTP layer.
sampleDocuments bool
+ // seedOnlyMembers, when set, keeps the sample library away from a
+ // person admitted only by a grant.
+ //
+ // The sample documents are "welcome to your instance" content. Somebody
+ // invited to read one document is not arriving at their instance; they
+ // are arriving at somebody else's, to read one thing. Seeding them six
+ // documents of ours is the outcome 0005_grants.sql's comment says the
+ // design avoids — it just avoided it at grant time and then did it at
+ // sign-in time instead. (Branch review, finding 9.)
+ //
+ // nil means "seed everybody", which is what every construction without
+ // an allowlist gets, and matches sampleDocuments' own default shape.
+ seedOnlyMembers func(address string) bool
+
// logger is where a sample-seeding failure goes — see
// seedIfEnabled's own doc comment for why that failure is logged
// and swallowed here rather than ever returned to EnsurePerson's
@@ -242,6 +256,9 @@ func (s *Store) seedIfEnabled(ctx context.Context, newOwner int64, email string)
if !s.sampleDocuments {
return
}
+ if s.seedOnlyMembers != nil && !s.seedOnlyMembers(email) {
+ return
+ }
if err := Seed(ctx, s, Actor{PersonID: newOwner, Subject: email}); err != nil {
s.logger.Error("docs: seed sample documents for new person", "person", newOwner, "address", email, "err", err)
}
@@ -693,7 +710,7 @@ func (s *Store) currentSeq(ctx context.Context, docID int64) (int64, error) {
// is to refuse the entire operation before any of them run. doc is a
// value a caller can construct or mutate freely (as the regression
// test below does); it is not a database guarantee. Store.Get is
-// owner-scoped, so doc.OwnerID is trustworthy *when doc came from
+// reader-scoped, so doc.OwnerID is trustworthy *when doc came from
// Get* — but nothing at this call site enforces that it did, which is
// the entire defect. So rather than trust the struct, this re-derives
// ownership from the database itself: look up document doc.ID through
@@ -724,7 +741,7 @@ func (s *Store) ApplyUpdate(ctx context.Context, doc *Document, actor Actor, upd
// This check used to be owned(db, doc.OwnerID).Where("id = ?", doc.ID) —
// the document against its own owner, which is true of every document by
// construction. That was harmless only because the sole way to hold a
- // *Document was to have fetched it through an owner-scoped query, so this
+ // *Document was to have fetched it through a scoped query, so this
// was a belt on top of the read scope's braces. Sharing removes that
// guarantee: a reader can now legitimately fetch a document, and the old
// check would have waved their write through, because it never asked
diff --git a/internal/docs/stream.go b/internal/docs/stream.go
index 35ce48e..bd73b35 100644
--- a/internal/docs/stream.go
+++ b/internal/docs/stream.go
@@ -55,7 +55,7 @@ const (
// opaque binary Yjs updates on it would have meant base64 and about a
// third more bytes on every keystroke batch.
//
-// Owner-scoped through getDocumentForUpdates, so a stranger gets the
+// Reader-scoped through getDocumentForUpdates, so a stranger gets the
// same 404 the update routes give — never a 403, which would confirm
// the document exists.
func (h *handlers) stream(w http.ResponseWriter, r *http.Request) {
diff --git a/internal/docs/testing.go b/internal/docs/testing.go
index 73b4395..29562f2 100644
--- a/internal/docs/testing.go
+++ b/internal/docs/testing.go
@@ -2,6 +2,7 @@ package docs
import (
"context"
+ "database/sql"
"fmt"
"io"
"net/http"
@@ -11,6 +12,7 @@ import (
"path/filepath"
"strings"
"testing"
+ "time"
)
// NewForTest builds an App against a temporary SQLite database. It is
@@ -26,12 +28,34 @@ import (
// sign-in landed and was deleted in fix round 1 on Task 1 — every
// test built on NewForTest signs in for real via TestSignIn now.
func NewForTest(t *testing.T) (*App, error) {
+ return NewForTestWith(t, Config{})
+}
+
+// NewForTestWith is NewForTest with the caller's own Config, for the tests
+// that need a real allowlist.
+//
+// It exists because NewForTest fixes an empty AllowedAddresses, which in
+// development means "admit everyone" (resolveAllowlist) — so the whole
+// grant-admission path, and the guest/member distinction built on it, were
+// unreachable from any test in the suite. The branch review found exactly
+// that hole: allows() and HasGrant were each tested in isolation, and the
+// wiring that makes them one thing was not, so moving one line in app.go
+// would have silently recreated the unredeemable invitation its comment
+// warns about, green. (Branch review, finding 7e.)
+//
+// DBPath and Env are filled in when the caller leaves them empty, so a test
+// that only wants an allowlist writes only an allowlist.
+func NewForTestWith(t *testing.T, cfg Config) (*App, error) {
t.Helper()
- app, err := New(Config{
- DBPath: filepath.Join(t.TempDir(), "docs.db"),
- Env: "development",
- })
+ if cfg.DBPath == "" {
+ cfg.DBPath = filepath.Join(t.TempDir(), "docs.db")
+ }
+ if cfg.Env == "" {
+ cfg.Env = "development"
+ }
+
+ app, err := New(cfg)
if err != nil {
return nil, err
}
@@ -66,6 +90,12 @@ func (a *App) TestSignIn(t *testing.T, address string) (string, error) {
return "", fmt.Errorf("docs: TestSignIn requires Env %q; app was built with Env %q", "development", a.cfg.Env)
}
+ // Snapshot the send count before the POST: the check below has to see
+ // a NEW mail, not merely a recorder whose last occupant matches.
+ a.mailRecorder.mu.Lock()
+ before := a.mailRecorder.sends
+ a.mailRecorder.mu.Unlock()
+
form := url.Values{"address": {address}, "force": {"1"}}
req := httptest.NewRequest(http.MethodPost, "/signin", strings.NewReader(form.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
@@ -88,9 +118,22 @@ func (a *App) TestSignIn(t *testing.T, address string) (string, error) {
}
a.mailRecorder.mu.Lock()
- body := a.mailRecorder.lastBody
+ to, body, after := a.mailRecorder.lastTo, a.mailRecorder.lastBody, a.mailRecorder.sends
a.mailRecorder.mu.Unlock()
+ // A refused address gets the same 303 ?sent=1 a real one gets
+ // (gateBegin, deliberately, so the endpoint is not a membership
+ // oracle). So the only evidence that this address was admitted is that
+ // a new mail went out, to it. Both halves are needed: the count alone
+ // would accept somebody else's mail, and the recipient alone stays
+ // true for every later refusal of an address that once succeeded.
+ if after == before {
+ return "", fmt.Errorf("docs: TestSignIn: no mail was sent for %q — the address was refused", address)
+ }
+ if normalizeAddress(to) != normalizeAddress(address) {
+ return "", fmt.Errorf("docs: TestSignIn: the mail went to %q, not %q", to, address)
+ }
+
link := magicLinkPattern.FindString(body)
if link == "" {
return "", fmt.Errorf("docs: TestSignIn: no magic link found in the mailed body for %q", address)
@@ -238,3 +281,78 @@ func (a *App) TestActorForPerson(t *testing.T, person int64) Actor {
}
return Actor{PersonID: p.ID, Subject: p.Email}
}
+
+// TestEditorScriptPath is the served path of the editor bundle, exported so
+// an external test can assert a page does or does not ship it.
+//
+// It exists because the branch review found a test banning the string
+// "editor-entry" from the public page — a string that appears on no page in
+// this app, so the ban could never fail. A test that names the real path
+// cannot drift from what staticHandler actually serves, because it is the
+// same function that builds the <script src>.
+func TestEditorScriptPath() string { return editorScriptPath() }
+
+// TestSoftDelete marks a document deleted the way a delete verb would, for
+// tests that need a live link pointing at a document that is no longer
+// there. There is no delete route yet; this is the state the schema
+// already models (documents.deleted_at) and that ByToken filters on.
+func (a *App) TestSoftDelete(t *testing.T, sqid string) {
+ t.Helper()
+ err := a.store.db.Model(&Document{}).
+ Where("sqid = ?", sqid).
+ Update("deleted_at", time.Now().UTC()).Error
+ if err != nil {
+ t.Fatalf("docs: TestSoftDelete(%q): %v", sqid, err)
+ }
+}
+
+// TestWrite applies one update to a document as the signed-in owner,
+// through the same store path a real edit takes, so a test can publish a
+// document that actually has text in it.
+func (a *App) TestWrite(t *testing.T, cookies []*http.Cookie, sqid, fixture string) {
+ t.Helper()
+ var doc Document
+ if err := a.store.db.Where("sqid = ?", sqid).First(&doc).Error; err != nil {
+ t.Fatalf("docs: TestWrite: find %q: %v", sqid, err)
+ }
+ actor := a.TestActorFor(t, &doc)
+ if _, err := a.store.ApplyUpdate(context.Background(), &doc, actor, ReadFragmentFixture(t, fixture)); err != nil {
+ t.Fatalf("docs: TestWrite(%q): %v", fixture, err)
+ }
+}
+
+// TestRowContains reports whether any column of any row in table holds
+// needle as a substring.
+//
+// It exists for the one assertion that cannot be made against behaviour:
+// ruling 16 says the plaintext token is not in the database, and the only
+// honest way to check that is to look in the database. A test that asked
+// the store instead would be asking the code that is supposed to be wrong.
+func (a *App) TestRowContains(t *testing.T, table, needle string) bool {
+ t.Helper()
+ rows, err := a.store.db.Raw("SELECT * FROM " + table).Rows()
+ if err != nil {
+ t.Fatalf("docs: TestRowContains: %v", err)
+ }
+ defer rows.Close()
+
+ cols, err := rows.Columns()
+ if err != nil {
+ t.Fatalf("docs: TestRowContains: columns: %v", err)
+ }
+ for rows.Next() {
+ cells := make([]any, len(cols))
+ for i := range cells {
+ cells[i] = new(sql.RawBytes)
+ }
+ if err := rows.Scan(cells...); err != nil {
+ t.Fatalf("docs: TestRowContains: scan: %v", err)
+ }
+ for _, c := range cells {
+ if strings.Contains(string(*(c.(*sql.RawBytes))), needle) {
+ return true
+ }
+ }
+ }
+ return false
+}
diff --git a/internal/docs/views/list.html b/internal/docs/views/list.html
index 27806b2..fe99b7b 100644
--- a/internal/docs/views/list.html
+++ b/internal/docs/views/list.html
@@ -328,7 +328,7 @@
<div rst-row>
{{template "thumb" .Thumb}}
<span rst-row-main><a href="{{.Href}}">{{.Title}}</a></span>
- <span docs-row-when>{{.When}}</span>
+ <span docs-row-when>{{.When}}</span>{{if .SharedNote}}<span rst-muted docs-row-shared>{{.SharedNote}}</span>{{end}}
</div>
{{end}}
</div>
@@ -347,7 +347,7 @@
<div rst-row>
{{template "thumb" .Thumb}}
<span rst-row-main><a href="{{.Href}}">{{.Title}}</a></span>
- <span docs-row-when>{{.When}}</span>
+ <span docs-row-when>{{.When}}</span>{{if .SharedNote}}<span rst-muted docs-row-shared>{{.SharedNote}}</span>{{end}}
</div>
{{end}}
{{end}}
diff --git a/internal/docs/views/public.html b/internal/docs/views/public.html
index 2d28f4d..c85172b 100644
--- a/internal/docs/views/public.html
+++ b/internal/docs/views/public.html
@@ -1,3 +1,4 @@
+{{define "title"}}{{.Title}}{{end}}
{{/* The published document: the one screen in this app a stranger sees.
No "account" block is defined, and that is deliberate rather than an
diff --git a/internal/docs/views/share.html b/internal/docs/views/share.html
index 2a42a32..86f0545 100644
--- a/internal/docs/views/share.html
+++ b/internal/docs/views/share.html
@@ -1,3 +1,4 @@
+{{define "title"}}{{.Title}} — {{.DocTitle}}{{end}}
{{define "account"}}<form method="post" action="/signout"><button type="submit">{{.SignOutLabel}}</button></form>{{end}}
{{/* The share screen. Scriptless throughout, per spec §6.2, which lists
@@ -85,13 +86,26 @@
<h2>{{.PublicHeading}}</h2>
<p rst-muted>{{.PublicBody}}</p>
+ {{if .Published}}
+ {{/* The URL appears here only in the render that follows the publish
+ that minted it — the database holds a SHA-256 and cannot produce
+ a URL (suite ruling 16). Afterwards this section says the
+ document is published and offers to replace or stop it. */}}
{{if .PublicURL}}
+ <p rst-notice docs-share-once role="status">{{.OnceWarning}}</p>
<p docs-share-url><code>{{.PublicURL}}</code></p>
+ {{else}}
+ <p rst-muted docs-share-state>{{.PublishedBody}}</p>
+ {{end}}
<p rst-muted>{{.CopyHint}}</p>
<form method="post" action="/d/{{.Sqid}}/share/link">
<input type="hidden" name="on" value="0">
<button type="submit">{{.UnpublishBtn}}</button>
</form>
+ <form method="post" action="/d/{{.Sqid}}/share/link">
+ <input type="hidden" name="on" value="1">
+ <button type="submit">{{.ReplaceButton}}</button>
+ </form>
{{else}}
<form method="post" action="/d/{{.Sqid}}/share/link">
<input type="hidden" name="on" value="1">
diff --git a/internal/docs/views/show.html b/internal/docs/views/show.html
index ccc4b71..b0735bd 100644
--- a/internal/docs/views/show.html
+++ b/internal/docs/views/show.html
@@ -1324,7 +1324,7 @@
controls, so it was the one place a person would not look. They are
still ordinary links to ordinary URLs, so both still work with no
JavaScript at all — spec §6.2 — and both are GETs of a document the
- signed-in person already owns, so neither needs a form or a CSRF
+ signed-in person may already read, so neither needs a form or a CSRF
token. download tells the browser to save rather than navigate; the
filename it uses comes from the server's Content-Disposition, not
from this attribute, because a title is arbitrary text a person
@@ -1538,7 +1538,13 @@
TestShowRendersContentWithoutScript pins it. data-sqid is the
bundle's only way to learn which document's update log
(/d/{{.Sqid}}/updates) to load and post to. */}}
- <div docs-text docs-editor data-sqid="{{.Sqid}}">{{.HTML}}</div>
+ {{/* A reader gets the same container without the mount point, the
+ data-sqid or (below) the script — absent, not disabled, because a
+ disabled editor is one CSS rule from being an editor again. The
+ server-rendered HTML is identical either way, so this is the
+ scriptless read view the whole page already falls back to. */}}
+ {{if .CanWrite}}<div docs-text docs-editor data-sqid="{{.Sqid}}">{{.HTML}}</div>
+ {{else}}<div docs-text>{{.HTML}}</div>{{end}}
</div>
</div>
{{/* [docs-save-status] (fix round 1, F2): a sibling of [docs-editor],
@@ -1603,6 +1609,6 @@
RequireSession group) and, if it throws, editor-entry.js's own catch
marks [docs-editor] data-editor-failed rather than clearing it — the
read view above stays exactly as rendered. */}}
-<script type="module" defer src="{{.EditorScript}}"></script>
+{{if .CanWrite}}<script type="module" defer src="{{.EditorScript}}"></script>{{end}}
{{template "docs-jumpto" .}}
{{end}}
diff --git a/internal/docs/writer_http_test.go b/internal/docs/writer_http_test.go
new file mode 100644
index 0000000..6658ec4
--- /dev/null
+++ b/internal/docs/writer_http_test.go
@@ -0,0 +1,61 @@
+package docs_test
+
+import (
+ "bytes"
+ "net/http"
+ "net/http/httptest"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/docs/internal/docs"
+)
+
+// A writer grantee can actually write, over HTTP, and the owner sees it.
+//
+// The branch tested the writer's authorization at the store
+// (grants_write_test.go asserts "not ErrNotWritable" for a one-byte
+// update) and the reader's refusal at the route, and never the writer's
+// success at the route. So if updatesPost regressed to owner-only Get,
+// every existing test would still pass while collaborative editing — the
+// point of a writer grant — was silently dead. (Branch review, finding
+// 7d.)
+func TestAWriterCanPostUpdatesOverHTTP(t *testing.T) {
+ app, _ := docs.NewForTest(t)
+ alice := signInCookies(t, app, "alice@example.com")
+ sqid := createDoc(t, app, alice, "Minutes")
+ bob := signInCookies(t, app, "bob@example.com")
+ post(t, app, alice, "/d/"+sqid+"/share", "address=bob@example.com&role=writer")
+
+ rec := postBytes(t, app, bob, "/d/"+sqid+"/updates", docs.ReadFragmentFixture(t, "hello"))
+ if rec.Code != http.StatusNoContent {
+ t.Fatalf("writer POST /updates: status %d, want 204", rec.Code)
+ }
+
+ // The owner sees what the writer wrote. Without this the test passes
+ // on a 204 that dropped the update on the floor.
+ if body := get(t, app, alice, "/d/"+sqid).Body.String(); !strings.Contains(body, "hello") {
+ t.Error("the owner's page does not show the writer's text")
+ }
+
+ // And a grantee can read the update log, which is how the editor
+ // catches up — also untested before.
+ logRec := get(t, app, bob, "/d/"+sqid+"/updates")
+ if logRec.Code != http.StatusOK || logRec.Body.Len() == 0 {
+ t.Errorf("writer GET /updates: status %d, %d bytes", logRec.Code, logRec.Body.Len())
+ }
+}
+
+// postBytes is post for a binary body: the update routes take raw CRDT
+// bytes, not a form.
+func postBytes(t *testing.T, app *docs.App, cookies []*http.Cookie, path string, body []byte) *httptest.ResponseRecorder {
+ t.Helper()
+ req := httptest.NewRequest(http.MethodPost, path, bytes.NewReader(body))
+ req.Header.Set("Content-Type", "application/octet-stream")
+ req.Header.Set("Origin", docs.DefaultOrigin)
+ for _, c := range cookies {
+ req.AddCookie(c)
+ }
+ rec := httptest.NewRecorder()
+ app.Handler().ServeHTTP(rec, req)
+ return rec
+}