test(webapp): pin "a read member sees grants and public links" (BEA-69) (#120)

A read-only member seeing the People matrix and every active public link
is deliberate — the owner re-confirmed it against Google Drive, where a
viewer can see who has access. Nothing asserted it: TestReadOnlyMemberRoutes
only checks GET .../shares is not 403, so a hardening pass that returned an
empty list (or dropped the creator field) to read members would pass every
test in the repo.

Adds TestReadMemberSeesSharesAndGrants — 200 plus the link's token, path,
url and creator from a read-only session — and extends the two rationale
comments the next reader lands on instead of adding a third copy.

No route change, no UI change; the ShareBanner edit is a comment, so
internal/webapp/static is unchanged.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Snow Lee (Sungwon)
2026-08-05 16:20:16 +09:00
committed by GitHub
co-authored by Claude Opus 5
parent 00581f2839
commit 631e5be947
3 changed files with 66 additions and 1 deletions
@@ -12,7 +12,13 @@ import { revokeShare, shareDetail } from "./SharesTable";
Anyone with read sees the banner (a member should know the folder they
rely on is exposed); Revoke is offered where the Share button already is,
and the server checks again anyway. */
and the server checks again anyway.
Same decision, one surface wider: project Settings shows a read-only member
every active public link and who created it (BEA-69 — the owner's call,
benchmarked against Google Drive showing a viewer who has access). Neither
this banner nor that list is a leak to hide later; both routes stay
PermRead. */
export function ShareBanner({
shares,
canRevoke,
+7
View File
@@ -141,6 +141,13 @@ func permDenied(level string) string {
// handleProjectPerms returns the project's permission settings: the default
// level, the caller's own effective level, and the explicit grants. Any member
// with read may look; the grants are org-internal, not secrets.
//
// Re-confirmed by the owner in BEA-69 after a read-only member reported seeing
// the whole People matrix: the benchmark is Google Drive, where a viewer can
// see who has access, and hiding grants makes "why can't I edit this?"
// unanswerable. Same call for the project's public-links list
// (handleShareList, also PermRead). Raising either to PermWrite is a product
// decision, not a hardening fix — TestReadMemberSeesSharesAndGrants pins both.
func (s *Server) handleProjectPerms(w http.ResponseWriter, r *http.Request) {
id := r.PathValue("project")
p, ok := s.project(w, r, id, PermRead)
+52
View File
@@ -337,6 +337,58 @@ func TestPermissionsGET(t *testing.T) {
}
}
// A read member sees the People matrix and the project's public links, creator
// included. Deliberate, and answered by the owner (BEA-69, 2026-08-03): the
// benchmark is Google Drive, where a viewer can see who has access — hiding
// grants makes "why can't I edit this?" unanswerable, and the per-file
// ShareBanner already tells a read-only member the file they rely on is
// public. TestReadOnlyMemberRoutes only checks these two routes are not 403,
// so an "improvement" that returned an empty body to read members would pass
// every other test in the repo. Tightening either route to PermWrite is a
// product decision, not a hardening fix; this test is here so it cannot happen
// by accident.
func TestReadMemberSeesSharesAndGrants(t *testing.T) {
h, srv, c, p := permHub(t)
if err := srv.Projects.SetPerm(p.ID, "bob@x.io", PermRead); err != nil {
t.Fatal(err)
}
sh, err := srv.Shares.Create(p.ID, "x.md", "alice@x.io", 0)
if err != nil {
t.Fatal(err)
}
rec := doAs(t, h, "GET", "/api/p/"+p.ID+"/shares", nil, c["bob"])
if rec.Code != 200 {
t.Fatalf("GET shares as a read member: %d %s", rec.Code, rec.Body)
}
var out struct {
Shares []map[string]any `json:"shares"`
}
if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil {
t.Fatal(err)
}
if len(out.Shares) != 1 {
t.Fatalf("shares = %+v, want the one link alice minted", out.Shares)
}
got := out.Shares[0]
if got["token"] != sh.Token || got["path"] != "x.md" {
t.Errorf("share = %+v, want token %q on x.md", got, sh.Token)
}
if url, _ := got["url"].(string); !strings.HasSuffix(url, "/s/"+sh.Token) {
t.Errorf("url = %q, want it to end in /s/%s", url, sh.Token)
}
// The field the issue is actually about, and the one a hardening pass
// would strip first. It exposes nothing new: the People table already
// shows every member's email to every member.
if got["creator"] != "alice@x.io" {
t.Errorf("creator = %v, want alice@x.io", got["creator"])
}
// And the grants alongside it, from the same read-only session.
rec = doAs(t, h, "GET", "/api/p/"+p.ID+"/permissions", nil, c["bob"])
if rec.Code != 200 || !strings.Contains(rec.Body.String(), "bob@x.io") {
t.Fatalf("GET permissions as a read member: %d %s", rec.Code, rec.Body)
}
}
// The project list carries the caller's level *alongside* every ordinary
// Project field. Regression guard: an earlier version hand-listed the fields
// it returned, which silently dropped description and icon the moment those