fix: wfctl improvements and security hardening by intel352 · Pull Request #173 · GoCodeAlone/workflow

intel352 · 2026-02-26T21:01:14Z

Summary

Fix OPA and Cedar policy stubs to deny by default instead of allowing all requests
Use Stripe billing provider when STRIPE_API_KEY is set (previously ignored the env var)
Fix wfctl deploy cloud by adding -env flag passthrough to runRun
Replace manual YAML parser in loadWfctlConfig with yaml.Unmarshal for correctness
Replace bytesReaderImpl wrapper with stdlib bytes.NewReader
Add compare as an alias for the contract test subcommand for discoverability

Test plan

go build ./...
go test ./...

🤖 Generated with Claude Code

Stub Evaluate() methods previously returned Allowed: true, silently permitting all requests when no real backend was connected. They now return Allowed: false with an explicit safety reason. A log.Warn is emitted at Init() time for both backends. Set allow_stub_backends: true in the module config to restore allow-all behaviour for testing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

runDeployCloud passes -env to the run subcommand but runRun did not accept it, causing an unknown flag error. Add -env flag and propagate it as WORKFLOW_ENV environment variable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The server always used MockBillingProvider regardless of environment, meaning real billing was never invoked in production. Now checks STRIPE_API_KEY at startup: if present, constructs a StripeProvider (also reads STRIPE_WEBHOOK_SECRET); otherwise falls back to mock with a Warn-level log so the omission is visible in production logs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…nmarshal The hand-rolled line splitter mishandled quoted values, YAML nesting, and special characters. Replace it with a proper wfctlConfigFile struct and yaml.Unmarshal to correctly parse the nested project/git/deploy sections written by writeWfctlConfig. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The hand-rolled bytesReaderImpl struct duplicates the stdlib bytes.NewReader. Delete it and replace the single call site with bytes.NewReader(data) directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Users expect wfctl contract compare to work as a natural alias for wfctl contract test. Add compare to the dispatch switch and update the usage text to document the alias. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens default security behavior for policy backends and improves wfctl ergonomics/config correctness, while also attempting to enable real Stripe billing in the server when environment variables are present.

Changes:

Make OPA/Cedar policy stubs deny-by-default (with an opt-in allow switch for stub testing).
Improve wfctl UX: add -env passthrough, add contract compare alias, and replace ad-hoc YAML parsing with yaml.Unmarshal.
Simplify plugin install archive extraction by using bytes.NewReader, and wire Stripe provider selection via STRIPE_API_KEY.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
module/policy_engine.go	Adds deny-by-default behavior for OPA/Cedar stubs and introduces `allow_stub_backends` parsing/logging.
cmd/wfctl/run.go	Adds `-env` flag to set `WORKFLOW_ENV` before running the engine.
cmd/wfctl/plugin_install.go	Replaces custom `bytesReader` wrapper with `bytes.NewReader` during tar.gz extraction.
cmd/wfctl/git_connect.go	Replaces line-based `.wfctl.yaml` parsing with structured `yaml.Unmarshal` and preserves defaults.
cmd/wfctl/contract.go	Adds `compare` alias for `wfctl contract test` and updates usage text.
cmd/server/main.go	Switches billing provider to Stripe when `STRIPE_API_KEY` is set (otherwise mock).

Copilot · 2026-02-26T21:04:50Z

module/policy_engine.go

+	allowStub := isTruthy(m.config["allow_stub_backends"])
+
 	switch m.backend {
 	case "mock":
 		m.engine = newMockPolicyEngine()
 	case "opa":
 		endpoint, _ := m.config["endpoint"].(string)
-		m.engine = newOPAPolicyEngine(endpoint)
+		m.engine = newOPAPolicyEngine(endpoint, allowStub)
+		slog.Warn("WARNING: using stub policy engine — all requests will be DENIED. Set allow_stub_backends: true in config to use stub backends for testing.",
+			"module", m.name, "backend", "opa", "allow_stub_backends", allowStub)


A new config knob (allow_stub_backends) is introduced/advertised in the warning, but this module’s config schema (via the policy plugin) doesn’t appear to expose or document it. That makes it hard for users to discover/configure and increases the chance of accidental misconfiguration. Consider documenting this option alongside other policy module config fields (or renaming it to match an existing convention) and ensuring it’s represented wherever module config is surfaced.

Copilot · 2026-02-26T21:04:50Z

module/policy_engine.go

 func (e *opaPolicyEngine) Evaluate(_ context.Context, input map[string]any) (*PolicyDecision, error) {
 	// Production: POST {"input": input} to <endpoint>/v1/data/<default-policy>
 	// and parse the result body for {"result": {"allow": true}}.
+	if e.allowStub {
+		return &PolicyDecision{
+			Allowed:  true,
+			Reasons:  []string{"opa stub: allow_stub_backends enabled"},
+			Metadata: map[string]any{"backend": "opa", "endpoint": e.endpoint, "input": input},
+		}, nil
+	}
 	return &PolicyDecision{
-		Allowed:  true,
-		Reasons:  []string{"opa stub: default allow"},
+		Allowed:  false,
+		Reasons:  []string{"STUB IMPLEMENTATION - not connected to real backend - denied for safety"},
 		Metadata: map[string]any{"backend": "opa", "endpoint": e.endpoint, "input": input},
 	}, nil


The updated OPA/Cedar stub behavior (deny-by-default, allow when allow_stub_backends is enabled) is security-sensitive but currently has no direct unit coverage. Add tests that assert Evaluate() denies when allow_stub_backends is false and allows when it is true for both OPA and Cedar engines, so future refactors don’t accidentally revert to allow-by-default.

Copilot · 2026-02-26T21:04:50Z

cmd/server/main.go

+		billingProvider = billing.NewStripeProvider(stripeKey, webhookSecret, billing.StripePlanIDs{})
+		logger.Info("Billing: using Stripe provider")


STRIPE_API_KEY now enables the Stripe billing provider, but it’s constructed with an empty planPriceIDs map (billing.StripePlanIDs{}). This guarantees CreateSubscription will fail at runtime with "no stripe price ID configured" for every plan, breaking the /subscribe endpoint. Consider loading plan→price IDs from config/env (or refusing to enable Stripe until they’re configured and falling back to mock with a clear warning/error).

Suggested change

billingProvider = billing.NewStripeProvider(stripeKey, webhookSecret, billing.StripePlanIDs{})

logger.Info("Billing: using Stripe provider")

// Load Stripe plan price IDs from environment. Expect a JSON object mapping plan names to Stripe price IDs,

// e.g. {"basic":"price_123","pro":"price_456"}.

var stripePlanIDs billing.StripePlanIDs

if rawPlanIDs := os.Getenv("STRIPE_PLAN_PRICE_IDS"); rawPlanIDs != "" {

if err := json.Unmarshal([]byte(rawPlanIDs), &stripePlanIDs); err != nil {

logger.Error("Billing: invalid STRIPE_PLAN_PRICE_IDS; falling back to MockBillingProvider", "error", err)

}

}

if len(stripePlanIDs) == 0 {

logger.Warn("Billing: STRIPE_API_KEY is set but no Stripe plan price IDs configured (STRIPE_PLAN_PRICE_IDS); using MockBillingProvider instead of Stripe to avoid runtime failures")

billingProvider = billing.NewMockBillingProvider()

} else {

billingProvider = billing.NewStripeProvider(stripeKey, webhookSecret, stripePlanIDs)

logger.Info("Billing: using Stripe provider")

}

Copilot · 2026-02-26T21:04:50Z

module/policy_engine.go

+		slog.Warn("WARNING: using stub policy engine — all requests will be DENIED. Set allow_stub_backends: true in config to use stub backends for testing.",
+			"module", m.name, "backend", "opa", "allow_stub_backends", allowStub)
 	case "cedar":
-		m.engine = newCedarPolicyEngine()
+		m.engine = newCedarPolicyEngine(allowStub)
+		slog.Warn("WARNING: using stub policy engine — all requests will be DENIED. Set allow_stub_backends: true in config to use stub backends for testing.",
+			"module", m.name, "backend", "cedar", "allow_stub_backends", allowStub)


The warning text says "all requests will be DENIED", but when allow_stub_backends is true the OPA/Cedar stubs return Allowed=true. This makes the log message misleading (and potentially dangerous if someone enables allow_stub_backends and relies on the warning). Consider making the warning conditional and explicitly stating the effective behavior (deny-all vs allow-all) based on allow_stub_backends.

intel352 and others added 6 commits February 26, 2026 15:58

fix(wfctl): remove bytesReaderImpl in favor of bytes.NewReader

678b6ed

The hand-rolled bytesReaderImpl struct duplicates the stdlib bytes.NewReader. Delete it and replace the single call site with bytes.NewReader(data) directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings February 26, 2026 21:01

Copilot started reviewing on behalf of intel352 February 26, 2026 21:01 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

intel352 merged commit c567b1b into main Feb 26, 2026
18 checks passed

intel352 deleted the fix/ws03-wfctl-fixes branch February 26, 2026 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wfctl improvements and security hardening#173

fix: wfctl improvements and security hardening#173
intel352 merged 6 commits intomainfrom
fix/ws03-wfctl-fixes

intel352 commented Feb 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		billingProvider = billing.NewStripeProvider(stripeKey, webhookSecret, billing.StripePlanIDs{})
		logger.Info("Billing: using Stripe provider")

-		billingProvider = billing.NewStripeProvider(stripeKey, webhookSecret, billing.StripePlanIDs{})
-		logger.Info("Billing: using Stripe provider")
+		// Load Stripe plan price IDs from environment. Expect a JSON object mapping plan names to Stripe price IDs,
+		// e.g. {"basic":"price_123","pro":"price_456"}.
+		var stripePlanIDs billing.StripePlanIDs
+		if rawPlanIDs := os.Getenv("STRIPE_PLAN_PRICE_IDS"); rawPlanIDs != "" {
+			if err := json.Unmarshal([]byte(rawPlanIDs), &stripePlanIDs); err != nil {
+				logger.Error("Billing: invalid STRIPE_PLAN_PRICE_IDS; falling back to MockBillingProvider", "error", err)
+			}
+		}
+		if len(stripePlanIDs) == 0 {
+			logger.Warn("Billing: STRIPE_API_KEY is set but no Stripe plan price IDs configured (STRIPE_PLAN_PRICE_IDS); using MockBillingProvider instead of Stripe to avoid runtime failures")
+			billingProvider = billing.NewMockBillingProvider()
+		} else {
+			billingProvider = billing.NewStripeProvider(stripeKey, webhookSecret, stripePlanIDs)
+			logger.Info("Billing: using Stripe provider")
+		}

Conversation

intel352 commented Feb 26, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants