diff --git a/internal/mcp/server.go b/internal/mcp/server.go index c6bc599..240b437 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -158,13 +158,24 @@ func NewServer(opts Options) (*Server, error) { al = &auditLogger{} // no-op: enc is nil, log() returns early } + // Resolve the profile once for both consumers (step-up + signer). + // SigningProfile is the explicit opt-in; empty means "use the active + // profile." Pre-KLA-420 this fallback lived only in the signer block, + // so a webhook-only setup (no signing) emitted `"profile": ""` in + // every outbound envelope. Receivers keying on profile (Slack bot + // routing rules, audit dashboards) saw empty strings. + profile := opts.SigningProfile + if profile == "" { + profile = config.ActiveProfile() + } + stepUp := opts.stepUp if stepUp == nil { su, err := newStepUp(stepUpConfig{ Required: opts.RequireStepUp, APIKey: opts.StepUpAPIKey, AuthenticatorPref: opts.StepUpAuthenticator, - Profile: opts.SigningProfile, + Profile: profile, WebhookURL: opts.ApprovalWebhookURL, WebhookCallbackAddr: opts.ApprovalCallbackAddr, WebhookTimeout: opts.ApprovalTimeout, @@ -177,10 +188,6 @@ func NewServer(opts Options) (*Server, error) { signer := opts.signer if signer == nil { - profile := opts.SigningProfile - if profile == "" { - profile = config.ActiveProfile() - } signer = newSigner(opts.SignDestructiveOps, profile) } diff --git a/internal/mcp/stepup_webhook_test.go b/internal/mcp/stepup_webhook_test.go index 7ac86bb..3559557 100644 --- a/internal/mcp/stepup_webhook_test.go +++ b/internal/mcp/stepup_webhook_test.go @@ -125,6 +125,62 @@ func TestWebhookStepUp_RemediationIsChannelAware(t *testing.T) { } } +// KLA-420: when an operator enables --require-step-up +// --step-up-authenticator=webhook without also enabling signing +// (mcp.sign_destructive_ops), the webhook envelope's profile field +// must still carry the active profile name. Pre-fix, server.go wired +// Profile = opts.SigningProfile directly with no fallback, so +// webhook-only setups emitted `"profile": ""` to receivers. +func TestNewServer_WebhookProfileFallsBackToActiveProfile(t *testing.T) { + setupTest(t) + + s := MustNewServer(Options{ + RequireStepUp: true, + StepUpAuthenticator: "webhook", + ApprovalWebhookURL: "http://127.0.0.1:1/notused", + ApprovalCallbackAddr: "127.0.0.1:0", + ApprovalTimeout: 1 * time.Second, + // SigningProfile intentionally left empty — that's the bug: + // pre-fix this would propagate `profile: ""` into the webhook. + }) + t.Cleanup(s.shutdownStepUp) + + w, ok := s.stepUp.(*webhookStepUp) + if !ok { + t.Fatalf("expected *webhookStepUp, got %T", s.stepUp) + } + // setupTest configures active_profile=default; resolution must pick + // that up rather than leaving the field empty. + if w.profile != "default" { + t.Errorf("webhookStepUp.profile = %q, want %q (active profile fallback)", w.profile, "default") + } +} + +// Companion: an explicit SigningProfile must still flow through +// unchanged. The fallback only kicks in when the explicit value is +// empty. +func TestNewServer_WebhookProfileHonorsExplicitSigningProfile(t *testing.T) { + setupTest(t) + + s := MustNewServer(Options{ + RequireStepUp: true, + StepUpAuthenticator: "webhook", + ApprovalWebhookURL: "http://127.0.0.1:1/notused", + ApprovalCallbackAddr: "127.0.0.1:0", + ApprovalTimeout: 1 * time.Second, + SigningProfile: "staging", + }) + t.Cleanup(s.shutdownStepUp) + + w, ok := s.stepUp.(*webhookStepUp) + if !ok { + t.Fatalf("expected *webhookStepUp, got %T", s.stepUp) + } + if w.profile != "staging" { + t.Errorf("webhookStepUp.profile = %q, want %q (explicit SigningProfile)", w.profile, "staging") + } +} + func TestNewWebhookStepUp_RejectsEmptyURL(t *testing.T) { _, err := newWebhookStepUp("", "", 0, "default") if err == nil {