From 37574a13e383e76cddc6d60dc56ac59c0a682559 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Tue, 6 Oct 2026 10:53:17 +0900 Subject: [PATCH 1/3] fix(sjskills): refuse local source identities in global restore Global desired sources are always remote and global inventory already distrusts `local:` provenance, but restore validated identities without scope. A tampered global quarantine could therefore restore a record that the next global plan would treat as untrusted. Restore now uses a scope-aware check so it fails at restore time instead. Co-Authored-By: Claude Opus 5.5 --- internal/sjskills/classify.go | 8 ++++++++ internal/sjskills/local_source_test.go | 18 ++++++++++++++++++ internal/sjskills/project_restore.go | 8 ++++---- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/internal/sjskills/classify.go b/internal/sjskills/classify.go index 9f68d60..f7cf0ae 100644 --- a/internal/sjskills/classify.go +++ b/internal/sjskills/classify.go @@ -582,6 +582,14 @@ func canonicalProjectSourceIdentity(source string) (string, bool) { return "https://" + hostPort + cleanPath, true } +// isCanonicalSourceIdentityForScope additionally refuses local identities in +// global scope, whose desired sources are always remote. Restore uses it so a +// tampered global quarantine cannot reintroduce provenance that global +// inventory would then distrust. +func isCanonicalSourceIdentityForScope(scope Scope, identity string) bool { + return isCanonicalProjectSourceIdentity(identity) && !(scope == ScopeGlobal && isCanonicalLocalSourceIdentity(identity)) +} + func isCanonicalProjectSourceIdentity(identity string) bool { if strings.HasPrefix(identity, localSourceIdentityPrefix) { return isCanonicalLocalSourceIdentity(identity) diff --git a/internal/sjskills/local_source_test.go b/internal/sjskills/local_source_test.go index 491f1fb..31f50d0 100644 --- a/internal/sjskills/local_source_test.go +++ b/internal/sjskills/local_source_test.go @@ -395,3 +395,21 @@ func TestLocalOnlyStatusDoesNotDependOnUpstreamCache(t *testing.T) { } requireStatusFinding(t, advisory, AdvisoryMissing, "team-tool", TargetAgents, string(ProjectStateReasonExpectedEntryAbsent)) } + +func TestRestoreRejectsLocalIdentityInGlobalScope(t *testing.T) { + if !isCanonicalSourceIdentityForScope(ScopeProject, "local:./skills/team-tool") || + isCanonicalSourceIdentityForScope(ScopeGlobal, "local:./skills/team-tool") || + !isCanonicalSourceIdentityForScope(ScopeGlobal, "github:owner/repo") { + t.Fatal("scope-aware identity validation is wrong") + } + entry := restoreEntry{ + entry: ProjectQuarantineManifestEntry{Skill: "team-tool", Target: TargetAgents, Action: ProjectQuarantineEntryActionRemove, OldSourceIdentity: "local:./skills/team-tool"}, + oldHash: classificationHash('a'), + } + if _, err := buildRestoreProvenanceState(ProvenanceState{}, []restoreEntry{entry}, time.Now(), ScopeGlobal); err == nil { + t.Fatal("global restore accepted a local source identity") + } + if _, err := buildRestoreProvenanceState(ProvenanceState{}, []restoreEntry{entry}, time.Now(), ScopeProject); err != nil { + t.Fatalf("project restore rejected a local source identity: %v", err) + } +} diff --git a/internal/sjskills/project_restore.go b/internal/sjskills/project_restore.go index 07f8c6d..dd0594f 100644 --- a/internal/sjskills/project_restore.go +++ b/internal/sjskills/project_restore.go @@ -490,7 +490,7 @@ func (tx *restoreTransaction) preflight(ctx context.Context) error { state.state = emptyTransactionProvenanceState(tx.scope) } for _, record := range state.state.Records { - if !isCanonicalProjectSourceIdentity(record.SourceIdentity) { + if !isCanonicalSourceIdentityForScope(tx.scope, record.SourceIdentity) { return restoreConflict("provenance identity is incompatible") } } @@ -596,7 +596,7 @@ func validateRestoreProvenance(state ProvenanceState, entry ProjectQuarantineMan } if !exists || record.Scope != scope || record.Target != entry.Target || record.Skill != entry.Skill || record.SourceIdentity != entry.NewSourceIdentity || record.TreeHashAlgorithm != entry.TreeHashAlgorithm || record.TreeHash != entry.NewTreeHash || - !isCanonicalProjectSourceIdentity(record.SourceIdentity) { + !isCanonicalSourceIdentityForScope(scope, record.SourceIdentity) { return restoreConflict("update provenance does not match the manifest") } return nil @@ -610,7 +610,7 @@ func validateRestoreProvenance(state ProvenanceState, entry ProjectQuarantineMan } if !exists || record.Scope != scope || record.Target != entry.Target || record.Skill != entry.Skill || record.SourceIdentity != oldSource || record.TreeHashAlgorithm != entry.TreeHashAlgorithm || record.TreeHash != entry.OldTreeHash || - !isCanonicalProjectSourceIdentity(record.SourceIdentity) { + !isCanonicalSourceIdentityForScope(scope, record.SourceIdentity) { return restoreConflict("restored provenance does not match the manifest") } return nil @@ -1016,7 +1016,7 @@ func buildRestoreProvenanceState(previous ProvenanceState, entries []restoreEntr delete(records, key) continue } - if !isCanonicalProjectSourceIdentity(entry.entry.OldSourceIdentity) { + if !isCanonicalSourceIdentityForScope(scope, entry.entry.OldSourceIdentity) { return ProvenanceState{}, restoreConflict("quarantine source identity is incompatible") } records[projectPlacementKey(entry.entry.Target, entry.entry.Skill)] = ProvenanceRecord{ From 8256221f09ae1e152e13876632fde078407d494e Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Tue, 6 Oct 2026 10:53:17 +0900 Subject: [PATCH 2/3] test(sjskills): skip the manifest-symlink status case without symlink privileges Windows hosts without Developer Mode or elevation cannot create symlinks. Skip that one subtest there, as the other symlink tests already do, so the full suite passes locally; Linux CI still exercises it. Co-Authored-By: Claude Opus 5.5 --- cmd/sjskills/default_status_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/sjskills/default_status_test.go b/cmd/sjskills/default_status_test.go index a023f7a..fb40f6e 100644 --- a/cmd/sjskills/default_status_test.go +++ b/cmd/sjskills/default_status_test.go @@ -202,6 +202,9 @@ func TestDefaultStatusNestedAndUnsafeConfiguration(t *testing.T) { err = os.Mkdir(manifest, 0755) } else { err = os.Symlink(filepath.Join(f.project, "absent"), manifest) + if err != nil && runtime.GOOS == "windows" { + t.Skipf("symlinks unavailable: %v", err) + } } if err != nil { t.Fatal(err) From 33da2757246f96a164dbbcac8dde274e1b4c5ff8 Mon Sep 17 00:00:00 2001 From: Sejun Park Date: Tue, 6 Oct 2026 11:00:31 +0900 Subject: [PATCH 3/3] Address CodeRabbit review on scope-aware restore and symlink skip Skip the Windows symlink subtest only for ERROR_PRIVILEGE_NOT_HELD so any other symlink failure still fails the test, and express the scope check without a negated conjunction. Co-Authored-By: Claude Opus 5.5 --- cmd/sjskills/default_status_test.go | 4 +++- internal/sjskills/classify.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/sjskills/default_status_test.go b/cmd/sjskills/default_status_test.go index fb40f6e..8ecb479 100644 --- a/cmd/sjskills/default_status_test.go +++ b/cmd/sjskills/default_status_test.go @@ -10,6 +10,7 @@ import ( "reflect" "runtime" "strings" + "syscall" "testing" "time" @@ -202,7 +203,8 @@ func TestDefaultStatusNestedAndUnsafeConfiguration(t *testing.T) { err = os.Mkdir(manifest, 0755) } else { err = os.Symlink(filepath.Join(f.project, "absent"), manifest) - if err != nil && runtime.GOOS == "windows" { + // ERROR_PRIVILEGE_NOT_HELD: no Developer Mode or elevation. + if runtime.GOOS == "windows" && errors.Is(err, syscall.Errno(1314)) { t.Skipf("symlinks unavailable: %v", err) } } diff --git a/internal/sjskills/classify.go b/internal/sjskills/classify.go index f7cf0ae..d28058a 100644 --- a/internal/sjskills/classify.go +++ b/internal/sjskills/classify.go @@ -587,7 +587,7 @@ func canonicalProjectSourceIdentity(source string) (string, bool) { // tampered global quarantine cannot reintroduce provenance that global // inventory would then distrust. func isCanonicalSourceIdentityForScope(scope Scope, identity string) bool { - return isCanonicalProjectSourceIdentity(identity) && !(scope == ScopeGlobal && isCanonicalLocalSourceIdentity(identity)) + return isCanonicalProjectSourceIdentity(identity) && (scope != ScopeGlobal || !isCanonicalLocalSourceIdentity(identity)) } func isCanonicalProjectSourceIdentity(identity string) bool {