From d2f96e79bb4161e182d8216de4a402452f7c3a7c Mon Sep 17 00:00:00 2001 From: weichen Date: Tue, 13 Oct 2026 10:23:37 +0000 Subject: [PATCH] Admit private member uploads and preserve masked metadata edits Why: Nested unchanged credential masks left empty parents that looked like managed subtree overwrites. Shared members also hit an owner-only upload guard, while upload ingestion cards could disclose private filenames and parser metadata. Evidence: SELF independent nested-mask probe, HTTP nested/mixed writeback, member upload, project card privacy and authorization-change regressions failed before each fix and passed after it. Affected settings/server suite passed; exact final-object build and independent replay remain delivery gates owned by the task coordinator. Impact: Prune only parents emptied by ignored masks; explicit empty/real managed writes stay rejected. Registered uploads check live work and trusted project existence, keep originals owned by the actor, and recheck before persistence. Suppress only upload project cards when no private stream exists; explicit structure cards and unregistered upload behavior remain available. Refs: Dev project-member-access plan r1 Task 1/R5; independent nested mask P2; 2026-10-13 composed member-upload and private project-stream acceptance findings --- internal/server/project_lifecycle_test.go | 222 +++++++++++++++++++++ internal/server/server.go | 34 +++- internal/settings/project_metadata.go | 24 ++- internal/settings/project_metadata_test.go | 33 +++ 4 files changed, 306 insertions(+), 7 deletions(-) diff --git a/internal/server/project_lifecycle_test.go b/internal/server/project_lifecycle_test.go index d4dc059..47ea686 100644 --- a/internal/server/project_lifecycle_test.go +++ b/internal/server/project_lifecycle_test.go @@ -1,6 +1,7 @@ package server import ( + "agent-system-backend/internal/cardsvc" "agent-system-backend/internal/cmdbus" "agent-system-backend/internal/surfacesvc" "agent-system-backend/pkg/extension" @@ -10,6 +11,7 @@ import ( "encoding/json" "errors" "fmt" + "mime/multipart" "net/http" "net/http/httptest" "testing" @@ -279,3 +281,223 @@ func TestGenericProjectPatchRejectsManagedSettingsAtomically(t *testing.T) { }) } } + +func TestGenericProjectPatchAcceptsNestedMaskedWriteback(t *testing.T) { + for _, registered := range []bool{false, true} { + t.Run(fmt.Sprint(registered), func(t *testing.T) { + s, st := newAuthTestServer(t) + key, _ := st.CreateAPIKey("alice", "test") + p, _ := st.CreateProject("alice", "Project") + if err := st.UpdateProjectSettings("alice", p.ID, map[string]any{"models": map[string]any{"chat": map[string]any{"apiKey": "fixture-persisted-key"}}, "display": map[string]any{"caption": "Original"}}); err != nil { + t.Fatal(err) + } + if registered { + s.SetProjectLifecycle(extension.ProjectLifecycle{List: func(context.Context, extension.Host) ([]core.Project, error) { + current, err := st.GetProject("alice", p.ID) + return []core.Project{current}, err + }}, nil) + } + for _, tc := range []struct{ body, caption string }{ + {`{"settings":{"models":{"chat":{"apiKey":"••••••"}},"display":{"caption":"Nested updated"}}}`, "Nested updated"}, + {`{"settings":{"models.chat":{"apiKey":"••••••"},"display":{"caption":"Mixed updated"}}}`, "Mixed updated"}, + {`{"settings":{"models":{"chat.apiKey":"••••••"},"display":{"caption":"Mixed leaf updated"}}}`, "Mixed leaf updated"}, + } { + w := lifecycleRequest(t, s, key, "PATCH", "/api/projects/"+p.ID, tc.body) + if w.Code != 200 { + t.Fatalf("masked writeback rejected legal edit: %d", w.Code) + } + current, err := st.GetProject("alice", p.ID) + if err != nil { + t.Fatal(err) + } + if current.Settings["models"].(map[string]any)["chat"].(map[string]any)["apiKey"] != "fixture-persisted-key" || current.Settings["display"].(map[string]any)["caption"] != tc.caption { + t.Fatalf("masked writeback secret preserved=%v caption=%q expected=%q", current.Settings["models"].(map[string]any)["chat"].(map[string]any)["apiKey"] == "fixture-persisted-key", current.Settings["display"].(map[string]any)["caption"], tc.caption) + } + } + for _, body := range []string{`{"settings":{"models":{}}}`, `{"settings":{"models":{"chat":{}}}}`, `{"settings":{"models.chat":{}}}`, `{"settings":{"models":{"chat":{"apiKey":"••••••","model":"Overwrite"}}}}`} { + if w := lifecycleRequest(t, s, key, "PATCH", "/api/projects/"+p.ID, body); w.Code != 400 { + t.Fatalf("explicit empty/real managed write accepted: %d", w.Code) + } + } + }) + } +} + +func lifecycleUpload(t *testing.T, s *Server, key, projectID, name, content string) *httptest.ResponseRecorder { + t.Helper() + var body bytes.Buffer + writer := multipart.NewWriter(&body) + file, err := writer.CreateFormFile("file", name) + if err != nil { + t.Fatal(err) + } + if _, err := file.Write([]byte(content)); err != nil { + t.Fatal(err) + } + if err := writer.Close(); err != nil { + t.Fatal(err) + } + req := httptest.NewRequest("POST", "/api/projects/"+projectID+"/uploads", &body) + req.Header.Set("Authorization", "Bearer "+key) + req.Header.Set("Content-Type", writer.FormDataContentType()) + rec := httptest.NewRecorder() + s.Handler().ServeHTTP(rec, req) + return rec +} + +func TestProjectLifecycleMemberUploadStaysPrivate(t *testing.T) { + s, st := newAuthTestServer(t) + alice, _ := st.CreateAPIKey("alice", "test") + bob, _ := st.CreateAPIKey("bob", "test") + mallory, _ := st.CreateAPIKey("mallory", "test") + p, _ := st.CreateProject("alice", "Shared") + ready := true + s.SetProjectLifecycle(extension.ProjectLifecycle{Work: func(_ context.Context, actor string, id core.ID) (bool, error) { + return ready && id == p.ID && (actor == "alice" || actor == "bob"), nil + }}, nil) + s.SetResourceAuthorizerV2(func(q extension.ResourceAuthzQuery) (bool, error) { + if q.ResourceType == "artifact" { + return q.Actor == q.Owner, nil + } + return q.ResourceType == "project" && q.ProjectID == p.ID && (q.Actor == "alice" || q.Actor == "bob"), nil + }) + s.DisableUploadIndexing = true + w := lifecycleUpload(t, s, bob, p.ID, "member-private.txt", "QA_MEMBER_PRIVATE_ORIGINAL") + if w.Code != 200 { + t.Fatalf("member upload=%d", w.Code) + } + var info core.UploadInfo + if err := json.Unmarshal(w.Body.Bytes(), &info); err != nil { + t.Fatal(err) + } + artifact, err := st.GetArtifact("bob", info.ArtifactID) + if err != nil || artifact.ProjectID != p.ID || artifact.Owner != "bob" { + t.Fatalf("upload ownership/target incorrect: %+v %v", artifact, err) + } + if w := lifecycleRequest(t, s, bob, "GET", "/api/artifacts/"+info.ArtifactID+"/content", ""); w.Code != 200 || w.Body.String() != "QA_MEMBER_PRIVATE_ORIGINAL" { + t.Fatalf("owner original read=%d", w.Code) + } + for _, key := range []string{alice, mallory} { + for _, suffix := range []string{"", "/content"} { + if w := lifecycleRequest(t, s, key, "GET", "/api/artifacts/"+info.ArtifactID+suffix, ""); w.Code != 404 { + t.Fatalf("other actor read private original=%d", w.Code) + } + } + } + if w := lifecycleUpload(t, s, mallory, p.ID, "forbidden.txt", "DENIED"); w.Code != 404 { + t.Fatalf("nonmember upload=%d", w.Code) + } + ready = false + if w := lifecycleUpload(t, s, bob, p.ID, "revoked.txt", "DENIED"); w.Code != 404 { + t.Fatalf("revoked upload=%d", w.Code) + } + artifacts, err := st.ListProjectArtifactsAll(p.ID) + if err != nil || len(artifacts) != 1 { + t.Fatalf("denied upload wrote artifacts: count=%d error=%v", len(artifacts), err) + } +} + +func TestProjectLifecycleUploadSuppressesPrivateProjectCards(t *testing.T) { + s, st := newAuthTestServer(t) + alice, _ := st.CreateAPIKey("alice", "test") + bob, _ := st.CreateAPIKey("bob", "test") + p, _ := st.CreateProject("alice", "Shared") + s.SetProjectLifecycle(extension.ProjectLifecycle{Work: func(_ context.Context, actor string, id core.ID) (bool, error) { + return id == p.ID && (actor == "alice" || actor == "bob"), nil + }}, nil) + s.SetResourceAuthorizerV2(func(q extension.ResourceAuthzQuery) (bool, error) { + return q.ResourceType == "project" && q.ProjectID == p.ID && (q.Actor == "alice" || q.Actor == "bob"), nil + }) + s.Cards = cardsvc.New(st, s.mgr.EmitCardEvent) + s.DisableUploadIndexing = true + if _, err := s.Cards.Create("alice", core.ActivityCard{ProjectID: p.ID, Title: "Shared structure", Kind: core.CardKindProgress}, ""); err != nil { + t.Fatal(err) + } + // The owner path already admitted upload before the member authorization fix. + if w := lifecycleUpload(t, s, alice, p.ID, "owner-private-filename.txt", "PRIVATE ORIGINAL"); w.Code != 200 { + t.Fatalf("owner upload=%d", w.Code) + } + if w := lifecycleUpload(t, s, bob, p.ID, "member-private-filename.txt", "MEMBER PRIVATE ORIGINAL"); w.Code != 200 { + t.Fatalf("member upload=%d", w.Code) + } + for _, owner := range []string{"alice", "bob"} { + cards, err := st.ListProjectCards(owner, p.ID) + if err != nil { + t.Fatal(err) + } + expected := 0 + if owner == "alice" { + expected = 1 + } + if len(cards) != expected { + t.Fatal("private upload added project card history") + } + for _, card := range cards { + if card.Title != "Shared structure" { + t.Fatal("private filename/parser card entered project history") + } + } + } + events, err := st.EventsAfter(core.ProjectChannelKey(p.ID), 0, 100) + if err != nil { + t.Fatal(err) + } + if len(events) != 1 { + t.Fatal("private upload added project events") + } + for _, event := range events { + if bytes.Contains(event.Payload, []byte("owner-private-filename.txt")) || bytes.Contains(event.Payload, []byte("member-private-filename.txt")) { + t.Fatal("private upload metadata entered persisted project stream") + } + } + request := httptest.NewRequest("GET", "/api/events?scope=project&projectId="+p.ID, nil) + request.Header.Set("Authorization", "Bearer "+bob) + ctx, cancel := context.WithTimeout(request.Context(), 20*time.Millisecond) + defer cancel() + request = request.WithContext(ctx) + response := httptest.NewRecorder() + s.Handler().ServeHTTP(response, request) + if response.Code != 200 || !bytes.Contains(response.Body.Bytes(), []byte("Shared structure")) { + t.Fatalf("member structural stream did not replay: %d", response.Code) + } + if bytes.Contains(response.Body.Bytes(), []byte("owner-private-filename.txt")) || bytes.Contains(response.Body.Bytes(), []byte("member-private-filename.txt")) { + t.Fatal("another member's SSE exposed private upload metadata") + } +} + +func TestUnregisteredUploadKeepsLegacyCardsAndValidation(t *testing.T) { + s, st := newAuthTestServer(t) + key, _ := st.CreateAPIKey("alice", "test") + p, _ := st.CreateProject("alice", "Private") + s.Cards = cardsvc.New(st, s.mgr.EmitCardEvent) + s.DisableUploadIndexing = true + if w := lifecycleUpload(t, s, key, p.ID, "legacy-upload.txt", "Original"); w.Code != 200 { + t.Fatalf("unregistered owner upload=%d", w.Code) + } + cards, err := st.ListProjectCards("alice", p.ID) + if err != nil || len(cards) != 1 || cards[0].Title != "legacy-upload.txt" { + t.Fatalf("unregistered ingestion card changed: count=%d error=%v", len(cards), err) + } + if w := lifecycleRequest(t, s, key, "POST", "/api/projects/missing/uploads", "not multipart"); w.Code != 400 { + t.Fatalf("legacy missing-file validation changed: %d", w.Code) + } +} + +func TestProjectLifecycleUploadRechecksBeforePersistence(t *testing.T) { + s, st := newAuthTestServer(t) + key, _ := st.CreateAPIKey("bob", "test") + p, _ := st.CreateProject("alice", "Shared") + ready := true + s.SetProjectLifecycle(extension.ProjectLifecycle{Work: func(context.Context, string, core.ID) (bool, error) { + allowed := ready + ready = false + return allowed, nil + }}, nil) + if w := lifecycleUpload(t, s, key, p.ID, "revoked-during-upload.txt", "PRIVATE"); w.Code != 404 { + t.Fatalf("upload persisted after authorization changed: %d", w.Code) + } + artifacts, err := st.ListProjectArtifactsAll(p.ID) + if err != nil || len(artifacts) != 0 { + t.Fatal("revoked upload created an artifact") + } +} diff --git a/internal/server/server.go b/internal/server/server.go index f932783..426070e 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1413,6 +1413,17 @@ func (s *Server) cardDismiss(w http.ResponseWriter, r *http.Request) { func (s *Server) upload(w http.ResponseWriter, r *http.Request) { projectID := r.PathValue("id") + owner := s.owner(r) + if s.projectLifecycle != nil { + if err := s.projectWork(r.Context(), owner, projectID); err != nil { + writeProjectErr(w, err) + return + } + if _, err := s.st.ProjectMetadata(projectID); err != nil { + writeStoreErr(w, err) + return + } + } r.ParseMultipartForm(64 << 20) file, header, err := r.FormFile("file") if err != nil { @@ -1425,10 +1436,18 @@ func (s *Server) upload(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } - owner := s.owner(r) - if _, err := s.st.GetProject(owner, projectID); err != nil { - writeStoreErr(w, err) - return + if s.projectLifecycle == nil { + if _, err := s.st.GetProject(owner, projectID); err != nil { + writeStoreErr(w, err) + return + } + } + if s.projectLifecycle != nil { + // Request-body reading can outlast the membership/ready decision. + if err := s.projectWork(r.Context(), owner, projectID); err != nil { + writeProjectErr(w, err) + return + } } mime := header.Header.Get("Content-Type") art := core.Artifact{ProjectID: projectID, Owner: owner, Name: header.Filename, Kind: kindOf(header.Filename), MIME: mime} @@ -1443,11 +1462,14 @@ func (s *Server) upload(w http.ResponseWriter, r *http.Request) { Status: core.CardActive, Tone: "info", Body: []core.UINode{{Type: core.UIProgress, Progress: &core.UIProgressSpec{Label: "解析中", Value: 0.3}}}, } - if s.Cards != nil { + // This endpoint has no private conversation stream. Registered uploads keep + // original filename/parser metadata out of shared project cards and events. + showIngestCard := s.Cards != nil && s.projectLifecycle == nil + if showIngestCard { ingestCard, _ = s.Cards.Create(owner, ingestCard, "") } doc := ingest.Parse(art.Name, mime, content) - if s.Cards != nil { + if showIngestCard { ingestCard.Status = core.CardDone ingestCard.Tone = "ok" body := []core.UINode{ diff --git a/internal/settings/project_metadata.go b/internal/settings/project_metadata.go index 56359ab..2eb0035 100644 --- a/internal/settings/project_metadata.go +++ b/internal/settings/project_metadata.go @@ -71,13 +71,35 @@ func maskedProjectValue(value any, path string, secrets map[string]bool) any { // rules as project settings, while preserving the metadata store's key shapes. // Project metadata lives in projects.settings, not the settings scope table. func (s *Service) PrepareProjectMetadataWrite(values map[string]any) (map[string]any, error) { - incoming := dropMaskPlaceholders(values) + incoming := dropProjectMaskPlaceholders(values) if err := s.checkManagedWrite(normalizeKeys(incoming)); err != nil { return nil, err } return incoming, nil } +// dropProjectMaskPlaceholders prunes a parent only when a previously nonempty +// object became empty through mask removal. Explicit empty objects still express +// an overwrite and must reach the managed-path guard unchanged. +func dropProjectMaskPlaceholders(values map[string]any) map[string]any { + out := make(map[string]any, len(values)) + for key, value := range values { + if child, ok := value.(map[string]any); ok { + prepared := dropProjectMaskPlaceholders(child) + if len(child) > 0 && len(prepared) == 0 { + continue + } + out[key] = prepared + continue + } + if text, ok := value.(string); ok && text == MaskPlaceholder { + continue + } + out[key] = value + } + return out +} + func (s *Service) checkManagedWrite(incoming map[string]any) error { if hits := core.TouchesManagedSettings(incoming, s.ManagedSettingPaths()); len(hits) > 0 { return &ErrManagedPath{Paths: hits} diff --git a/internal/settings/project_metadata_test.go b/internal/settings/project_metadata_test.go index 12fa9f3..443eb83 100644 --- a/internal/settings/project_metadata_test.go +++ b/internal/settings/project_metadata_test.go @@ -51,3 +51,36 @@ func TestPrepareProjectMetadataWriteUsesManagedGuard(t *testing.T) { t.Fatal("write preparation mutated its input") } } + +func TestPrepareProjectMetadataWritePrunesOnlyMaskedParents(t *testing.T) { + s := NewService(nil) + for _, raw := range []map[string]any{ + {"models": map[string]any{"chat": map[string]any{"apiKey": MaskPlaceholder}}, "display": map[string]any{"caption": "Updated"}}, + {"models.chat": map[string]any{"apiKey": MaskPlaceholder}, "display.caption": "Updated"}, + {"models": map[string]any{"chat.apiKey": MaskPlaceholder}, "display.caption": "Updated"}, + } { + before, _ := json.Marshal(raw) + got, err := s.PrepareProjectMetadataWrite(raw) + if err != nil { + t.Fatalf("unchanged masked credential rejected legal metadata edit: %v", err) + } + if len(got) != 1 { + t.Fatal("mask-only parent remained in prepared metadata") + } + after, _ := json.Marshal(raw) + if !bytes.Equal(before, after) { + t.Fatal("mask preparation changed its input") + } + } + for _, raw := range []map[string]any{ + {"models": map[string]any{}}, + {"models": map[string]any{"chat": map[string]any{}}}, + {"models.chat": map[string]any{}}, + {"models": map[string]any{"chat": map[string]any{"apiKey": MaskPlaceholder, "model": "Real overwrite"}}}, + {"models.chat": map[string]any{"apiKey": MaskPlaceholder}, "models": map[string]any{"chat": map[string]any{}}}, + } { + if _, err := s.PrepareProjectMetadataWrite(raw); err == nil { + t.Fatal("explicit empty or real managed overwrite accepted") + } + } +}