fix: Schema failure fixes (#51)

mvicknr · web-flow · commit b55d4176f3b4 · 2026-06-04T10:22:22.000-04:00
* fix: Allow schemas to live anywhere in the agent workspace

* fix: Remove invalid schema or content before calling service

* fix: Make logs less noisy
diff --git a/cmd/agent-metadata-action/main.go b/cmd/agent-metadata-action/main.go
@@ -57,7 +57,6 @@ func initNewRelic(ctx context.Context) *newrelic.Application {
 	app, err := newrelic.NewApplication(
 		newrelic.ConfigAppName("agent-metadata-action"),
 		newrelic.ConfigLicense(licenseKey),
-		newrelic.ConfigDebugLogger(os.Stdout),
 		newrelic.ConfigDistributedTracerEnabled(true),
 		newrelic.ConfigAppLogForwardingEnabled(true),
 		newrelic.ConfigFromEnvironment(), // This reads NEW_RELIC_HOST
diff --git a/internal/loader/definitions.go b/internal/loader/definitions.go
@@ -31,14 +31,19 @@ func ReadConfigurationDefinitions(ctx context.Context, workspacePath string) ([]
 		}
 		schemaPath, ok := definitions[i]["schema"].(string)
 		if !ok {
-			logging.Warn(ctx, "schema field is not a string - skipping")
+			// Drop the field so the server doesn't reject the whole request over a malformed type.
+			logging.Warn(ctx, "schema field is not a string - dropping it")
+			delete(definitions[i], "schema")
 			continue
 		}
 
 		// @todo at some point, we may want to do this concurrently if there are any agents with a large number of files
 		encoded, err := loadAndEncodeFile(workspacePath, schemaPath, "schema")
 		if err != nil {
-			logging.Warnf(ctx, "failed to load schema at schema path %s: %v -- continuing without it", schemaPath, err)
+			// Drop the field rather than leaving the path string in place — the server would
+			// otherwise try to base64-decode the path and reject the whole bundled request.
+			logging.Warnf(ctx, "failed to load schema at schema path %s: %v -- dropping schema field", schemaPath, err)
+			delete(definitions[i], "schema")
 			continue
 		}
 		definitions[i]["schema"] = encoded
@@ -71,14 +76,19 @@ func ReadAgentControlDefinitions(ctx context.Context, workspacePath string) ([]m
 		}
 		contentPath, ok := definitions[i]["content"].(string)
 		if !ok {
-			logging.Warn(ctx, "content field is not a string - skipping")
+			// Drop the field so the server doesn't reject the whole request over a malformed type.
+			logging.Warn(ctx, "content field is not a string - dropping it")
+			delete(definitions[i], "content")
 			continue
 		}
 
 		// @todo at some point, we may want to do this concurrently if there are any agents with a large number of files
 		encoded, err := loadAndEncodeFile(workspacePath, contentPath, "content")
 		if err != nil {
-			logging.Warnf(ctx, "failed to load content at path %s: %v -- continuing without it", contentPath, err)
+			// Drop the field rather than leaving the path string in place — the server would
+			// otherwise try to base64-decode the path and reject the whole bundled request.
+			logging.Warnf(ctx, "failed to load content at path %s: %v -- dropping content field", contentPath, err)
+			delete(definitions[i], "content")
 			continue
 		}
 		definitions[i]["content"] = encoded
@@ -139,28 +149,22 @@ func loadAndEncodeFile(workspacePath string, contentPath string, filePathField s
 		return "", nil
 	}
 
-	// Validate content path to prevent directory traversal attacks
-	if strings.Contains(contentPath, "..") {
-		return "", fmt.Errorf("invalid %s path: contains directory traversal", filePathField)
-	}
-
-	// Content paths are relative to the expected root directory
+	// Content paths are relative to the .fleetControl directory; the resolved path
+	// must stay within the workspace so we can't read arbitrary files on the runner.
 	fullPath := filepath.Join(workspacePath, config.GetRootFolderForAgentRepo(), contentPath)
 
-	// Additional security check: ensure the resolved path is within the expected root directory
-	expectedRootDir := filepath.Join(workspacePath, config.GetRootFolderForAgentRepo())
 	resolvedPath, err := filepath.Abs(fullPath)
 	if err != nil {
 		return "", fmt.Errorf("failed to resolve %s path: %w", filePathField, err)
 	}
 
-	resolvedRootDirectory, err := filepath.Abs(expectedRootDir)
+	resolvedWorkspace, err := filepath.Abs(workspacePath)
 	if err != nil {
-		return "", fmt.Errorf("failed to resolve expected root directory:%s %w", expectedRootDir, err)
+		return "", fmt.Errorf("failed to resolve workspace path %s: %w", workspacePath, err)
 	}
 
-	if !strings.HasPrefix(resolvedPath, resolvedRootDirectory+string(filepath.Separator)) && resolvedPath != resolvedRootDirectory {
-		return "", fmt.Errorf("invalid %s path: must be within expected root directory: %s\n", filePathField, expectedRootDir)
+	if !strings.HasPrefix(resolvedPath, resolvedWorkspace+string(filepath.Separator)) && resolvedPath != resolvedWorkspace {
+		return "", fmt.Errorf("invalid %s path: must be within workspace: %s", filePathField, resolvedWorkspace)
 	}
 
 	data, err := os.ReadFile(fullPath)
diff --git a/internal/loader/definitions_test.go b/internal/loader/definitions_test.go
@@ -385,10 +385,6 @@ func TestReadConfigurationDefinitions_DirectoryTraversal(t *testing.T) {
 			name:       "parent directory traversal with ../",
 			schemaPath: "../../../etc/passwd",
 		},
-		{
-			name:       "relative parent traversal",
-			schemaPath: "schemas/../../secrets.json",
-		},
 		{
 			name:       "multiple parent traversals",
 			schemaPath: "./../.././../../../sensitive.json",
@@ -428,7 +424,7 @@ func TestReadConfigurationDefinitions_DirectoryTraversal(t *testing.T) {
 
 			require.NoError(t, err)
 			assert.NotNil(t, configs)
-			assert.Contains(t, outputStr, "directory traversal")
+			assert.Contains(t, outputStr, "must be within workspace")
 		})
 	}
 }
@@ -530,11 +526,11 @@ func TestReadAgentControlDefinitions_ContentLoadingWarnings(t *testing.T) {
 			expectedWarning: "failed to load content",
 		},
 		{
-			name: "directory traversal in content path",
+			name: "content path escapes workspace",
 			setupFunc: func(t *testing.T, tmpDir string) string {
 				return "../../../etc/passwd"
 			},
-			expectedWarning: "directory traversal",
+			expectedWarning: "must be within workspace",
 		},
 	}
 
@@ -820,43 +816,15 @@ func TestLoadAndEncodeFile_PathValidation(t *testing.T) {
 		expectedErrMsg string
 	}{
 		{
-			name: "path with directory traversal using ../",
+			name: "path escapes workspace via parent traversal",
 			setupFunc: func(t *testing.T) (string, string) {
 				tmpDir := t.TempDir()
 				workspace := filepath.Join(tmpDir, "workspace", config.GetRootFolderForAgentRepo())
 				require.NoError(t, os.MkdirAll(workspace, 0755))
 
-				// Path that would escape using ../
 				return filepath.Join(tmpDir, "workspace"), "../../../etc/passwd"
 			},
-			expectedErrMsg: "directory traversal",
-		},
-		{
-			name: "path with multiple directory traversals",
-			setupFunc: func(t *testing.T) (string, string) {
-				tmpDir := t.TempDir()
-				workspace := filepath.Join(tmpDir, "workspace", config.GetRootFolderForAgentRepo())
-				require.NoError(t, os.MkdirAll(workspace, 0755))
-
-				return filepath.Join(tmpDir, "workspace"), "schemas/../../sensitive.json"
-			},
-			expectedErrMsg: "directory traversal",
-		},
-		{
-			name: "path attempting to read outside .fleetControl via symlink-like path",
-			setupFunc: func(t *testing.T) (string, string) {
-				tmpDir := t.TempDir()
-				workspace := filepath.Join(tmpDir, "workspace", config.GetRootFolderForAgentRepo())
-				require.NoError(t, os.MkdirAll(workspace, 0755))
-
-				// Create a file at workspace level (outside .fleetControl)
-				outsideFile := filepath.Join(tmpDir, "workspace", "outside.json")
-				require.NoError(t, os.WriteFile(outsideFile, []byte(`{"test": "data"}`), 0644))
-
-				// Try to access it from .fleetControl
-				return filepath.Join(tmpDir, "workspace"), "../outside.json"
-			},
-			expectedErrMsg: "directory traversal",
+			expectedErrMsg: "must be within workspace",
 		},
 	}
 
@@ -890,3 +858,147 @@ func TestLoadAndEncodeFile_PathValidation(t *testing.T) {
 		})
 	}
 }
+
+// TestLoadAndEncodeFile_OutsideFleetControl verifies that schema files referenced
+// from outside .fleetControl (but still within the workspace) load successfully.
+// This is the dotnet agent shape: schema lives at ../src/.../Configuration.xsd.
+func TestLoadAndEncodeFile_OutsideFleetControl(t *testing.T) {
+	tmpDir := t.TempDir()
+	workspace := filepath.Join(tmpDir, "workspace")
+	configDir := filepath.Join(workspace, config.GetRootFolderForAgentRepo())
+	require.NoError(t, os.MkdirAll(configDir, 0755))
+
+	// Schema lives outside .fleetControl, in a sibling directory inside the workspace.
+	srcDir := filepath.Join(workspace, "src")
+	require.NoError(t, os.MkdirAll(srcDir, 0755))
+	schemaContent := `<?xml version="1.0"?><xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema"/>`
+	schemaFile := filepath.Join(srcDir, "Configuration.xsd")
+	require.NoError(t, os.WriteFile(schemaFile, []byte(schemaContent), 0644))
+
+	configFile := filepath.Join(configDir, config.GetConfigurationDefinitionsFilename())
+	testYAML := `configurationDefinitions:
+  - platform: linux
+    description: Test config
+    type: test-config
+    version: 1.0.0
+    format: xml
+    schema: ../src/Configuration.xsd`
+	require.NoError(t, os.WriteFile(configFile, []byte(testYAML), 0644))
+
+	configs, err := ReadConfigurationDefinitions(context.Background(), workspace)
+	require.NoError(t, err)
+	require.Len(t, configs, 1)
+
+	encoded, ok := configs[0]["schema"].(string)
+	require.True(t, ok, "schema should be base64-encoded string, got %T", configs[0]["schema"])
+	expected := base64.StdEncoding.EncodeToString([]byte(schemaContent))
+	assert.Equal(t, expected, encoded)
+}
+
+// TestReadConfigurationDefinitions_DropsBrokenSchemaField verifies that when a
+// schema can't be loaded or has the wrong type, the schema field is removed from
+// the entry rather than left in place. This prevents the server from rejecting the
+// entire bundled request when one entry has a broken schema reference.
+func TestReadConfigurationDefinitions_DropsBrokenSchemaField(t *testing.T) {
+	tests := []struct {
+		name        string
+		yamlContent string
+	}{
+		{
+			name: "schema path points to missing file",
+			yamlContent: `configurationDefinitions:
+  - platform: linux
+    description: good entry
+    type: test-config
+    version: 1.0.0
+    format: yaml
+    schema: ./schemas/does-not-exist.json`,
+		},
+		{
+			name: "schema field is not a string",
+			yamlContent: `configurationDefinitions:
+  - platform: linux
+    description: good entry
+    type: test-config
+    version: 1.0.0
+    format: yaml
+    schema: 12345`,
+		},
+		{
+			name: "schema path escapes workspace",
+			yamlContent: `configurationDefinitions:
+  - platform: linux
+    description: good entry
+    type: test-config
+    version: 1.0.0
+    format: yaml
+    schema: ../../../etc/passwd`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tmpDir := t.TempDir()
+			configDir := filepath.Join(tmpDir, config.GetRootFolderForAgentRepo())
+			require.NoError(t, os.MkdirAll(configDir, 0755))
+
+			configFile := filepath.Join(configDir, config.GetConfigurationDefinitionsFilename())
+			require.NoError(t, os.WriteFile(configFile, []byte(tt.yamlContent), 0644))
+
+			configs, err := ReadConfigurationDefinitions(context.Background(), tmpDir)
+			require.NoError(t, err)
+			require.Len(t, configs, 1)
+
+			_, hasSchema := configs[0]["schema"]
+			assert.False(t, hasSchema, "schema field should be removed when load fails, but got: %v", configs[0]["schema"])
+			// Sibling fields stay so the rest of the entry can still ship.
+			assert.Equal(t, "good entry", configs[0]["description"])
+		})
+	}
+}
+
+// TestReadAgentControlDefinitions_DropsBrokenContentField is the agent-control
+// counterpart to TestReadConfigurationDefinitions_DropsBrokenSchemaField.
+func TestReadAgentControlDefinitions_DropsBrokenContentField(t *testing.T) {
+	tests := []struct {
+		name        string
+		yamlContent string
+	}{
+		{
+			name: "content path points to missing file",
+			yamlContent: `agentControlDefinitions:
+  - platform: KUBERNETES
+    supportFromAgent: 1.0.0
+    supportFromAgentControl: 1.0.0
+    content: ./agentControl/does-not-exist.yml`,
+		},
+		{
+			name: "content field is not a string",
+			yamlContent: `agentControlDefinitions:
+  - platform: KUBERNETES
+    supportFromAgent: 1.0.0
+    supportFromAgentControl: 1.0.0
+    content: 42`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tmpDir := t.TempDir()
+			configDir := filepath.Join(tmpDir, config.GetRootFolderForAgentRepo())
+			require.NoError(t, os.MkdirAll(configDir, 0755))
+
+			agentControlFile := filepath.Join(configDir, config.GetAgentControlDefinitionsFilename())
+			require.NoError(t, os.WriteFile(agentControlFile, []byte(tt.yamlContent), 0644))
+
+			defs, err := ReadAgentControlDefinitions(context.Background(), tmpDir)
+			require.NoError(t, err)
+			require.Len(t, defs, 1)
+
+			_, hasContent := defs[0]["content"]
+			assert.False(t, hasContent, "content field should be removed when load fails, but got: %v", defs[0]["content"])
+			// Sibling fields stay so the rest of the entry can still ship.
+			assert.Equal(t, "KUBERNETES", defs[0]["platform"])
+		})
+	}
+}
