From e6d991b2cc2dbc3fb74c659afe546719af47297c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 05:29:00 +0000 Subject: [PATCH 1/4] Initial plan From a55d812ff6822875d21dfdf65218b2db23a523a2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 05:52:12 +0000 Subject: [PATCH 2/4] feat: add AuthDefinition and RequestShape for provider-owned auth and request shaping (Phase 4) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/engine_secrets.go | 47 ++ pkg/cli/workflow_secrets.go | 55 +- pkg/parser/schemas/main_workflow_schema.json | 88 ++++ pkg/workflow/compiler_orchestrator_engine.go | 3 + pkg/workflow/engine.go | 77 ++- pkg/workflow/engine_auth_test.go | 498 +++++++++++++++++++ pkg/workflow/engine_definition.go | 94 +++- pkg/workflow/engine_validation.go | 66 ++- pkg/workflow/strict_mode_validation.go | 11 + 9 files changed, 930 insertions(+), 9 deletions(-) create mode 100644 pkg/workflow/engine_auth_test.go diff --git a/pkg/cli/engine_secrets.go b/pkg/cli/engine_secrets.go index b56034b31a6..eb9de5f1538 100644 --- a/pkg/cli/engine_secrets.go +++ b/pkg/cli/engine_secrets.go @@ -102,6 +102,53 @@ func getEngineSecretDescription(opt *constants.EngineOption) string { } } +// secretRequirementsFromAuthDefinition converts an AuthDefinition into SecretRequirement +// entries so that auth-binding secrets are treated as required secrets (same as built-in +// engine secrets). Returns nil when auth is nil. +func secretRequirementsFromAuthDefinition(auth *workflow.AuthDefinition, engineName string) []SecretRequirement { + if auth == nil { + return nil + } + + var reqs []SecretRequirement + + switch auth.Strategy { + case workflow.AuthStrategyOAuthClientCreds: + // OAuth client-credentials flow: require client-id and client-secret secrets. + if auth.ClientIDRef != "" { + reqs = append(reqs, SecretRequirement{ + Name: auth.ClientIDRef, + WhenNeeded: fmt.Sprintf("OAuth client ID for %s engine", engineName), + Description: "GitHub Actions secret holding the OAuth 2.0 client ID used to obtain access tokens.", + IsEngineSecret: true, + EngineName: engineName, + }) + } + if auth.ClientSecretRef != "" { + reqs = append(reqs, SecretRequirement{ + Name: auth.ClientSecretRef, + WhenNeeded: fmt.Sprintf("OAuth client secret for %s engine", engineName), + Description: "GitHub Actions secret holding the OAuth 2.0 client secret used to obtain access tokens.", + IsEngineSecret: true, + EngineName: engineName, + }) + } + default: + // api-key, bearer, or unset strategy: require the direct secret. + if auth.Secret != "" { + reqs = append(reqs, SecretRequirement{ + Name: auth.Secret, + WhenNeeded: fmt.Sprintf("API key or token for %s engine", engineName), + Description: "GitHub Actions secret holding the API key or bearer token for provider authentication.", + IsEngineSecret: true, + EngineName: engineName, + }) + } + } + + return reqs +} + // getMissingRequiredSecrets filters requirements to return only missing required secrets. // It skips optional secrets and checks both primary and alternative secret names. func getMissingRequiredSecrets(requirements []SecretRequirement, existingSecrets map[string]bool) []SecretRequirement { diff --git a/pkg/cli/workflow_secrets.go b/pkg/cli/workflow_secrets.go index c13237cf281..b8cbecaf459 100644 --- a/pkg/cli/workflow_secrets.go +++ b/pkg/cli/workflow_secrets.go @@ -1,8 +1,13 @@ package cli import ( + "fmt" + "os" + "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/workflow" ) var workflowSecretsLog = logger.New("cli:workflow_secrets") @@ -45,7 +50,8 @@ func getSecretsRequirementsForWorkflows(workflowFiles []string) []SecretRequirem return allRequirements } -// getSecretRequirementsForWorkflow extracts the engine from a workflow file and returns its required secrets +// getSecretRequirementsForWorkflow extracts the engine from a workflow file and returns its required secrets. +// It also extracts AuthDefinition secrets from inline engine definitions. // // NOTE: In future we will want to analyse more parts of the // workflow to work out other secrets required, or detect that the particular @@ -55,7 +61,7 @@ func getSecretRequirementsForWorkflow(workflowFile string) []SecretRequirement { workflowSecretsLog.Printf("Extracting secrets for workflow: %s", workflowFile) // Extract engine from workflow file - engine := extractEngineIDFromFile(workflowFile) + engine, engineConfig := extractEngineConfigFromFile(workflowFile) if engine == "" { workflowSecretsLog.Printf("No engine found in workflow %s, skipping", workflowFile) return nil @@ -65,5 +71,48 @@ func getSecretRequirementsForWorkflow(workflowFile string) []SecretRequirement { // Get engine-specific secrets only (no system secrets, no optional) // System secrets will be added separately to avoid duplication - return getSecretRequirementsForEngine(engine, true, true) + reqs := getSecretRequirementsForEngine(engine, true, true) + + // For inline engine definitions with an AuthDefinition, also include auth secrets. + if engineConfig != nil && engineConfig.InlineProviderAuth != nil { + authReqs := secretRequirementsFromAuthDefinition(engineConfig.InlineProviderAuth, engine) + workflowSecretsLog.Printf("Adding %d auth definition secret(s) for workflow %s", len(authReqs), workflowFile) + reqs = append(reqs, authReqs...) + } + + return reqs +} + +// extractEngineConfigFromFile parses a workflow file and returns the engine ID and config. +// Returns ("", nil) when the file cannot be read or parsed. +func extractEngineConfigFromFile(filePath string) (string, *workflow.EngineConfig) { + content, err := readWorkflowFileContent(filePath) + if err != nil { + return "", nil + } + + result, err := parser.ExtractFrontmatterFromContent(content) + if err != nil { + return "", nil + } + + compiler := &workflow.Compiler{} + engineSetting, engineConfig := compiler.ExtractEngineConfig(result.Frontmatter) + + if engineConfig != nil && engineConfig.ID != "" { + return engineConfig.ID, engineConfig + } + if engineSetting != "" { + return engineSetting, engineConfig + } + return "copilot", engineConfig // Default engine +} + +// readWorkflowFileContent reads a workflow file's content as a string. +func readWorkflowFileContent(filePath string) (string, error) { + content, err := os.ReadFile(filePath) + if err != nil { + return "", fmt.Errorf("reading workflow file %s: %w", filePath, err) + } + return string(content), nil } diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index a99851517ad..59fbf41354d 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -7799,6 +7799,28 @@ "provider": { "model": "claude-3-7-sonnet-20250219" } + }, + { + "runtime": { + "id": "codex" + }, + "provider": { + "id": "azure-openai", + "model": "gpt-4o", + "auth": { + "strategy": "oauth-client-credentials", + "token-url": "https://auth.example.com/oauth/token", + "client-id": "AZURE_CLIENT_ID", + "client-secret": "AZURE_CLIENT_SECRET", + "header-name": "api-key" + }, + "request": { + "path-template": "/openai/deployments/{model}/chat/completions", + "query": { + "api-version": "2024-10-01-preview" + } + } + } } ], "oneOf": [ @@ -7978,6 +8000,72 @@ "type": "string", "description": "Name of the GitHub Actions secret that contains the API key for this provider", "examples": ["OPENAI_API_KEY", "ANTHROPIC_API_KEY", "CUSTOM_API_KEY"] + }, + "strategy": { + "type": "string", + "enum": ["api-key", "oauth-client-credentials", "bearer"], + "description": "Authentication strategy for the provider (default: api-key when secret is set)" + }, + "token-url": { + "type": "string", + "description": "OAuth 2.0 token endpoint URL. Required when strategy is 'oauth-client-credentials'.", + "examples": ["https://auth.example.com/oauth/token"] + }, + "client-id": { + "type": "string", + "description": "GitHub Actions secret name that holds the OAuth client ID. Required when strategy is 'oauth-client-credentials'.", + "examples": ["OAUTH_CLIENT_ID"] + }, + "client-secret": { + "type": "string", + "description": "GitHub Actions secret name that holds the OAuth client secret. Required when strategy is 'oauth-client-credentials'.", + "examples": ["OAUTH_CLIENT_SECRET"] + }, + "token-field": { + "type": "string", + "description": "JSON field name in the token response that contains the access token. Defaults to 'access_token'.", + "examples": ["access_token", "token"] + }, + "header-name": { + "type": "string", + "description": "HTTP header name to inject the API key or token into (e.g. 'api-key', 'x-api-key'). Required when strategy is not 'bearer'.", + "examples": ["api-key", "x-api-key", "Authorization"] + } + }, + "additionalProperties": false + }, + "request": { + "type": "object", + "description": "Request shaping configuration for non-standard provider URL and body transformations", + "properties": { + "path-template": { + "type": "string", + "description": "URL path template with {model} and other variable placeholders (e.g. '/openai/deployments/{model}/chat/completions')", + "examples": ["/openai/deployments/{model}/chat/completions"] + }, + "query": { + "type": "object", + "description": "Static or template query-parameter values appended to every request", + "additionalProperties": { + "type": "string" + }, + "examples": [ + { + "api-version": "2024-10-01-preview" + } + ] + }, + "body-inject": { + "type": "object", + "description": "Key/value pairs injected into the JSON request body before sending", + "additionalProperties": { + "type": "string" + }, + "examples": [ + { + "appKey": "{APP_KEY_SECRET}" + } + ] } }, "additionalProperties": false diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index b26e9aed8fc..6e582e85dca 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -42,6 +42,9 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean if err := c.validateEngineInlineDefinition(engineConfig); err != nil { return nil, err } + if err := c.validateEngineAuthDefinition(engineConfig); err != nil { + return nil, err + } c.registerInlineEngineDefinition(engineConfig) } diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index fd8ae81ace2..ad0e3043623 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -30,7 +30,15 @@ type EngineConfig struct { // Inline definition fields (populated when engine.runtime is specified in frontmatter) IsInlineDefinition bool // true when the engine is defined inline via engine.runtime + optional engine.provider InlineProviderID string // engine.provider.id (e.g. "openai", "anthropic") - InlineProviderSecret string // engine.provider.auth.secret (name of the GitHub Actions secret for the provider API key) + // Deprecated: Use InlineProviderAuth instead. Kept for backwards compatibility when only + // engine.provider.auth.secret is specified without a strategy. + InlineProviderSecret string // engine.provider.auth.secret (backwards compat: simple API key secret name) + + // Extended inline auth fields (engine.provider.auth.* beyond the simple secret) + InlineProviderAuth *AuthDefinition // full auth definition parsed from engine.provider.auth + + // Extended inline request shaping fields (engine.provider.request.*) + InlineProviderRequest *RequestShape // request shaping parsed from engine.provider.request } // NetworkPermissions represents network access permissions for workflow execution @@ -118,9 +126,15 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng } if auth, hasAuth := providerObj["auth"]; hasAuth { if authObj, ok := auth.(map[string]any); ok { - if secret, ok := authObj["secret"].(string); ok { - config.InlineProviderSecret = secret - } + authDef := parseAuthDefinition(authObj) + config.InlineProviderAuth = authDef + // Backwards compat: expose the simple secret field directly. + config.InlineProviderSecret = authDef.Secret + } + } + if request, hasRequest := providerObj["request"]; hasRequest { + if requestObj, ok := request.(map[string]any); ok { + config.InlineProviderRequest = parseRequestShape(requestObj) } } } @@ -351,3 +365,58 @@ func (c *Compiler) extractEngineConfigFromJSON(engineJSON string) (*EngineConfig _, config := c.ExtractEngineConfig(tempFrontmatter) return config, nil } + +// parseAuthDefinition converts a raw auth config map (from engine.provider.auth) into +// an AuthDefinition. It is backward-compatible: a map with only a "secret" key produces +// an AuthDefinition with Strategy="" and Secret set (callers normalise Strategy to api-key). +func parseAuthDefinition(authObj map[string]any) *AuthDefinition { + def := &AuthDefinition{} + if s, ok := authObj["strategy"].(string); ok { + def.Strategy = AuthStrategy(s) + } + if s, ok := authObj["secret"].(string); ok { + def.Secret = s + } + if s, ok := authObj["token-url"].(string); ok { + def.TokenURL = s + } + if s, ok := authObj["client-id"].(string); ok { + def.ClientIDRef = s + } + if s, ok := authObj["client-secret"].(string); ok { + def.ClientSecretRef = s + } + if s, ok := authObj["token-field"].(string); ok { + def.TokenField = s + } + if s, ok := authObj["header-name"].(string); ok { + def.HeaderName = s + } + return def +} + +// parseRequestShape converts a raw request config map (from engine.provider.request) into +// a RequestShape. +func parseRequestShape(requestObj map[string]any) *RequestShape { + shape := &RequestShape{} + if s, ok := requestObj["path-template"].(string); ok { + shape.PathTemplate = s + } + if q, ok := requestObj["query"].(map[string]any); ok { + shape.Query = make(map[string]string, len(q)) + for k, v := range q { + if vs, ok := v.(string); ok { + shape.Query[k] = vs + } + } + } + if b, ok := requestObj["body-inject"].(map[string]any); ok { + shape.BodyInject = make(map[string]string, len(b)) + for k, v := range b { + if vs, ok := v.(string); ok { + shape.BodyInject[k] = vs + } + } + } + return shape +} diff --git a/pkg/workflow/engine_auth_test.go b/pkg/workflow/engine_auth_test.go new file mode 100644 index 00000000000..ee8d73a2419 --- /dev/null +++ b/pkg/workflow/engine_auth_test.go @@ -0,0 +1,498 @@ +//go:build !integration + +// This file contains tests for AuthDefinition and RequestShape validation. +// It covers: +// - OAuth client-credentials definition validates correctly +// - Missing tokenUrl/clientId/clientSecret produce helpful errors +// - Unknown auth strategy produces a clear error +// - Strict mode includes auth-binding secrets in required secret list +// - Existing built-in auth flows are unchanged (regression) + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAuthDefinition_RequiredSecretNames verifies that RequiredSecretNames returns the +// correct secret names depending on the auth strategy. +func TestAuthDefinition_RequiredSecretNames(t *testing.T) { + tests := []struct { + name string + auth *AuthDefinition + expected []string + }{ + { + name: "nil auth returns empty", + auth: nil, + expected: nil, + }, + { + name: "api-key strategy returns Secret", + auth: &AuthDefinition{ + Strategy: AuthStrategyAPIKey, + Secret: "MY_API_KEY", + }, + expected: []string{"MY_API_KEY"}, + }, + { + name: "bearer strategy returns Secret", + auth: &AuthDefinition{ + Strategy: AuthStrategyBearer, + Secret: "MY_BEARER_TOKEN", + }, + expected: []string{"MY_BEARER_TOKEN"}, + }, + { + name: "oauth-client-credentials returns ClientIDRef and ClientSecretRef", + auth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + HeaderName: "api-key", + }, + expected: []string{"MY_CLIENT_ID", "MY_CLIENT_SECRET"}, + }, + { + name: "oauth with only ClientIDRef returns just that", + auth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + ClientIDRef: "MY_CLIENT_ID", + }, + expected: []string{"MY_CLIENT_ID"}, + }, + { + name: "empty strategy with Secret treated like api-key/bearer", + auth: &AuthDefinition{ + Secret: "SOME_SECRET", + }, + expected: []string{"SOME_SECRET"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := tt.auth.RequiredSecretNames() + assert.Equal(t, tt.expected, result, "RequiredSecretNames() should return expected secrets") + }) + } +} + +// TestValidateEngineAuthDefinition_OAuthClientCredentials checks that a valid +// oauth-client-credentials definition passes validation. +func TestValidateEngineAuthDefinition_OAuthClientCredentials(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + HeaderName: "api-key", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.NoError(t, err, "valid oauth-client-credentials definition should pass validation") +} + +// TestValidateEngineAuthDefinition_MissingTokenURL verifies that omitting token-url +// in an oauth-client-credentials definition produces a clear error. +func TestValidateEngineAuthDefinition_MissingTokenURL(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + HeaderName: "api-key", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "missing token-url should produce an error") + assert.Contains(t, err.Error(), "token-url", "error should mention missing token-url field") + assert.Contains(t, err.Error(), "oauth-client-credentials", "error should mention the strategy") +} + +// TestValidateEngineAuthDefinition_MissingClientID verifies that omitting client-id +// produces a clear error. +func TestValidateEngineAuthDefinition_MissingClientID(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientSecretRef: "MY_CLIENT_SECRET", + HeaderName: "api-key", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "missing client-id should produce an error") + assert.Contains(t, err.Error(), "client-id", "error should mention missing client-id field") +} + +// TestValidateEngineAuthDefinition_MissingClientSecret verifies that omitting client-secret +// produces a clear error. +func TestValidateEngineAuthDefinition_MissingClientSecret(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + HeaderName: "api-key", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "missing client-secret should produce an error") + assert.Contains(t, err.Error(), "client-secret", "error should mention missing client-secret field") +} + +// TestValidateEngineAuthDefinition_MissingHeaderNameForOAuth verifies that omitting +// header-name for oauth-client-credentials produces a clear error. +func TestValidateEngineAuthDefinition_MissingHeaderNameForOAuth(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "missing header-name for oauth should produce an error") + assert.Contains(t, err.Error(), "header-name", "error should mention missing header-name field") +} + +// TestValidateEngineAuthDefinition_MissingHeaderNameForAPIKey verifies that api-key +// strategy without header-name produces a clear error. +func TestValidateEngineAuthDefinition_MissingHeaderNameForAPIKey(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyAPIKey, + Secret: "MY_API_KEY", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "api-key without header-name should produce an error") + assert.Contains(t, err.Error(), "header-name", "error should mention missing header-name field") +} + +// TestValidateEngineAuthDefinition_BearerNoHeaderRequired verifies that bearer strategy +// does not require header-name (it uses the standard Authorization header by convention). +func TestValidateEngineAuthDefinition_BearerNoHeaderRequired(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyBearer, + Secret: "MY_BEARER_TOKEN", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + assert.NoError(t, err, "bearer strategy without header-name should be valid") +} + +// TestValidateEngineAuthDefinition_UnknownStrategy verifies that an unknown strategy +// produces a clear error listing the valid strategies. +func TestValidateEngineAuthDefinition_UnknownStrategy(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: "invalid-strategy", + Secret: "MY_SECRET", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "unknown strategy should produce an error") + assert.Contains(t, err.Error(), "invalid-strategy", "error should mention the unknown strategy") + assert.Contains(t, err.Error(), "api-key", "error should list valid strategies") + assert.Contains(t, err.Error(), "oauth-client-credentials", "error should list valid strategies") + assert.Contains(t, err.Error(), "bearer", "error should list valid strategies") +} + +// TestValidateEngineAuthDefinition_NilAuth verifies that a nil InlineProviderAuth is a no-op. +func TestValidateEngineAuthDefinition_NilAuth(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: nil, + } + + err := compiler.validateEngineAuthDefinition(config) + assert.NoError(t, err, "nil auth definition should pass validation") +} + +// TestRegisterInlineEngineDefinition_WithAuthDefinition checks that an inline engine +// definition with an AuthDefinition is correctly stored in the catalog. +func TestRegisterInlineEngineDefinition_WithAuthDefinition(t *testing.T) { + compiler := newTestCompiler(t) + + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderID: "azure-openai", + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + HeaderName: "api-key", + }, + InlineProviderRequest: &RequestShape{ + PathTemplate: "/openai/deployments/{model}/chat/completions", + Query: map[string]string{"api-version": "2024-10-01-preview"}, + }, + } + + compiler.registerInlineEngineDefinition(config) + + def := compiler.engineCatalog.Get("codex") + require.NotNil(t, def, "engine definition should be in catalog after registration") + assert.Equal(t, "azure-openai", def.Provider.Name, "provider name should be set") + require.NotNil(t, def.Provider.Auth, "AuthDefinition should be populated") + assert.Equal(t, AuthStrategyOAuthClientCreds, def.Provider.Auth.Strategy, "strategy should be oauth-client-credentials") + assert.Equal(t, "MY_CLIENT_ID", def.Provider.Auth.ClientIDRef, "ClientIDRef should be set") + assert.Equal(t, "MY_CLIENT_SECRET", def.Provider.Auth.ClientSecretRef, "ClientSecretRef should be set") + require.NotNil(t, def.Provider.Request, "RequestShape should be populated") + assert.Equal(t, "/openai/deployments/{model}/chat/completions", def.Provider.Request.PathTemplate, "PathTemplate should match") +} + +// TestRegisterInlineEngineDefinition_BackwardsCompatSimpleSecret checks that the legacy +// auth.secret path (no strategy) still works with backwards-compat normalization. +func TestRegisterInlineEngineDefinition_BackwardsCompatSimpleSecret(t *testing.T) { + compiler := newTestCompiler(t) + + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderSecret: "MY_API_KEY", + InlineProviderAuth: &AuthDefinition{ + Secret: "MY_API_KEY", + // Strategy intentionally empty (backwards compat) + }, + } + + compiler.registerInlineEngineDefinition(config) + + def := compiler.engineCatalog.Get("codex") + require.NotNil(t, def, "engine definition should be in catalog") + require.NotNil(t, def.Provider.Auth, "AuthDefinition should be populated") + assert.Equal(t, AuthStrategyAPIKey, def.Provider.Auth.Strategy, + "empty strategy with Secret should be normalised to api-key") + assert.Equal(t, "MY_API_KEY", def.Provider.Auth.Secret, "Secret should be set") +} + +// TestStrictModeGetEngineBaseEnvVarKeys_IncludesAuthSecrets verifies that +// getEngineBaseEnvVarKeys includes secrets from an inline engine's AuthDefinition. +func TestStrictModeGetEngineBaseEnvVarKeys_IncludesAuthSecrets(t *testing.T) { + compiler := newTestCompiler(t) + + // Register an inline definition with an AuthDefinition so the catalog carries it. + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + HeaderName: "api-key", + }, + } + compiler.registerInlineEngineDefinition(config) + + keys := compiler.getEngineBaseEnvVarKeys("codex") + assert.True(t, keys["MY_CLIENT_ID"], "client ID secret should be in allowed env-var keys") + assert.True(t, keys["MY_CLIENT_SECRET"], "client secret should be in allowed env-var keys") +} + +// TestBuiltInEngineAuthUnchanged is a regression test verifying that the built-in engines +// (claude, codex, copilot, gemini) retain their original auth configuration after the +// AuthDefinition changes. +func TestBuiltInEngineAuthUnchanged(t *testing.T) { + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + + tests := []struct { + engineID string + wantAuthSecret string // expected legacy AuthBinding secret (empty = no binding) + }{ + {"claude", "ANTHROPIC_API_KEY"}, + {"codex", "CODEX_API_KEY"}, + {"copilot", ""}, // copilot has no API-key binding + {"gemini", ""}, // gemini has no API-key binding + } + + for _, tt := range tests { + t.Run(tt.engineID, func(t *testing.T) { + def := catalog.Get(tt.engineID) + require.NotNil(t, def, "built-in engine %s should be in catalog", tt.engineID) + + // Provider.Auth should be nil for built-in engines (they use AuthBinding only). + assert.Nil(t, def.Provider.Auth, + "built-in engine %s should have no Provider.Auth (uses legacy AuthBinding)", tt.engineID) + + if tt.wantAuthSecret != "" { + require.Len(t, def.Auth, 1, "engine %s should have exactly one AuthBinding", tt.engineID) + assert.Equal(t, tt.wantAuthSecret, def.Auth[0].Secret, + "engine %s AuthBinding.Secret should be unchanged", tt.engineID) + } else { + assert.Empty(t, def.Auth, "engine %s should have no AuthBinding", tt.engineID) + } + }) + } +} + +// TestParseAuthDefinition verifies that parseAuthDefinition correctly maps YAML map keys +// to AuthDefinition fields. +func TestParseAuthDefinition(t *testing.T) { + tests := []struct { + name string + input map[string]any + expected AuthDefinition + }{ + { + name: "empty map produces zero-value AuthDefinition", + input: map[string]any{}, + expected: AuthDefinition{}, + }, + { + name: "simple secret only (backwards compat)", + input: map[string]any{ + "secret": "MY_API_KEY", + }, + expected: AuthDefinition{Secret: "MY_API_KEY"}, + }, + { + name: "full oauth-client-credentials config", + input: map[string]any{ + "strategy": "oauth-client-credentials", + "token-url": "https://auth.example.com/oauth/token", + "client-id": "MY_CLIENT_ID", + "client-secret": "MY_CLIENT_SECRET", + "token-field": "access_token", + "header-name": "api-key", + }, + expected: AuthDefinition{ + Strategy: AuthStrategyOAuthClientCreds, + TokenURL: "https://auth.example.com/oauth/token", + ClientIDRef: "MY_CLIENT_ID", + ClientSecretRef: "MY_CLIENT_SECRET", + TokenField: "access_token", + HeaderName: "api-key", + }, + }, + { + name: "api-key strategy with header-name", + input: map[string]any{ + "strategy": "api-key", + "secret": "CUSTOM_API_KEY", + "header-name": "x-api-key", + }, + expected: AuthDefinition{ + Strategy: AuthStrategyAPIKey, + Secret: "CUSTOM_API_KEY", + HeaderName: "x-api-key", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := parseAuthDefinition(tt.input) + require.NotNil(t, result, "parseAuthDefinition should never return nil") + assert.Equal(t, tt.expected, *result, "parsed AuthDefinition should match expected") + }) + } +} + +// TestParseRequestShape verifies that parseRequestShape correctly maps YAML map keys +// to RequestShape fields. +func TestParseRequestShape(t *testing.T) { + tests := []struct { + name string + input map[string]any + expected RequestShape + }{ + { + name: "empty map produces zero-value RequestShape", + input: map[string]any{}, + expected: RequestShape{}, + }, + { + name: "path-template only", + input: map[string]any{ + "path-template": "/openai/deployments/{model}/chat/completions", + }, + expected: RequestShape{ + PathTemplate: "/openai/deployments/{model}/chat/completions", + }, + }, + { + name: "full request shape", + input: map[string]any{ + "path-template": "/openai/deployments/{model}/chat/completions", + "query": map[string]any{ + "api-version": "2024-10-01-preview", + }, + "body-inject": map[string]any{ + "appKey": "{APP_KEY_SECRET}", + }, + }, + expected: RequestShape{ + PathTemplate: "/openai/deployments/{model}/chat/completions", + Query: map[string]string{"api-version": "2024-10-01-preview"}, + BodyInject: map[string]string{"appKey": "{APP_KEY_SECRET}"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := parseRequestShape(tt.input) + require.NotNil(t, result, "parseRequestShape should never return nil") + assert.Equal(t, tt.expected, *result, "parsed RequestShape should match expected") + }) + } +} + +// newTestCompiler creates a minimal Compiler for unit testing engine-definition logic. +func newTestCompiler(t *testing.T) *Compiler { + t.Helper() + registry := NewEngineRegistry() + catalog := NewEngineCatalog(registry) + return &Compiler{ + engineRegistry: registry, + engineCatalog: catalog, + } +} diff --git a/pkg/workflow/engine_definition.go b/pkg/workflow/engine_definition.go index 17975c6f035..ace1366b688 100644 --- a/pkg/workflow/engine_definition.go +++ b/pkg/workflow/engine_definition.go @@ -36,9 +36,77 @@ import ( "github.com/github/gh-aw/pkg/parser" ) +// AuthStrategy identifies how an engine authenticates with its provider. +type AuthStrategy string + +const ( + // AuthStrategyAPIKey uses a direct API key sent via a header (default when Secret is set). + AuthStrategyAPIKey AuthStrategy = "api-key" + // AuthStrategyOAuthClientCreds exchanges client credentials for a bearer token before each call. + AuthStrategyOAuthClientCreds AuthStrategy = "oauth-client-credentials" + // AuthStrategyBearer sends a pre-obtained token as a standard Authorization: Bearer header. + AuthStrategyBearer AuthStrategy = "bearer" +) + +// AuthDefinition describes how the engine authenticates with a provider backend. +// It extends the simple AuthBinding model to support OAuth client-credentials flows, +// custom header injection, and template-based secret references. +// +// For backwards compatibility, a plain auth.secret field without a strategy is treated as +// AuthStrategyAPIKey. +type AuthDefinition struct { + // Strategy selects the authentication flow (api-key, oauth-client-credentials, bearer). + // Defaults to api-key when Secret is non-empty and Strategy is unset. + Strategy AuthStrategy + + // Secret is the env-var / GitHub Actions secret name that holds the raw API key or token. + // Required for api-key and bearer strategies. + Secret string + + // TokenURL is the OAuth token endpoint (e.g. "https://auth.example.com/oauth/token"). + // Required for oauth-client-credentials strategy. + TokenURL string + + // ClientIDRef is the secret name that holds the OAuth client ID. + // Required for oauth-client-credentials strategy. + ClientIDRef string + + // ClientSecretRef is the secret name that holds the OAuth client secret. + // Required for oauth-client-credentials strategy. + ClientSecretRef string + + // TokenField is the JSON field name in the token response that contains the access token. + // Defaults to "access_token" when empty. + TokenField string + + // HeaderName is the HTTP header to inject the token into (e.g. "api-key"). + // Required when strategy is not bearer (bearer always uses Authorization header). + HeaderName string +} + +// RequestShape describes non-standard URL and body transformations applied to each +// API call before it is sent to the provider backend. +type RequestShape struct { + // PathTemplate is a URL path template with {model} and other variable placeholders + // (e.g. "/openai/deployments/{model}/chat/completions"). + PathTemplate string + + // Query holds static or template query-parameter values appended to every request + // (e.g. {"api-version": "2024-10-01-preview"}). + Query map[string]string + + // BodyInject holds key/value pairs injected into the JSON request body before sending + // (e.g. {"appKey": "{APP_KEY_SECRET}"}). + BodyInject map[string]string +} + // ProviderSelection identifies the AI provider for an engine (e.g. "anthropic", "openai"). +// It optionally carries advanced authentication and request-shaping configuration for +// non-standard backends. type ProviderSelection struct { - Name string + Name string + Auth *AuthDefinition + Request *RequestShape } // ModelSelection specifies the default and supported models for an engine. @@ -53,6 +121,30 @@ type AuthBinding struct { Secret string } +// RequiredSecretNames returns the env-var names that must be provided at runtime for +// this AuthDefinition. Returns an empty slice when Auth is nil. +func (a *AuthDefinition) RequiredSecretNames() []string { + if a == nil { + return nil + } + var secrets []string + switch a.Strategy { + case AuthStrategyOAuthClientCreds: + if a.ClientIDRef != "" { + secrets = append(secrets, a.ClientIDRef) + } + if a.ClientSecretRef != "" { + secrets = append(secrets, a.ClientSecretRef) + } + default: + // api-key, bearer, or unset strategy – Secret is the raw credential. + if a.Secret != "" { + secrets = append(secrets, a.Secret) + } + } + return secrets +} + // EngineDefinition holds the declarative metadata for an AI engine. // It is separate from the runtime adapter (CodingAgentEngine) to allow the catalog // layer to carry identity and provider information without coupling to implementation. diff --git a/pkg/workflow/engine_validation.go b/pkg/workflow/engine_validation.go index d3c6b44dcf6..1ef6a2c1e27 100644 --- a/pkg/workflow/engine_validation.go +++ b/pkg/workflow/engine_validation.go @@ -109,15 +109,79 @@ func (c *Compiler) registerInlineEngineDefinition(config *EngineConfig) { if config.InlineProviderID != "" { def.Provider = ProviderSelection{Name: config.InlineProviderID} } - if config.InlineProviderSecret != "" { + + // Prefer the full AuthDefinition over the legacy simple-secret path. + if config.InlineProviderAuth != nil { + // Normalise strategy: treat empty strategy as api-key when a secret is set. + auth := config.InlineProviderAuth + if auth.Strategy == "" && auth.Secret != "" { + auth.Strategy = AuthStrategyAPIKey + } + def.Provider.Auth = auth + // Keep legacy AuthBinding in sync for callers that still read def.Auth. + if auth.Secret != "" { + def.Auth = []AuthBinding{{Role: string(auth.Strategy), Secret: auth.Secret}} + } + } else if config.InlineProviderSecret != "" { def.Auth = []AuthBinding{{Role: "api-key", Secret: config.InlineProviderSecret}} } + if config.InlineProviderRequest != nil { + def.Provider.Request = config.InlineProviderRequest + } + engineValidationLog.Printf("Registering inline engine definition in session catalog: id=%s, runtimeID=%s, providerID=%s", def.ID, def.RuntimeID, def.Provider.Name) c.engineCatalog.Register(def) } +// validateEngineAuthDefinition validates AuthDefinition fields for an inline engine definition. +// Returns an error describing the first (or all, in non-fail-fast mode) validation problems found. +func (c *Compiler) validateEngineAuthDefinition(config *EngineConfig) error { + auth := config.InlineProviderAuth + if auth == nil { + return nil + } + + engineValidationLog.Printf("Validating engine auth definition: strategy=%s", auth.Strategy) + + switch auth.Strategy { + case AuthStrategyOAuthClientCreds: + // oauth-client-credentials requires tokenUrl, clientId, clientSecret. + if auth.TokenURL == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.token-url' to be set.\n\nExample:\nengine:\n runtime:\n id: codex\n provider:\n auth:\n strategy: oauth-client-credentials\n token-url: https://auth.example.com/oauth/token\n client-id: MY_CLIENT_ID_SECRET\n client-secret: MY_CLIENT_SECRET_SECRET\n\nSee: %s", constants.DocsEnginesURL) + } + if auth.ClientIDRef == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.client-id' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } + if auth.ClientSecretRef == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.client-secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } + // For oauth, header-name is required (the token must go somewhere). + if auth.HeaderName == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.header-name' to be set (e.g. 'api-key' or 'Authorization').\n\nSee: %s", constants.DocsEnginesURL) + } + case AuthStrategyAPIKey: + // api-key requires a header-name so the caller knows where to inject the key. + if auth.HeaderName == "" { + return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.header-name' to be set (e.g. 'api-key' or 'x-api-key').\n\nSee: %s", constants.DocsEnginesURL) + } + case AuthStrategyBearer, "": + // bearer strategy and unset strategy (simple backwards-compat secret) need no extra fields. + default: + validStrategies := []string{ + string(AuthStrategyAPIKey), + string(AuthStrategyOAuthClientCreds), + string(AuthStrategyBearer), + } + return fmt.Errorf("engine auth: unknown strategy %q. Valid strategies are: %s.\n\nSee: %s", + auth.Strategy, strings.Join(validStrategies, ", "), constants.DocsEnginesURL) + } + + engineValidationLog.Printf("Engine auth definition is valid: strategy=%s", auth.Strategy) + return nil +} + // validateEngine validates that the given engine ID is supported func (c *Compiler) validateEngine(engineID string) error { if engineID == "" { diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go index db2f3edcd5b..eca8b545c42 100644 --- a/pkg/workflow/strict_mode_validation.go +++ b/pkg/workflow/strict_mode_validation.go @@ -290,6 +290,17 @@ func (c *Compiler) getEngineBaseEnvVarKeys(engineID string) map[string]bool { for _, name := range engine.GetRequiredSecretNames(minimalData) { keys[name] = true } + + // Also include secrets declared in the AuthDefinition for inline engine definitions. + // This allows workflows to pass auth secrets through engine.env without triggering the + // "secret in env" strict-mode check. + if def := c.engineCatalog.Get(engineID); def != nil && def.Provider.Auth != nil { + for _, name := range def.Provider.Auth.RequiredSecretNames() { + strictModeValidationLog.Printf("Adding auth-definition secret key to allowlist: %s", name) + keys[name] = true + } + } + return keys } From f6248783d4e50aa8f96e84f10efd4971eee67252 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 06:11:25 +0000 Subject: [PATCH 3/4] fix: run gofmt on engine.go to fix lint failure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index ad0e3043623..75f57b88213 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -28,8 +28,8 @@ type EngineConfig struct { Agent string // Agent identifier for copilot --agent flag (copilot engine only) // Inline definition fields (populated when engine.runtime is specified in frontmatter) - IsInlineDefinition bool // true when the engine is defined inline via engine.runtime + optional engine.provider - InlineProviderID string // engine.provider.id (e.g. "openai", "anthropic") + IsInlineDefinition bool // true when the engine is defined inline via engine.runtime + optional engine.provider + InlineProviderID string // engine.provider.id (e.g. "openai", "anthropic") // Deprecated: Use InlineProviderAuth instead. Kept for backwards compatibility when only // engine.provider.auth.secret is specified without a strategy. InlineProviderSecret string // engine.provider.auth.secret (backwards compat: simple API key secret name) From 70de7efce33d06c4f688da0e2a3764230231b2dc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 06:31:25 +0000 Subject: [PATCH 4/4] fix: apply all copilot-pull-request-reviewer comments on AuthDefinition/RequestShape PR Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/workflow_secrets.go | 2 +- pkg/workflow/engine.go | 14 +++++-- pkg/workflow/engine_auth_test.go | 62 +++++++++++++++++++++++++++++-- pkg/workflow/engine_definition.go | 2 +- pkg/workflow/engine_validation.go | 13 ++++++- 5 files changed, 82 insertions(+), 11 deletions(-) diff --git a/pkg/cli/workflow_secrets.go b/pkg/cli/workflow_secrets.go index b8cbecaf459..5bf3e58c049 100644 --- a/pkg/cli/workflow_secrets.go +++ b/pkg/cli/workflow_secrets.go @@ -71,7 +71,7 @@ func getSecretRequirementsForWorkflow(workflowFile string) []SecretRequirement { // Get engine-specific secrets only (no system secrets, no optional) // System secrets will be added separately to avoid duplication - reqs := getSecretRequirementsForEngine(engine, true, true) + reqs := getSecretRequirementsForEngine(engine, false, false) // For inline engine definitions with an AuthDefinition, also include auth secrets. if engineConfig != nil && engineConfig.InlineProviderAuth != nil { diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index 75f57b88213..0149a6cdfb6 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -127,9 +127,17 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng if auth, hasAuth := providerObj["auth"]; hasAuth { if authObj, ok := auth.(map[string]any); ok { authDef := parseAuthDefinition(authObj) - config.InlineProviderAuth = authDef - // Backwards compat: expose the simple secret field directly. - config.InlineProviderSecret = authDef.Secret + // Only store an AuthDefinition when the user actually provided + // at least one recognised field. An empty map (e.g. `auth: {}`) + // must not be treated as an explicit auth override. + if authDef.Strategy != "" || authDef.Secret != "" || + authDef.TokenURL != "" || authDef.ClientIDRef != "" || + authDef.ClientSecretRef != "" || authDef.HeaderName != "" || + authDef.TokenField != "" { + config.InlineProviderAuth = authDef + // Backwards compat: expose the simple secret field directly. + config.InlineProviderSecret = authDef.Secret + } } } if request, hasRequest := providerObj["request"]; hasRequest { diff --git a/pkg/workflow/engine_auth_test.go b/pkg/workflow/engine_auth_test.go index ee8d73a2419..e9398d1f5e2 100644 --- a/pkg/workflow/engine_auth_test.go +++ b/pkg/workflow/engine_auth_test.go @@ -28,7 +28,7 @@ func TestAuthDefinition_RequiredSecretNames(t *testing.T) { { name: "nil auth returns empty", auth: nil, - expected: nil, + expected: []string{}, }, { name: "api-key strategy returns Secret", @@ -201,7 +201,61 @@ func TestValidateEngineAuthDefinition_MissingHeaderNameForAPIKey(t *testing.T) { assert.Contains(t, err.Error(), "header-name", "error should mention missing header-name field") } -// TestValidateEngineAuthDefinition_BearerNoHeaderRequired verifies that bearer strategy +// TestValidateEngineAuthDefinition_APIKeyRequiresSecret verifies that api-key +// strategy without a secret produces a clear error. +func TestValidateEngineAuthDefinition_APIKeyRequiresSecret(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyAPIKey, + HeaderName: "x-api-key", + // Secret intentionally omitted + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "api-key without secret should produce an error") + assert.Contains(t, err.Error(), "auth.secret", "error should mention missing secret field") +} + +// TestValidateEngineAuthDefinition_BearerRequiresSecret verifies that bearer +// strategy without a secret produces a clear error. +func TestValidateEngineAuthDefinition_BearerRequiresSecret(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyBearer, + // Secret intentionally omitted + }, + } + + err := compiler.validateEngineAuthDefinition(config) + require.Error(t, err, "bearer without secret should produce an error") + assert.Contains(t, err.Error(), "auth.secret", "error should mention missing secret field") +} + +// TestValidateEngineAuthDefinition_APIKeyValid verifies that a complete api-key definition +// (secret + header-name) passes validation. +func TestValidateEngineAuthDefinition_APIKeyValid(t *testing.T) { + compiler := newTestCompiler(t) + config := &EngineConfig{ + ID: "codex", + IsInlineDefinition: true, + InlineProviderAuth: &AuthDefinition{ + Strategy: AuthStrategyAPIKey, + Secret: "MY_API_KEY", + HeaderName: "x-api-key", + }, + } + + err := compiler.validateEngineAuthDefinition(config) + assert.NoError(t, err, "complete api-key definition should pass validation") +} + // does not require header-name (it uses the standard Authorization header by convention). func TestValidateEngineAuthDefinition_BearerNoHeaderRequired(t *testing.T) { compiler := newTestCompiler(t) @@ -356,14 +410,14 @@ func TestBuiltInEngineAuthUnchanged(t *testing.T) { for _, tt := range tests { t.Run(tt.engineID, func(t *testing.T) { def := catalog.Get(tt.engineID) - require.NotNil(t, def, "built-in engine %s should be in catalog", tt.engineID) + require.NotNilf(t, def, "built-in engine %s should be in catalog", tt.engineID) // Provider.Auth should be nil for built-in engines (they use AuthBinding only). assert.Nil(t, def.Provider.Auth, "built-in engine %s should have no Provider.Auth (uses legacy AuthBinding)", tt.engineID) if tt.wantAuthSecret != "" { - require.Len(t, def.Auth, 1, "engine %s should have exactly one AuthBinding", tt.engineID) + require.Lenf(t, def.Auth, 1, "engine %s should have exactly one AuthBinding", tt.engineID) assert.Equal(t, tt.wantAuthSecret, def.Auth[0].Secret, "engine %s AuthBinding.Secret should be unchanged", tt.engineID) } else { diff --git a/pkg/workflow/engine_definition.go b/pkg/workflow/engine_definition.go index ace1366b688..b670886beb9 100644 --- a/pkg/workflow/engine_definition.go +++ b/pkg/workflow/engine_definition.go @@ -125,7 +125,7 @@ type AuthBinding struct { // this AuthDefinition. Returns an empty slice when Auth is nil. func (a *AuthDefinition) RequiredSecretNames() []string { if a == nil { - return nil + return []string{} } var secrets []string switch a.Strategy { diff --git a/pkg/workflow/engine_validation.go b/pkg/workflow/engine_validation.go index 1ef6a2c1e27..e62c879258d 100644 --- a/pkg/workflow/engine_validation.go +++ b/pkg/workflow/engine_validation.go @@ -119,6 +119,9 @@ func (c *Compiler) registerInlineEngineDefinition(config *EngineConfig) { } def.Provider.Auth = auth // Keep legacy AuthBinding in sync for callers that still read def.Auth. + // When an AuthDefinition is provided, always reset legacy bindings to avoid + // leaking stale secrets from existing engine definitions. + def.Auth = nil if auth.Secret != "" { def.Auth = []AuthBinding{{Role: string(auth.Strategy), Secret: auth.Secret}} } @@ -162,12 +165,18 @@ func (c *Compiler) validateEngineAuthDefinition(config *EngineConfig) error { return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.header-name' to be set (e.g. 'api-key' or 'Authorization').\n\nSee: %s", constants.DocsEnginesURL) } case AuthStrategyAPIKey: - // api-key requires a header-name so the caller knows where to inject the key. + // api-key requires a secret value and a header-name so the caller knows where to inject the key. + if auth.Secret == "" { + return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } if auth.HeaderName == "" { return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.header-name' to be set (e.g. 'api-key' or 'x-api-key').\n\nSee: %s", constants.DocsEnginesURL) } case AuthStrategyBearer, "": - // bearer strategy and unset strategy (simple backwards-compat secret) need no extra fields. + // bearer strategy and unset strategy (simple backwards-compat secret) require a secret value. + if auth.Secret == "" { + return fmt.Errorf("engine auth: strategy 'bearer' (or unset) requires 'auth.secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } default: validStrategies := []string{ string(AuthStrategyAPIKey),