From 29f6f35749443de276da17c25eff8cab5c504dc9 Mon Sep 17 00:00:00 2001 From: Rafael Garcia Date: Thu, 29 Jan 2026 08:10:00 -0500 Subject: [PATCH 1/2] fix: preserve unknown fields in policy.json during read-modify-write cycles The Policy struct only defined fields it explicitly used, causing any other fields in policy.json to be silently dropped when AddExtension performed a read-modify-write cycle. This broke the DefaultGeolocationSetting added in #136. Changes: - Add custom UnmarshalJSON/MarshalJSON to preserve unknown JSON fields - Simplify Policy struct to only include fields we programmatically modify (ExtensionInstallForcelist, ExtensionSettings) - All other Chrome policy settings are now automatically preserved - Add comprehensive tests for the preservation behavior This allows policy.json to contain any Chrome policy setting without requiring Go struct updates, as long as we don't need to modify it in code. Fixes issue introduced by #136 --- server/lib/policy/policy.go | 112 +++++++++++++++----- server/lib/policy/policy_test.go | 170 +++++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+), 28 deletions(-) create mode 100644 server/lib/policy/policy_test.go diff --git a/server/lib/policy/policy.go b/server/lib/policy/policy.go index 16f43a62..ff3b35a2 100644 --- a/server/lib/policy/policy.go +++ b/server/lib/policy/policy.go @@ -12,30 +12,94 @@ import ( ) const PolicyPath = "/etc/chromium/policies/managed/policy.json" -const DefaultSearchProviderName = "DuckDuckGo" -const DefaultSearchProviderSearchURL = "https://duckduckgo.com/?q={searchTerms}" -const DefaultSearchProviderSuggestURL = "https://duckduckgo.com/ac/?q={searchTerms}" -const NewTabPageLocation = "https://start.duckduckgo.com/" // Chrome extension IDs are 32 lowercase a-p characters var extensionIDRegex = regexp.MustCompile(`^[a-p]{32}$`) var extensionPathRegex = regexp.MustCompile(`/extensions/[^/]+/`) -// Policy represents the Chrome enterprise policy structure +// Policy represents the Chrome enterprise policy structure. +// Only fields that are programmatically modified are defined here. +// All other fields (like DefaultGeolocationSetting, PasswordManagerEnabled, etc.) +// are preserved through the unknownFields mechanism during read-modify-write cycles. type Policy struct { mu sync.Mutex - PasswordManagerEnabled bool `json:"PasswordManagerEnabled"` - AutofillCreditCardEnabled bool `json:"AutofillCreditCardEnabled"` - TranslateEnabled bool `json:"TranslateEnabled"` - DefaultNotificationsSetting int `json:"DefaultNotificationsSetting"` - DefaultSearchProviderEnabled bool `json:"DefaultSearchProviderEnabled"` - DefaultSearchProviderName string `json:"DefaultSearchProviderName"` - DefaultSearchProviderSearchURL string `json:"DefaultSearchProviderSearchURL"` - DefaultSearchProviderSuggestURL string `json:"DefaultSearchProviderSuggestURL"` - NewTabPageLocation string `json:"NewTabPageLocation"` - ExtensionInstallForcelist []string `json:"ExtensionInstallForcelist,omitempty"` - ExtensionSettings map[string]ExtensionSetting `json:"ExtensionSettings"` + // ExtensionInstallForcelist is modified when adding force-installed extensions + ExtensionInstallForcelist []string `json:"ExtensionInstallForcelist,omitempty"` + // ExtensionSettings is modified when adding/configuring extensions + ExtensionSettings map[string]ExtensionSetting `json:"ExtensionSettings"` + + // unknownFields preserves all JSON fields not explicitly defined in this struct. + // This allows policy.json to contain any Chrome policy settings without + // requiring updates to this Go struct. + unknownFields map[string]json.RawMessage +} + +// knownPolicyFields lists all JSON field names that have corresponding struct fields. +// All other fields are automatically preserved in unknownFields. +var knownPolicyFields = map[string]bool{ + "ExtensionInstallForcelist": true, + "ExtensionSettings": true, +} + +// UnmarshalJSON implements custom JSON unmarshaling that preserves unknown fields. +func (p *Policy) UnmarshalJSON(data []byte) error { + // First, unmarshal into a map to capture all fields + var allFields map[string]json.RawMessage + if err := json.Unmarshal(data, &allFields); err != nil { + return err + } + + // Use an alias type to avoid infinite recursion when unmarshaling known fields + type PolicyAlias Policy + var alias PolicyAlias + if err := json.Unmarshal(data, &alias); err != nil { + return err + } + + // Copy the alias back to p (excluding unknownFields which we'll set separately) + *p = Policy(alias) + + // Extract unknown fields + p.unknownFields = make(map[string]json.RawMessage) + for key, value := range allFields { + if !knownPolicyFields[key] { + p.unknownFields[key] = value + } + } + + return nil +} + +// MarshalJSON implements custom JSON marshaling that includes unknown fields. +func (p Policy) MarshalJSON() ([]byte, error) { + // Use an alias type to avoid infinite recursion + type PolicyAlias Policy + alias := PolicyAlias(p) + + // Marshal the known fields first + knownData, err := json.Marshal(alias) + if err != nil { + return nil, err + } + + // If no unknown fields, return as-is + if len(p.unknownFields) == 0 { + return knownData, nil + } + + // Unmarshal known fields into a map so we can add unknown fields + var result map[string]json.RawMessage + if err := json.Unmarshal(knownData, &result); err != nil { + return nil, err + } + + // Add unknown fields + for key, value := range p.unknownFields { + result[key] = value + } + + return json.Marshal(result) } // ExtensionSetting represents settings for a specific extension @@ -54,19 +118,11 @@ func (p *Policy) readPolicyUnlocked() (*Policy, error) { data, err := os.ReadFile(PolicyPath) if err != nil { if os.IsNotExist(err) { - // Return default policy if file doesn't exist + // Return minimal policy if file doesn't exist. + // In practice, policy.json ships with the container image. return &Policy{ - PasswordManagerEnabled: false, - AutofillCreditCardEnabled: false, - TranslateEnabled: false, - DefaultNotificationsSetting: 2, - DefaultSearchProviderEnabled: true, - DefaultSearchProviderName: DefaultSearchProviderName, - DefaultSearchProviderSearchURL: DefaultSearchProviderSearchURL, - DefaultSearchProviderSuggestURL: DefaultSearchProviderSuggestURL, - NewTabPageLocation: NewTabPageLocation, - ExtensionInstallForcelist: []string{}, - ExtensionSettings: make(map[string]ExtensionSetting), + ExtensionInstallForcelist: []string{}, + ExtensionSettings: make(map[string]ExtensionSetting), }, nil } return nil, fmt.Errorf("failed to read policy file: %w", err) diff --git a/server/lib/policy/policy_test.go b/server/lib/policy/policy_test.go new file mode 100644 index 00000000..f33bfb32 --- /dev/null +++ b/server/lib/policy/policy_test.go @@ -0,0 +1,170 @@ +package policy + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPolicy_PreservesUnknownFields(t *testing.T) { + // Simulate the policy.json with many fields not in the struct + // Only ExtensionInstallForcelist and ExtensionSettings are in the struct + input := `{ + "PasswordManagerEnabled": false, + "AutofillCreditCardEnabled": false, + "TranslateEnabled": false, + "DefaultNotificationsSetting": 2, + "DefaultGeolocationSetting": 2, + "DefaultSearchProviderEnabled": true, + "DefaultSearchProviderName": "DuckDuckGo", + "DefaultSearchProviderSearchURL": "https://duckduckgo.com/?q={searchTerms}", + "DefaultSearchProviderSuggestURL": "https://duckduckgo.com/ac/?q={searchTerms}", + "NewTabPageLocation": "https://start.duckduckgo.com/", + "SomeOtherUnknownField": {"nested": "value"}, + "ExtensionSettings": { + "*": { + "allowed_types": ["extension"], + "install_sources": ["*"] + } + } + }` + + var policy Policy + err := json.Unmarshal([]byte(input), &policy) + require.NoError(t, err) + + // Verify known struct fields are parsed correctly + assert.NotNil(t, policy.ExtensionSettings) + assert.Contains(t, policy.ExtensionSettings, "*") + + // Verify all non-struct fields are captured as unknown + // All fields except ExtensionSettings should be in unknownFields + expectedUnknownFields := []string{ + "PasswordManagerEnabled", + "AutofillCreditCardEnabled", + "TranslateEnabled", + "DefaultNotificationsSetting", + "DefaultGeolocationSetting", + "DefaultSearchProviderEnabled", + "DefaultSearchProviderName", + "DefaultSearchProviderSearchURL", + "DefaultSearchProviderSuggestURL", + "NewTabPageLocation", + "SomeOtherUnknownField", + } + for _, field := range expectedUnknownFields { + assert.Contains(t, policy.unknownFields, field, "field %s should be preserved", field) + } + + // Now marshal it back + output, err := json.Marshal(&policy) + require.NoError(t, err) + + // Unmarshal into a map to verify all fields are present + var result map[string]interface{} + err = json.Unmarshal(output, &result) + require.NoError(t, err) + + // Verify all fields are preserved + assert.Equal(t, float64(2), result["DefaultGeolocationSetting"]) + assert.Equal(t, float64(2), result["DefaultNotificationsSetting"]) + assert.Equal(t, false, result["PasswordManagerEnabled"]) + assert.Equal(t, "DuckDuckGo", result["DefaultSearchProviderName"]) + assert.NotNil(t, result["SomeOtherUnknownField"]) +} + +func TestPolicy_ModifyAndPreserveUnknownFields(t *testing.T) { + // This test simulates the AddExtension read-modify-write cycle + input := `{ + "PasswordManagerEnabled": false, + "DefaultGeolocationSetting": 2, + "DefaultNotificationsSetting": 2, + "ExtensionSettings": {} + }` + + var policy Policy + err := json.Unmarshal([]byte(input), &policy) + require.NoError(t, err) + + // Add an extension setting (this is what AddExtension does) + policy.ExtensionSettings["test-extension"] = ExtensionSetting{ + UpdateUrl: "http://127.0.0.1:10001/extensions/test/update.xml", + } + + // Marshal it back + output, err := json.Marshal(&policy) + require.NoError(t, err) + + // Verify the unknown fields are still there + var result map[string]interface{} + err = json.Unmarshal(output, &result) + require.NoError(t, err) + + assert.Equal(t, float64(2), result["DefaultGeolocationSetting"], "DefaultGeolocationSetting should be preserved") + assert.Equal(t, float64(2), result["DefaultNotificationsSetting"], "DefaultNotificationsSetting should be preserved") + assert.Equal(t, false, result["PasswordManagerEnabled"], "PasswordManagerEnabled should be preserved") + + // Verify extension was added + extSettings, ok := result["ExtensionSettings"].(map[string]interface{}) + require.True(t, ok) + assert.Contains(t, extSettings, "test-extension") +} + +func TestPolicy_ExtensionInstallForcelist(t *testing.T) { + // Test that ExtensionInstallForcelist works correctly + input := `{ + "DefaultGeolocationSetting": 2, + "ExtensionInstallForcelist": ["existing-ext;http://example.com/update.xml"], + "ExtensionSettings": {} + }` + + var policy Policy + err := json.Unmarshal([]byte(input), &policy) + require.NoError(t, err) + + // Verify forcelist is parsed + assert.Len(t, policy.ExtensionInstallForcelist, 1) + assert.Equal(t, "existing-ext;http://example.com/update.xml", policy.ExtensionInstallForcelist[0]) + + // Add to forcelist + policy.ExtensionInstallForcelist = append(policy.ExtensionInstallForcelist, "new-ext;http://example.com/new.xml") + + // Marshal back + output, err := json.Marshal(&policy) + require.NoError(t, err) + + var result map[string]interface{} + err = json.Unmarshal(output, &result) + require.NoError(t, err) + + // Both unknown field and forcelist should be present + assert.Equal(t, float64(2), result["DefaultGeolocationSetting"]) + forcelist, ok := result["ExtensionInstallForcelist"].([]interface{}) + require.True(t, ok) + assert.Len(t, forcelist, 2) +} + +func TestPolicy_EmptyPolicy(t *testing.T) { + // Test with minimal input + input := `{}` + + var policy Policy + err := json.Unmarshal([]byte(input), &policy) + require.NoError(t, err) + + assert.Empty(t, policy.unknownFields) + assert.Nil(t, policy.ExtensionSettings) + assert.Nil(t, policy.ExtensionInstallForcelist) + + output, err := json.Marshal(&policy) + require.NoError(t, err) + + var result map[string]interface{} + err = json.Unmarshal(output, &result) + require.NoError(t, err) + + // Should have empty ExtensionSettings as null/missing + assert.Nil(t, result["ExtensionSettings"]) +} From 81e35e836e4ee77ecee123bb0eb99be75df7ec2d Mon Sep 17 00:00:00 2001 From: Rafael Garcia Date: Thu, 29 Jan 2026 08:30:14 -0500 Subject: [PATCH 2/2] fix: avoid copying sync.Mutex in JSON marshal/unmarshal Use a separate policyJSON struct without the mutex for JSON operations to satisfy go vet checks. --- server/lib/policy/policy.go | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/server/lib/policy/policy.go b/server/lib/policy/policy.go index ff3b35a2..d77d6814 100644 --- a/server/lib/policy/policy.go +++ b/server/lib/policy/policy.go @@ -35,6 +35,13 @@ type Policy struct { unknownFields map[string]json.RawMessage } +// policyJSON is used for JSON marshaling/unmarshaling without the mutex. +// This avoids go vet warnings about copying mutex values. +type policyJSON struct { + ExtensionInstallForcelist []string `json:"ExtensionInstallForcelist,omitempty"` + ExtensionSettings map[string]ExtensionSetting `json:"ExtensionSettings"` +} + // knownPolicyFields lists all JSON field names that have corresponding struct fields. // All other fields are automatically preserved in unknownFields. var knownPolicyFields = map[string]bool{ @@ -50,15 +57,15 @@ func (p *Policy) UnmarshalJSON(data []byte) error { return err } - // Use an alias type to avoid infinite recursion when unmarshaling known fields - type PolicyAlias Policy - var alias PolicyAlias - if err := json.Unmarshal(data, &alias); err != nil { + // Unmarshal known fields into the helper struct (no mutex) + var pj policyJSON + if err := json.Unmarshal(data, &pj); err != nil { return err } - // Copy the alias back to p (excluding unknownFields which we'll set separately) - *p = Policy(alias) + // Copy the known fields to p + p.ExtensionInstallForcelist = pj.ExtensionInstallForcelist + p.ExtensionSettings = pj.ExtensionSettings // Extract unknown fields p.unknownFields = make(map[string]json.RawMessage) @@ -72,13 +79,15 @@ func (p *Policy) UnmarshalJSON(data []byte) error { } // MarshalJSON implements custom JSON marshaling that includes unknown fields. -func (p Policy) MarshalJSON() ([]byte, error) { - // Use an alias type to avoid infinite recursion - type PolicyAlias Policy - alias := PolicyAlias(p) +func (p *Policy) MarshalJSON() ([]byte, error) { + // Create helper struct with known fields (no mutex) + pj := policyJSON{ + ExtensionInstallForcelist: p.ExtensionInstallForcelist, + ExtensionSettings: p.ExtensionSettings, + } // Marshal the known fields first - knownData, err := json.Marshal(alias) + knownData, err := json.Marshal(pj) if err != nil { return nil, err }