From 7af1276c3b43cabb4806251410fbb562096b19ea Mon Sep 17 00:00:00 2001 From: rcourtman Date: Mon, 4 May 2026 22:43:35 +0100 Subject: [PATCH] Fail closed on blank magic-link principals --- .../v6/internal/IDENTITY_INVARIANTS.md | 3 + .../v6/internal/subsystems/agent-lifecycle.md | 4 + .../v6/internal/subsystems/api-contracts.md | 3 + .../subsystems/organization-settings.md | 4 +- .../internal/subsystems/storage-recovery.md | 4 + internal/api/contract_test.go | 3 + .../api/identity_invariant_contract_test.go | 4 + internal/api/magic_link_handlers.go | 14 +-- internal/api/magic_link_handlers_test.go | 87 +++++++++++++++++++ internal/models/organization.go | 4 +- .../models/organization_additional_test.go | 22 +++++ 11 files changed, 137 insertions(+), 15 deletions(-) diff --git a/docs/release-control/v6/internal/IDENTITY_INVARIANTS.md b/docs/release-control/v6/internal/IDENTITY_INVARIANTS.md index 8ce9ccf80..673e39eff 100644 --- a/docs/release-control/v6/internal/IDENTITY_INVARIANTS.md +++ b/docs/release-control/v6/internal/IDENTITY_INVARIANTS.md @@ -79,6 +79,9 @@ principal once a stable user ID exists. email-keyed tenant records after a stable subject is already present. 2. Magic-link sessions bind to the stored organization principal resolved from contact email at verification time, not to the email embedded in the token. + If the matching organization owner/member has no stored principal, the + magic-link flow must fail closed instead of synthesizing a principal from + contact email. 3. Hosted checkout and tenant provisioning seed organization membership from stable Pulse user IDs. Registry-backed paths must create or resolve the registry user before writing hosted tenant `OwnerUserID` or member `UserID`; diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index f031400ce..069b964a1 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -193,6 +193,10 @@ profile and assignment columns, but embedded table framing must route through already be stable, non-email principals; lifecycle-adjacent routes must not recover authority from a blank handoff subject by falling back to contact email. + The same rule applies to hosted public magic-link sessions consumed by + lifecycle-adjacent routes: shared auth must mint a browser session only for + a stored organization principal, not for a contact email on a blank + owner/member row. API-token owner metadata follows the same rule: lifecycle-adjacent setup or mobile-pairing token flows may consume the shared token helper, but they must not pass extension metadata that authors or overwrites `owner_user_id`. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index cc1ff3af8..fe63b0343 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -395,6 +395,9 @@ the canonical monitored-system blocked payload. the token may carry contact email for delivery, but `/api/public/magic-link/verify` must resolve that email against current server-side organization metadata and create the browser session for the stored owner/member principal. + Magic-link request and verify paths must fail closed when contact email + matches a blank owner/member principal instead of sending or accepting a + token that would turn email into the session identity. Public hosted signup must therefore keep the generated owner user ID server-side for org metadata and RBAC assignment while using returned contact email only for `GenerateToken`/`SendMagicLink`; the accepted signup diff --git a/docs/release-control/v6/internal/subsystems/organization-settings.md b/docs/release-control/v6/internal/subsystems/organization-settings.md index 59482f9af..a29480e61 100644 --- a/docs/release-control/v6/internal/subsystems/organization-settings.md +++ b/docs/release-control/v6/internal/subsystems/organization-settings.md @@ -174,7 +174,9 @@ Email-delivery flows that need to mint sessions, including hosted magic links, must resolve contact email back through this organization model before authorizing. `internal/models/organization.go` owns the email-to-principal helper so handlers do not duplicate membership lookup or accidentally bind -sessions to email. +sessions to email. A matching owner/member contact email without a stored +`OwnerUserID` or member `UserID` is not a principal; the helper must fail +closed instead of manufacturing a session key from email. That same org-control surface also treats owner transfer as a re-auth-bound operation. Existing membership remains a prerequisite for the target user, and the acting owner must present a fresh browser session minted through the diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 35e2ff6b1..846959bde 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -78,6 +78,10 @@ Hosted direct handoff subjects that reach recovery-adjacent protected routes must therefore already be stable non-email principals; a blank handoff `UserID` must fail at the shared API boundary instead of being repaired from contact email. +Hosted public magic-link sessions follow the same dependent-auth contract: +storage/recovery-adjacent routes may consume the resulting browser session, but +shared auth must reject blank owner/member principals rather than minting an +email-keyed session from delivery metadata. The canonical actor vocabulary for those shared sessions is `docs/release-control/v6/internal/IDENTITY_INVARIANTS.md`; recovery and storage work may consume accepted org access, but must not mint or widen access from a diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 3af48b061..d5d0799e7 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -112,6 +112,9 @@ func TestContract_HostedMagicLinkStablePrincipalProof(t *testing.T) { if !strings.Contains(string(modelSource), "ResolvePrincipalByEmail") { t.Fatal("organization model must expose email-to-stable-principal resolution") } + if strings.Contains(string(modelSource), "userID = email") { + t.Fatal("organization model must not synthesize magic-link principals from email") + } } func TestContract_HostedHandoffRequiresStableSubjectProof(t *testing.T) { diff --git a/internal/api/identity_invariant_contract_test.go b/internal/api/identity_invariant_contract_test.go index 45a6d284a..9d4de5efc 100644 --- a/internal/api/identity_invariant_contract_test.go +++ b/internal/api/identity_invariant_contract_test.go @@ -113,6 +113,7 @@ func TestContract_HostedIdentityUsesStablePrincipals(t *testing.T) { "OwnerEmail string", "ResolvePrincipalByEmail", "CanonicalizePrincipalIdentity", + "return \"\", \"\", false", }, "../../docs/release-control/v6/internal/IDENTITY_INVARIANTS.md": { "Email is contact metadata", @@ -172,6 +173,9 @@ func TestContract_HostedIdentityUsesStablePrincipals(t *testing.T) { "UserID: ownerEmail", "contactEmailForLegacyUserID", }, + "../models/organization.go": { + "userID = email", + }, } for file, needles := range forbidden { for _, needle := range needles { diff --git a/internal/api/magic_link_handlers.go b/internal/api/magic_link_handlers.go index bf407c199..88f3b3011 100644 --- a/internal/api/magic_link_handlers.go +++ b/internal/api/magic_link_handlers.go @@ -246,25 +246,15 @@ func (h *MagicLinkHandlers) findOrgForEmail(email string) (string, bool, error) return "", false, nil } - // Prefer exact owner match; otherwise accept membership. for _, org := range orgs { if org == nil { continue } - if strings.EqualFold(org.OwnerEmail, email) || strings.EqualFold(org.OwnerUserID, email) { + userID, role, ok := org.ResolvePrincipalByEmail(email) + if ok && strings.TrimSpace(userID) != "" && models.IsValidOrganizationRole(role) { return org.ID, true, nil } } - for _, org := range orgs { - if org == nil { - continue - } - for _, m := range org.Members { - if strings.EqualFold(m.Email, email) || strings.EqualFold(m.UserID, email) { - return org.ID, true, nil - } - } - } return "", false, nil } diff --git a/internal/api/magic_link_handlers_test.go b/internal/api/magic_link_handlers_test.go index 5439dccb9..198a4a46d 100644 --- a/internal/api/magic_link_handlers_test.go +++ b/internal/api/magic_link_handlers_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -11,6 +12,17 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/models" ) +type magicLinkCaptureEmailer struct { + to []string + urls []string +} + +func (c *magicLinkCaptureEmailer) SendMagicLink(to, magicLinkURL string) error { + c.to = append(c.to, to) + c.urls = append(c.urls, magicLinkURL) + return nil +} + func TestHandlePublicMagicLinkVerifyRejectsInvalidOrgIDInToken(t *testing.T) { key := []byte("0123456789abcdef0123456789abcdef") store := NewInMemoryMagicLinkStore() @@ -139,6 +151,81 @@ func TestHandlePublicMagicLinkVerifyUsesStableOrganizationPrincipal(t *testing.T } } +func TestHandlePublicMagicLinkVerifyRejectsBlankOrganizationPrincipal(t *testing.T) { + resetPersistentAuthStoresForTests() + t.Cleanup(resetPersistentAuthStoresForTests) + + dataDir := t.TempDir() + InitSessionStore(dataDir) + InitCSRFStore(dataDir) + persistence := config.NewMultiTenantPersistence(dataDir) + + if err := persistence.SaveOrganization(&models.Organization{ + ID: "org_magic_blank_owner", + DisplayName: "Magic Blank Owner", + CreatedAt: time.Now().UTC(), + OwnerEmail: "owner@example.com", + }); err != nil { + t.Fatalf("SaveOrganization: %v", err) + } + + key := []byte("0123456789abcdef0123456789abcdef") + store := NewInMemoryMagicLinkStore() + svc := NewMagicLinkServiceWithKey(key, store, nil, nil) + t.Cleanup(func() { svc.Stop() }) + token, err := svc.GenerateToken("owner@example.com", "org_magic_blank_owner") + if err != nil { + t.Fatalf("GenerateToken: %v", err) + } + + h := NewMagicLinkHandlers(persistence, svc, true, nil) + req := httptest.NewRequest(http.MethodGet, "/api/public/magic-link/verify?format=json&token="+token, nil) + req.Header.Set("Accept", "application/json") + rec := httptest.NewRecorder() + h.HandlePublicMagicLinkVerify(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400: %s", rec.Code, rec.Body.String()) + } + for _, cookie := range rec.Result().Cookies() { + if cookie.Name == cookieNameSession { + t.Fatalf("did not expect session cookie for blank stored principal") + } + } +} + +func TestHandlePublicMagicLinkRequestDoesNotSendForBlankOrganizationPrincipal(t *testing.T) { + dataDir := t.TempDir() + persistence := config.NewMultiTenantPersistence(dataDir) + if err := persistence.SaveOrganization(&models.Organization{ + ID: "org_magic_blank_request", + DisplayName: "Magic Blank Request", + CreatedAt: time.Now().UTC(), + OwnerEmail: "owner@example.com", + }); err != nil { + t.Fatalf("SaveOrganization: %v", err) + } + + emailer := &magicLinkCaptureEmailer{} + svc := NewMagicLinkServiceWithKey([]byte("0123456789abcdef0123456789abcdef"), NewInMemoryMagicLinkStore(), emailer, nil) + t.Cleanup(func() { svc.Stop() }) + h := NewMagicLinkHandlers(persistence, svc, true, func(*http.Request) string { + return "https://pulse.example.com" + }) + + req := httptest.NewRequest(http.MethodPost, "/api/public/magic-link/request", strings.NewReader(`{"email":"owner@example.com"}`)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.HandlePublicMagicLinkRequest(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200: %s", rec.Code, rec.Body.String()) + } + if len(emailer.to) != 0 || len(emailer.urls) != 0 { + t.Fatalf("sent magic links = to:%v urls:%v, want none", emailer.to, emailer.urls) + } +} + func TestHandlePublicMagicLinkVerifyRejectsRemovedOrganizationMember(t *testing.T) { resetPersistentAuthStoresForTests() t.Cleanup(resetPersistentAuthStoresForTests) diff --git a/internal/models/organization.go b/internal/models/organization.go index 977e55549..a0b5a4263 100644 --- a/internal/models/organization.go +++ b/internal/models/organization.go @@ -386,7 +386,7 @@ func (o *Organization) ResolvePrincipalByEmail(email string) (string, Organizati if ownerMatchesEmail(o, email) { userID := normalizeOrganizationIdentityValue(o.OwnerUserID) if userID == "" { - userID = email + return "", "", false } return userID, OrgRoleOwner, true } @@ -396,7 +396,7 @@ func (o *Organization) ResolvePrincipalByEmail(email string) (string, Organizati } userID := normalizeOrganizationIdentityValue(member.UserID) if userID == "" { - userID = email + return "", "", false } return userID, NormalizeOrganizationRole(member.Role), true } diff --git a/internal/models/organization_additional_test.go b/internal/models/organization_additional_test.go index c27d25c8f..a82c0d83c 100644 --- a/internal/models/organization_additional_test.go +++ b/internal/models/organization_additional_test.go @@ -90,6 +90,28 @@ func TestOrganizationResolvePrincipalByEmail(t *testing.T) { } } +func TestOrganizationResolvePrincipalByEmailRejectsBlankStoredPrincipal(t *testing.T) { + ownerOnlyEmail := &Organization{ + ID: "org-blank-owner", + OwnerEmail: "owner@example.com", + } + userID, role, ok := ownerOnlyEmail.ResolvePrincipalByEmail("owner@example.com") + if ok || userID != "" || role != "" { + t.Fatalf("blank owner principal = (%q, %q, %v), want rejection", userID, role, ok) + } + + memberOnlyEmail := &Organization{ + ID: "org-blank-member", + Members: []OrganizationMember{ + {Email: "member@example.com", Role: OrgRoleViewer}, + }, + } + userID, role, ok = memberOnlyEmail.ResolvePrincipalByEmail("member@example.com") + if ok || userID != "" || role != "" { + t.Fatalf("blank member principal = (%q, %q, %v), want rejection", userID, role, ok) + } +} + func TestOrganizationRoleNormalization(t *testing.T) { if got := NormalizeOrganizationRole("member"); got != OrganizationRole("member") { t.Fatalf("expected member to remain unchanged, got %q", got)