From 2285998221b04146f36941435b4ead1a3500371b Mon Sep 17 00:00:00 2001 From: Khaliq Date: Sat, 15 Aug 2026 10:58:40 +0200 Subject: [PATCH 01/12] fix(auth): enforce path ACLs with RelayAuth scopes --- internal/httpapi/acl.go | 97 ++++++++++++++++++++++-- internal/httpapi/acl_test.go | 86 ++++++++++++++++++++- internal/httpapi/github_tarball.go | 2 +- internal/httpapi/server.go | 26 +++---- internal/httpapi/server_test.go | 115 +++++++++++++++++++++++++++++ 5 files changed, 304 insertions(+), 22 deletions(-) diff --git a/internal/httpapi/acl.go b/internal/httpapi/acl.go index ff20a9c9..739c4269 100644 --- a/internal/httpapi/acl.go +++ b/internal/httpapi/acl.go @@ -66,7 +66,9 @@ func parsePermissionRule(raw string) *ParsedPermissionRule { // aclAgentNamePattern allows alphanumerics, hyphens, underscores, and dots. var aclAgentNamePattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]{0,127}$`) -// aclScopePattern allows scope values like "fs:read", "sync:trigger". +// aclScopePattern allows unscoped capability/tag values like "fs:read", +// "sync:trigger", and "finance". Path-bearing filesystem scopes are +// validated separately by isValidACLFilesystemScope. var aclScopePattern = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9]*(?::[a-zA-Z][a-zA-Z0-9]*)*$`) // aclWorkspacePattern allows workspace IDs like "ws_123" or UUIDs. @@ -78,7 +80,7 @@ func isValidACLRuleValue(kind, value string) bool { case "agent": return aclAgentNamePattern.MatchString(value) case "scope": - return aclScopePattern.MatchString(value) + return aclScopePattern.MatchString(value) || isValidACLFilesystemScope(value) case "workspace": return aclWorkspacePattern.MatchString(value) default: @@ -86,9 +88,72 @@ func isValidACLRuleValue(kind, value string) bool { } } -// filePermissionAllows evaluates ACL rules against agent claims. +type parsedACLFilesystemScope struct { + action string + path string +} + +// parseACLFilesystemScope recognizes both the RelayAuth four-segment scope +// vocabulary and Relayfile's legacy workspace-tag vocabulary. The latter is +// still present in durable ACL markers created by Cloud, but no longer needs +// to be carried literally by delegated tokens. +func parseACLFilesystemScope(scope string) (*parsedACLFilesystemScope, bool) { + segments := strings.SplitN(scope, ":", 4) + if len(segments) == 2 && segments[0] == "fs" { + if !isACLFilesystemAction(segments[1]) { + return nil, false + } + return &parsedACLFilesystemScope{action: segments[1], path: "*"}, true + } + if len(segments) < 3 { + return nil, false + } + + switch segments[0] { + case "relayfile": + if segments[1] != "fs" { + return nil, false + } + case "workspace": + if !aclAgentNamePattern.MatchString(segments[1]) { + return nil, false + } + default: + return nil, false + } + + if !isACLFilesystemAction(segments[2]) { + return nil, false + } + path := "*" + if len(segments) == 4 { + path = segments[3] + } + return &parsedACLFilesystemScope{action: segments[2], path: path}, true +} + +func isACLFilesystemAction(action string) bool { + return action == "read" || action == "write" || action == "manage" || action == "*" +} + +func isValidACLFilesystemScope(scope string) bool { + if strings.TrimSpace(scope) != scope { + return false + } + parsed, ok := parseACLFilesystemScope(scope) + if !ok { + return false + } + return scopePathValid(parsed.path) +} + +// filePermissionAllows evaluates ACL rules against agent claims for one +// filesystem action and path. Scope rules are semantic: a durable rule such +// as relayfile:fs:write:/protected/* matches a delegated token carrying the +// broader relayfile:fs:write:* grant without requiring the rule itself to be +// copied into the token. // Returns true if access is allowed. -func filePermissionAllows(permissions []string, workspaceID string, claims *tokenClaims) bool { +func filePermissionAllows(permissions []string, workspaceID string, claims *tokenClaims, requiredAction, requestedPath string) bool { if len(permissions) == 0 { // No ACL policy in effect — allow access. return true @@ -110,9 +175,7 @@ func filePermissionAllows(permissions []string, workspaceID string, claims *toke case "public": match = true case "scope": - if claims != nil { - _, match = claims.Scopes[rule.Value] - } + match = aclScopeRuleMatches(rule.Value, claims, requiredAction, requestedPath) case "agent": match = claims != nil && claims.AgentName == rule.Value case "workspace": @@ -137,6 +200,26 @@ func filePermissionAllows(permissions []string, workspaceID string, claims *toke return !enforceableRuleSeen } +func aclScopeRuleMatches(scope string, claims *tokenClaims, requiredAction, requestedPath string) bool { + if claims == nil { + return false + } + + parsed, filesystemScope := parseACLFilesystemScope(scope) + if !filesystemScope { + _, exactMatch := claims.Scopes[scope] + return exactMatch + } + if !scopeActionMatches(parsed.action, requiredAction) { + return false + } + if parsed.path != "*" && !scopePathMatches(parsed.path, requestedPath) { + return false + } + + return scopeMatchesPath(claims.Scopes, "fs:"+requiredAction, requestedPath) +} + // resolveFilePermissions walks ancestor dirs to collect ACL rules. // store is an interface that can read files from the workspace. func resolveFilePermissions(getFile func(path string) ([]byte, error), path string) []string { diff --git a/internal/httpapi/acl_test.go b/internal/httpapi/acl_test.go index dbfa911b..9cd1ef24 100644 --- a/internal/httpapi/acl_test.go +++ b/internal/httpapi/acl_test.go @@ -73,6 +73,8 @@ func TestFilePermissionAllows(t *testing.T) { permissions []string workspaceID string claims tokenClaims + action string + path string want bool }{ { @@ -130,6 +132,70 @@ func TestFilePermissionAllows(t *testing.T) { }, want: true, }, + { + name: "bare allow matches four segment RelayAuth grant", + permissions: []string{"allow:scope:fs:read"}, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:*": {}}, + }, + want: true, + }, + { + name: "readonly deny matches broader RelayAuth write grant", + permissions: []string{ + "allow:scope:fs:write", + "deny:scope:relayfile:fs:write:/protected/*", + }, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:write:*": {}}, + }, + action: "write", + path: "/protected/document.md", + want: false, + }, + { + name: "readonly write deny does not block reads", + permissions: []string{ + "allow:scope:fs:read", + "deny:scope:relayfile:fs:write:/protected/*", + }, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{ + "relayfile:fs:read:*": {}, + "relayfile:fs:write:*": {}, + }, + }, + path: "/protected/document.md", + want: true, + }, + { + name: "ignored read deny matches broader RelayAuth read grant", + permissions: []string{ + "allow:scope:fs:read", + "deny:scope:relayfile:fs:read:/ignored/*", + }, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:*": {}}, + }, + path: "/ignored/document.md", + want: false, + }, + { + name: "legacy workspace allow matches RelayAuth path grant semantically", + permissions: []string{ + "allow:scope:workspace:relayfile-local:read:/protected/*", + }, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:*": {}}, + }, + path: "/protected/document.md", + want: true, + }, { name: "deny overrides allow", permissions: []string{"allow:agent:code-agent", "deny:agent:code-agent"}, @@ -153,7 +219,15 @@ func TestFilePermissionAllows(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := filePermissionAllows(tt.permissions, tt.workspaceID, &tt.claims) + action := tt.action + if action == "" { + action = "read" + } + path := tt.path + if path == "" { + path = "/document.md" + } + got := filePermissionAllows(tt.permissions, tt.workspaceID, &tt.claims, action, path) if got != tt.want { t.Fatalf("expected %v, got %v", tt.want, got) } @@ -303,6 +377,11 @@ func TestIsValidACLRuleValue(t *testing.T) { {"valid scope", "scope", "fs:read", true}, {"valid scope simple", "scope", "admin", true}, {"scope with numbers", "scope", "fs2:read", true}, + {"valid RelayAuth path scope", "scope", "relayfile:fs:write:/protected/*", true}, + {"valid legacy workspace path scope", "scope", "workspace:relayfile-local:read:/protected/*", true}, + {"scope with path traversal", "scope", "relayfile:fs:read:/protected/../private/*", false}, + {"scope with internal glob", "scope", "relayfile:fs:read:/protected/*/private", false}, + {"scope with unsupported plane", "scope", "other:fs:read:/protected/*", false}, {"scope empty segment", "scope", "fs:", false}, {"valid workspace", "workspace", "ws_123", true}, {"workspace with uuid", "workspace", "abc-def-123", true}, @@ -350,6 +429,11 @@ func TestParsePermissionRuleValidation(t *testing.T) { raw: "allow:scope:fs:read", want: &ParsedPermissionRule{Effect: "allow", Kind: "scope", Value: "fs:read"}, }, + { + name: "valid path scope rule", + raw: "deny:scope:relayfile:fs:write:/protected/*", + want: &ParsedPermissionRule{Effect: "deny", Kind: "scope", Value: "relayfile:fs:write:/protected/*"}, + }, { name: "scope with invalid chars rejected", raw: "allow:scope:fs read", diff --git a/internal/httpapi/github_tarball.go b/internal/httpapi/github_tarball.go index abbaa94e..1d634497 100644 --- a/internal/httpapi/github_tarball.go +++ b/internal/httpapi/github_tarball.go @@ -626,7 +626,7 @@ func (s *Server) githubTarballWritePermissionError(workspaceID, workspacePath st Message: "failed to check file permissions", } } - if !filePermissionAllows(permissions, workspaceID, &claims) { + if !filePermissionAllows(permissions, workspaceID, &claims, "write", workspacePath) { return &relayfile.BulkWriteError{ Code: "forbidden", Message: "file access denied by permission policy", diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index ca3fba07..77816aef 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -326,7 +326,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { aclReader = s.aclGetForkFile(workspaceID, forkID) } permissions := resolveFilePermissionsWithTarget(aclReader, aclPath, includeTarget) - if !filePermissionAllows(permissions, workspaceID, &claims) { + if !filePermissionAllows(permissions, workspaceID, &claims, strings.TrimPrefix(requiredScope, "fs:"), aclPath) { writeError(w, http.StatusForbidden, "forbidden", "access denied by ACL", getCorrelationID(r)) return } @@ -1483,7 +1483,7 @@ func validateForkCommitEntries(workspaceID string, claims tokenClaims, entries [ if !scopeMatchesPath(claims.Scopes, "fs:write", entry.Path) { return &forkCommitAuthorizationError{message: "fork commit denied by path scope"} } - if !filePermissionAllows(entry.Permissions, workspaceID, &claims) { + if !filePermissionAllows(entry.Permissions, workspaceID, &claims, "write", entry.Path) { return &forkCommitAuthorizationError{message: "fork commit denied by permission policy"} } } @@ -1641,7 +1641,7 @@ func (s *Server) handleTree(w http.ResponseWriter, r *http.Request, workspaceID, } for _, item := range batch.Items { effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, item.Path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path) { continue } visibleFiles[item.Path] = struct{}{} @@ -1709,7 +1709,7 @@ func (s *Server) handleReadFile(w http.ResponseWriter, r *http.Request, workspac return } effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -1941,7 +1941,7 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { errorsOut = append(errorsOut, relayfile.BulkWriteError{ Path: path, Code: "forbidden", @@ -1951,7 +1951,7 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa } } else if readErr == relayfile.ErrNotFound || readErr == relayfile.ErrForkExpired { inheritedPermissions := s.resolveFilePermissions(workspaceID, forkID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { errorsOut = append(errorsOut, relayfile.BulkWriteError{ Path: path, Code: "forbidden", @@ -2016,7 +2016,7 @@ func (s *Server) handleExport(w http.ResponseWriter, r *http.Request, workspaceI continue } effectivePermissions := s.store.ResolveFilePermissions(workspaceID, file.Path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", file.Path) { continue } visible = append(visible, file) @@ -2058,13 +2058,13 @@ func (s *Server) handleWriteFile(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } } else if readErr == relayfile.ErrNotFound || readErr == relayfile.ErrForkExpired { inheritedPermissions := s.resolveFilePermissions(workspaceID, forkID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2143,13 +2143,13 @@ func (s *Server) handleMergeFile(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.store.ReadFile(workspaceID, path) if readErr == nil { existingPermissions := s.store.ResolveFilePermissions(workspaceID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } } else if readErr == relayfile.ErrNotFound { inheritedPermissions := s.store.ResolveFilePermissions(workspaceID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2232,7 +2232,7 @@ func (s *Server) handleDeleteFile(w http.ResponseWriter, r *http.Request, worksp _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2368,7 +2368,7 @@ func (s *Server) handleQueryFiles(w http.ResponseWriter, r *http.Request, worksp if permission != "" && !stringSliceContainsExact(effectivePermissions, permission) { continue } - if !filePermissionAllows(effectivePermissions, workspaceID, &claims) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path) { continue } items = append(items, item) diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index fad4d176..8649b532 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -3288,6 +3288,121 @@ func TestFilePermissionPolicyInheritanceFromAclMarker(t *testing.T) { } } +func TestFilePermissionPolicyPathOverridesWithRelayAuthScopes(t *testing.T) { + server := NewServer(relayfile.NewStore()) + workspaceID := "ws_perm_path_overrides" + token := mustTestJWT( + t, + "dev-secret", + workspaceID, + "relayfile-local", + []string{"relayfile:fs:read:*", "relayfile:fs:write:*"}, + time.Now().Add(time.Hour), + ) + + allowed := writeFileForTest(t, server, token, workspaceID, "/allowed/document.md", "0", "allowed", "corr_perm_path_1") + protected := writeFileForTest(t, server, token, workspaceID, "/protected/document.md", "0", "protected", "corr_perm_path_2") + ignored := writeFileForTest(t, server, token, workspaceID, "/ignored/document.md", "0", "ignored", "corr_perm_path_3") + + aclWrite := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/.relayfile.acl", + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_perm_path_4", + "If-Match": "0", + }, + body: map[string]any{ + "contentType": "text/plain", + "content": "# Managed by workspace API\n", + "semantics": map[string]any{ + "permissions": []string{ + "allow:scope:fs:read", + "allow:scope:fs:write", + "deny:scope:relayfile:fs:write:/protected/*", + "deny:scope:relayfile:fs:read:/ignored/*", + "deny:scope:relayfile:fs:write:/ignored/*", + }, + }, + }, + }) + if aclWrite.Code != http.StatusAccepted { + t.Fatalf("expected ACL marker write 202, got %d (%s)", aclWrite.Code, aclWrite.Body.String()) + } + + protectedRead := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/protected/document.md", + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_perm_path_5", + }, + }) + if protectedRead.Code != http.StatusOK { + t.Fatalf("expected readonly path read 200, got %d (%s)", protectedRead.Code, protectedRead.Body.String()) + } + + protectedWrite := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/protected/document.md", + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_perm_path_6", + "If-Match": protected.TargetRevision, + }, + body: map[string]any{ + "contentType": "text/markdown", + "content": "must stay protected", + }, + }) + if protectedWrite.Code != http.StatusForbidden { + t.Fatalf("expected readonly path write 403, got %d (%s)", protectedWrite.Code, protectedWrite.Body.String()) + } + + ignoredRead := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/ignored/document.md", + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_perm_path_7", + }, + }) + if ignoredRead.Code != http.StatusForbidden { + t.Fatalf("expected ignored path read 403, got %d (%s)", ignoredRead.Code, ignoredRead.Body.String()) + } + + ignoredWrite := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/ignored/document.md", + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_perm_path_8", + "If-Match": ignored.TargetRevision, + }, + body: map[string]any{ + "contentType": "text/markdown", + "content": "must stay ignored", + }, + }) + if ignoredWrite.Code != http.StatusForbidden { + t.Fatalf("expected ignored path write 403, got %d (%s)", ignoredWrite.Code, ignoredWrite.Body.String()) + } + + updated := writeFileForTest( + t, + server, + token, + workspaceID, + "/allowed/document.md", + allowed.TargetRevision, + "updated", + "corr_perm_path_9", + ) + if updated.TargetRevision == allowed.TargetRevision { + t.Fatalf("expected allowed path write to advance revision") + } +} + func TestOpsListProviderFilterEndpoint(t *testing.T) { store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{ Adapters: []relayfile.ProviderAdapter{ From 99a4e28f1c3f6ed57ce3506f30593d606c6a3882 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 10:24:54 +0200 Subject: [PATCH 02/12] Enforce ACL scope shapes --- internal/httpapi/acl.go | 81 ++++++++++++++--- internal/httpapi/acl_test.go | 66 ++++++++++++-- internal/httpapi/github_tarball.go | 2 +- internal/httpapi/server.go | 57 +++++++----- internal/httpapi/server_test.go | 139 +++++++++++++++++++++++++++++ 5 files changed, 301 insertions(+), 44 deletions(-) diff --git a/internal/httpapi/acl.go b/internal/httpapi/acl.go index 739c4269..63cfc0c3 100644 --- a/internal/httpapi/acl.go +++ b/internal/httpapi/acl.go @@ -80,6 +80,9 @@ func isValidACLRuleValue(kind, value string) bool { case "agent": return aclAgentNamePattern.MatchString(value) case "scope": + if strings.HasPrefix(value, "relayfile:") || strings.HasPrefix(value, "workspace:") || strings.HasPrefix(value, "*:") { + return isValidACLFilesystemScope(value) + } return aclScopePattern.MatchString(value) || isValidACLFilesystemScope(value) case "workspace": return aclWorkspacePattern.MatchString(value) @@ -105,31 +108,35 @@ func parseACLFilesystemScope(scope string) (*parsedACLFilesystemScope, bool) { } return &parsedACLFilesystemScope{action: segments[1], path: "*"}, true } - if len(segments) < 3 { + if len(segments) != 4 { + return nil, false + } + + plane := segments[0] + resource := segments[1] + action := segments[2] + path := segments[3] + if strings.TrimSpace(path) == "" { return nil, false } - switch segments[0] { - case "relayfile": - if segments[1] != "fs" { + switch plane { + case "relayfile", "*": + if resource != "fs" && resource != "*" { return nil, false } case "workspace": - if !aclAgentNamePattern.MatchString(segments[1]) { + if !aclAgentNamePattern.MatchString(resource) { return nil, false } default: return nil, false } - if !isACLFilesystemAction(segments[2]) { + if !isACLFilesystemAction(action) { return nil, false } - path := "*" - if len(segments) == 4 { - path = segments[3] - } - return &parsedACLFilesystemScope{action: segments[2], path: path}, true + return &parsedACLFilesystemScope{action: action, path: path}, true } func isACLFilesystemAction(action string) bool { @@ -151,14 +158,21 @@ func isValidACLFilesystemScope(scope string) bool { // filesystem action and path. Scope rules are semantic: a durable rule such // as relayfile:fs:write:/protected/* matches a delegated token carrying the // broader relayfile:fs:write:* grant without requiring the rule itself to be -// copied into the token. +// copied into the token. When allowDescendants is true the function also +// considers allow rules whose scoped path sits beneath the requested path if +// the agent holds a matching scope for the descendant detail (used by tree/query +// preflight checks). // Returns true if access is allowed. -func filePermissionAllows(permissions []string, workspaceID string, claims *tokenClaims, requiredAction, requestedPath string) bool { +func filePermissionAllows(permissions []string, workspaceID string, claims *tokenClaims, requiredAction, requestedPath string, allowDescendants bool) bool { if len(permissions) == 0 { // No ACL policy in effect — allow access. return true } + if requestedPath != "" { + requestedPath = normalizeACLPath(requestedPath) + } + enforceableRuleSeen := false allowMatch := false for _, raw := range permissions { @@ -176,6 +190,9 @@ func filePermissionAllows(permissions []string, workspaceID string, claims *toke match = true case "scope": match = aclScopeRuleMatches(rule.Value, claims, requiredAction, requestedPath) + if !match && allowDescendants && rule.Effect == "allow" { + match = aclScopeRuleMatchesDescendant(rule.Value, claims, requiredAction, requestedPath) + } case "agent": match = claims != nil && claims.AgentName == rule.Value case "workspace": @@ -220,6 +237,44 @@ func aclScopeRuleMatches(scope string, claims *tokenClaims, requiredAction, requ return scopeMatchesPath(claims.Scopes, "fs:"+requiredAction, requestedPath) } +func aclScopeRuleMatchesDescendant(scope string, claims *tokenClaims, requiredAction, requestedPath string) bool { + if claims == nil || requestedPath == "" { + return false + } + + parsed, filesystemScope := parseACLFilesystemScope(scope) + if !filesystemScope || parsed.path == "*" { + return false + } + if !scopeActionMatches(parsed.action, requiredAction) { + return false + } + + normalizedRulePath := normalizeScopePath(parsed.path) + normalizedRequestPath := normalizeACLPath(requestedPath) + for claimScope := range claims.Scopes { + claimPath, ok := pathScopeForRequired(claimScope, "fs", requiredAction) + if !ok { + continue + } + normalizedClaimPath := normalizeScopePath(claimPath) + if normalizedClaimPath != "*" && !withinBasePath(normalizedRequestPath, normalizedClaimPath) { + continue + } + if scopePathMatches(normalizedRulePath, normalizedClaimPath) || scopePathMatches(normalizedClaimPath, normalizedRulePath) { + return true + } + } + return false +} + +func normalizeScopePath(path string) string { + if path == "*" { + return "*" + } + return normalizeACLPath(path) +} + // resolveFilePermissions walks ancestor dirs to collect ACL rules. // store is an interface that can read files from the workspace. func resolveFilePermissions(getFile func(path string) ([]byte, error), path string) []string { diff --git a/internal/httpapi/acl_test.go b/internal/httpapi/acl_test.go index 9cd1ef24..e02ff50e 100644 --- a/internal/httpapi/acl_test.go +++ b/internal/httpapi/acl_test.go @@ -69,13 +69,14 @@ func TestFilePermissionAllows(t *testing.T) { t.Parallel() tests := []struct { - name string - permissions []string - workspaceID string - claims tokenClaims - action string - path string - want bool + name string + permissions []string + workspaceID string + claims tokenClaims + action string + path string + allowDescendants bool + want bool }{ { name: "no rules allows access (no ACL policy)", @@ -172,7 +173,7 @@ func TestFilePermissionAllows(t *testing.T) { want: true, }, { - name: "ignored read deny matches broader RelayAuth read grant", + name: "path-scoped read deny blocks read", permissions: []string{ "allow:scope:fs:read", "deny:scope:relayfile:fs:read:/ignored/*", @@ -212,6 +213,39 @@ func TestFilePermissionAllows(t *testing.T) { claims: tokenClaims{}, want: true, }, + { + name: "tree allow matches descendant-specific rule", + permissions: []string{"allow:scope:relayfile:fs:read:/allowed/document.md"}, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:/allowed/document.md": {}}, + }, + path: "/allowed", + allowDescendants: true, + want: true, + }, + { + name: "tree allow when wildcard claim spans descendant", + permissions: []string{"allow:scope:relayfile:fs:read:/allowed/document.md"}, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:*": {}}, + }, + path: "/allowed", + allowDescendants: true, + want: true, + }, + { + name: "tree allow when rule is wildcard but claim specific", + permissions: []string{"allow:scope:relayfile:fs:read:/allowed/*"}, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:/allowed/document.md": {}}, + }, + path: "/allowed", + allowDescendants: true, + want: true, + }, } for _, tt := range tests { @@ -227,7 +261,7 @@ func TestFilePermissionAllows(t *testing.T) { if path == "" { path = "/document.md" } - got := filePermissionAllows(tt.permissions, tt.workspaceID, &tt.claims, action, path) + got := filePermissionAllows(tt.permissions, tt.workspaceID, &tt.claims, action, path, tt.allowDescendants) if got != tt.want { t.Fatalf("expected %v, got %v", tt.want, got) } @@ -383,9 +417,14 @@ func TestIsValidACLRuleValue(t *testing.T) { {"scope with internal glob", "scope", "relayfile:fs:read:/protected/*/private", false}, {"scope with unsupported plane", "scope", "other:fs:read:/protected/*", false}, {"scope empty segment", "scope", "fs:", false}, + {"pathless relayfile scope", "scope", "relayfile:fs:read", false}, + {"pathless workspace scope", "scope", "workspace:relayfile-local:read", false}, {"valid workspace", "workspace", "ws_123", true}, {"workspace with uuid", "workspace", "abc-def-123", true}, {"workspace empty", "workspace", "", false}, + {"wildcard resource scope", "scope", "relayfile:*:read:/protected/*", true}, + {"wildcard plane scope", "scope", "*:fs:write:/protected/*", true}, + {"invalid wildcard combination", "scope", "*:other:read:/protected/*", false}, {"unknown kind", "unknown", "value", false}, } @@ -401,6 +440,15 @@ func TestIsValidACLRuleValue(t *testing.T) { } } +func TestACLPathlessNamespacedScopesIgnoreWildcardClaims(t *testing.T) { + claims := tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read:*": {}}, + } + if aclScopeRuleMatches("relayfile:fs:read", &claims, "read", "/docs/item.md") { + t.Fatalf("pathless namespaced scope should not match wildcard claim") + } +} + func TestParsePermissionRuleValidation(t *testing.T) { t.Parallel() diff --git a/internal/httpapi/github_tarball.go b/internal/httpapi/github_tarball.go index 1d634497..0ed33edd 100644 --- a/internal/httpapi/github_tarball.go +++ b/internal/httpapi/github_tarball.go @@ -626,7 +626,7 @@ func (s *Server) githubTarballWritePermissionError(workspaceID, workspacePath st Message: "failed to check file permissions", } } - if !filePermissionAllows(permissions, workspaceID, &claims, "write", workspacePath) { + if !filePermissionAllows(permissions, workspaceID, &claims, "write", workspacePath, false) { return &relayfile.BulkWriteError{ Code: "forbidden", Message: "file access denied by permission policy", diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 77816aef..f476b801 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -320,15 +320,19 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { writeError(w, authErr.status, authErr.code, authErr.message, getCorrelationID(r)) return } - if aclPath, includeTarget, ok := aclCheckPath(route, r); ok { - aclReader := s.aclGetFile(workspaceID) - if forkID := strings.TrimSpace(r.URL.Query().Get("forkId")); forkID != "" { - aclReader = s.aclGetForkFile(workspaceID, forkID) - } - permissions := resolveFilePermissionsWithTarget(aclReader, aclPath, includeTarget) - if !filePermissionAllows(permissions, workspaceID, &claims, strings.TrimPrefix(requiredScope, "fs:"), aclPath) { - writeError(w, http.StatusForbidden, "forbidden", "access denied by ACL", getCorrelationID(r)) - return + action, actionOK := fileActionFromScope(requiredScope) + if actionOK { + if aclPath, includeTarget, ok := aclCheckPath(route, r); ok { + aclReader := s.aclGetFile(workspaceID) + if forkID := strings.TrimSpace(r.URL.Query().Get("forkId")); forkID != "" { + aclReader = s.aclGetForkFile(workspaceID, forkID) + } + permissions := resolveFilePermissionsWithTarget(aclReader, aclPath, includeTarget) + allowDescendants := route == "tree" || route == "query_files" + if !filePermissionAllows(permissions, workspaceID, &claims, action, aclPath, allowDescendants) { + writeError(w, http.StatusForbidden, "forbidden", "access denied by ACL", getCorrelationID(r)) + return + } } } correlationID := getCorrelationID(r) @@ -1380,6 +1384,17 @@ func aclCheckPath(route string, r *http.Request) (string, bool, bool) { } } +func fileActionFromScope(requiredScope string) (string, bool) { + switch requiredScope { + case "fs:read": + return "read", true + case "fs:write": + return "write", true + default: + return "", false + } +} + func aclTargetExists(r *http.Request) bool { ifMatch := normalizeIfMatchHeader(r.Header.Get("If-Match")) return ifMatch != "" && ifMatch != "*" @@ -1483,7 +1498,7 @@ func validateForkCommitEntries(workspaceID string, claims tokenClaims, entries [ if !scopeMatchesPath(claims.Scopes, "fs:write", entry.Path) { return &forkCommitAuthorizationError{message: "fork commit denied by path scope"} } - if !filePermissionAllows(entry.Permissions, workspaceID, &claims, "write", entry.Path) { + if !filePermissionAllows(entry.Permissions, workspaceID, &claims, "write", entry.Path, false) { return &forkCommitAuthorizationError{message: "fork commit denied by permission policy"} } } @@ -1641,7 +1656,7 @@ func (s *Server) handleTree(w http.ResponseWriter, r *http.Request, workspaceID, } for _, item := range batch.Items { effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, item.Path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path, false) { continue } visibleFiles[item.Path] = struct{}{} @@ -1709,7 +1724,7 @@ func (s *Server) handleReadFile(w http.ResponseWriter, r *http.Request, workspac return } effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", path) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", path, false) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -1941,7 +1956,7 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { errorsOut = append(errorsOut, relayfile.BulkWriteError{ Path: path, Code: "forbidden", @@ -1951,7 +1966,7 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa } } else if readErr == relayfile.ErrNotFound || readErr == relayfile.ErrForkExpired { inheritedPermissions := s.resolveFilePermissions(workspaceID, forkID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path, false) { errorsOut = append(errorsOut, relayfile.BulkWriteError{ Path: path, Code: "forbidden", @@ -2016,7 +2031,7 @@ func (s *Server) handleExport(w http.ResponseWriter, r *http.Request, workspaceI continue } effectivePermissions := s.store.ResolveFilePermissions(workspaceID, file.Path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", file.Path) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", file.Path, false) { continue } visible = append(visible, file) @@ -2058,13 +2073,13 @@ func (s *Server) handleWriteFile(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } } else if readErr == relayfile.ErrNotFound || readErr == relayfile.ErrForkExpired { inheritedPermissions := s.resolveFilePermissions(workspaceID, forkID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path, false) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2143,13 +2158,13 @@ func (s *Server) handleMergeFile(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.store.ReadFile(workspaceID, path) if readErr == nil { existingPermissions := s.store.ResolveFilePermissions(workspaceID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } } else if readErr == relayfile.ErrNotFound { inheritedPermissions := s.store.ResolveFilePermissions(workspaceID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path, false) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2232,7 +2247,7 @@ func (s *Server) handleDeleteFile(w http.ResponseWriter, r *http.Request, worksp _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2368,7 +2383,7 @@ func (s *Server) handleQueryFiles(w http.ResponseWriter, r *http.Request, worksp if permission != "" && !stringSliceContainsExact(effectivePermissions, permission) { continue } - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path, false) { continue } items = append(items, item) diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index 8649b532..dfa9da51 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -3133,6 +3133,61 @@ func TestTreeEndpointFiltersUnauthorizedFiles(t *testing.T) { } } +func TestTreeEndpointAllowsDescendantScopedACL(t *testing.T) { + server := NewServer(relayfile.NewStore()) + workspaceID := "ws_tree_descendant" + ownerToken := mustTestJWT(t, "dev-secret", workspaceID, "Owner", []string{"relayfile:fs:read:*", "relayfile:fs:write:*"}, time.Now().Add(time.Hour)) + limitedToken := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"relayfile:fs:read:/allowed/**"}, time.Now().Add(time.Hour)) + + writeFileForTest(t, server, ownerToken, workspaceID, "/allowed/document.md", "0", "descendant", "corr_tree_descendant_file") + + aclWrite := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/allowed/.relayfile.acl", + headers: map[string]string{ + "Authorization": "Bearer " + ownerToken, + "X-Correlation-Id": "corr_tree_descendant_acl", + "If-Match": "0", + }, + body: map[string]any{ + "contentType": "text/plain", + "content": "marker", + "semantics": map[string]any{ + "permissions": []string{"allow:scope:relayfile:fs:read:/allowed/document.md"}, + }, + }, + }) + if aclWrite.Code != http.StatusAccepted { + t.Fatalf("expected ACL marker write 202, got %d (%s)", aclWrite.Code, aclWrite.Body.String()) + } + + treeResp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/tree?path=/allowed", + headers: map[string]string{ + "Authorization": "Bearer " + limitedToken, + "X-Correlation-Id": "corr_tree_descendant", + }, + }) + if treeResp.Code != http.StatusOK { + t.Fatalf("expected tree 200 for descendant ACL, got %d (%s)", treeResp.Code, treeResp.Body.String()) + } + var tree relayfile.TreeResponse + if err := json.NewDecoder(treeResp.Body).Decode(&tree); err != nil { + t.Fatalf("decode tree response: %v", err) + } + found := false + for _, entry := range tree.Entries { + if entry.Path == "/allowed/document.md" { + found = true + break + } + } + if !found { + t.Fatalf("expected descendant file in tree entries, got %+v", tree.Entries) + } +} + func TestFilePermissionPolicyDenyOverridesAllowAndPublic(t *testing.T) { server := NewServer(relayfile.NewStore()) ownerToken := mustTestJWT(t, "dev-secret", "ws_perm_deny", "Owner", []string{"fs:read", "fs:write", "finance"}, time.Now().Add(time.Hour)) @@ -3359,6 +3414,23 @@ func TestFilePermissionPolicyPathOverridesWithRelayAuthScopes(t *testing.T) { t.Fatalf("expected readonly path write 403, got %d (%s)", protectedWrite.Code, protectedWrite.Body.String()) } + relativeProtectedWrite := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=protected/document.md", + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_perm_relative_2", + "If-Match": protected.TargetRevision, + }, + body: map[string]any{ + "contentType": "text/markdown", + "content": "still protected", + }, + }) + if relativeProtectedWrite.Code != http.StatusForbidden { + t.Fatalf("expected normalized relative write 403, got %d (%s)", relativeProtectedWrite.Code, relativeProtectedWrite.Body.String()) + } + ignoredRead := doRequest(t, server, request{ method: http.MethodGet, path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/ignored/document.md", @@ -3403,6 +3475,73 @@ func TestFilePermissionPolicyPathOverridesWithRelayAuthScopes(t *testing.T) { } } +func TestCloudProducerScopeShape(t *testing.T) { + server := NewServer(relayfile.NewStore()) + workspaceID := "ws_cloud_scope_shape" + ownerToken := mustTestJWT(t, "dev-secret", workspaceID, "Owner", []string{"fs:read", "fs:write"}, time.Now().Add(time.Hour)) + workerToken := mustTestJWT(t, "dev-secret", workspaceID, "Worker", []string{"relayfile:fs:read:*", "relayfile:fs:write:*"}, time.Now().Add(time.Hour)) + + writeFileForTest(t, server, ownerToken, workspaceID, "/protected/Document.md", "0", "protected", "corr_cloud_scope_protected") + writeFileForTest(t, server, ownerToken, workspaceID, "/secret/Doc.md", "0", "secret", "corr_cloud_scope_secret") + writeFileForTest(t, server, ownerToken, workspaceID, "/public/Doc.md", "0", "public", "corr_cloud_scope_public") + + markerResp := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/.relayfile.acl", + headers: map[string]string{ + "Authorization": "Bearer " + ownerToken, + "X-Correlation-Id": "corr_cloud_scope_acl", + "If-Match": "0", + }, + body: map[string]any{ + "contentType": "text/plain", + "content": "cloud marker", + "semantics": map[string]any{ + "permissions": []string{ + "allow:scope:workspace:relayfile-local:read:*", + "allow:scope:workspace:relayfile-local:write:*", + "deny:scope:relayfile:fs:write:/protected/*", + "deny:scope:relayfile:fs:read:/secret/*", + "deny:scope:relayfile:fs:write:/secret/*", + }, + }, + }, + }) + if markerResp.Code != http.StatusAccepted { + t.Fatalf("expected ACL marker 202, got %d (%s)", markerResp.Code, markerResp.Body.String()) + } + + for _, tt := range []struct { + name string + path string + method string + wantStatus int + }{ + {"protected read", "/protected/Document.md", http.MethodGet, http.StatusOK}, + {"protected write", "/protected/Document.md", http.MethodPut, http.StatusForbidden}, + {"secret read", "/secret/Doc.md", http.MethodGet, http.StatusForbidden}, + {"secret write", "/secret/Doc.md", http.MethodPut, http.StatusForbidden}, + {"public read", "/public/Doc.md", http.MethodGet, http.StatusOK}, + } { + req := request{ + method: tt.method, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=" + tt.path, + headers: map[string]string{ + "Authorization": "Bearer " + workerToken, + "X-Correlation-Id": "corr_cloud_scope_" + tt.name, + }, + } + if tt.method == http.MethodPut { + req.headers["If-Match"] = "0" + req.body = map[string]any{"contentType": "text/markdown", "content": "test"} + } + resp := doRequest(t, server, req) + if resp.Code != tt.wantStatus { + t.Fatalf("%s: expected %d, got %d (%s)", tt.name, tt.wantStatus, resp.Code, resp.Body.String()) + } + } +} + func TestOpsListProviderFilterEndpoint(t *testing.T) { store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{ Adapters: []relayfile.ProviderAdapter{ From c874e09fe4404c879caf5a7c7b8e62501eab8d73 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 10:36:05 +0200 Subject: [PATCH 03/12] fix(auth): preserve exact scopes across current bulk reads --- internal/httpapi/acl.go | 3 --- internal/httpapi/acl_test.go | 8 +++++++- internal/httpapi/server.go | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/internal/httpapi/acl.go b/internal/httpapi/acl.go index 63cfc0c3..c365b444 100644 --- a/internal/httpapi/acl.go +++ b/internal/httpapi/acl.go @@ -80,9 +80,6 @@ func isValidACLRuleValue(kind, value string) bool { case "agent": return aclAgentNamePattern.MatchString(value) case "scope": - if strings.HasPrefix(value, "relayfile:") || strings.HasPrefix(value, "workspace:") || strings.HasPrefix(value, "*:") { - return isValidACLFilesystemScope(value) - } return aclScopePattern.MatchString(value) || isValidACLFilesystemScope(value) case "workspace": return aclWorkspacePattern.MatchString(value) diff --git a/internal/httpapi/acl_test.go b/internal/httpapi/acl_test.go index e02ff50e..61ce86ca 100644 --- a/internal/httpapi/acl_test.go +++ b/internal/httpapi/acl_test.go @@ -417,7 +417,7 @@ func TestIsValidACLRuleValue(t *testing.T) { {"scope with internal glob", "scope", "relayfile:fs:read:/protected/*/private", false}, {"scope with unsupported plane", "scope", "other:fs:read:/protected/*", false}, {"scope empty segment", "scope", "fs:", false}, - {"pathless relayfile scope", "scope", "relayfile:fs:read", false}, + {"pathless relayfile scope remains a valid exact tag", "scope", "relayfile:fs:read", true}, {"pathless workspace scope", "scope", "workspace:relayfile-local:read", false}, {"valid workspace", "workspace", "ws_123", true}, {"workspace with uuid", "workspace", "abc-def-123", true}, @@ -447,6 +447,12 @@ func TestACLPathlessNamespacedScopesIgnoreWildcardClaims(t *testing.T) { if aclScopeRuleMatches("relayfile:fs:read", &claims, "read", "/docs/item.md") { t.Fatalf("pathless namespaced scope should not match wildcard claim") } + exactClaims := tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:read": {}}, + } + if !aclScopeRuleMatches("relayfile:fs:read", &exactClaims, "read", "/docs/item.md") { + t.Fatalf("pathless namespaced scope should preserve exact-tag matching") + } } func TestParsePermissionRuleValidation(t *testing.T) { diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index f476b801..eefdf96f 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -1851,7 +1851,7 @@ func (s *Server) handleBulkRead(w http.ResponseWriter, r *http.Request, workspac // prevents an ACL-denied caller from distinguishing an existing file // from a missing one through the per-file status. permissions := resolveFilePermissionsWithTarget(cachedACLReader, path, true) - if !filePermissionAllows(permissions, workspaceID, &claims) { + if !filePermissionAllows(permissions, workspaceID, &claims, "read", path, false) { results = append(results, bulkReadError(path, http.StatusForbidden, "forbidden", "file access denied by permission policy")) continue } @@ -1872,7 +1872,7 @@ func (s *Server) handleBulkRead(w http.ResponseWriter, r *http.Request, workspac // snapshot before returning content so a concurrent target permission // tightening cannot authorize the old snapshot and expose the new file. freshPermissions := resolveBulkReadPermissionsForReturnedFile(aclReader, path, file) - if !filePermissionAllows(freshPermissions, workspaceID, &claims) { + if !filePermissionAllows(freshPermissions, workspaceID, &claims, "read", path, false) { results = append(results, bulkReadError(path, http.StatusForbidden, "forbidden", "file access denied by permission policy")) continue } From c5c5e065e4a3d6f6c9cb7f93b53e03b2df439a59 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 10:57:27 +0200 Subject: [PATCH 04/12] fix(auth): filter directory ACLs per returned file Session-Id: 01a080a8-9019-7573-9993-a17d22288ad5 --- internal/httpapi/acl.go | 48 +-- internal/httpapi/acl_test.go | 50 +-- internal/httpapi/github_tarball.go | 2 +- internal/httpapi/server.go | 288 +++++++++++------- internal/httpapi/server_test.go | 328 +++++++++++++++++++- internal/relayfile/store.go | 468 ++++++++++++++--------------- internal/relayfile/store_test.go | 28 ++ openapi/relayfile-v1.openapi.yaml | 4 + 8 files changed, 780 insertions(+), 436 deletions(-) diff --git a/internal/httpapi/acl.go b/internal/httpapi/acl.go index c365b444..63995320 100644 --- a/internal/httpapi/acl.go +++ b/internal/httpapi/acl.go @@ -155,12 +155,9 @@ func isValidACLFilesystemScope(scope string) bool { // filesystem action and path. Scope rules are semantic: a durable rule such // as relayfile:fs:write:/protected/* matches a delegated token carrying the // broader relayfile:fs:write:* grant without requiring the rule itself to be -// copied into the token. When allowDescendants is true the function also -// considers allow rules whose scoped path sits beneath the requested path if -// the agent holds a matching scope for the descendant detail (used by tree/query -// preflight checks). +// copied into the token. // Returns true if access is allowed. -func filePermissionAllows(permissions []string, workspaceID string, claims *tokenClaims, requiredAction, requestedPath string, allowDescendants bool) bool { +func filePermissionAllows(permissions []string, workspaceID string, claims *tokenClaims, requiredAction, requestedPath string) bool { if len(permissions) == 0 { // No ACL policy in effect — allow access. return true @@ -187,9 +184,6 @@ func filePermissionAllows(permissions []string, workspaceID string, claims *toke match = true case "scope": match = aclScopeRuleMatches(rule.Value, claims, requiredAction, requestedPath) - if !match && allowDescendants && rule.Effect == "allow" { - match = aclScopeRuleMatchesDescendant(rule.Value, claims, requiredAction, requestedPath) - } case "agent": match = claims != nil && claims.AgentName == rule.Value case "workspace": @@ -234,44 +228,6 @@ func aclScopeRuleMatches(scope string, claims *tokenClaims, requiredAction, requ return scopeMatchesPath(claims.Scopes, "fs:"+requiredAction, requestedPath) } -func aclScopeRuleMatchesDescendant(scope string, claims *tokenClaims, requiredAction, requestedPath string) bool { - if claims == nil || requestedPath == "" { - return false - } - - parsed, filesystemScope := parseACLFilesystemScope(scope) - if !filesystemScope || parsed.path == "*" { - return false - } - if !scopeActionMatches(parsed.action, requiredAction) { - return false - } - - normalizedRulePath := normalizeScopePath(parsed.path) - normalizedRequestPath := normalizeACLPath(requestedPath) - for claimScope := range claims.Scopes { - claimPath, ok := pathScopeForRequired(claimScope, "fs", requiredAction) - if !ok { - continue - } - normalizedClaimPath := normalizeScopePath(claimPath) - if normalizedClaimPath != "*" && !withinBasePath(normalizedRequestPath, normalizedClaimPath) { - continue - } - if scopePathMatches(normalizedRulePath, normalizedClaimPath) || scopePathMatches(normalizedClaimPath, normalizedRulePath) { - return true - } - } - return false -} - -func normalizeScopePath(path string) string { - if path == "*" { - return "*" - } - return normalizeACLPath(path) -} - // resolveFilePermissions walks ancestor dirs to collect ACL rules. // store is an interface that can read files from the workspace. func resolveFilePermissions(getFile func(path string) ([]byte, error), path string) []string { diff --git a/internal/httpapi/acl_test.go b/internal/httpapi/acl_test.go index 61ce86ca..b2b7addc 100644 --- a/internal/httpapi/acl_test.go +++ b/internal/httpapi/acl_test.go @@ -69,14 +69,13 @@ func TestFilePermissionAllows(t *testing.T) { t.Parallel() tests := []struct { - name string - permissions []string - workspaceID string - claims tokenClaims - action string - path string - allowDescendants bool - want bool + name string + permissions []string + workspaceID string + claims tokenClaims + action string + path string + want bool }{ { name: "no rules allows access (no ACL policy)", @@ -213,39 +212,6 @@ func TestFilePermissionAllows(t *testing.T) { claims: tokenClaims{}, want: true, }, - { - name: "tree allow matches descendant-specific rule", - permissions: []string{"allow:scope:relayfile:fs:read:/allowed/document.md"}, - workspaceID: "ws_123", - claims: tokenClaims{ - Scopes: map[string]struct{}{"relayfile:fs:read:/allowed/document.md": {}}, - }, - path: "/allowed", - allowDescendants: true, - want: true, - }, - { - name: "tree allow when wildcard claim spans descendant", - permissions: []string{"allow:scope:relayfile:fs:read:/allowed/document.md"}, - workspaceID: "ws_123", - claims: tokenClaims{ - Scopes: map[string]struct{}{"relayfile:fs:read:*": {}}, - }, - path: "/allowed", - allowDescendants: true, - want: true, - }, - { - name: "tree allow when rule is wildcard but claim specific", - permissions: []string{"allow:scope:relayfile:fs:read:/allowed/*"}, - workspaceID: "ws_123", - claims: tokenClaims{ - Scopes: map[string]struct{}{"relayfile:fs:read:/allowed/document.md": {}}, - }, - path: "/allowed", - allowDescendants: true, - want: true, - }, } for _, tt := range tests { @@ -261,7 +227,7 @@ func TestFilePermissionAllows(t *testing.T) { if path == "" { path = "/document.md" } - got := filePermissionAllows(tt.permissions, tt.workspaceID, &tt.claims, action, path, tt.allowDescendants) + got := filePermissionAllows(tt.permissions, tt.workspaceID, &tt.claims, action, path) if got != tt.want { t.Fatalf("expected %v, got %v", tt.want, got) } diff --git a/internal/httpapi/github_tarball.go b/internal/httpapi/github_tarball.go index 0ed33edd..1d634497 100644 --- a/internal/httpapi/github_tarball.go +++ b/internal/httpapi/github_tarball.go @@ -626,7 +626,7 @@ func (s *Server) githubTarballWritePermissionError(workspaceID, workspacePath st Message: "failed to check file permissions", } } - if !filePermissionAllows(permissions, workspaceID, &claims, "write", workspacePath, false) { + if !filePermissionAllows(permissions, workspaceID, &claims, "write", workspacePath) { return &relayfile.BulkWriteError{ Code: "forbidden", Message: "file access denied by permission policy", diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index eefdf96f..3f2bd6d5 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -23,12 +23,13 @@ import ( ) const ( - maxBulkReadPaths = 32 - maxBulkReadRequestBytes = 64 << 10 - maxBulkReadPathBytes = 4096 - maxBulkReadPathsBytes = 32 << 10 - maxBulkReadContentBytes = 32 << 20 - maxBulkReadResponseBytes = 64 << 20 + maxBulkReadPaths = 32 + maxBulkReadRequestBytes = 64 << 10 + maxBulkReadPathBytes = 4096 + maxBulkReadPathsBytes = 32 << 10 + maxBulkReadContentBytes = 32 << 20 + maxBulkReadResponseBytes = 64 << 20 + maxTreeEntriesPerHTTPPage = 1000 ) type ServerConfig struct { @@ -328,8 +329,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { aclReader = s.aclGetForkFile(workspaceID, forkID) } permissions := resolveFilePermissionsWithTarget(aclReader, aclPath, includeTarget) - allowDescendants := route == "tree" || route == "query_files" - if !filePermissionAllows(permissions, workspaceID, &claims, action, aclPath, allowDescendants) { + if !filePermissionAllows(permissions, workspaceID, &claims, action, aclPath) { writeError(w, http.StatusForbidden, "forbidden", "access denied by ACL", getCorrelationID(r)) return } @@ -1372,10 +1372,10 @@ func aclCheckPath(route string, r *http.Request) (string, bool, bool) { return path, aclTargetExists(r), true case "merge_file": return path, true, true - case "tree", "query_files": - // Directory-level operations: check ACL for the path prefix. - // Handlers also do per-file ACL filtering as a second layer of defense. - return path, false, true + // tree and query_files deliberately skip a coarse path-prefix preflight. + // Their handlers enforce ACLs on every returned file so mixed-policy + // directories produce a filtered 200 response rather than leaking entries + // or rejecting the whole directory because one child carries a tag ACL. // bulk_write: ACL is checked per-file inside handleBulkWrite. // fs_ws: auth is handled in handleFileEventsWebSocket. // export: per-file ACL filtering in handleExport. @@ -1498,7 +1498,7 @@ func validateForkCommitEntries(workspaceID string, claims tokenClaims, entries [ if !scopeMatchesPath(claims.Scopes, "fs:write", entry.Path) { return &forkCommitAuthorizationError{message: "fork commit denied by path scope"} } - if !filePermissionAllows(entry.Permissions, workspaceID, &claims, "write", entry.Path, false) { + if !filePermissionAllows(entry.Permissions, workspaceID, &claims, "write", entry.Path) { return &forkCommitAuthorizationError{message: "fork commit denied by permission policy"} } } @@ -1621,96 +1621,157 @@ func (s *Server) handleTree(w http.ResponseWriter, r *http.Request, workspaceID, } depth := parseBoundedInt(r.URL.Query().Get("depth"), 10, 1, 100) forkID := strings.TrimSpace(r.URL.Query().Get("forkId")) - var resp relayfile.TreeResponse - var err error - if forkID != "" { - resp, err = s.store.ListForkTree(workspaceID, forkID, path, depth, r.URL.Query().Get("cursor")) - } else { - resp, err = s.store.ListTree(workspaceID, path, depth, r.URL.Query().Get("cursor")) + requestedCursor := r.URL.Query().Get("cursor") + listPage := func(cursor string) (relayfile.TreeResponse, error) { + if forkID != "" { + return s.store.ListForkTree(workspaceID, forkID, path, depth, cursor) + } + return s.store.ListTree(workspaceID, path, depth, cursor) } + firstPage, err := listPage("") if err != nil { writeForkAwareError(w, err, correlationID) return } - if len(resp.Entries) > 0 || resp.TotalFiles > 0 { - base := normalizeRoutePath(resp.Path) - visibleFiles := map[string]struct{}{} - visibleDirs := map[string]struct{}{} - cursor := "" - for { - queryReq := relayfile.FileQueryRequest{ - PathPrefix: base, - Cursor: cursor, - Limit: 200, - } - var batch relayfile.FileQueryResponse - var queryErr error - if forkID != "" { - batch, queryErr = s.store.QueryForkFiles(workspaceID, forkID, queryReq) + base := normalizeRoutePath(firstPage.Path) + totalFiles, err := s.countVisibleTreeFiles(workspaceID, forkID, base, claims) + if err != nil { + writeError(w, http.StatusInternalServerError, "internal_error", err.Error(), correlationID) + return + } + + cursorFound := requestedCursor == "" + visibleEntries := make([]relayfile.TreeEntry, 0, maxTreeEntriesPerHTTPPage) + resp := relayfile.TreeResponse{Path: base, TotalFiles: totalFiles} + rawPage := firstPage + rawCursor := "" + seenCursors := map[string]struct{}{} + for { + for _, entry := range rawPage.Entries { + visible := false + if entry.Type == "file" { + visible = s.treeFileVisible(workspaceID, forkID, entry.Path, claims) } else { - batch, queryErr = s.store.QueryFiles(workspaceID, queryReq) - } - if queryErr != nil { - writeError(w, http.StatusInternalServerError, "internal_error", queryErr.Error(), correlationID) - return - } - for _, item := range batch.Items { - effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, item.Path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path, false) { - continue - } - visibleFiles[item.Path] = struct{}{} - dirPath := item.Path - for { - lastSlash := strings.LastIndex(dirPath, "/") - if lastSlash <= 0 { - break - } - dirPath = dirPath[:lastSlash] - if dirPath == "" { - dirPath = "/" - } - if !withinBasePath(base, dirPath) { - break - } - if dirPath != "/" { - visibleDirs[dirPath] = struct{}{} - } - if dirPath == base || dirPath == "/" { - break - } + visible, err = s.treeDirectoryVisible(workspaceID, forkID, entry.Path, claims) + if err != nil { + writeError(w, http.StatusInternalServerError, "internal_error", err.Error(), correlationID) + return } } - if batch.NextCursor == nil || *batch.NextCursor == "" { - break - } - nextCursor := *batch.NextCursor - if nextCursor == cursor { - break + if !visible { + continue } - cursor = nextCursor - } - filtered := make([]relayfile.TreeEntry, 0, len(resp.Entries)) - for _, entry := range resp.Entries { - if entry.Type == "file" { - if _, ok := visibleFiles[entry.Path]; ok { - filtered = append(filtered, entry) + if !cursorFound { + if entry.Path == requestedCursor { + cursorFound = true } continue } - if _, ok := visibleDirs[entry.Path]; ok { - filtered = append(filtered, entry) + visibleEntries = append(visibleEntries, entry) + if len(visibleEntries) > maxTreeEntriesPerHTTPPage { + visibleEntries = visibleEntries[:maxTreeEntriesPerHTTPPage] + nextCursor := visibleEntries[len(visibleEntries)-1].Path + resp.NextCursor = &nextCursor + resp.Entries = visibleEntries + writeJSON(w, http.StatusOK, resp) + return } } - resp.Entries = filtered - // Store-level totals include every file below the requested path. The - // HTTP contract must expose only files this caller can see, while still - // keeping the total stable across pagination pages. - resp.TotalFiles = len(visibleFiles) + if rawPage.NextCursor == nil || *rawPage.NextCursor == "" { + break + } + nextCursor := *rawPage.NextCursor + if nextCursor == rawCursor { + writeError(w, http.StatusInternalServerError, "internal_error", "tree pagination did not advance", correlationID) + return + } + if _, duplicate := seenCursors[nextCursor]; duplicate { + writeError(w, http.StatusInternalServerError, "internal_error", "tree pagination repeated a cursor", correlationID) + return + } + seenCursors[nextCursor] = struct{}{} + rawCursor = nextCursor + rawPage, err = listPage(rawCursor) + if err != nil { + writeForkAwareError(w, err, correlationID) + return + } } + if !cursorFound { + writeError(w, http.StatusBadRequest, "bad_request", relayfile.ErrInvalidInput.Error(), correlationID) + return + } + resp.Entries = visibleEntries writeJSON(w, http.StatusOK, resp) } +func (s *Server) treeFileVisible(workspaceID, forkID, filePath string, claims tokenClaims) bool { + effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, filePath, true) + return scopeMatchesPath(claims.Scopes, "fs:read", filePath) && + filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", filePath) +} + +func (s *Server) countVisibleTreeFiles(workspaceID, forkID, base string, claims tokenClaims) (int, error) { + cursor := "" + count := 0 + for { + queryReq := relayfile.FileQueryRequest{PathPrefix: base, Cursor: cursor, Limit: maxTreeEntriesPerHTTPPage} + var batch relayfile.FileQueryResponse + var err error + if forkID != "" { + batch, err = s.store.QueryForkFiles(workspaceID, forkID, queryReq) + } else { + batch, err = s.store.QueryFiles(workspaceID, queryReq) + } + if err != nil { + return 0, err + } + for _, item := range batch.Items { + if s.treeFileVisible(workspaceID, forkID, item.Path, claims) { + count++ + } + } + if batch.NextCursor == nil || *batch.NextCursor == "" { + return count, nil + } + next := *batch.NextCursor + if next == cursor { + return 0, fmt.Errorf("tree file pagination did not advance") + } + cursor = next + } +} + +func (s *Server) treeDirectoryVisible(workspaceID, forkID, directoryPath string, claims tokenClaims) (bool, error) { + cursor := "" + for { + queryReq := relayfile.FileQueryRequest{PathPrefix: directoryPath, Cursor: cursor, Limit: maxTreeEntriesPerHTTPPage} + var batch relayfile.FileQueryResponse + var err error + if forkID != "" { + batch, err = s.store.QueryForkFiles(workspaceID, forkID, queryReq) + } else { + batch, err = s.store.QueryFiles(workspaceID, queryReq) + } + if err != nil { + return false, err + } + for _, item := range batch.Items { + if s.treeFileVisible(workspaceID, forkID, item.Path, claims) { + return true, nil + } + } + if batch.NextCursor == nil || *batch.NextCursor == "" { + return false, nil + } + next := *batch.NextCursor + if next == cursor { + return false, fmt.Errorf("tree directory pagination did not advance") + } + cursor = next + } +} + func (s *Server) handleReadFile(w http.ResponseWriter, r *http.Request, workspaceID, correlationID string, claims tokenClaims) { path := r.URL.Query().Get("path") if path == "" { @@ -1724,7 +1785,7 @@ func (s *Server) handleReadFile(w http.ResponseWriter, r *http.Request, workspac return } effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", path, false) { + if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -1851,7 +1912,7 @@ func (s *Server) handleBulkRead(w http.ResponseWriter, r *http.Request, workspac // prevents an ACL-denied caller from distinguishing an existing file // from a missing one through the per-file status. permissions := resolveFilePermissionsWithTarget(cachedACLReader, path, true) - if !filePermissionAllows(permissions, workspaceID, &claims, "read", path, false) { + if !filePermissionAllows(permissions, workspaceID, &claims, "read", path) { results = append(results, bulkReadError(path, http.StatusForbidden, "forbidden", "file access denied by permission policy")) continue } @@ -1872,7 +1933,7 @@ func (s *Server) handleBulkRead(w http.ResponseWriter, r *http.Request, workspac // snapshot before returning content so a concurrent target permission // tightening cannot authorize the old snapshot and expose the new file. freshPermissions := resolveBulkReadPermissionsForReturnedFile(aclReader, path, file) - if !filePermissionAllows(freshPermissions, workspaceID, &claims, "read", path, false) { + if !filePermissionAllows(freshPermissions, workspaceID, &claims, "read", path) { results = append(results, bulkReadError(path, http.StatusForbidden, "forbidden", "file access denied by permission policy")) continue } @@ -1956,7 +2017,7 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { errorsOut = append(errorsOut, relayfile.BulkWriteError{ Path: path, Code: "forbidden", @@ -1966,7 +2027,7 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa } } else if readErr == relayfile.ErrNotFound || readErr == relayfile.ErrForkExpired { inheritedPermissions := s.resolveFilePermissions(workspaceID, forkID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { errorsOut = append(errorsOut, relayfile.BulkWriteError{ Path: path, Code: "forbidden", @@ -2014,14 +2075,17 @@ func (s *Server) handleExport(w http.ResponseWriter, r *http.Request, workspaceI if exportRoot == "" { exportRoot = "/" } + forkID := strings.TrimSpace(r.URL.Query().Get("forkId")) - files, err := s.store.ExportWorkspace(workspaceID) + var files []relayfile.File + var err error + if forkID != "" { + files, err = s.store.ExportForkWorkspace(workspaceID, forkID) + } else { + files, err = s.store.ExportWorkspace(workspaceID) + } if err != nil { - if err == relayfile.ErrInvalidInput { - writeError(w, http.StatusBadRequest, "bad_request", err.Error(), correlationID) - return - } - writeError(w, http.StatusInternalServerError, "internal_error", err.Error(), correlationID) + writeForkAwareError(w, err, correlationID) return } @@ -2030,8 +2094,9 @@ func (s *Server) handleExport(w http.ResponseWriter, r *http.Request, workspaceI if !withinBasePath(exportRoot, file.Path) { continue } - effectivePermissions := s.store.ResolveFilePermissions(workspaceID, file.Path, true) - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", file.Path, false) { + effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, file.Path, true) + if !scopeMatchesPath(claims.Scopes, "fs:read", file.Path) || + !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", file.Path) { continue } visible = append(visible, file) @@ -2073,13 +2138,13 @@ func (s *Server) handleWriteFile(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } } else if readErr == relayfile.ErrNotFound || readErr == relayfile.ErrForkExpired { inheritedPermissions := s.resolveFilePermissions(workspaceID, forkID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2158,13 +2223,13 @@ func (s *Server) handleMergeFile(w http.ResponseWriter, r *http.Request, workspa _, readErr := s.store.ReadFile(workspaceID, path) if readErr == nil { existingPermissions := s.store.ResolveFilePermissions(workspaceID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } } else if readErr == relayfile.ErrNotFound { inheritedPermissions := s.store.ResolveFilePermissions(workspaceID, path, false) - if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(inheritedPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2247,7 +2312,7 @@ func (s *Server) handleDeleteFile(w http.ResponseWriter, r *http.Request, worksp _, readErr := s.readFile(workspaceID, forkID, path) if readErr == nil { existingPermissions := s.resolveFilePermissions(workspaceID, forkID, path, true) - if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path, false) { + if !filePermissionAllows(existingPermissions, workspaceID, &claims, "write", path) { writeError(w, http.StatusForbidden, "forbidden", "file access denied by permission policy", correlationID) return } @@ -2345,6 +2410,20 @@ func (s *Server) handleQueryFiles(w http.ResponseWriter, r *http.Request, worksp permission := strings.TrimSpace(r.URL.Query().Get("permission")) comment := r.URL.Query().Get("comment") cursor := r.URL.Query().Get("cursor") + if strings.TrimSpace(cursor) != "" { + // Store pagination validates that the cursor names an existing file, + // but it cannot see the caller's ACL. Reject a real cursor that is + // hidden by ACL instead of silently treating it as a valid page edge. + cursorPath := normalizeRoutePath(cursor) + if _, readErr := s.readFile(workspaceID, forkID, cursorPath); readErr == nil { + effectivePermissions := s.resolveFilePermissions(workspaceID, forkID, cursorPath, true) + if !scopeMatchesPath(claims.Scopes, "fs:read", cursorPath) || + !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", cursorPath) { + writeError(w, http.StatusBadRequest, "bad_request", relayfile.ErrInvalidInput.Error(), correlationID) + return + } + } + } items := make([]relayfile.FileQueryItem, 0, limit) var nextCursor *string @@ -2383,7 +2462,8 @@ func (s *Server) handleQueryFiles(w http.ResponseWriter, r *http.Request, worksp if permission != "" && !stringSliceContainsExact(effectivePermissions, permission) { continue } - if !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path, false) { + if !scopeMatchesPath(claims.Scopes, "fs:read", item.Path) || + !filePermissionAllows(effectivePermissions, workspaceID, &claims, "read", item.Path) { continue } items = append(items, item) diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index dfa9da51..8a9af702 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -2254,6 +2254,51 @@ func TestExportJSONPathFilter(t *testing.T) { } } +func TestForkExportUsesOverlayDeletionAndACLMarkers(t *testing.T) { + server := newForkTestServer(t) + workspaceID := "ws_export_fork_overlay" + token := forkTestToken(t, workspaceID) + parent := writeFileForTest(t, server, token, workspaceID, "/docs/Parent.md", "0", "parent", "corr_export_fork_parent") + writeFileForTest(t, server, token, workspaceID, "/docs/Keep.md", "0", "keep", "corr_export_fork_keep") + marker := writeFileForTest(t, server, token, workspaceID, "/docs/.relayfile.acl", "0", "parent marker", "corr_export_fork_marker") + fork := createForkForTest(t, server, token, workspaceID, "proposal-export-fork", nil) + + deleteResp := doRequest(t, server, request{ + method: http.MethodDelete, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/docs/Parent.md&forkId=" + fork.ForkID, + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_export_fork_delete", + "If-Match": parent.TargetRevision, + }, + }) + if deleteResp.Code != http.StatusAccepted { + t.Fatalf("expected fork delete 202, got %d (%s)", deleteResp.Code, deleteResp.Body.String()) + } + writeForkFileForTest(t, server, token, workspaceID, fork.ForkID, "/docs/.relayfile.acl", marker.TargetRevision, "fork marker", "corr_export_fork_marker_overlay") + + for _, format := range []string{"json", "tar", "patch"} { + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/export?format=" + format + "&forkId=" + fork.ForkID, + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_export_fork_" + format, + }, + }) + if resp.Code != http.StatusOK { + t.Fatalf("expected fork %s export 200, got %d (%s)", format, resp.Code, resp.Body.String()) + } + body := resp.Body.String() + if strings.Contains(body, "parent") || strings.Contains(body, "Parent.md") { + t.Fatalf("fork %s export leaked deleted parent file: %s", format, body) + } + if !strings.Contains(body, "Keep.md") && format != "tar" { + t.Fatalf("fork %s export omitted retained overlay view: %s", format, body) + } + } +} + func TestExportEnforcesPathScopedMountGrant(t *testing.T) { store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) t.Cleanup(store.Close) @@ -2714,6 +2759,206 @@ func TestTreeEndpointPaginatesBoundedEntries(t *testing.T) { } } +func TestCollectionEndpointsDoNotExpandExactPathScopes(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + server := NewServer(store) + workspaceID := "ws_collection_exact_scope" + for index, seed := range []struct { + path string + content string + permissions []string + }{ + {path: "/private/Plain.md", content: "plain secret"}, + {path: "/private/Public.md", content: "public secret", permissions: []string{"public"}}, + {path: "/private/Tagged.md", content: "tagged secret", permissions: []string{"role:finance"}}, + } { + if _, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, Path: seed.path, IfMatch: "0", ContentType: "text/markdown", + Content: seed.content, Semantics: relayfile.FileSemantics{Permissions: seed.permissions}, + CorrelationID: fmt.Sprintf("corr_collection_exact_seed_%d", index), + }); err != nil { + t.Fatalf("seed write failed for %s: %v", seed.path, err) + } + } + fork, err := store.CreateFork(workspaceID, "proposal-collection-exact-scope", 3600) + if err != nil { + t.Fatalf("create fork: %v", err) + } + + for _, scopePath := range []string{"/", "/private"} { + scopePath := scopePath + t.Run(scopePath, func(t *testing.T) { + token := mustTestJWT(t, "dev-secret", workspaceID, "ExactScope", []string{"relayfile:fs:read:" + scopePath}, time.Now().Add(time.Hour)) + requestPath := url.QueryEscape(scopePath) + for _, forkQuery := range []string{"", "&forkId=" + url.QueryEscape(fork.ForkID)} { + for _, endpoint := range []string{"tree", "query"} { + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/" + endpoint + "?path=" + requestPath + forkQuery, + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_collection_exact_" + endpoint, + }, + }) + if resp.Code != http.StatusOK { + t.Fatalf("expected %s%s 200, got %d (%s)", endpoint, forkQuery, resp.Code, resp.Body.String()) + } + if endpoint == "tree" { + var payload relayfile.TreeResponse + if err := json.NewDecoder(resp.Body).Decode(&payload); err != nil { + t.Fatalf("decode tree response: %v", err) + } + if len(payload.Entries) != 0 || payload.TotalFiles != 0 || payload.NextCursor != nil { + t.Fatalf("exact scope exposed descendant tree state: %+v", payload) + } + continue + } + var payload relayfile.FileQueryResponse + if err := json.NewDecoder(resp.Body).Decode(&payload); err != nil { + t.Fatalf("decode query response: %v", err) + } + if len(payload.Items) != 0 || payload.NextCursor != nil { + t.Fatalf("exact scope exposed descendant query state: %+v", payload) + } + } + } + + for _, format := range []string{"json", "tar"} { + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/export?format=" + format + "&path=" + requestPath, + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_collection_exact_export_" + format, + }, + }) + if resp.Code != http.StatusOK { + t.Fatalf("expected %s export 200, got %d (%s)", format, resp.Code, resp.Body.String()) + } + if strings.Contains(resp.Body.String(), "secret") || strings.Contains(resp.Body.String(), "/private/") { + t.Fatalf("exact scope exposed descendant %s export bytes", format) + } + } + }) + } +} + +func TestTreePaginationUsesOnlyACLVisibleCursors(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + server := NewServer(store) + workspaceID := "ws_tree_acl_cursor" + token := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"fs:read"}, time.Now().Add(time.Hour)) + + for index := 0; index < 1002; index++ { + permissions := []string{"scope:finance"} + if index == 1001 { + permissions = nil + } + path := fmt.Sprintf("/private/File%04d.md", index) + if _, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, Path: path, IfMatch: "0", ContentType: "text/markdown", + Content: "classified", Semantics: relayfile.FileSemantics{Permissions: permissions}, + CorrelationID: fmt.Sprintf("corr_tree_acl_cursor_%04d", index), + }); err != nil { + t.Fatalf("write failed for %s: %v", path, err) + } + } + fork, err := store.CreateFork(workspaceID, "proposal-tree-acl-cursor", 3600) + if err != nil { + t.Fatalf("create fork: %v", err) + } + + for _, forkQuery := range []string{"", "&forkId=" + url.QueryEscape(fork.ForkID)} { + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/tree?path=/private&depth=1" + forkQuery, + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_tree_acl_cursor_read", + }, + }) + if resp.Code != http.StatusOK { + t.Fatalf("expected tree%s 200, got %d (%s)", forkQuery, resp.Code, resp.Body.String()) + } + var payload relayfile.TreeResponse + if err := json.NewDecoder(resp.Body).Decode(&payload); err != nil { + t.Fatalf("decode tree response: %v", err) + } + if len(payload.Entries) != 1 || payload.Entries[0].Path != "/private/File1001.md" { + t.Fatalf("expected only the visible tail entry, got %+v", payload.Entries) + } + if payload.TotalFiles != 1 || payload.NextCursor != nil { + t.Fatalf("hidden tree page boundary leaked through totals/cursor: %+v", payload) + } + } +} + +func TestTreePaginationFillsPagesAcrossHiddenBoundaries(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + server := NewServer(store) + workspaceID := "ws_tree_acl_mixed_cursor" + token := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"fs:read"}, time.Now().Add(time.Hour)) + + for index := 0; index < 1002; index++ { + var permissions []string + if index == 999 { + permissions = []string{"scope:finance"} + } + path := fmt.Sprintf("/mixed/File%04d.md", index) + if _, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, Path: path, IfMatch: "0", ContentType: "text/markdown", + Content: "mixed", Semantics: relayfile.FileSemantics{Permissions: permissions}, + CorrelationID: fmt.Sprintf("corr_tree_acl_mixed_%04d", index), + }); err != nil { + t.Fatalf("write failed for %s: %v", path, err) + } + } + + readPage := func(cursor string) relayfile.TreeResponse { + t.Helper() + requestPath := "/v1/workspaces/" + workspaceID + "/fs/tree?path=/mixed&depth=1" + if cursor != "" { + requestPath += "&cursor=" + url.QueryEscape(cursor) + } + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: requestPath, + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_tree_acl_mixed_read", + }, + }) + if resp.Code != http.StatusOK { + t.Fatalf("expected tree 200, got %d (%s)", resp.Code, resp.Body.String()) + } + var payload relayfile.TreeResponse + if err := json.NewDecoder(resp.Body).Decode(&payload); err != nil { + t.Fatalf("decode tree response: %v", err) + } + return payload + } + + pageOne := readPage("") + if len(pageOne.Entries) != 1000 || pageOne.TotalFiles != 1001 || pageOne.NextCursor == nil { + t.Fatalf("unexpected mixed page one: entries=%d total=%d cursor=%v", len(pageOne.Entries), pageOne.TotalFiles, pageOne.NextCursor) + } + if *pageOne.NextCursor != "/mixed/File1000.md" { + t.Fatalf("nextCursor must name the last visible entry, got %q", *pageOne.NextCursor) + } + for _, entry := range pageOne.Entries { + if entry.Path == "/mixed/File0999.md" { + t.Fatalf("hidden page-boundary entry was returned") + } + } + pageTwo := readPage(*pageOne.NextCursor) + if len(pageTwo.Entries) != 1 || pageTwo.Entries[0].Path != "/mixed/File1001.md" || pageTwo.NextCursor != nil { + t.Fatalf("unexpected mixed page two: %+v", pageTwo) + } +} + func TestQueryFilesEndpoint(t *testing.T) { server := NewServer(relayfile.NewStore()) token := mustTestJWT(t, "dev-secret", "ws_query_api", "Worker1", []string{"fs:read", "fs:write"}, time.Now().Add(time.Hour)) @@ -3133,6 +3378,52 @@ func TestTreeEndpointFiltersUnauthorizedFiles(t *testing.T) { } } +func TestQueryRejectsExistingACLHiddenCursor(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + server := NewServer(store) + workspaceID := "ws_query_acl_cursor" + if _, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, Path: "/docs/Hidden.md", IfMatch: "0", ContentType: "text/markdown", + Content: "hidden", Semantics: relayfile.FileSemantics{Permissions: []string{"allow:agent:Finance"}}, CorrelationID: "corr_query_acl_hidden", + }); err != nil { + t.Fatalf("write hidden file: %v", err) + } + if _, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, Path: "/docs/Visible.md", IfMatch: "0", ContentType: "text/markdown", + Content: "visible", CorrelationID: "corr_query_acl_visible", + }); err != nil { + t.Fatalf("write visible file: %v", err) + } + token := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"fs:read"}, time.Now().Add(time.Hour)) + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/query?path=/docs&cursor=" + url.QueryEscape("/docs/Hidden.md"), + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_query_acl_hidden_cursor", + }, + }) + if resp.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for ACL-hidden query cursor, got %d (%s)", resp.Code, resp.Body.String()) + } + fork, err := store.CreateFork(workspaceID, "proposal-query-acl-cursor", 3600) + if err != nil { + t.Fatalf("create query ACL cursor fork: %v", err) + } + forkResp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/query?path=/docs&forkId=" + url.QueryEscape(fork.ForkID) + "&cursor=" + url.QueryEscape("/docs/Hidden.md"), + headers: map[string]string{ + "Authorization": "Bearer " + token, + "X-Correlation-Id": "corr_query_acl_hidden_cursor_fork", + }, + }) + if forkResp.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for ACL-hidden fork query cursor, got %d (%s)", forkResp.Code, forkResp.Body.String()) + } +} + func TestTreeEndpointAllowsDescendantScopedACL(t *testing.T) { server := NewServer(relayfile.NewStore()) workspaceID := "ws_tree_descendant" @@ -3140,6 +3431,7 @@ func TestTreeEndpointAllowsDescendantScopedACL(t *testing.T) { limitedToken := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"relayfile:fs:read:/allowed/**"}, time.Now().Add(time.Hour)) writeFileForTest(t, server, ownerToken, workspaceID, "/allowed/document.md", "0", "descendant", "corr_tree_descendant_file") + writeFileForTest(t, server, ownerToken, workspaceID, "/allowed/sibling-secret.md", "0", "secret", "corr_tree_descendant_sibling") aclWrite := doRequest(t, server, request{ method: http.MethodPut, @@ -3161,6 +3453,26 @@ func TestTreeEndpointAllowsDescendantScopedACL(t *testing.T) { t.Fatalf("expected ACL marker write 202, got %d (%s)", aclWrite.Code, aclWrite.Body.String()) } + rootACLWrite := doRequest(t, server, request{ + method: http.MethodPut, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/.relayfile.acl", + headers: map[string]string{ + "Authorization": "Bearer " + ownerToken, + "X-Correlation-Id": "corr_tree_descendant_root_acl", + "If-Match": "0", + }, + body: map[string]any{ + "contentType": "text/plain", + "content": "unrelated root policy", + "semantics": map[string]any{ + "permissions": []string{"allow:agent:Unrelated"}, + }, + }, + }) + if rootACLWrite.Code != http.StatusAccepted { + t.Fatalf("expected root ACL marker write 202, got %d (%s)", rootACLWrite.Code, rootACLWrite.Body.String()) + } + treeResp := doRequest(t, server, request{ method: http.MethodGet, path: "/v1/workspaces/" + workspaceID + "/fs/tree?path=/allowed", @@ -3180,12 +3492,26 @@ func TestTreeEndpointAllowsDescendantScopedACL(t *testing.T) { for _, entry := range tree.Entries { if entry.Path == "/allowed/document.md" { found = true - break + } + if entry.Path == "/allowed/sibling-secret.md" { + t.Fatalf("expected sibling denied by descendant ACL to be hidden, got %+v", tree.Entries) } } if !found { t.Fatalf("expected descendant file in tree entries, got %+v", tree.Entries) } + + siblingRead := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/file?path=/allowed/sibling-secret.md", + headers: map[string]string{ + "Authorization": "Bearer " + limitedToken, + "X-Correlation-Id": "corr_tree_descendant_sibling_read", + }, + }) + if siblingRead.Code != http.StatusForbidden { + t.Fatalf("expected direct sibling read 403, got %d (%s)", siblingRead.Code, siblingRead.Body.String()) + } } func TestFilePermissionPolicyDenyOverridesAllowAndPublic(t *testing.T) { diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index 57c32843..72188f26 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -1175,67 +1175,7 @@ func (s *Store) ListTree(workspaceID, path string, depth int, cursor string) (Tr return TreeResponse{Path: normalizePath(path), Entries: []TreeEntry{}, NextCursor: nil}, nil } - base := normalizePath(path) - if depth <= 0 { - depth = 1 - } - - entryMap := map[string]TreeEntry{} - totalFiles := 0 - for filePath, file := range ws.Files { - if !withinBase(base, filePath) { - continue - } - rest := strings.TrimPrefix(filePath, base) - rest = strings.TrimPrefix(rest, "/") - if rest == "" { - continue - } - totalFiles++ - parts := strings.Split(rest, "/") - if len(parts) == 0 { - continue - } - maxLevel := depth - if len(parts) < maxLevel { - maxLevel = len(parts) - } - for level := 1; level <= maxLevel; level++ { - child := joinPath(base, strings.Join(parts[:level], "/")) - if level == len(parts) { - entryMap[child] = TreeEntry{ - Path: child, - Type: "file", - Revision: file.Revision, - ContentHash: storedContentHashForFile(file), - Provider: file.Provider, - ProviderObjectID: file.ProviderObjectID, - Size: int64(len(file.Content)), - UpdatedAt: file.LastEditedAt, - PropertyCount: len(file.Semantics.Properties), - RelationCount: len(file.Semantics.Relations), - PermissionCount: len(file.Semantics.Permissions), - CommentCount: len(file.Semantics.Comments), - } - continue - } - if _, exists := entryMap[child]; !exists { - entryMap[child] = TreeEntry{Path: child, Type: "dir", Revision: "dir"} - } - } - } - - entries := make([]TreeEntry, 0, len(entryMap)) - for _, entry := range entryMap { - entries = append(entries, entry) - } - sort.Slice(entries, func(i, j int) bool { return entries[i].Path < entries[j].Path }) - entries, nextCursor, err := paginateTreeEntries(entries, cursor) - if err != nil { - return TreeResponse{}, err - } - - return TreeResponse{Path: base, Entries: entries, NextCursor: nextCursor, TotalFiles: totalFiles}, nil + return listTreeFromFiles(ws.Files, path, depth, cursor) } func (s *Store) ReadFile(workspaceID, path string) (File, error) { @@ -1291,23 +1231,6 @@ func (s *Store) QueryFiles(workspaceID string, req FileQueryRequest) (FileQueryR if workspaceID == "" { return FileQueryResponse{}, ErrInvalidInput } - base := normalizePath(req.PathPrefix) - if req.PathPrefix == "" { - base = "/" - } - provider := normalizeProvider(req.Provider) - relation := strings.TrimSpace(req.Relation) - permission := strings.TrimSpace(req.Permission) - comment := strings.TrimSpace(req.Comment) - limit := req.Limit - if limit <= 0 { - limit = 100 - } - if limit > 1000 { - limit = 1000 - } - properties := normalizeProperties(req.Properties) - s.mu.RLock() defer s.mu.RUnlock() @@ -1318,77 +1241,7 @@ func (s *Store) QueryFiles(workspaceID string, req FileQueryRequest) (FileQueryR } return FileQueryResponse{Items: []FileQueryItem{}, NextCursor: nil}, nil } - paths := make([]string, 0, len(ws.Files)) - for path := range ws.Files { - paths = append(paths, path) - } - sort.Strings(paths) - - start := 0 - cursor := normalizePath(req.Cursor) - if strings.TrimSpace(req.Cursor) != "" { - found := false - for i := range paths { - if paths[i] == cursor { - start = i + 1 - found = true - break - } - } - if !found { - return FileQueryResponse{}, ErrInvalidInput - } - } - - items := make([]FileQueryItem, 0, limit) - var nextCursor *string - - for i := start; i < len(paths); i++ { - path := paths[i] - if !withinBase(base, path) { - continue - } - file := ws.Files[path] - if provider != "" && normalizeProvider(file.Provider) != provider { - continue - } - semantics := normalizeSemantics(file.Semantics) - if relation != "" && !stringSliceContains(semantics.Relations, relation) { - continue - } - if permission != "" && !stringSliceContains(semantics.Permissions, permission) { - continue - } - if comment != "" && !stringSliceContains(semantics.Comments, comment) { - continue - } - if !propertiesMatch(semantics.Properties, properties) { - continue - } - if len(items) >= limit { - cursorValue := items[len(items)-1].Path - nextCursor = &cursorValue - break - } - items = append(items, FileQueryItem{ - Path: path, - Revision: file.Revision, - ContentType: file.ContentType, - Provider: file.Provider, - ProviderObjectID: file.ProviderObjectID, - LastEditedAt: file.LastEditedAt, - Size: int64(len(file.Content)), - Properties: copyStringMap(semantics.Properties), - Relations: append([]string(nil), semantics.Relations...), - Permissions: append([]string(nil), semantics.Permissions...), - Comments: append([]string(nil), semantics.Comments...), - }) - } - - return FileQueryResponse{ - Items: items, - NextCursor: nextCursor, - }, nil + return queryFilesFromMap(ws.Files, req) } func (s *Store) WriteFile(req WriteRequest) (WriteResult, error) { @@ -1675,14 +1528,23 @@ func (s *Store) ExportWorkspace(workspaceID string) ([]File, error) { if !ok { return []File{}, nil } - files := make([]File, 0, len(ws.Files)) - for _, file := range ws.Files { - files = append(files, file) + return sortedFilesFromMap(ws.Files), nil +} + +// ExportForkWorkspace returns the fork's merged view, including overlay +// writes and deletions. Callers must use this instead of exporting the live +// workspace when a forkId is supplied. +func (s *Store) ExportForkWorkspace(workspaceID, forkID string) ([]File, error) { + if strings.TrimSpace(workspaceID) == "" || strings.TrimSpace(forkID) == "" { + return nil, ErrInvalidInput } - sort.Slice(files, func(i, j int) bool { - return files[i].Path < files[j].Path - }) - return files, nil + s.mu.Lock() + defer s.mu.Unlock() + fork, err := s.getLiveForkLocked(workspaceID, forkID, time.Now().UTC()) + if err != nil { + return nil, err + } + return sortedFilesFromMap(s.mergedForkFilesLocked(fork)), nil } func (s *Store) DeleteFile(req DeleteRequest) (WriteResult, error) { @@ -2284,8 +2146,11 @@ func (s *Store) ListForkTree(workspaceID, forkID, path string, depth int, cursor if err != nil { return TreeResponse{}, err } - files := s.mergedForkFilesLocked(fork) - return listTreeFromFiles(files, path, depth, cursor) + ws := s.workspaces[workspaceID] + if ws == nil { + return TreeResponse{Path: normalizePath(path), Entries: []TreeEntry{}, NextCursor: nil}, nil + } + return listTreeFromForkEntries(ws.Files, fork.Overlay, path, depth, cursor) } func (s *Store) QueryForkFiles(workspaceID, forkID string, req FileQueryRequest) (FileQueryResponse, error) { @@ -2298,8 +2163,14 @@ func (s *Store) QueryForkFiles(workspaceID, forkID string, req FileQueryRequest) if err != nil { return FileQueryResponse{}, err } - files := s.mergedForkFilesLocked(fork) - return queryFilesFromMap(files, req) + ws := s.workspaces[workspaceID] + if ws == nil { + if strings.TrimSpace(req.Cursor) != "" { + return FileQueryResponse{}, ErrInvalidInput + } + return FileQueryResponse{Items: []FileQueryItem{}, NextCursor: nil}, nil + } + return queryFilesFromForkEntries(ws.Files, fork.Overlay, req) } func (s *Store) ResolveForkFilePermissions(workspaceID, forkID, path string, includeTarget bool) []string { @@ -2309,8 +2180,11 @@ func (s *Store) ResolveForkFilePermissions(workspaceID, forkID, path string, inc if err != nil { return nil } - files := s.mergedForkFilesLocked(fork) - return resolvePermissionsFromFiles(files, path, includeTarget) + ws := s.workspaces[workspaceID] + if ws == nil { + return nil + } + return resolvePermissionsFromForkEntries(ws.Files, fork.Overlay, path, includeTarget) } func (s *Store) GetEvents(workspaceID, provider, cursor string, limit int) (EventFeed, error) { @@ -5003,27 +4877,71 @@ func (s *Store) mergedForkFilesLocked(fork *forkState) map[string]File { return files } +func sortedFilesFromMap(files map[string]File) []File { + result := make([]File, 0, len(files)) + for _, file := range files { + result = append(result, file) + } + sort.Slice(result, func(i, j int) bool { return result[i].Path < result[j].Path }) + return result +} + func listTreeFromFiles(files map[string]File, path string, depth int, cursor string) (TreeResponse, error) { + return listTreeFromEntries(func(visit func(string, File)) { + for filePath, file := range files { + visit(filePath, file) + } + }, path, depth, cursor) +} + +func listTreeFromForkEntries(files map[string]File, overlay map[string]ForkOverlayEntry, path string, depth int, cursor string) (TreeResponse, error) { + return listTreeFromEntries(func(visit func(string, File)) { + for filePath, file := range files { + if _, touched := overlay[normalizePath(filePath)]; touched { + continue + } + visit(filePath, file) + } + for filePath, entry := range overlay { + if entry.Type != "write" || entry.File == nil { + continue + } + file := *entry.File + normalized := normalizePath(filePath) + file.Path = normalized + file.Revision = entry.Revision + visit(normalized, file) + } + }, path, depth, cursor) +} + +func listTreeFromEntries(iterate func(func(string, File)), path string, depth int, cursor string) (TreeResponse, error) { base := normalizePath(path) if depth <= 0 { depth = 1 } - entryMap := map[string]TreeEntry{} + // Keep one page plus a sentinel entry while walking the source map. The + // previous implementation retained every descendant entry before sorting; + // a large workspace could therefore consume memory proportional to the + // entire tree for a single request. + const retainedEntries = maxTreeEntriesPerPage + 1 + entryMap := make(map[string]TreeEntry, retainedEntries) totalFiles := 0 - for filePath, file := range files { + cursorFound := cursor == "" + iterate(func(filePath string, file File) { if !withinBase(base, filePath) { - continue + return } rest := strings.TrimPrefix(filePath, base) rest = strings.TrimPrefix(rest, "/") if rest == "" { - continue + return } totalFiles++ parts := strings.Split(rest, "/") if len(parts) == 0 { - continue + return } maxLevel := depth if len(parts) < maxLevel { @@ -5031,6 +4949,12 @@ func listTreeFromFiles(files map[string]File, path string, depth int, cursor str } for level := 1; level <= maxLevel; level++ { child := joinPath(base, strings.Join(parts[:level], "/")) + if child == cursor { + cursorFound = true + } + if cursor != "" && child <= cursor { + continue + } if level == len(parts) { entryMap[child] = TreeEntry{ Path: child, @@ -5046,12 +4970,22 @@ func listTreeFromFiles(files map[string]File, path string, depth int, cursor str PermissionCount: len(file.Semantics.Permissions), CommentCount: len(file.Semantics.Comments), } - continue - } - if _, exists := entryMap[child]; !exists { + } else if _, exists := entryMap[child]; !exists { entryMap[child] = TreeEntry{Path: child, Type: "dir", Revision: "dir"} } + if len(entryMap) > retainedEntries { + var greatest string + for candidate := range entryMap { + if greatest == "" || candidate > greatest { + greatest = candidate + } + } + delete(entryMap, greatest) + } } + }) + if !cursorFound { + return TreeResponse{}, ErrInvalidInput } entries := make([]TreeEntry, 0, len(entryMap)) @@ -5059,50 +4993,48 @@ func listTreeFromFiles(files map[string]File, path string, depth int, cursor str entries = append(entries, entry) } sort.Slice(entries, func(i, j int) bool { return entries[i].Path < entries[j].Path }) - entries, nextCursor, err := paginateTreeEntries(entries, cursor) - if err != nil { - return TreeResponse{}, err + if len(entries) > maxTreeEntriesPerPage { + entries = entries[:maxTreeEntriesPerPage] + } + var nextCursor *string + if len(entryMap) > maxTreeEntriesPerPage { + cursorValue := entries[len(entries)-1].Path + nextCursor = &cursorValue } return TreeResponse{Path: base, Entries: entries, NextCursor: nextCursor, TotalFiles: totalFiles}, nil } -// paginateTreeEntries slices the supplied entries with the supplied cursor. -// A non-empty cursor that doesn't match any entry returns ErrInvalidInput so -// stale/typo cursors are rejected rather than silently restarting pagination -// at page 1 (which previously caused duplicate-page loops for clients). -func paginateTreeEntries(entries []TreeEntry, cursor string) ([]TreeEntry, *string, error) { - start := 0 - if cursor != "" { - found := false - for index, entry := range entries { - if entry.Path == cursor { - start = index + 1 - found = true - break +func queryFilesFromMap(files map[string]File, req FileQueryRequest) (FileQueryResponse, error) { + return queryFilesFromEntries(func(visit func(string, File)) { + for path, file := range files { + visit(path, file) + } + }, req) +} + +func queryFilesFromForkEntries(files map[string]File, overlay map[string]ForkOverlayEntry, req FileQueryRequest) (FileQueryResponse, error) { + return queryFilesFromEntries(func(visit func(string, File)) { + for path, file := range files { + if _, touched := overlay[normalizePath(path)]; touched { + continue } + visit(path, file) } - if !found { - return nil, nil, ErrInvalidInput + for path, entry := range overlay { + if entry.Type != "write" || entry.File == nil { + continue + } + file := *entry.File + normalized := normalizePath(path) + file.Path = normalized + file.Revision = entry.Revision + visit(normalized, file) } - } - if start >= len(entries) { - return []TreeEntry{}, nil, nil - } - - end := start + maxTreeEntriesPerPage - if end > len(entries) { - end = len(entries) - } - page := append([]TreeEntry(nil), entries[start:end]...) - if end >= len(entries) { - return page, nil, nil - } - cursorValue := page[len(page)-1].Path - return page, &cursorValue, nil + }, req) } -func queryFilesFromMap(files map[string]File, req FileQueryRequest) (FileQueryResponse, error) { +func queryFilesFromEntries(iterate func(func(string, File)), req FileQueryRequest) (FileQueryResponse, error) { base := normalizePath(req.PathPrefix) if req.PathPrefix == "" { base = "/" @@ -5120,58 +5052,39 @@ func queryFilesFromMap(files map[string]File, req FileQueryRequest) (FileQueryRe } properties := normalizeProperties(req.Properties) - paths := make([]string, 0, len(files)) - for path := range files { - paths = append(paths, path) - } - sort.Strings(paths) - - start := 0 cursor := normalizePath(req.Cursor) - if strings.TrimSpace(req.Cursor) != "" { - found := false - for i := range paths { - if paths[i] == cursor { - start = i + 1 - found = true - break - } - } - if !found { - return FileQueryResponse{}, ErrInvalidInput + cursorFound := strings.TrimSpace(req.Cursor) == "" + // Retain only one public page plus a sentinel while scanning the map. The + // old implementation first materialized and sorted every path in the + // workspace, which made each paged request proportional to workspace size. + candidates := make(map[string]FileQueryItem, limit+1) + iterate(func(path string, file File) { + if path == cursor { + cursorFound = true + } + if strings.TrimSpace(req.Cursor) != "" && path <= cursor { + return } - } - - items := make([]FileQueryItem, 0, limit) - var nextCursor *string - for i := start; i < len(paths); i++ { - path := paths[i] if !withinBase(base, path) { - continue + return } - file := files[path] if provider != "" && normalizeProvider(file.Provider) != provider { - continue + return } semantics := normalizeSemantics(file.Semantics) if relation != "" && !stringSliceContains(semantics.Relations, relation) { - continue + return } if permission != "" && !stringSliceContains(semantics.Permissions, permission) { - continue + return } if comment != "" && !stringSliceContains(semantics.Comments, comment) { - continue + return } if !propertiesMatch(semantics.Properties, properties) { - continue - } - if len(items) >= limit { - cursorValue := items[len(items)-1].Path - nextCursor = &cursorValue - break + return } - items = append(items, FileQueryItem{ + candidates[path] = FileQueryItem{ Path: path, Revision: file.Revision, ContentType: file.ContentType, @@ -5183,7 +5096,36 @@ func queryFilesFromMap(files map[string]File, req FileQueryRequest) (FileQueryRe Relations: append([]string(nil), semantics.Relations...), Permissions: append([]string(nil), semantics.Permissions...), Comments: append([]string(nil), semantics.Comments...), - }) + } + if len(candidates) <= limit+1 { + return + } + var greatest string + for candidate := range candidates { + if greatest == "" || candidate > greatest { + greatest = candidate + } + } + delete(candidates, greatest) + }) + if !cursorFound { + return FileQueryResponse{}, ErrInvalidInput + } + + paths := make([]string, 0, len(candidates)) + for path := range candidates { + paths = append(paths, path) + } + sort.Strings(paths) + items := make([]FileQueryItem, 0, len(paths)) + for _, path := range paths { + items = append(items, candidates[path]) + } + var nextCursor *string + if len(items) > limit { + cursorValue := items[limit-1].Path + nextCursor = &cursorValue + items = items[:limit] } return FileQueryResponse{Items: items, NextCursor: nextCursor}, nil @@ -5216,6 +5158,48 @@ func resolvePermissionsFromFiles(files map[string]File, path string, includeTarg return out } +func resolvePermissionsFromForkEntries(files map[string]File, overlay map[string]ForkOverlayEntry, path string, includeTarget bool) []string { + lookup := func(target string) (File, bool) { + target = normalizePath(target) + if entry, touched := overlay[target]; touched { + if entry.Type != "write" || entry.File == nil { + return File{}, false + } + file := *entry.File + file.Path = target + file.Revision = entry.Revision + return file, true + } + file, exists := files[target] + return file, exists + } + + target := normalizePath(path) + permissions := make([]string, 0, 8) + for _, dir := range ancestorDirectories(target) { + markerPath := joinPath(dir, DirectoryPermissionMarkerFile) + if markerPath == target { + continue + } + marker, exists := lookup(markerPath) + if !exists || len(marker.Semantics.Permissions) == 0 { + continue + } + permissions = append(permissions, marker.Semantics.Permissions...) + } + if includeTarget { + if file, exists := lookup(target); exists && len(file.Semantics.Permissions) > 0 { + permissions = append(permissions, file.Semantics.Permissions...) + } + } + if len(permissions) == 0 { + return nil + } + out := make([]string, len(permissions)) + copy(out, permissions) + return out +} + func forkWorkspaceProposalKey(workspaceID, proposalID string) string { return workspaceID + "\x00" + proposalID } diff --git a/internal/relayfile/store_test.go b/internal/relayfile/store_test.go index 65e24f1e..929f276b 100644 --- a/internal/relayfile/store_test.go +++ b/internal/relayfile/store_test.go @@ -1318,6 +1318,34 @@ func TestListTreePaginatesBoundedEntries(t *testing.T) { } } +func TestListTreeLargeWorkspaceRetainsOnlyPageEntries(t *testing.T) { + store := NewStoreWithOptions(StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + const workspaceID = "ws_tree_large_bounded" + const fileCount = 5000 + writes := make([]BulkWriteFile, 0, fileCount) + for index := 0; index < fileCount; index++ { + writes = append(writes, BulkWriteFile{ + Path: fmt.Sprintf("/large/File%05d.md", index), + ContentType: "text/markdown", + Content: "x", + }) + } + if written, _, errs := store.BulkWrite(workspaceID, writes); written != fileCount || len(errs) != 0 { + t.Fatalf("seed large tree failed: written=%d errs=%+v", written, errs) + } + page, err := store.ListTree(workspaceID, "/large", 1, "") + if err != nil { + t.Fatalf("ListTree: %v", err) + } + if len(page.Entries) != maxTreeEntriesPerPage || page.TotalFiles != fileCount { + t.Fatalf("unexpected large tree page: entries=%d total=%d", len(page.Entries), page.TotalFiles) + } + if page.NextCursor == nil || *page.NextCursor != "/large/File00999.md" { + t.Fatalf("unexpected large tree cursor: %v", page.NextCursor) + } +} + func TestQueryFilesSupportsSemanticFilters(t *testing.T) { store := NewStore() t.Cleanup(store.Close) diff --git a/openapi/relayfile-v1.openapi.yaml b/openapi/relayfile-v1.openapi.yaml index 1fa996fe..b29ecbf7 100644 --- a/openapi/relayfile-v1.openapi.yaml +++ b/openapi/relayfile-v1.openapi.yaml @@ -96,6 +96,8 @@ paths: application/json: schema: $ref: '#/components/schemas/TreeResponse' + '400': + $ref: '#/components/responses/BadRequest' '401': $ref: '#/components/responses/Unauthorized' '403': @@ -395,6 +397,8 @@ paths: $ref: '#/components/responses/Unauthorized' '403': $ref: '#/components/responses/Forbidden' + '404': + $ref: '#/components/responses/NotFound' '429': $ref: '#/components/responses/RateLimited' '500': From a765271e47b1147db6acc14f1168a0ece533a6dd Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 15:25:06 +0200 Subject: [PATCH 05/12] fix(auth): filter ACL-hidden filesystem events Session-Id: 01a0810c-374f-7d80-b113-42a09af744cb --- internal/httpapi/server.go | 75 +++++++++- internal/httpapi/server_test.go | 134 ++++++++++++++++++ internal/httpapi/websocket.go | 12 +- internal/relayfile/store.go | 87 +++++++++--- internal/relayfile/store_content_hash_test.go | 37 +++++ 5 files changed, 317 insertions(+), 28 deletions(-) diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 3f2bd6d5..c7af22f1 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -385,7 +385,7 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { case "delete_file": s.handleDeleteFile(w, r, workspaceID, correlationID, claims) case "events": - s.handleEvents(w, r, workspaceID, correlationID) + s.handleEvents(w, r, workspaceID, correlationID, claims) case "query_files": s.handleQueryFiles(w, r, workspaceID, correlationID, claims) case "sync_status": @@ -2365,20 +2365,87 @@ func (s *Server) handleDeleteFile(w http.ResponseWriter, r *http.Request, worksp writeJSON(w, http.StatusAccepted, result) } -func (s *Server) handleEvents(w http.ResponseWriter, r *http.Request, workspaceID, correlationID string) { +func (s *Server) eventVisibleToClaims(workspaceID string, claims tokenClaims, event relayfile.Event) bool { + path := strings.TrimSpace(event.Path) + if path == "" { + return true + } + path = normalizeACLPath(path) + if !scopeMatchesPath(claims.Scopes, "fs:read", path) { + return false + } + if event.ACLPermissions != nil && !filePermissionAllows(event.ACLPermissions, workspaceID, &claims, "read", path) { + return false + } + includeTarget := event.Type != "file.deleted" + return filePermissionAllows(s.resolveFilePermissions(workspaceID, "", path, includeTarget), workspaceID, &claims, "read", path) +} + +func (s *Server) fileReadAllowedNow(workspaceID string, claims tokenClaims, path string, includeTarget bool) bool { + path = normalizeACLPath(path) + return scopeMatchesPath(claims.Scopes, "fs:read", path) && + filePermissionAllows(s.resolveFilePermissions(workspaceID, "", path, includeTarget), workspaceID, &claims, "read", path) +} + +const eventFilterPageSize = 1000 + +func (s *Server) visibleEvents(workspaceID string, claims tokenClaims, provider, direction, cursor string, limit int) (relayfile.EventFeed, error) { + if limit <= 0 { + limit = 200 + } + visible := make([]relayfile.Event, 0, limit) + seenCursors := map[string]struct{}{} + for { + var page relayfile.EventFeed + var err error + if direction == "desc" { + page, err = s.store.GetEventsTail(workspaceID, provider, cursor, eventFilterPageSize) + } else { + page, err = s.store.GetEvents(workspaceID, provider, cursor, eventFilterPageSize) + } + if err != nil { + return relayfile.EventFeed{}, err + } + for _, event := range page.Events { + if !s.eventVisibleToClaims(workspaceID, claims, event) { + continue + } + if len(visible) < limit { + visible = append(visible, event) + continue + } + next := visible[len(visible)-1].EventID + return relayfile.EventFeed{Events: visible, NextCursor: &next}, nil + } + if page.NextCursor == nil || strings.TrimSpace(*page.NextCursor) == "" { + return relayfile.EventFeed{Events: visible, NextCursor: nil}, nil + } + next := strings.TrimSpace(*page.NextCursor) + if next == cursor { + return relayfile.EventFeed{Events: visible, NextCursor: nil}, nil + } + if _, ok := seenCursors[next]; ok { + return relayfile.EventFeed{Events: visible, NextCursor: nil}, nil + } + seenCursors[next] = struct{}{} + cursor = next + } +} + +func (s *Server) handleEvents(w http.ResponseWriter, r *http.Request, workspaceID, correlationID string, claims tokenClaims) { limit := parseBoundedInt(r.URL.Query().Get("limit"), 200, 1, 1000) provider := r.URL.Query().Get("provider") direction := strings.ToLower(strings.TrimSpace(r.URL.Query().Get("direction"))) switch direction { case "", "asc": - feed, err := s.store.GetEvents(workspaceID, provider, r.URL.Query().Get("cursor"), limit) + feed, err := s.visibleEvents(workspaceID, claims, provider, "asc", r.URL.Query().Get("cursor"), limit) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", err.Error(), correlationID) return } writeJSON(w, http.StatusOK, feed) case "desc": - feed, err := s.store.GetEventsTail(workspaceID, provider, r.URL.Query().Get("cursor"), limit) + feed, err := s.visibleEvents(workspaceID, claims, provider, "desc", r.URL.Query().Get("cursor"), limit) if err != nil { writeError(w, http.StatusInternalServerError, "internal_error", err.Error(), correlationID) return diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index 8a9af702..73834a61 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -163,6 +163,140 @@ func TestFileEventsWebSocketCatchUpAndPingPong(t *testing.T) { } } +func TestACLFiltersHTTPEventsAndWebSocketCatchUpAndLive(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + workspaceID := "ws_acl_event_filter" + write := func(path, content string, semantics relayfile.FileSemantics) { + t.Helper() + if _, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, + Path: path, + IfMatch: "0", + ContentType: "text/plain", + Content: content, + Semantics: semantics, + }); err != nil { + t.Fatalf("seed %s failed: %v", path, err) + } + } + write("/public.md", "public", relayfile.FileSemantics{}) + write("/secret/.relayfile.acl", `{"semantics":{"permissions":["deny:agent:Limited"]}}`, relayfile.FileSemantics{Permissions: []string{"deny:agent:Limited"}}) + write("/secret/hidden.md", "secret", relayfile.FileSemantics{}) + + server := httptest.NewServer(NewServer(store)) + defer server.Close() + limitedToken := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"fs:read"}, time.Now().Add(time.Hour)) + headers := map[string]string{ + "Authorization": "Bearer " + limitedToken, + "X-Correlation-Id": "corr_acl_event_filter", + } + for _, direction := range []string{"", "desc"} { + path := "/v1/workspaces/" + workspaceID + "/fs/events?limit=100" + if direction != "" { + path += "&direction=" + direction + } + resp := doRequest(t, NewServer(store), request{method: http.MethodGet, path: path, headers: headers}) + if resp.Code != http.StatusOK { + t.Fatalf("events %s returned %d: %s", direction, resp.Code, resp.Body.String()) + } + var feed relayfile.EventFeed + if err := json.NewDecoder(resp.Body).Decode(&feed); err != nil { + t.Fatalf("decode events %s: %v", direction, err) + } + for _, event := range feed.Events { + if strings.Contains(event.Path, "/secret/") { + t.Fatalf("ACL-hidden event leaked in %s feed: %+v", direction, event) + } + } + if len(feed.Events) != 1 || feed.Events[0].Path != "/public.md" { + t.Fatalf("unexpected visible events in %s feed: %+v", direction, feed.Events) + } + if feed.NextCursor != nil { + t.Fatalf("hidden events leaked through cursor in %s feed: %v", direction, *feed.NextCursor) + } + } + + wsURL := "ws" + strings.TrimPrefix(server.URL, "http") + "/v1/workspaces/" + workspaceID + "/fs/ws?token=" + limitedToken + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + conn, _, err := websocket.Dial(ctx, wsURL, nil) + if err != nil { + t.Fatalf("websocket dial failed: %v", err) + } + defer conn.Close(websocket.StatusNormalClosure, "") + var catchUp map[string]any + if err := wsjson.Read(ctx, conn, &catchUp); err != nil { + t.Fatalf("read visible catch-up event: %v", err) + } + if catchUp["path"] != "/public.md" || catchUp["content"] != "public" || catchUp["inlineContent"] != true { + t.Fatalf("unexpected catch-up event: %+v", catchUp) + } + if err := wsjson.Write(ctx, conn, map[string]any{"type": "ping"}); err != nil { + t.Fatalf("write ping failed: %v", err) + } + var pong map[string]any + if err := wsjson.Read(ctx, conn, &pong); err != nil || pong["type"] != "pong" { + t.Fatalf("expected pong after filtered catch-up, got %+v (%v)", pong, err) + } + write("/secret/live-hidden.md", "secret-live", relayfile.FileSemantics{}) + write("/public-live.md", "public-live", relayfile.FileSemantics{}) + var live map[string]any + if err := wsjson.Read(ctx, conn, &live); err != nil { + t.Fatalf("read visible live event: %v", err) + } + if live["path"] != "/public-live.md" || live["content"] != "public-live" || live["inlineContent"] != true { + t.Fatalf("unexpected visible live event: %+v", live) + } +} + +func TestACLDeleteEventSnapshotAndPathNormalization(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + workspaceID := "ws_acl_delete_snapshot" + created, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, + Path: "/docs/deleted.md", + IfMatch: "0", + Content: "secret", + Semantics: relayfile.FileSemantics{Permissions: []string{"deny:agent:Limited"}}, + }) + if err != nil { + t.Fatalf("seed protected file failed: %v", err) + } + if _, err := store.DeleteFile(relayfile.DeleteRequest{WorkspaceID: workspaceID, Path: "/docs/deleted.md", IfMatch: created.TargetRevision}); err != nil { + t.Fatalf("delete protected file failed: %v", err) + } + feed, err := store.GetEvents(workspaceID, "", "", 100) + if err != nil || len(feed.Events) != 2 { + t.Fatalf("unexpected event feed: %+v (%v)", feed, err) + } + server := NewServer(store) + claims := tokenClaims{WorkspaceID: workspaceID, AgentName: "Limited", Scopes: map[string]struct{}{"fs:read": {}}} + for _, event := range feed.Events { + if event.Type != "file.deleted" { + continue + } + if s := event.ACLPermissions; len(s) == 0 { + t.Fatalf("delete event lost ACL snapshot: %+v", event) + } + if server.eventVisibleToClaims(workspaceID, claims, event) { + t.Fatalf("protected delete event became visible: %+v", event) + } + } + deleteEvent := relayfile.Event{Type: "file.deleted", Path: "/docs/deleted.md"} + for _, event := range feed.Events { + if event.Type == "file.deleted" { + deleteEvent = event + break + } + } + deleteEvent.Path = "/docs/../docs/deleted.md" + if server.eventVisibleToClaims(workspaceID, claims, deleteEvent) { + t.Fatal("normalized protected path was treated as visible") + } +} + func TestFileEventsWebSocketCursorCatchUpDrainsMoreThanOnePage(t *testing.T) { store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) t.Cleanup(store.Close) diff --git a/internal/httpapi/websocket.go b/internal/httpapi/websocket.go index 330d5101..369e2bbe 100644 --- a/internal/httpapi/websocket.go +++ b/internal/httpapi/websocket.go @@ -107,7 +107,7 @@ func (s *Server) handleFileEventsWebSocket(w http.ResponseWriter, r *http.Reques if !webSocketEventMatchesPaths(event, options.Paths) { continue } - if err := s.writeWebSocketEvent(ctx, conn, workspaceID, event); err != nil { + if err := s.writeWebSocketEvent(ctx, conn, workspaceID, claims, event); err != nil { return } } @@ -147,7 +147,7 @@ func (s *Server) handleFileEventsWebSocket(w http.ResponseWriter, r *http.Reques continue } } - if err := s.writeWebSocketEvent(ctx, conn, workspaceID, event); err != nil { + if err := s.writeWebSocketEvent(ctx, conn, workspaceID, claims, event); err != nil { return } } @@ -279,7 +279,10 @@ func (s *Server) readWebSocketMessages(ctx context.Context, conn *websocket.Conn } } -func (s *Server) writeWebSocketEvent(ctx context.Context, conn *websocket.Conn, workspaceID string, event relayfile.Event) error { +func (s *Server) writeWebSocketEvent(ctx context.Context, conn *websocket.Conn, workspaceID string, claims tokenClaims, event relayfile.Event) error { + if !s.eventVisibleToClaims(workspaceID, claims, event) { + return nil + } message := fileEventMessage{ EventID: event.EventID, Type: event.Type, @@ -293,7 +296,8 @@ func (s *Server) writeWebSocketEvent(ctx context.Context, conn *websocket.Conn, } if event.Type == "file.created" || event.Type == "file.updated" { if file, err := s.store.ReadFile(workspaceID, event.Path); err == nil && - file.Revision == event.Revision && len(file.Content) <= maxWebSocketInlineContentBytes { + file.Revision == event.Revision && len(file.Content) <= maxWebSocketInlineContentBytes && + s.fileReadAllowedNow(workspaceID, claims, event.Path, true) { message.ContentType = file.ContentType message.Content = file.Content message.Encoding = file.Encoding diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index 72188f26..b56a19ff 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -184,6 +184,10 @@ type Event struct { Provider string `json:"provider,omitempty"` CorrelationID string `json:"correlationId"` Timestamp string `json:"timestamp"` + // ACLPermissions is an internal snapshot used to keep delete and rename + // events subject to the permissions that governed the file revision. It is + // deliberately excluded from the public event payload. + ACLPermissions []string `json:"-"` } type EventFeed struct { @@ -595,12 +599,13 @@ type Store struct { } type workspaceState struct { - Revision string `json:"revision,omitempty"` - Files map[string]File `json:"files"` - Events []Event `json:"events"` - Ops map[string]OperationStatus `json:"ops"` - ProviderIndex map[string]string `json:"providerIndex,omitempty"` - ProviderWatermarks map[string]string `json:"providerWatermarks,omitempty"` + Revision string `json:"revision,omitempty"` + Files map[string]File `json:"files"` + Events []Event `json:"events"` + ACLPermissionsByEvent map[string][]string `json:"aclPermissionsByEvent,omitempty"` + Ops map[string]OperationStatus `json:"ops"` + ProviderIndex map[string]string `json:"providerIndex,omitempty"` + ProviderWatermarks map[string]string `json:"providerWatermarks,omitempty"` } type WritebackQueueItem struct { @@ -1578,11 +1583,12 @@ func (s *Store) DeleteFile(req DeleteRequest) (WriteResult, error) { s.mu.Unlock() return WriteResult{}, err } + aclPermissions := resolvePermissionsFromFiles(ws.Files, path, true) delete(ws.Files, path) revision := s.nextRevisionLocked() ws.Revision = revision - result, task := s.recordWriteLocked(ws, path, revision, "file.deleted", existing.Provider, req.CorrelationID) + result, task := s.recordWriteWithACLPermissionsLocked(ws, path, revision, "file.deleted", existing.Provider, req.CorrelationID, aclPermissions) _ = s.saveLocked() s.mu.Unlock() s.enqueueWriteback(task) @@ -1777,10 +1783,11 @@ func (s *Store) CommitForkWithValidator(workspaceID, forkID, correlationID strin case "delete": existing, existed := ws.Files[path] if existed { + aclPermissions := resolvePermissionsFromFiles(ws.Files, path, true) delete(ws.Files, path) revision = s.nextRevisionLocked() ws.Revision = revision - _, task := s.recordWriteLocked(ws, path, revision, "file.deleted", existing.Provider, correlationID) + _, task := s.recordWriteWithACLPermissionsLocked(ws, path, revision, "file.deleted", existing.Provider, correlationID, aclPermissions) tasks = append(tasks, task) } if existed { @@ -3667,15 +3674,19 @@ func (s *Store) ensureWorkspaceLocked(workspaceID string) *workspaceState { if ws.ProviderWatermarks == nil { ws.ProviderWatermarks = map[string]string{} } + if ws.ACLPermissionsByEvent == nil { + ws.ACLPermissionsByEvent = map[string][]string{} + } return ws } ws = &workspaceState{ - Revision: "0", - Files: map[string]File{}, - Events: []Event{}, - Ops: map[string]OperationStatus{}, - ProviderIndex: map[string]string{}, - ProviderWatermarks: map[string]string{}, + Revision: "0", + Files: map[string]File{}, + Events: []Event{}, + Ops: map[string]OperationStatus{}, + ACLPermissionsByEvent: map[string][]string{}, + ProviderIndex: map[string]string{}, + ProviderWatermarks: map[string]string{}, } s.workspaces[workspaceID] = ws return ws @@ -3751,6 +3762,14 @@ func (s *Store) recordWriteLocked(ws *workspaceState, path, revision, eventType, } func (s *Store) recordWriteWithContentIdentityLocked(ws *workspaceState, path, revision, eventType, provider, correlationID string, contentIdentity *ContentIdentity) (WriteResult, writebackTask) { + return s.recordWriteWithContentIdentityAndACLPermissionsLocked(ws, path, revision, eventType, provider, correlationID, contentIdentity, nil, false) +} + +func (s *Store) recordWriteWithACLPermissionsLocked(ws *workspaceState, path, revision, eventType, provider, correlationID string, aclPermissions []string) (WriteResult, writebackTask) { + return s.recordWriteWithContentIdentityAndACLPermissionsLocked(ws, path, revision, eventType, provider, correlationID, nil, aclPermissions, true) +} + +func (s *Store) recordWriteWithContentIdentityAndACLPermissionsLocked(ws *workspaceState, path, revision, eventType, provider, correlationID string, contentIdentity *ContentIdentity, aclPermissions []string, snapshotACL bool) (WriteResult, writebackTask) { if provider == "" { } workspaceID := s.workspaceIDForStateLocked(ws) @@ -3787,6 +3806,12 @@ func (s *Store) recordWriteWithContentIdentityLocked(ws *workspaceState, path, r CorrelationID: correlationID, Timestamp: nowTS, } + if !snapshotACL && strings.HasPrefix(eventType, "file.") { + aclPermissions = resolvePermissionsFromFiles(ws.Files, path, eventType != "file.deleted") + } + if snapshotACL || aclPermissions != nil { + event.ACLPermissions = append([]string(nil), aclPermissions...) + } s.appendWorkspaceEventLocked(workspaceID, ws, event) result := WriteResult{OpID: opID, Status: "queued", TargetRevision: revision} @@ -4361,10 +4386,11 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a if objectID != "" { key := providerObjectKey(provider, objectID) if previousPath, ok := ws.ProviderIndex[key]; ok && previousPath != path { + aclPermissions := resolvePermissionsFromFiles(ws.Files, previousPath, true) delete(ws.Files, previousPath) moveRevision := s.nextRevisionLocked() ws.Revision = moveRevision - s.appendWorkspaceEventLocked(workspaceID, ws, Event{ + event := Event{ EventID: s.nextEventIDLocked(), Type: "file.deleted", Path: previousPath, @@ -4373,7 +4399,9 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a Provider: provider, CorrelationID: correlationID, Timestamp: now, - }) + } + event.ACLPermissions = append([]string(nil), aclPermissions...) + s.appendWorkspaceEventLocked(workspaceID, ws, event) } } @@ -4408,7 +4436,7 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a // Keep update event for sync observability, but still revision-incremented. } } - s.appendWorkspaceEventLocked(workspaceID, ws, Event{ + event := Event{ EventID: s.nextEventIDLocked(), Type: fsEvent, Path: path, @@ -4418,7 +4446,9 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a Provider: provider, CorrelationID: correlationID, Timestamp: now, - }) + } + event.ACLPermissions = append([]string(nil), resolvePermissionsFromFiles(ws.Files, path, true)...) + s.appendWorkspaceEventLocked(workspaceID, ws, event) } func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, action ApplyAction, correlationID string) { @@ -4438,13 +4468,14 @@ func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, a if _, ok := ws.Files[path]; !ok { return } + aclPermissions := resolvePermissionsFromFiles(ws.Files, path, true) delete(ws.Files, path) if objectID != "" { delete(ws.ProviderIndex, providerObjectKey(provider, objectID)) } revision := s.nextRevisionLocked() ws.Revision = revision - s.appendWorkspaceEventLocked(workspaceID, ws, Event{ + event := Event{ EventID: s.nextEventIDLocked(), Type: "file.deleted", Path: path, @@ -4453,7 +4484,9 @@ func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, a Provider: provider, CorrelationID: correlationID, Timestamp: now, - }) + } + event.ACLPermissions = append([]string(nil), aclPermissions...) + s.appendWorkspaceEventLocked(workspaceID, ws, event) } func canonicalizeProviderActionLocked(ws *workspaceState, provider string, action ApplyAction) ApplyAction { @@ -4540,6 +4573,14 @@ func (s *Store) loadFromDisk() error { if ws.ProviderWatermarks == nil { ws.ProviderWatermarks = map[string]string{} } + if ws.ACLPermissionsByEvent == nil { + ws.ACLPermissionsByEvent = map[string][]string{} + } + for index := range ws.Events { + if permissions, ok := ws.ACLPermissionsByEvent[ws.Events[index].EventID]; ok { + ws.Events[index].ACLPermissions = append([]string(nil), permissions...) + } + } } } if snapshot.Forks != nil { @@ -5815,6 +5856,12 @@ func (s *Store) appendWorkspaceEventLocked(workspaceID string, ws *workspaceStat return } ws.Events = append(ws.Events, event) + if event.ACLPermissions != nil { + if ws.ACLPermissionsByEvent == nil { + ws.ACLPermissionsByEvent = map[string][]string{} + } + ws.ACLPermissionsByEvent[event.EventID] = append([]string(nil), event.ACLPermissions...) + } s.publishEvent(workspaceID, event) } diff --git a/internal/relayfile/store_content_hash_test.go b/internal/relayfile/store_content_hash_test.go index cf053a7f..aec9afca 100644 --- a/internal/relayfile/store_content_hash_test.go +++ b/internal/relayfile/store_content_hash_test.go @@ -224,6 +224,43 @@ func TestProviderUpsertPopulatesContentHash(t *testing.T) { t.Fatalf("provider upsert did not materialize file with expected hash") } +func TestProviderRenameEventCapturesACLPermissions(t *testing.T) { + store := NewStoreWithOptions(StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + store.mu.Lock() + ws := store.ensureWorkspaceLocked("ws_provider_acl_rename") + store.applyProviderUpsertLocked(ws, "custom", ApplyAction{ + Type: ActionFileUpsert, + Path: "/custom/old.md", + Content: "secret", + ContentType: "text/plain", + ProviderObjectID: "obj_acl_rename", + Semantics: FileSemantics{Permissions: []string{"deny:agent:Limited"}}, + }, "corr_acl_rename_1") + store.applyProviderUpsertLocked(ws, "custom", ApplyAction{ + Type: ActionFileUpsert, + Path: "/custom/new.md", + Content: "secret", + ContentType: "text/plain", + ProviderObjectID: "obj_acl_rename", + }, "corr_acl_rename_2") + store.mu.Unlock() + + feed, err := store.GetEvents("ws_provider_acl_rename", "", "", 100) + if err != nil { + t.Fatalf("get events failed: %v", err) + } + for _, event := range feed.Events { + if event.Type == "file.deleted" && event.Path == "/custom/old.md" { + if len(event.ACLPermissions) == 0 { + t.Fatalf("provider rename delete lost ACL snapshot: %+v", event) + } + return + } + } + t.Fatal("provider rename did not emit old-path delete event") +} + // TestContentHashSurvivesSaveLoadCycle writes a file via WriteFile, marshals // the in-memory persistedState through Save/Load (round-trip via JSON), and // asserts the ContentHash is preserved on reload. From b0df4ad48361e49138538f5faa025679f5990fdf Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 16:32:56 +0200 Subject: [PATCH 06/12] fix(auth): snapshot ACL on draft deletes; fail closed on unsnapshotted deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconcileAckedDraftLocked and removeDraftLocked emitted file.deleted directly via appendWorkspaceEventLocked (classification-exempt, bypassing recordWriteWithACLPermissionsLocked), so they never populated Event.ACLPermissions. Once the File record left ws.Files, eventVisibleToClaims had nothing left to check but the (now-absent) current file state, so a file-level-denied agent could still see that a protected draft existed and was deleted, over both the HTTP events feed and the websocket feed. Both producers now snapshot the file's ACL permissions before deleting it, exactly like DeleteFile/ applyProviderDeleteLocked/applyProviderUpsertLocked already do. Separately, every ACLPermissions assignment could still collapse an evaluated-but-empty permission set back to nil (append onto a nil dst), making "no ACL snapshot was ever computed" indistinguishable from "ACL was evaluated and no rule applied" — both on directly-emitted events and after a save/load round trip through persistedState.ACLPermissionsByEvent. That ambiguity meant a legacy state.json written before this snapshot mechanism existed (or any future producer that forgets to snapshot) would fail OPEN: eventVisibleToClaims would skip the snapshot check and fall back to a current-state check that can't see permissions on a file that no longer exists. snapshotACLPermissions() now guarantees every computed value is non-nil (even when empty), on every producer and through the load-from-disk restore path, so nil is reserved exclusively for "never evaluated". eventVisibleToClaims fails closed on Type=="file.deleted" with a nil snapshot: a delete event with no recoverable ACL history is hidden rather than assumed unrestricted, while any event whose snapshot (possibly empty) survives is unaffected. Adds adversarial, real-code coverage: ack-rename and sweep delete paths proven to snapshot ACL and to actually hide/reveal correctly for a denied vs. unrestricted agent over HTTP and websocket, plus an upgrade-simulation test that strips one event's persisted snapshot from real on-disk JSON and confirms it fails closed while a sibling event and a fresh post-upgrade delete remain visible. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BPGQHoS7sMMyFxX3iWk2Le Session-Id: c9e3bbb9-166e-4a37-bccf-c5516372bbb5 --- internal/httpapi/server.go | 12 + internal/httpapi/server_test.go | 427 +++++++++++++++++++++ internal/relayfile/draft_reconcile.go | 44 ++- internal/relayfile/draft_reconcile_test.go | 229 +++++++++++ internal/relayfile/store.go | 47 ++- 5 files changed, 734 insertions(+), 25 deletions(-) diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index c7af22f1..afb4353a 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -2374,6 +2374,18 @@ func (s *Server) eventVisibleToClaims(workspaceID string, claims tokenClaims, ev if !scopeMatchesPath(claims.Scopes, "fs:read", path) { return false } + if event.Type == "file.deleted" && event.ACLPermissions == nil { + // Fail closed: relayfile.snapshotACLPermissions guarantees every + // delete event produced by ACL-snapshot-aware code carries a non-nil + // ACLPermissions slice (empty when no rule applied). A nil slice here + // means either a state.json written before the snapshot mechanism + // existed, or a producer that forgot to snapshot — in both cases the + // deleted file's own permissions can no longer be resolved (the file + // is gone from ws.Files), so we cannot rule out a file-level deny + // that would have hidden this path. Denying visibility is the only + // choice that cannot leak a hidden file's prior existence/deletion. + return false + } if event.ACLPermissions != nil && !filePermissionAllows(event.ACLPermissions, workspaceID, &claims, "read", path) { return false } diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index 73834a61..c348391e 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -297,6 +297,433 @@ func TestACLDeleteEventSnapshotAndPathNormalization(t *testing.T) { } } +// TestACLAckRenameDeleteEventHiddenFromDeniedAgent is the adversarial, +// real-code end-to-end proof for the ack-rename half of ACL-423: +// reconcileAckedDraftLocked emits the draft's file.deleted directly via +// appendWorkspaceEventLocked (classification-exempt, bypassing +// recordWriteWithACLPermissionsLocked), so it must independently snapshot +// the draft's ACL before the File record disappears from ws.Files. This +// drives the real WriteFile -> AcknowledgeWriteback production path (not a +// synthetic relayfile.Event) and asserts what a file-level-denied agent +// actually receives over both the HTTP events feed and the websocket +// catch-up feed. +func TestACLAckRenameDeleteEventHiddenFromDeniedAgent(t *testing.T) { + t.Parallel() + const workspaceID = "ws_acl_ack_rename_delete" + const draftPath = "/slack/channels/C0ALQ06AAUT/messages/messages 0e89a031-65f0-480e-a823-ab1d94b324ea.json" + const canonicalPath = "/slack/channels/C0ALQ06AAUT/messages/1780018871.351819.json" + const denyRule = "deny:agent:Limited" + + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true, ExternalWritebackMode: true}) + t.Cleanup(store.Close) + created, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, + Path: draftPath, + IfMatch: "0", + ContentType: "application/json", + Content: `{"text":"hi"}`, + // allow:public makes this an ordinary "everyone but Limited" ACL — + // filePermissionAllows fails closed for ANY agent once an + // enforceable rule exists with no matching allow, so a bare + // deny:agent:Limited alone would (correctly) also hide this from + // Trusted, and the test needs a genuine visible/hidden split to be + // adversarial. + Semantics: relayfile.FileSemantics{Permissions: []string{"allow:public", denyRule}}, + CorrelationID: "corr_draft_write", + }) + if err != nil { + t.Fatalf("seed protected draft failed: %v", err) + } + if _, err := store.AcknowledgeWriteback(workspaceID, created.OpID, relayfile.WritebackAck{ + Success: true, + ExternalID: "1780018871.351819", + }, "corr_ack_1"); err != nil { + t.Fatalf("ack failed: %v", err) + } + if _, err := store.ReadFile(workspaceID, canonicalPath); err != nil { + t.Fatalf("expected the draft renamed to the canonical path: %v", err) + } + + server := httptest.NewServer(NewServer(store)) + // Cleanup (not defer): a parallel subtest resumes only after this + // function body returns, which happens immediately once t.Run hits + // t.Parallel() — a plain defer here would close the server out from + // under the still-pending subtests. + t.Cleanup(server.Close) + + cases := []struct { + name string + agentName string + wantVisible bool + }{ + {name: "file_level_denied_agent_cannot_see_draft_ever_existed", agentName: "Limited", wantVisible: false}, + {name: "unrestricted_agent_sees_the_full_rename", agentName: "Trusted", wantVisible: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + token := mustTestJWT(t, "dev-secret", workspaceID, tc.agentName, []string{"fs:read"}, time.Now().Add(time.Hour)) + headers := map[string]string{"Authorization": "Bearer " + token, "X-Correlation-Id": "corr_" + tc.name} + + resp := doRequest(t, NewServer(store), request{method: http.MethodGet, path: "/v1/workspaces/" + workspaceID + "/fs/events?limit=100", headers: headers}) + if resp.Code != http.StatusOK { + t.Fatalf("events returned %d: %s", resp.Code, resp.Body.String()) + } + var feed relayfile.EventFeed + if err := json.NewDecoder(resp.Body).Decode(&feed); err != nil { + t.Fatalf("decode events: %v", err) + } + var sawDraftDelete, sawCanonicalCreate bool + for _, event := range feed.Events { + if event.Type == "file.deleted" && event.Path == draftPath { + sawDraftDelete = true + } + if event.Type == "file.created" && event.Path == canonicalPath { + sawCanonicalCreate = true + } + } + if sawDraftDelete != tc.wantVisible { + t.Fatalf("HTTP feed: draft delete visibility = %v, want %v (events=%+v)", sawDraftDelete, tc.wantVisible, feed.Events) + } + if sawCanonicalCreate != tc.wantVisible { + t.Fatalf("HTTP feed: canonical create visibility = %v, want %v (events=%+v)", sawCanonicalCreate, tc.wantVisible, feed.Events) + } + + wsURL := "ws" + strings.TrimPrefix(server.URL, "http") + "/v1/workspaces/" + workspaceID + "/fs/ws?token=" + token + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + conn, _, err := websocket.Dial(ctx, wsURL, nil) + if err != nil { + t.Fatalf("websocket dial failed: %v", err) + } + defer conn.Close(websocket.StatusNormalClosure, "") + + if tc.wantVisible { + var originalCreated map[string]any + if err := wsjson.Read(ctx, conn, &originalCreated); err != nil { + t.Fatalf("read original draft-created catch-up event: %v", err) + } + if originalCreated["type"] != "file.created" || originalCreated["path"] != draftPath { + t.Fatalf("unexpected first catch-up event: %+v", originalCreated) + } + var deleted map[string]any + if err := wsjson.Read(ctx, conn, &deleted); err != nil { + t.Fatalf("read draft-deleted catch-up event: %v", err) + } + if deleted["type"] != "file.deleted" || deleted["path"] != draftPath { + t.Fatalf("unexpected second catch-up event: %+v", deleted) + } + var createdEvt map[string]any + if err := wsjson.Read(ctx, conn, &createdEvt); err != nil { + t.Fatalf("read canonical-created catch-up event: %v", err) + } + if createdEvt["type"] != "file.created" || createdEvt["path"] != canonicalPath { + t.Fatalf("unexpected third catch-up event: %+v", createdEvt) + } + } + // Whether or not the rename was visible, no further catch-up + // events remain queued: for the denied agent this proves ZERO + // leakage (not even a truncated/garbled event — the original + // draft creation is denied the same way), and for the + // unrestricted agent it proves nothing beyond the three expected + // events was emitted. + if err := wsjson.Write(ctx, conn, map[string]any{"type": "ping"}); err != nil { + t.Fatalf("write ping failed: %v", err) + } + var pong map[string]any + if err := wsjson.Read(ctx, conn, &pong); err != nil || pong["type"] != "pong" { + t.Fatalf("expected pong immediately after catch-up (wantVisible=%v), got %+v (%v)", tc.wantVisible, pong, err) + } + }) + } +} + +// TestACLSweepDeleteEventHiddenFromDeniedAgent is the adversarial, real-code +// end-to-end proof for the sweep half of ACL-423: SweepWritebackDrafts's +// removal also goes through removeDraftLocked's classification-exempt +// appendWorkspaceEventLocked call. This drives WriteFile -> ack (clearing the +// pending op, mirroring how a delivered draft's op stops blocking the sweep) +// -> the real SweepWritebackDrafts entry point, then asserts visibility over +// the real HTTP events endpoint for a file-level-denied agent vs. an +// unrestricted one. +func TestACLSweepDeleteEventHiddenFromDeniedAgent(t *testing.T) { + t.Parallel() + const workspaceID = "ws_acl_sweep_delete" + const residuePath = "/slack/channels/C0ALQ06AAUT/messages/messages 15250fcf-de54-44b0-a808-c8e514480647.json" + const denyRule = "deny:agent:Limited" + + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true, ExternalWritebackMode: true}) + t.Cleanup(store.Close) + created, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, + Path: residuePath, + IfMatch: "0", + ContentType: "application/json", + Content: `{"text":"old"}`, + // allow:public + deny:agent:Limited: see the comment in + // TestACLAckRenameDeleteEventHiddenFromDeniedAgent on why a bare + // deny rule alone can't produce a visible/hidden split here. + Semantics: relayfile.FileSemantics{Permissions: []string{"allow:public", denyRule}}, + CorrelationID: "corr_residue_write", + }) + if err != nil { + t.Fatalf("seed protected residue failed: %v", err) + } + // Ack without an externalId: the op stops blocking the sweep (no longer + // pending/running) while the draft itself is left untouched — exactly + // the "delivered, but pre-dates the rename-at-ack contract" residue + // shape SweepWritebackDrafts exists to drain. + if _, err := store.AcknowledgeWriteback(workspaceID, created.OpID, relayfile.WritebackAck{Success: true}, "corr_ack_1"); err != nil { + t.Fatalf("ack failed: %v", err) + } + if _, err := store.ReadFile(workspaceID, residuePath); err != nil { + t.Fatalf("draft must remain until swept: %v", err) + } + sweepResult, err := store.SweepWritebackDrafts(workspaceID, relayfile.SweepDraftsRequest{Apply: true, CorrelationID: "corr_sweep_1"}) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if len(sweepResult.Removed) != 1 || sweepResult.Removed[0].Path != residuePath { + t.Fatalf("expected the protected residue swept, got %v", sweepResult.Removed) + } + + server := NewServer(store) + + cases := []struct { + name string + agentName string + wantVisible bool + }{ + {name: "file_level_denied_agent_cannot_see_swept_residue", agentName: "Limited", wantVisible: false}, + {name: "unrestricted_agent_sees_swept_residue", agentName: "Trusted", wantVisible: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + token := mustTestJWT(t, "dev-secret", workspaceID, tc.agentName, []string{"fs:read"}, time.Now().Add(time.Hour)) + headers := map[string]string{"Authorization": "Bearer " + token, "X-Correlation-Id": "corr_" + tc.name} + resp := doRequest(t, server, request{method: http.MethodGet, path: "/v1/workspaces/" + workspaceID + "/fs/events?limit=100", headers: headers}) + if resp.Code != http.StatusOK { + t.Fatalf("events returned %d: %s", resp.Code, resp.Body.String()) + } + var feed relayfile.EventFeed + if err := json.NewDecoder(resp.Body).Decode(&feed); err != nil { + t.Fatalf("decode events: %v", err) + } + var sawResidueDelete bool + for _, event := range feed.Events { + if event.Type == "file.deleted" && event.Path == residuePath { + sawResidueDelete = true + } + } + if sawResidueDelete != tc.wantVisible { + t.Fatalf("residue delete visibility = %v, want %v (events=%+v)", sawResidueDelete, tc.wantVisible, feed.Events) + } + }) + } +} + +// TestLegacyFileDeletedEventWithoutACLSnapshotFailsClosedAfterUpgrade covers +// finding (2): a state.json written before the ACL-snapshot mechanism +// existed (or by any producer that failed to snapshot) has file.deleted +// events with no recoverable ACL history. Since the deleted file's own File +// record — the only place a file-level permission could have lived — is +// gone, there is no way to tell "definitely never restricted" apart from +// "restricted, but we can no longer prove it". This simulates that upgrade +// by round-tripping a real on-disk snapshot through the JSON state backend +// and surgically removing one event's recorded ACL snapshot (as if it had +// been written by code that pre-dates ACLPermissionsByEvent), then asserts: +// - the reloaded legacy event comes back with ACLPermissions == nil +// (relayfile-side migration signal for "unknown"), +// - it fails closed over both the internal visibility check and the real +// HTTP events endpoint, even for an agent with no matching deny rule — +// because we cannot rule one out, +// - a sibling delete event whose (empty) snapshot DID survive reload +// stays visible, and a brand-new delete created after the reload in the +// same process also stays visible — the fail-closed rule must not +// swallow ordinary, fully-verified post-upgrade behavior. +func TestLegacyFileDeletedEventWithoutACLSnapshotFailsClosedAfterUpgrade(t *testing.T) { + t.Parallel() + const workspaceID = "ws_legacy_acl_upgrade" + const protectedPath = "/legacy/protected.md" + const unrestrictedPath = "/legacy/public.md" + const denyRule = "deny:agent:Limited" + + statePath := filepath.Join(t.TempDir(), "state.json") + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{ + StateBackend: relayfile.NewJSONFileStateBackend(statePath), + DisableWorkers: true, + }) + + protected, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, + Path: protectedPath, + IfMatch: "0", + Content: "secret", + Semantics: relayfile.FileSemantics{Permissions: []string{denyRule}}, + }) + if err != nil { + t.Fatalf("seed protected file failed: %v", err) + } + if _, err := store.DeleteFile(relayfile.DeleteRequest{WorkspaceID: workspaceID, Path: protectedPath, IfMatch: protected.TargetRevision}); err != nil { + t.Fatalf("delete protected file failed: %v", err) + } + unrestricted, err := store.WriteFile(relayfile.WriteRequest{ + WorkspaceID: workspaceID, + Path: unrestrictedPath, + IfMatch: "0", + Content: "public", + }) + if err != nil { + t.Fatalf("seed unrestricted file failed: %v", err) + } + if _, err := store.DeleteFile(relayfile.DeleteRequest{WorkspaceID: workspaceID, Path: unrestrictedPath, IfMatch: unrestricted.TargetRevision}); err != nil { + t.Fatalf("delete unrestricted file failed: %v", err) + } + + feed, err := store.GetEvents(workspaceID, "", "", 100) + if err != nil { + t.Fatalf("get events failed: %v", err) + } + var protectedDeleteID, unrestrictedDeleteID string + for _, event := range feed.Events { + switch { + case event.Type == "file.deleted" && event.Path == protectedPath: + protectedDeleteID = event.EventID + case event.Type == "file.deleted" && event.Path == unrestrictedPath: + unrestrictedDeleteID = event.EventID + } + } + if protectedDeleteID == "" || unrestrictedDeleteID == "" { + t.Fatalf("expected both delete events recorded, got %+v", feed.Events) + } + store.Close() + + // Surgically strip the recorded ACL snapshot for the protected delete + // only, simulating a state.json written before ACLPermissionsByEvent + // existed (the unrestricted delete's entry — empty but present — is left + // alone, simulating an entry that DID survive from a version with this + // fix already applied). + raw, err := os.ReadFile(statePath) + if err != nil { + t.Fatalf("read state.json: %v", err) + } + var snapshot map[string]any + if err := json.Unmarshal(raw, &snapshot); err != nil { + t.Fatalf("unmarshal state.json: %v", err) + } + workspaces, _ := snapshot["workspaces"].(map[string]any) + wsSnapshot, _ := workspaces[workspaceID].(map[string]any) + aclMap, _ := wsSnapshot["aclPermissionsByEvent"].(map[string]any) + if aclMap == nil { + t.Fatalf("expected aclPermissionsByEvent in persisted snapshot: %+v", wsSnapshot) + } + if _, ok := aclMap[protectedDeleteID]; !ok { + t.Fatalf("expected a persisted ACL snapshot entry for the protected delete before stripping it: %+v", aclMap) + } + delete(aclMap, protectedDeleteID) + patched, err := json.Marshal(snapshot) + if err != nil { + t.Fatalf("marshal patched state.json: %v", err) + } + if err := os.WriteFile(statePath, patched, 0o644); err != nil { + t.Fatalf("write patched state.json: %v", err) + } + + // Reopen exactly as a fresh process would on upgrade. + reloaded := relayfile.NewStoreWithOptions(relayfile.StoreOptions{ + StateBackend: relayfile.NewJSONFileStateBackend(statePath), + DisableWorkers: true, + }) + t.Cleanup(reloaded.Close) + server := NewServer(reloaded) + // AnyAgent carries no ACL rule targeting it anywhere: the point is that + // fail-closed hides the legacy event even from an agent nothing denies. + claims := tokenClaims{WorkspaceID: workspaceID, AgentName: "AnyAgent", Scopes: map[string]struct{}{"fs:read": {}}} + + reloadedFeed, err := reloaded.GetEvents(workspaceID, "", "", 100) + if err != nil { + t.Fatalf("get reloaded events failed: %v", err) + } + var checkedProtected, checkedUnrestricted bool + for _, event := range reloadedFeed.Events { + switch event.EventID { + case protectedDeleteID: + checkedProtected = true + if event.ACLPermissions != nil { + t.Fatalf("expected the legacy delete event to reload with ACLPermissions == nil (unknown), got %v", event.ACLPermissions) + } + if server.eventVisibleToClaims(workspaceID, claims, event) { + t.Fatal("legacy delete event with no recoverable ACL snapshot must fail closed, even for an agent no rule names — the deleted file may have had a file-level deny we can no longer verify") + } + case unrestrictedDeleteID: + checkedUnrestricted = true + if event.ACLPermissions == nil { + t.Fatal("expected the sibling delete event's surviving (empty) ACL snapshot to reload as non-nil") + } + if !server.eventVisibleToClaims(workspaceID, claims, event) { + t.Fatal("preserving allowed behavior: a delete event whose ACL snapshot survived reload (even empty) must remain visible") + } + } + } + if !checkedProtected || !checkedUnrestricted { + t.Fatalf("expected to find both delete events after reload, got %+v", reloadedFeed.Events) + } + + // A brand-new delete created after the upgrade, in the same reloaded + // process, must also stay visible: the fail-closed rule is scoped to + // genuinely unsnapshotted events, not "any file.deleted with no + // permissions". + freshPath := "/legacy/fresh.md" + fresh, err := reloaded.WriteFile(relayfile.WriteRequest{WorkspaceID: workspaceID, Path: freshPath, IfMatch: "0", Content: "fresh"}) + if err != nil { + t.Fatalf("seed fresh file failed: %v", err) + } + if _, err := reloaded.DeleteFile(relayfile.DeleteRequest{WorkspaceID: workspaceID, Path: freshPath, IfMatch: fresh.TargetRevision}); err != nil { + t.Fatalf("delete fresh file failed: %v", err) + } + freshFeed, err := reloaded.GetEvents(workspaceID, "", "", 100) + if err != nil { + t.Fatalf("get fresh events failed: %v", err) + } + var freshVisible, sawFresh bool + for _, event := range freshFeed.Events { + if event.Type == "file.deleted" && event.Path == freshPath { + sawFresh = true + freshVisible = server.eventVisibleToClaims(workspaceID, claims, event) + } + } + if !sawFresh { + t.Fatalf("expected the fresh post-upgrade delete event, got %+v", freshFeed.Events) + } + if !freshVisible { + t.Fatal("a fresh, fully-snapshotted post-upgrade delete event must remain visible") + } + + // And confirm the leak is actually closed over the real HTTP surface, + // not just the internal helper. + httpServer := httptest.NewServer(server) + defer httpServer.Close() + token := mustTestJWT(t, "dev-secret", workspaceID, "AnyAgent", []string{"fs:read"}, time.Now().Add(time.Hour)) + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/events?limit=100", + headers: map[string]string{"Authorization": "Bearer " + token, "X-Correlation-Id": "corr_legacy_http"}, + }) + if resp.Code != http.StatusOK { + t.Fatalf("events returned %d: %s", resp.Code, resp.Body.String()) + } + var httpFeed relayfile.EventFeed + if err := json.NewDecoder(resp.Body).Decode(&httpFeed); err != nil { + t.Fatalf("decode events: %v", err) + } + for _, event := range httpFeed.Events { + if event.Path == protectedPath && event.Type == "file.deleted" { + t.Fatalf("legacy unsnapshotted delete leaked over HTTP: %+v", event) + } + } +} + func TestFileEventsWebSocketCursorCatchUpDrainsMoreThanOnePage(t *testing.T) { store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) t.Cleanup(store.Close) diff --git a/internal/relayfile/draft_reconcile.go b/internal/relayfile/draft_reconcile.go index 5ae194e3..02979da7 100644 --- a/internal/relayfile/draft_reconcile.go +++ b/internal/relayfile/draft_reconcile.go @@ -207,6 +207,12 @@ func (s *Store) reconcileAckedDraftLocked(workspaceID string, ws *workspaceState // consumers are eventId-keyed, so neither convention affects cursors. nowTS := nowRFC3339NanoUTC() revision := s.nextRevisionLocked() + // Snapshot the draft's own ACL state before it disappears from ws.Files: + // the file.deleted event below is the only remaining record of what + // governed reads at draftPath, and it must carry that snapshot so a + // file-level denied agent cannot learn the draft existed/was deleted by + // watching the event feed. See snapshotACLPermissions. + aclPermissions := snapshotACLPermissions(resolvePermissionsFromFiles(ws.Files, draftPath, true)) delete(ws.Files, draftPath) file.Path = targetPath file.Revision = revision @@ -218,14 +224,15 @@ func (s *Store) reconcileAckedDraftLocked(workspaceID string, ws *workspaceState ws.ProviderIndex[key] = targetPath s.appendWorkspaceEventLocked(workspaceID, ws, Event{ - EventID: s.nextEventIDLocked(), - Type: "file.deleted", - Path: draftPath, - Revision: revision, - Origin: "system", - Provider: provider, - CorrelationID: correlationID, - Timestamp: nowTS, + EventID: s.nextEventIDLocked(), + Type: "file.deleted", + Path: draftPath, + Revision: revision, + Origin: "system", + Provider: provider, + CorrelationID: correlationID, + Timestamp: nowTS, + ACLPermissions: aclPermissions, }) s.appendWorkspaceEventLocked(workspaceID, ws, Event{ EventID: s.nextEventIDLocked(), @@ -270,18 +277,23 @@ func (s *Store) removeDraftLocked(workspaceID string, ws *workspaceState, draftP if _, exists := ws.Files[draftPath]; !exists { return } + // Snapshot before delete — see the matching comment in + // reconcileAckedDraftLocked. This path is shared by ack-time draft + // cleanup and SweepWritebackDrafts, so both call sites inherit the fix. + aclPermissions := snapshotACLPermissions(resolvePermissionsFromFiles(ws.Files, draftPath, true)) delete(ws.Files, draftPath) revision := s.nextRevisionLocked() ws.Revision = revision s.appendWorkspaceEventLocked(workspaceID, ws, Event{ - EventID: s.nextEventIDLocked(), - Type: "file.deleted", - Path: draftPath, - Revision: revision, - Origin: "system", - Provider: provider, - CorrelationID: correlationID, - Timestamp: nowRFC3339NanoUTC(), + EventID: s.nextEventIDLocked(), + Type: "file.deleted", + Path: draftPath, + Revision: revision, + Origin: "system", + Provider: provider, + ACLPermissions: aclPermissions, + CorrelationID: correlationID, + Timestamp: nowRFC3339NanoUTC(), }) } diff --git a/internal/relayfile/draft_reconcile_test.go b/internal/relayfile/draft_reconcile_test.go index 901d0446..157ac4f8 100644 --- a/internal/relayfile/draft_reconcile_test.go +++ b/internal/relayfile/draft_reconcile_test.go @@ -48,6 +48,41 @@ func writeDraft(t *testing.T, store *Store, workspaceID, path, content string) W return result } +// writeProtectedDraft writes a draft carrying a file-level ACL deny rule, so +// tests can prove the rule survives into the delete event's ACL snapshot +// after the draft is renamed/removed and its File record is gone from +// ws.Files. +func writeProtectedDraft(t *testing.T, store *Store, workspaceID, path, content string, permissions []string) WriteResult { + t.Helper() + result, err := store.WriteFile(WriteRequest{ + WorkspaceID: workspaceID, + Path: path, + IfMatch: "0", + ContentType: "application/json", + Content: content, + Semantics: FileSemantics{Permissions: permissions}, + CorrelationID: "corr_draft_write", + }) + if err != nil { + t.Fatalf("protected draft write failed: %v", err) + } + return result +} + +// deleteEventForPath returns the (last) file.deleted event recorded for path, +// or nil if none was found. +func deleteEventForPath(t *testing.T, store *Store, workspaceID, path string) *Event { + t.Helper() + var found *Event + for _, event := range eventsForPath(t, store, workspaceID, path) { + if event.Type == "file.deleted" { + e := event + found = &e + } + } + return found +} + func opCount(t *testing.T, store *Store, workspaceID string) int { t.Helper() feed, err := store.ListOperations(workspaceID, "", "", "", "", 1000) @@ -395,6 +430,15 @@ func TestRenamedDraftConvergesWithLaterProviderSync(t *testing.T) { } func seedResidueFile(t *testing.T, store *Store, workspaceID, path, content string) { + t.Helper() + seedResidueFileWithPermissions(t, store, workspaceID, path, content, nil) +} + +// seedResidueFileWithPermissions seeds residue carrying a file-level ACL rule +// directly, bypassing WriteFile/writeback bookkeeping — mirroring how real +// residue accumulates (the op that created it is long gone by restart time) +// while still exercising the ACL-snapshot code path under sweep. +func seedResidueFileWithPermissions(t *testing.T, store *Store, workspaceID, path, content string, permissions []string) { t.Helper() store.mu.Lock() ws := store.ensureWorkspaceLocked(workspaceID) @@ -405,6 +449,7 @@ func seedResidueFile(t *testing.T, store *Store, workspaceID, path, content stri ContentType: "application/json", Content: content, Provider: inferProviderFromPath(path), + Semantics: FileSemantics{Permissions: permissions}, } ws.Revision = revision store.mu.Unlock() @@ -669,3 +714,187 @@ func TestDraftBasenameDetection(t *testing.T) { t.Fatalf("sanity: draftFile space-form fixture lost its space") } } + +// --- ACL snapshot on classification-exempt deletes (ACL-423) --- +// +// reconcileAckedDraftLocked and removeDraftLocked emit file.deleted directly +// via appendWorkspaceEventLocked, bypassing recordWriteLocked/ +// recordWriteWithACLPermissionsLocked entirely (that bypass is the whole +// point — see the file-level doc comment). Before this fix that meant they +// never populated Event.ACLPermissions, so httpapi.eventVisibleToClaims fell +// through to checking the CURRENT filesystem state at the (now-deleted) +// path — which can never see a file-level deny rule that lived only on the +// File record that was just removed from ws.Files. A file-level-denied agent +// could therefore learn that a protected draft existed and was deleted, over +// both the HTTP /fs/events feed and the live/catch-up websocket, despite +// never having been able to read it. These tests pin that the ACL snapshot +// captured before deletion (a) is present and (b) actually carries the +// deny rule, for every producer that funnels through removeDraftLocked: the +// ack-time rename, the ack-time "canonical already materialized" removal, +// and the residue sweep. httpapi-level adversarial coverage (denied agent +// really can't see the event over HTTP/WS) lives in +// internal/httpapi/server_test.go, which cannot reach these unexported +// producers directly. + +func TestAckRenameEmitsACLSnapshotForProtectedDraft(t *testing.T) { + store := newExternalStore(t) + draftPath := "/slack/channels/C0ALQ06AAUT/messages/messages " + draftUUIDA + ".json" + denyRule := "deny:agent:Limited" + result := writeProtectedDraft(t, store, "ws_1", draftPath, `{"text":"hi"}`, []string{denyRule}) + + if _, err := store.AcknowledgeWriteback("ws_1", result.OpID, WritebackAck{ + Success: true, + ExternalID: "1780018871.351819", + }, "corr_ack_1"); err != nil { + t.Fatalf("ack failed: %v", err) + } + + event := deleteEventForPath(t, store, "ws_1", draftPath) + if event == nil { + t.Fatalf("expected a file.deleted event for renamed draft %s", draftPath) + } + if len(event.ACLPermissions) == 0 { + t.Fatalf("ack rename delete event lost the draft's ACL snapshot: %+v", event) + } + var sawDeny bool + for _, rule := range event.ACLPermissions { + if rule == denyRule { + sawDeny = true + } + } + if !sawDeny { + t.Fatalf("ack rename delete event snapshot missing %q, got %v", denyRule, event.ACLPermissions) + } +} + +func TestAckRemovalWhenCanonicalAlreadyMaterializedEmitsACLSnapshotForProtectedDraft(t *testing.T) { + store := newExternalStore(t) + draftPath := "/slack/channels/C0ALQ06AAUT/messages/messages " + draftUUIDA + ".json" + denyRule := "deny:agent:Limited" + result := writeProtectedDraft(t, store, "ws_1", draftPath, `{"text":"hi"}`, []string{denyRule}) + + canonicalPath := "/slack/channels/C0ALQ06AAUT/messages/1780018871_351819.json" + store.mu.Lock() + ws := store.ensureWorkspaceLocked("ws_1") + store.applyProviderUpsertLocked(ws, "slack", ApplyAction{ + Type: ActionFileUpsert, + Path: canonicalPath, + Content: `{"text":"hi","ts":"1780018871.351819"}`, + ContentType: "application/json", + ProviderObjectID: "1780018871.351819", + }, "corr_sync_1") + store.mu.Unlock() + + resp, err := store.AcknowledgeWriteback("ws_1", result.OpID, WritebackAck{ + Success: true, + ExternalID: "1780018871.351819", + }, "corr_ack_1") + if err != nil { + t.Fatalf("ack failed: %v", err) + } + draft, _ := resp["draft"].(map[string]any) + if draft == nil || draft["action"] != "removed" { + t.Fatalf("expected removed disposition (this test exercises removeDraftLocked via the ack path), got %v", resp) + } + + event := deleteEventForPath(t, store, "ws_1", draftPath) + if event == nil { + t.Fatalf("expected a file.deleted event for removed draft %s", draftPath) + } + if len(event.ACLPermissions) == 0 { + t.Fatalf("ack removal delete event lost the draft's ACL snapshot: %+v", event) + } + var sawDeny bool + for _, rule := range event.ACLPermissions { + if rule == denyRule { + sawDeny = true + } + } + if !sawDeny { + t.Fatalf("ack removal delete event snapshot missing %q, got %v", denyRule, event.ACLPermissions) + } +} + +func TestSweepAppliedRemovalEmitsACLSnapshotForProtectedResidue(t *testing.T) { + store := newExternalStore(t) + denyRule := "deny:agent:Limited" + residue := "/slack/channels/C0ALQ06AAUT/messages/messages " + draftUUIDA + ".json" + seedResidueFileWithPermissions(t, store, "ws_1", residue, `{"text":"old"}`, []string{denyRule}) + + result, err := store.SweepWritebackDrafts("ws_1", SweepDraftsRequest{ + Apply: true, + CorrelationID: "corr_sweep_1", + }) + if err != nil { + t.Fatalf("sweep failed: %v", err) + } + if len(result.Removed) != 1 || result.Removed[0].Path != residue { + t.Fatalf("expected the protected residue removed, got %v", result.Removed) + } + + event := deleteEventForPath(t, store, "ws_1", residue) + if event == nil { + t.Fatalf("expected a file.deleted event for swept residue %s", residue) + } + if len(event.ACLPermissions) == 0 { + t.Fatalf("sweep delete event lost the residue's ACL snapshot: %+v", event) + } + var sawDeny bool + for _, rule := range event.ACLPermissions { + if rule == denyRule { + sawDeny = true + } + } + if !sawDeny { + t.Fatalf("sweep delete event snapshot missing %q, got %v", denyRule, event.ACLPermissions) + } +} + +// TestUnrestrictedDraftDeletesCarryNonNilACLSnapshot guards the invariant +// httpapi.eventVisibleToClaims now depends on for fail-closed legacy +// handling: Event.ACLPermissions must be non-nil (even if empty) for every +// file.deleted event this version's code computed a snapshot for, so nil +// unambiguously means "no snapshot was ever computed" (legacy/pre-migration +// or a producer bug) rather than "computed, and no ACL rule applied". If a +// draft-delete producer regresses to leaving ACLPermissions nil for the +// ordinary unrestricted case, httpapi would now wrongly treat every routine +// draft deletion as an unverifiable legacy event and hide it from everyone. +func TestUnrestrictedDraftDeletesCarryNonNilACLSnapshot(t *testing.T) { + t.Run("ack_rename", func(t *testing.T) { + store := newExternalStore(t) + draftPath := "/slack/channels/C0ALQ06AAUT/messages/messages " + draftUUIDA + ".json" + result := writeDraft(t, store, "ws_1", draftPath, `{"text":"hi"}`) + if _, err := store.AcknowledgeWriteback("ws_1", result.OpID, WritebackAck{ + Success: true, + ExternalID: "1780018871.351819", + }, "corr_ack_1"); err != nil { + t.Fatalf("ack failed: %v", err) + } + event := deleteEventForPath(t, store, "ws_1", draftPath) + if event == nil { + t.Fatalf("expected a file.deleted event for renamed draft %s", draftPath) + } + if event.ACLPermissions == nil { + t.Fatalf("unrestricted ack-rename delete event has nil ACLPermissions: httpapi will now wrongly treat it as an unverifiable legacy event and hide it") + } + }) + + t.Run("sweep", func(t *testing.T) { + store := newExternalStore(t) + residue := "/slack/channels/C0ALQ06AAUT/messages/messages " + draftUUIDA + ".json" + seedResidueFile(t, store, "ws_1", residue, `{"text":"old"}`) + if _, err := store.SweepWritebackDrafts("ws_1", SweepDraftsRequest{ + Apply: true, + CorrelationID: "corr_sweep_1", + }); err != nil { + t.Fatalf("sweep failed: %v", err) + } + event := deleteEventForPath(t, store, "ws_1", residue) + if event == nil { + t.Fatalf("expected a file.deleted event for swept residue %s", residue) + } + if event.ACLPermissions == nil { + t.Fatalf("unrestricted sweep delete event has nil ACLPermissions: httpapi will now wrongly treat it as an unverifiable legacy event and hide it") + } + }) +} diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index b56a19ff..cb98ffdc 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -3806,11 +3806,14 @@ func (s *Store) recordWriteWithContentIdentityAndACLPermissionsLocked(ws *worksp CorrelationID: correlationID, Timestamp: nowTS, } - if !snapshotACL && strings.HasPrefix(eventType, "file.") { - aclPermissions = resolvePermissionsFromFiles(ws.Files, path, eventType != "file.deleted") - } - if snapshotACL || aclPermissions != nil { - event.ACLPermissions = append([]string(nil), aclPermissions...) + if strings.HasPrefix(eventType, "file.") { + if !snapshotACL { + aclPermissions = resolvePermissionsFromFiles(ws.Files, path, eventType != "file.deleted") + } + // Always non-nil (snapshotACLPermissions), even when aclPermissions is + // nil/empty: a nil Event.ACLPermissions must mean "never evaluated", + // not "evaluated, no rules applied" — see snapshotACLPermissions. + event.ACLPermissions = snapshotACLPermissions(aclPermissions) } s.appendWorkspaceEventLocked(workspaceID, ws, event) @@ -4400,7 +4403,7 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a CorrelationID: correlationID, Timestamp: now, } - event.ACLPermissions = append([]string(nil), aclPermissions...) + event.ACLPermissions = snapshotACLPermissions(aclPermissions) s.appendWorkspaceEventLocked(workspaceID, ws, event) } } @@ -4447,7 +4450,7 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a CorrelationID: correlationID, Timestamp: now, } - event.ACLPermissions = append([]string(nil), resolvePermissionsFromFiles(ws.Files, path, true)...) + event.ACLPermissions = snapshotACLPermissions(resolvePermissionsFromFiles(ws.Files, path, true)) s.appendWorkspaceEventLocked(workspaceID, ws, event) } @@ -4577,8 +4580,17 @@ func (s *Store) loadFromDisk() error { ws.ACLPermissionsByEvent = map[string][]string{} } for index := range ws.Events { + // A present map entry (even one whose value is empty/nil) + // means this event's ACL snapshot was actually computed — + // restore it as a non-nil slice so it stays distinguishable + // from a legacy event, which has no entry at all here and is + // left with ACLPermissions == nil. httpapi.eventVisibleToClaims + // fails closed on Type=="file.deleted" with a nil snapshot, + // so an absent entry (pre-dating this snapshot mechanism, or + // written by a version with the now-fixed nil-collapse bug) + // is hidden rather than assumed unrestricted. if permissions, ok := ws.ACLPermissionsByEvent[ws.Events[index].EventID]; ok { - ws.Events[index].ACLPermissions = append([]string(nil), permissions...) + ws.Events[index].ACLPermissions = snapshotACLPermissions(permissions) } } } @@ -5172,6 +5184,23 @@ func queryFilesFromEntries(iterate func(func(string, File)), req FileQueryReques return FileQueryResponse{Items: items, NextCursor: nextCursor}, nil } +// snapshotACLPermissions returns a defensive copy of permissions that is +// NEVER nil, even when permissions is nil/empty. Event.ACLPermissions relies +// on the nil/non-nil distinction to tell "no ACL snapshot was ever computed +// for this event" (nil — legacy, pre-dates the snapshot mechanism, or a +// producer bug) apart from "ACL was evaluated and no rules applied" (non-nil +// empty slice). Every delete-event producer must route its computed +// permissions through this helper before assigning Event.ACLPermissions; +// skipping it silently reintroduces the nil ambiguity that lets a legacy or +// unsnapshotted delete fail open instead of closed. See +// httpapi.eventVisibleToClaims, which fails closed on Type=="file.deleted" +// with ACLPermissions == nil. +func snapshotACLPermissions(permissions []string) []string { + out := make([]string, len(permissions)) + copy(out, permissions) + return out +} + func resolvePermissionsFromFiles(files map[string]File, path string, includeTarget bool) []string { target := normalizePath(path) permissions := make([]string, 0, 8) @@ -5860,7 +5889,7 @@ func (s *Store) appendWorkspaceEventLocked(workspaceID string, ws *workspaceStat if ws.ACLPermissionsByEvent == nil { ws.ACLPermissionsByEvent = map[string][]string{} } - ws.ACLPermissionsByEvent[event.EventID] = append([]string(nil), event.ACLPermissions...) + ws.ACLPermissionsByEvent[event.EventID] = snapshotACLPermissions(event.ACLPermissions) } s.publishEvent(workspaceID, event) } From 45820e7a50002a19245f2c5984df996bd650d455 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 16:59:37 +0200 Subject: [PATCH 07/12] fix(auth): route provider-sync deletes through snapshotACLPermissions; fail closed on empty-path deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fresh review on b0df4ad4 found two blockers: P1: applyProviderDeleteLocked (the provider-webhook delete path, distinct from DeleteFile/CommitForkWithValidator/applyProviderUpsertLocked/the draft-reconcile paths, all already fixed) still assigned event.ACLPermissions = append([]string(nil), aclPermissions...). For an unrestricted file that collapses back to nil (append of zero elements onto a nil slice returns nil) — indistinguishable from "never snapshotted" — so eventVisibleToClaims' fail-closed guard on Type=="file.deleted" && ACLPermissions==nil hid the event from EVERY agent, not just a denied one. Now routes through snapshotACLPermissions like every other producer. P2: eventVisibleToClaims checked `path == "" { return true }` before the file.deleted nil-snapshot fail-closed guard. A malformed or corrupted persisted file.deleted event carrying an empty Path — not producible by any current in-process code path, but plausible from hand-edited/corrupted state.json or a future producer bug — would skip the guard entirely via that unconditional fast path. The guard now runs first, unconditionally, for Type=="file.deleted"; every other event type (sync.* progress events) keeps the empty-path fast path unchanged. Adds regression coverage: TestACLProviderSyncDeleteEventHiddenFromDeniedAgent drives the real webhook-ingest HTTP endpoint through the async envelope worker (not a direct call to the unexported applyProviderDeleteLocked) for both an unrestricted and a file-level-protected file, checking HTTP history and WebSocket catch-up AND live delivery for a denied vs. unrestricted agent. TestACLEmptyPathUnsnapshottedFileDeletedFailsClosed round-trips a real on-disk snapshot with a hand-injected malformed empty-path event and confirms it fails closed over the internal check, HTTP, and WebSocket catch-up, while a normal sibling delete and a legitimate non-file empty-path event stay visible. Verified both regressions are real: reverted store.go/server.go to b0df4ad4 and confirmed both new tests fail, then restored. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01BPGQHoS7sMMyFxX3iWk2Le Session-Id: c9e3bbb9-166e-4a37-bccf-c5516372bbb5 --- internal/httpapi/server.go | 24 +- internal/httpapi/server_test.go | 494 ++++++++++++++++++++++++++++++++ internal/relayfile/store.go | 2 +- 3 files changed, 511 insertions(+), 9 deletions(-) diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index afb4353a..febd239e 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -2366,14 +2366,6 @@ func (s *Server) handleDeleteFile(w http.ResponseWriter, r *http.Request, worksp } func (s *Server) eventVisibleToClaims(workspaceID string, claims tokenClaims, event relayfile.Event) bool { - path := strings.TrimSpace(event.Path) - if path == "" { - return true - } - path = normalizeACLPath(path) - if !scopeMatchesPath(claims.Scopes, "fs:read", path) { - return false - } if event.Type == "file.deleted" && event.ACLPermissions == nil { // Fail closed: relayfile.snapshotACLPermissions guarantees every // delete event produced by ACL-snapshot-aware code carries a non-nil @@ -2384,6 +2376,22 @@ func (s *Server) eventVisibleToClaims(workspaceID string, claims tokenClaims, ev // is gone from ws.Files), so we cannot rule out a file-level deny // that would have hidden this path. Denying visibility is the only // choice that cannot leak a hidden file's prior existence/deletion. + // + // This check MUST run before the empty-path fast path below: a + // malformed or legacy file.deleted event can carry an empty Path + // (e.g. truncated/corrupted persisted data), and that fast path is + // an unconditional "visible to everyone" — routing an unsnapshotted + // delete through it would silently defeat this whole guard. Every + // other event type (sync.* progress events with Path "/" or "", + // etc.) is unaffected and keeps the fast path. + return false + } + path := strings.TrimSpace(event.Path) + if path == "" { + return true + } + path = normalizeACLPath(path) + if !scopeMatchesPath(claims.Scopes, "fs:read", path) { return false } if event.ACLPermissions != nil && !filePermissionAllows(event.ACLPermissions, workspaceID, &claims, "read", path) { diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index c348391e..8a14d074 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -724,6 +724,500 @@ func TestLegacyFileDeletedEventWithoutACLSnapshotFailsClosedAfterUpgrade(t *test } } +// waitForStoreCondition polls check until it reports true or timeout +// elapses. Used to await state produced by the async envelope worker (real +// production code — IngestEnvelope only enqueues, it does not process +// synchronously), which the test cannot otherwise be notified of. +func waitForStoreCondition(t *testing.T, timeout time.Duration, check func() bool) { + t.Helper() + deadline := time.Now().Add(timeout) + for { + if check() { + return + } + if time.Now().After(deadline) { + t.Fatalf("condition not met within %s", timeout) + } + time.Sleep(10 * time.Millisecond) + } +} + +type wsEventExpect struct{ typ, path string } + +// TestACLProviderSyncDeleteEventHiddenFromDeniedAgent is the regression +// test for finding P1 of the fresh review on commit b0df4ad4: +// applyProviderDeleteLocked — the provider-webhook delete path, distinct +// from DeleteFile and the draft-reconcile paths already covered by earlier +// ACL-423 tests — still assigned +// `event.ACLPermissions = append([]string(nil), aclPermissions...)` +// instead of routing through snapshotACLPermissions. For an UNRESTRICTED +// file that collapses right back to nil (append of zero elements onto a +// nil slice returns nil), which is indistinguishable from "never +// snapshotted" — so once eventVisibleToClaims started failing closed on +// Type=="file.deleted" && ACLPermissions==nil (the fix for finding (2) of +// the original review), this bug turned into an over-blocking regression: +// an ordinary, unrestricted provider-sync delete became invisible to EVERY +// agent, not just a denied one. +// +// This drives the real HTTP webhook-ingest endpoint end to end — through +// ParseGenericEnvelope and the async envelope worker, not a direct call to +// the unexported applyProviderDeleteLocked — for both an unrestricted file +// (must stay visible to everyone) and a file-level-protected one (must +// stay hidden from the denied agent only), and checks both HTTP history +// and WebSocket catch-up AND live delivery. +func TestACLProviderSyncDeleteEventHiddenFromDeniedAgent(t *testing.T) { + t.Parallel() + const workspaceID = "ws_acl_provider_sync_delete" + const provider = "acltestprovider" + const denyRule = "deny:agent:Limited" + const unrestrictedPath = "/acltestprovider/public.md" + const protectedPath = "/acltestprovider/protected.md" + + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{}) + t.Cleanup(store.Close) + ingestToken := mustTestJWT(t, "dev-secret", workspaceID, "Ingestor", []string{"fs:read", "fs:write", "sync:read", "sync:trigger"}, time.Now().Add(time.Hour)) + + deliverySeq := 0 + ingest := func(t *testing.T, eventType, path string, data map[string]any) { + t.Helper() + deliverySeq++ + deliveryID := fmt.Sprintf("dlv_acl_provider_delete_%d", deliverySeq) + body := map[string]any{ + "provider": provider, + "event_type": eventType, + "path": path, + "delivery_id": deliveryID, + "timestamp": time.Now().UTC().Format(time.RFC3339), + } + if data != nil { + body["data"] = data + } + resp := doRequest(t, NewServer(store), request{ + method: http.MethodPost, + path: "/v1/workspaces/" + workspaceID + "/webhooks/ingest", + headers: map[string]string{"Authorization": "Bearer " + ingestToken, "X-Correlation-Id": "corr_" + deliveryID}, + body: body, + }) + if resp.Code != http.StatusAccepted { + t.Fatalf("webhook ingest %s %s returned %d: %s", eventType, path, resp.Code, resp.Body.String()) + } + } + waitForFile := func(t *testing.T, path string) { + t.Helper() + waitForStoreCondition(t, 5*time.Second, func() bool { + _, err := store.ReadFile(workspaceID, path) + return err == nil + }) + } + waitForDelete := func(t *testing.T, path string) { + t.Helper() + waitForStoreCondition(t, 5*time.Second, func() bool { + feed, err := store.GetEvents(workspaceID, "", "", 1000) + if err != nil { + return false + } + for _, event := range feed.Events { + if event.Type == "file.deleted" && event.Path == path { + return true + } + } + return false + }) + } + protectedSemantics := map[string]any{"semantics": map[string]any{"permissions": []any{"allow:public", denyRule}}} + + // Catch-up target: create then delete both files via the real provider + // webhook path (applyProviderDeleteLocked), waiting for each async step + // so the create->delete pairs never coalesce into a single envelope. + ingest(t, "file.created", unrestrictedPath, map[string]any{"content": "public", "contentType": "text/plain"}) + waitForFile(t, unrestrictedPath) + ingest(t, "file.created", protectedPath, mergeMaps(map[string]any{"content": "secret", "contentType": "text/plain"}, protectedSemantics)) + waitForFile(t, protectedPath) + ingest(t, "file.deleted", unrestrictedPath, nil) + waitForDelete(t, unrestrictedPath) + ingest(t, "file.deleted", protectedPath, nil) + waitForDelete(t, protectedPath) + + // Pin the exact regression: the unrestricted delete's snapshot must be + // non-nil (empty), not nil — nil is what made it fail closed for + // everyone instead of just the denied agent. + feed, err := store.GetEvents(workspaceID, "", "", 1000) + if err != nil { + t.Fatalf("get events failed: %v", err) + } + for _, event := range feed.Events { + if event.Type != "file.deleted" { + continue + } + switch event.Path { + case unrestrictedPath: + if event.ACLPermissions == nil { + t.Fatalf("regression: applyProviderDeleteLocked left ACLPermissions nil for an unrestricted file (%+v) — this is exactly the P1 bug, and now hides the event from every agent, not just a denied one", event) + } + case protectedPath: + if len(event.ACLPermissions) == 0 { + t.Fatalf("expected the protected file's provider-sync delete to carry its deny rule, got %+v", event) + } + } + } + + server := httptest.NewServer(NewServer(store)) + t.Cleanup(server.Close) + limitedToken := mustTestJWT(t, "dev-secret", workspaceID, "Limited", []string{"fs:read"}, time.Now().Add(time.Hour)) + trustedToken := mustTestJWT(t, "dev-secret", workspaceID, "Trusted", []string{"fs:read"}, time.Now().Add(time.Hour)) + + // --- HTTP history --- + for _, tc := range []struct { + agent string + token string + wantUnrestrictedVisible bool + wantProtectedVisible bool + }{ + {"Limited", limitedToken, true, false}, + {"Trusted", trustedToken, true, true}, + } { + resp := doRequest(t, NewServer(store), request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/events?limit=1000", + headers: map[string]string{"Authorization": "Bearer " + tc.token, "X-Correlation-Id": "corr_http_" + tc.agent}, + }) + if resp.Code != http.StatusOK { + t.Fatalf("[%s] events returned %d: %s", tc.agent, resp.Code, resp.Body.String()) + } + var httpFeed relayfile.EventFeed + if err := json.NewDecoder(resp.Body).Decode(&httpFeed); err != nil { + t.Fatalf("[%s] decode events: %v", tc.agent, err) + } + var sawUnrestrictedDelete, sawProtectedDelete bool + for _, event := range httpFeed.Events { + if event.Type != "file.deleted" { + continue + } + if event.Path == unrestrictedPath { + sawUnrestrictedDelete = true + } + if event.Path == protectedPath { + sawProtectedDelete = true + } + } + if sawUnrestrictedDelete != tc.wantUnrestrictedVisible { + t.Fatalf("[%s] HTTP history: unrestricted provider delete visibility = %v, want %v", tc.agent, sawUnrestrictedDelete, tc.wantUnrestrictedVisible) + } + if sawProtectedDelete != tc.wantProtectedVisible { + t.Fatalf("[%s] HTTP history: protected provider delete visibility = %v, want %v", tc.agent, sawProtectedDelete, tc.wantProtectedVisible) + } + } + + // --- WebSocket catch-up --- + dial := func(t *testing.T, token string) *websocket.Conn { + t.Helper() + wsURL := "ws" + strings.TrimPrefix(server.URL, "http") + "/v1/workspaces/" + workspaceID + "/fs/ws?token=" + token + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + conn, _, err := websocket.Dial(ctx, wsURL, nil) + if err != nil { + t.Fatalf("websocket dial failed: %v", err) + } + return conn + } + readExact := func(t *testing.T, agent string, conn *websocket.Conn, phase string, want []wsEventExpect) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + for i, w := range want { + var msg map[string]any + if err := wsjson.Read(ctx, conn, &msg); err != nil { + t.Fatalf("[%s/%s] read event %d: %v", agent, phase, i, err) + } + if msg["type"] != w.typ || msg["path"] != w.path { + t.Fatalf("[%s/%s] event %d = %+v, want type=%s path=%s", agent, phase, i, msg, w.typ, w.path) + } + } + if err := wsjson.Write(ctx, conn, map[string]any{"type": "ping"}); err != nil { + t.Fatalf("[%s/%s] write ping failed: %v", agent, phase, err) + } + var pong map[string]any + if err := wsjson.Read(ctx, conn, &pong); err != nil || pong["type"] != "pong" { + t.Fatalf("[%s/%s] expected pong immediately after drain, got %+v (%v)", agent, phase, pong, err) + } + } + + limitedConn := dial(t, limitedToken) + defer limitedConn.Close(websocket.StatusNormalClosure, "") + readExact(t, "Limited", limitedConn, "catch-up", []wsEventExpect{ + {"file.created", unrestrictedPath}, + {"file.deleted", unrestrictedPath}, + }) + trustedConn := dial(t, trustedToken) + defer trustedConn.Close(websocket.StatusNormalClosure, "") + // Setup order was create(unrestricted), create(protected), + // delete(unrestricted), delete(protected) — all creates before either + // delete — so Trusted's unfiltered catch-up preserves that order. + readExact(t, "Trusted", trustedConn, "catch-up", []wsEventExpect{ + {"file.created", unrestrictedPath}, + {"file.created", protectedPath}, + {"file.deleted", unrestrictedPath}, + {"file.deleted", protectedPath}, + }) + + // --- WebSocket live delivery: both connections stay open, one more + // unrestricted and one more protected create+delete pair land while + // connected, and each socket must independently see exactly its own + // authorized subset. Ingests run from this single goroutine (no + // parallel subtests touching the shared store) so there is no + // cross-connection ordering race between the two already-open sockets. + const liveUnrestrictedPath = "/acltestprovider/public-live.md" + const liveProtectedPath = "/acltestprovider/protected-live.md" + ingest(t, "file.created", liveUnrestrictedPath, map[string]any{"content": "public-live", "contentType": "text/plain"}) + waitForFile(t, liveUnrestrictedPath) + ingest(t, "file.deleted", liveUnrestrictedPath, nil) + waitForDelete(t, liveUnrestrictedPath) + ingest(t, "file.created", liveProtectedPath, mergeMaps(map[string]any{"content": "protected-live", "contentType": "text/plain"}, protectedSemantics)) + waitForFile(t, liveProtectedPath) + ingest(t, "file.deleted", liveProtectedPath, nil) + waitForDelete(t, liveProtectedPath) + + readExact(t, "Limited", limitedConn, "live", []wsEventExpect{ + {"file.created", liveUnrestrictedPath}, + {"file.deleted", liveUnrestrictedPath}, + }) + readExact(t, "Trusted", trustedConn, "live", []wsEventExpect{ + {"file.created", liveUnrestrictedPath}, + {"file.deleted", liveUnrestrictedPath}, + {"file.created", liveProtectedPath}, + {"file.deleted", liveProtectedPath}, + }) +} + +func mergeMaps(base, extra map[string]any) map[string]any { + out := make(map[string]any, len(base)+len(extra)) + for k, v := range base { + out[k] = v + } + for k, v := range extra { + out[k] = v + } + return out +} + +// TestACLEmptyPathUnsnapshottedFileDeletedFailsClosed is the regression +// test for finding P2 of the fresh review on commit b0df4ad4: +// eventVisibleToClaims returned true for ANY event with an empty Path via +// its unconditional fast path, evaluated BEFORE the file.deleted +// nil-ACLPermissions fail-closed guard added for finding (2) of the +// original ACL-423 fix. A malformed or corrupted persisted file.deleted +// event carrying an empty Path — which no current in-process producer can +// create; DeleteFile, CommitForkWithValidator, applyProviderDeleteLocked, +// applyProviderUpsertLocked, reconcileAckedDraftLocked and +// removeDraftLocked all normalize a non-"/" path before emitting — would +// therefore slip past the fail-closed guard entirely and be treated as +// unconditionally visible, regardless of whether it ever had an ACL +// snapshot. +// +// This simulates that malformed state the only way it can actually arise +// — hand-edited or corrupted persisted JSON, e.g. from truncation or a +// future producer bug — by round-tripping a real on-disk snapshot and +// injecting one such event (with no aclPermissionsByEvent entry, so it +// reloads with ACLPermissions == nil like any other unsnapshotted delete), +// then checks it is hidden over the internal visibility check, the real +// HTTP events endpoint, and the real WebSocket catch-up feed — while a +// normal sibling delete event AND a legitimate non-file empty-path +// progress event (proving the fast path itself is preserved, not removed) +// both remain visible. +func TestACLEmptyPathUnsnapshottedFileDeletedFailsClosed(t *testing.T) { + t.Parallel() + const workspaceID = "ws_acl_empty_path_malformed" + const visiblePath = "/malformed/visible.md" + const malformedEventID = "evt_malformed_empty_path" + const nonFileEventID = "evt_nonfile_empty_path" + + statePath := filepath.Join(t.TempDir(), "state.json") + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{ + StateBackend: relayfile.NewJSONFileStateBackend(statePath), + DisableWorkers: true, + }) + seeded, err := store.WriteFile(relayfile.WriteRequest{WorkspaceID: workspaceID, Path: visiblePath, IfMatch: "0", Content: "visible"}) + if err != nil { + t.Fatalf("seed visible file failed: %v", err) + } + if _, err := store.DeleteFile(relayfile.DeleteRequest{WorkspaceID: workspaceID, Path: visiblePath, IfMatch: seeded.TargetRevision}); err != nil { + t.Fatalf("delete visible file failed: %v", err) + } + store.Close() + + raw, err := os.ReadFile(statePath) + if err != nil { + t.Fatalf("read state.json: %v", err) + } + var snapshot map[string]any + if err := json.Unmarshal(raw, &snapshot); err != nil { + t.Fatalf("unmarshal state.json: %v", err) + } + workspaces, _ := snapshot["workspaces"].(map[string]any) + wsSnapshot, _ := workspaces[workspaceID].(map[string]any) + events, _ := wsSnapshot["events"].([]any) + if len(events) == 0 { + t.Fatalf("expected seeded events in persisted snapshot, got %+v", wsSnapshot) + } + events = append(events, + // The malformed event under test: file.deleted, empty path, no + // recorded ACL snapshot. + map[string]any{ + "eventId": malformedEventID, + "type": "file.deleted", + "path": "", + "revision": "rev_malformed", + "origin": "system", + "correlationId": "corr_malformed", + "timestamp": time.Now().UTC().Format(time.RFC3339Nano), + }, + // A legitimate non-file, empty-path progress event: the fast path + // exists for exactly this shape and must keep working. + map[string]any{ + "eventId": nonFileEventID, + "type": "sync.suppressed", + "path": "", + "revision": "", + "origin": "provider_sync", + "correlationId": "corr_nonfile", + "timestamp": time.Now().UTC().Format(time.RFC3339Nano), + }, + ) + wsSnapshot["events"] = events + workspaces[workspaceID] = wsSnapshot + snapshot["workspaces"] = workspaces + patched, err := json.Marshal(snapshot) + if err != nil { + t.Fatalf("marshal patched state.json: %v", err) + } + if err := os.WriteFile(statePath, patched, 0o644); err != nil { + t.Fatalf("write patched state.json: %v", err) + } + + reloaded := relayfile.NewStoreWithOptions(relayfile.StoreOptions{ + StateBackend: relayfile.NewJSONFileStateBackend(statePath), + DisableWorkers: true, + }) + t.Cleanup(reloaded.Close) + server := NewServer(reloaded) + claims := tokenClaims{WorkspaceID: workspaceID, AgentName: "AnyAgent", Scopes: map[string]struct{}{"fs:read": {}}} + + reloadedFeed, err := reloaded.GetEvents(workspaceID, "", "", 100) + if err != nil { + t.Fatalf("get reloaded events failed: %v", err) + } + var checkedMalformed, checkedNonFile, checkedVisible bool + for _, event := range reloadedFeed.Events { + switch { + case event.EventID == malformedEventID: + checkedMalformed = true + if event.ACLPermissions != nil { + t.Fatalf("expected the malformed event to reload with ACLPermissions == nil, got %v", event.ACLPermissions) + } + if event.Path != "" { + t.Fatalf("expected the malformed event's empty path to survive reload, got %q", event.Path) + } + if server.eventVisibleToClaims(workspaceID, claims, event) { + t.Fatal("malformed empty-path unsnapshotted file.deleted event must fail closed, not fall through the empty-path fast path") + } + case event.EventID == nonFileEventID: + checkedNonFile = true + if !server.eventVisibleToClaims(workspaceID, claims, event) { + t.Fatal("a legitimate non-file empty-path event must still take the always-visible fast path — it must not have been collateral damage from the P2 fix") + } + case event.Type == "file.deleted" && event.Path == visiblePath: + checkedVisible = true + if !server.eventVisibleToClaims(workspaceID, claims, event) { + t.Fatal("normal sibling delete event (real ACL snapshot, empty permissions) must remain visible") + } + } + } + if !checkedMalformed || !checkedNonFile || !checkedVisible { + t.Fatalf("expected to find all three events after reload: malformed=%v nonfile=%v visible=%v (events=%+v)", checkedMalformed, checkedNonFile, checkedVisible, reloadedFeed.Events) + } + + // --- Real HTTP surface --- + httpServer := httptest.NewServer(server) + defer httpServer.Close() + token := mustTestJWT(t, "dev-secret", workspaceID, "AnyAgent", []string{"fs:read"}, time.Now().Add(time.Hour)) + resp := doRequest(t, server, request{ + method: http.MethodGet, + path: "/v1/workspaces/" + workspaceID + "/fs/events?limit=100", + headers: map[string]string{"Authorization": "Bearer " + token, "X-Correlation-Id": "corr_malformed_http"}, + }) + if resp.Code != http.StatusOK { + t.Fatalf("events returned %d: %s", resp.Code, resp.Body.String()) + } + var httpFeed relayfile.EventFeed + if err := json.NewDecoder(resp.Body).Decode(&httpFeed); err != nil { + t.Fatalf("decode events: %v", err) + } + var sawMalformed, sawNonFile, sawVisible bool + for _, event := range httpFeed.Events { + if event.EventID == malformedEventID { + sawMalformed = true + } + if event.EventID == nonFileEventID { + sawNonFile = true + } + if event.Type == "file.deleted" && event.Path == visiblePath { + sawVisible = true + } + } + if sawMalformed { + t.Fatal("malformed empty-path unsnapshotted delete leaked over HTTP") + } + if !sawNonFile { + t.Fatal("legitimate non-file empty-path event incorrectly hidden over HTTP") + } + if !sawVisible { + t.Fatal("normal sibling delete incorrectly hidden over HTTP") + } + + // --- Real WebSocket catch-up: collect until pong rather than asserting + // a fixed order/count, since the malformed and non-file events share + // the same empty Path (and fileEventMessage.Path is `omitempty`) — + // what matters is the malformed event never arrives and the other two + // do. + wsURL := "ws" + strings.TrimPrefix(httpServer.URL, "http") + "/v1/workspaces/" + workspaceID + "/fs/ws?token=" + token + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + conn, _, err := websocket.Dial(ctx, wsURL, nil) + if err != nil { + t.Fatalf("websocket dial failed: %v", err) + } + defer conn.Close(websocket.StatusNormalClosure, "") + if err := wsjson.Write(ctx, conn, map[string]any{"type": "ping"}); err != nil { + t.Fatalf("write ping failed: %v", err) + } + var sawWSVisibleDelete, sawWSNonFile bool + for { + var msg map[string]any + if err := wsjson.Read(ctx, conn, &msg); err != nil { + t.Fatalf("websocket read failed: %v", err) + } + if msg["type"] == "pong" { + break + } + if msg["type"] == "file.deleted" { + if msg["path"] == visiblePath { + sawWSVisibleDelete = true + } else { + t.Fatalf("unexpected file.deleted over websocket catch-up: %+v (malformed empty-path event must never be delivered)", msg) + } + } + if msg["type"] == "sync.suppressed" { + sawWSNonFile = true + } + } + if !sawWSVisibleDelete { + t.Fatal("normal sibling delete incorrectly hidden over websocket catch-up") + } + if !sawWSNonFile { + t.Fatal("legitimate non-file empty-path event incorrectly hidden over websocket catch-up") + } +} + func TestFileEventsWebSocketCursorCatchUpDrainsMoreThanOnePage(t *testing.T) { store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) t.Cleanup(store.Close) diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index cb98ffdc..1439b5a1 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -4488,7 +4488,7 @@ func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, a CorrelationID: correlationID, Timestamp: now, } - event.ACLPermissions = append([]string(nil), aclPermissions...) + event.ACLPermissions = snapshotACLPermissions(aclPermissions) s.appendWorkspaceEventLocked(workspaceID, ws, event) } From d99d41ec5473cb5e054c57298d05ba38be388016 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 17:47:36 +0200 Subject: [PATCH 08/12] fix: reconcile pathless provider deletes safely --- .../completed/2026-09/traj_qtxkeaa5cphq.json | 80 +++++++++++++ .../completed/2026-09/traj_qtxkeaa5cphq.md | 33 ++++++ .trajectories/index.json | 9 +- docs/bulk-export-design.md | 2 +- internal/httpapi/server.go | 4 +- internal/httpapi/server_test.go | 28 +++++ internal/httpapi/websocket.go | 6 + internal/mountsync/syncer.go | 41 ++++++- internal/mountsync/syncer_test.go | 109 ++++++++++++++++++ internal/relayfile/adapters.go | 10 +- internal/relayfile/adapters_test.go | 34 ++++++ internal/relayfile/store.go | 33 ++++++ internal/relayfile/store_test.go | 43 +++++++ openapi/relayfile-v1.openapi.yaml | 2 + packages/core/src/webhooks.ts | 1 + .../file-observer/src/lib/relayfile-client.ts | 9 +- packages/sdk/typescript/src/sync.test.ts | 37 ++++++ packages/sdk/typescript/src/sync.ts | 2 +- packages/sdk/typescript/src/types.ts | 1 + 19 files changed, 468 insertions(+), 16 deletions(-) create mode 100644 .trajectories/completed/2026-09/traj_qtxkeaa5cphq.json create mode 100644 .trajectories/completed/2026-09/traj_qtxkeaa5cphq.md diff --git a/.trajectories/completed/2026-09/traj_qtxkeaa5cphq.json b/.trajectories/completed/2026-09/traj_qtxkeaa5cphq.json new file mode 100644 index 00000000..6d22c839 --- /dev/null +++ b/.trajectories/completed/2026-09/traj_qtxkeaa5cphq.json @@ -0,0 +1,80 @@ +{ + "id": "traj_qtxkeaa5cphq", + "version": 1, + "task": { + "title": "Repair ACL pathless provider deletion reconciliation", + "source": { + "system": "plain", + "id": "Relayfile ACL-423" + } + }, + "status": "completed", + "startedAt": "2026-09-08T15:41:42.556Z", + "completedAt": "2026-09-08T15:47:16.049Z", + "agents": [ + { + "name": "default", + "role": "lead", + "joinedAt": "2026-09-08T15:41:47.673Z" + } + ], + "chapters": [ + { + "id": "chap_bevz9521bn7k", + "title": "Work", + "agentName": "default", + "startedAt": "2026-09-08T15:41:47.673Z", + "endedAt": "2026-09-08T15:47:16.049Z", + "events": [ + { + "ts": 1788882107674, + "type": "decision", + "content": "Use a pathless sync.reconcile control event for unresolved provider deletes: Use a pathless sync.reconcile control event for unresolved provider deletes", + "raw": { + "question": "Use a pathless sync.reconcile control event for unresolved provider deletes", + "chosen": "Use a pathless sync.reconcile control event for unresolved provider deletes", + "alternatives": [], + "reasoning": "Never guess or disclose hidden paths; filtered mounts receive an authoritative full reconciliation in the same sync cycle while malformed empty-path file.deleted events remain fail-closed." + }, + "significance": "high" + }, + { + "ts": 1788882435983, + "type": "reflection", + "content": "Pathless provider deletion now emits only a durable sync.reconcile control when no ACL-backed path can be named; mounts persist the prompt, reconcile authoritatively in-cycle, and filtered consumers receive no path. Focused/full serialized Go and package gates are green; parallel full Go and mountsync race expose inherited test-environment races.", + "raw": { + "focalPoints": [ + "pathless deletion", + "ACL fail-closed", + "restart durability", + "inherited races" + ], + "adjustments": "Added durable PendingFullReconcile state and consumer path-filter exceptions", + "confidence": 0.92 + }, + "significance": "high", + "tags": [ + "focal:pathless deletion", + "focal:ACL fail-closed", + "focal:restart durability", + "focal:inherited races", + "confidence:0.92" + ] + } + ] + } + ], + "retrospective": { + "summary": "Repaired Relayfile ACL pathless provider deletion handling with sync.reconcile control events, durable prompt reconciliation, no-path fail-closed filtering, SDK/file-observer parity, and regression tests.", + "approach": "Standard approach", + "confidence": 0.92 + }, + "commits": [], + "filesChanged": [], + "projectId": "/private/tmp/relayfile-acl423-fix-0908", + "tags": [], + "_trace": { + "startRef": "45820e7a50002a19245f2c5984df996bd650d455", + "endRef": "45820e7a50002a19245f2c5984df996bd650d455" + } +} \ No newline at end of file diff --git a/.trajectories/completed/2026-09/traj_qtxkeaa5cphq.md b/.trajectories/completed/2026-09/traj_qtxkeaa5cphq.md new file mode 100644 index 00000000..9988b031 --- /dev/null +++ b/.trajectories/completed/2026-09/traj_qtxkeaa5cphq.md @@ -0,0 +1,33 @@ +# Trajectory: Repair ACL pathless provider deletion reconciliation + +> **Status:** ✅ Completed +> **Task:** Relayfile ACL-423 +> **Confidence:** 92% +> **Started:** September 8, 2026 at 05:41 PM +> **Completed:** September 8, 2026 at 05:47 PM + +--- + +## Summary + +Repaired Relayfile ACL pathless provider deletion handling with sync.reconcile control events, durable prompt reconciliation, no-path fail-closed filtering, SDK/file-observer parity, and regression tests. + +**Approach:** Standard approach + +--- + +## Key Decisions + +### Use a pathless sync.reconcile control event for unresolved provider deletes +- **Chose:** Use a pathless sync.reconcile control event for unresolved provider deletes +- **Reasoning:** Never guess or disclose hidden paths; filtered mounts receive an authoritative full reconciliation in the same sync cycle while malformed empty-path file.deleted events remain fail-closed. + +--- + +## Chapters + +### 1. Work +*Agent: default* + +- Use a pathless sync.reconcile control event for unresolved provider deletes: Use a pathless sync.reconcile control event for unresolved provider deletes +- Pathless provider deletion now emits only a durable sync.reconcile control when no ACL-backed path can be named; mounts persist the prompt, reconcile authoritatively in-cycle, and filtered consumers receive no path. Focused/full serialized Go and package gates are green; parallel full Go and mountsync race expose inherited test-environment races. diff --git a/.trajectories/index.json b/.trajectories/index.json index f0f590c9..84dd7c06 100644 --- a/.trajectories/index.json +++ b/.trajectories/index.json @@ -1,6 +1,6 @@ { "version": 1, - "lastUpdated": "2026-08-27T20:20:27.504Z", + "lastUpdated": "2026-09-08T16:47:53.839Z", "trajectories": { "traj_4pvrlmqfnzng": { "title": "Review PR #278 in AgentWorkforce/relayfile", @@ -204,6 +204,13 @@ "startedAt": "2026-08-27T20:19:59.085Z", "completedAt": "2026-08-27T20:20:27.355Z", "path": ".trajectories/completed/2026-08/traj_7n063n1f3wai.json" + }, + "traj_qtxkeaa5cphq": { + "title": "Repair ACL pathless provider deletion reconciliation", + "status": "completed", + "startedAt": "2026-09-08T15:41:42.556Z", + "completedAt": "2026-09-08T15:47:16.049Z", + "path": ".trajectories/completed/2026-09/traj_qtxkeaa5cphq.json" } } } diff --git a/docs/bulk-export-design.md b/docs/bulk-export-design.md index beb80fd6..3dbf3804 100644 --- a/docs/bulk-export-design.md +++ b/docs/bulk-export-design.md @@ -336,7 +336,7 @@ To prevent missing events between subscribe and the first live event: } ``` -Event types: `file.created`, `file.updated`, `file.deleted`, `dir.created`, `dir.deleted`, `sync.error`, `sync.ignored`, `sync.suppressed`, `sync.stale`, `writeback.failed`, `writeback.succeeded`. +Event types: `file.created`, `file.updated`, `file.deleted`, `dir.created`, `dir.deleted`, `sync.error`, `sync.ignored`, `sync.suppressed`, `sync.stale`, `sync.reconcile`, `writeback.failed`, `writeback.succeeded`. `sync.reconcile` is a pathless provider-sync control event; mounts must perform an authoritative reconciliation and must not infer a path from it. **Pong (response to client ping):** diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index febd239e..e5506f02 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -2366,7 +2366,7 @@ func (s *Server) handleDeleteFile(w http.ResponseWriter, r *http.Request, worksp } func (s *Server) eventVisibleToClaims(workspaceID string, claims tokenClaims, event relayfile.Event) bool { - if event.Type == "file.deleted" && event.ACLPermissions == nil { + if event.Type == "file.deleted" && (event.ACLPermissions == nil || strings.TrimSpace(event.Path) == "") { // Fail closed: relayfile.snapshotACLPermissions guarantees every // delete event produced by ACL-snapshot-aware code carries a non-nil // ACLPermissions slice (empty when no rule applied). A nil slice here @@ -2380,7 +2380,7 @@ func (s *Server) eventVisibleToClaims(workspaceID string, claims tokenClaims, ev // This check MUST run before the empty-path fast path below: a // malformed or legacy file.deleted event can carry an empty Path // (e.g. truncated/corrupted persisted data), and that fast path is - // an unconditional "visible to everyone" — routing an unsnapshotted + // an unconditional "visible to everyone" — routing any pathless // delete through it would silently defeat this whole guard. Every // other event type (sync.* progress events with Path "/" or "", // etc.) is unaffected and keeps the fast path. diff --git a/internal/httpapi/server_test.go b/internal/httpapi/server_test.go index 8a14d074..da8ada57 100644 --- a/internal/httpapi/server_test.go +++ b/internal/httpapi/server_test.go @@ -1000,6 +1000,34 @@ func mergeMaps(base, extra map[string]any) map[string]any { return out } +func TestACLPathlessReconcileControlDoesNotLeakOrLookLikeDelete(t *testing.T) { + store := relayfile.NewStoreWithOptions(relayfile.StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + server := NewServer(store) + claims := tokenClaims{Scopes: map[string]struct{}{"fs:read": {}}} + + if server.eventVisibleToClaims("ws_acl_pathless_control", claims, relayfile.Event{ + Type: "file.deleted", + ACLPermissions: []string{}, + }) { + t.Fatal("pathless file.deleted with a present ACL snapshot must remain hidden") + } + reconcile := relayfile.Event{Type: "sync.reconcile", Origin: "provider_sync"} + if !server.eventVisibleToClaims("ws_acl_pathless_control", claims, reconcile) { + t.Fatal("pathless reconciliation control event should be visible without a file path") + } + if !webSocketEventMatchesPaths(reconcile, []string{"/secret/**"}) { + t.Fatal("pathless reconciliation control event must reach path-filtered mounts") + } + payload, err := json.Marshal(fileEventMessage{Type: reconcile.Type, Origin: reconcile.Origin}) + if err != nil { + t.Fatalf("marshal pathless control event: %v", err) + } + if strings.Contains(string(payload), `"path"`) { + t.Fatalf("pathless reconciliation control event disclosed a path field: %s", payload) + } +} + // TestACLEmptyPathUnsnapshottedFileDeletedFailsClosed is the regression // test for finding P2 of the fresh review on commit b0df4ad4: // eventVisibleToClaims returned true for ANY event with an empty Path via diff --git a/internal/httpapi/websocket.go b/internal/httpapi/websocket.go index 369e2bbe..41926b1f 100644 --- a/internal/httpapi/websocket.go +++ b/internal/httpapi/websocket.go @@ -211,6 +211,12 @@ func normalizeWebSocketPathFilters(values []string) []string { } func webSocketEventMatchesPaths(event relayfile.Event, filters []string) bool { + if event.Type == "sync.reconcile" { + // Pathless reconciliation is a control signal, not a file event. It + // must reach path-filtered mounts so they can refresh their local + // view, while it discloses no path to the subscriber. + return true + } if len(filters) == 0 { return true } diff --git a/internal/mountsync/syncer.go b/internal/mountsync/syncer.go index 06d84f4d..8bf7e8fd 100644 --- a/internal/mountsync/syncer.go +++ b/internal/mountsync/syncer.go @@ -1820,11 +1820,16 @@ func (s *Syncer) readLocalSnapshot(path string, includeContent bool) (localSnaps } type mountState struct { - WorkspaceID string `json:"workspaceId,omitempty"` - RemoteRoot string `json:"remoteRoot,omitempty"` - LocalRoot string `json:"localRoot,omitempty"` - Files map[string]trackedFile `json:"files"` - EventsCursor string `json:"eventsCursor,omitempty"` + WorkspaceID string `json:"workspaceId,omitempty"` + RemoteRoot string `json:"remoteRoot,omitempty"` + LocalRoot string `json:"localRoot,omitempty"` + Files map[string]trackedFile `json:"files"` + EventsCursor string `json:"eventsCursor,omitempty"` + // PendingFullReconcile is durable recovery state for pathless control + // events. The cursor may advance before the authoritative pull runs; keep + // the request across a process restart so acknowledging the control event + // cannot strand revoked content until the periodic audit. + PendingFullReconcile bool `json:"pendingFullReconcile,omitempty"` IncrementalCheckpoint *incrementalCheckpoint `json:"incrementalCheckpoint,omitempty"` IncrementalBacklogDraining bool `json:"incrementalBacklogDraining,omitempty"` LastReconcileAt string `json:"lastReconcileAt,omitempty"` @@ -5379,6 +5384,16 @@ func (s *Syncer) applyWebSocketEventWithPersistence(ctx context.Context, event w switch eventType := strings.TrimSpace(event.Type); eventType { case "", "pong": return nil + case "sync.reconcile": + // The server intentionally omits paths from this control event. It + // requests a full authoritative pull without disclosing or guessing + // which hidden file changed. + s.mu.Lock() + s.forceFullReconcile = true + s.markWebSocketEventAppliedLocked(event, eventAt) + err := s.persistWebSocketStateLocked(persist) + s.mu.Unlock() + return err case "file.created", "file.updated": remotePath := normalizeRemotePath(event.Path) if remotePath == "/" || !isUnderRemoteRoot(s.remoteRoot, remotePath) { @@ -6026,6 +6041,13 @@ func (s *Syncer) pullRemote(ctx context.Context, conflicted map[string]struct{}) nextCursor, err := s.pullRemoteIncremental(ctx, conflicted, s.state.EventsCursor) if err == nil { s.state.EventsCursor = advanceEventCursor(s.state.EventsCursor, nextCursor) + if s.forceFullReconcile { + // A pathless reconciliation control event is intentionally + // handled without naming any hidden path. Run the authoritative + // pull in this same cycle so a revoked/deleted local copy is not + // left until the periodic audit. + return s.pullRemote(ctx, conflicted) + } return nil } if strings.TrimSpace(nextCursor) != "" && nextCursor != s.state.EventsCursor { @@ -7777,6 +7799,7 @@ func (s *Syncer) markBootstrapComplete() { // Clear persisted quarantine so a fixed adapter gets a clean slate. s.state.QuarantinedPaths = nil s.clearAllIncrementalReadNotReady() + s.state.PendingFullReconcile = false // One-shot escape hatch / clobber-remnant recovery: after a single // successful full reconcile, clear the in-memory force flag so // subsequent cycles can use the fast-path again. @@ -8414,6 +8437,10 @@ func (s *Syncer) pullRemoteIncremental(ctx context.Context, conflicted map[strin if ts := strings.TrimSpace(event.Timestamp); ts != "" { pageLastEventAt = ts } + if event.Type == "sync.reconcile" { + s.forceFullReconcile = true + continue + } remotePath := normalizeRemotePath(event.Path) if remotePath == "/" || !isUnderRemoteRoot(s.remoteRoot, remotePath) { continue @@ -10105,6 +10132,9 @@ func (s *Syncer) loadState() error { s.state.WorkspaceID = s.workspace s.state.RemoteRoot = s.remoteRoot s.state.LocalRoot = s.localRoot + if s.state.PendingFullReconcile { + s.forceFullReconcile = true + } if err := s.enforceSyncModePermissionsOnTransition(); err != nil { // loadState marks the state loaded optimistically. Re-arm it here so a // transient chmod/stat failure cannot let the next cycle bypass an @@ -10151,6 +10181,7 @@ func (s *Syncer) currentSyncMode() string { func (s *Syncer) savePrivateState() error { s.state.SyncMode = s.currentSyncMode() + s.state.PendingFullReconcile = s.forceFullReconcile data, err := json.Marshal(s.state) if err != nil { return err diff --git a/internal/mountsync/syncer_test.go b/internal/mountsync/syncer_test.go index 20b44b71..c79f3f14 100644 --- a/internal/mountsync/syncer_test.go +++ b/internal/mountsync/syncer_test.go @@ -11816,6 +11816,90 @@ func TestApplyWebSocketEventClearsReadNotReadyMarker(t *testing.T) { assertLocalFileContent(t, filepath.Join(localDir, "Docs", "ws.md"), "# websocket") } +func TestPathlessReconcileEventRequestsPromptAuthoritativePull(t *testing.T) { + const remotePath = "/notion/Docs/revoked.md" + const allowedPath = "/notion/Docs/allowed.md" + localDir := t.TempDir() + localPath := filepath.Join(localDir, "Docs", "revoked.md") + if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { + t.Fatalf("mkdir local doc dir: %v", err) + } + if err := os.WriteFile(localPath, []byte("revoked content"), 0o644); err != nil { + t.Fatalf("write stale local doc: %v", err) + } + client := &fakeClient{files: map[string]RemoteFile{ + allowedPath: { + Path: allowedPath, + Revision: "rev_allowed", + ContentType: "text/markdown", + Content: "allowed content", + }, + }} + syncer, err := NewSyncer(client, SyncerOptions{ + WorkspaceID: "ws_pathless_reconcile", + RemoteRoot: "/notion", + LocalRoot: localDir, + }) + if err != nil { + t.Fatalf("new syncer failed: %v", err) + } + syncer.state.BootstrapComplete = true + syncer.state.EventsCursor = "evt_before_reconcile" + syncer.state.Files[remotePath] = trackedFile{Revision: "rev_secret", Hash: hashString("revoked content")} + syncer.state.Files[allowedPath] = trackedFile{Revision: "rev_allowed", Hash: hashString("allowed content")} + + if err := syncer.applyWebSocketEvent(context.Background(), websocketEvent{ + EventID: "evt_pathless_reconcile", + Type: "sync.reconcile", + }); err != nil { + t.Fatalf("apply pathless reconcile event: %v", err) + } + if !syncer.forceFullReconcile { + t.Fatal("pathless reconcile event did not arm an authoritative pull") + } + restarted, err := NewSyncer(client, SyncerOptions{ + WorkspaceID: "ws_pathless_reconcile", + RemoteRoot: "/notion", + LocalRoot: localDir, + StateFile: syncer.stateFile, + }) + if err != nil { + t.Fatalf("new restarted syncer failed: %v", err) + } + if err := restarted.loadState(); err != nil { + t.Fatalf("load restarted syncer state: %v", err) + } + if !restarted.forceFullReconcile { + t.Fatal("pathless reconcile request was not durable across restart") + } + if _, err := os.Stat(localPath); err != nil { + t.Fatalf("pathless control event should not guess/delete a local path before reconciliation: %v", err) + } + + if err := syncer.pullRemote(context.Background(), nil); err != nil { + t.Fatalf("prompt authoritative pull failed: %v", err) + } + if syncer.forceFullReconcile { + t.Fatal("authoritative pull did not clear the reconciliation request") + } + if _, err := os.Stat(localPath); err != nil { + t.Fatalf("first authoritative observation should retain content pending delete confirmation: %v", err) + } + client.files[allowedPath] = RemoteFile{ + Path: allowedPath, + Revision: "rev_allowed_2", + ContentType: "text/markdown", + Content: "allowed content", + } + syncer.forceFullReconcile = true + if err := syncer.pullRemote(context.Background(), nil); err != nil { + t.Fatalf("confirmed authoritative pull failed: %v", err) + } + if _, err := os.Stat(localPath); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("authoritative reconciliation did not remove revoked content, stat err=%v", err) + } +} + func TestApplyRemoteSnapshotDeletesRevClearsReadNotReadyMarkerAfterConfirmedDelete(t *testing.T) { const remotePath = "/notion/Docs/deleted.md" localDir := t.TempDir() @@ -12029,6 +12113,31 @@ func TestPullRemoteIncrementalDeleteEventStillDeletes(t *testing.T) { } } +func TestPullRemoteIncrementalPathlessReconcileArmsFullPull(t *testing.T) { + client := &fakeClient{events: []FilesystemEvent{ + {EventID: "evt_reconcile", Type: "sync.reconcile"}, + }} + syncer, err := NewSyncer(client, SyncerOptions{ + WorkspaceID: "ws_incremental_reconcile", + RemoteRoot: "/notion", + LocalRoot: t.TempDir(), + }) + if err != nil { + t.Fatalf("new syncer failed: %v", err) + } + + cursor, err := syncer.pullRemoteIncremental(context.Background(), nil, "evt_before") + if err != nil { + t.Fatalf("pullRemoteIncremental failed: %v", err) + } + if cursor != "evt_reconcile" { + t.Fatalf("cursor = %q, want evt_reconcile", cursor) + } + if !syncer.forceFullReconcile { + t.Fatal("pathless reconciliation event did not request an authoritative pull") + } +} + func TestScanLocalFilesLogsOversizedFileOncePerSize(t *testing.T) { t.Setenv("RELAYFILE_MAX_WRITEBACK_BYTES", "4") diff --git a/internal/relayfile/adapters.go b/internal/relayfile/adapters.go index 52dfea1d..ee98b28d 100644 --- a/internal/relayfile/adapters.go +++ b/internal/relayfile/adapters.go @@ -63,7 +63,9 @@ func ParseGenericEnvelope(req WebhookEnvelopeRequest) ([]ApplyAction, error) { switch eventType { case "file.created", "file.updated", "file.deleted": if providerObjectID != "" { - path = "/" + // Preserve the absent path. The store can resolve an object ID + // through its provider index; an unresolved delete emits a + // pathless reconciliation control event instead of file.deleted. break } return []ApplyAction{{Type: ActionIgnored}}, nil @@ -71,7 +73,11 @@ func ParseGenericEnvelope(req WebhookEnvelopeRequest) ([]ApplyAction, error) { return []ApplyAction{{Type: ActionIgnored}}, nil } } - canonicalPath, pathAllowed := canonicalProviderEnvelopePath(req.Provider, path) + canonicalPath := "" + pathAllowed := true + if strings.TrimSpace(path) != "" { + canonicalPath, pathAllowed = canonicalProviderEnvelopePath(req.Provider, path) + } if !pathAllowed { return []ApplyAction{{Type: ActionIgnored}}, nil } diff --git a/internal/relayfile/adapters_test.go b/internal/relayfile/adapters_test.go index d5fcbff0..ca8363c9 100644 --- a/internal/relayfile/adapters_test.go +++ b/internal/relayfile/adapters_test.go @@ -70,6 +70,40 @@ func TestParseGenericEnvelopeFileDeleted(t *testing.T) { } } +func TestParseGenericEnvelopeFileDeletedWithoutPathPreservesMissingPath(t *testing.T) { + actions, err := ParseGenericEnvelope(WebhookEnvelopeRequest{ + Provider: "salesforce", + Payload: map[string]any{ + "event_type": "file.deleted", + "providerObjectId": "sf_acc_pathless", + }, + }) + if err != nil { + t.Fatalf("parse envelope failed: %v", err) + } + if len(actions) != 1 || actions[0].Type != ActionFileDelete { + t.Fatalf("expected one pathless delete action, got %+v", actions) + } + if actions[0].Path != "" { + t.Fatalf("pathless delete was normalized into a guessed path %q", actions[0].Path) + } +} + +func TestParseGenericEnvelopeMalformedPathlessDeleteWithoutIdentityIsIgnored(t *testing.T) { + actions, err := ParseGenericEnvelope(WebhookEnvelopeRequest{ + Provider: "salesforce", + Payload: map[string]any{ + "event_type": "file.deleted", + }, + }) + if err != nil { + t.Fatalf("parse malformed envelope failed: %v", err) + } + if len(actions) != 1 || actions[0].Type != ActionIgnored { + t.Fatalf("malformed pathless delete was not ignored: %+v", actions) + } +} + func TestParseGenericEnvelopeFileCreated(t *testing.T) { actions, err := ParseGenericEnvelope(WebhookEnvelopeRequest{ Provider: "custom", diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index 1439b5a1..f606cf3f 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -4466,9 +4466,36 @@ func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, a } } if path == "/" { + if strings.TrimSpace(action.Path) == "" { + // A provider delete without a resolvable path cannot safely name a + // local file. Emit only a pathless control event so mounts perform + // an authoritative reconciliation; never emit file.deleted with an + // empty path, which could be confused with malformed persisted data. + s.appendWorkspaceEventLocked(workspaceID, ws, Event{ + EventID: s.nextEventIDLocked(), + Type: "sync.reconcile", + Origin: "provider_sync", + Provider: provider, + CorrelationID: correlationID, + Timestamp: now, + }) + } return } if _, ok := ws.Files[path]; !ok { + if strings.TrimSpace(action.Path) == "" { + // A stale provider index is no safer than an unknown object: the + // server cannot produce a path-backed ACL snapshot for this delete. + // Ask mounts to reconcile without exposing the indexed path. + s.appendWorkspaceEventLocked(workspaceID, ws, Event{ + EventID: s.nextEventIDLocked(), + Type: "sync.reconcile", + Origin: "provider_sync", + Provider: provider, + CorrelationID: correlationID, + Timestamp: now, + }) + } return } aclPermissions := resolvePermissionsFromFiles(ws.Files, path, true) @@ -4498,6 +4525,12 @@ func canonicalizeProviderActionLocked(ws *workspaceState, provider string, actio default: return action } + if strings.TrimSpace(action.Path) == "" { + // Preserve object-identity actions without a path. Upserts can fall + // back to their provider-object projection; deletes can request a + // safe workspace reconciliation when identity cannot resolve locally. + return action + } canonicalPath, ok := canonicalProviderEnvelopePath(provider, action.Path) if !ok { action.Type = ActionIgnored diff --git a/internal/relayfile/store_test.go b/internal/relayfile/store_test.go index 929f276b..014bd83c 100644 --- a/internal/relayfile/store_test.go +++ b/internal/relayfile/store_test.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "path/filepath" + "strings" "sync/atomic" "testing" "time" @@ -1706,6 +1707,48 @@ func TestProviderUpsertWithoutPathUsesObjectIdentity(t *testing.T) { t.Fatalf("expected object-identity upsert without path to update existing projected file") } +func TestProviderDeleteWithoutPathEmitsReconcileControlEvent(t *testing.T) { + store := NewStore() + t.Cleanup(store.Close) + const workspaceID = "ws_provider_pathless_delete" + _, err := store.IngestEnvelope(WebhookEnvelopeRequest{ + EnvelopeID: "env_provider_pathless_delete", + WorkspaceID: workspaceID, + Provider: "external", + DeliveryID: "delivery_provider_pathless_delete", + ReceivedAt: time.Now().UTC().Format(time.RFC3339Nano), + Payload: map[string]any{ + "event_type": "file.deleted", + "providerObjectId": "object_missing_from_local_index", + }, + CorrelationID: "corr_provider_pathless_delete", + }) + if err != nil { + t.Fatalf("pathless delete ingest failed: %v", err) + } + + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + feed, feedErr := store.GetEvents(workspaceID, "", "", 100) + if feedErr == nil { + for _, event := range feed.Events { + if event.Type != "sync.reconcile" { + if event.Type == "file.deleted" && strings.TrimSpace(event.Path) == "" { + t.Fatalf("pathless provider deletion emitted malformed file.deleted event: %+v", event) + } + continue + } + if event.Path != "" || event.Origin != "provider_sync" || event.Provider != "external" { + t.Fatalf("unexpected pathless reconcile event: %+v", event) + } + return + } + } + time.Sleep(10 * time.Millisecond) + } + t.Fatal("timed out waiting for pathless provider deletion reconciliation event") +} + func TestPendingWritebacksRecoveredOnRestart(t *testing.T) { stateFile := filepath.Join(t.TempDir(), "relayfile-state.json") diff --git a/openapi/relayfile-v1.openapi.yaml b/openapi/relayfile-v1.openapi.yaml index b29ecbf7..6f45c4ef 100644 --- a/openapi/relayfile-v1.openapi.yaml +++ b/openapi/relayfile-v1.openapi.yaml @@ -3166,10 +3166,12 @@ components: - sync.ignored - sync.suppressed - sync.stale + - sync.reconcile - writeback.failed - writeback.succeeded path: type: string + description: Empty for pathless control events such as sync.reconcile; clients must not infer a file path. revision: type: string contentHash: diff --git a/packages/core/src/webhooks.ts b/packages/core/src/webhooks.ts index ef384a3e..18999c33 100644 --- a/packages/core/src/webhooks.ts +++ b/packages/core/src/webhooks.ts @@ -515,6 +515,7 @@ const VALID_EVENT_TYPES = new Set([ "sync.ignored", "sync.suppressed", "sync.stale", + "sync.reconcile", "writeback.failed", "writeback.succeeded", ]); diff --git a/packages/file-observer/src/lib/relayfile-client.ts b/packages/file-observer/src/lib/relayfile-client.ts index f8a7e25f..d4fdf34c 100644 --- a/packages/file-observer/src/lib/relayfile-client.ts +++ b/packages/file-observer/src/lib/relayfile-client.ts @@ -70,6 +70,7 @@ export type FilesystemEventType = | 'sync.ignored' | 'sync.suppressed' | 'sync.stale' + | 'sync.reconcile' | 'writeback.failed' | 'writeback.succeeded'; @@ -275,10 +276,10 @@ class DefaultRelayfileWebSocketConnection implements RelayfileWebSocketConnectio if (!raw || typeof raw !== 'object' || typeof raw.type !== 'string') { throw new Error("Invalid Relayfile WebSocket event: missing required 'type' field."); } - if (typeof raw.path !== 'string') { + if (typeof raw.path !== 'string' && raw.type !== 'sync.reconcile') { throw new Error("Invalid Relayfile WebSocket event: missing required 'path' field."); } - if (typeof raw.revision !== 'string') { + if (typeof raw.revision !== 'string' && raw.type !== 'sync.reconcile') { throw new Error("Invalid Relayfile WebSocket event: missing required 'revision' field."); } if (typeof raw.timestamp !== 'string') { @@ -288,8 +289,8 @@ class DefaultRelayfileWebSocketConnection implements RelayfileWebSocketConnectio parsed = { eventId: typeof raw.eventId === 'string' ? raw.eventId : '', type: raw.type as FilesystemEventType, - path: raw.path, - revision: raw.revision, + path: typeof raw.path === 'string' ? raw.path : '', + revision: typeof raw.revision === 'string' ? raw.revision : '', origin: raw.origin, provider: raw.provider, correlationId: raw.correlationId, diff --git a/packages/sdk/typescript/src/sync.test.ts b/packages/sdk/typescript/src/sync.test.ts index 2172baa4..762f1e30 100644 --- a/packages/sdk/typescript/src/sync.test.ts +++ b/packages/sdk/typescript/src/sync.test.ts @@ -103,6 +103,43 @@ describe("RelayFileSync", () => { await sync.stop(); }); + it("delivers pathless reconciliation controls through path filters", async () => { + const sockets: MockWebSocket[] = []; + const sync = new RelayFileSync({ + client: makeClient(), + workspaceId: "ws_acme", + baseUrl: "https://relay.test", + token: "ws_token", + paths: ["/private/**"], + webSocketFactory: (url) => { + const socket = new MockWebSocket(url); + sockets.push(socket); + return socket; + } + }); + const events: FilesystemEvent[] = []; + sync.on("event", (event) => events.push(event)); + + sync.start(); + sockets[0]!.emit("open", {}); + sockets[0]!.emit("message", { + data: JSON.stringify({ + eventId: "evt_reconcile", + type: "sync.reconcile", + timestamp: "2026-03-26T00:00:00Z" + }) + }); + + expect(events).toEqual([{ + eventId: "evt_reconcile", + type: "sync.reconcile", + path: "", + revision: "", + timestamp: "2026-03-26T00:00:00Z" + }]); + await sync.stop(); + }); + it("normalizes malformed filesystem events with stable fallbacks", () => { const first = normalizeFilesystemEvent(null); const second = normalizeFilesystemEvent({ type: "relayfile.changed", resource: { path: "/linear/issues/ENG-1.json" } }); diff --git a/packages/sdk/typescript/src/sync.ts b/packages/sdk/typescript/src/sync.ts index b002ba7c..6dceb8a1 100644 --- a/packages/sdk/typescript/src/sync.ts +++ b/packages/sdk/typescript/src/sync.ts @@ -908,7 +908,7 @@ export class RelayFileSync { } private emitFilesystemEvent(event: FilesystemEvent): void { - if (!pathMatchesAnyFilter(this.paths, event.path)) { + if (event.type !== "sync.reconcile" && !pathMatchesAnyFilter(this.paths, event.path)) { return; } this.emit("event", event); diff --git a/packages/sdk/typescript/src/types.ts b/packages/sdk/typescript/src/types.ts index c21b70d6..07204890 100644 --- a/packages/sdk/typescript/src/types.ts +++ b/packages/sdk/typescript/src/types.ts @@ -328,6 +328,7 @@ export type FilesystemEventType = | "sync.ignored" | "sync.suppressed" | "sync.stale" + | "sync.reconcile" | "writeback.failed" | "writeback.succeeded"; From e11a5b7402db3dea0a61e89e321637bfb232dbd2 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 19:34:42 +0200 Subject: [PATCH 09/12] fix: resolve provider deletes by verified object identity --- internal/mountsync/syncer_test.go | 5 +- internal/relayfile/store.go | 75 ++++++++++++++++-- internal/relayfile/store_test.go | 125 ++++++++++++++++++++++++++++++ 3 files changed, 199 insertions(+), 6 deletions(-) diff --git a/internal/mountsync/syncer_test.go b/internal/mountsync/syncer_test.go index c79f3f14..0e6cabd5 100644 --- a/internal/mountsync/syncer_test.go +++ b/internal/mountsync/syncer_test.go @@ -12057,7 +12057,10 @@ func TestLoadStateResetsBootstrapCompleteOnlyOnWriteOnlyToMirrorSyncMode(t *test } } -func TestPullRemoteIncrementalDeleteEventStillDeletes(t *testing.T) { +// A resolved provider-object delete is emitted as one path-backed file.deleted +// event. The mount must converge the local mirror from that single event; it +// must not require a second full-pull observation. +func TestPullRemoteIncrementalResolvedProviderDeleteConvergesInOneEvent(t *testing.T) { client := &fakeClient{ files: map[string]RemoteFile{}, events: []FilesystemEvent{ diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index f606cf3f..5cfa0cc2 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -4456,17 +4456,28 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, action ApplyAction, correlationID string) { workspaceID := s.workspaceIDForStateLocked(ws) - objectID := action.ProviderObjectID + objectID := strings.TrimSpace(action.ProviderObjectID) path := normalizePath(action.Path) now := time.Now().UTC().Format(time.RFC3339Nano) - if objectID != "" && path == "/" { - if indexed, ok := ws.ProviderIndex[providerObjectKey(provider, objectID)]; ok { - path = indexed + if objectID != "" { + if path == "/" { + if resolved, ok := resolveProviderObjectPathLocked(ws, provider, objectID); ok { + path = resolved + } + } else if file, exists := ws.Files[path]; !exists || !providerObjectMatchesFile(file, provider, objectID) { + // When an envelope carries both fields, never let a stale path + // override the provider object identity. Recover through the same + // verified index/unique-metadata resolver used by pathless deletes. + if resolved, ok := resolveProviderObjectPathLocked(ws, provider, objectID); ok { + path = resolved + } else { + path = "/" + } } } if path == "/" { - if strings.TrimSpace(action.Path) == "" { + if strings.TrimSpace(action.Path) == "" || objectID != "" { // A provider delete without a resolvable path cannot safely name a // local file. Emit only a pathless control event so mounts perform // an authoritative reconciliation; never emit file.deleted with an @@ -4498,6 +4509,9 @@ func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, a } return } + // A path supplied by the provider is authoritative for path-based + // adapters. For object-identity deletes, however, the resolved path must + // have been verified by resolveProviderObjectPathLocked above. aclPermissions := resolvePermissionsFromFiles(ws.Files, path, true) delete(ws.Files, path) if objectID != "" { @@ -4519,6 +4533,57 @@ func (s *Store) applyProviderDeleteLocked(ws *workspaceState, provider string, a s.appendWorkspaceEventLocked(workspaceID, ws, event) } +// resolveProviderObjectPathLocked resolves an object-identity delete to a +// currently materialized file. The persisted index is only a hint: legacy +// state and interrupted writes can leave it absent or pointing at an +// unrelated path. In those cases, a unique metadata match is safe to use and +// repairs the index. Zero or multiple matches fail closed. +func resolveProviderObjectPathLocked(ws *workspaceState, provider, objectID string) (string, bool) { + if ws == nil || strings.TrimSpace(objectID) == "" { + return "", false + } + normalizedProvider := normalizeProvider(provider) + if normalizedProvider == "" { + return "", false + } + objectID = strings.TrimSpace(objectID) + if ws.ProviderIndex != nil { + if indexed, ok := ws.ProviderIndex[providerObjectKey(provider, objectID)]; ok { + if file, exists := ws.Files[indexed]; exists && + providerObjectMatchesFile(file, provider, objectID) { + return normalizePath(indexed), true + } + } + } + + candidate := "" + matches := 0 + for filePath, file := range ws.Files { + if !providerObjectMatchesFile(file, provider, objectID) { + continue + } + matches++ + candidate = normalizePath(filePath) + if matches > 1 { + return "", false + } + } + if matches != 1 { + return "", false + } + if ws.ProviderIndex == nil { + ws.ProviderIndex = map[string]string{} + } + ws.ProviderIndex[providerObjectKey(provider, objectID)] = candidate + return candidate, true +} + +func providerObjectMatchesFile(file File, provider, objectID string) bool { + return normalizeProvider(file.Provider) == normalizeProvider(provider) && + normalizeProvider(provider) != "" && + strings.TrimSpace(file.ProviderObjectID) == strings.TrimSpace(objectID) +} + func canonicalizeProviderActionLocked(ws *workspaceState, provider string, action ApplyAction) ApplyAction { switch action.Type { case ActionFileUpsert, ActionFileDelete: diff --git a/internal/relayfile/store_test.go b/internal/relayfile/store_test.go index 014bd83c..ca7d9316 100644 --- a/internal/relayfile/store_test.go +++ b/internal/relayfile/store_test.go @@ -1749,6 +1749,131 @@ func TestProviderDeleteWithoutPathEmitsReconcileControlEvent(t *testing.T) { t.Fatal("timed out waiting for pathless provider deletion reconciliation event") } +func TestProviderDeleteObjectIdentityResolution(t *testing.T) { + const provider = "external" + const objectID = "obj_delete_resolution" + + tests := []struct { + name string + files map[string]File + indexPath string + wantDelete string + wantControl bool + }{ + { + name: "unique metadata match repairs missing index", + files: map[string]File{ + "/external/unique.md": { + Path: "/external/unique.md", + Provider: provider, + ProviderObjectID: objectID, + Semantics: FileSemantics{Permissions: []string{"deny:agent:limited"}}, + }, + }, + wantDelete: "/external/unique.md", + }, + { + name: "stale index does not delete wrong target", + indexPath: "/external/wrong.md", + files: map[string]File{ + "/external/wrong.md": { + Path: "/external/wrong.md", + Provider: provider, + ProviderObjectID: "different-object", + }, + "/external/real.md": { + Path: "/external/real.md", + Provider: provider, + ProviderObjectID: objectID, + }, + }, + wantDelete: "/external/real.md", + }, + { + name: "ambiguous metadata matches fail closed", + files: map[string]File{ + "/external/one.md": {Path: "/external/one.md", Provider: provider, ProviderObjectID: objectID}, + "/external/two.md": {Path: "/external/two.md", Provider: provider, ProviderObjectID: objectID}, + }, + wantControl: true, + }, + { + name: "zero metadata matches fail closed", + files: map[string]File{ + "/external/other.md": {Path: "/external/other.md", Provider: provider, ProviderObjectID: "other-object"}, + }, + wantControl: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + store := NewStoreWithOptions(StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + workspaceID := "ws_provider_delete_resolution_" + strings.ReplaceAll(tc.name, " ", "_") + + store.mu.Lock() + ws := store.ensureWorkspaceLocked(workspaceID) + ws.Revision = "rev_before_delete" + for path, file := range tc.files { + ws.Files[path] = file + } + if tc.indexPath != "" { + ws.ProviderIndex[providerObjectKey(provider, objectID)] = tc.indexPath + } + store.applyProviderDeleteLocked(ws, provider, ApplyAction{ + Type: ActionFileDelete, + ProviderObjectID: objectID, + }, "corr_provider_delete_resolution") + gotRevision := ws.Revision + gotEvents := append([]Event(nil), ws.Events...) + gotFiles := make(map[string]File, len(ws.Files)) + for path, file := range ws.Files { + gotFiles[path] = file + } + gotIndex := ws.ProviderIndex[providerObjectKey(provider, objectID)] + store.mu.Unlock() + + if tc.wantDelete != "" { + if _, exists := gotFiles[tc.wantDelete]; exists { + t.Fatalf("resolved path %s was not deleted; files=%v", tc.wantDelete, gotFiles) + } + if gotRevision == "rev_before_delete" { + t.Fatal("resolved provider delete did not advance workspace revision") + } + if gotIndex != "" { + t.Fatalf("provider index entry survived resolved delete: %q", gotIndex) + } + if len(gotEvents) != 1 || gotEvents[0].Type != "file.deleted" || gotEvents[0].Path != tc.wantDelete { + t.Fatalf("resolved delete events = %+v", gotEvents) + } + if gotEvents[0].ACLPermissions == nil { + t.Fatal("resolved delete did not preserve a non-nil ACL snapshot") + } + if tc.name == "unique metadata match repairs missing index" && len(gotEvents[0].ACLPermissions) != 1 { + t.Fatalf("ACL snapshot = %v, want one permission", gotEvents[0].ACLPermissions) + } + if tc.indexPath == "/external/wrong.md" { + if _, exists := gotFiles[tc.indexPath]; !exists { + t.Fatalf("stale index target was incorrectly deleted: %s", tc.indexPath) + } + } + return + } + + if !tc.wantControl { + t.Fatal("test case missing expected outcome") + } + if gotRevision != "rev_before_delete" { + t.Fatalf("ambiguous/unresolved delete advanced revision: %q", gotRevision) + } + if len(gotEvents) != 1 || gotEvents[0].Type != "sync.reconcile" || gotEvents[0].Path != "" { + t.Fatalf("ambiguous/unresolved delete events = %+v", gotEvents) + } + }) + } +} + func TestPendingWritebacksRecoveredOnRestart(t *testing.T) { stateFile := filepath.Join(t.TempDir(), "relayfile-state.json") From 9d5f256e63c4d3adb48c069cf64c440812cb5e8f Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 20:11:54 +0200 Subject: [PATCH 10/12] fix: make provider projections and reconcile events safe --- .trajectories/index.json | 11 +++- internal/mountsync/syncer.go | 13 ++++ internal/mountsync/syncer_test.go | 52 +++++++++++++++ internal/relayfile/store.go | 105 ++++++++++++++++++++++++------ internal/relayfile/store_test.go | 97 +++++++++++++++++++++++++++ 5 files changed, 257 insertions(+), 21 deletions(-) diff --git a/.trajectories/index.json b/.trajectories/index.json index 84dd7c06..0c50342f 100644 --- a/.trajectories/index.json +++ b/.trajectories/index.json @@ -1,6 +1,6 @@ { "version": 1, - "lastUpdated": "2026-09-08T16:47:53.839Z", + "lastUpdated": "2026-09-08T18:12:03.089Z", "trajectories": { "traj_4pvrlmqfnzng": { "title": "Review PR #278 in AgentWorkforce/relayfile", @@ -211,6 +211,13 @@ "startedAt": "2026-09-08T15:41:42.556Z", "completedAt": "2026-09-08T15:47:16.049Z", "path": ".trajectories/completed/2026-09/traj_qtxkeaa5cphq.json" + }, + "traj_l5ctfmv1nb5j": { + "title": "Repair ACL-safe provider upserts and healthy realtime reconciliation", + "status": "completed", + "startedAt": "2026-09-08T18:05:42.152Z", + "completedAt": "2026-09-08T18:12:02.896Z", + "path": "/private/tmp/relayfile-acl423-fix-0908/.trajectories/completed/2026-09/traj_l5ctfmv1nb5j.json" } } -} +} \ No newline at end of file diff --git a/internal/mountsync/syncer.go b/internal/mountsync/syncer.go index 8bf7e8fd..278f2c44 100644 --- a/internal/mountsync/syncer.go +++ b/internal/mountsync/syncer.go @@ -5644,6 +5644,19 @@ func (s *Syncer) RefreshRealtimeStateWithContext(ctx context.Context) error { return err } } + if !s.writeOnly && s.forceFullReconcile { + // A pathless sync.reconcile websocket event cannot identify the + // hidden path that changed. Healthy realtime mounts still need to + // consume the durable request here; otherwise the normal heartbeat + // would keep skipping O(tree) reconciliation until the websocket + // failed or an operator manually pulled. pullRemote clears the flag + // only after the authoritative pull completes successfully. + if err := s.pullRemote(ctx, nil); err != nil { + s.markSyncError(err) + _ = s.saveStateWithoutLocalScan() + return err + } + } s.markSyncSuccess() return s.saveStateWithoutLocalScan() } diff --git a/internal/mountsync/syncer_test.go b/internal/mountsync/syncer_test.go index 0e6cabd5..e6ea093a 100644 --- a/internal/mountsync/syncer_test.go +++ b/internal/mountsync/syncer_test.go @@ -11900,6 +11900,58 @@ func TestPathlessReconcileEventRequestsPromptAuthoritativePull(t *testing.T) { } } +func TestHealthyRealtimeHeartbeatConsumesPathlessReconcileRequest(t *testing.T) { + const remotePath = "/notion/Docs/changed.md" + localDir := t.TempDir() + localPath := filepath.Join(localDir, "Docs", "changed.md") + if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { + t.Fatalf("mkdir local doc dir: %v", err) + } + if err := os.WriteFile(localPath, []byte("old content"), 0o644); err != nil { + t.Fatalf("write stale local doc: %v", err) + } + client := &fakeClient{files: map[string]RemoteFile{ + remotePath: { + Path: remotePath, + Revision: "rev_new", + ContentType: "text/markdown", + Content: "new content", + }, + }} + syncer, err := NewSyncer(client, SyncerOptions{ + WorkspaceID: "ws_healthy_realtime_reconcile", + RemoteRoot: "/notion", + LocalRoot: localDir, + FullPullEvery: -1, + }) + if err != nil { + t.Fatalf("new syncer failed: %v", err) + } + syncer.state.BootstrapComplete = true + syncer.state.EventsCursor = "evt_before_reconcile" + syncer.state.Files[remotePath] = trackedFile{Revision: "rev_old", Hash: hashString("old content")} + if err := syncer.applyWebSocketEvent(context.Background(), websocketEvent{ + EventID: "evt_healthy_reconcile", + Type: "sync.reconcile", + }); err != nil { + t.Fatalf("apply pathless reconcile event: %v", err) + } + if !syncer.forceFullReconcile { + t.Fatal("pathless reconcile event did not arm an authoritative pull") + } + beforeListTree := client.listTreeCalls + if err := syncer.RefreshRealtimeStateWithContext(context.Background()); err != nil { + t.Fatalf("healthy realtime heartbeat did not consume reconcile request: %v", err) + } + if client.listTreeCalls <= beforeListTree { + t.Fatalf("healthy realtime heartbeat skipped the requested authoritative pull: listTreeCalls=%d before=%d", client.listTreeCalls, beforeListTree) + } + if syncer.forceFullReconcile { + t.Fatal("successful heartbeat pull left the durable reconcile request armed") + } + assertLocalFileContent(t, localPath, "new content") +} + func TestApplyRemoteSnapshotDeletesRevClearsReadNotReadyMarkerAfterConfirmedDelete(t *testing.T) { const remotePath = "/notion/Docs/deleted.md" localDir := t.TempDir() diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index 5cfa0cc2..70917d02 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -4369,11 +4369,22 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a path := normalizePath(action.Path) objectID := strings.TrimSpace(action.ProviderObjectID) if path == "/" && objectID != "" { - if indexedPath, ok := ws.ProviderIndex[providerObjectKey(provider, objectID)]; ok { - path = indexedPath - } else { - path = fallbackProviderPath(provider, objectID) + resolvedPath, ok := resolveProviderObjectUpsertPathLocked(ws, provider, objectID) + if !ok { + // A pathless provider action must never overwrite a path whose + // ownership cannot be proven. Ask mounts to perform an authoritative + // reconciliation rather than materializing an unsafe projection. + s.appendWorkspaceEventLocked(workspaceID, ws, Event{ + EventID: s.nextEventIDLocked(), + Type: "sync.reconcile", + Origin: "provider_sync", + Provider: provider, + CorrelationID: correlationID, + Timestamp: time.Now().UTC().Format(time.RFC3339Nano), + }) + return } + path = resolvedPath } if path == "/" { return @@ -4389,22 +4400,30 @@ func (s *Store) applyProviderUpsertLocked(ws *workspaceState, provider string, a if objectID != "" { key := providerObjectKey(provider, objectID) if previousPath, ok := ws.ProviderIndex[key]; ok && previousPath != path { - aclPermissions := resolvePermissionsFromFiles(ws.Files, previousPath, true) - delete(ws.Files, previousPath) - moveRevision := s.nextRevisionLocked() - ws.Revision = moveRevision - event := Event{ - EventID: s.nextEventIDLocked(), - Type: "file.deleted", - Path: previousPath, - Revision: moveRevision, - Origin: "provider_sync", - Provider: provider, - CorrelationID: correlationID, - Timestamp: now, + previousFile, previousExists := ws.Files[previousPath] + if previousExists && providerObjectMatchesFile(previousFile, provider, objectID) { + aclPermissions := resolvePermissionsFromFiles(ws.Files, previousPath, true) + delete(ws.Files, previousPath) + moveRevision := s.nextRevisionLocked() + ws.Revision = moveRevision + event := Event{ + EventID: s.nextEventIDLocked(), + Type: "file.deleted", + Path: previousPath, + Revision: moveRevision, + Origin: "provider_sync", + Provider: provider, + CorrelationID: correlationID, + Timestamp: now, + } + event.ACLPermissions = snapshotACLPermissions(aclPermissions) + s.appendWorkspaceEventLocked(workspaceID, ws, event) + } else { + // The index was stale or pointed at a different object's file. + // It is not an authorization to delete that file; discard only + // the bad index entry before recording the new projection. + delete(ws.ProviderIndex, key) } - event.ACLPermissions = snapshotACLPermissions(aclPermissions) - s.appendWorkspaceEventLocked(workspaceID, ws, event) } } @@ -4584,6 +4603,54 @@ func providerObjectMatchesFile(file File, provider, objectID string) bool { strings.TrimSpace(file.ProviderObjectID) == strings.TrimSpace(objectID) } +// resolveProviderObjectUpsertPathLocked resolves the identity projection for +// a pathless provider upsert. The persisted index is only a hint; unlike the +// old pathless path, every indexed hit is checked against the live file's +// provider/object identity before it can be overwritten. If no existing +// identity can be proved, retain the legacy projection when it is free and +// deterministically disambiguate only when that projection is occupied by a +// different object. +func resolveProviderObjectUpsertPathLocked(ws *workspaceState, provider, objectID string) (string, bool) { + if ws == nil || strings.TrimSpace(objectID) == "" || normalizeProvider(provider) == "" { + return "", false + } + objectID = strings.TrimSpace(objectID) + if resolved, ok := resolveProviderObjectPathLocked(ws, provider, objectID); ok { + return resolved, true + } + + legacyPath := fallbackProviderPath(provider, objectID) + if legacyPath == "/" { + return "", false + } + if existing, exists := ws.Files[legacyPath]; !exists || providerObjectMatchesFile(existing, provider, objectID) { + return legacyPath, true + } + + // Keep the legacy path for the first object (backward compatibility), but + // never let sanitized IDs such as "a/b" and "a_b" overwrite one another. + // Include the canonical provider and raw object ID in the digest so the + // disambiguated projection is stable across restarts and index loss. + digest := sha256.Sum256([]byte(normalizeProvider(provider) + "\x00" + objectID)) + base := strings.TrimSuffix(legacyPath, ".md") + for _, suffix := range []string{hex.EncodeToString(digest[:8]), hex.EncodeToString(digest[:])} { + candidate := normalizePath(base + "-" + suffix + ".md") + if existing, exists := ws.Files[candidate]; !exists || providerObjectMatchesFile(existing, provider, objectID) { + return candidate, true + } + } + // A full digest collision is extraordinarily unlikely, but do not turn + // that assumption into a destructive overwrite. A bounded deterministic + // suffix gives a corrupt/adversarial workspace a safe escape hatch. + for attempt := 2; attempt <= 1024; attempt++ { + candidate := normalizePath(fmt.Sprintf("%s-%s-%d.md", base, hex.EncodeToString(digest[:]), attempt)) + if existing, exists := ws.Files[candidate]; !exists || providerObjectMatchesFile(existing, provider, objectID) { + return candidate, true + } + } + return "", false +} + func canonicalizeProviderActionLocked(ws *workspaceState, provider string, action ApplyAction) ApplyAction { switch action.Type { case ActionFileUpsert, ActionFileDelete: diff --git a/internal/relayfile/store_test.go b/internal/relayfile/store_test.go index ca7d9316..a4ad2fde 100644 --- a/internal/relayfile/store_test.go +++ b/internal/relayfile/store_test.go @@ -1707,6 +1707,103 @@ func TestProviderUpsertWithoutPathUsesObjectIdentity(t *testing.T) { t.Fatalf("expected object-identity upsert without path to update existing projected file") } +func TestProviderUpsertWithoutPathRejectsStaleIndexIdentity(t *testing.T) { + const provider = "external" + const objectID = "object_target" + const stalePath = "/external/wrong.md" + + store := NewStoreWithOptions(StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + store.mu.Lock() + ws := store.ensureWorkspaceLocked("ws_provider_upsert_stale_index") + ws.Files[stalePath] = File{ + Path: stalePath, + Content: "wrong object must survive", + ContentType: "text/markdown", + Provider: provider, + ProviderObjectID: "different_object", + } + ws.ProviderIndex[providerObjectKey(provider, objectID)] = stalePath + store.applyProviderUpsertLocked(ws, provider, ApplyAction{ + Type: ActionFileUpsert, + ProviderObjectID: objectID, + Content: "target content", + }, "corr_provider_upsert_stale_index") + gotFiles := make(map[string]File, len(ws.Files)) + for path, file := range ws.Files { + gotFiles[path] = file + } + gotPath := ws.ProviderIndex[providerObjectKey(provider, objectID)] + store.mu.Unlock() + + wrong, ok := gotFiles[stalePath] + if !ok || wrong.ProviderObjectID != "different_object" || wrong.Content != "wrong object must survive" { + t.Fatalf("stale index target was overwritten: files=%+v", gotFiles) + } + if gotPath == "" || gotPath == stalePath { + t.Fatalf("pathless upsert trusted stale index: provider index=%q", gotPath) + } + target, ok := gotFiles[gotPath] + if !ok || target.ProviderObjectID != objectID || target.Content != "target content" { + t.Fatalf("target identity was not materialized safely: path=%q files=%+v", gotPath, gotFiles) + } +} + +func TestProviderUpsertWithoutPathDisambiguatesSanitizedIDCollision(t *testing.T) { + const provider = "external" + store := NewStoreWithOptions(StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + store.mu.Lock() + ws := store.ensureWorkspaceLocked("ws_provider_upsert_collision") + for _, tc := range []struct { + objectID string + content string + }{ + {objectID: "a/b", content: "slash identity"}, + {objectID: "a_b", content: "underscore identity"}, + } { + store.applyProviderUpsertLocked(ws, provider, ApplyAction{ + Type: ActionFileUpsert, + ProviderObjectID: tc.objectID, + Content: tc.content, + }, "corr_provider_upsert_collision") + } + firstPath := ws.ProviderIndex[providerObjectKey(provider, "a/b")] + secondPath := ws.ProviderIndex[providerObjectKey(provider, "a_b")] + first := ws.Files[firstPath] + second := ws.Files[secondPath] + store.mu.Unlock() + + if firstPath != fallbackProviderPath(provider, "a/b") { + t.Fatalf("first legacy projection changed unexpectedly: %q", firstPath) + } + if secondPath == "" || secondPath == firstPath { + t.Fatalf("sanitized object IDs collided at %q", secondPath) + } + if first.ProviderObjectID != "a/b" || first.Content != "slash identity" { + t.Fatalf("first object was overwritten: %+v", first) + } + if second.ProviderObjectID != "a_b" || second.Content != "underscore identity" { + t.Fatalf("second object was not materialized: %+v", second) + } + + // A repeated pathless update must remain on the same deterministic + // disambiguated projection rather than creating another collision path. + store.mu.Lock() + ws = store.ensureWorkspaceLocked("ws_provider_upsert_collision") + store.applyProviderUpsertLocked(ws, provider, ApplyAction{ + Type: ActionFileUpsert, + ProviderObjectID: "a_b", + Content: "underscore identity updated", + }, "corr_provider_upsert_collision_update") + updatedPath := ws.ProviderIndex[providerObjectKey(provider, "a_b")] + updated := ws.Files[updatedPath] + store.mu.Unlock() + if updatedPath != secondPath || updated.Content != "underscore identity updated" { + t.Fatalf("repeated pathless update moved collision-safe projection: path=%q file=%+v", updatedPath, updated) + } +} + func TestProviderDeleteWithoutPathEmitsReconcileControlEvent(t *testing.T) { store := NewStore() t.Cleanup(store.Close) From d64547f77d36ce61d44378397fb342a212defdff Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 20:17:19 +0200 Subject: [PATCH 11/12] fix: reject ambiguous provider upserts --- .trajectories/index.json | 9 +++++++- internal/relayfile/store.go | 9 ++++++++ internal/relayfile/store_test.go | 39 ++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/.trajectories/index.json b/.trajectories/index.json index 0c50342f..7364b88d 100644 --- a/.trajectories/index.json +++ b/.trajectories/index.json @@ -1,6 +1,6 @@ { "version": 1, - "lastUpdated": "2026-09-08T18:12:03.089Z", + "lastUpdated": "2026-09-08T18:17:10.919Z", "trajectories": { "traj_4pvrlmqfnzng": { "title": "Review PR #278 in AgentWorkforce/relayfile", @@ -218,6 +218,13 @@ "startedAt": "2026-09-08T18:05:42.152Z", "completedAt": "2026-09-08T18:12:02.896Z", "path": "/private/tmp/relayfile-acl423-fix-0908/.trajectories/completed/2026-09/traj_l5ctfmv1nb5j.json" + }, + "traj_f5e9ezrw8o6g": { + "title": "Fail closed on ambiguous provider-object pathless upserts", + "status": "completed", + "startedAt": "2026-09-08T18:17:10.022Z", + "completedAt": "2026-09-08T18:17:10.768Z", + "path": "/private/tmp/relayfile-acl423-fix-0908/.trajectories/completed/2026-09/traj_f5e9ezrw8o6g.json" } } } \ No newline at end of file diff --git a/internal/relayfile/store.go b/internal/relayfile/store.go index 70917d02..28a3cb14 100644 --- a/internal/relayfile/store.go +++ b/internal/relayfile/store.go @@ -4618,6 +4618,15 @@ func resolveProviderObjectUpsertPathLocked(ws *workspaceState, provider, objectI if resolved, ok := resolveProviderObjectPathLocked(ws, provider, objectID); ok { return resolved, true } + // A failed lookup can mean either no projection or an ambiguous corrupt + // state with multiple projections for the same identity. Only the former + // may allocate a fallback path; choosing one of several matches would make + // a pathless upsert mutate an arbitrary duplicate instead of failing closed. + for _, file := range ws.Files { + if providerObjectMatchesFile(file, provider, objectID) { + return "", false + } + } legacyPath := fallbackProviderPath(provider, objectID) if legacyPath == "/" { diff --git a/internal/relayfile/store_test.go b/internal/relayfile/store_test.go index a4ad2fde..b56fb044 100644 --- a/internal/relayfile/store_test.go +++ b/internal/relayfile/store_test.go @@ -1804,6 +1804,45 @@ func TestProviderUpsertWithoutPathDisambiguatesSanitizedIDCollision(t *testing.T } } +func TestProviderUpsertWithoutPathFailsClosedOnAmbiguousIdentity(t *testing.T) { + const provider = "external" + const objectID = "object_ambiguous" + store := NewStoreWithOptions(StoreOptions{DisableWorkers: true}) + t.Cleanup(store.Close) + store.mu.Lock() + ws := store.ensureWorkspaceLocked("ws_provider_upsert_ambiguous") + for _, path := range []string{"/external/first.md", "/external/second.md"} { + ws.Files[path] = File{ + Path: path, + Content: "must survive " + path, + ContentType: "text/markdown", + Provider: provider, + ProviderObjectID: objectID, + } + } + beforeRevision := ws.Revision + store.applyProviderUpsertLocked(ws, provider, ApplyAction{ + Type: ActionFileUpsert, + ProviderObjectID: objectID, + Content: "must not replace an arbitrary duplicate", + }, "corr_provider_upsert_ambiguous") + first := ws.Files["/external/first.md"] + second := ws.Files["/external/second.md"] + events := append([]Event(nil), ws.Events...) + gotRevision := ws.Revision + store.mu.Unlock() + + if first.Content != "must survive /external/first.md" || second.Content != "must survive /external/second.md" { + t.Fatalf("ambiguous pathless upsert mutated a duplicate: first=%+v second=%+v", first, second) + } + if gotRevision != beforeRevision { + t.Fatalf("ambiguous pathless upsert advanced file revision: got %s want %s", gotRevision, beforeRevision) + } + if len(events) != 1 || events[0].Type != "sync.reconcile" || events[0].Path != "" { + t.Fatalf("ambiguous pathless upsert events = %+v, want one pathless sync.reconcile", events) + } +} + func TestProviderDeleteWithoutPathEmitsReconcileControlEvent(t *testing.T) { store := NewStore() t.Cleanup(store.Close) From 00358450200cc4ab1c2ff56cf0471d814d483f52 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Tue, 8 Sep 2026 20:59:01 +0200 Subject: [PATCH 12/12] test(acl): prove relative paths cannot bypass denies --- internal/httpapi/acl_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/httpapi/acl_test.go b/internal/httpapi/acl_test.go index b2b7addc..8ebd9eee 100644 --- a/internal/httpapi/acl_test.go +++ b/internal/httpapi/acl_test.go @@ -155,6 +155,20 @@ func TestFilePermissionAllows(t *testing.T) { path: "/protected/document.md", want: false, }, + { + name: "relative request path cannot bypass a normalized deny", + permissions: []string{ + "allow:scope:fs:write", + "deny:scope:relayfile:fs:write:/protected/*", + }, + workspaceID: "ws_123", + claims: tokenClaims{ + Scopes: map[string]struct{}{"relayfile:fs:write:*": {}}, + }, + action: "write", + path: "protected/document.md", + want: false, + }, { name: "readonly write deny does not block reads", permissions: []string{