diff --git a/pkg/agentdrain/data/default_weights.json b/pkg/agentdrain/data/default_weights.json index f232d2e905d..74ea4e2b819 100644 --- a/pkg/agentdrain/data/default_weights.json +++ b/pkg/agentdrain/data/default_weights.json @@ -100,12 +100,7 @@ ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -151,21 +146,12 @@ "id": 1, "size": 100, "stage": "finish", - "template": [ - "stage=finish", - "\u003c*\u003e", - "tokens=\u003cNUM\u003e" - ] + "template": ["stage=finish", "\u003c*\u003e", "tokens=\u003cNUM\u003e"] } ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -211,21 +197,12 @@ "id": 1, "size": 58, "stage": "plan", - "template": [ - "stage=plan", - "errors=\u003cNUM\u003e", - "turns=\u003cNUM\u003e" - ] + "template": ["stage=plan", "errors=\u003cNUM\u003e", "turns=\u003cNUM\u003e"] } ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -269,12 +246,7 @@ "clusters": null, "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -318,12 +290,7 @@ "clusters": null, "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -1448,12 +1415,7 @@ ], "config": { "Depth": 4, - "ExcludeFields": [ - "session_id", - "trace_id", - "span_id", - "timestamp" - ], + "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], "MaskRules": [ { "Name": "uuid", @@ -1493,4 +1455,4 @@ }, "next_id": 15 } -} \ No newline at end of file +} diff --git a/pkg/cli/codemod_permissions_test.go b/pkg/cli/codemod_permissions_test.go index 4f0d93cd9b2..7d177070bcf 100644 --- a/pkg/cli/codemod_permissions_test.go +++ b/pkg/cli/codemod_permissions_test.go @@ -428,3 +428,124 @@ This workflow needs permissions.` assert.Contains(t, result, "# Test Workflow") assert.Contains(t, result, "This workflow needs permissions.") } + +func TestWritePermissionsCodemod_SkipsIdToken(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read + id-token: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "read", + "id-token": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "codemod should not return an error") + assert.False(t, applied, "Should not be applied when only write-only permissions have write") + assert.Equal(t, content, result, "codemod result should be unchanged when only write-only permissions have write") + assert.Contains(t, result, "id-token: write", "id-token permission should remain write after codemod") + assert.NotContains(t, result, "id-token: read", "id-token should never be converted to read") +} + +func TestWritePermissionsCodemod_SkipsCopilotRequests(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: read + copilot-requests: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "read", + "copilot-requests": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "codemod should not return an error") + assert.False(t, applied, "Should not be applied when only write-only permissions have write") + assert.Equal(t, content, result, "codemod result should be unchanged when only write-only permissions have write") + assert.Contains(t, result, "copilot-requests: write", "copilot-requests permission should remain write after codemod") + assert.NotContains(t, result, "copilot-requests: read", "copilot-requests should never be converted to read") +} + +func TestWritePermissionsCodemod_MixedWithIdToken(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: write + issues: write + id-token: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "write", + "issues": "write", + "id-token": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "codemod should not return an error") + assert.True(t, applied, "codemod should be applied when non-write-only permissions have write") + assert.Contains(t, result, "contents: read", "contents permission should be downgraded from write to read") + assert.Contains(t, result, "issues: read", "issues permission should be downgraded from write to read") + // id-token must remain write — "read" is not a valid value for it + assert.Contains(t, result, "id-token: write", "id-token permission should remain write after codemod") + assert.NotContains(t, result, "id-token: read", "id-token should never be converted to read") +} + +func TestWritePermissionsCodemod_MixedWithCopilotRequests(t *testing.T) { + codemod := getMigrateWritePermissionsToReadCodemod() + + content := `--- +on: workflow_dispatch +permissions: + contents: write + copilot-requests: write +--- + +# Test` + + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "permissions": map[string]any{ + "contents": "write", + "copilot-requests": "write", + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err, "codemod should not return an error") + assert.True(t, applied, "codemod should be applied when non-write-only permissions have write") + assert.Contains(t, result, "contents: read", "contents permission should be downgraded from write to read") + // copilot-requests must remain write — "read" is not a valid value for it + assert.Contains(t, result, "copilot-requests: write", "copilot-requests permission should remain write after codemod") + assert.NotContains(t, result, "copilot-requests: read", "copilot-requests should never be converted to read") +} diff --git a/pkg/cli/codemod_permissions_write.go b/pkg/cli/codemod_permissions_write.go index 88623b7a511..3cd4fe419b3 100644 --- a/pkg/cli/codemod_permissions_write.go +++ b/pkg/cli/codemod_permissions_write.go @@ -9,6 +9,13 @@ import ( var writePermissionsCodemodLog = logger.New("cli:codemod_permissions") +// writeOnlyPermissions are permission scopes that only accept "write" or "none" as valid values. +// These must never be converted to "read" since "read" is not a valid value for them. +var writeOnlyPermissions = map[string]bool{ + "id-token": true, // OIDC token: only write or none + "copilot-requests": true, // Copilot authentication token: only write or none +} + // getMigrateWritePermissionsToReadCodemod creates a codemod for converting write permissions to read func getMigrateWritePermissionsToReadCodemod() Codemod { return Codemod{ @@ -35,7 +42,12 @@ func getMigrateWritePermissionsToReadCodemod() Codemod { // Handle map format if mapValue, ok := permissionsValue.(map[string]any); ok { - for _, value := range mapValue { + for key, value := range mapValue { + // Skip write-only permissions (e.g. id-token, copilot-requests) since + // "read" is not a valid value for them — they only accept "write" or "none" + if writeOnlyPermissions[key] { + continue + } if strValue, ok := value.(string); ok && strValue == "write" { hasWritePermissions = true break @@ -91,8 +103,17 @@ func getMigrateWritePermissionsToReadCodemod() Codemod { parts := strings.SplitN(line, ":", 2) if len(parts) >= 2 { key := parts[0] + permKey := strings.TrimSpace(key) valueAndComment := parts[1] + // Skip write-only permissions (e.g. id-token, copilot-requests) since + // "read" is not a valid value for them — they only accept "write" or "none" + if writeOnlyPermissions[permKey] { + result[i] = line + writePermissionsCodemodLog.Printf("Skipping write-only permission %q on line %d", permKey, i+1) + continue + } + // Replace "write" with "read" in the value part newValueAndComment := strings.Replace(valueAndComment, " write", " read", 1) result[i] = fmt.Sprintf("%s:%s", key, newValueAndComment)