From c26420a3d8ce5d91b8b4bc04fa28b8bd4f74306c Mon Sep 17 00:00:00 2001 From: Subham Ray Date: Thu, 23 Jul 2026 03:00:55 +0530 Subject: [PATCH 1/5] feat(devicepolicy): enforce VS Code private marketplace URL Fold an optional extensions.gallery.serviceUrl into the existing ide_extension#vscode reconciler, written into the user-scope settings.json beside extensions.allowed via one atomic multi-key write. The agent owns both keys: set is authoritative, removal is ownership-gated (never deletes a user-configured value), and drift, convergence, selective clear, and post-write rollback all cover both. - api: lift gallery_service_url from run-config into EffectivePolicy - settings_writer: managedSettingsWriter (ReadManaged / ApplyManaged / RestoreManaged) over a set of keyed ops; single-key Write/Read/Clear kept - reconcile: ownership-gated set/remove/preserve for the gallery key; convergence over both keys before the idempotency short-circuit; drift over owned keys; atomic multi-key rollback; per-key ownership-safe clear - cache: additive WrittenSettings map (omitempty; an allowlist-only record is unchanged on disk) - probe: yield mdm_managed when an MDM owns AllowedExtensions OR ExtensionGalleryServiceUrl (Windows / macOS / Linux) Additive and regression-guarded: an allowlist-only policy writes exactly as before (byte-identical settings.json and ownership record). --- internal/devicepolicy/api.go | 39 +- internal/devicepolicy/api_test.go | 31 ++ internal/devicepolicy/cache.go | 16 +- internal/devicepolicy/cache_test.go | 50 ++ internal/devicepolicy/probe.go | 28 +- internal/devicepolicy/probe_darwin.go | 14 +- internal/devicepolicy/probe_darwin_test.go | 61 +++ internal/devicepolicy/probe_linux.go | 16 +- internal/devicepolicy/probe_linux_test.go | 44 ++ internal/devicepolicy/probe_test.go | 34 ++ internal/devicepolicy/probe_windows.go | 22 +- internal/devicepolicy/probe_windows_test.go | 13 + internal/devicepolicy/reconcile.go | 357 +++++++++++-- internal/devicepolicy/reconcile_test.go | 505 +++++++++++++++++- internal/devicepolicy/settings_writer.go | 180 ++++++- internal/devicepolicy/settings_writer_test.go | 221 ++++++++ 16 files changed, 1542 insertions(+), 89 deletions(-) create mode 100644 internal/devicepolicy/probe_darwin_test.go create mode 100644 internal/devicepolicy/probe_linux_test.go diff --git a/internal/devicepolicy/api.go b/internal/devicepolicy/api.go index 1d4c9e72..bd1bc015 100644 --- a/internal/devicepolicy/api.go +++ b/internal/devicepolicy/api.go @@ -48,12 +48,13 @@ const maxRunConfigBytes = 4 << 20 // means run-config carried no directive for this category/target → reconciler // no-op. type EffectivePolicy struct { - Category string - Target string - Clear bool - Policy json.RawMessage - Hash string - GeneratedAt string + Category string + Target string + Clear bool + Policy json.RawMessage + Hash string + GeneratedAt string + GalleryServiceURL string } // present reports whether the backend expressed a policy directive for this @@ -68,12 +69,13 @@ func (ep EffectivePolicy) present() bool { return ep.Clear || len(ep.Policy) > 0 // backend still emitting legacy extras (e.g. the removed min_vscode_version) // stays compatible. type policyEnvelope struct { - Category string `json:"category"` - Target string `json:"target"` - Clear bool `json:"clear"` - Policy json.RawMessage `json:"policy,omitempty"` - Hash string `json:"hash,omitempty"` - GeneratedAt string `json:"generated_at"` + Category string `json:"category"` + Target string `json:"target"` + Clear bool `json:"clear"` + Policy json.RawMessage `json:"policy,omitempty"` + Hash string `json:"hash,omitempty"` + GeneratedAt string `json:"generated_at"` + GalleryServiceURL string `json:"gallery_service_url,omitempty"` } // Fetcher returns the effective policy for one device + category + target. @@ -192,12 +194,13 @@ func (c *HTTPFetcher) Fetch(ctx context.Context, customerID, deviceID, category, p := env.Policy ep := EffectivePolicy{ - Category: strings.TrimSpace(p.Category), - Target: strings.TrimSpace(p.Target), - Clear: p.Clear, - Policy: p.Policy, - Hash: strings.TrimSpace(p.Hash), - GeneratedAt: p.GeneratedAt, + Category: strings.TrimSpace(p.Category), + Target: strings.TrimSpace(p.Target), + Clear: p.Clear, + Policy: p.Policy, + Hash: strings.TrimSpace(p.Hash), + GeneratedAt: p.GeneratedAt, + GalleryServiceURL: strings.TrimSpace(p.GalleryServiceURL), } if ep.Category == "" { ep.Category = category diff --git a/internal/devicepolicy/api_test.go b/internal/devicepolicy/api_test.go index e6d2ca0b..f4329297 100644 --- a/internal/devicepolicy/api_test.go +++ b/internal/devicepolicy/api_test.go @@ -80,6 +80,37 @@ func TestFetchClear(t *testing.T) { } } +func TestFetchLiftsGalleryServiceURL(t *testing.T) { + // A policy carrying gallery_service_url lifts it into EffectivePolicy; the + // hash is unchanged in shape (backend folds the URL into it). + body := `{"policy":{"category":"ide_extension","target":"vscode","clear":false,` + + `"policy":{"*":false},"hash":"sha256:g","gallery_service_url":"https://mkt.example/api/v1",` + + `"generated_at":"2026-07-23T00:00:00Z"}}` + _, f := newFetchServer(t, 200, body) + ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if ep.GalleryServiceURL != "https://mkt.example/api/v1" { + t.Fatalf("gallery_service_url = %q, want the URL", ep.GalleryServiceURL) + } +} + +func TestFetchAbsentGalleryServiceURLIsEmpty(t *testing.T) { + // A policy without gallery_service_url (the common allowlist-only case) + // leaves the field empty and is not an error. + body := `{"policy":{"category":"ide_extension","clear":false,` + + `"policy":{"*":false},"hash":"sha256:h","generated_at":"2026-07-23T00:00:00Z"}}` + _, f := newFetchServer(t, 200, body) + ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if ep.GalleryServiceURL != "" { + t.Fatalf("gallery_service_url should be empty, got %q", ep.GalleryServiceURL) + } +} + func TestFetchAbsentPolicyReturnsEmptyNoError(t *testing.T) { // An omitted/null `policy` means run-config carried no directive for this // category. It is NOT an error and NOT a clear: Fetch returns a zero diff --git a/internal/devicepolicy/cache.go b/internal/devicepolicy/cache.go index a5973da6..57a3e40a 100644 --- a/internal/devicepolicy/cache.go +++ b/internal/devicepolicy/cache.go @@ -51,7 +51,7 @@ type AppliedCategoryState struct { } // AppliedTargetState records what the agent last wrote to the user-scope VS -// Code settings.json for one (category, target). Two fields drive correctness: +// Code settings.json for one (category, target). Three fields drive correctness: // // - AppliedHash is the backend's content hash, stored VERBATIM (never // recomputed). Compared against the freshly-fetched hash for idempotency. @@ -61,13 +61,21 @@ type AppliedCategoryState struct { // WrittenValue (a differing value — e.g. the user's own — is left // untouched); on enforce, an on-disk value differing from WrittenValue is // drift and is converged back. +// - WrittenSettings holds the same value-based ownership for the other +// managed settings keys (the gallery service URL): setting id → the exact +// compacted value the agent wrote. A key absent from the map is one the +// agent does not own, so removal and clear leave it untouched — as an empty +// WrittenValue does for the allowlist. The allowlist stays in WrittenValue, +// and omitempty drops the map, so an allowlist-only record is unchanged on +// disk. // // A zero-value entry means "the agent owns nothing on disk" for that // category/target. type AppliedTargetState struct { - AppliedHash string `json:"applied_hash"` - WrittenValue string `json:"written_value"` - FetchedAt time.Time `json:"fetched_at"` + AppliedHash string `json:"applied_hash"` + WrittenValue string `json:"written_value"` + WrittenSettings map[string]string `json:"written_settings,omitempty"` + FetchedAt time.Time `json:"fetched_at"` } // cacheMu serializes the read-modify-write of the shared state file so two diff --git a/internal/devicepolicy/cache_test.go b/internal/devicepolicy/cache_test.go index e56f708b..0cb2b1c6 100644 --- a/internal/devicepolicy/cache_test.go +++ b/internal/devicepolicy/cache_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "time" ) @@ -358,3 +359,52 @@ func TestOldCategoryShapeReadsAsOwnsNothing(t *testing.T) { t.Fatal("pre-target category-only file should read as owns-nothing (no migration)") } } + +func TestAppliedTargetWrittenSettingsRoundTrip(t *testing.T) { + restore := SetCachePathForTest(filepath.Join(t.TempDir(), CacheFilename)) + defer restore() + + want := AppliedTargetState{ + AppliedHash: "sha256:abc", + WrittenValue: samplePolicy, + WrittenSettings: map[string]string{ + galleryServiceURLSettingKey: `"https://mkt.example/api/v1"`, + }, + FetchedAt: time.Date(2026, 7, 23, 0, 0, 0, 0, time.UTC), + } + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, want); err != nil { + t.Fatal(err) + } + got, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode) + if !ok { + t.Fatal("ok=false after write") + } + if got.WrittenSettings[galleryServiceURLSettingKey] != want.WrittenSettings[galleryServiceURLSettingKey] { + t.Fatalf("WrittenSettings not round-tripped: got %+v", got.WrittenSettings) + } +} + +func TestAppliedTargetNoWrittenSettingsOmitsField(t *testing.T) { + path := filepath.Join(t.TempDir(), CacheFilename) + restore := SetCachePathForTest(path) + defer restore() + + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:H", WrittenValue: samplePolicy, + }); err != nil { + t.Fatal(err) + } + // Byte-shape parity: an allowlist-only record must omit written_settings. + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "written_settings") { + t.Fatalf("allowlist-only record must omit written_settings:\n%s", raw) + } + // And it reads back as a nil map (owns no extra keys). + got, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode) + if !ok || got.WrittenSettings != nil { + t.Fatalf("WrittenSettings must be nil for an allowlist-only record, got %+v ok=%v", got.WrittenSettings, ok) + } +} diff --git a/internal/devicepolicy/probe.go b/internal/devicepolicy/probe.go index 687065bc..4be10e61 100644 --- a/internal/devicepolicy/probe.go +++ b/internal/devicepolicy/probe.go @@ -17,11 +17,29 @@ import ( // would be ignored. const allowedExtensionsName = "AllowedExtensions" -// ProbeManagedPolicy (probe_.go) reports whether an MDM/admin-managed -// AllowedExtensions policy is present at this OS's policy location, plus a -// human-readable location for logs. Read-only, never elevated, and -// best-effort: an unreadable location reads as "not managed" — enforcement -// must not be blocked by a probe that cannot decide. +// galleryServiceURLName is VS Code's registered POLICY name for the +// `extensions.gallery.serviceUrl` setting — the registry value name on Windows, +// the JSON key in /etc/vscode/policy.json on Linux, and the plist key in macOS +// managed preferences. Like allowedExtensionsName it is the POLICY name probed +// read-only at OS policy locations, NOT the setting id the agent writes +// (galleryServiceURLSettingKey). +const galleryServiceURLName = "ExtensionGalleryServiceUrl" + +// managedPolicyNames are the VS Code POLICY names whose presence makes the +// agent yield the whole ide_extension category (mdm_managed). An MDM/admin +// policy for EITHER key means a higher-precedence surface owns this category, +// so the agent never half-owns it alongside an MDM. Presence-only, not a value +// comparison. Order is the reporting preference for the log detail. +func managedPolicyNames() []string { + return []string{allowedExtensionsName, galleryServiceURLName} +} + +// ProbeManagedPolicy (probe_.go) reports whether an MDM/admin-managed VS +// Code policy (AllowedExtensions or ExtensionGalleryServiceUrl) is present at +// this OS's policy location, plus a human-readable location for logs. +// Read-only, never elevated, and best-effort: an unreadable location reads as +// "not managed" — enforcement must not be blocked by a probe that cannot +// decide. // jsonFileHasKey reports whether the JSON object file at path contains key as // a top-level member. Falls back to a byte scan for `"key"` when the file is diff --git a/internal/devicepolicy/probe_darwin.go b/internal/devicepolicy/probe_darwin.go index f49879ad..5463a92d 100644 --- a/internal/devicepolicy/probe_darwin.go +++ b/internal/devicepolicy/probe_darwin.go @@ -14,10 +14,10 @@ const ( ) // ProbeManagedPolicy reports whether an MDM-installed VS Code managed -// preference mentions AllowedExtensions, machine-wide or for any user. The -// plists are typically binary, but plist key names are stored as plain ASCII -// runs in both binary and XML encodings, so a byte scan is a reliable -// presence check without a plist parser dependency. +// preference mentions AllowedExtensions or ExtensionGalleryServiceUrl, +// machine-wide or for any user. The plists are typically binary, but plist key +// names are stored as plain ASCII runs in both binary and XML encodings, so a +// byte scan is a reliable presence check without a plist parser dependency. func ProbeManagedPolicy() (bool, string) { return probeDarwinManagedPrefs(darwinManagedPrefsDir) } @@ -29,8 +29,10 @@ func probeDarwinManagedPrefs(root string) (bool, string) { perUser, _ := filepath.Glob(filepath.Join(root, "*", darwinVSCodePlistName)) candidates = append(candidates, perUser...) for _, p := range candidates { - if fileMentionsKey(p, allowedExtensionsName) { - return true, p + " [" + allowedExtensionsName + "]" + for _, name := range managedPolicyNames() { + if fileMentionsKey(p, name) { + return true, p + " [" + name + "]" + } } } return false, "" diff --git a/internal/devicepolicy/probe_darwin_test.go b/internal/devicepolicy/probe_darwin_test.go new file mode 100644 index 00000000..3cc09736 --- /dev/null +++ b/internal/devicepolicy/probe_darwin_test.go @@ -0,0 +1,61 @@ +//go:build darwin + +package devicepolicy + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestProbeDarwinEitherKey stages a fake managed-preferences plist and confirms +// the probe yields on AllowedExtensions OR ExtensionGalleryServiceUrl (the key +// names appear as plain ASCII runs, matching real binary plists). +func TestProbeDarwinEitherKey(t *testing.T) { + cases := []struct { + name string + content string + want bool + detail string + }{ + {"allowlist only", "bplist00\x00AllowedExtensions\x00", true, allowedExtensionsName}, + {"gallery only", "bplist00\x00ExtensionGalleryServiceUrl\x00", true, galleryServiceURLName}, + {"both (allowlist preferred in detail)", "bplist00\x00AllowedExtensions\x00ExtensionGalleryServiceUrl\x00", true, allowedExtensionsName}, + {"neither", "bplist00\x00SomethingElse\x00", false, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + p := filepath.Join(root, darwinVSCodePlistName) + if err := os.WriteFile(p, []byte(tc.content), 0o644); err != nil { + t.Fatal(err) + } + managed, detail := probeDarwinManagedPrefs(root) + if managed != tc.want { + t.Fatalf("managed = %v, want %v (detail %q)", managed, tc.want, detail) + } + if tc.want && !strings.Contains(detail, tc.detail) { + t.Fatalf("detail = %q, want to contain %q", detail, tc.detail) + } + }) + } +} + +// TestProbeDarwinPerUserGalleryKey confirms the per-user glob path also detects +// the gallery key (an MDM may materialize the plist under a subdir). +func TestProbeDarwinPerUserGalleryKey(t *testing.T) { + root := t.TempDir() + userDir := filepath.Join(root, "alice") + if err := os.MkdirAll(userDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(userDir, darwinVSCodePlistName), + []byte("bplist00\x00ExtensionGalleryServiceUrl\x00"), 0o644); err != nil { + t.Fatal(err) + } + managed, detail := probeDarwinManagedPrefs(root) + if !managed || !strings.Contains(detail, galleryServiceURLName) { + t.Fatalf("per-user gallery key: want managed=true with gallery detail, got (%v, %q)", managed, detail) + } +} diff --git a/internal/devicepolicy/probe_linux.go b/internal/devicepolicy/probe_linux.go index 23be6332..17710ee7 100644 --- a/internal/devicepolicy/probe_linux.go +++ b/internal/devicepolicy/probe_linux.go @@ -7,11 +7,19 @@ package devicepolicy // needs no elevation. const linuxPolicyFilePath = "/etc/vscode/policy.json" -// ProbeManagedPolicy reports whether an AllowedExtensions policy exists in the -// Linux policy file. +// ProbeManagedPolicy reports whether an AllowedExtensions or +// ExtensionGalleryServiceUrl policy exists in the Linux policy file. func ProbeManagedPolicy() (bool, string) { - if jsonFileHasKey(linuxPolicyFilePath, allowedExtensionsName) { - return true, linuxPolicyFilePath + " [" + allowedExtensionsName + "]" + return probeLinuxPolicyFile(linuxPolicyFilePath) +} + +// probeLinuxPolicyFile is ProbeManagedPolicy parameterized over the policy-file +// path so tests can stage a fixture instead of touching /etc. +func probeLinuxPolicyFile(path string) (bool, string) { + for _, name := range managedPolicyNames() { + if jsonFileHasKey(path, name) { + return true, path + " [" + name + "]" + } } return false, "" } diff --git a/internal/devicepolicy/probe_linux_test.go b/internal/devicepolicy/probe_linux_test.go new file mode 100644 index 00000000..a09f869c --- /dev/null +++ b/internal/devicepolicy/probe_linux_test.go @@ -0,0 +1,44 @@ +//go:build linux + +package devicepolicy + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestProbeLinuxEitherKey stages a fake /etc/vscode/policy.json and confirms +// the probe yields on AllowedExtensions OR ExtensionGalleryServiceUrl. +func TestProbeLinuxEitherKey(t *testing.T) { + cases := []struct { + name string + content string // empty → no file + want bool + detail string + }{ + {"allowlist only", `{"AllowedExtensions":{"*":false}}`, true, allowedExtensionsName}, + {"gallery only", `{"ExtensionGalleryServiceUrl":"https://mkt.example/api/v1"}`, true, galleryServiceURLName}, + {"both (allowlist preferred)", `{"AllowedExtensions":{},"ExtensionGalleryServiceUrl":"x"}`, true, allowedExtensionsName}, + {"neither", `{"Other":1}`, false, ""}, + {"missing file", "", false, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "policy.json") + if tc.content != "" { + if err := os.WriteFile(path, []byte(tc.content), 0o644); err != nil { + t.Fatal(err) + } + } + managed, detail := probeLinuxPolicyFile(path) + if managed != tc.want { + t.Fatalf("managed = %v, want %v (detail %q)", managed, tc.want, detail) + } + if tc.want && !strings.Contains(detail, tc.detail) { + t.Fatalf("detail = %q, want to contain %q", detail, tc.detail) + } + }) + } +} diff --git a/internal/devicepolicy/probe_test.go b/internal/devicepolicy/probe_test.go index a7a3d5ff..d839158e 100644 --- a/internal/devicepolicy/probe_test.go +++ b/internal/devicepolicy/probe_test.go @@ -59,3 +59,37 @@ func TestFileMentionsKey(t *testing.T) { t.Error("missing file: want false") } } + +// TestProbeHelpersDetectEitherPolicyName exercises the shared probe helpers +// against the gallery policy name (the per-OS ProbeManagedPolicy loops over +// managedPolicyNames, so proving both names are detectable + the name set is +// the linux/darwin either-key coverage that runs on any OS). +func TestProbeHelpersDetectEitherPolicyName(t *testing.T) { + dir := t.TempDir() + + // jsonFileHasKey (linux policy.json shape) finds the gallery policy name. + p := filepath.Join(dir, "policy.json") + if err := os.WriteFile(p, []byte(`{"ExtensionGalleryServiceUrl":"https://mkt.example/api/v1"}`), 0o644); err != nil { + t.Fatal(err) + } + if !jsonFileHasKey(p, galleryServiceURLName) { + t.Error("jsonFileHasKey must detect ExtensionGalleryServiceUrl") + } + if jsonFileHasKey(p, allowedExtensionsName) { + t.Error("AllowedExtensions absent → false") + } + + // fileMentionsKey (darwin plist byte scan) finds it too. + plist := filepath.Join(dir, "vscode.plist") + if err := os.WriteFile(plist, append([]byte("bplist00\x00"), []byte("ExtensionGalleryServiceUrl\x00")...), 0o644); err != nil { + t.Fatal(err) + } + if !fileMentionsKey(plist, galleryServiceURLName) { + t.Error("fileMentionsKey must detect ExtensionGalleryServiceUrl") + } + + // managedPolicyNames covers both, allowlist first (reporting preference). + if names := managedPolicyNames(); len(names) != 2 || names[0] != allowedExtensionsName || names[1] != galleryServiceURLName { + t.Fatalf("managedPolicyNames = %v", names) + } +} diff --git a/internal/devicepolicy/probe_windows.go b/internal/devicepolicy/probe_windows.go index 51bd8609..5a11626f 100644 --- a/internal/devicepolicy/probe_windows.go +++ b/internal/devicepolicy/probe_windows.go @@ -25,10 +25,11 @@ type registryProbe struct { path string } -// ProbeManagedPolicy reports whether an AllowedExtensions value of ANY -// registry type exists under the VS Code policy key in HKLM or HKCU. Type -// does not matter: VS Code's policy service claims the setting as soon as the -// value exists, so a wrong-typed value still outranks user settings. +// ProbeManagedPolicy reports whether an AllowedExtensions or +// ExtensionGalleryServiceUrl value of ANY registry type exists under the VS +// Code policy key in HKLM or HKCU. Type does not matter: VS Code's policy +// service claims the setting as soon as the value exists, so a wrong-typed +// value still outranks user settings. func ProbeManagedPolicy() (bool, string) { return probeRegistryLocations([]registryProbe{ {registry.LOCAL_MACHINE, "HKLM", windowsPolicyKeyPath}, @@ -59,11 +60,14 @@ func probeRegistry(loc registryProbe) (bool, string) { // GetValue with a nil buffer asks only for existence/metadata. Any error // other than ErrNotExist still proves the value exists or the key is - // unreadable; only a clean not-exists reads as unmanaged. - if _, _, err := k.GetValue(allowedExtensionsName, nil); err != nil { - if errors.Is(err, registry.ErrNotExist) { - return false, "" + // unreadable; only a clean not-exists reads as unmanaged. Check each managed + // policy name; the first present one wins (AllowedExtensions preferred in + // the detail). + for _, name := range managedPolicyNames() { + if _, _, err := k.GetValue(name, nil); errors.Is(err, registry.ErrNotExist) { + continue } + return true, loc.name + `\` + loc.path + ` [` + name + `]` } - return true, loc.name + `\` + loc.path + ` [` + allowedExtensionsName + `]` + return false, "" } diff --git a/internal/devicepolicy/probe_windows_test.go b/internal/devicepolicy/probe_windows_test.go index f41c261b..4b41d873 100644 --- a/internal/devicepolicy/probe_windows_test.go +++ b/internal/devicepolicy/probe_windows_test.go @@ -65,6 +65,19 @@ func TestProbeRegistry(t *testing.T) { if managed, _ := probeRegistry(hkcuProbe("HKCU", testPolicyKeyPath)); !managed { t.Fatal("dword value present: want managed=true") } + + // Gallery-only: remove AllowedExtensions, set ExtensionGalleryServiceUrl → + // still managed (either key yields), reported under the gallery name. + if err := k.DeleteValue(allowedExtensionsName); err != nil { + t.Fatal(err) + } + if err := k.SetStringValue(galleryServiceURLName, "https://mkt.example/api/v1"); err != nil { + t.Fatal(err) + } + managed, detail = probeRegistry(hkcuProbe("HKCU", testPolicyKeyPath)) + if !managed || !strings.Contains(detail, galleryServiceURLName) { + t.Fatalf("gallery-only: want managed=true with gallery detail, got (%v, %q)", managed, detail) + } } func TestProbeRegistryLocationsFallsBackToSecond(t *testing.T) { diff --git a/internal/devicepolicy/reconcile.go b/internal/devicepolicy/reconcile.go index 04a179f2..2cd0e17b 100644 --- a/internal/devicepolicy/reconcile.go +++ b/internal/devicepolicy/reconcile.go @@ -1,7 +1,9 @@ package devicepolicy import ( + "bytes" "context" + "encoding/json" "errors" "fmt" "time" @@ -138,13 +140,24 @@ func (r *Reconciler) Reconcile(ctx context.Context) error { return r.handleEnforce(ctx, cat, tgt, ep) } -// handleClear removes the agent-owned settings key on unassignment. It clears -// the on-disk value ONLY when it still equals what the agent last wrote -// (ownership); a value the agent has no record of writing — the user's own -// extensions.allowed predates enforcement, or the record was lost — is left -// intact. +// handleClear removes the agent-owned settings on unassignment, then drops the +// ownership record. It dispatches on the Writer: a managed multi-key writer +// clears each owned key independently (clearManaged); any other Writer keeps +// the single-key path (clearSingle). Both are ownership-gated: a value the +// agent has no record of writing is left intact. func (r *Reconciler) handleClear(cat, tgt string) error { prev, hadPrev := ReadAppliedState(cat, tgt) + if mw, ok := r.Writer.(managedSettingsWriter); ok { + return r.clearManaged(cat, tgt, prev, hadPrev, mw) + } + return r.clearSingle(cat, tgt, prev, hadPrev) +} + +// clearSingle is the single-key unassignment path. It clears the on-disk value +// ONLY when it still equals what the agent last wrote (ownership); a value the +// agent has no record of writing — the user's own extensions.allowed predates +// enforcement, or the record was lost — is left intact. +func (r *Reconciler) clearSingle(cat, tgt string, prev AppliedTargetState, hadPrev bool) error { onDisk, present, err := r.Writer.Read() if err != nil { return fmt.Errorf("devicepolicy: clear: read %s: %w", r.Writer.Location(), err) @@ -162,10 +175,44 @@ func (r *Reconciler) handleClear(cat, tgt string) error { r.logf("devicepolicy: clear requested but %s holds a value the agent did not write; leaving it", r.Writer.Location()) } - // Drop our ownership record whenever we hold an entry for this category. - // Beyond the obvious case (we owned a value), this also reclaims an empty - // record a preflight may have left after its settings write later failed. - // An absent entry → no-op (idempotent). + return r.dropClearedState(cat, tgt, hadPrev) +} + +// clearManaged is the managed multi-key unassignment path. It removes each +// managed key INDEPENDENTLY, and only when its on-disk value still equals what +// the agent wrote (per-key ownership); a foreign-valued or absent key is +// preserved. One atomic write carries only the owned-key removes. +func (r *Reconciler) clearManaged(cat, tgt string, prev AppliedTargetState, hadPrev bool, mw managedSettingsWriter) error { + keys := managedKeys() + cur, err := mw.ReadManaged(keys) + if err != nil { + return fmt.Errorf("devicepolicy: clear: read %s: %w", r.Writer.Location(), err) + } + owned := ownedKeys(prev, hadPrev) + var ops []settingOp + for _, key := range keys { // fixed order → deterministic write + ov := owned[key] + if ov != "" && cur[key].Present && cur[key].Raw == ov { + ops = append(ops, settingOp{Key: key, Remove: true}) + } + } + if len(ops) > 0 { + if _, err := mw.ApplyManaged(ops); err != nil { + return fmt.Errorf("devicepolicy: clear %s: %w", r.Writer.Location(), err) + } + r.logf("devicepolicy: cleared %d agent-owned key(s) at %s", len(ops), r.Writer.Location()) + } else { + r.logf("devicepolicy: clear requested but %s holds no agent-owned value; leaving it", r.Writer.Location()) + } + + return r.dropClearedState(cat, tgt, hadPrev) +} + +// dropClearedState drops the ownership record whenever an entry exists for this +// (category, target). Beyond the obvious case (we owned a value), this reclaims +// an empty record a preflight may have left after its settings write later +// failed. An absent entry → no-op (idempotent). +func (r *Reconciler) dropClearedState(cat, tgt string, hadPrev bool) error { if hadPrev { if err := r.dropState(cat, tgt); err != nil { return fmt.Errorf("devicepolicy: clear: update state: %w", err) @@ -174,19 +221,22 @@ func (r *Reconciler) handleClear(cat, tgt string) error { return nil } -// handleEnforce converges settings.json to the compiled policy and reports. -// The ladder, in order: +// handleEnforce converges settings.json to the compiled policy and reports. It +// runs the shared head of the ladder (compact the allowlist, then the +// managed-policy probe), then dispatches on the Writer: a managed multi-key +// writer converges the full SET of managed keys (enforceManaged); any other +// Writer keeps the single-key path (enforceSingle). // // probe (managed policy exists → mdm_managed, never write) -// → read current value -// → idempotency (hash unchanged ∧ on-disk converged → report, no write) +// → read current value(s) +// → idempotency (hash unchanged ∧ every managed key converged → report, no write) // → preflight ownership-store writability -// → drift detection (on-disk diverged from the recorded written value) +// → drift detection (an OWNED key diverged from its recorded written value) // → merge-write + readback // → persist ownership on every successful write (rollback if that fails) // → Verify → report (drift upgrades a would-be compliant to drift_detected) func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep EffectivePolicy) error { - // The compiled policy compacted: the canonical comparison form for + // The compiled allowlist compacted: the canonical comparison form for // readback, idempotency, and ownership. (The backend's hash still travels // verbatim; only the value bytes are normalized for comparison.) newValue, err := compactJSON(ep.Policy) @@ -196,15 +246,26 @@ func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep Effe return fmt.Errorf("devicepolicy: enforce: compact policy: %w", err) } - // 1. Managed-policy probe. A policy at the OS policy location outranks - // user settings inside VS Code — writing would be ineffective at best and - // fight the MDM at worst. Yield and report. + // Managed-policy probe. A policy at the OS policy location outranks user + // settings inside VS Code — writing would be ineffective at best and fight + // the MDM at worst. Yield and report. (Presence of EITHER managed policy + // key yields; see the probe.) if managed, detail := r.probe(); managed { r.logf("devicepolicy: managed policy present at %s → mdm_managed (yielding)", detail) return r.report(ctx, cat, tgt, StateMDMManaged, "") } - // 2. Read the current settings value. + if mw, ok := r.Writer.(managedSettingsWriter); ok { + return r.enforceManaged(ctx, cat, tgt, ep, newValue, mw) + } + return r.enforceSingle(ctx, cat, tgt, ep, newValue) +} + +// enforceSingle is the single-key convergence path (any Writer without the +// managed multi-key API). It is unchanged from the original enforce: it manages +// exactly the extensions.allowed key. +func (r *Reconciler) enforceSingle(ctx context.Context, cat, tgt string, ep EffectivePolicy, newValue string) error { + // Read the current settings value. prev, hadPrev := ReadAppliedState(cat, tgt) onDisk, present, err := r.Writer.Read() if err != nil { @@ -215,25 +276,25 @@ func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep Effe return fmt.Errorf("devicepolicy: enforce: read %s: %w", r.Writer.Location(), err) } - // 3. Idempotency: the desired policy is already in place and unchanged. - // No write — but still report so the backend sees a fresh evaluation. + // Idempotency: the desired policy is already in place and unchanged. No + // write — but still report so the backend sees a fresh evaluation. if present && onDisk == newValue && prev.AppliedHash == ep.Hash { r.logf("devicepolicy: policy already applied (hash unchanged) — no write") return r.report(ctx, cat, tgt, StateCompliant, ep.Hash) } - // 4. Drift: the agent wrote a value before, and what is on disk now is not - // it (edited or removed — typically the user hand-editing settings.json). - // Enforcement means converging it back; the distinct state lets the - // backend surface that it happened. + // Drift: the agent wrote a value before, and what is on disk now is not it + // (edited or removed — typically the user hand-editing settings.json). + // Enforcement means converging it back; the distinct state lets the backend + // surface that it happened. drifted := hadPrev && prev.WrittenValue != "" && (!present || onDisk != prev.WrittenValue) if drifted { r.logf("devicepolicy: %s diverged from the recorded written value → re-applying (drift)", r.Writer.Location()) } - // 5. Preflight: prove the ownership store is writable BEFORE mutating the - // settings file. An enforced value with no ownership record is orphaned — - // a later clear refuses to remove it. Re-persisting the current state is a + // Preflight: prove the ownership store is writable BEFORE mutating the + // settings file. An enforced value with no ownership record is orphaned — a + // later clear refuses to remove it. Re-persisting the current state is a // meaning-preserving writability probe. probe := prev if !hadPrev { @@ -244,7 +305,7 @@ func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep Effe return fmt.Errorf("devicepolicy: enforce: ownership state not writable, refusing to write policy: %w", perr) } - // 6. Merge-write + readback. + // Merge-write + readback. rb, werr := r.Writer.Write(newValue) if werr != nil { _ = r.report(ctx, cat, tgt, StateWriteFailed, "") @@ -252,19 +313,18 @@ func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep Effe } readbackMatch := rb == newValue - // 7. Ownership is recorded on EVERY successful write — it means "what the - // agent wrote", not "what it verified". On a readback mismatch the write - // may still have landed; without a record the next cycle would classify - // the agent's own value as drift forever. Value-based ownership - // self-corrects: the record only takes effect when the on-disk value - // actually equals it. + // Ownership is recorded on EVERY successful write — it means "what the agent + // wrote", not "what it verified". On a readback mismatch the write may still + // have landed; without a record the next cycle would classify the agent's + // own value as drift forever. Value-based ownership self-corrects: the + // record only takes effect when the on-disk value actually equals it. if err := r.persistState(cat, tgt, AppliedTargetState{ AppliedHash: ep.Hash, WrittenValue: newValue, FetchedAt: r.now(), }); err != nil { - // The write happened but ownership couldn't be recorded — undo it so - // no unrecorded value is left behind, and report a failed write. + // The write happened but ownership couldn't be recorded — undo it so no + // unrecorded value is left behind, and report a failed write. r.rollbackWrite(onDisk, present) _ = r.report(ctx, cat, tgt, StateWriteFailed, "") return fmt.Errorf("devicepolicy: enforce: update state: %w", err) @@ -288,6 +348,229 @@ func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep Effe return r.report(ctx, cat, tgt, state, appliedHash) } +// enforceManaged is the managed multi-key convergence path. The allowlist is +// always authoritative (Set); the gallery URL is Set when the policy carries +// one, else an ownership-gated Remove (only a value the agent itself wrote), +// else preserved (a foreign or absent value is never deleted). +func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep EffectivePolicy, newValue string, mw managedSettingsWriter) error { + prev, hadPrev := ReadAppliedState(cat, tgt) + owned := ownedKeys(prev, hadPrev) + + // 1. Read the managed keys up front. + keys := managedKeys() + cur, err := mw.ReadManaged(keys) + if err != nil { + _ = r.report(ctx, cat, tgt, StateVerificationFailed, "") + return fmt.Errorf("devicepolicy: enforce: read %s: %w", r.Writer.Location(), err) + } + + // 2. Build the desired end-state ops. Allowlist is ALWAYS Set. The gallery + // op is decided from the policy URL and per-key ownership. + galValue, err := managedGalleryValue(ep.GalleryServiceURL) + if err != nil { + // Defensive: encoding a validated URL string cannot fail in practice. + // If it ever did, nothing has been written — report verification_failed + // symmetrically with the ReadManaged-error path above. + _ = r.report(ctx, cat, tgt, StateVerificationFailed, "") + return fmt.Errorf("devicepolicy: enforce: encode gallery url: %w", err) + } + desired := []settingOp{{Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(newValue)}} + + galKey := galleryServiceURLSettingKey + var galleryOp settingOp + switch { + case ep.GalleryServiceURL != "": + // Authoritative: set the gallery key to the policy's URL. + galleryOp = settingOp{Key: galKey, Set: true, Value: json.RawMessage(galValue)} + case owned[galKey] != "" && cur[galKey].Present && cur[galKey].Raw == owned[galKey]: + // No URL and the on-disk value is the one the agent wrote → remove it + // (ownership-gated). + galleryOp = settingOp{Key: galKey, Remove: true} + default: + // No URL and either no ownership record or a foreign value on disk → + // preserve (never delete a value the agent did not write). + galleryOp = settingOp{Key: galKey} + } + desired = append(desired, galleryOp) + + // 3. Convergence over the FULL desired end-state, computed BEFORE the + // idempotency short-circuit. It covers every managed key, so gallery-only + // drift with an unchanged hash re-applies rather than short-circuiting to + // compliant. + converged := true + for _, op := range desired { + if !opConverged(op, cur) { + converged = false + break + } + } + if converged && prev.AppliedHash == ep.Hash { + r.logf("devicepolicy: policy already applied (hash unchanged) — no write") + return r.report(ctx, cat, tgt, StateCompliant, ep.Hash) + } + + // 4. Drift: an OWNED key diverged from its recorded written value (edited or + // removed). Only owned keys count — a foreign value is not the agent's drift. + drifted := false + for key, ov := range owned { + if !cur[key].Present || cur[key].Raw != ov { + drifted = true + break + } + } + if drifted { + r.logf("devicepolicy: %s diverged from a recorded written value → re-applying (drift)", r.Writer.Location()) + } + + // 5. Snapshot the full pre-write state for an atomic multi-key rollback. + snapshot := make(map[string]settingValue, len(cur)) + for k, sv := range cur { + snapshot[k] = sv + } + + // 6. Preflight: prove the ownership store is writable BEFORE mutating the + // settings file (same rationale as the single-key path). + probe := prev + if !hadPrev { + probe = AppliedTargetState{FetchedAt: r.now()} + } + if perr := r.persistState(cat, tgt, probe); perr != nil { + _ = r.report(ctx, cat, tgt, StateWriteFailed, "") + return fmt.Errorf("devicepolicy: enforce: ownership state not writable, refusing to write policy: %w", perr) + } + + // 7. Write the mutating ops in one atomic patch; a preserve contributes + // nothing. + writeOps := make([]settingOp, 0, len(desired)) + for _, op := range desired { + if op.Set || op.Remove { + writeOps = append(writeOps, op) + } + } + readback, werr := mw.ApplyManaged(writeOps) + if werr != nil { + _ = r.report(ctx, cat, tgt, StateWriteFailed, "") + return fmt.Errorf("devicepolicy: enforce: write %s: %w", r.Writer.Location(), werr) + } + + // 8. Readback over every mutating op — value + presence prove the requested + // mutation (a bare string cannot distinguish absent from present-empty). + readbackMatch := true + for _, op := range writeOps { + if !opConverged(op, readback) { + readbackMatch = false + break + } + } + + // 9. Persist ownership: what the agent now OWNS. Allowlist → WrittenValue; + // each extra Set key → WrittenSettings; a Remove or preserve key asserts no + // ownership this cycle (omitted). + ownedAfter := map[string]string{} + if galleryOp.Set { + ownedAfter[galKey] = galValue + } + if len(ownedAfter) == 0 { + ownedAfter = nil // keep an allowlist-only record byte-identical to before + } + if err := r.persistState(cat, tgt, AppliedTargetState{ + AppliedHash: ep.Hash, + WrittenValue: newValue, + WrittenSettings: ownedAfter, + FetchedAt: r.now(), + }); err != nil { + // The write happened but ownership couldn't be recorded — roll back ALL + // keys atomically so no unrecorded value is left behind. A clean undo → + // write_failed; a failed restore leaves the on-disk state uncertain → + // verification_failed. + if rerr := mw.RestoreManaged(snapshot); rerr != nil { + r.logf("devicepolicy: rollback at %s failed: %v", r.Writer.Location(), rerr) + _ = r.report(ctx, cat, tgt, StateVerificationFailed, "") + return fmt.Errorf("devicepolicy: enforce: update state (rollback failed: %v): %w", rerr, err) + } + _ = r.report(ctx, cat, tgt, StateWriteFailed, "") + return fmt.Errorf("devicepolicy: enforce: update state: %w", err) + } + r.logf("devicepolicy: wrote policy to %s (readback_match=%v)", r.Writer.Location(), readbackMatch) + + state := Verify(VerifyInput{WriteOK: true, ReadbackMatch: readbackMatch}) + if drifted && state == StateCompliant { + state = StateDriftDetected + } + + // applied_hash is echoed only when readback-confirmed (compliant or + // drift_detected). It is the backend's hash verbatim — never recomputed — so + // the backend's byte-exact applied==desired check gates `compliant`. + appliedHash := "" + if state == StateCompliant || state == StateDriftDetected { + appliedHash = ep.Hash + } + return r.report(ctx, cat, tgt, state, appliedHash) +} + +// managedKeys is the fixed, ordered set of VS Code settings.json keys the IDE +// reconciler manages. The order is stable so reads, convergence, and writes are +// deterministic. +func managedKeys() []string { + return []string{allowedExtensionsSettingKey, galleryServiceURLSettingKey} +} + +// opConverged reports whether on-disk state m already satisfies op: a Set +// converges when the key is present with the exact value; a Remove when the key +// is absent; a preserve (neither Set nor Remove) is always satisfied. +func opConverged(op settingOp, m map[string]settingValue) bool { + sv := m[op.Key] + switch { + case op.Set: + return sv.Present && sv.Raw == string(op.Value) + case op.Remove: + return !sv.Present + default: + return true + } +} + +// ownedKeys folds an ownership record into a flat map of setting id → the exact +// value the agent last wrote, skipping empty entries. The allowlist comes from +// WrittenValue, the other managed keys from WrittenSettings. Drift detection and +// ownership-gated removal act only on keys the agent actually wrote. +func ownedKeys(prev AppliedTargetState, hadPrev bool) map[string]string { + owned := map[string]string{} + if !hadPrev { + return owned + } + if prev.WrittenValue != "" { + owned[allowedExtensionsSettingKey] = prev.WrittenValue + } + for k, v := range prev.WrittenSettings { + if v != "" { + owned[k] = v + } + } + return owned +} + +// managedGalleryValue returns the compacted JSON-string encoding of a gallery +// URL — the exact bytes written to settings.json AND recorded as owned, so the +// readback and the next-cycle ownership comparison are canonical against what +// ReadManaged returns. HTML escaping is disabled so the value round-trips +// byte-stably (json.Compact does not escape, and the JSONC writer preserves the +// literal string token). An empty URL yields "" (no value). +func managedGalleryValue(url string) (string, error) { + if url == "" { + return "", nil + } + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + enc.SetEscapeHTML(false) + if err := enc.Encode(url); err != nil { + return "", fmt.Errorf("devicepolicy: encode gallery url: %w", err) + } + // Encode appends a newline; compact strips it (a JSON string has no other + // insignificant whitespace). + return compactJSON(buf.Bytes()) +} + // rollbackWrite restores the settings key to its pre-cycle condition after the // post-write ownership persist failed. WriteAppliedState is atomic // (temp+rename), so the failed persist left the previous state file intact — diff --git a/internal/devicepolicy/reconcile_test.go b/internal/devicepolicy/reconcile_test.go index 662c2b41..b2000e82 100644 --- a/internal/devicepolicy/reconcile_test.go +++ b/internal/devicepolicy/reconcile_test.go @@ -645,7 +645,510 @@ func TestReconcilePreservesSiblingTargetOwnership(t *testing.T) { t.Fatalf("vscode ownership not recorded: got %+v ok=%v", vs, ok) } // jetbrains sibling untouched. - if got, ok := ReadAppliedState(CategoryIDEExtension, "jetbrains"); !ok || got != jb { + if got, ok := ReadAppliedState(CategoryIDEExtension, "jetbrains"); !ok || got.AppliedHash != jb.AppliedHash || got.WrittenValue != jb.WrittenValue { t.Fatalf("sibling jetbrains ownership must survive a vscode reconcile: got %+v ok=%v", got, ok) } } + +// --- managed multi-key path (gallery URL) ---------------------------------- + +const ( + galURLA = "https://mkt.example/api/v1" + galURLB = "https://other.example/api/v1" +) + +// galleryRaw is the compacted JSON-string form of a URL as it lands in +// settings.json and is recorded as owned (test URLs have no chars needing +// escaping, so this equals managedGalleryValue's output). +func galleryRaw(url string) string { return `"` + url + `"` } + +// fakeManagedWriter implements Writer AND managedSettingsWriter over an +// in-memory key→state map, so the reconciler drives the managed multi-key path. +type fakeManagedWriter struct { + state map[string]settingValue + readErr error + applyErr error + restoreErr error + // readbackOverride forces ApplyManaged's returned readback for a key + // (simulates a silent non-apply) without changing stored state. + readbackOverride map[string]settingValue + applies [][]settingOp + restores []map[string]settingValue + reads int +} + +func (w *fakeManagedWriter) ensure() { + if w.state == nil { + w.state = map[string]settingValue{} + } +} + +func (w *fakeManagedWriter) ReadManaged(keys []string) (map[string]settingValue, error) { + w.reads++ + if w.readErr != nil { + return nil, w.readErr + } + w.ensure() + out := make(map[string]settingValue, len(keys)) + for _, k := range keys { + out[k] = w.state[k] // zero settingValue when absent + } + return out, nil +} + +func (w *fakeManagedWriter) ApplyManaged(ops []settingOp) (map[string]settingValue, error) { + w.applies = append(w.applies, append([]settingOp(nil), ops...)) + if w.applyErr != nil { + return nil, w.applyErr + } + w.ensure() + for _, op := range ops { + switch { + case op.Set: + w.state[op.Key] = settingValue{Present: true, Raw: string(op.Value)} + case op.Remove: + delete(w.state, op.Key) + } + } + out := make(map[string]settingValue, len(ops)) + for _, op := range ops { + if ov, ok := w.readbackOverride[op.Key]; ok { + out[op.Key] = ov + continue + } + out[op.Key] = w.state[op.Key] + } + return out, nil +} + +func (w *fakeManagedWriter) RestoreManaged(snapshot map[string]settingValue) error { + cp := make(map[string]settingValue, len(snapshot)) + for k, v := range snapshot { + cp[k] = v + } + w.restores = append(w.restores, cp) + if w.restoreErr != nil { + return w.restoreErr + } + w.ensure() + for k, sv := range snapshot { + if sv.Present { + w.state[k] = sv + } else { + delete(w.state, k) + } + } + return nil +} + +// Writer conformance — the managed path never calls these, but r.Writer is +// typed Writer so they must exist. They operate on the allowlist key so they +// stay coherent if ever exercised. +func (w *fakeManagedWriter) Read() (string, bool, error) { + w.ensure() + sv := w.state[allowedExtensionsSettingKey] + return sv.Raw, sv.Present, w.readErr +} +func (w *fakeManagedWriter) Write(v string) (string, error) { + if w.applyErr != nil { + return "", w.applyErr + } + w.ensure() + w.state[allowedExtensionsSettingKey] = settingValue{Present: true, Raw: v} + return v, nil +} +func (w *fakeManagedWriter) Clear() error { + w.ensure() + delete(w.state, allowedExtensionsSettingKey) + return nil +} +func (w *fakeManagedWriter) Location() string { return "fake://managed-settings.json" } + +func policyEPGallery(hash, url string) EffectivePolicy { + ep := policyEP(hash) + ep.GalleryServiceURL = url + return ep +} + +func newManagedRec(t *testing.T, ep EffectivePolicy, w *fakeManagedWriter) (*Reconciler, *fakeReporter) { + t.Helper() + withTempCache(t) + rep := &fakeReporter{} + r := &Reconciler{ + Fetcher: &fakeFetcher{ep: ep}, + Reporter: rep, + Writer: w, + CustomerID: "cust", + DeviceID: "dev-1", + Platform: "linux", + Probe: func() (bool, string) { return false, "" }, + Now: func() time.Time { return time.Date(2026, 7, 23, 0, 0, 0, 0, time.UTC) }, + } + return r, rep +} + +// lastApply returns the single op set passed to ApplyManaged, failing if it was +// not called exactly once. +func lastApply(t *testing.T, w *fakeManagedWriter) []settingOp { + t.Helper() + if len(w.applies) != 1 { + t.Fatalf("expected exactly 1 ApplyManaged call, got %d: %v", len(w.applies), w.applies) + } + return w.applies[0] +} + +func opFor(ops []settingOp, key string) (settingOp, bool) { + for _, op := range ops { + if op.Key == key { + return op, true + } + } + return settingOp{}, false +} + +func TestEnforceManagedFirstEnforceWritesBothKeys(t *testing.T) { + w := &fakeManagedWriter{} + r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + ops := lastApply(t, w) + al, ok := opFor(ops, allowedExtensionsSettingKey) + if !ok || !al.Set || string(al.Value) != samplePolicy { + t.Fatalf("allowlist op = %+v, want Set to %s", al, samplePolicy) + } + gal, ok := opFor(ops, galleryServiceURLSettingKey) + if !ok || !gal.Set || string(gal.Value) != galleryRaw(galURLA) { + t.Fatalf("gallery op = %+v, want Set to %s", gal, galleryRaw(galURLA)) + } + got := lastReport(t, rep) + if got.State != StateCompliant || got.AppliedHash != "sha256:H" { + t.Fatalf("report = %+v, want compliant + echoed hash", got) + } + st, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode) + if !ok || st.WrittenValue != samplePolicy || st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { + t.Fatalf("ownership = %+v ok=%v, want allowlist + gallery owned", st, ok) + } +} + +func TestEnforceManagedIdempotentNoRewrite(t *testing.T) { + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw(galURLA)}, + }} + r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:H", WrittenValue: samplePolicy, + WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if len(w.applies) != 0 { + t.Fatalf("idempotent run must not write, got applies=%v", w.applies) + } + if got := lastReport(t, rep); got.State != StateCompliant || got.AppliedHash != "sha256:H" { + t.Fatalf("report = %+v, want compliant + echoed hash", got) + } +} + +func TestEnforceManagedGalleryOnlyDriftReapplies(t *testing.T) { + // The allowlist is untouched but the user edited the gallery key. Because + // convergence covers BOTH keys before the idempotency short-circuit, this is + // caught and re-applied even though the hash is unchanged. + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw("https://tampered.example/api/v1")}, + }} + r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:H", WrittenValue: samplePolicy, + WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + ops := lastApply(t, w) + if gal, ok := opFor(ops, galleryServiceURLSettingKey); !ok || !gal.Set || string(gal.Value) != galleryRaw(galURLA) { + t.Fatalf("gallery must be re-applied to %s, got %+v", galleryRaw(galURLA), gal) + } + if got := lastReport(t, rep); got.State != StateDriftDetected || got.AppliedHash != "sha256:H" { + t.Fatalf("report = %+v, want drift_detected + echoed hash", got) + } + // Second cycle: converged, hash unchanged → plain compliant, no rewrite. + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("second Reconcile: %v", err) + } + if len(w.applies) != 1 { + t.Fatalf("second cycle must be idempotent, applies=%v", w.applies) + } + if rep.reports[1].State != StateCompliant { + t.Fatalf("second cycle state = %q, want compliant", rep.reports[1].State) + } +} + +func TestEnforceManagedAllowlistDriftReapplies(t *testing.T) { + // No gallery URL in the policy; the user tampered the allowlist. Drift over + // the owned allowlist key re-applies it; the untracked gallery is preserved. + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: `{"user.tampered":true}`}, + }} + r, rep := newManagedRec(t, policyEP("sha256:H"), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:H", WrittenValue: samplePolicy, + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + ops := lastApply(t, w) + if _, ok := opFor(ops, galleryServiceURLSettingKey); ok { + t.Fatalf("no URL and no gallery ownership → gallery must not be written, ops=%v", ops) + } + if al, ok := opFor(ops, allowedExtensionsSettingKey); !ok || !al.Set || string(al.Value) != samplePolicy { + t.Fatalf("allowlist must be re-applied, got %+v", al) + } + if got := lastReport(t, rep); got.State != StateDriftDetected { + t.Fatalf("state = %q, want drift_detected", got.State) + } +} + +func TestEnforceManagedURLAdded(t *testing.T) { + // Previously allowlist-only; the policy now carries a URL (new hash). This is + // a policy change (not drift): plain compliant, gallery now owned. + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + }} + r, rep := newManagedRec(t, policyEPGallery("sha256:NEW", galURLA), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if gal, ok := opFor(lastApply(t, w), galleryServiceURLSettingKey); !ok || !gal.Set || string(gal.Value) != galleryRaw(galURLA) { + t.Fatalf("gallery must be set to %s, got %+v", galleryRaw(galURLA), gal) + } + if got := lastReport(t, rep); got.State != StateCompliant || got.AppliedHash != "sha256:NEW" { + t.Fatalf("report = %+v, want compliant + sha256:NEW", got) + } + if st, _ := ReadAppliedState(CategoryIDEExtension, TargetVSCode); st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { + t.Fatalf("gallery must now be owned, got %+v", st) + } +} + +func TestEnforceManagedURLReplaced(t *testing.T) { + // A → B, agent-owned throughout. + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw(galURLA)}, + }} + r, rep := newManagedRec(t, policyEPGallery("sha256:NEW", galURLB), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, + WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if gal, ok := opFor(lastApply(t, w), galleryServiceURLSettingKey); !ok || !gal.Set || string(gal.Value) != galleryRaw(galURLB) { + t.Fatalf("gallery must be replaced with %s, got %+v", galleryRaw(galURLB), gal) + } + if got := lastReport(t, rep); got.State != StateCompliant || got.AppliedHash != "sha256:NEW" { + t.Fatalf("report = %+v, want compliant + sha256:NEW", got) + } + if st, _ := ReadAppliedState(CategoryIDEExtension, TargetVSCode); st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLB) { + t.Fatalf("gallery ownership must be updated to B, got %+v", st) + } +} + +func TestEnforceManagedURLRemovedWhenOwned(t *testing.T) { + // The policy drops its URL and the on-disk value is the one the agent wrote + // → the gallery key is removed and ownership of it dropped. + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw(galURLA)}, + }} + r, rep := newManagedRec(t, policyEP("sha256:NEW"), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, + WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if gal, ok := opFor(lastApply(t, w), galleryServiceURLSettingKey); !ok || !gal.Remove { + t.Fatalf("gallery must be removed (owned), got %+v", gal) + } + if _, present := w.state[galleryServiceURLSettingKey]; present { + t.Fatal("gallery key must be gone from disk after removal") + } + if got := lastReport(t, rep); got.State != StateCompliant { + t.Fatalf("state = %q, want compliant", got.State) + } + if st, _ := ReadAppliedState(CategoryIDEExtension, TargetVSCode); st.WrittenSettings[galleryServiceURLSettingKey] != "" { + t.Fatalf("gallery ownership must be dropped after removal, got %+v", st) + } +} + +func TestEnforceManagedURLAbsentForeignValuePreserved(t *testing.T) { + // The policy has no URL and the on-disk gallery value is FOREIGN (the agent + // never wrote it). It must be preserved, never deleted — even while the + // allowlist is (re)written for a new hash. + const foreign = "https://user-configured.example/api/v1" + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw(foreign)}, + }} + r, rep := newManagedRec(t, policyEP("sha256:NEW"), w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, // owns allowlist only + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + ops := lastApply(t, w) + if _, ok := opFor(ops, galleryServiceURLSettingKey); ok { + t.Fatalf("foreign gallery value must NOT be part of the write, ops=%v", ops) + } + if got := w.state[galleryServiceURLSettingKey]; !got.Present || got.Raw != galleryRaw(foreign) { + t.Fatalf("foreign gallery value must be preserved, got %+v", got) + } + if got := lastReport(t, rep); got.State != StateCompliant { + t.Fatalf("state = %q, want compliant", got.State) + } + if st, _ := ReadAppliedState(CategoryIDEExtension, TargetVSCode); st.WrittenSettings != nil { + t.Fatalf("agent must not claim ownership of the foreign value, got %+v", st.WrittenSettings) + } +} + +func TestClearManagedRemovesOwnedKeysLeavesForeign(t *testing.T) { + // clear:true removes each owned key independently; a tampered/foreign key is + // preserved. Here the allowlist is agent-owned (removed) but the gallery + // holds a foreign value (left). + const foreign = "https://user-configured.example/api/v1" + w := &fakeManagedWriter{state: map[string]settingValue{ + allowedExtensionsSettingKey: {Present: true, Raw: samplePolicy}, + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw(foreign)}, + }} + r, rep := newManagedRec(t, EffectivePolicy{Category: CategoryIDEExtension, Clear: true}, w) + if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ + AppliedHash: "sha256:H", WrittenValue: samplePolicy, + WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, // owned A, but disk is foreign + }); err != nil { + t.Fatal(err) + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + ops := lastApply(t, w) + if al, ok := opFor(ops, allowedExtensionsSettingKey); !ok || !al.Remove { + t.Fatalf("owned allowlist must be removed, got %+v", al) + } + if _, ok := opFor(ops, galleryServiceURLSettingKey); ok { + t.Fatalf("foreign gallery must NOT be removed, ops=%v", ops) + } + if got := w.state[galleryServiceURLSettingKey]; !got.Present || got.Raw != galleryRaw(foreign) { + t.Fatalf("foreign gallery must survive clear, got %+v", got) + } + if _, present := w.state[allowedExtensionsSettingKey]; present { + t.Fatal("owned allowlist must be gone after clear") + } + if len(rep.reports) != 0 { + t.Fatalf("clear reports no compliance state, got %+v", rep.reports) + } + if _, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode); ok { + t.Fatal("ownership record must be dropped on clear") + } +} + +func TestEnforceManagedPersistFailureRollsBackBothKeys(t *testing.T) { + // Post-write ownership persist fails: BOTH just-written keys are restored to + // their pre-write (absent) state via RestoreManaged, and the state is + // write_failed. + w := &fakeManagedWriter{} + r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) + calls := 0 + r.writeState = func(string, string, AppliedTargetState) error { + calls++ + if calls == 1 { + return nil // preflight probe + } + return errors.New("disk full") + } + if err := r.Reconcile(context.Background()); err == nil { + t.Fatal("persist failure should surface an error") + } + if len(w.applies) != 1 { + t.Fatalf("want exactly one write attempt, applies=%v", w.applies) + } + if len(w.restores) != 1 { + t.Fatalf("want exactly one RestoreManaged, restores=%v", w.restores) + } + // Both keys were absent pre-write, so rollback must leave them absent. + if _, ok := w.state[allowedExtensionsSettingKey]; ok { + t.Fatal("allowlist must be rolled back to absent") + } + if _, ok := w.state[galleryServiceURLSettingKey]; ok { + t.Fatal("gallery must be rolled back to absent") + } + if got := lastReport(t, rep); got.State != StateWriteFailed { + t.Fatalf("state = %q, want write_failed", got.State) + } +} + +func TestEnforceManagedRollbackFailureReportsVerificationFailed(t *testing.T) { + // Persist fails AND the rollback restore also fails → the on-disk state is + // uncertain, so verification_failed rather than write_failed. + w := &fakeManagedWriter{restoreErr: errors.New("restore boom")} + r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) + calls := 0 + r.writeState = func(string, string, AppliedTargetState) error { + calls++ + if calls == 1 { + return nil + } + return errors.New("disk full") + } + if err := r.Reconcile(context.Background()); err == nil { + t.Fatal("persist + rollback failure should surface an error") + } + if got := lastReport(t, rep); got.State != StateVerificationFailed { + t.Fatalf("state = %q, want verification_failed", got.State) + } +} + +func TestEnforceManagedReadbackMismatchReportsPolicyNotApplied(t *testing.T) { + // The gallery write silently does not land (readback differs). State is + // policy_not_applied with no applied_hash, but ownership is still recorded + // (value-based ownership self-corrects next cycle). + w := &fakeManagedWriter{readbackOverride: map[string]settingValue{ + galleryServiceURLSettingKey: {Present: true, Raw: galleryRaw("https://wrong.example/api/v1")}, + }} + r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + got := lastReport(t, rep) + if got.State != StatePolicyNotApplied || got.AppliedHash != "" { + t.Fatalf("report = %+v, want policy_not_applied + empty applied_hash", got) + } + if st, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode); !ok || + st.WrittenValue != samplePolicy || + st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { + t.Fatalf("ownership must record the intended values even on readback mismatch, got %+v ok=%v", st, ok) + } +} diff --git a/internal/devicepolicy/settings_writer.go b/internal/devicepolicy/settings_writer.go index 0a76aa5a..bb664821 100644 --- a/internal/devicepolicy/settings_writer.go +++ b/internal/devicepolicy/settings_writer.go @@ -8,6 +8,8 @@ import ( "os" "path/filepath" "runtime" + "sort" + "strings" "github.com/tailscale/hujson" @@ -51,6 +53,46 @@ type Writer interface { Location() string } +// settingOp is a requested mutation of ONE managed settings.json key. Exactly +// one of Set / Remove is chosen by the caller; neither set (the zero value) +// means "preserve" — leave whatever is on disk, the ownership-safe default that +// never deletes a value the agent did not write. Remove is ownership-gated by +// the caller (reconcile), not here. +type settingOp struct { + Key string // e.g. allowedExtensionsSettingKey, galleryServiceURLSettingKey + Set bool // set Key to Value + Value json.RawMessage // already-encoded, compacted JSON value (when Set) + Remove bool // remove Key +} + +// settingValue is a key's on-disk state. Present distinguishes an absent key +// from a present-but-empty one (needed for convergence + readback); Raw is the +// compacted encoded value (empty when absent). +type settingValue struct { + Present bool + Raw string +} + +// managedSettingsWriter is implemented by settingsWriter. The reconciler +// type-asserts for it; a Writer without it keeps the single-key Write/Read/Clear +// path. Each method acts on one atomic file write over a set of keyed ops. +type managedSettingsWriter interface { + // ReadManaged returns each requested key's on-disk value + presence in one + // file read. An unparseable / non-object settings.json is an error (the + // never-clobber contract), exactly as Read. + ReadManaged(keys []string) (map[string]settingValue, error) + // ApplyManaged performs ONE atomic load→patch→store over the ops (Set → + // add, Remove → remove-if-present, preserve → skipped), then reads back and + // returns every op key's resulting value + presence. All requested mutations + // land or none do. When no op mutates anything (all preserve, or every + // Remove targets an absent key) it performs no write at all. + ApplyManaged(ops []settingOp) (map[string]settingValue, error) + // RestoreManaged restores each key in the snapshot to its recorded state + // (Present → set to Raw, absent → remove) in one atomic write. Used by + // enforce's post-write rollback. + RestoreManaged(snapshot map[string]settingValue) error +} + // allowedExtensionsSettingKey is the `extensions.allowed` SETTING ID — the key // VS Code reads from settings.json. This is deliberately NOT the registered // policy name "AllowedExtensions" (allowedExtensionsName): policy locations @@ -59,6 +101,14 @@ type Writer interface { // is the surface the agent writes. const allowedExtensionsSettingKey = "extensions.allowed" +// galleryServiceURLSettingKey is the `extensions.gallery.serviceUrl` SETTING ID +// — the (hidden, application-scope) key VS Code reads from settings.json to +// repoint the extension marketplace. Like allowedExtensionsSettingKey it is +// deliberately NOT the registered policy name "ExtensionGalleryServiceUrl" +// (galleryServiceURLName), which the read-only probes look for at OS policy +// locations; this is the setting id the agent writes. +const galleryServiceURLSettingKey = "extensions.gallery.serviceUrl" + // settingsFileMode is the fallback mode for a settings.json the agent creates. // An existing file keeps its current mode (atomicfile.PickMode); 0600 for a // new one is safe because VS Code runs as the same user. @@ -177,18 +227,24 @@ func (w *settingsWriter) load() (v hujson.Value, existed bool, err error) { return v, true, nil } -// extract returns the compacted current value of the extensions.allowed key -// from a parsed tree, or ok=false when the key is absent. Compaction -// normalizes whitespace (and any comments inside the value, which Standardize -// strips) so values compare canonically regardless of on-disk formatting. +// extractAllowedExtensions returns the compacted current value of the +// extensions.allowed key, or ok=false when it is absent. func extractAllowedExtensions(v hujson.Value) (string, bool, error) { + return extractKey(v, allowedExtensionsSettingKey) +} + +// extractKey returns the compacted current value of one top-level settings key +// from a parsed tree, or ok=false when the key is absent. Compaction normalizes +// whitespace (and any comments inside the value, which Standardize strips) so +// values compare canonically regardless of on-disk formatting. +func extractKey(v hujson.Value, key string) (string, bool, error) { std := v.Clone() std.Standardize() m := map[string]json.RawMessage{} if err := json.Unmarshal(std.Pack(), &m); err != nil { return "", false, fmt.Errorf("devicepolicy: standardize settings: %w", err) } - raw, ok := m[allowedExtensionsSettingKey] + raw, ok := m[key] if !ok { return "", false, nil } @@ -285,3 +341,117 @@ func (w *settingsWriter) store(v hujson.Value) error { } return nil } + +// ReadManaged returns each requested key's compacted value + presence in one +// file read. An absent/blank file yields all-absent; an unparseable or +// non-object file is an error (the never-clobber contract), same as Read. +func (w *settingsWriter) ReadManaged(keys []string) (map[string]settingValue, error) { + v, existed, err := w.load() + if err != nil { + return nil, err + } + out := make(map[string]settingValue, len(keys)) + if !existed { + for _, k := range keys { + out[k] = settingValue{} + } + return out, nil + } + // Standardize + unmarshal once, then look up every key (cheaper than + // extractKey per key, and identical in result). + std := v.Clone() + std.Standardize() + m := map[string]json.RawMessage{} + if err := json.Unmarshal(std.Pack(), &m); err != nil { + return nil, fmt.Errorf("devicepolicy: standardize settings: %w", err) + } + for _, k := range keys { + raw, ok := m[k] + if !ok { + out[k] = settingValue{} + continue + } + s, err := compactJSON(raw) + if err != nil { + return nil, err + } + out[k] = settingValue{Present: true, Raw: s} + } + return out, nil +} + +// ApplyManaged builds one RFC-6902 patch from ops — Set → add (upsert), Remove +// → remove (pre-checked so an absent key is skipped, not an error), preserve → +// nothing — and applies it in a single atomic load→patch→store. An empty patch +// writes nothing, so a remove-absent or preserve-only call leaves the file +// untouched. Returns a readback of every op's key. +func (w *settingsWriter) ApplyManaged(ops []settingOp) (map[string]settingValue, error) { + v, _, err := w.load() + if err != nil { + return nil, err + } + patchOps := make([]string, 0, len(ops)) + for _, op := range ops { + switch { + case op.Set: + // The patch document embeds the value verbatim; reject anything that + // is not valid JSON before it can corrupt the patch (defense in depth + // — the caller passes already-compacted, validated values). Object + // shape is NOT required: a managed value may be a JSON string (the + // gallery URL) as well as an object (the allowlist). + if !json.Valid(op.Value) { + return nil, fmt.Errorf("devicepolicy: refusing to write invalid JSON value for %q to %s", op.Key, w.path) + } + // op.Key is a dotted setting id with no '/' or '~', so it needs no + // JSON-Pointer escaping; the dot is literal. + patchOps = append(patchOps, `{"op":"add","path":"/`+op.Key+`","value":`+string(op.Value)+`}`) + case op.Remove: + // RFC 6902 "remove" errors on an absent member; pre-check presence so + // a Remove of a key that is not there is simply skipped. + _, present, perr := extractKey(v, op.Key) + if perr != nil { + return nil, perr + } + if present { + patchOps = append(patchOps, `{"op":"remove","path":"/`+op.Key+`"}`) + } + } + } + if len(patchOps) > 0 { + patch := "[" + strings.Join(patchOps, ",") + "]" + if err := v.Patch([]byte(patch)); err != nil { + return nil, fmt.Errorf("devicepolicy: patch %s: %w", w.path, err) + } + if err := w.store(v); err != nil { + return nil, err + } + } + keys := make([]string, len(ops)) + for i, op := range ops { + keys[i] = op.Key + } + return w.ReadManaged(keys) +} + +// RestoreManaged restores each key in the snapshot to its recorded state in one +// atomic write: a Present key is set back to its Raw value, an absent key is +// removed. Keys are applied in sorted order so the output is deterministic. +// Used by enforce's post-write rollback to undo a multi-key write atomically. +func (w *settingsWriter) RestoreManaged(snapshot map[string]settingValue) error { + keys := make([]string, 0, len(snapshot)) + for k := range snapshot { + keys = append(keys, k) + } + sort.Strings(keys) + ops := make([]settingOp, 0, len(keys)) + for _, k := range keys { + sv := snapshot[k] + if sv.Present { + ops = append(ops, settingOp{Key: k, Set: true, Value: json.RawMessage(sv.Raw)}) + } else { + ops = append(ops, settingOp{Key: k, Remove: true}) + } + } + _, err := w.ApplyManaged(ops) + return err +} diff --git a/internal/devicepolicy/settings_writer_test.go b/internal/devicepolicy/settings_writer_test.go index 2172e922..0676615a 100644 --- a/internal/devicepolicy/settings_writer_test.go +++ b/internal/devicepolicy/settings_writer_test.go @@ -406,3 +406,224 @@ func TestSettingsPathPerOS(t *testing.T) { } } } + +// --- managed multi-key API ------------------------------------------------- + +// TestApplyManagedAllowlistOnlyMatchesWriteBytes pins that a managed write of +// only the allowlist produces settings.json bytes identical to the single-key +// Write, so the allowlist path is unchanged in effect. +func TestApplyManagedAllowlistOnlyMatchesWriteBytes(t *testing.T) { + w1, p1 := newTestSettingsWriter(t) + writeSettingsFixture(t, p1, sampleSettings) + if _, err := w1.Write(samplePolicyObject); err != nil { + t.Fatalf("Write: %v", err) + } + + w2, p2 := newTestSettingsWriter(t) + writeSettingsFixture(t, p2, sampleSettings) + if _, err := w2.ApplyManaged([]settingOp{{Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(samplePolicyObject)}}); err != nil { + t.Fatalf("ApplyManaged: %v", err) + } + + if b1, b2 := readFileString(t, p1), readFileString(t, p2); b1 != b2 { + t.Fatalf("ApplyManaged(allowlist-only) bytes differ from Write:\n--- Write:\n%s\n--- ApplyManaged:\n%s", b1, b2) + } +} + +func TestApplyManagedWritesBothKeysPreservingFile(t *testing.T) { + w, path := newTestSettingsWriter(t) + writeSettingsFixture(t, path, sampleSettings) + rb, err := w.ApplyManaged([]settingOp{ + {Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(samplePolicyObject)}, + {Key: galleryServiceURLSettingKey, Set: true, Value: json.RawMessage(galleryRaw(galURLA))}, + }) + if err != nil { + t.Fatalf("ApplyManaged: %v", err) + } + if al := rb[allowedExtensionsSettingKey]; !al.Present || al.Raw != samplePolicyObject { + t.Fatalf("allowlist readback = %+v", al) + } + if gal := rb[galleryServiceURLSettingKey]; !gal.Present || gal.Raw != galleryRaw(galURLA) { + t.Fatalf("gallery readback = %+v", gal) + } + assertFragmentsPreserved(t, readFileString(t, path)) + // Confirm on-disk persistence of both keys (fresh read). + got, err := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if err != nil { + t.Fatal(err) + } + if got[allowedExtensionsSettingKey].Raw != samplePolicyObject || got[galleryServiceURLSettingKey].Raw != galleryRaw(galURLA) { + t.Fatalf("on-disk keys = %+v", got) + } +} + +func TestApplyManagedRemoveGalleryToleratesAbsence(t *testing.T) { + w, path := newTestSettingsWriter(t) + writeSettingsFixture(t, path, sampleSettings) + if _, err := w.ApplyManaged([]settingOp{ + {Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(samplePolicyObject)}, + {Key: galleryServiceURLSettingKey, Set: true, Value: json.RawMessage(galleryRaw(galURLA))}, + }); err != nil { + t.Fatalf("ApplyManaged set: %v", err) + } + if _, err := w.ApplyManaged([]settingOp{{Key: galleryServiceURLSettingKey, Remove: true}}); err != nil { + t.Fatalf("ApplyManaged remove: %v", err) + } + got, _ := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if got[galleryServiceURLSettingKey].Present { + t.Fatal("gallery must be removed") + } + if !got[allowedExtensionsSettingKey].Present { + t.Fatal("allowlist must remain") + } + // Removing an already-absent key is a no-op: no error, no rewrite. + before := readFileString(t, path) + if _, err := w.ApplyManaged([]settingOp{{Key: galleryServiceURLSettingKey, Remove: true}}); err != nil { + t.Fatalf("ApplyManaged remove-absent: %v", err) + } + if after := readFileString(t, path); after != before { + t.Fatalf("remove of an absent key rewrote the file:\n--- before:\n%s\n--- after:\n%s", before, after) + } +} + +func TestApplyManagedLeavesUnmentionedKeys(t *testing.T) { + // A foreign gallery value already on disk; a write mentioning ONLY the + // allowlist must leave it (and every other key/comment) untouched. + w, path := newTestSettingsWriter(t) + fixture := strings.Replace(sampleSettings, + "\t// telemetry opt-out", + "\t\"extensions.gallery.serviceUrl\": \"https://foreign.example/api/v1\",\n\n\t// telemetry opt-out", 1) + writeSettingsFixture(t, path, fixture) + if _, err := w.ApplyManaged([]settingOp{{Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(samplePolicyObject)}}); err != nil { + t.Fatalf("ApplyManaged: %v", err) + } + got, _ := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if got[allowedExtensionsSettingKey].Raw != samplePolicyObject { + t.Fatalf("allowlist = %+v", got[allowedExtensionsSettingKey]) + } + if got[galleryServiceURLSettingKey].Raw != `"https://foreign.example/api/v1"` { + t.Fatalf("foreign gallery value must be preserved untouched, got %+v", got[galleryServiceURLSettingKey]) + } + assertFragmentsPreserved(t, readFileString(t, path)) +} + +func TestReadManagedPresenceFlags(t *testing.T) { + w, path := newTestSettingsWriter(t) + // Missing file: all absent. + got, err := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if err != nil { + t.Fatal(err) + } + if got[allowedExtensionsSettingKey].Present || got[galleryServiceURLSettingKey].Present { + t.Fatalf("missing file → all absent, got %+v", got) + } + // A present-but-empty-string value is distinct from absent. + writeSettingsFixture(t, path, `{"extensions.gallery.serviceUrl": ""}`) + got, err = w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if err != nil { + t.Fatal(err) + } + if got[allowedExtensionsSettingKey].Present { + t.Fatalf("allowlist must be absent, got %+v", got[allowedExtensionsSettingKey]) + } + if gv := got[galleryServiceURLSettingKey]; !gv.Present || gv.Raw != `""` { + t.Fatalf("gallery must be present-empty (Raw==%q), got %+v", `""`, gv) + } +} + +func TestRestoreManagedRoundTrip(t *testing.T) { + w, path := newTestSettingsWriter(t) + writeSettingsFixture(t, path, sampleSettings) + // Snapshot: allowlist present, gallery absent. + if _, err := w.ApplyManaged([]settingOp{{Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(samplePolicyObject)}}); err != nil { + t.Fatal(err) + } + snapshot, err := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if err != nil { + t.Fatal(err) + } + // Mutate away: change allowlist, add gallery. + if _, err := w.ApplyManaged([]settingOp{ + {Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(`{"changed":true}`)}, + {Key: galleryServiceURLSettingKey, Set: true, Value: json.RawMessage(galleryRaw(galURLA))}, + }); err != nil { + t.Fatal(err) + } + // Restore → back to the snapshot exactly. + if err := w.RestoreManaged(snapshot); err != nil { + t.Fatalf("RestoreManaged: %v", err) + } + got, err := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey}) + if err != nil { + t.Fatal(err) + } + if got[allowedExtensionsSettingKey] != snapshot[allowedExtensionsSettingKey] { + t.Fatalf("allowlist not restored: got %+v want %+v", got[allowedExtensionsSettingKey], snapshot[allowedExtensionsSettingKey]) + } + if got[galleryServiceURLSettingKey].Present { + t.Fatalf("gallery should be restored to absent, got %+v", got[galleryServiceURLSettingKey]) + } +} + +func TestManagedMethodsRefuseUnsalvageableFile(t *testing.T) { + const broken = `{"editor.fontSize": 14, << — the HTML-escaping edge. +func TestManagedGalleryValueRoundTrips(t *testing.T) { + for _, url := range []string{ + "https://mkt.example/api/v1", + "https://mkt.example/api/v1?tenant=acme&mode=strict", + "https://mkt.example/p/", + } { + w, _ := newTestSettingsWriter(t) + gv, err := managedGalleryValue(url) + if err != nil { + t.Fatalf("managedGalleryValue(%q): %v", url, err) + } + if _, err := w.ApplyManaged([]settingOp{{Key: galleryServiceURLSettingKey, Set: true, Value: json.RawMessage(gv)}}); err != nil { + t.Fatalf("ApplyManaged(%q): %v", url, err) + } + got, err := w.ReadManaged([]string{galleryServiceURLSettingKey}) + if err != nil { + t.Fatal(err) + } + if sv := got[galleryServiceURLSettingKey]; !sv.Present || sv.Raw != gv { + t.Fatalf("url %q: readback Raw=%q, want %q (owned value must equal readback)", url, sv.Raw, gv) + } + } +} From 7177bed25ecf0bf8f44f56e031c15a1e60abde5d Mon Sep 17 00:00:00 2001 From: Subham Ray Date: Thu, 23 Jul 2026 03:35:11 +0530 Subject: [PATCH 2/5] refactor(devicepolicy): track all managed-key ownership in WrittenSettings Move the VS Code allowlist's ownership record from the scalar WrittenValue into the WrittenSettings map alongside the gallery URL, so every managed key is tracked uniformly by setting id. WrittenValue is now used only by the single-key (npm) writer; adds omitempty so managed records no longer carry a dead written_value field. No behavior change: ownership is read back from the same values the agent writes. --- internal/devicepolicy/cache.go | 30 ++++++++--------- internal/devicepolicy/cache_test.go | 9 ++--- internal/devicepolicy/reconcile.go | 20 ++++------- internal/devicepolicy/reconcile_test.go | 44 +++++++++++++++---------- 4 files changed, 52 insertions(+), 51 deletions(-) diff --git a/internal/devicepolicy/cache.go b/internal/devicepolicy/cache.go index 57a3e40a..5d1487a9 100644 --- a/internal/devicepolicy/cache.go +++ b/internal/devicepolicy/cache.go @@ -32,7 +32,7 @@ const ( // "categories": { // "ide_extension": { // "targets": { -// "vscode": { "applied_hash": …, "written_value": …, "fetched_at": … } +// "vscode": { "applied_hash": …, "written_settings": …, "fetched_at": … } // } // } // } @@ -50,30 +50,26 @@ type AppliedCategoryState struct { Targets map[string]AppliedTargetState `json:"targets"` } -// AppliedTargetState records what the agent last wrote to the user-scope VS -// Code settings.json for one (category, target). Three fields drive correctness: +// AppliedTargetState records what the agent last wrote to the user-scope +// settings for one (category, target). Value-based ownership drives both drift +// and clear: a key is converged or removed only while its on-disk value still +// equals what the agent recorded writing (a differing value — e.g. the user's +// own — is left untouched). // // - AppliedHash is the backend's content hash, stored VERBATIM (never // recomputed). Compared against the freshly-fetched hash for idempotency. -// - WrittenValue is the exact compacted extensions.allowed value the agent -// wrote. It drives value-based ownership and drift: on a clear, the agent -// removes the settings key only if the on-disk value still equals -// WrittenValue (a differing value — e.g. the user's own — is left -// untouched); on enforce, an on-disk value differing from WrittenValue is -// drift and is converged back. -// - WrittenSettings holds the same value-based ownership for the other -// managed settings keys (the gallery service URL): setting id → the exact -// compacted value the agent wrote. A key absent from the map is one the -// agent does not own, so removal and clear leave it untouched — as an empty -// WrittenValue does for the allowlist. The allowlist stays in WrittenValue, -// and omitempty drops the map, so an allowlist-only record is unchanged on -// disk. +// - WrittenSettings holds ownership for the managed multi-key path: setting id +// → the exact compacted value the agent wrote, for every managed key (the VS +// Code allowlist and the gallery service URL). A key absent from the map is +// one the agent does not own. +// - WrittenValue holds ownership for the single-key path (the npm writer, which +// owns one opaque value). The managed multi-key path does not use it. // // A zero-value entry means "the agent owns nothing on disk" for that // category/target. type AppliedTargetState struct { AppliedHash string `json:"applied_hash"` - WrittenValue string `json:"written_value"` + WrittenValue string `json:"written_value,omitempty"` WrittenSettings map[string]string `json:"written_settings,omitempty"` FetchedAt time.Time `json:"fetched_at"` } diff --git a/internal/devicepolicy/cache_test.go b/internal/devicepolicy/cache_test.go index 0cb2b1c6..9aa16539 100644 --- a/internal/devicepolicy/cache_test.go +++ b/internal/devicepolicy/cache_test.go @@ -394,17 +394,18 @@ func TestAppliedTargetNoWrittenSettingsOmitsField(t *testing.T) { }); err != nil { t.Fatal(err) } - // Byte-shape parity: an allowlist-only record must omit written_settings. + // A single-key writer's record (npm: one opaque WrittenValue, no managed + // multi-key map) must omit written_settings on disk. raw, err := os.ReadFile(path) if err != nil { t.Fatal(err) } if strings.Contains(string(raw), "written_settings") { - t.Fatalf("allowlist-only record must omit written_settings:\n%s", raw) + t.Fatalf("single-key record must omit written_settings:\n%s", raw) } - // And it reads back as a nil map (owns no extra keys). + // And it reads back as a nil map (owns no managed multi-key state). got, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode) if !ok || got.WrittenSettings != nil { - t.Fatalf("WrittenSettings must be nil for an allowlist-only record, got %+v ok=%v", got.WrittenSettings, ok) + t.Fatalf("WrittenSettings must be nil for a single-key record, got %+v ok=%v", got.WrittenSettings, ok) } } diff --git a/internal/devicepolicy/reconcile.go b/internal/devicepolicy/reconcile.go index 2cd0e17b..062018e2 100644 --- a/internal/devicepolicy/reconcile.go +++ b/internal/devicepolicy/reconcile.go @@ -463,19 +463,16 @@ func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep Eff } } - // 9. Persist ownership: what the agent now OWNS. Allowlist → WrittenValue; - // each extra Set key → WrittenSettings; a Remove or preserve key asserts no - // ownership this cycle (omitted). - ownedAfter := map[string]string{} + // 9. Persist ownership: every managed key the agent now OWNS, keyed by setting + // id. The allowlist is always owned (authoritative Set); a Remove or preserve + // key asserts no ownership this cycle (omitted). WrittenValue is the single-key + // path's field and is left untouched here. + ownedAfter := map[string]string{allowedExtensionsSettingKey: newValue} if galleryOp.Set { ownedAfter[galKey] = galValue } - if len(ownedAfter) == 0 { - ownedAfter = nil // keep an allowlist-only record byte-identical to before - } if err := r.persistState(cat, tgt, AppliedTargetState{ AppliedHash: ep.Hash, - WrittenValue: newValue, WrittenSettings: ownedAfter, FetchedAt: r.now(), }); err != nil { @@ -531,17 +528,14 @@ func opConverged(op settingOp, m map[string]settingValue) bool { } // ownedKeys folds an ownership record into a flat map of setting id → the exact -// value the agent last wrote, skipping empty entries. The allowlist comes from -// WrittenValue, the other managed keys from WrittenSettings. Drift detection and +// value the agent last wrote, skipping empty entries. Every managed key — the +// allowlist included — lives in WrittenSettings. Drift detection and // ownership-gated removal act only on keys the agent actually wrote. func ownedKeys(prev AppliedTargetState, hadPrev bool) map[string]string { owned := map[string]string{} if !hadPrev { return owned } - if prev.WrittenValue != "" { - owned[allowedExtensionsSettingKey] = prev.WrittenValue - } for k, v := range prev.WrittenSettings { if v != "" { owned[k] = v diff --git a/internal/devicepolicy/reconcile_test.go b/internal/devicepolicy/reconcile_test.go index b2000e82..5e0b9cbd 100644 --- a/internal/devicepolicy/reconcile_test.go +++ b/internal/devicepolicy/reconcile_test.go @@ -826,7 +826,7 @@ func TestEnforceManagedFirstEnforceWritesBothKeys(t *testing.T) { t.Fatalf("report = %+v, want compliant + echoed hash", got) } st, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode) - if !ok || st.WrittenValue != samplePolicy || st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { + if !ok || st.WrittenSettings[allowedExtensionsSettingKey] != samplePolicy || st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { t.Fatalf("ownership = %+v ok=%v, want allowlist + gallery owned", st, ok) } } @@ -838,8 +838,10 @@ func TestEnforceManagedIdempotentNoRewrite(t *testing.T) { }} r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:H", WrittenValue: samplePolicy, - WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + AppliedHash: "sha256:H", WrittenSettings: map[string]string{ + allowedExtensionsSettingKey: samplePolicy, + galleryServiceURLSettingKey: galleryRaw(galURLA), + }, }); err != nil { t.Fatal(err) } @@ -864,8 +866,10 @@ func TestEnforceManagedGalleryOnlyDriftReapplies(t *testing.T) { }} r, rep := newManagedRec(t, policyEPGallery("sha256:H", galURLA), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:H", WrittenValue: samplePolicy, - WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + AppliedHash: "sha256:H", WrittenSettings: map[string]string{ + allowedExtensionsSettingKey: samplePolicy, + galleryServiceURLSettingKey: galleryRaw(galURLA), + }, }); err != nil { t.Fatal(err) } @@ -899,7 +903,7 @@ func TestEnforceManagedAllowlistDriftReapplies(t *testing.T) { }} r, rep := newManagedRec(t, policyEP("sha256:H"), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:H", WrittenValue: samplePolicy, + AppliedHash: "sha256:H", WrittenSettings: map[string]string{allowedExtensionsSettingKey: samplePolicy}, }); err != nil { t.Fatal(err) } @@ -926,7 +930,7 @@ func TestEnforceManagedURLAdded(t *testing.T) { }} r, rep := newManagedRec(t, policyEPGallery("sha256:NEW", galURLA), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, + AppliedHash: "sha256:OLD", WrittenSettings: map[string]string{allowedExtensionsSettingKey: samplePolicy}, }); err != nil { t.Fatal(err) } @@ -952,8 +956,10 @@ func TestEnforceManagedURLReplaced(t *testing.T) { }} r, rep := newManagedRec(t, policyEPGallery("sha256:NEW", galURLB), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, - WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + AppliedHash: "sha256:OLD", WrittenSettings: map[string]string{ + allowedExtensionsSettingKey: samplePolicy, + galleryServiceURLSettingKey: galleryRaw(galURLA), + }, }); err != nil { t.Fatal(err) } @@ -980,8 +986,10 @@ func TestEnforceManagedURLRemovedWhenOwned(t *testing.T) { }} r, rep := newManagedRec(t, policyEP("sha256:NEW"), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, - WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, + AppliedHash: "sha256:OLD", WrittenSettings: map[string]string{ + allowedExtensionsSettingKey: samplePolicy, + galleryServiceURLSettingKey: galleryRaw(galURLA), + }, }); err != nil { t.Fatal(err) } @@ -1013,7 +1021,7 @@ func TestEnforceManagedURLAbsentForeignValuePreserved(t *testing.T) { }} r, rep := newManagedRec(t, policyEP("sha256:NEW"), w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:OLD", WrittenValue: samplePolicy, // owns allowlist only + AppliedHash: "sha256:OLD", WrittenSettings: map[string]string{allowedExtensionsSettingKey: samplePolicy}, // owns allowlist only }); err != nil { t.Fatal(err) } @@ -1030,8 +1038,8 @@ func TestEnforceManagedURLAbsentForeignValuePreserved(t *testing.T) { if got := lastReport(t, rep); got.State != StateCompliant { t.Fatalf("state = %q, want compliant", got.State) } - if st, _ := ReadAppliedState(CategoryIDEExtension, TargetVSCode); st.WrittenSettings != nil { - t.Fatalf("agent must not claim ownership of the foreign value, got %+v", st.WrittenSettings) + if st, _ := ReadAppliedState(CategoryIDEExtension, TargetVSCode); st.WrittenSettings[galleryServiceURLSettingKey] != "" { + t.Fatalf("agent must not claim ownership of the foreign gallery value, got %+v", st.WrittenSettings) } } @@ -1046,8 +1054,10 @@ func TestClearManagedRemovesOwnedKeysLeavesForeign(t *testing.T) { }} r, rep := newManagedRec(t, EffectivePolicy{Category: CategoryIDEExtension, Clear: true}, w) if err := WriteAppliedState(CategoryIDEExtension, TargetVSCode, AppliedTargetState{ - AppliedHash: "sha256:H", WrittenValue: samplePolicy, - WrittenSettings: map[string]string{galleryServiceURLSettingKey: galleryRaw(galURLA)}, // owned A, but disk is foreign + AppliedHash: "sha256:H", WrittenSettings: map[string]string{ + allowedExtensionsSettingKey: samplePolicy, + galleryServiceURLSettingKey: galleryRaw(galURLA), // owned A, but disk is foreign + }, }); err != nil { t.Fatal(err) } @@ -1147,7 +1157,7 @@ func TestEnforceManagedReadbackMismatchReportsPolicyNotApplied(t *testing.T) { t.Fatalf("report = %+v, want policy_not_applied + empty applied_hash", got) } if st, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode); !ok || - st.WrittenValue != samplePolicy || + st.WrittenSettings[allowedExtensionsSettingKey] != samplePolicy || st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { t.Fatalf("ownership must record the intended values even on readback mismatch, got %+v ok=%v", st, ok) } From d935b13f18ebb4ab9e4b059ccfa34573a933d028 Mon Sep 17 00:00:00 2001 From: Subham Ray Date: Thu, 23 Jul 2026 15:45:23 +0530 Subject: [PATCH 3/5] refactor(devicepolicy): key run-config policy by setting id (settings map) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The run-config policy payload is now a settings map (setting id → compiled value) instead of the bare extensions.allowed object plus a sibling gallery_service_url field; the hash covers the whole map. enforce and clear are fully driven by the settings map and recorded ownership — no per-key special-casing — so a new managed setting rides through with no code change. Fetch now requires extensions.allowed to be present and a JSON object. Deleted managedKeys/managedGalleryValue; added compactSettings/sortedUnion/sortedKeys. Breaking run-config wire change (pre-GA): requires agent-api to emit the settings-map shape. --- internal/devicepolicy/api.go | 94 ++++---- internal/devicepolicy/api_test.go | 74 +++++-- internal/devicepolicy/reconcile.go | 203 ++++++++++-------- internal/devicepolicy/reconcile_test.go | 67 +++++- internal/devicepolicy/settings_writer_test.go | 29 +-- 5 files changed, 300 insertions(+), 167 deletions(-) diff --git a/internal/devicepolicy/api.go b/internal/devicepolicy/api.go index bd1bc015..5120b44b 100644 --- a/internal/devicepolicy/api.go +++ b/internal/devicepolicy/api.go @@ -39,43 +39,43 @@ const maxRunConfigBytes = 4 << 20 // EffectivePolicy is the parsed device-policy directive, lifted from the // `policy` sub-object of the run-config response (it mirrors agent-api's -// EffectivePolicyResponse). Policy carries the compiled VS Code -// extensions.allowed object as canonical JSON (sorted keys) — the exact bytes -// the backend hashed — so the agent writes it verbatim and never re-serializes +// EffectivePolicyResponse). Policy is the settings map: VS Code setting id +// (e.g. "extensions.allowed", "extensions.gallery.serviceUrl") → that setting's +// compiled value as canonical JSON — the exact bytes the backend hashed. The +// agent writes each value verbatim and never re-serializes // (re-serialization could reorder keys and break the backend's byte-exact -// applied==desired check). Policy identity is (Category, Target); Target -// defaults to vscode for ide_extension. A zero EffectivePolicy (present()==false) -// means run-config carried no directive for this category/target → reconciler -// no-op. +// applied==desired check). extensions.allowed is always present; other keys +// appear only when the policy sets them. Policy identity is (Category, Target); +// Target defaults to vscode for ide_extension. A zero EffectivePolicy +// (present()==false) means run-config carried no directive for this +// category/target → reconciler no-op. type EffectivePolicy struct { - Category string - Target string - Clear bool - Policy json.RawMessage - Hash string - GeneratedAt string - GalleryServiceURL string + Category string + Target string + Clear bool + Policy map[string]json.RawMessage + Hash string + GeneratedAt string } // present reports whether the backend expressed a policy directive for this // category — a value to enforce, or an explicit clear. The fetcher guarantees -// clear=false ⇒ non-empty policy object, so the only successful-fetch state +// clear=false ⇒ a non-empty settings map, so the only successful-fetch state // with neither is "no policy in run-config" (absent), which the reconciler // treats as a no-op (NEVER a clear). func (ep EffectivePolicy) present() bool { return ep.Clear || len(ep.Policy) > 0 } // policyEnvelope is the wire shape of the run-config `policy` sub-object (must -// match agent-api EffectivePolicyResponse). Unknown fields are ignored, so a -// backend still emitting legacy extras (e.g. the removed min_vscode_version) -// stays compatible. +// match agent-api EffectivePolicyResponse). `policy` is the settings map +// (setting id → compiled value). Unknown fields are ignored, so a backend +// emitting extra fields the agent does not model stays compatible. type policyEnvelope struct { - Category string `json:"category"` - Target string `json:"target"` - Clear bool `json:"clear"` - Policy json.RawMessage `json:"policy,omitempty"` - Hash string `json:"hash,omitempty"` - GeneratedAt string `json:"generated_at"` - GalleryServiceURL string `json:"gallery_service_url,omitempty"` + Category string `json:"category"` + Target string `json:"target"` + Clear bool `json:"clear"` + Policy map[string]json.RawMessage `json:"policy,omitempty"` + Hash string `json:"hash,omitempty"` + GeneratedAt string `json:"generated_at"` } // Fetcher returns the effective policy for one device + category + target. @@ -120,8 +120,9 @@ func NewHTTPFetcher(cfg ingest.Config, h *http.Client) (*HTTPFetcher, bool) { // - body that is not valid JSON → error; // - a non-clear result missing policy or hash → error (a malformed policy // must not be written, and must not be mistaken for a clear); -// - a non-clear policy that is not itself a JSON object → error (a string or -// array written verbatim could even read back "compliant"). +// - a non-clear result whose `policy` is not a JSON object → error: it must +// decode as a setting id → value map, so a string/array/scalar in its place +// fails the decode above and no-ops the reconciler. // // An omitted/null `policy` is NOT an error: it means run-config carried no // directive for this category/target (a degraded/rules-only response, an older @@ -194,13 +195,12 @@ func (c *HTTPFetcher) Fetch(ctx context.Context, customerID, deviceID, category, p := env.Policy ep := EffectivePolicy{ - Category: strings.TrimSpace(p.Category), - Target: strings.TrimSpace(p.Target), - Clear: p.Clear, - Policy: p.Policy, - Hash: strings.TrimSpace(p.Hash), - GeneratedAt: p.GeneratedAt, - GalleryServiceURL: strings.TrimSpace(p.GalleryServiceURL), + Category: strings.TrimSpace(p.Category), + Target: strings.TrimSpace(p.Target), + Clear: p.Clear, + Policy: p.Policy, + Hash: strings.TrimSpace(p.Hash), + GeneratedAt: p.GeneratedAt, } if ep.Category == "" { ep.Category = category @@ -209,22 +209,32 @@ func (c *HTTPFetcher) Fetch(ctx context.Context, customerID, deviceID, category, ep.Target = target } if !ep.Clear { + // A non-clear directive must carry a settings map and a hash. The map's + // object shape is already guaranteed: a non-object `policy` fails to + // decode into the map above and returns a decode error, so a + // string/array/scalar never reaches here. if len(ep.Policy) == 0 || ep.Hash == "" { return EffectivePolicy{}, errors.New("devicepolicy: malformed policy: clear=false but policy or hash missing") } - // The compiled policy is always a JSON object. Shape is checked here so a - // malformed payload no-ops at the reconciler; value-level validation stays - // backend-owned. - if !isJSONObject(ep.Policy) { - return EffectivePolicy{}, errors.New("devicepolicy: malformed policy: policy is not a JSON object") + // extensions.allowed is the mandatory core of every ide_extension policy: + // it MUST be present and a JSON object. This rejects an allowlist-missing + // settings map (e.g. a gallery-only response) before it can be written and + // read back "compliant". Other keys stay backend-owned — their values are + // heterogeneous (the allowlist an object, the gallery URL a string). + allow, ok := ep.Policy[allowedExtensionsSettingKey] + if !ok { + return EffectivePolicy{}, errors.New("devicepolicy: malformed policy: settings missing " + allowedExtensionsSettingKey) + } + if !isJSONObject(allow) { + return EffectivePolicy{}, errors.New("devicepolicy: malformed policy: " + allowedExtensionsSettingKey + " is not a JSON object") } } return ep, nil } -// isJSONObject reports whether raw's first JSON token opens an object. The -// envelope already passed json.Unmarshal, so raw is syntactically valid JSON — -// only the shape needs checking. +// isJSONObject reports whether raw's first JSON token opens an object. Callers +// pass syntactically valid JSON (decoded by json.Unmarshal, or json.Valid-checked +// first), so only the shape needs checking. func isJSONObject(raw json.RawMessage) bool { for _, b := range raw { switch b { diff --git a/internal/devicepolicy/api_test.go b/internal/devicepolicy/api_test.go index f4329297..1b2a0a92 100644 --- a/internal/devicepolicy/api_test.go +++ b/internal/devicepolicy/api_test.go @@ -47,7 +47,7 @@ func TestFetchPolicy(t *testing.T) { // min_vscode_version is no longer part of the contract; it stays in the // fixture to prove a backend still emitting legacy fields is tolerated. body := `{"detection_rules":{"version":1},"policy":{"category":"ide_extension","target":"vscode","clear":false,` + - `"policy":{"*":false,"ms-python.python":true},` + + `"policy":{"extensions.allowed":{"*":false,"ms-python.python":true}},` + `"hash":"sha256:abc","min_vscode_version":"1.96.0","generated_at":"2026-06-08T00:00:00Z"}}` _, f := newFetchServer(t, 200, body) ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) @@ -63,9 +63,13 @@ func TestFetchPolicy(t *testing.T) { if ep.Hash != "sha256:abc" { t.Fatalf("hash = %q", ep.Hash) } - // Policy must round-trip as the canonical bytes the backend sent. - if got := string(ep.Policy); !strings.Contains(got, `"ms-python.python":true`) { - t.Fatalf("policy = %s", got) + // The allowlist value in the settings map must round-trip as the bytes the backend sent. + al, ok := ep.Policy[allowedExtensionsSettingKey] + if !ok { + t.Fatal("settings map missing extensions.allowed") + } + if got := string(al); !strings.Contains(got, `"ms-python.python":true`) { + t.Fatalf("allowlist = %s", got) } } @@ -81,33 +85,34 @@ func TestFetchClear(t *testing.T) { } func TestFetchLiftsGalleryServiceURL(t *testing.T) { - // A policy carrying gallery_service_url lifts it into EffectivePolicy; the - // hash is unchanged in shape (backend folds the URL into it). + // A settings map carrying the gallery key lifts it alongside the allowlist; + // the hash covers the whole map (backend folds the URL in). body := `{"policy":{"category":"ide_extension","target":"vscode","clear":false,` + - `"policy":{"*":false},"hash":"sha256:g","gallery_service_url":"https://mkt.example/api/v1",` + - `"generated_at":"2026-07-23T00:00:00Z"}}` + `"policy":{"extensions.allowed":{"*":false},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"},` + + `"hash":"sha256:g","generated_at":"2026-07-23T00:00:00Z"}}` _, f := newFetchServer(t, 200, body) ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) if err != nil { t.Fatalf("Fetch: %v", err) } - if ep.GalleryServiceURL != "https://mkt.example/api/v1" { - t.Fatalf("gallery_service_url = %q, want the URL", ep.GalleryServiceURL) + gal, ok := ep.Policy[galleryServiceURLSettingKey] + if !ok || string(gal) != `"https://mkt.example/api/v1"` { + t.Fatalf("gallery value = %q ok=%v, want the URL as a JSON string", string(gal), ok) } } func TestFetchAbsentGalleryServiceURLIsEmpty(t *testing.T) { - // A policy without gallery_service_url (the common allowlist-only case) - // leaves the field empty and is not an error. + // A settings map without the gallery key (the common allowlist-only case) + // simply omits it; that is not an error. body := `{"policy":{"category":"ide_extension","clear":false,` + - `"policy":{"*":false},"hash":"sha256:h","generated_at":"2026-07-23T00:00:00Z"}}` + `"policy":{"extensions.allowed":{"*":false}},"hash":"sha256:h","generated_at":"2026-07-23T00:00:00Z"}}` _, f := newFetchServer(t, 200, body) ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) if err != nil { t.Fatalf("Fetch: %v", err) } - if ep.GalleryServiceURL != "" { - t.Fatalf("gallery_service_url should be empty, got %q", ep.GalleryServiceURL) + if _, ok := ep.Policy[galleryServiceURLSettingKey]; ok { + t.Fatalf("gallery key should be absent from the settings map, got %q", string(ep.Policy[galleryServiceURLSettingKey])) } } @@ -146,7 +151,7 @@ func TestFetchIgnoresDetectionRules(t *testing.T) { // owns that), proving the two consumers decode independent slices. body := `{"detection_rules":{"version":7,"rules":[{"id":"r1"}]},` + `"policy":{"category":"ide_extension","clear":false,` + - `"policy":{"ms-python.python":true},"hash":"sha256:xyz","generated_at":"2026-06-08T00:00:00Z"}}` + `"policy":{"extensions.allowed":{"ms-python.python":true}},"hash":"sha256:xyz","generated_at":"2026-06-08T00:00:00Z"}}` _, f := newFetchServer(t, 200, body) ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) if err != nil { @@ -155,8 +160,8 @@ func TestFetchIgnoresDetectionRules(t *testing.T) { if ep.Hash != "sha256:xyz" { t.Fatalf("hash = %q, want sha256:xyz", ep.Hash) } - if got := string(ep.Policy); !strings.Contains(got, `"ms-python.python":true`) { - t.Fatalf("policy = %s", got) + if got := string(ep.Policy[allowedExtensionsSettingKey]); !strings.Contains(got, `"ms-python.python":true`) { + t.Fatalf("allowlist = %s", got) } // The policy object omits `target`; Fetch defaults it to the requested target. if ep.Target != TargetVSCode { @@ -168,7 +173,7 @@ func TestFetchDefaultsRequestTargetToVSCode(t *testing.T) { // An empty target argument must still send target=vscode on the wire // (newFetchServer asserts the query param) and parse back as vscode. body := `{"policy":{"category":"ide_extension","clear":false,` + - `"policy":{"ms-python.python":true},"hash":"sha256:abc","generated_at":"2026-06-08T00:00:00Z"}}` + `"policy":{"extensions.allowed":{"ms-python.python":true}},"hash":"sha256:abc","generated_at":"2026-06-08T00:00:00Z"}}` _, f := newFetchServer(t, 200, body) ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, "") if err != nil { @@ -196,8 +201,9 @@ func TestFetchNonClearMissingPolicyIsError(t *testing.T) { } func TestFetchNonObjectPolicyIsError(t *testing.T) { - // A policy that is not a JSON object must never reach the writer: written - // verbatim it could even read back as "compliant". + // The `policy` must decode as a JSON object (setting id → value map). A + // string / array / scalar / null in its place is malformed: it fails the + // decode (or yields an empty map) → error, so nothing reaches the writer. for _, body := range []string{ `{"policy":{"category":"ide_extension","clear":false,"policy":"bad","hash":"sha256:x","generated_at":"x"}}`, `{"policy":{"category":"ide_extension","clear":false,"policy":[],"hash":"sha256:x","generated_at":"x"}}`, @@ -211,6 +217,32 @@ func TestFetchNonObjectPolicyIsError(t *testing.T) { } } +func TestFetchSettingsMissingAllowlistIsError(t *testing.T) { + // extensions.allowed is mandatory. A settings map that carries only other keys + // (e.g. a gallery-only response) is malformed and must error — never be written + // or read back "compliant". + body := `{"policy":{"category":"ide_extension","clear":false,` + + `"policy":{"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"},"hash":"sha256:x","generated_at":"x"}}` + _, f := newFetchServer(t, 200, body) + if _, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode); err == nil { + t.Fatal("settings map missing extensions.allowed must be an error") + } +} + +func TestFetchAllowlistNotObjectIsError(t *testing.T) { + // extensions.allowed present but not an object (a string / array written + // verbatim could even read back "compliant") → malformed. + for _, body := range []string{ + `{"policy":{"category":"ide_extension","clear":false,"policy":{"extensions.allowed":"bad"},"hash":"sha256:x","generated_at":"x"}}`, + `{"policy":{"category":"ide_extension","clear":false,"policy":{"extensions.allowed":[]},"hash":"sha256:x","generated_at":"x"}}`, + } { + _, f := newFetchServer(t, 200, body) + if _, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode); err == nil { + t.Fatalf("non-object extensions.allowed must be an error, body: %s", body) + } + } +} + func TestFetchNon200IsError(t *testing.T) { _, f := newFetchServer(t, 500, `{"error":"boom"}`) if _, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode); err == nil { diff --git a/internal/devicepolicy/reconcile.go b/internal/devicepolicy/reconcile.go index 062018e2..337587c0 100644 --- a/internal/devicepolicy/reconcile.go +++ b/internal/devicepolicy/reconcile.go @@ -1,11 +1,11 @@ package devicepolicy import ( - "bytes" "context" "encoding/json" "errors" "fmt" + "sort" "time" ) @@ -179,18 +179,20 @@ func (r *Reconciler) clearSingle(cat, tgt string, prev AppliedTargetState, hadPr } // clearManaged is the managed multi-key unassignment path. It removes each -// managed key INDEPENDENTLY, and only when its on-disk value still equals what -// the agent wrote (per-key ownership); a foreign-valued or absent key is -// preserved. One atomic write carries only the owned-key removes. +// agent-OWNED key INDEPENDENTLY, and only when its on-disk value still equals +// what the agent wrote (per-key ownership); a foreign-valued or absent key is +// preserved. The candidate set is exactly the recorded ownership (not a static +// key list), so any key the agent ever wrote is cleared without code change. +// One atomic write carries only the owned-key removes. func (r *Reconciler) clearManaged(cat, tgt string, prev AppliedTargetState, hadPrev bool, mw managedSettingsWriter) error { - keys := managedKeys() + owned := ownedKeys(prev, hadPrev) + keys := sortedKeys(owned) // only owned keys can be removed; sorted → deterministic cur, err := mw.ReadManaged(keys) if err != nil { return fmt.Errorf("devicepolicy: clear: read %s: %w", r.Writer.Location(), err) } - owned := ownedKeys(prev, hadPrev) var ops []settingOp - for _, key := range keys { // fixed order → deterministic write + for _, key := range keys { ov := owned[key] if ov != "" && cur[key].Present && cur[key].Raw == ov { ops = append(ops, settingOp{Key: key, Remove: true}) @@ -236,13 +238,15 @@ func (r *Reconciler) dropClearedState(cat, tgt string, hadPrev bool) error { // → persist ownership on every successful write (rollback if that fails) // → Verify → report (drift upgrades a would-be compliant to drift_detected) func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep EffectivePolicy) error { - // The compiled allowlist compacted: the canonical comparison form for - // readback, idempotency, and ownership. (The backend's hash still travels - // verbatim; only the value bytes are normalized for comparison.) - newValue, err := compactJSON(ep.Policy) + // Compact every value in the settings map to its canonical comparison form + // (setting id → compacted value): the form used for readback, idempotency, + // and ownership. (The backend's hash still travels verbatim; only the value + // bytes are normalized for comparison.) + desired, err := compactSettings(ep.Policy) if err != nil { - // Defensive: the fetcher already validated object shape, so this is a - // malformed-payload class failure → no-op, never write. + // Defensive: the fetcher already validated the settings map decodes, so a + // value that will not compact is a malformed-payload class failure → no-op, + // never write. return fmt.Errorf("devicepolicy: enforce: compact policy: %w", err) } @@ -256,7 +260,21 @@ func (r *Reconciler) handleEnforce(ctx context.Context, cat, tgt string, ep Effe } if mw, ok := r.Writer.(managedSettingsWriter); ok { - return r.enforceManaged(ctx, cat, tgt, ep, newValue, mw) + return r.enforceManaged(ctx, cat, tgt, ep, desired, mw) + } + // Single-key fallback: a plain Writer manages only extensions.allowed; other + // settings need the managed writer. The production settings writer is always + // managed, so this path is the fake/degraded case. + newValue, ok := desired[allowedExtensionsSettingKey] + if !ok { + _ = r.report(ctx, cat, tgt, StateVerificationFailed, "") + return fmt.Errorf("devicepolicy: enforce: settings missing %q for single-key writer", allowedExtensionsSettingKey) + } + if len(desired) > 1 { + // A multi-key policy on a single-key writer enforces only the allowlist; + // surface it so a partial-enforce is never invisible. + r.logf("devicepolicy: single-key writer at %s enforces only %s; %d other setting(s) dropped", + r.Writer.Location(), allowedExtensionsSettingKey, len(desired)-1) } return r.enforceSingle(ctx, cat, tgt, ep, newValue) } @@ -348,57 +366,51 @@ func (r *Reconciler) enforceSingle(ctx context.Context, cat, tgt string, ep Effe return r.report(ctx, cat, tgt, state, appliedHash) } -// enforceManaged is the managed multi-key convergence path. The allowlist is -// always authoritative (Set); the gallery URL is Set when the policy carries -// one, else an ownership-gated Remove (only a value the agent itself wrote), -// else preserved (a foreign or absent value is never deleted). -func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep EffectivePolicy, newValue string, mw managedSettingsWriter) error { +// enforceManaged is the managed multi-key convergence path. It is fully driven +// by the settings map: every setting the backend sent is authoritatively Set, +// and any key the agent previously owned that is NO LONGER in the map is an +// ownership-gated Remove (only a value the agent itself wrote), else preserved +// (a foreign or absent value is never deleted). No setting id is special-cased, +// so a new managed key rides through with no change here. +func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep EffectivePolicy, desired map[string]string, mw managedSettingsWriter) error { prev, hadPrev := ReadAppliedState(cat, tgt) owned := ownedKeys(prev, hadPrev) - // 1. Read the managed keys up front. - keys := managedKeys() + // 1. Read every key this cycle may touch: the union of the settings map's keys + // (to Set) and the owned keys (Set again, or an ownership-gated Remove when a + // key has left the map). Sorted so reads, convergence, and writes are + // deterministic. + keys := sortedUnion(desired, owned) cur, err := mw.ReadManaged(keys) if err != nil { _ = r.report(ctx, cat, tgt, StateVerificationFailed, "") return fmt.Errorf("devicepolicy: enforce: read %s: %w", r.Writer.Location(), err) } - // 2. Build the desired end-state ops. Allowlist is ALWAYS Set. The gallery - // op is decided from the policy URL and per-key ownership. - galValue, err := managedGalleryValue(ep.GalleryServiceURL) - if err != nil { - // Defensive: encoding a validated URL string cannot fail in practice. - // If it ever did, nothing has been written — report verification_failed - // symmetrically with the ReadManaged-error path above. - _ = r.report(ctx, cat, tgt, StateVerificationFailed, "") - return fmt.Errorf("devicepolicy: enforce: encode gallery url: %w", err) - } - desired := []settingOp{{Key: allowedExtensionsSettingKey, Set: true, Value: json.RawMessage(newValue)}} - - galKey := galleryServiceURLSettingKey - var galleryOp settingOp - switch { - case ep.GalleryServiceURL != "": - // Authoritative: set the gallery key to the policy's URL. - galleryOp = settingOp{Key: galKey, Set: true, Value: json.RawMessage(galValue)} - case owned[galKey] != "" && cur[galKey].Present && cur[galKey].Raw == owned[galKey]: - // No URL and the on-disk value is the one the agent wrote → remove it - // (ownership-gated). - galleryOp = settingOp{Key: galKey, Remove: true} - default: - // No URL and either no ownership record or a foreign value on disk → - // preserve (never delete a value the agent did not write). - galleryOp = settingOp{Key: galKey} + // 2. Build the desired end-state ops, one per key in the union: + // - present in the settings map → Set to its compiled value; + // - owned, gone from the map, and its + // on-disk value still matches → ownership-gated Remove; + // - otherwise (foreign or absent value) → preserve (never delete). + ops := make([]settingOp, 0, len(keys)) + for _, key := range keys { + if v, ok := desired[key]; ok { + ops = append(ops, settingOp{Key: key, Set: true, Value: json.RawMessage(v)}) + continue + } + if ov := owned[key]; ov != "" && cur[key].Present && cur[key].Raw == ov { + ops = append(ops, settingOp{Key: key, Remove: true}) + } else { + ops = append(ops, settingOp{Key: key}) + } } - desired = append(desired, galleryOp) // 3. Convergence over the FULL desired end-state, computed BEFORE the - // idempotency short-circuit. It covers every managed key, so gallery-only - // drift with an unchanged hash re-applies rather than short-circuiting to + // idempotency short-circuit. It covers every managed key, so drift on any one + // key with an unchanged hash re-applies rather than short-circuiting to // compliant. converged := true - for _, op := range desired { + for _, op := range ops { if !opConverged(op, cur) { converged = false break @@ -441,8 +453,8 @@ func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep Eff // 7. Write the mutating ops in one atomic patch; a preserve contributes // nothing. - writeOps := make([]settingOp, 0, len(desired)) - for _, op := range desired { + writeOps := make([]settingOp, 0, len(ops)) + for _, op := range ops { if op.Set || op.Remove { writeOps = append(writeOps, op) } @@ -463,13 +475,13 @@ func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep Eff } } - // 9. Persist ownership: every managed key the agent now OWNS, keyed by setting - // id. The allowlist is always owned (authoritative Set); a Remove or preserve - // key asserts no ownership this cycle (omitted). WrittenValue is the single-key - // path's field and is left untouched here. - ownedAfter := map[string]string{allowedExtensionsSettingKey: newValue} - if galleryOp.Set { - ownedAfter[galKey] = galValue + // 9. Persist ownership: every key the agent Set this cycle (i.e. the whole + // settings map), keyed by setting id → the exact value written. A Remove or + // preserve key asserts no ownership this cycle (omitted). WrittenValue is the + // single-key path's field and is left untouched here. + ownedAfter := make(map[string]string, len(desired)) + for key, v := range desired { + ownedAfter[key] = v } if err := r.persistState(cat, tgt, AppliedTargetState{ AppliedHash: ep.Hash, @@ -505,11 +517,51 @@ func (r *Reconciler) enforceManaged(ctx context.Context, cat, tgt string, ep Eff return r.report(ctx, cat, tgt, state, appliedHash) } -// managedKeys is the fixed, ordered set of VS Code settings.json keys the IDE -// reconciler manages. The order is stable so reads, convergence, and writes are -// deterministic. -func managedKeys() []string { - return []string{allowedExtensionsSettingKey, galleryServiceURLSettingKey} +// compactSettings compacts every value in the settings map to its canonical +// comparison form, returning setting id → compacted value. Compaction +// normalizes whitespace so on-disk readback and next-cycle ownership compare +// byte-exactly regardless of the backend's wire formatting; member order within +// each value is preserved (it is the backend's canonical, hashed order). +func compactSettings(settings map[string]json.RawMessage) (map[string]string, error) { + out := make(map[string]string, len(settings)) + for k, raw := range settings { + c, err := compactJSON(raw) + if err != nil { + return nil, fmt.Errorf("devicepolicy: compact policy key %q: %w", k, err) + } + out[k] = c + } + return out, nil +} + +// sortedUnion returns the sorted union of two key sets — every settings key a +// cycle may touch (a Set from the settings map, or an ownership-gated +// Remove/preserve for an owned key no longer in it). The stable order makes +// reads, convergence, writes, and logs deterministic. +func sortedUnion(a, b map[string]string) []string { + set := make(map[string]struct{}, len(a)+len(b)) + for k := range a { + set[k] = struct{}{} + } + for k := range b { + set[k] = struct{}{} + } + keys := make([]string, 0, len(set)) + for k := range set { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// sortedKeys returns m's keys in sorted order, for a deterministic write. +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys } // opConverged reports whether on-disk state m already satisfies op: a Set @@ -544,27 +596,6 @@ func ownedKeys(prev AppliedTargetState, hadPrev bool) map[string]string { return owned } -// managedGalleryValue returns the compacted JSON-string encoding of a gallery -// URL — the exact bytes written to settings.json AND recorded as owned, so the -// readback and the next-cycle ownership comparison are canonical against what -// ReadManaged returns. HTML escaping is disabled so the value round-trips -// byte-stably (json.Compact does not escape, and the JSONC writer preserves the -// literal string token). An empty URL yields "" (no value). -func managedGalleryValue(url string) (string, error) { - if url == "" { - return "", nil - } - var buf bytes.Buffer - enc := json.NewEncoder(&buf) - enc.SetEscapeHTML(false) - if err := enc.Encode(url); err != nil { - return "", fmt.Errorf("devicepolicy: encode gallery url: %w", err) - } - // Encode appends a newline; compact strips it (a JSON string has no other - // insignificant whitespace). - return compactJSON(buf.Bytes()) -} - // rollbackWrite restores the settings key to its pre-cycle condition after the // post-write ownership persist failed. WriteAppliedState is atomic // (temp+rename), so the failed persist left the previous state file intact — diff --git a/internal/devicepolicy/reconcile_test.go b/internal/devicepolicy/reconcile_test.go index 5e0b9cbd..06dca1a6 100644 --- a/internal/devicepolicy/reconcile_test.go +++ b/internal/devicepolicy/reconcile_test.go @@ -108,7 +108,7 @@ func policyEP(hash string) EffectivePolicy { return EffectivePolicy{ Category: CategoryIDEExtension, Clear: false, - Policy: json.RawMessage(samplePolicyWire), + Policy: map[string]json.RawMessage{allowedExtensionsSettingKey: json.RawMessage(samplePolicyWire)}, Hash: hash, } } @@ -657,9 +657,9 @@ const ( galURLB = "https://other.example/api/v1" ) -// galleryRaw is the compacted JSON-string form of a URL as it lands in -// settings.json and is recorded as owned (test URLs have no chars needing -// escaping, so this equals managedGalleryValue's output). +// galleryRaw is the JSON-string form of a URL as it rides in the settings map, +// lands in settings.json, and is recorded as owned (test URLs have no chars +// needing escaping, so compaction is a no-op). func galleryRaw(url string) string { return `"` + url + `"` } // fakeManagedWriter implements Writer AND managedSettingsWriter over an @@ -766,7 +766,7 @@ func (w *fakeManagedWriter) Location() string { return "fake://managed-settings. func policyEPGallery(hash, url string) EffectivePolicy { ep := policyEP(hash) - ep.GalleryServiceURL = url + ep.Policy[galleryServiceURLSettingKey] = json.RawMessage(galleryRaw(url)) return ep } @@ -1162,3 +1162,60 @@ func TestEnforceManagedReadbackMismatchReportsPolicyNotApplied(t *testing.T) { t.Fatalf("ownership must record the intended values even on readback mismatch, got %+v ok=%v", st, ok) } } + +func TestEnforceManagedRealWriterArbitraryThirdKey(t *testing.T) { + // End-to-end through the REAL settings.json writer (hujson), not a fake: a + // settings map with a novel third setting id — carrying a non-object, + // non-string value — must land verbatim on disk, be owned, and report + // compliant. Proves the whole reconcile→write→readback→ownership path is key- + // AND value-type-agnostic, so a future managed setting is free (no code change + // here). + withTempCache(t) + w, _ := newTestSettingsWriter(t) + + const thirdKey = "extensions.autoCheckUpdates" + ep := EffectivePolicy{ + Category: CategoryIDEExtension, + Hash: "sha256:H", + Policy: map[string]json.RawMessage{ + allowedExtensionsSettingKey: json.RawMessage(samplePolicyWire), + galleryServiceURLSettingKey: json.RawMessage(galleryRaw(galURLA)), + thirdKey: json.RawMessage("false"), + }, + } + rep := &fakeReporter{} + r := &Reconciler{ + Fetcher: &fakeFetcher{ep: ep}, Reporter: rep, Writer: w, + CustomerID: "c", DeviceID: "d", Platform: "linux", + Probe: func() (bool, string) { return false, "" }, + Now: func() time.Time { return time.Unix(0, 0).UTC() }, + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if got := lastReport(t, rep); got.State != StateCompliant || got.AppliedHash != "sha256:H" { + t.Fatalf("report = %+v, want compliant + echoed hash", got) + } + // Re-read from the real file on disk (ReadManaged does a fresh os.ReadFile), + // proving each key landed as a top-level member with the exact bytes. + cur, err := w.ReadManaged([]string{allowedExtensionsSettingKey, galleryServiceURLSettingKey, thirdKey}) + if err != nil { + t.Fatal(err) + } + if sv := cur[thirdKey]; !sv.Present || sv.Raw != "false" { + t.Fatalf("third key on disk = %+v, want present false", sv) + } + if sv := cur[allowedExtensionsSettingKey]; !sv.Present || sv.Raw != samplePolicy { + t.Fatalf("allowlist on disk = %+v, want %s", sv, samplePolicy) + } + if sv := cur[galleryServiceURLSettingKey]; !sv.Present || sv.Raw != galleryRaw(galURLA) { + t.Fatalf("gallery on disk = %+v, want %s", sv, galleryRaw(galURLA)) + } + // Ownership recorded for every setting, the third included. + st, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode) + if !ok || st.WrittenSettings[thirdKey] != "false" || + st.WrittenSettings[allowedExtensionsSettingKey] != samplePolicy || + st.WrittenSettings[galleryServiceURLSettingKey] != galleryRaw(galURLA) { + t.Fatalf("ownership = %+v ok=%v, want all three keys owned", st, ok) + } +} diff --git a/internal/devicepolicy/settings_writer_test.go b/internal/devicepolicy/settings_writer_test.go index 0676615a..3e77ac57 100644 --- a/internal/devicepolicy/settings_writer_test.go +++ b/internal/devicepolicy/settings_writer_test.go @@ -600,30 +600,33 @@ func TestApplyManagedRejectsInvalidJSONValue(t *testing.T) { } } -// TestManagedGalleryValueRoundTrips pins the ownership invariant: the value the -// reconciler writes and records (managedGalleryValue) must equal what a -// write→read round-trip returns, or ownership/convergence would churn forever. -// Includes a URL with &, =, <, > — the HTML-escaping edge. -func TestManagedGalleryValueRoundTrips(t *testing.T) { - for _, url := range []string{ - "https://mkt.example/api/v1", - "https://mkt.example/api/v1?tenant=acme&mode=strict", - "https://mkt.example/p/", +// TestGalleryValueRoundTrips pins the ownership invariant on the value path the +// reconciler now uses: the gallery URL arrives as a JSON string in the settings +// map, is compacted (compactSettings), written, and recorded as owned — and that +// value must equal what a write→read round-trip returns, or ownership / +// convergence would churn forever. Includes a URL with &, =, <, > — the +// HTML-escaping edge (canonical JSON must not HTML-escape; json.Compact and the +// JSONC writer both preserve the literal token byte-for-byte). +func TestGalleryValueRoundTrips(t *testing.T) { + for _, wire := range []string{ + `"https://mkt.example/api/v1"`, + `"https://mkt.example/api/v1?tenant=acme&mode=strict"`, + `"https://mkt.example/p/"`, } { w, _ := newTestSettingsWriter(t) - gv, err := managedGalleryValue(url) + gv, err := compactJSON([]byte(wire)) if err != nil { - t.Fatalf("managedGalleryValue(%q): %v", url, err) + t.Fatalf("compactJSON(%s): %v", wire, err) } if _, err := w.ApplyManaged([]settingOp{{Key: galleryServiceURLSettingKey, Set: true, Value: json.RawMessage(gv)}}); err != nil { - t.Fatalf("ApplyManaged(%q): %v", url, err) + t.Fatalf("ApplyManaged(%s): %v", wire, err) } got, err := w.ReadManaged([]string{galleryServiceURLSettingKey}) if err != nil { t.Fatal(err) } if sv := got[galleryServiceURLSettingKey]; !sv.Present || sv.Raw != gv { - t.Fatalf("url %q: readback Raw=%q, want %q (owned value must equal readback)", url, sv.Raw, gv) + t.Fatalf("wire %s: readback Raw=%q, want %q (owned value must equal readback)", wire, sv.Raw, gv) } } } From bbc89352eab64394632c411f187beb0a002a9fe6 Mon Sep 17 00:00:00 2001 From: Subham Ray Date: Fri, 24 Jul 2026 03:05:04 +0530 Subject: [PATCH 4/5] feat(devicepolicy): add MDM verify-only enforcement channel for IDE-extension policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enforcement selects a channel per cycle: "dmg" (or "") is the write-and-verify path; "mdm" is verify-only — the agent probes the OS-managed VS Code policy and reports what it observed, and never writes, patches, or clears settings.json (an external MDM owns the policy). - ProbeManagedContent (per-OS) reads AllowedExtensions and ExtensionGalleryServiceUrl into the observed bag keyed by VS Code setting id: - windows: registry, gating on the types VS Code honors (REG_SZ / REG_MULTI_SZ; REG_EXPAND_SZ and others are ignored, matching vscode-policy-watcher). - darwin: managed-preferences plist, machine-wide overlaid by the console user's per-user plist (user resolved via $HOME to match the settings writer). - linux: /etc/vscode/policy.json; malformed/null values are verification_failed, never a silent absence. - Compliance reports carry observed + evaluated_enforcement (the normalized, canonical channel) so the backend can diff like-for-like. - Channel routing is case/space-insensitive; unknown channels fail safe to the DMG write path. - The presence probe (DMG-mode yield) also gates on REG_SZ/REG_MULTI_SZ, so a wrong-typed policy value that VS Code ignores no longer suppresses enforcement. --- cmd/stepsecurity-dev-machine-guard/main.go | 6 +- go.mod | 1 + go.sum | 4 + internal/devicepolicy/api.go | 31 ++- internal/devicepolicy/api_test.go | 79 ++++++++ internal/devicepolicy/probe.go | 52 ++++++ internal/devicepolicy/probe_darwin.go | 103 +++++++++- internal/devicepolicy/probe_darwin_test.go | 113 ++++++++++- internal/devicepolicy/probe_linux.go | 100 ++++++++++ internal/devicepolicy/probe_linux_test.go | 79 +++++++- internal/devicepolicy/probe_other.go | 12 +- internal/devicepolicy/probe_test.go | 71 +++++++ internal/devicepolicy/probe_windows.go | 145 ++++++++++++-- internal/devicepolicy/probe_windows_test.go | 149 ++++++++++++++- internal/devicepolicy/reconcile.go | 109 ++++++++++- internal/devicepolicy/reconcile_test.go | 197 ++++++++++++++++++++ 16 files changed, 1203 insertions(+), 48 deletions(-) diff --git a/cmd/stepsecurity-dev-machine-guard/main.go b/cmd/stepsecurity-dev-machine-guard/main.go index c6213344..f705b730 100644 --- a/cmd/stepsecurity-dev-machine-guard/main.go +++ b/cmd/stepsecurity-dev-machine-guard/main.go @@ -701,8 +701,10 @@ func runIDEExtensionEnforce(exec executor.Executor, log *progress.Logger) { } writer, ok := devicepolicy.NewWriter() if !ok { - log.Debug("ide-extension enforce: skipped (no settings path on this platform)") - return + // No user-scope settings path (no home / %APPDATA%). The write path + // no-ops on a nil Writer, but verify-only (MDM) mode owns nothing on + // disk and must still probe and report — so continue, don't return. + log.Debug("ide-extension enforce: no settings path; verify-only still runs if assigned") } cfg, ok := ingest.Snapshot() if !ok { diff --git a/go.mod b/go.mod index e948ad85..dac27de1 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/tidwall/pretty v1.2.1 github.com/tidwall/sjson v1.2.5 gopkg.in/yaml.v3 v3.0.1 + howett.net/plist v1.0.1 ) require github.com/tidwall/match v1.1.1 // indirect diff --git a/go.sum b/go.sum index 7880f83e..57b2cb36 100644 --- a/go.sum +++ b/go.sum @@ -4,6 +4,7 @@ github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 h1:El6M4kTTCOh6aBiKaU github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510/go.mod h1:pupxD2MaaD3pAXIBCelhxNneeOaAeabZDe5s4K6zSpQ= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= github.com/pelletier/go-toml/v2 v2.3.1 h1:MYEvvGnQjeNkRF1qUuGolNtNExTDwct51yp7olPtrEc= github.com/pelletier/go-toml/v2 v2.3.1/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= github.com/tailscale/hujson v0.0.0-20260302212456-ecc657c15afd h1:Rf9uhF1+VJ7ZHqxrG8pJ6YacmHvVCmByDmGbAWCc/gA= @@ -22,5 +23,8 @@ golang.org/x/sys v0.33.0 h1:q3i8TbbEz+JRD9ywIRlyRAQbM0qF7hu24q3teo2hbuw= golang.org/x/sys v0.33.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v1 v1.0.0-20140924161607-9f9df34309c0/go.mod h1:WDnlLJ4WF5VGsH/HVa3CI79GS0ol3YnhVnKP89i0kNg= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +howett.net/plist v1.0.1 h1:37GdZ8tP09Q35o9ych3ehygcsL+HqKSwzctveSlarvM= +howett.net/plist v1.0.1/go.mod h1:lqaXoTrLY4hg8tnEzNru53gicrbv7rrk+2xJA/7hw9g= diff --git a/internal/devicepolicy/api.go b/internal/devicepolicy/api.go index 5120b44b..0cfb3a48 100644 --- a/internal/devicepolicy/api.go +++ b/internal/devicepolicy/api.go @@ -46,9 +46,10 @@ const maxRunConfigBytes = 4 << 20 // (re-serialization could reorder keys and break the backend's byte-exact // applied==desired check). extensions.allowed is always present; other keys // appear only when the policy sets them. Policy identity is (Category, Target); -// Target defaults to vscode for ide_extension. A zero EffectivePolicy -// (present()==false) means run-config carried no directive for this -// category/target → reconciler no-op. +// Target defaults to vscode for ide_extension. Enforcement selects the channel: +// "mdm" is verify-only (probe and report, no settings write), "dmg" or "" is the +// write-and-verify path. A zero EffectivePolicy (present()==false) means +// run-config carried no directive for this category/target → reconciler no-op. type EffectivePolicy struct { Category string Target string @@ -56,6 +57,7 @@ type EffectivePolicy struct { Policy map[string]json.RawMessage Hash string GeneratedAt string + Enforcement string } // present reports whether the backend expressed a policy directive for this @@ -76,6 +78,7 @@ type policyEnvelope struct { Policy map[string]json.RawMessage `json:"policy,omitempty"` Hash string `json:"hash,omitempty"` GeneratedAt string `json:"generated_at"` + Enforcement string `json:"enforcement,omitempty"` // "dmg" | "mdm" | "" } // Fetcher returns the effective policy for one device + category + target. @@ -201,6 +204,7 @@ func (c *HTTPFetcher) Fetch(ctx context.Context, customerID, deviceID, category, Policy: p.Policy, Hash: strings.TrimSpace(p.Hash), GeneratedAt: p.GeneratedAt, + Enforcement: strings.TrimSpace(p.Enforcement), } if ep.Category == "" { ep.Category = category @@ -250,14 +254,21 @@ func isJSONObject(raw json.RawMessage) bool { // computed on-device. It is the agent-side mirror of agent-api's // complianceReport. AppliedHash is the backend's hash echoed verbatim — never // recomputed locally — so the backend's byte-exact applied==desired check -// (which gates the `compliant` verdict) can succeed. +// (which gates the `compliant` verdict) can succeed. In verify-only (mdm) mode +// AppliedHash is empty and Observed instead carries the OS-managed policy the +// agent read, keyed by VS Code setting id (the same keys as the desired policy) +// so the backend can diff like-for-like and decide drift — the agent does not +// judge match. EvaluatedEnforcement echoes the channel the cycle ran ("", "dmg" +// or "mdm"). Observed and EvaluatedEnforcement are omitted when empty. type ComplianceReport struct { - Category string `json:"category"` - Target string `json:"target"` - State string `json:"state"` - AppliedHash string `json:"applied_hash"` - AgentVersion string `json:"agent_version"` - Platform string `json:"platform"` + Category string `json:"category"` + Target string `json:"target"` + State string `json:"state"` + AppliedHash string `json:"applied_hash"` + AgentVersion string `json:"agent_version"` + Platform string `json:"platform"` + Observed json.RawMessage `json:"observed,omitempty"` + EvaluatedEnforcement string `json:"evaluated_enforcement,omitempty"` } // Reporter submits a compliance report for one device. diff --git a/internal/devicepolicy/api_test.go b/internal/devicepolicy/api_test.go index 1b2a0a92..d819faa7 100644 --- a/internal/devicepolicy/api_test.go +++ b/internal/devicepolicy/api_test.go @@ -345,3 +345,82 @@ func TestReportDefaultsCategory(t *testing.T) { t.Fatalf("category should default to %q, got %q", CategoryIDEExtension, gotCategory) } } + +func TestFetchLiftsEnforcement(t *testing.T) { + cases := []struct { + name string + field string // spliced into the policy sub-object (empty → omitted) + want string + }{ + {"mdm", `"enforcement":"mdm",`, "mdm"}, + {"dmg", `"enforcement":"dmg",`, "dmg"}, + {"absent (older backend)", ``, ""}, + {"whitespace trimmed", `"enforcement":" mdm ",`, "mdm"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body := `{"policy":{"category":"ide_extension","target":"vscode","clear":false,` + tc.field + + `"policy":{"extensions.allowed":{"*":false}},"hash":"sha256:e","generated_at":"2026-07-23T00:00:00Z"}}` + _, f := newFetchServer(t, 200, body) + ep, err := f.Fetch(context.Background(), "cust", "dev-1", CategoryIDEExtension, TargetVSCode) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if ep.Enforcement != tc.want { + t.Fatalf("enforcement = %q, want %q", ep.Enforcement, tc.want) + } + }) + } +} + +func TestReportSerializesObservedAndEvaluatedEnforcement(t *testing.T) { + var body ComplianceReport + var raw map[string]json.RawMessage + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(b, &body) + _ = json.Unmarshal(b, &raw) + w.WriteHeader(200) + })) + t.Cleanup(srv.Close) + rep, _ := NewHTTPReporter(ingest.Config{APIEndpoint: srv.URL, APIKey: "k"}, srv.Client()) + + observed := json.RawMessage(`{"extensions.allowed":{"*":false},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}`) + if err := rep.Report(context.Background(), "cust", "dev-1", ComplianceReport{ + Category: CategoryIDEExtension, State: StateMDMManaged, Observed: observed, EvaluatedEnforcement: "mdm", + }); err != nil { + t.Fatalf("Report: %v", err) + } + if body.EvaluatedEnforcement != "mdm" { + t.Fatalf("evaluated_enforcement = %q, want mdm", body.EvaluatedEnforcement) + } + if string(body.Observed) != string(observed) { + t.Fatalf("observed = %s, want %s", body.Observed, observed) + } + if _, ok := raw["observed"]; !ok { + t.Fatal(`wire body missing "observed" key`) + } + if _, ok := raw["evaluated_enforcement"]; !ok { + t.Fatal(`wire body missing "evaluated_enforcement" key`) + } +} + +func TestReportOmitsEmptyMDMFields(t *testing.T) { + var raw map[string]json.RawMessage + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(b, &raw) + w.WriteHeader(200) + })) + t.Cleanup(srv.Close) + rep, _ := NewHTTPReporter(ingest.Config{APIEndpoint: srv.URL, APIKey: "k"}, srv.Client()) + if err := rep.Report(context.Background(), "cust", "dev-1", ComplianceReport{State: StateCompliant}); err != nil { + t.Fatalf("Report: %v", err) + } + if _, ok := raw["observed"]; ok { + t.Fatal(`empty observed must be omitted from the wire`) + } + if _, ok := raw["evaluated_enforcement"]; ok { + t.Fatal(`empty evaluated_enforcement must be omitted from the wire`) + } +} diff --git a/internal/devicepolicy/probe.go b/internal/devicepolicy/probe.go index 4be10e61..12020de7 100644 --- a/internal/devicepolicy/probe.go +++ b/internal/devicepolicy/probe.go @@ -3,6 +3,7 @@ package devicepolicy import ( "bytes" "encoding/json" + "fmt" "os" ) @@ -74,3 +75,54 @@ func fileMentionsKey(path, key string) bool { } return bytes.Contains(b, []byte(key)) } + +// buildObserved converts OS-managed policy values keyed by policy name (the +// inner text each per-OS reader unwrapped) into the observed bag keyed by VS +// Code setting id. AllowedExtensions is a stringified JSON object, parsed back +// to an object; the gallery URL is wrapped as a JSON string. A malformed value +// is an error, not a silent omission. present is true when at least one key was +// observed. +func buildObserved(raw map[string]string) (present bool, observed map[string]json.RawMessage, err error) { + observed = make(map[string]json.RawMessage, len(raw)) + if s, ok := raw[allowedExtensionsName]; ok { + v, err := parseAllowedExtensionsValue(s) + if err != nil { + return false, nil, err + } + observed[allowedExtensionsSettingKey] = v + } + if s, ok := raw[galleryServiceURLName]; ok { + v, err := galleryURLValue(s) + if err != nil { + return false, nil, err + } + observed[galleryServiceURLSettingKey] = v + } + return len(observed) > 0, observed, nil +} + +// parseAllowedExtensionsValue parses the AllowedExtensions inner JSON text into +// compacted extensions.allowed object bytes. It must be a JSON object; anything +// else is a malformed value and errors. +func parseAllowedExtensionsValue(s string) (json.RawMessage, error) { + raw := json.RawMessage(s) + if !json.Valid(raw) { + return nil, fmt.Errorf("devicepolicy: %s is not valid JSON", allowedExtensionsName) + } + if !isJSONObject(raw) { + return nil, fmt.Errorf("devicepolicy: %s is not a JSON object", allowedExtensionsName) + } + c, err := compactJSON(raw) + if err != nil { + return nil, err + } + return json.RawMessage(c), nil +} + +func galleryURLValue(s string) (json.RawMessage, error) { + b, err := json.Marshal(s) + if err != nil { + return nil, fmt.Errorf("devicepolicy: encode %s: %w", galleryServiceURLName, err) + } + return json.RawMessage(b), nil +} diff --git a/internal/devicepolicy/probe_darwin.go b/internal/devicepolicy/probe_darwin.go index 5463a92d..e802eeb7 100644 --- a/internal/devicepolicy/probe_darwin.go +++ b/internal/devicepolicy/probe_darwin.go @@ -2,7 +2,16 @@ package devicepolicy -import "path/filepath" +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "howett.net/plist" +) // darwinManagedPrefsDir holds MDM-delivered managed preferences. VS Code's // policy watcher resolves AllowedExtensions through CFPreferences on the @@ -37,3 +46,95 @@ func probeDarwinManagedPrefs(root string) (bool, string) { } return false, "" } + +// ProbeManagedContent reads the VS Code managed-preferences values the running +// user's VS Code would resolve on the com.microsoft.VSCode domain: the machine- +// wide managed-prefs plist overlaid by that user's per-user plist (per-user +// wins). It parses the plists (binary or XML) and never globs other users. +func ProbeManagedContent() (bool, map[string]json.RawMessage, error) { + return probeDarwinManagedContent(darwinManagedPrefsDir, currentManagedPrefsUser()) +} + +// probeDarwinManagedContent is ProbeManagedContent parameterized over the +// managed-preferences root and the running user's short name so tests can stage +// a fake tree. An empty user reads machine-wide managed prefs only. +func probeDarwinManagedContent(root, user string) (bool, map[string]json.RawMessage, error) { + // Merge in ascending precedence: machine-wide, then this user's per-user + // prefs (which override machine-wide for the same key). + merged := map[string]any{} + for _, path := range darwinManagedPrefsFiles(root, user) { + dict, err := readPlistDict(path) + if err != nil { + return false, nil, err + } + for k, v := range dict { + merged[k] = v + } + } + + raw := map[string]string{} + for _, name := range []string{allowedExtensionsName, galleryServiceURLName} { + v, ok := merged[name] + if !ok { + continue + } + // These are stored as string values; a non-string managed pref is + // malformed evidence. + s, ok := v.(string) + if !ok { + return false, nil, fmt.Errorf("devicepolicy: %s in managed preferences is not a string", name) + } + raw[name] = s + } + return buildObserved(raw) +} + +// darwinManagedPrefsFiles lists the managed-prefs plists to merge, in ascending +// precedence: machine-wide, then the running user's per-user file (skipped when +// user is empty). +func darwinManagedPrefsFiles(root, user string) []string { + files := []string{filepath.Join(root, darwinVSCodePlistName)} + if user != "" { + files = append(files, filepath.Join(root, user, darwinVSCodePlistName)) + } + return files +} + +// readPlistDict parses the plist at path into its top-level dictionary. An +// absent file yields (nil, nil) — no managed prefs there. A present-but- +// unreadable or non-dictionary plist is an error (verification_failed). +func readPlistDict(path string) (map[string]any, error) { + // #nosec G304 -- path is the package-constant managed-prefs location joined + // with the running user's short name (or a test fixture), never external input. + b, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("devicepolicy: read %s: %w", path, err) + } + var dict map[string]any + if _, err := plist.Unmarshal(b, &dict); err != nil { + return nil, fmt.Errorf("devicepolicy: parse %s: %w", path, err) + } + return dict, nil +} + +// currentManagedPrefsUser resolves the running user's short name for the per- +// user managed-prefs path from $HOME's base name, matching the settings writer +// (settingsPath uses os.UserHomeDir); $USER is only the fallback. Both must +// resolve the same user, else the agent could write one user's settings.json +// while probing another's managed prefs. A root LaunchDaemon runs with $HOME +// baked to the console user but no $USER (see internal/launchd), so $HOME leads. +// Empty when neither resolves → machine-wide prefs only. +func currentManagedPrefsUser() string { + if home, err := os.UserHomeDir(); err == nil { + if base := filepath.Base(home); base != "" && base != "." && base != string(filepath.Separator) { + return base + } + } + if u := strings.TrimSpace(os.Getenv("USER")); u != "" { + return u + } + return "" +} diff --git a/internal/devicepolicy/probe_darwin_test.go b/internal/devicepolicy/probe_darwin_test.go index 3cc09736..5b368293 100644 --- a/internal/devicepolicy/probe_darwin_test.go +++ b/internal/devicepolicy/probe_darwin_test.go @@ -3,15 +3,17 @@ package devicepolicy import ( + "encoding/json" "os" "path/filepath" "strings" "testing" + + "howett.net/plist" ) -// TestProbeDarwinEitherKey stages a fake managed-preferences plist and confirms -// the probe yields on AllowedExtensions OR ExtensionGalleryServiceUrl (the key -// names appear as plain ASCII runs, matching real binary plists). +// TestProbeDarwinEitherKey confirms the probe yields on AllowedExtensions OR +// ExtensionGalleryServiceUrl (key names are plain ASCII runs in binary plists). func TestProbeDarwinEitherKey(t *testing.T) { cases := []struct { name string @@ -42,8 +44,8 @@ func TestProbeDarwinEitherKey(t *testing.T) { } } -// TestProbeDarwinPerUserGalleryKey confirms the per-user glob path also detects -// the gallery key (an MDM may materialize the plist under a subdir). +// TestProbeDarwinPerUserGalleryKey covers the per-user glob path (plist under a +// subdir). func TestProbeDarwinPerUserGalleryKey(t *testing.T) { root := t.TempDir() userDir := filepath.Join(root, "alice") @@ -59,3 +61,104 @@ func TestProbeDarwinPerUserGalleryKey(t *testing.T) { t.Fatalf("per-user gallery key: want managed=true with gallery detail, got (%v, %q)", managed, detail) } } + +// writeManagedPrefsPlist writes dict as a real binary plist at path (creating +// parent dirs), so the content probe exercises the actual plist parser rather +// than a byte scan. +func writeManagedPrefsPlist(t *testing.T, path string, dict map[string]any) { + t.Helper() + b, err := plist.Marshal(dict, plist.BinaryFormat) + if err != nil { + t.Fatalf("marshal plist: %v", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, b, 0o644); err != nil { + t.Fatal(err) + } +} + +// TestProbeDarwinManagedContent parses real binary plists: per-user precedence +// over machine-wide, an absent tree as not-managed, a non-string/unparseable +// value as an error. +func TestProbeDarwinManagedContent(t *testing.T) { + t.Run("machine-wide stringified allowlist + gallery", func(t *testing.T) { + root := t.TempDir() + writeManagedPrefsPlist(t, filepath.Join(root, darwinVSCodePlistName), map[string]any{ + allowedExtensionsName: `{"*":false,"ms-python.python":"stable"}`, + galleryServiceURLName: "https://mkt.example/api/v1", + }) + present, observed, err := probeDarwinManagedContent(root, "") + if err != nil || !present { + t.Fatalf("present=%v err=%v", present, err) + } + got, _ := json.Marshal(observed) + want := `{"extensions.allowed":{"*":false,"ms-python.python":"stable"},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}` + if string(got) != want { + t.Fatalf("observed = %s, want %s", got, want) + } + }) + + t.Run("per-user overrides machine-wide", func(t *testing.T) { + root := t.TempDir() + writeManagedPrefsPlist(t, filepath.Join(root, darwinVSCodePlistName), map[string]any{ + allowedExtensionsName: `{"*":false}`, + }) + writeManagedPrefsPlist(t, filepath.Join(root, "alice", darwinVSCodePlistName), map[string]any{ + allowedExtensionsName: `{"golang.go":true}`, + }) + present, observed, err := probeDarwinManagedContent(root, "alice") + if err != nil || !present { + t.Fatalf("present=%v err=%v", present, err) + } + got, _ := json.Marshal(observed) + if want := `{"extensions.allowed":{"golang.go":true}}`; string(got) != want { + t.Fatalf("per-user should win: observed = %s, want %s", got, want) + } + }) + + t.Run("no managed prefs is absent", func(t *testing.T) { + present, observed, err := probeDarwinManagedContent(t.TempDir(), "") + if err != nil || present || len(observed) != 0 { + t.Fatalf("absent: present=%v observed=%v err=%v", present, observed, err) + } + }) + + t.Run("non-string value is verification_failed", func(t *testing.T) { + root := t.TempDir() + writeManagedPrefsPlist(t, filepath.Join(root, darwinVSCodePlistName), map[string]any{ + allowedExtensionsName: 42, + }) + if _, _, err := probeDarwinManagedContent(root, ""); err == nil { + t.Fatal("non-string AllowedExtensions must be an error") + } + }) + + t.Run("unparseable plist is verification_failed", func(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, darwinVSCodePlistName), []byte("not a plist"), 0o644); err != nil { + t.Fatal(err) + } + if _, _, err := probeDarwinManagedContent(root, ""); err == nil { + t.Fatal("unparseable plist must be an error") + } + }) +} + +// TestCurrentManagedPrefsUserPrefersHome locks $HOME as the lead signal (matching +// the writer's os.UserHomeDir), $USER as fallback: a root LaunchDaemon bakes +// $HOME to the console user but leaves $USER=root. +func TestCurrentManagedPrefsUserPrefersHome(t *testing.T) { + t.Setenv("HOME", "/Users/alice") + t.Setenv("USER", "root") + if got := currentManagedPrefsUser(); got != "alice" { + t.Fatalf("HOME=/Users/alice USER=root: got %q, want alice", got) + } + + t.Setenv("HOME", "") + t.Setenv("USER", "bob") + if got := currentManagedPrefsUser(); got != "bob" { + t.Fatalf("empty HOME falls back to USER: got %q, want bob", got) + } +} diff --git a/internal/devicepolicy/probe_linux.go b/internal/devicepolicy/probe_linux.go index 17710ee7..5d0dc716 100644 --- a/internal/devicepolicy/probe_linux.go +++ b/internal/devicepolicy/probe_linux.go @@ -2,6 +2,13 @@ package devicepolicy +import ( + "encoding/json" + "errors" + "fmt" + "os" +) + // linuxPolicyFilePath is VS Code's managed-policy file on Linux, read by its // FilePolicyService. World-readable when an MDM/admin creates it, so the probe // needs no elevation. @@ -23,3 +30,96 @@ func probeLinuxPolicyFile(path string) (bool, string) { } return false, "" } + +// ProbeManagedContent reads the values of the Linux VS Code managed-policy file +// for the verify-only path. +func ProbeManagedContent() (bool, map[string]json.RawMessage, error) { + return probeLinuxPolicyContent(linuxPolicyFilePath) +} + +// probeLinuxPolicyContent reads path (…/policy.json) and extracts the two VS +// Code policy values. An absent file is a clean "not managed"; a present-but- +// unreadable or non-JSON file is an error, never a silent absence. +// AllowedExtensions is accepted as a stringified JSON object (as exported) or a +// direct object (hand-authored); the gallery URL as a JSON string. +func probeLinuxPolicyContent(path string) (bool, map[string]json.RawMessage, error) { + // #nosec G304 -- path is the package-constant policy location (or a test + // fixture), never external input. + b, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return false, nil, nil + } + if err != nil { + return false, nil, fmt.Errorf("devicepolicy: read %s: %w", path, err) + } + if isJSONNull(b) { + // A literal `null` document unmarshals into an empty map with no error; it + // is malformed policy evidence, not "no policy present". + return false, nil, fmt.Errorf("devicepolicy: %s is a null document, not a policy object", path) + } + m := map[string]json.RawMessage{} + if err := json.Unmarshal(b, &m); err != nil { + return false, nil, fmt.Errorf("devicepolicy: %s is not valid JSON: %w", path, err) + } + raw := map[string]string{} + if v, ok := m[allowedExtensionsName]; ok { + s, err := allowedInnerText(v) + if err != nil { + return false, nil, err + } + raw[allowedExtensionsName] = s + } + if v, ok := m[galleryServiceURLName]; ok { + s, err := jsonStringValue(v, galleryServiceURLName) + if err != nil { + return false, nil, err + } + raw[galleryServiceURLName] = s + } + return buildObserved(raw) +} + +// allowedInnerText returns the inner JSON text of an AllowedExtensions value: a +// direct object is used as-is (tolerating a hand-authored file), a JSON string +// is decoded to its contents (the exported shape). buildObserved rejects any +// non-object result. +func allowedInnerText(raw json.RawMessage) (string, error) { + if isJSONNull(raw) { + return "", fmt.Errorf("devicepolicy: %s is null, not a JSON object or string", allowedExtensionsName) + } + if isJSONObject(raw) { + return string(raw), nil + } + var s string + if err := json.Unmarshal(raw, &s); err != nil { + return "", fmt.Errorf("devicepolicy: %s is neither a JSON object nor a JSON string", allowedExtensionsName) + } + return s, nil +} + +// jsonStringValue decodes raw as a JSON string, erroring on any other JSON type. +// A JSON null is rejected explicitly: json.Unmarshal leaves the empty string +// with no error, which would turn malformed evidence into an empty URL. +func jsonStringValue(raw json.RawMessage, name string) (string, error) { + if isJSONNull(raw) { + return "", fmt.Errorf("devicepolicy: %s is null, not a JSON string", name) + } + var s string + if err := json.Unmarshal(raw, &s); err != nil { + return "", fmt.Errorf("devicepolicy: %s is not a JSON string: %w", name, err) + } + return s, nil +} + +// isJSONNull reports whether raw is the JSON null literal. Callers pass +// syntactically valid JSON, so a leading `n` can only be null. +func isJSONNull(raw json.RawMessage) bool { + for _, b := range raw { + switch b { + case ' ', '\t', '\r', '\n': + continue + } + return b == 'n' + } + return false +} diff --git a/internal/devicepolicy/probe_linux_test.go b/internal/devicepolicy/probe_linux_test.go index a09f869c..2f157660 100644 --- a/internal/devicepolicy/probe_linux_test.go +++ b/internal/devicepolicy/probe_linux_test.go @@ -3,14 +3,13 @@ package devicepolicy import ( + "encoding/json" "os" "path/filepath" "strings" "testing" ) -// TestProbeLinuxEitherKey stages a fake /etc/vscode/policy.json and confirms -// the probe yields on AllowedExtensions OR ExtensionGalleryServiceUrl. func TestProbeLinuxEitherKey(t *testing.T) { cases := []struct { name string @@ -42,3 +41,79 @@ func TestProbeLinuxEitherKey(t *testing.T) { }) } } + +// AllowedExtensions may be stringified or a direct object; an absent file is +// not-managed, a present-but-malformed one an error. +func TestProbeLinuxPolicyContent(t *testing.T) { + cases := []struct { + name string + content string + writeIt bool + present bool + want string + wantErr bool + }{ + { + name: "stringified allowlist", + content: `{"AllowedExtensions":"{\"*\":false,\"ms-python.python\":\"stable\"}"}`, + writeIt: true, present: true, + want: `{"extensions.allowed":{"*":false,"ms-python.python":"stable"}}`, + }, + { + name: "direct-object allowlist (hand-authored)", + content: `{"AllowedExtensions":{"*":false}}`, + writeIt: true, present: true, + want: `{"extensions.allowed":{"*":false}}`, + }, + { + name: "gallery only", + content: `{"ExtensionGalleryServiceUrl":"https://mkt.example/api/v1"}`, + writeIt: true, present: true, + want: `{"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}`, + }, + { + name: "both", + content: `{"AllowedExtensions":{"golang.go":true},"ExtensionGalleryServiceUrl":"https://mkt.example/api/v1"}`, + writeIt: true, present: true, + want: `{"extensions.allowed":{"golang.go":true},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}`, + }, + {name: "neither key", content: `{"Other":1}`, writeIt: true, present: false}, + {name: "absent file", writeIt: false, present: false}, + {name: "malformed json file", content: `{oops`, writeIt: true, wantErr: true}, + {name: "allowlist wrong type", content: `{"AllowedExtensions":123}`, writeIt: true, wantErr: true}, + {name: "stringified but not an object", content: `{"AllowedExtensions":"[1,2]"}`, writeIt: true, wantErr: true}, + {name: "gallery null is malformed", content: `{"ExtensionGalleryServiceUrl":null}`, writeIt: true, wantErr: true}, + {name: "allowlist null is malformed", content: `{"AllowedExtensions":null}`, writeIt: true, wantErr: true}, + {name: "top-level null document", content: `null`, writeIt: true, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "policy.json") + if tc.writeIt { + if err := os.WriteFile(path, []byte(tc.content), 0o644); err != nil { + t.Fatal(err) + } + } + present, observed, err := probeLinuxPolicyContent(path) + if tc.wantErr { + if err == nil { + t.Fatalf("want error, got present=%v observed=%v", present, observed) + } + return + } + if err != nil { + t.Fatalf("probeLinuxPolicyContent: %v", err) + } + if present != tc.present { + t.Fatalf("present = %v, want %v", present, tc.present) + } + if !tc.present { + return + } + got, _ := json.Marshal(observed) + if string(got) != tc.want { + t.Fatalf("observed = %s, want %s", got, tc.want) + } + }) + } +} diff --git a/internal/devicepolicy/probe_other.go b/internal/devicepolicy/probe_other.go index dcd88160..8b8ca7be 100644 --- a/internal/devicepolicy/probe_other.go +++ b/internal/devicepolicy/probe_other.go @@ -2,8 +2,12 @@ package devicepolicy -// ProbeManagedPolicy: no known VS Code policy location on this OS. The -// platform also has no settings writer (settingsPath returns ok=false), so -// enforcement never runs here — this exists only to keep the package -// compiling on every GOOS. +import "encoding/json" + +// ProbeManagedPolicy and ProbeManagedContent have no VS Code policy location to +// read on this OS. The platform also has no settings writer (settingsPath +// returns ok=false), so enforcement never runs here — these exist only to keep +// the package compiling on every GOOS. func ProbeManagedPolicy() (bool, string) { return false, "" } + +func ProbeManagedContent() (bool, map[string]json.RawMessage, error) { return false, nil, nil } diff --git a/internal/devicepolicy/probe_test.go b/internal/devicepolicy/probe_test.go index d839158e..c5d7c465 100644 --- a/internal/devicepolicy/probe_test.go +++ b/internal/devicepolicy/probe_test.go @@ -1,6 +1,7 @@ package devicepolicy import ( + "encoding/json" "os" "path/filepath" "testing" @@ -93,3 +94,73 @@ func TestProbeHelpersDetectEitherPolicyName(t *testing.T) { t.Fatalf("managedPolicyNames = %v", names) } } + +// TestBuildObserved covers the shared policy-name → setting-id translation the +// per-OS content probes funnel through, including malformed values as errors. +func TestBuildObserved(t *testing.T) { + cases := []struct { + name string + raw map[string]string + present bool + want string // marshaled observed (map keys sort); "" when !present + wantErr bool + }{ + { + name: "allowlist only", + raw: map[string]string{allowedExtensionsName: `{"*":false,"ms-python.python":"stable"}`}, + present: true, + want: `{"extensions.allowed":{"*":false,"ms-python.python":"stable"}}`, + }, + { + name: "allowlist whitespace normalized", + raw: map[string]string{allowedExtensionsName: "{ \"*\" : false }"}, + present: true, + want: `{"extensions.allowed":{"*":false}}`, + }, + { + name: "gallery only wrapped as JSON string", + raw: map[string]string{galleryServiceURLName: "https://mkt.example/api/v1"}, + present: true, + want: `{"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}`, + }, + { + name: "both", + raw: map[string]string{ + allowedExtensionsName: `{"*":false}`, + galleryServiceURLName: "https://mkt.example/api/v1", + }, + present: true, + want: `{"extensions.allowed":{"*":false},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}`, + }, + {name: "neither", raw: map[string]string{}, present: false}, + {name: "allowlist is an array not an object", raw: map[string]string{allowedExtensionsName: `["a","b"]`}, wantErr: true}, + {name: "allowlist is not valid JSON", raw: map[string]string{allowedExtensionsName: `{oops`}, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + present, observed, err := buildObserved(tc.raw) + if tc.wantErr { + if err == nil { + t.Fatalf("want error, got observed=%v", observed) + } + return + } + if err != nil { + t.Fatalf("buildObserved: %v", err) + } + if present != tc.present { + t.Fatalf("present = %v, want %v", present, tc.present) + } + if !tc.present { + return + } + got, err := json.Marshal(observed) + if err != nil { + t.Fatal(err) + } + if string(got) != tc.want { + t.Fatalf("observed = %s, want %s", got, tc.want) + } + }) + } +} diff --git a/internal/devicepolicy/probe_windows.go b/internal/devicepolicy/probe_windows.go index 5a11626f..423e224f 100644 --- a/internal/devicepolicy/probe_windows.go +++ b/internal/devicepolicy/probe_windows.go @@ -3,7 +3,10 @@ package devicepolicy import ( + "encoding/json" "errors" + "fmt" + "strings" "golang.org/x/sys/windows/registry" ) @@ -26,10 +29,10 @@ type registryProbe struct { } // ProbeManagedPolicy reports whether an AllowedExtensions or -// ExtensionGalleryServiceUrl value of ANY registry type exists under the VS -// Code policy key in HKLM or HKCU. Type does not matter: VS Code's policy -// service claims the setting as soon as the value exists, so a wrong-typed -// value still outranks user settings. +// ExtensionGalleryServiceUrl value of a type VS Code honors (REG_SZ or +// REG_MULTI_SZ) exists under the VS Code policy key in HKLM or HKCU. A value of +// any other type is dropped by vscode-policy-watcher (nullopt) and does not +// outrank user settings, so it does not count as managed. func ProbeManagedPolicy() (bool, string) { return probeRegistryLocations([]registryProbe{ {registry.LOCAL_MACHINE, "HKLM", windowsPolicyKeyPath}, @@ -58,16 +61,134 @@ func probeRegistry(loc registryProbe) (bool, string) { } defer k.Close() - // GetValue with a nil buffer asks only for existence/metadata. Any error - // other than ErrNotExist still proves the value exists or the key is - // unreadable; only a clean not-exists reads as unmanaged. Check each managed - // policy name; the first present one wins (AllowedExtensions preferred in - // the detail). + // A value counts as managed only when its type is one VS Code honors (REG_SZ + // or REG_MULTI_SZ). A value of any other type is dropped by + // vscode-policy-watcher (nullopt) and does not outrank user settings, so skip + // it and keep scanning the remaining names (and, via the caller, hives). The + // first honored value wins (AllowedExtensions preferred in the detail). for _, name := range managedPolicyNames() { - if _, _, err := k.GetValue(name, nil); errors.Is(err, registry.ErrNotExist) { - continue + if _, valtype, err := k.GetValue(name, nil); isHonoredStringType(valtype, err) { + return true, loc.name + `\` + loc.path + ` [` + name + `]` } - return true, loc.name + `\` + loc.path + ` [` + name + `]` } return false, "" } + +// isHonoredStringType reports whether a GetValue(name, nil) result denotes a +// present value of a type VS Code honors for a string/object policy (REG_SZ or +// REG_MULTI_SZ). A nil probe buffer sets valtype and returns ErrShortBuffer (or +// nil); a clean ErrNotExist — or any other error — is not a honored value. +func isHonoredStringType(valtype uint32, err error) bool { + if err != nil && !errors.Is(err, registry.ErrShortBuffer) { + return false + } + return valtype == registry.SZ || valtype == registry.MULTI_SZ +} + +// keyStatus classifies one registry lookup for a single policy value. +type keyStatus int + +const ( + keyAbsent keyStatus = iota // value not set at this hive + keyFound // value present and read as a supported string type + keyUnreadable // value present but not a type VS Code honors, or unreadable +) + +// ProbeManagedContent reads the VS Code managed-policy values from the registry +// for the verify-only path. Each value is resolved with its own HKLM-then-HKCU +// fallback (values may live in different hives), reading only the string types +// VS Code's policy watcher honors. +func ProbeManagedContent() (bool, map[string]json.RawMessage, error) { + return probeRegistryContent([]registryProbe{ + {registry.LOCAL_MACHINE, "HKLM", windowsPolicyKeyPath}, + {registry.CURRENT_USER, "HKCU", windowsPolicyKeyPath}, + }) +} + +// probeRegistryContent resolves each policy value independently over locs. A +// value present at the effective hive but of an unsupported type — or otherwise +// unreadable — is a verification_failed for that key (never a silent absence); +// an unsupported HKLM value never masks a valid HKCU one. +func probeRegistryContent(locs []registryProbe) (bool, map[string]json.RawMessage, error) { + raw := map[string]string{} + for _, name := range []string{allowedExtensionsName, galleryServiceURLName} { + v, st := resolveRegistryPolicy(name, locs) + switch st { + case keyFound: + raw[name] = v + case keyUnreadable: + return false, nil, fmt.Errorf("devicepolicy: %s present but not a readable string policy", name) + case keyAbsent: + // MDM did not set this key; leave it out of observed. + } + } + return buildObserved(raw) +} + +// resolveRegistryPolicy reads one policy value across the hives in order and +// returns the first readable string value (keyFound). If no hive holds a +// readable value but at least one held a present-but-unsupported/unreadable +// value, it returns keyUnreadable; otherwise keyAbsent. +func resolveRegistryPolicy(name string, locs []registryProbe) (string, keyStatus) { + sawUnreadable := false + for _, l := range locs { + v, st := readRegistryStringValue(l.root, l.path, name) + switch st { + case keyFound: + return v, keyFound + case keyUnreadable: + sawUnreadable = true // must not mask a valid value in a later hive + case keyAbsent: + } + } + if sawUnreadable { + return "", keyUnreadable + } + return "", keyAbsent +} + +// readRegistryStringValue reads one value at one hive, accepting only the +// registry types vscode-policy-watcher honors for a string/object policy: REG_SZ +// and REG_MULTI_SZ (its StringPolicy supportedTypes). REG_EXPAND_SZ and every +// other type are dropped by VS Code (RegistryPolicy returns nullopt), so +// reporting one would claim a value VS Code does not apply — they map to +// keyUnreadable, never keyFound. A missing key or value is keyAbsent; a present +// value of an unhonored type, or a read error on a present value, is +// keyUnreadable (never a silent absence). +func readRegistryStringValue(root registry.Key, path, name string) (string, keyStatus) { + k, err := registry.OpenKey(root, path, registry.QUERY_VALUE) + if err != nil { + // Key absent at this hive (no policy) or unreadable; treat as absent here + // and let the other hive decide. + return "", keyAbsent + } + defer k.Close() + + // Gate on the exact type: GetStringValue also accepts REG_EXPAND_SZ, which VS + // Code does not, so query the type ourselves rather than trust the getter. A + // nil probe buffer sets valtype and returns ErrShortBuffer (or nil); only a + // clean not-exists reads as absent. + _, valtype, err := k.GetValue(name, nil) + switch { + case errors.Is(err, registry.ErrNotExist): + return "", keyAbsent + case err != nil && !errors.Is(err, registry.ErrShortBuffer): + return "", keyUnreadable + } + + switch valtype { + case registry.SZ: + if s, _, gErr := k.GetStringValue(name); gErr == nil { + return s, keyFound + } + return "", keyUnreadable + case registry.MULTI_SZ: + if lines, _, gErr := k.GetStringsValue(name); gErr == nil { + return strings.Join(lines, "\n"), keyFound + } + return "", keyUnreadable + default: + // REG_EXPAND_SZ, REG_DWORD, REG_BINARY, … — not a type VS Code honors. + return "", keyUnreadable + } +} diff --git a/internal/devicepolicy/probe_windows_test.go b/internal/devicepolicy/probe_windows_test.go index 4b41d873..56c8e3a7 100644 --- a/internal/devicepolicy/probe_windows_test.go +++ b/internal/devicepolicy/probe_windows_test.go @@ -3,6 +3,7 @@ package devicepolicy import ( + "encoding/json" "strings" "testing" @@ -33,6 +34,21 @@ func stageTestPolicyKey(t *testing.T) registry.Key { return k } +// stageTestPolicyKeyAt creates a disposable HKCU key at an arbitrary path (for +// the multi-hive fallback tests) and deletes it on cleanup. +func stageTestPolicyKeyAt(t *testing.T, path string) registry.Key { + t.Helper() + k, _, err := registry.CreateKey(registry.CURRENT_USER, path, registry.SET_VALUE|registry.QUERY_VALUE) + if err != nil { + t.Fatalf("create test key %s: %v", path, err) + } + t.Cleanup(func() { + k.Close() + _ = registry.DeleteKey(registry.CURRENT_USER, path) + }) + return k +} + func TestProbeRegistry(t *testing.T) { // Absent key → not managed. if managed, _ := probeRegistry(hkcuProbe("HKCU", testPolicyKeyPath)); managed { @@ -55,15 +71,33 @@ func TestProbeRegistry(t *testing.T) { t.Fatalf("string value present: want managed=true with HKCU detail, got (%v, %q)", managed, detail) } - // Wrong-typed value still counts: VS Code claims the setting on existence. + // Wrong-typed value does NOT count: vscode-policy-watcher honors only REG_SZ + // and REG_MULTI_SZ, so a DWORD is dropped (nullopt) and does not outrank user + // settings. if err := k.DeleteValue(allowedExtensionsName); err != nil { t.Fatal(err) } if err := k.SetDWordValue(allowedExtensionsName, 1); err != nil { t.Fatal(err) } + if managed, _ := probeRegistry(hkcuProbe("HKCU", testPolicyKeyPath)); managed { + t.Fatal("dword value: want managed=false (VS Code ignores non-string types)") + } + + // REG_EXPAND_SZ is likewise not honored. + if err := k.SetExpandStringValue(allowedExtensionsName, `%APPDATA%\Code`); err != nil { + t.Fatal(err) + } + if managed, _ := probeRegistry(hkcuProbe("HKCU", testPolicyKeyPath)); managed { + t.Fatal("expand-sz value: want managed=false") + } + + // REG_MULTI_SZ IS honored. + if err := k.SetStringsValue(allowedExtensionsName, []string{`{"a":true}`}); err != nil { + t.Fatal(err) + } if managed, _ := probeRegistry(hkcuProbe("HKCU", testPolicyKeyPath)); !managed { - t.Fatal("dword value present: want managed=true") + t.Fatal("multi_sz value: want managed=true") } // Gallery-only: remove AllowedExtensions, set ExtensionGalleryServiceUrl → @@ -113,4 +147,115 @@ func TestProbeRegistryLocationsFallsBackToSecond(t *testing.T) { }); managed { t.Fatal("all locations missing: want managed=false") } + + // A present-but-unsupported-type value in the first location must NOT count: + // the probe skips it and finds the honored value in the second (HKLM→HKCU). + wrong := stageTestPolicyKeyAt(t, testPolicyKeyPath+`\WrongType`) + if err := wrong.SetDWordValue(allowedExtensionsName, 1); err != nil { + t.Fatal(err) + } + managed, detail = probeRegistryLocations([]registryProbe{ + hkcuProbe("FIRST", testPolicyKeyPath+`\WrongType`), + hkcuProbe("SECOND", testPolicyKeyPath), // holds the valid REG_SZ set above + }) + if !managed || !strings.HasPrefix(detail, `SECOND\`) { + t.Fatalf("unsupported first location must not mask valid second: got (%v, %q)", managed, detail) + } +} + +// An absent key is a clean absence; a present-but-unsupported-type value is an +// error (never a silent absence). +func TestProbeRegistryContent(t *testing.T) { + locs := []registryProbe{hkcuProbe("HKCU", testPolicyKeyPath)} + + // Absent policy key → present=false, no error. + if present, observed, err := probeRegistryContent(locs); err != nil || present || len(observed) != 0 { + t.Fatalf("absent: got present=%v observed=%v err=%v", present, observed, err) + } + + k := stageTestPolicyKey(t) + // AllowedExtensions as a stringified JSON object (REG_SZ) + gallery URL. + if err := k.SetStringValue(allowedExtensionsName, `{"*":false,"ms-python.python":"stable"}`); err != nil { + t.Fatal(err) + } + if err := k.SetStringValue(galleryServiceURLName, "https://mkt.example/api/v1"); err != nil { + t.Fatal(err) + } + present, observed, err := probeRegistryContent(locs) + if err != nil || !present { + t.Fatalf("present: got present=%v err=%v", present, err) + } + got, _ := json.Marshal(observed) + want := `{"extensions.allowed":{"*":false,"ms-python.python":"stable"},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}` + if string(got) != want { + t.Fatalf("observed = %s, want %s", got, want) + } + + // REG_MULTI_SZ is honored too (lines joined). + if err := k.DeleteValue(galleryServiceURLName); err != nil { + t.Fatal(err) + } + if err := k.SetStringsValue(galleryServiceURLName, []string{"https://mkt.example/api/v1"}); err != nil { + t.Fatal(err) + } + if present, observed, err := probeRegistryContent(locs); err != nil || !present || len(observed[galleryServiceURLSettingKey]) == 0 { + t.Fatalf("multi_sz gallery: present=%v observed=%v err=%v", present, observed, err) + } + + // Unsupported type (DWORD) present at the winning hive → verification_failed. + if err := k.DeleteValue(allowedExtensionsName); err != nil { + t.Fatal(err) + } + if err := k.SetDWordValue(allowedExtensionsName, 1); err != nil { + t.Fatal(err) + } + if _, _, err := probeRegistryContent(locs); err == nil { + t.Fatal("unsupported-type AllowedExtensions must be verification_failed (error)") + } + + // REG_EXPAND_SZ is NOT honored by vscode-policy-watcher (supportedTypes is + // {REG_SZ, REG_MULTI_SZ}), so a value of that type must not read as applied. + if err := k.DeleteValue(allowedExtensionsName); err != nil { + t.Fatal(err) + } + if err := k.SetExpandStringValue(allowedExtensionsName, `%APPDATA%\Code`); err != nil { + t.Fatal(err) + } + if _, _, err := probeRegistryContent(locs); err == nil { + t.Fatal("REG_EXPAND_SZ AllowedExtensions must be verification_failed (VS Code ignores it)") + } +} + +// TestResolveRegistryPolicyFallbackAndMasking proves each value resolves with +// its own HKLM→HKCU fallback and that an unsupported-type value never masks a +// valid value in a later hive. +func TestResolveRegistryPolicyFallbackAndMasking(t *testing.T) { + _ = stageTestPolicyKey(t) // owns cleanup of the shared parent key + first := stageTestPolicyKeyAt(t, testPolicyKeyPath+`\First`) + second := stageTestPolicyKeyAt(t, testPolicyKeyPath+`\Second`) + + // First hive holds an unsupported type; second holds a valid string. + if err := first.SetDWordValue(allowedExtensionsName, 1); err != nil { + t.Fatal(err) + } + if err := second.SetStringValue(allowedExtensionsName, `{"*":false}`); err != nil { + t.Fatal(err) + } + locs := []registryProbe{ + hkcuProbe("FIRST", testPolicyKeyPath+`\First`), + hkcuProbe("SECOND", testPolicyKeyPath+`\Second`), + } + if v, st := resolveRegistryPolicy(allowedExtensionsName, locs); st != keyFound || v != `{"*":false}` { + t.Fatalf("unsupported first hive must not mask valid second: got st=%v v=%q", st, v) + } + + // Only an unsupported-type value anywhere → keyUnreadable. + if _, st := resolveRegistryPolicy(allowedExtensionsName, locs[:1]); st != keyUnreadable { + t.Fatalf("want keyUnreadable, got %v", st) + } + + // Absent everywhere → keyAbsent. + if _, st := resolveRegistryPolicy(galleryServiceURLName, locs); st != keyAbsent { + t.Fatalf("want keyAbsent, got %v", st) + } } diff --git a/internal/devicepolicy/reconcile.go b/internal/devicepolicy/reconcile.go index 337587c0..6583bde6 100644 --- a/internal/devicepolicy/reconcile.go +++ b/internal/devicepolicy/reconcile.go @@ -6,9 +6,19 @@ import ( "errors" "fmt" "sort" + "strings" "time" ) +// enforcementDMG and enforcementMDM are the enforcement channels carried in +// ep.Enforcement. DMG (or empty) is the write-and-verify path; MDM is +// verify-only — the agent probes the OS-managed policy and reports what it +// observed, and never writes, patches, or clears settings.json. +const ( + enforcementDMG = "dmg" + enforcementMDM = "mdm" +) + // Reconciler converges the user-scope VS Code settings.json to the backend's // effective policy for one device, once per scheduled cycle. It is OS-agnostic: // the settings Writer, the managed-policy Probe, the policy Fetcher, and the @@ -35,6 +45,11 @@ type Reconciler struct { // inject a stub so results never depend on the host machine. Probe func() (managed bool, detail string) + // ProbeContent reads the OS-managed VS Code policy values (keyed by setting + // id) for the verify-only path. nil → ProbeManagedContent (the per-OS + // implementation); tests inject a stub. The DMG path never calls it. + ProbeContent func() (present bool, observed map[string]json.RawMessage, err error) + // Now and Logf are optional seams. Now defaults to time.Now().UTC; Logf to a // no-op. Now func() time.Time @@ -44,6 +59,11 @@ type Reconciler struct { // (WriteAppliedState / ClearAppliedState). nil → the real implementation. writeState func(category, target string, s AppliedTargetState) error clearState func(category, target string) error + + // enforcement is the normalized channel the current cycle ran (lower-cased, + // trimmed ep.Enforcement), stamped onto every report as EvaluatedEnforcement + // so it matches the backend's exact-match gate. Per-cycle scratch. + enforcement string } func (r *Reconciler) persistState(cat, tgt string, s AppliedTargetState) error { @@ -94,11 +114,22 @@ func (r *Reconciler) probe() (bool, string) { return ProbeManagedPolicy() } +func (r *Reconciler) probeContent() (bool, map[string]json.RawMessage, error) { + if r.ProbeContent != nil { + return r.ProbeContent() + } + return ProbeManagedContent() +} + // Reconcile runs one enforcement cycle. It NEVER panics into the caller's hot // path; failures are returned for logging. The contract: // // - fetch error (transport / non-200 / malformed) → NO-OP, error returned. // Enforcement on disk is never wiped on a transient or malformed response. +// - MDM enforcement (ep.Enforcement=="mdm") → verify-only: probe the OS-managed +// policy and report what was observed; never writes or clears settings.json. +// Checked right after fetch, before the gates below, so it runs even with a +// nil Writer or a clear directive. // - platform not enforceable (nil Writer) → silent no-op. // - absent policy (run-config carried no `policy` directive for this // category/target) → silent no-op; the on-disk value and ownership record @@ -120,6 +151,20 @@ func (r *Reconciler) Reconcile(ctx context.Context) error { // Malformed/transient: do nothing. The on-disk policy (if any) stands. return fmt.Errorf("devicepolicy: fetch: %w", err) } + // Normalize once for routing AND reporting: the backend gates on an exact + // "mdm"/"dmg", so EvaluatedEnforcement must carry the canonical value, not + // whatever casing/spacing arrived. + r.enforcement = strings.ToLower(strings.TrimSpace(ep.Enforcement)) + + // MDM is verify-only and owns nothing on disk, so it routes before the + // Writer/clear checks below. + switch r.enforcement { + case enforcementMDM: + return r.verifyMDM(ctx, cat, tgt, ep) + case enforcementDMG, "": + default: + r.logf("devicepolicy: unknown enforcement %q; running DMG path", ep.Enforcement) + } if r.Writer == nil { r.logf("devicepolicy: no settings path on this platform; skipping (category=%s target=%s)", cat, tgt) @@ -140,6 +185,42 @@ func (r *Reconciler) Reconcile(ctx context.Context) error { return r.handleEnforce(ctx, cat, tgt, ep) } +// verifyMDM is the verify-only path: it never opens settings.json (an external +// MDM owns the VS Code policy), probes the OS-managed policy, and reports what +// it saw. It does not compare observed against ep.Policy or decide drift — the +// backend does that on read — and applied_hash is always empty. +func (r *Reconciler) verifyMDM(ctx context.Context, cat, tgt string, ep EffectivePolicy) error { + if ep.Clear || !ep.present() { + r.logf("devicepolicy: mdm enforcement with no active policy for %s/%s; nothing to verify", cat, tgt) + return nil + } + + present, observed, err := r.probeContent() + if err != nil { + r.logf("devicepolicy: mdm content probe failed: %v → verification_failed", err) + return r.sendReport(ctx, ComplianceReport{Category: cat, Target: tgt, State: StateVerificationFailed}) + } + + if !present { + r.logf("devicepolicy: mdm enforcement but no managed VS Code policy present → policy_not_applied") + return r.sendReport(ctx, ComplianceReport{Category: cat, Target: tgt, State: StatePolicyNotApplied}) + } + + raw, err := json.Marshal(observed) + if err != nil { + // observed is built from our own parsers; a marshal failure is not expected. + r.logf("devicepolicy: mdm marshal observed failed: %v → verification_failed", err) + return r.sendReport(ctx, ComplianceReport{Category: cat, Target: tgt, State: StateVerificationFailed}) + } + r.logf("devicepolicy: mdm managed policy present → mdm_managed (%d observed key(s))", len(observed)) + return r.sendReport(ctx, ComplianceReport{ + Category: cat, + Target: tgt, + State: StateMDMManaged, + Observed: raw, + }) +} + // handleClear removes the agent-owned settings on unassignment, then drops the // ownership record. It dispatches on the Writer: a managed multi-key writer // clears each owned key independently (clearManaged); any other Writer keeps @@ -614,21 +695,29 @@ func (r *Reconciler) rollbackWrite(prevOnDisk string, prevPresent bool) { } } +// report submits a write-path compliance report. func (r *Reconciler) report(ctx context.Context, cat, tgt, state, appliedHash string) error { - r.logf("devicepolicy: reporting state=%s category=%s target=%s", state, cat, tgt) + return r.sendReport(ctx, ComplianceReport{ + Category: cat, + Target: tgt, + State: state, + AppliedHash: appliedHash, + }) +} + +// sendReport stamps the shared fields (agent version, platform, +// EvaluatedEnforcement) and submits. Callers fill Category/Target/State and the +// lane-specific field: AppliedHash for the write path, Observed for MDM. +func (r *Reconciler) sendReport(ctx context.Context, rep ComplianceReport) error { + rep.AgentVersion = AgentVersion() + rep.Platform = r.Platform + rep.EvaluatedEnforcement = r.enforcement + r.logf("devicepolicy: reporting state=%s category=%s target=%s", rep.State, rep.Category, rep.Target) if r.Reporter == nil { return nil } - rep := ComplianceReport{ - Category: cat, - Target: tgt, - State: state, - AppliedHash: appliedHash, - AgentVersion: AgentVersion(), - Platform: r.Platform, - } if err := r.Reporter.Report(ctx, r.CustomerID, r.DeviceID, rep); err != nil { - return fmt.Errorf("devicepolicy: report %s: %w", state, err) + return fmt.Errorf("devicepolicy: report %s: %w", rep.State, err) } return nil } diff --git a/internal/devicepolicy/reconcile_test.go b/internal/devicepolicy/reconcile_test.go index 06dca1a6..b86abfdd 100644 --- a/internal/devicepolicy/reconcile_test.go +++ b/internal/devicepolicy/reconcile_test.go @@ -1219,3 +1219,200 @@ func TestEnforceManagedRealWriterArbitraryThirdKey(t *testing.T) { t.Fatalf("ownership = %+v ok=%v, want all three keys owned", st, ok) } } + +// --- MDM verify-only path (ep.Enforcement=="mdm") -------------------------- + +func mdmEP(hash string) EffectivePolicy { + ep := policyEP(hash) + ep.Enforcement = enforcementMDM + return ep +} + +// sampleObserved is a parsed observed bag as ProbeManagedContent would return. +func sampleObserved() map[string]json.RawMessage { + return map[string]json.RawMessage{ + allowedExtensionsSettingKey: json.RawMessage(`{"*":false,"ms-python.python":"stable"}`), + galleryServiceURLSettingKey: json.RawMessage(`"https://mkt.example/api/v1"`), + } +} + +func TestReconcileMDMReportsManagedWithObserved(t *testing.T) { + w := &fakeWriter{} + r, rep := newRec(t, mdmEP("sha256:H"), nil, w) + r.ProbeContent = func() (bool, map[string]json.RawMessage, error) { return true, sampleObserved(), nil } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + got := lastReport(t, rep) + if got.State != StateMDMManaged { + t.Fatalf("state = %q, want mdm_managed", got.State) + } + if got.AppliedHash != "" { + t.Fatalf("applied_hash must be EMPTY in MDM mode, got %q", got.AppliedHash) + } + if got.EvaluatedEnforcement != enforcementMDM { + t.Fatalf("evaluated_enforcement = %q, want mdm", got.EvaluatedEnforcement) + } + want := `{"extensions.allowed":{"*":false,"ms-python.python":"stable"},"extensions.gallery.serviceUrl":"https://mkt.example/api/v1"}` + if string(got.Observed) != want { + t.Fatalf("observed = %s, want %s", got.Observed, want) + } + // MDM owns nothing on disk: never write / clear / read the settings writer. + if len(w.writes) != 0 || w.clears != 0 || w.reads != 0 { + t.Fatalf("MDM must not touch settings.json: writes=%v clears=%d reads=%d", w.writes, w.clears, w.reads) + } + // And it records no ownership. + if _, ok := ReadAppliedState(CategoryIDEExtension, TargetVSCode); ok { + t.Fatal("MDM mode must not record ownership state") + } +} + +func TestReconcileMDMPolicyNotApplied(t *testing.T) { + w := &fakeWriter{} + r, rep := newRec(t, mdmEP("sha256:H"), nil, w) + r.ProbeContent = func() (bool, map[string]json.RawMessage, error) { return false, nil, nil } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + got := lastReport(t, rep) + if got.State != StatePolicyNotApplied { + t.Fatalf("state = %q, want policy_not_applied", got.State) + } + if len(got.Observed) != 0 { + t.Fatalf("observed must be omitted when not applied, got %s", got.Observed) + } + if got.AppliedHash != "" || got.EvaluatedEnforcement != enforcementMDM { + t.Fatalf("report = %+v", got) + } + if len(w.writes) != 0 || w.clears != 0 { + t.Fatal("must not touch settings.json") + } +} + +func TestReconcileMDMProbeErrorVerificationFailed(t *testing.T) { + w := &fakeWriter{} + r, rep := newRec(t, mdmEP("sha256:H"), nil, w) + r.ProbeContent = func() (bool, map[string]json.RawMessage, error) { + return false, nil, errors.New("policy.json present but unreadable") + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + got := lastReport(t, rep) + if got.State != StateVerificationFailed { + t.Fatalf("state = %q, want verification_failed", got.State) + } + if len(got.Observed) != 0 { + t.Fatalf("observed must be omitted on probe error, got %s", got.Observed) + } + if len(w.writes) != 0 || w.clears != 0 { + t.Fatal("must not write on probe error") + } +} + +func TestReconcileMDMRunsWithNilWriter(t *testing.T) { + // MDM verification runs before the nil-Writer check, so a platform with no + // settings path still probes and reports. + withTempCache(t) + rep := &fakeReporter{} + r := &Reconciler{ + Fetcher: &fakeFetcher{ep: mdmEP("sha256:H")}, + Reporter: rep, + Writer: nil, // untyped nil interface + CustomerID: "c", + DeviceID: "d", + Platform: "darwin", + ProbeContent: func() (bool, map[string]json.RawMessage, error) { return true, sampleObserved(), nil }, + } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if got := lastReport(t, rep); got.State != StateMDMManaged { + t.Fatalf("nil-writer MDM: state = %q, want mdm_managed", got.State) + } +} + +func TestReconcileMDMClearIsNoOp(t *testing.T) { + // enforcement=mdm + clear=true → defensive no-op: no report, no probe, no + // writer touch. + w := &fakeWriter{present: true, value: "x"} + ep := EffectivePolicy{Category: CategoryIDEExtension, Clear: true, Enforcement: enforcementMDM} + r, rep := newRec(t, ep, nil, w) + probed := false + r.ProbeContent = func() (bool, map[string]json.RawMessage, error) { probed = true; return false, nil, nil } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if len(rep.reports) != 0 { + t.Fatalf("MDM clear must not report, got %+v", rep.reports) + } + if probed { + t.Fatal("MDM clear must not probe") + } + if w.clears != 0 || len(w.writes) != 0 { + t.Fatalf("MDM clear must not touch the writer: clears=%d writes=%v", w.clears, w.writes) + } +} + +func TestReconcileUnknownEnforcementFallsToDMG(t *testing.T) { + // An unrecognized channel fails safe to the DMG write path (never skip + // enforcement) and still echoes the raw channel. + w := &fakeWriter{} + ep := policyEP("sha256:H") + ep.Enforcement = "future-mode" + r, rep := newRec(t, ep, nil, w) + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if len(w.writes) != 1 || w.writes[0] != samplePolicy { + t.Fatalf("unknown enforcement must run the DMG write path, writes=%v", w.writes) + } + got := lastReport(t, rep) + if got.State != StateCompliant { + t.Fatalf("state = %q, want compliant", got.State) + } + if got.EvaluatedEnforcement != "future-mode" { + t.Fatalf("evaluated_enforcement = %q, want the echoed channel", got.EvaluatedEnforcement) + } +} + +func TestReconcileEnforcementRoutingIsCaseInsensitive(t *testing.T) { + // A case/space variant of a known channel routes like the canonical one and + // reports canonically: " MDM " reaches the verify-only path (no settings + // write), not the DMG writer, and echoes "mdm" so it matches the backend gate. + w := &fakeWriter{} + ep := policyEP("sha256:H") + ep.Enforcement = " MDM " + r, rep := newRec(t, ep, nil, w) + r.ProbeContent = func() (bool, map[string]json.RawMessage, error) { return false, nil, nil } + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if len(w.writes) != 0 || w.clears != 0 { + t.Fatalf("verify-only must not touch the writer: writes=%v clears=%d", w.writes, w.clears) + } + got := lastReport(t, rep) + if got.State != StatePolicyNotApplied { + t.Fatalf("state = %q, want policy_not_applied", got.State) + } + if got.EvaluatedEnforcement != enforcementMDM { + t.Fatalf("evaluated_enforcement = %q, want canonical %q", got.EvaluatedEnforcement, enforcementMDM) + } +} + +func TestReconcileDMGEchoesEvaluatedEnforcement(t *testing.T) { + // An explicit dmg channel runs the DMG path and every report echoes "dmg". + w := &fakeWriter{} + ep := policyEP("sha256:H") + ep.Enforcement = enforcementDMG + r, rep := newRec(t, ep, nil, w) + if err := r.Reconcile(context.Background()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if len(w.writes) != 1 { + t.Fatalf("dmg must write, got %v", w.writes) + } + if got := lastReport(t, rep); got.EvaluatedEnforcement != enforcementDMG { + t.Fatalf("evaluated_enforcement = %q, want dmg", got.EvaluatedEnforcement) + } +} From 707b8aac573ce8e0b4e4d593bdc4c550099acda6 Mon Sep 17 00:00:00 2001 From: Subham Ray Date: Fri, 24 Jul 2026 16:44:30 +0530 Subject: [PATCH 5/5] fix(devicepolicy): harden managed-settings patch keys and report canonical enforcement channel Address PR review comments: - ApplyManaged built the RFC-6902 patch path by raw string concat with the backend-supplied key. RFC 6901-escape the pointer token (~->~0, /->~1) and JSON-encode the path (jsonPointerPath) so an unusual key cannot forge a nested pointer path or corrupt the patch document. The value is still spliced verbatim (never re-encoded, which would reorder members or HTML-escape it). - EvaluatedEnforcement reported the raw request, so an unknown channel ran the DMG path yet reported the raw string. Resolve and stamp the canonical channel the cycle actually ran -- always "dmg" or "mdm" (empty/unknown -> "dmg") -- so the backend's exact-match gate sees the channel that executed. --- internal/devicepolicy/api.go | 6 ++-- internal/devicepolicy/reconcile.go | 30 +++++++++------- internal/devicepolicy/reconcile_test.go | 7 ++-- internal/devicepolicy/settings_writer.go | 34 ++++++++++++++++--- internal/devicepolicy/settings_writer_test.go | 24 +++++++++++++ 5 files changed, 80 insertions(+), 21 deletions(-) diff --git a/internal/devicepolicy/api.go b/internal/devicepolicy/api.go index 0cfb3a48..54c19456 100644 --- a/internal/devicepolicy/api.go +++ b/internal/devicepolicy/api.go @@ -258,8 +258,10 @@ func isJSONObject(raw json.RawMessage) bool { // AppliedHash is empty and Observed instead carries the OS-managed policy the // agent read, keyed by VS Code setting id (the same keys as the desired policy) // so the backend can diff like-for-like and decide drift — the agent does not -// judge match. EvaluatedEnforcement echoes the channel the cycle ran ("", "dmg" -// or "mdm"). Observed and EvaluatedEnforcement are omitted when empty. +// judge match. EvaluatedEnforcement names the canonical channel the cycle +// actually ran — "dmg" or "mdm" (an empty or unrecognized request resolves to +// "dmg") — so the backend can gate on an exact match. Observed is omitted when +// empty. type ComplianceReport struct { Category string `json:"category"` Target string `json:"target"` diff --git a/internal/devicepolicy/reconcile.go b/internal/devicepolicy/reconcile.go index 6583bde6..abd0cc1a 100644 --- a/internal/devicepolicy/reconcile.go +++ b/internal/devicepolicy/reconcile.go @@ -60,9 +60,10 @@ type Reconciler struct { writeState func(category, target string, s AppliedTargetState) error clearState func(category, target string) error - // enforcement is the normalized channel the current cycle ran (lower-cased, - // trimmed ep.Enforcement), stamped onto every report as EvaluatedEnforcement - // so it matches the backend's exact-match gate. Per-cycle scratch. + // enforcement is the canonical channel the current cycle actually ran — + // always "dmg" or "mdm" (an empty or unrecognized ep.Enforcement resolves to + // "dmg"). Stamped onto every report as EvaluatedEnforcement so it matches the + // backend's exact-match gate. Per-cycle scratch. enforcement string } @@ -151,19 +152,21 @@ func (r *Reconciler) Reconcile(ctx context.Context) error { // Malformed/transient: do nothing. The on-disk policy (if any) stands. return fmt.Errorf("devicepolicy: fetch: %w", err) } - // Normalize once for routing AND reporting: the backend gates on an exact - // "mdm"/"dmg", so EvaluatedEnforcement must carry the canonical value, not - // whatever casing/spacing arrived. - r.enforcement = strings.ToLower(strings.TrimSpace(ep.Enforcement)) - - // MDM is verify-only and owns nothing on disk, so it routes before the - // Writer/clear checks below. - switch r.enforcement { + // Resolve the requested channel to the canonical one this cycle actually + // runs, and stamp THAT on every report as EvaluatedEnforcement: the backend + // gates on an exact "mdm"/"dmg", so the report must name the channel that ran + // — never the raw request. An empty or unrecognized channel runs, and + // reports, the DMG path. MDM is verify-only and owns nothing on disk, so it + // routes before the Writer/clear checks below. + switch strings.ToLower(strings.TrimSpace(ep.Enforcement)) { case enforcementMDM: + r.enforcement = enforcementMDM return r.verifyMDM(ctx, cat, tgt, ep) case enforcementDMG, "": + r.enforcement = enforcementDMG default: r.logf("devicepolicy: unknown enforcement %q; running DMG path", ep.Enforcement) + r.enforcement = enforcementDMG } if r.Writer == nil { @@ -663,7 +666,10 @@ func opConverged(op settingOp, m map[string]settingValue) bool { // ownedKeys folds an ownership record into a flat map of setting id → the exact // value the agent last wrote, skipping empty entries. Every managed key — the // allowlist included — lives in WrittenSettings. Drift detection and -// ownership-gated removal act only on keys the agent actually wrote. +// ownership-gated removal act only on keys the agent actually wrote. The managed +// path reads WrittenSettings only; a legacy single-key WrittenValue is NOT +// migrated in (pre-GA — no shipped single-key VS Code state exists to carry over, +// so there is nothing to be backward-compatible with). func ownedKeys(prev AppliedTargetState, hadPrev bool) map[string]string { owned := map[string]string{} if !hadPrev { diff --git a/internal/devicepolicy/reconcile_test.go b/internal/devicepolicy/reconcile_test.go index b86abfdd..1b93e302 100644 --- a/internal/devicepolicy/reconcile_test.go +++ b/internal/devicepolicy/reconcile_test.go @@ -1356,7 +1356,8 @@ func TestReconcileMDMClearIsNoOp(t *testing.T) { func TestReconcileUnknownEnforcementFallsToDMG(t *testing.T) { // An unrecognized channel fails safe to the DMG write path (never skip - // enforcement) and still echoes the raw channel. + // enforcement) and reports the canonical channel it actually ran ("dmg"), not + // the raw request — the backend gates on an exact match. w := &fakeWriter{} ep := policyEP("sha256:H") ep.Enforcement = "future-mode" @@ -1371,8 +1372,8 @@ func TestReconcileUnknownEnforcementFallsToDMG(t *testing.T) { if got.State != StateCompliant { t.Fatalf("state = %q, want compliant", got.State) } - if got.EvaluatedEnforcement != "future-mode" { - t.Fatalf("evaluated_enforcement = %q, want the echoed channel", got.EvaluatedEnforcement) + if got.EvaluatedEnforcement != enforcementDMG { + t.Fatalf("evaluated_enforcement = %q, want canonical %q", got.EvaluatedEnforcement, enforcementDMG) } } diff --git a/internal/devicepolicy/settings_writer.go b/internal/devicepolicy/settings_writer.go index bb664821..06dc773a 100644 --- a/internal/devicepolicy/settings_writer.go +++ b/internal/devicepolicy/settings_writer.go @@ -380,6 +380,23 @@ func (w *settingsWriter) ReadManaged(keys []string) (map[string]settingValue, er return out, nil } +// jsonPointerPath returns key as a ready-to-splice RFC 6902 patch path: the +// single-segment JSON Pointer "/"+token, the token RFC 6901-escaped ('~'→'~0', +// '/'→'~1'), the whole thing JSON-string-encoded (quotes included). VS Code +// setting ids are dotted (extensions.allowed), so this is normally just quoting +// — but the managed key set is backend-driven, and escaping + encoding stop an +// unusual key (one holding '/', '~', '"', '\\', or a control char) from forging +// a nested pointer path or corrupting the patch document. +func jsonPointerPath(key string) (string, error) { + token := strings.ReplaceAll(key, "~", "~0") + token = strings.ReplaceAll(token, "/", "~1") + b, err := json.Marshal("/" + token) + if err != nil { + return "", fmt.Errorf("devicepolicy: encode patch path for %q: %w", key, err) + } + return string(b), nil +} + // ApplyManaged builds one RFC-6902 patch from ops — Set → add (upsert), Remove // → remove (pre-checked so an absent key is skipped, not an error), preserve → // nothing — and applies it in a single atomic load→patch→store. An empty patch @@ -402,9 +419,14 @@ func (w *settingsWriter) ApplyManaged(ops []settingOp) (map[string]settingValue, if !json.Valid(op.Value) { return nil, fmt.Errorf("devicepolicy: refusing to write invalid JSON value for %q to %s", op.Key, w.path) } - // op.Key is a dotted setting id with no '/' or '~', so it needs no - // JSON-Pointer escaping; the dot is literal. - patchOps = append(patchOps, `{"op":"add","path":"/`+op.Key+`","value":`+string(op.Value)+`}`) + path, perr := jsonPointerPath(op.Key) + if perr != nil { + return nil, perr + } + // Value is spliced verbatim (already json.Valid-checked) — never + // re-encoded, which would reorder members or HTML-escape it and break + // the backend's byte-exact applied==desired check. + patchOps = append(patchOps, `{"op":"add","path":`+path+`,"value":`+string(op.Value)+`}`) case op.Remove: // RFC 6902 "remove" errors on an absent member; pre-check presence so // a Remove of a key that is not there is simply skipped. @@ -413,7 +435,11 @@ func (w *settingsWriter) ApplyManaged(ops []settingOp) (map[string]settingValue, return nil, perr } if present { - patchOps = append(patchOps, `{"op":"remove","path":"/`+op.Key+`"}`) + path, perr := jsonPointerPath(op.Key) + if perr != nil { + return nil, perr + } + patchOps = append(patchOps, `{"op":"remove","path":`+path+`}`) } } } diff --git a/internal/devicepolicy/settings_writer_test.go b/internal/devicepolicy/settings_writer_test.go index 3e77ac57..fa02041a 100644 --- a/internal/devicepolicy/settings_writer_test.go +++ b/internal/devicepolicy/settings_writer_test.go @@ -600,6 +600,30 @@ func TestApplyManagedRejectsInvalidJSONValue(t *testing.T) { } } +func TestApplyManagedEscapesUnusualKeys(t *testing.T) { + // The managed key set is backend-driven. A key holding JSON-Pointer + // metacharacters ('/', '~') or JSON-string metacharacters ('"', '\') must be + // written as ONE literal top-level member — never a nested pointer path, and + // never a corrupt patch document that clobbers the file. + for _, key := range []string{`weird/key`, `tilde~key`, `quote"key`, `back\slash`} { + w, path := newTestSettingsWriter(t) + writeSettingsFixture(t, path, `{"keep.me": 1}`) + if _, err := w.ApplyManaged([]settingOp{{Key: key, Set: true, Value: json.RawMessage(`"v"`)}}); err != nil { + t.Fatalf("ApplyManaged(%q): %v", key, err) + } + var m map[string]json.RawMessage + if err := json.Unmarshal([]byte(readFileString(t, path)), &m); err != nil { + t.Fatalf("file not valid JSON after writing key %q: %v", key, err) + } + if string(m["keep.me"]) != "1" { + t.Fatalf("untouched key lost after writing %q: %v", key, m) + } + if got, ok := m[key]; !ok || string(got) != `"v"` { + t.Fatalf("key %q must be one literal top-level member = \"v\", got ok=%v val=%s", key, ok, got) + } + } +} + // TestGalleryValueRoundTrips pins the ownership invariant on the value path the // reconciler now uses: the gallery URL arrives as a JSON string in the settings // map, is compacted (compactSettings), written, and recorded as owned — and that