From f7ef5f14f8e00466dd0bc1de4c00e4d3800e80b8 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 13 May 2026 08:44:10 -0400 Subject: [PATCH] fix: close round-3 OAuth strict-proto config-field gaps (v0.2.4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BMW v0.51.5 local smoke surfaced two OAuth gaps the v0.2.3 sweep missed: both fields BMW supplies via the step's config: block (templated at runtime, but strict-proto validates Config at build-time as unresolved literals). - OAuthProviderConfig: added `string return_to = 11`. BMW's step.auth_oauth_start passes `return_to: '{{ .return_to }}'` in config. Handler now prefers config.return_to when non-empty, falls back to current.return_to (OAuthProviderInput). - OAuthProviderConfig: added `string access_token = 12`. BMW's step.auth_oauth_userinfo passes `access_token: '{{ index .steps "exchange_code" "access_token" }}'` in config. Handler now prefers config.access_token when non-empty, falls back to current.access_token (OAuthProviderInput). New helper `oauthConfigOrCurrent(config, current, key)` enforces the Config-when-non-empty rule. OAuthProviderInput remains valid for the runtime-input shape; Config wins as tie-breaker. Exhaustive BMW app.yaml audit of every step.auth_oauth_* config block confirmed only `return_to` (start) and `access_token` (userinfo) were the remaining gaps; provider + google_oauth_* fields are already on the contract. Tests: - TestOAuthProviderConfig_AcceptsReturnToAndAccessToken — strict-proto acceptance across all 4 OAuth step types. - TestOAuthStart_{UsesReturnToFromConfig,ConfigReturnToWinsOverCurrent, FallsBackToCurrentReturnTo} — start_oauth precedence. - TestOAuthUserinfo_{UsesAccessTokenFromConfig, ConfigAccessTokenWinsOverCurrent,FallsBackToCurrentAccessToken} — userinfo precedence via httptest Bearer assertion. CI fixture (.github/fixtures/workflow-compat.yaml) now exercises both new config fields. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/fixtures/workflow-compat.yaml | 6 + CHANGELOG.md | 38 ++++++ internal/contracts/auth.pb.go | 33 ++++- internal/contracts/auth.proto | 9 ++ internal/step_oauth.go | 24 +++- internal/strict_proto_fields_test.go | 178 ++++++++++++++++++++++++++ plugin.json | 10 +- 7 files changed, 287 insertions(+), 11 deletions(-) diff --git a/.github/fixtures/workflow-compat.yaml b/.github/fixtures/workflow-compat.yaml index beeb41e..0b377d9 100644 --- a/.github/fixtures/workflow-compat.yaml +++ b/.github/fixtures/workflow-compat.yaml @@ -156,6 +156,9 @@ workflows: google_oauth_client_id: "google-client" google_oauth_client_secret: "google-secret" google_oauth_redirect_url: "https://example.test/cb" + # return_to added in v0.2.4 (BMW supplies it via config when the + # incoming request includes a return_to query param): + return_to: "/auth/callback" - name: oauth_exchange type: step.auth_oauth_exchange @@ -175,6 +178,9 @@ workflows: google_oauth_client_id: "google-client" google_oauth_client_secret: "google-secret" google_oauth_redirect_url: "https://example.test/cb" + # access_token added in v0.2.4 (BMW supplies it via config from a + # preceding exchange_code step's output): + access_token: "compat-access-token" - name: credential_list type: step.auth_credential_list diff --git a/CHANGELOG.md b/CHANGELOG.md index 90fbc7b..5742d78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,43 @@ # Changelog +## v0.2.4 (2026-05-13) + +### Strict-proto config-field gaps closed (BMW local smoke vs workflow v0.51.5, round 3) + +Round 3 closes two OAuth gaps the v0.2.3 sweep missed. Both surfaced when BMW +v0.51.5 strict-proto validation rejected fields BMW supplies via the step's +`config:` block (templates render at runtime, but strict-proto validates Config +at build-time when templates are still unresolved literals): + +- `OAuthProviderConfig`: added `string return_to = 11`. BMW's `step.auth_oauth_start` + passes `return_to: '{{ .return_to }}'` in config. The handler now prefers + `config.return_to` when non-empty, otherwise falls back to `current.return_to` + (OAuthProviderInput). +- `OAuthProviderConfig`: added `string access_token = 12`. BMW's + `step.auth_oauth_userinfo` passes + `access_token: '{{ index .steps "exchange_code" "access_token" }}'` in config. + The handler now prefers `config.access_token` when non-empty, otherwise falls + back to `current.access_token` (OAuthProviderInput). + +Both fields remain valid on `OAuthProviderInput` for callers that pass them at +runtime (the v0.2.3 contract). Config-when-non-empty is the new tie-breaker. + +### Tests + +- `TestOAuthProviderConfig_AcceptsReturnToAndAccessToken` — strict-proto accepts + the new config fields across all four OAuth step types. +- `TestOAuthStart_UsesReturnToFromConfig`, `TestOAuthStart_ConfigReturnToWinsOverCurrent`, + `TestOAuthStart_FallsBackToCurrentReturnTo` — handler precedence for `return_to`. +- `TestOAuthUserinfo_UsesAccessTokenFromConfig`, + `TestOAuthUserinfo_ConfigAccessTokenWinsOverCurrent`, + `TestOAuthUserinfo_FallsBackToCurrentAccessToken` — handler precedence for + `access_token` (via httptest userinfo server asserting the Bearer header). + +### CI fixture + +- `.github/fixtures/workflow-compat.yaml` now exercises `config.return_to` on + `step.auth_oauth_start` and `config.access_token` on `step.auth_oauth_userinfo`. + ## v0.2.3 (2026-05-13) ### Strict-proto config-field gaps closed (BMW local smoke vs workflow v0.51.5, round 2) diff --git a/internal/contracts/auth.pb.go b/internal/contracts/auth.pb.go index fe5adab..2536fe9 100644 --- a/internal/contracts/auth.pb.go +++ b/internal/contracts/auth.pb.go @@ -3326,8 +3326,17 @@ type OAuthProviderConfig struct { GoogleOauthScopes []string `protobuf:"bytes,8,rep,name=google_oauth_scopes,json=googleOauthScopes,proto3" json:"google_oauth_scopes,omitempty"` AllowInsecureTestOauthEndpoints *bool `protobuf:"varint,9,opt,name=allow_insecure_test_oauth_endpoints,json=allowInsecureTestOauthEndpoints,proto3,oneof" json:"allow_insecure_test_oauth_endpoints,omitempty"` PkceRequired *bool `protobuf:"varint,10,opt,name=pkce_required,json=pkceRequired,proto3,oneof" json:"pkce_required,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // return_to and access_token added in v0.2.4: BMW passes both via the + // step's config: block (BMW templates `{{ .return_to }}` and + // `{{ index .steps "exchange_code" "access_token" }}` respectively). + // Under strict-proto these are validated at build-time when templates + // are unresolved literals, so they must live on OAuthProviderConfig. + // Handlers prefer config values when non-empty, then fall back to + // OAuthProviderInput (runtime input). + ReturnTo string `protobuf:"bytes,11,opt,name=return_to,json=returnTo,proto3" json:"return_to,omitempty"` + AccessToken string `protobuf:"bytes,12,opt,name=access_token,json=accessToken,proto3" json:"access_token,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *OAuthProviderConfig) Reset() { @@ -3430,6 +3439,20 @@ func (x *OAuthProviderConfig) GetPkceRequired() bool { return false } +func (x *OAuthProviderConfig) GetReturnTo() string { + if x != nil { + return x.ReturnTo + } + return "" +} + +func (x *OAuthProviderConfig) GetAccessToken() string { + if x != nil { + return x.AccessToken + } + return "" +} + type OAuthProviderInput struct { state protoimpl.MessageState `protogen:"open.v1"` Provider string `protobuf:"bytes,1,opt,name=provider,proto3" json:"provider,omitempty"` @@ -4597,7 +4620,7 @@ const file_internal_contracts_auth_proto_rawDesc = "" + "\n" + "violations\x18\x02 \x03(\tR\n" + "violations\x12\x14\n" + - "\x05error\x18d \x01(\tR\x05error\"\xfa\x04\n" + + "\x05error\x18d \x01(\tR\x05error\"\xba\x05\n" + "\x13OAuthProviderConfig\x12\x1a\n" + "\bprovider\x18\x01 \x01(\tR\bprovider\x123\n" + "\x16google_oauth_client_id\x18\x02 \x01(\tR\x13googleOauthClientId\x12;\n" + @@ -4609,7 +4632,9 @@ const file_internal_contracts_auth_proto_rawDesc = "" + "\x13google_oauth_scopes\x18\b \x03(\tR\x11googleOauthScopes\x12Q\n" + "#allow_insecure_test_oauth_endpoints\x18\t \x01(\bH\x00R\x1fallowInsecureTestOauthEndpoints\x88\x01\x01\x12(\n" + "\rpkce_required\x18\n" + - " \x01(\bH\x01R\fpkceRequired\x88\x01\x01B&\n" + + " \x01(\bH\x01R\fpkceRequired\x88\x01\x01\x12\x1b\n" + + "\treturn_to\x18\v \x01(\tR\breturnTo\x12!\n" + + "\faccess_token\x18\f \x01(\tR\vaccessTokenB&\n" + "$_allow_insecure_test_oauth_endpointsB\x10\n" + "\x0e_pkce_required\"\xe5\x01\n" + "\x12OAuthProviderInput\x12\x1a\n" + diff --git a/internal/contracts/auth.proto b/internal/contracts/auth.proto index 8ce0e11..2ee1814 100644 --- a/internal/contracts/auth.proto +++ b/internal/contracts/auth.proto @@ -363,6 +363,15 @@ message OAuthProviderConfig { repeated string google_oauth_scopes = 8; optional bool allow_insecure_test_oauth_endpoints = 9; optional bool pkce_required = 10; + // return_to and access_token added in v0.2.4: BMW passes both via the + // step's config: block (BMW templates `{{ .return_to }}` and + // `{{ index .steps "exchange_code" "access_token" }}` respectively). + // Under strict-proto these are validated at build-time when templates + // are unresolved literals, so they must live on OAuthProviderConfig. + // Handlers prefer config values when non-empty, then fall back to + // OAuthProviderInput (runtime input). + string return_to = 11; + string access_token = 12; } message OAuthProviderInput { diff --git a/internal/step_oauth.go b/internal/step_oauth.go index 6a0947f..2ce3063 100644 --- a/internal/step_oauth.go +++ b/internal/step_oauth.go @@ -66,7 +66,10 @@ func (s *oauthStartStep) Execute(_ context.Context, _ map[string]any, _ map[stri return &sdk.StepResult{Output: map[string]any{"started": false, "provider": provider, "error": disabledReason}}, nil } - returnTo, err := normalizeOAuthReturnTo(oauthString(current, "return_to")) + // return_to may be supplied via OAuthProviderConfig (BMW yaml shape: + // `step.auth_oauth_start.config.return_to`) or via OAuthProviderInput + // (`current.return_to`). Config wins when non-empty (v0.2.4). + returnTo, err := normalizeOAuthReturnTo(oauthConfigOrCurrent(s.config, current, "return_to")) if err != nil { return &sdk.StepResult{Output: map[string]any{"started": false, "provider": provider, "error": err.Error()}}, nil } @@ -195,7 +198,11 @@ func (s *oauthUserinfoStep) Execute(ctx context.Context, _ map[string]any, _ map return &sdk.StepResult{Output: map[string]any{"fetched": false, "provider": provider, "error": disabledReason}}, nil } - accessToken := oauthString(current, "access_token") + // access_token may be supplied via OAuthProviderConfig (BMW yaml shape: + // `step.auth_oauth_userinfo.config.access_token`, templated from a + // preceding exchange step) or via OAuthProviderInput (`current.access_token`). + // Config wins when non-empty (v0.2.4). + accessToken := oauthConfigOrCurrent(s.config, current, "access_token") if accessToken == "" { return &sdk.StepResult{Output: map[string]any{"fetched": false, "provider": provider, "error": "missing access_token"}}, nil } @@ -380,6 +387,19 @@ func oauthDoJSON(client *http.Client, req *http.Request, label string, target an return nil } +// oauthConfigOrCurrent returns config[key] when non-empty, otherwise falls back +// to current[key]. BMW supplies fields like return_to / access_token via the +// step's config: block (templated at runtime). Under strict-proto these are +// validated against OAuthProviderConfig at build-time, so the handler must +// honor config when it carries a value but still accept runtime input +// otherwise. Added in v0.2.4. +func oauthConfigOrCurrent(config, current map[string]any, key string) string { + if value := oauthString(config, key); value != "" { + return value + } + return oauthString(current, key) +} + func oauthString(values map[string]any, key string) string { if values == nil { return "" diff --git a/internal/strict_proto_fields_test.go b/internal/strict_proto_fields_test.go index 8e6fba8..f2944e3 100644 --- a/internal/strict_proto_fields_test.go +++ b/internal/strict_proto_fields_test.go @@ -2,6 +2,9 @@ package internal import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" "testing" "time" @@ -221,3 +224,178 @@ func TestChallengeVerify_FallsBackToConfigSigningSecret(t *testing.T) { t.Fatalf("expected valid=true when signing_secret only supplied via config, got %#v", verifyResult.Output) } } + +// TestOAuthProviderConfig_AcceptsReturnToAndAccessToken ensures the typed +// OAuthProviderConfig accepts the return_to and access_token fields BMW +// supplies via step.auth_oauth_start.config and step.auth_oauth_userinfo.config +// (round-3 strict-proto gap, v0.2.4). +func TestOAuthProviderConfig_AcceptsReturnToAndAccessToken(t *testing.T) { + cfg := &contracts.OAuthProviderConfig{ + Provider: "google", + GoogleOauthClientId: "google-client", + GoogleOauthClientSecret: "google-secret", + GoogleOauthRedirectUrl: "https://example.test/cb", + ReturnTo: "/auth/callback", + AccessToken: "access-token-from-exchange", + } + packed, err := anypb.New(cfg) + if err != nil { + t.Fatalf("pack config: %v", err) + } + provider := NewAuthPlugin().(interface { + CreateTypedStep(typeName, name string, config *anypb.Any) (sdk.StepInstance, error) + }) + for _, stepType := range []string{ + "step.auth_oauth_start", + "step.auth_oauth_exchange", + "step.auth_oauth_userinfo", + "step.auth_oauth_provider_config", + } { + if _, err := provider.CreateTypedStep(stepType, "oauth", packed); err != nil { + t.Fatalf("CreateTypedStep(%s) rejected return_to/access_token: %v", stepType, err) + } + } +} + +// TestOAuthStart_UsesReturnToFromConfig verifies start_oauth honors return_to +// when supplied via config (BMW yaml shape), not just via input. +func TestOAuthStart_UsesReturnToFromConfig(t *testing.T) { + step := newOAuthStartStep("test", googleOAuthTestConfig(map[string]any{ + "return_to": "/wishlists", + })) + result, err := step.Execute(context.Background(), nil, nil, map[string]any{ + "provider": "google", + }, nil, nil) + if err != nil { + t.Fatalf("execute: %v", err) + } + if errStr, _ := result.Output["error"].(string); errStr != "" { + t.Fatalf("expected no error, got %q (output=%#v)", errStr, result.Output) + } + if got, _ := result.Output["return_to"].(string); got != "/wishlists" { + t.Fatalf("expected return_to=/wishlists from config, got %v", result.Output["return_to"]) + } +} + +// TestOAuthStart_ConfigReturnToWinsOverCurrent verifies config.return_to wins +// when both config and input supply it (Config-when-non-empty rule). +func TestOAuthStart_ConfigReturnToWinsOverCurrent(t *testing.T) { + step := newOAuthStartStep("test", googleOAuthTestConfig(map[string]any{ + "return_to": "/from-config", + })) + result, err := step.Execute(context.Background(), nil, nil, map[string]any{ + "provider": "google", + "return_to": "/from-input", + }, nil, nil) + if err != nil { + t.Fatalf("execute: %v", err) + } + if got, _ := result.Output["return_to"].(string); got != "/from-config" { + t.Fatalf("expected config.return_to to win, got %v", result.Output["return_to"]) + } +} + +// TestOAuthStart_FallsBackToCurrentReturnTo verifies start_oauth honors +// return_to from current/input when config does not supply one. +func TestOAuthStart_FallsBackToCurrentReturnTo(t *testing.T) { + step := newOAuthStartStep("test", googleOAuthTestConfig(nil)) + result, err := step.Execute(context.Background(), nil, nil, map[string]any{ + "provider": "google", + "return_to": "/from-input", + }, nil, nil) + if err != nil { + t.Fatalf("execute: %v", err) + } + if got, _ := result.Output["return_to"].(string); got != "/from-input" { + t.Fatalf("expected return_to from input fallback, got %v", result.Output["return_to"]) + } +} + +// TestOAuthUserinfo_UsesAccessTokenFromConfig verifies fetch_userinfo honors +// access_token when supplied via config (BMW yaml shape templates the token +// from a preceding exchange_code step into config.access_token). +func TestOAuthUserinfo_UsesAccessTokenFromConfig(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer config-access-token" { + t.Fatalf("expected bearer from config.access_token, got %q", r.Header.Get("Authorization")) + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "sub": "user-id", + "email": "user@example.com", + }) + })) + defer server.Close() + + step := newOAuthUserinfoStep("test", googleOAuthTestConfig(map[string]any{ + "google_oauth_userinfo_url": server.URL, + "allow_insecure_test_oauth_endpoints": true, + "access_token": "config-access-token", + })) + result, err := step.Execute(context.Background(), nil, nil, map[string]any{ + "provider": "google", + }, nil, nil) + if err != nil { + t.Fatalf("execute: %v", err) + } + if errStr, _ := result.Output["error"].(string); errStr != "" { + t.Fatalf("expected no error, got %q (output=%#v)", errStr, result.Output) + } + if result.Output["fetched"] != true { + t.Fatalf("expected fetched=true, got %v", result.Output["fetched"]) + } +} + +// TestOAuthUserinfo_ConfigAccessTokenWinsOverCurrent verifies config.access_token +// wins when both config and input supply it. +func TestOAuthUserinfo_ConfigAccessTokenWinsOverCurrent(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer config-wins" { + t.Fatalf("expected config.access_token to win, got %q", r.Header.Get("Authorization")) + } + _ = json.NewEncoder(w).Encode(map[string]any{"sub": "x"}) + })) + defer server.Close() + + step := newOAuthUserinfoStep("test", googleOAuthTestConfig(map[string]any{ + "google_oauth_userinfo_url": server.URL, + "allow_insecure_test_oauth_endpoints": true, + "access_token": "config-wins", + })) + result, err := step.Execute(context.Background(), nil, nil, map[string]any{ + "provider": "google", + "access_token": "input-loses", + }, nil, nil) + if err != nil { + t.Fatalf("execute: %v", err) + } + if result.Output["fetched"] != true { + t.Fatalf("expected fetched=true, got %#v", result.Output) + } +} + +// TestOAuthUserinfo_FallsBackToCurrentAccessToken verifies fetch_userinfo falls +// back to current/input access_token when config does not supply one. +func TestOAuthUserinfo_FallsBackToCurrentAccessToken(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer input-access-token" { + t.Fatalf("expected bearer from input fallback, got %q", r.Header.Get("Authorization")) + } + _ = json.NewEncoder(w).Encode(map[string]any{"sub": "user"}) + })) + defer server.Close() + + step := newOAuthUserinfoStep("test", googleOAuthTestConfig(map[string]any{ + "google_oauth_userinfo_url": server.URL, + "allow_insecure_test_oauth_endpoints": true, + })) + result, err := step.Execute(context.Background(), nil, nil, map[string]any{ + "provider": "google", + "access_token": "input-access-token", + }, nil, nil) + if err != nil { + t.Fatalf("execute: %v", err) + } + if result.Output["fetched"] != true { + t.Fatalf("expected fetched=true via input fallback, got %#v", result.Output) + } +} diff --git a/plugin.json b/plugin.json index 53f0f28..938cab4 100644 --- a/plugin.json +++ b/plugin.json @@ -1,6 +1,6 @@ { "name": "workflow-plugin-auth", - "version": "0.2.3", + "version": "0.2.4", "description": "Passwordless authentication plugin: WebAuthn/passkeys, TOTP, email magic links", "author": "GoCodeAlone", "license": "MIT", @@ -22,22 +22,22 @@ { "os": "linux", "arch": "amd64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.3/workflow-plugin-auth_0.2.3_linux_amd64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.4/workflow-plugin-auth_0.2.4_linux_amd64.tar.gz" }, { "os": "linux", "arch": "arm64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.3/workflow-plugin-auth_0.2.3_linux_arm64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.4/workflow-plugin-auth_0.2.4_linux_arm64.tar.gz" }, { "os": "darwin", "arch": "amd64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.3/workflow-plugin-auth_0.2.3_darwin_amd64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.4/workflow-plugin-auth_0.2.4_darwin_amd64.tar.gz" }, { "os": "darwin", "arch": "arm64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.3/workflow-plugin-auth_0.2.3_darwin_arm64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-auth/releases/download/v0.2.4/workflow-plugin-auth_0.2.4_darwin_arm64.tar.gz" } ], "moduleTypes": [