feat(service): add oauth2_token_endpoint outbound auth provider#36
Open
dy-shimizu wants to merge 6 commits into
Open
feat(service): add oauth2_token_endpoint outbound auth provider#36dy-shimizu wants to merge 6 commits into
dy-shimizu wants to merge 6 commits into
Conversation
New outbound HTTP auth type for OAuth2-like flows whose token endpoints don't publish OIDC discovery metadata. Reuses the existing TokenFetcher cache infrastructure (getCachedToken, cacheToken, executeTokenRequest) and TokenResponse shape — no parallel token-fetch code path is introduced. Credentials can be sent in the form body (default) or as a Basic Authorization header via credentials_location. grant_type is not sent by default — callers that need one set it via extra_params. Security hardenings included: - extra_params rejects reserved keys (client_id, client_secret, scope, audience) to prevent silent override of validated canonical fields. - token_type allowlist: only Bearer (case-insensitive) is accepted from the token endpoint response. Rejects Basic/MAC/DPoP to close a credential-disclosure vector where a malicious IdP could force a non-Bearer scheme on downstream requests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…list Wires the new outbound auth type into the existing integration surface so configs can flow through the API end-to-end: - pkg/model/executor_configuration.go: adds oauth2_token_endpoint to the validTypes whitelist and to the ErrExecutorConfigAuthTypeInvalid message. - pkg/model/executor_configuration_input.go: extends the validator 'oneof' tag on ExecutorAuthenticationInput.Type. - pkg/executors/http/http.go: adds oauth2_token_endpoint to the JSON Schema 'auth.type' enum and a matching allOf if/then branch with the config shape (token_url required, credentials_location enum, scopes/audience/extra_params). Without these, the /v1/catalog/executors/http/validate endpoint and the ExecutorConfiguration domain constructor would reject configs with the new type even though the underlying provider exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds end-to-end tests that drive the new provider through the public runner surface (httpExecutor.NewRunner().Execute) so the entire chain — config map → factory → provider → token endpoint → downstream call — is exercised on the same shape consumed by the production HTTP path. Tests added: - TestHTTPProviderAuthOAuth2TokenEndpoint: body credentials happy path - TestHTTPProviderAuthOAuth2TokenEndpointBasicHeader: basic_header path - TestHTTPProviderAuthOAuth2TokenEndpointWithExtraParams: verifies extra_params reach the token endpoint - TestHTTPProviderAuthOAuth2TokenEndpointInvalidCredentials: 401 from token endpoint surfaces as ExecutionStatusError The new mockTokenEndpointServer helper exposes ONLY POST /token (no /.well-known/openid-configuration handler) and intentionally fails the test if discovery is attempted — this proves the oauth2_token_endpoint provider does not perform OIDC discovery. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Route fetchNewOAuth2TokenEndpointToken through newPinnedTokenRequest, matching the 3 OIDC fetch paths after PR #24 (safehttp). - Drop misleading validate:"required,url" struct tags from OAuth2TokenEndpointConfig — the package does not invoke validator.Struct on this config, so the tags signaled validation that never runs. Replaced by docblock comment explaining intent. - Apply gofmt alignment to factory_test.go map literal.
F1 — Regenerate Swagger/OpenAPI artifacts to include oauth2_token_endpoint
in the auth-type enum. Hand-patched narrowly (api/docs.go, api/swagger.json,
api/swagger.yaml, api/openapi.yaml) because `make generate-docs` is broken
at HEAD by a pre-existing malformed @Param annotation in
internal/adapters/http/in/webhook/handler.go:83 (out-of-scope; flagged for
product owner as Finding A.19).
F2 — Add TestOAuth2TokenEndpointProvider_SSRFBlocksPrivateNetwork to verify
the SSRF rejection invariant for the new provider. Uses
safehttp.SetAllowPrivateForTest(false) + t.Cleanup to override the
suite-wide allow-private setup from main_test.go without leaking state.
F3 — Stop mutating the caller's *OAuth2TokenEndpointConfig in the
constructor. The empty-string default for CredentialsLocation is now
applied at the request-build site (buildOAuth2TokenEndpointData treats
"" and "body" identically). Test inverted to assert the caller's cfg
pointer is preserved AND that body-credentials behavior is correct.
F4 — Add five missing test scenarios:
- CacheKeyIsolation: two providers with different ClientID must hit
the endpoint independently (no cross-config cache reuse).
- TokenEndpointReturns401: assert error, no Authorization header,
error message identifies the upstream status.
- TokenEndpointReturnsMalformedJSON: decode failure propagates.
- RejectsEmptyAccessToken: new sentinel
ErrOAuth2TokenEndpointEmptyAccessToken + Apply-side guard rejects
{"access_token":"",...} responses (defense-in-depth).
- NoGrantTypeByDefault: locks in the documented Delorean accommodation
(no grant_type form field unless caller supplies via extra_params).
F5 — Strengthen factory_test "full valid config" success assertion via
a same-package testConfigSnapshot() helper that round-trips and asserts
all 7 fields (token_url, client_id, client_secret, credentials_location,
scopes, audience, extra_params). Catches mapToStruct deserialization
regressions that the prior provider.Type()-only assertion would miss.
Validation: build, vet, gofmt, golangci-lint, unit, integration, e2e — all
clean. Gate 8 codereview (10 parallel reviewers) PASS in iteration 2 with
no NEW blocking findings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds OAuth2 token endpoint authentication to the HTTP executor. A new ChangesOAuth2 Token Endpoint Authentication
Comment |
Contributor
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
Pre-release Version Check
✅ No unstable version pins found.
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 47.2% |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/flowker/api |
100.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/audit |
0.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/catalog |
60.9% |
github.com/LerianStudio/flowker/internal/adapters/http/in/dashboard |
91.5% |
github.com/LerianStudio/flowker/internal/adapters/http/in/execution |
81.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/executor_configuration |
70.8% |
github.com/LerianStudio/flowker/internal/adapters/http/in/health |
100.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/middleware/testutil |
0.0% |
github.com/LerianStudio/flowker/internal/adapters/http/in/middleware |
57.9% |
github.com/LerianStudio/flowker/internal/adapters/http/in/provider_configuration |
33.1% |
github.com/LerianStudio/flowker/internal/adapters/http/in/readyz |
81.3% |
github.com/LerianStudio/flowker/internal/adapters/http/in/webhook |
87.5% |
github.com/LerianStudio/flowker/internal/adapters/http/in/workflow |
69.7% |
github.com/LerianStudio/flowker/internal/adapters/http/in |
33.3% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/dashboard |
43.0% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/execution |
64.8% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/executor_configuration |
36.6% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/provider_configuration |
63.0% |
github.com/LerianStudio/flowker/internal/adapters/mongodb/workflow |
69.2% |
github.com/LerianStudio/flowker/internal/adapters/postgresql/audit |
41.9% |
github.com/LerianStudio/flowker/internal/bootstrap |
40.5% |
github.com/LerianStudio/flowker/internal/services/command |
42.1% |
github.com/LerianStudio/flowker/internal/services/query |
11.2% |
github.com/LerianStudio/flowker/internal/services |
3.2% |
github.com/LerianStudio/flowker/internal/testutil |
63.6% |
github.com/LerianStudio/flowker/pkg/circuitbreaker |
97.6% |
github.com/LerianStudio/flowker/pkg/clock |
0.0% |
github.com/LerianStudio/flowker/pkg/condition |
87.9% |
github.com/LerianStudio/flowker/pkg/contextutil |
100.0% |
github.com/LerianStudio/flowker/pkg/executor/base |
20.0% |
github.com/LerianStudio/flowker/pkg/executor |
39.5% |
github.com/LerianStudio/flowker/pkg/executors/http/auth |
72.1% |
github.com/LerianStudio/flowker/pkg/executors/http |
49.5% |
github.com/LerianStudio/flowker/pkg/executors/midaz |
80.8% |
github.com/LerianStudio/flowker/pkg/executors/s3 |
0.0% |
github.com/LerianStudio/flowker/pkg/executors/tracer |
88.0% |
github.com/LerianStudio/flowker/pkg/executors |
57.1% |
github.com/LerianStudio/flowker/pkg/model |
60.7% |
github.com/LerianStudio/flowker/pkg/net/http |
91.3% |
github.com/LerianStudio/flowker/pkg/pagination |
0.0% |
github.com/LerianStudio/flowker/pkg/safehttp |
90.3% |
github.com/LerianStudio/flowker/pkg/ssrfopt |
100.0% |
github.com/LerianStudio/flowker/pkg/templates/tracer_midaz |
81.5% |
github.com/LerianStudio/flowker/pkg/templates |
0.0% |
github.com/LerianStudio/flowker/pkg/transformation |
67.2% |
github.com/LerianStudio/flowker/pkg/triggers/webhook |
0.0% |
github.com/LerianStudio/flowker/pkg/triggers |
0.0% |
github.com/LerianStudio/flowker/pkg/webhook |
100.0% |
github.com/LerianStudio/flowker/pkg |
90.3% |
Generated by Go PR Analysis workflow
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/executors/http/auth/factory.go`:
- Around line 181-213: Replace sentinel auth errors in
parseOAuth2TokenEndpointConfig with pkg/errors.go ValidationError values: if
mapToStruct returns an error, wrap it into a ValidationError (e.g.,
NewValidationError or constructing ValidationError) with field="" or more
specific context from the mapToStruct error; for empty
TokenURL/ClientID/ClientSecret return a ValidationError that identifies the
specific field ("token_url", "client_id", "client_secret") and an appropriate
message; for invalid CredentialsLocation return a ValidationError on field
"credentials_location"; and when reservedOAuth2ExtraParams[k] is true return a
ValidationError that references the offending extra param (field like
fmt.Sprintf("extra_params.%s", k)) and includes the reserved-param message; keep
the original checks (mapToStruct, reservedOAuth2ExtraParams loop) and replace
the ErrOAuth2TokenEndpoint* returns with constructed ValidationError instances
from pkg/errors.go so downstream code receives typed validation errors.
In `@pkg/executors/http/auth/oauth2_token_endpoint.go`:
- Around line 82-87: The OAuth2TokenEndpointProvider constructor is storing the
caller's mutable cfg pointer (config: cfg), which can lead to races or drifting
behavior if the caller later mutates Scopes or ExtraParams; update the
constructor that returns *OAuth2TokenEndpointProvider to deep-clone cfg (copy
primitive fields, duplicate slices like Scopes and maps like ExtraParams) and
store that copy in the provider's config field so Apply and cacheKey semantics
remain stable and thread-safe; ensure any code referencing config inside
OAuth2TokenEndpointProvider and its Apply method uses the cloned copy and not
the original cfg.
- Around line 79-80: The cache key built for the OAuth2 token endpoint is
missing auth-shaping fields and can cause token reuse collisions; update the
generateCacheKey invocation that assigns cacheKey (currently using "oauth2te",
cfg.TokenURL, cfg.ClientID, cfg.Scopes) to also include cfg.Audience,
cfg.CredentialsLocation, and cfg.ExtraParams (or their normalized/stringified
form) so the key uniquely represents request-shaping parameters; ensure any
map/struct in ExtraParams is deterministically serialized before passing to
generateCacheKey.
In `@pkg/executors/http/auth/token_fetcher.go`:
- Around line 390-416: FetchOAuth2TokenEndpointToken currently accepts a
*OAuth2TokenEndpointConfig but doesn't validate it, causing
fetchNewOAuth2TokenEndpointToken to dereference a nil cfg; add an early nil
check at the top of FetchOAuth2TokenEndpointToken that returns a typed
validation error (e.g., a wrapped or package-level error value indicating "nil
OAuth2TokenEndpointConfig") when cfg == nil, so callers get a clear, non-panic
failure instead of a runtime panic in fetchNewOAuth2TokenEndpointToken or
buildOAuth2TokenEndpointData.
In `@pkg/executors/http/http.go`:
- Around line 325-328: The schema currently allows forbidden keys in the
"extra_params" object despite the comment; update the JSON Schema for the
"extra_params" property so validation rejects keys "client_id", "client_secret",
"scope", and "audience" (e.g., add a "propertyNames" constraint with a "not" +
"enum" listing those reserved names or equivalent denylist). Modify the schema
for the "extra_params" property in pkg/executors/http/http.go (the object that
currently has "additionalProperties": { "type": "string" }) to include that
propertyNames denylist so invalid configs fail at schema validation time.
In `@tests/integration/http_provider_auth_test.go`:
- Around line 778-831: The test currently relies on capturedTenant being set by
the tokenSrv handler before the assertion after runner.Execute; to make the
intent explicit and avoid any future flakiness, change the test to synchronize
the token handler and the assertion: create a channel (or a sync.WaitGroup) and
have the tokenSrv handler send the tenant value (or signal done) when it
extracts r.Form.Get("tenant") (instead of only writing to capturedTenant), then
after runner.Execute but before the assert, receive from that channel (or wait)
to guarantee the handler ran; refer to capturedTenant, tokenSrv (the httptest
handler) and runner.Execute to locate where to add the channel send/receive (or
WaitGroup.Done/Wait).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a59d529a-c995-45f1-a749-8b6951726747
📒 Files selected for processing (17)
api/docs.goapi/openapi.yamlapi/swagger.jsonapi/swagger.yamlpkg/executors/http/auth/errors.gopkg/executors/http/auth/factory.gopkg/executors/http/auth/factory_test.gopkg/executors/http/auth/oauth2_token_endpoint.gopkg/executors/http/auth/oauth2_token_endpoint_export_test.gopkg/executors/http/auth/oauth2_token_endpoint_test.gopkg/executors/http/auth/token_fetcher.gopkg/executors/http/auth/types.gopkg/executors/http/http.gopkg/model/executor_configuration.gopkg/model/executor_configuration_input.gopkg/model/executor_configuration_test.gotests/integration/http_provider_auth_test.go
Comment on lines
+181
to
+213
| func parseOAuth2TokenEndpointConfig(data map[string]any) (*OAuth2TokenEndpointConfig, error) { | ||
| cfg := &OAuth2TokenEndpointConfig{} | ||
|
|
||
| if err := mapToStruct(data, cfg); err != nil { | ||
| return nil, fmt.Errorf("parse oauth2_token_endpoint config: %w", err) | ||
| } | ||
|
|
||
| if cfg.TokenURL == "" { | ||
| return nil, ErrOAuth2TokenEndpointURLRequired | ||
| } | ||
|
|
||
| if cfg.ClientID == "" { | ||
| return nil, ErrOAuth2TokenEndpointClientRequired | ||
| } | ||
|
|
||
| if cfg.ClientSecret == "" { | ||
| return nil, ErrOAuth2TokenEndpointSecretRequired | ||
| } | ||
|
|
||
| if cfg.CredentialsLocation != "" && | ||
| cfg.CredentialsLocation != "body" && | ||
| cfg.CredentialsLocation != "basic_header" { | ||
| return nil, ErrOAuth2TokenEndpointLocationInvalid | ||
| } | ||
|
|
||
| for k := range cfg.ExtraParams { | ||
| if reservedOAuth2ExtraParams[k] { | ||
| return nil, ErrOAuth2TokenEndpointReservedExtraParam | ||
| } | ||
| } | ||
|
|
||
| return cfg, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use pkg/errors.go validation types for parser failures.
parseOAuth2TokenEndpointConfig currently returns auth-local sentinel errors; this diverges from the repository’s typed error contract. Please map these validation failures to ValidationError (with field-level context) for consistent downstream handling.
As per coding guidelines, Use custom error types from pkg/errors.go (EntityNotFoundError, ValidationError) for error handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executors/http/auth/factory.go` around lines 181 - 213, Replace sentinel
auth errors in parseOAuth2TokenEndpointConfig with pkg/errors.go ValidationError
values: if mapToStruct returns an error, wrap it into a ValidationError (e.g.,
NewValidationError or constructing ValidationError) with field="" or more
specific context from the mapToStruct error; for empty
TokenURL/ClientID/ClientSecret return a ValidationError that identifies the
specific field ("token_url", "client_id", "client_secret") and an appropriate
message; for invalid CredentialsLocation return a ValidationError on field
"credentials_location"; and when reservedOAuth2ExtraParams[k] is true return a
ValidationError that references the offending extra param (field like
fmt.Sprintf("extra_params.%s", k)) and includes the reserved-param message; keep
the original checks (mapToStruct, reservedOAuth2ExtraParams loop) and replace
the ErrOAuth2TokenEndpoint* returns with constructed ValidationError instances
from pkg/errors.go so downstream code receives typed validation errors.
Comment on lines
+79
to
+80
| // Distinct prefix "oauth2te" to avoid collision with OIDC ("cc", "user"). | ||
| cacheKey := generateCacheKey("oauth2te", cfg.TokenURL, cfg.ClientID, cfg.Scopes) |
There was a problem hiding this comment.
Cache key is missing auth-shaping fields, causing token reuse collisions.
At Line [80], the key excludes Audience, CredentialsLocation, and ExtraParams. Two providers with same token_url + client_id + scopes but different request parameters can incorrectly share one cached token.
Suggested fix
- cacheKey := generateCacheKey("oauth2te", cfg.TokenURL, cfg.ClientID, cfg.Scopes)
+ cacheKey := generateCacheKey(
+ "oauth2te",
+ cfg.TokenURL,
+ cfg.ClientID,
+ cfg.CredentialsLocation,
+ cfg.Scopes,
+ cfg.Audience,
+ cfg.ExtraParams,
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executors/http/auth/oauth2_token_endpoint.go` around lines 79 - 80, The
cache key built for the OAuth2 token endpoint is missing auth-shaping fields and
can cause token reuse collisions; update the generateCacheKey invocation that
assigns cacheKey (currently using "oauth2te", cfg.TokenURL, cfg.ClientID,
cfg.Scopes) to also include cfg.Audience, cfg.CredentialsLocation, and
cfg.ExtraParams (or their normalized/stringified form) so the key uniquely
represents request-shaping parameters; ensure any map/struct in ExtraParams is
deterministically serialized before passing to generateCacheKey.
Comment on lines
+82
to
+87
| return &OAuth2TokenEndpointProvider{ | ||
| config: cfg, | ||
| cacheConfig: cacheCfg, | ||
| tokenFetcher: NewTokenFetcher(httpClient, nil), | ||
| cacheKey: cacheKey, | ||
| }, nil |
There was a problem hiding this comment.
Provider keeps a mutable caller config pointer; clone it to avoid races and drift.
At Lines [82]-[87], config: cfg stores external mutable state. If the caller mutates Scopes/ExtraParams after construction, Apply behavior and cache semantics can change unexpectedly, and concurrent map access can race.
Suggested fix
+func cloneOAuth2TokenEndpointConfig(in *OAuth2TokenEndpointConfig) *OAuth2TokenEndpointConfig {
+ if in == nil {
+ return nil
+ }
+ out := *in
+ if in.Scopes != nil {
+ out.Scopes = append([]string(nil), in.Scopes...)
+ }
+ if in.ExtraParams != nil {
+ out.ExtraParams = make(map[string]string, len(in.ExtraParams))
+ for k, v := range in.ExtraParams {
+ out.ExtraParams[k] = v
+ }
+ }
+ return &out
+}
+
func NewOAuth2TokenEndpointProvider(cfg *OAuth2TokenEndpointConfig, cacheCfg *CacheCfg, httpClient *http.Client) (*OAuth2TokenEndpointProvider, error) {
@@
return &OAuth2TokenEndpointProvider{
- config: cfg,
+ config: cloneOAuth2TokenEndpointConfig(cfg),
cacheConfig: cacheCfg,
tokenFetcher: NewTokenFetcher(httpClient, nil),
cacheKey: cacheKey,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return &OAuth2TokenEndpointProvider{ | |
| config: cfg, | |
| cacheConfig: cacheCfg, | |
| tokenFetcher: NewTokenFetcher(httpClient, nil), | |
| cacheKey: cacheKey, | |
| }, nil | |
| func cloneOAuth2TokenEndpointConfig(in *OAuth2TokenEndpointConfig) *OAuth2TokenEndpointConfig { | |
| if in == nil { | |
| return nil | |
| } | |
| out := *in | |
| if in.Scopes != nil { | |
| out.Scopes = append([]string(nil), in.Scopes...) | |
| } | |
| if in.ExtraParams != nil { | |
| out.ExtraParams = make(map[string]string, len(in.ExtraParams)) | |
| for k, v := range in.ExtraParams { | |
| out.ExtraParams[k] = v | |
| } | |
| } | |
| return &out | |
| } | |
| func NewOAuth2TokenEndpointProvider(cfg *OAuth2TokenEndpointConfig, cacheCfg *CacheCfg, httpClient *http.Client) (*OAuth2TokenEndpointProvider, error) { | |
| // ... function body ... | |
| return &OAuth2TokenEndpointProvider{ | |
| config: cloneOAuth2TokenEndpointConfig(cfg), | |
| cacheConfig: cacheCfg, | |
| tokenFetcher: NewTokenFetcher(httpClient, nil), | |
| cacheKey: cacheKey, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executors/http/auth/oauth2_token_endpoint.go` around lines 82 - 87, The
OAuth2TokenEndpointProvider constructor is storing the caller's mutable cfg
pointer (config: cfg), which can lead to races or drifting behavior if the
caller later mutates Scopes or ExtraParams; update the constructor that returns
*OAuth2TokenEndpointProvider to deep-clone cfg (copy primitive fields, duplicate
slices like Scopes and maps like ExtraParams) and store that copy in the
provider's config field so Apply and cacheKey semantics remain stable and
thread-safe; ensure any code referencing config inside
OAuth2TokenEndpointProvider and its Apply method uses the cloned copy and not
the original cfg.
Comment on lines
+390
to
+416
| func (f *TokenFetcher) FetchOAuth2TokenEndpointToken( | ||
| ctx context.Context, | ||
| cfg *OAuth2TokenEndpointConfig, | ||
| cacheKey string, | ||
| cacheConfig *CacheCfg, | ||
| ) (*TokenResponse, error) { | ||
| if token := f.getCachedToken(cacheKey, cacheConfig); token != nil { | ||
| return token, nil | ||
| } | ||
|
|
||
| token, err := f.fetchNewOAuth2TokenEndpointToken(ctx, cfg) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| f.cacheToken(cacheKey, token, cacheConfig) | ||
|
|
||
| return token, nil | ||
| } | ||
|
|
||
| func (f *TokenFetcher) fetchNewOAuth2TokenEndpointToken(ctx context.Context, cfg *OAuth2TokenEndpointConfig) (*TokenResponse, error) { | ||
| data := f.buildOAuth2TokenEndpointData(cfg) | ||
|
|
||
| req, err := newPinnedTokenRequest(ctx, cfg.TokenURL, data.Encode()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add a nil-config guard to prevent panic in the public fetch API.
FetchOAuth2TokenEndpointToken accepts a pointer but does not validate it. If cfg == nil, dereferences in fetchNewOAuth2TokenEndpointToken will panic (for example, Line [413]). Please fail fast with a typed validation error.
Suggested fix
func (f *TokenFetcher) FetchOAuth2TokenEndpointToken(
ctx context.Context,
cfg *OAuth2TokenEndpointConfig,
cacheKey string,
cacheConfig *CacheCfg,
) (*TokenResponse, error) {
+ if cfg == nil {
+ return nil, ErrOAuth2TokenEndpointConfigRequired
+ }
+
if token := f.getCachedToken(cacheKey, cacheConfig); token != nil {
return token, nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executors/http/auth/token_fetcher.go` around lines 390 - 416,
FetchOAuth2TokenEndpointToken currently accepts a *OAuth2TokenEndpointConfig but
doesn't validate it, causing fetchNewOAuth2TokenEndpointToken to dereference a
nil cfg; add an early nil check at the top of FetchOAuth2TokenEndpointToken that
returns a typed validation error (e.g., a wrapped or package-level error value
indicating "nil OAuth2TokenEndpointConfig") when cfg == nil, so callers get a
clear, non-panic failure instead of a runtime panic in
fetchNewOAuth2TokenEndpointToken or buildOAuth2TokenEndpointData.
Comment on lines
+325
to
+328
| "scopes": { "type": "array", "items": { "type": "string" }, "description": "OAuth2 scopes to request" }, | ||
| "audience": { "type": "string", "description": "Optional audience parameter" }, | ||
| "extra_params": { "type": "object", "additionalProperties": { "type": "string" }, "description": "Extra form parameters for token request (must not include client_id, client_secret, scope, audience)" } | ||
| } |
There was a problem hiding this comment.
Enforce the reserved extra_params key denylist in schema.
Line 327 documents forbidden keys, but the schema still accepts them. Add a structural constraint so invalid configs fail at schema validation time.
Suggested diff
- "extra_params": { "type": "object", "additionalProperties": { "type": "string" }, "description": "Extra form parameters for token request (must not include client_id, client_secret, scope, audience)" }
+ "extra_params": {
+ "type": "object",
+ "additionalProperties": { "type": "string" },
+ "propertyNames": {
+ "not": { "enum": ["client_id", "client_secret", "scope", "audience"] }
+ },
+ "description": "Extra form parameters for token request (must not include client_id, client_secret, scope, audience)"
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executors/http/http.go` around lines 325 - 328, The schema currently
allows forbidden keys in the "extra_params" object despite the comment; update
the JSON Schema for the "extra_params" property so validation rejects keys
"client_id", "client_secret", "scope", and "audience" (e.g., add a
"propertyNames" constraint with a "not" + "enum" listing those reserved names or
equivalent denylist). Modify the schema for the "extra_params" property in
pkg/executors/http/http.go (the object that currently has
"additionalProperties": { "type": "string" }) to include that propertyNames
denylist so invalid configs fail at schema validation time.
Comment on lines
+778
to
+831
| var capturedTenant string | ||
|
|
||
| tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if err := r.ParseForm(); err != nil { | ||
| http.Error(w, "invalid form", http.StatusBadRequest) | ||
| return | ||
| } | ||
|
|
||
| capturedTenant = r.Form.Get("tenant") | ||
|
|
||
| clientID := r.Form.Get("client_id") | ||
| clientSecret := r.Form.Get("client_secret") | ||
|
|
||
| if clientID != "test-client" || clientSecret != "test-secret" { | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
| return | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| writeJSON(t, w, map[string]any{ | ||
| "access_token": "mock-access-token-client-credentials", | ||
| "token_type": "Bearer", | ||
| "expires_in": 3600, | ||
| }) | ||
| })) | ||
| defer tokenSrv.Close() | ||
|
|
||
| target := mockTargetServer(t) | ||
| defer target.Close() | ||
|
|
||
| runner := httpExecutor.NewRunner() | ||
|
|
||
| input := executor.ExecutionInput{ | ||
| Config: map[string]any{ | ||
| "method": "GET", | ||
| "url": target.URL + "/protected", | ||
| "auth": map[string]any{ | ||
| "type": "oauth2_token_endpoint", | ||
| "config": map[string]any{ | ||
| "token_url": tokenSrv.URL + "/token", | ||
| "client_id": "test-client", | ||
| "client_secret": "test-secret", | ||
| "extra_params": map[string]any{"tenant": "acme"}, | ||
| }, | ||
| }, | ||
| }, | ||
| HTTPClient: &http.Client{Timeout: 10 * time.Second}, | ||
| } | ||
|
|
||
| result, err := runner.Execute(context.Background(), input) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, executor.ExecutionStatusSuccess, result.Status) | ||
| assert.Equal(t, "acme", capturedTenant, "extra_params must reach the token endpoint") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test file and inspect the relevant section
ls -la tests/integration/http_provider_auth_test.go
# Print the snippet region with line numbers
sed -n '740,860p' tests/integration/http_provider_auth_test.go | nl -ba
# Find all occurrences of capturedTenant in the file
rg -n "capturedTenant" tests/integration/http_provider_auth_test.go
# Also locate the exact test function name for context (if present)
rg -n "TestHTTPProviderAuthOAuth2TokenEndpointWithExtraParams" -S tests/integration/http_provider_auth_test.go
# Search for other similar patterns in the integration tests to compare style
rg -n "ParseForm\(\)|tenantCh|extra_params must reach the token endpoint" tests/integration/http_provider_auth_test.goRepository: LerianStudio/flowker
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/integration/http_provider_auth_test.go"
echo "== File =="; ls -la "$FILE"
echo "== Captured tenant occurrences ==";
rg -n "capturedTenant" "$FILE"
echo "== Context around the snippet (approx lines 760-860) ==";
sed -n '760,860p' "$FILE" | cat -n
echo "== Test function signature (exact) ==";
rg -n "func TestHTTPProviderAuthOAuth2TokenEndpointWithExtraParams" "$FILE"Repository: LerianStudio/flowker
Length of output: 4136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate httpExecutor runner implementation
rg -n "package httpExecutor|NewRunner\\(" -S . -g'*.go'
rg -n "type Runner|func \\(.*\\) Execute\\(" -S . -g'*.go' | head -n 50
# 2) Narrow search to the http executor package by using the identifier seen in the test
rg -n "httpExecutor\\.NewRunner" -S . -g'*.go'
fd -a -t f 'http*executor*' . 2>/dev/null || true
fd -t f 'http_executor*' . 2>/dev/null || true
fd -t f '*executor*' . | rg -n "http" | head -n 50 || true
# 3) Once files are found, print the Execute method body (first ~120 lines around it)
# (We rely on rg output; if it fails, the next block will still run useful searches.)
echo "---- End of searches ----"
# 4) Attempt race test for the specific test (may fail if Go toolchain/deps aren't available)
go test ./... -run TestHTTPProviderAuthOAuth2TokenEndpointWithExtraParams -race -count=1 2>&1 | head -n 200 || trueRepository: LerianStudio/flowker
Length of output: 12988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print runner.go with line numbers (bounded)
sed -n '1,220p' pkg/executors/http/runner.go | cat -n
echo "----"
# Print http.go (maybe contains auth wiring) with line numbers (bounded)
sed -n '1,220p' pkg/executors/http/http.go | cat -n
echo "----"
# Search for goroutines in the http executor package
rg -n "^\s*go\s" pkg/executors/http -S -g'*.go' || true
rg -n "goroutine" pkg/executors/http -S -g'*.go' || true
echo "----"
# Search for oauth2_token_endpoint handling
rg -n "oauth2_token_endpoint|token_endpoint" -S pkg/executors pkg | head -n 50
echo "----"
# Search for "token_url" usage in code
rg -n "token_url" -S pkg/executors pkg | head -n 50Repository: LerianStudio/flowker
Length of output: 27828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any goroutines in http executor package
rg -n "^\s*go\s" pkg/executors/http -S -g'*.go' || true
rg -n "goroutine" pkg/executors/http -S -g'*.go' || true
# Inspect applyAuth tail in runner.go (for any async patterns)
sed -n '200,320p' pkg/executors/http/runner.go | cat -n
# Find oauth2_token_endpoint implementation files
fd -t f 'oauth2*' pkg/executors/http/auth 2>/dev/null || true
fd -t f '*token*endpoint*' pkg/executors/http/auth 2>/dev/null || true
ls -ლა pkg/executors/http/auth
# Print oauth2 token endpoint files (bounded)
for f in $(ls pkg/executors/http/auth | rg -n 'oauth2|token_endpoint' | cut -d: -f1 || true); do
echo "---- $f ----"
sed -n '1,260p' "pkg/executors/http/auth/$f" | cat -n
done
# Also search within auth package for Apply method and goroutines
rg -n "func \\(.*\\) Apply\\(" pkg/executors/http/auth -S -g'*.go'
rg -n "oauth2_token_endpoint" pkg/executors/http/auth -S -g'*.go'Repository: LerianStudio/flowker
Length of output: 4112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show oauth2_token_endpoint implementation (bounded)
sed -n '1,260p' pkg/executors/http/auth/oauth2_token_endpoint.go | cat -n
# Search inside it for any async behavior / goroutines
rg -n "^\s*go\s|goroutine|WaitGroup|chan\s*<-" pkg/executors/http/auth/oauth2_token_endpoint.go -S || true
rg -n "client\\.Do\\(|http\\.NewRequest|Do\\(" pkg/executors/http/auth/oauth2_token_endpoint.go -S || true
# Also inspect related auth types for Apply implementation if Apply is in another file
rg -n "func \\(.*\\) Apply\\(" pkg/executors/http/auth -S -g'*.go'
# Finally, show the Apply() method body for oauth2 token endpoint (narrow by grep around its function)
rg -n "Apply\\(" pkg/executors/http/auth/oauth2_token_endpoint.go -S || trueRepository: LerianStudio/flowker
Length of output: 6109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find token fetcher implementation
rg -n "type TokenFetcher|NewTokenFetcher|FetchOAuth2TokenEndpointToken" pkg/executors/http/auth -S -g'*.go'
# Show the relevant file(s) around TokenFetcher and FetchOAuth2TokenEndpointToken (bounded)
FILES=$(rg -l "FetchOAuth2TokenEndpointToken" pkg/executors/http/auth -S -g'*.go')
echo "$FILES"
for f in $FILES; do
echo "---- $f ----"
sed -n '1,260p' "$f" | cat -n
rg -n "^\s*go\s|goroutine|WaitGroup|chan\s*<-|sync\\.|sync\\.Mutex|Mutex|RWMutex" "$f" -S -g'*.go' || true
done
# Search for any goroutines/channels anywhere in oauth2 token endpoint codepath
rg -n "^\s*go\s|goroutine|WaitGroup|chan\s*<-" pkg/executors/http/auth -S -g'*.go' || trueRepository: LerianStudio/flowker
Length of output: 15001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/executors/http/auth/token_fetcher.go"
# Goroutines in token_fetcher
rg -n "^\s*go\s|goroutine|WaitGroup|chan\s*<-" "$FILE" -S || true
# Show around FetchOAuth2TokenEndpointToken and helpers
sed -n '340,520p' "$FILE" | cat -n
# Show executeTokenRequest to see sync behavior
rg -n "func \\(f \\*TokenFetcher\\) executeTokenRequest" "$FILE"
sed -n '520,700p' "$FILE" | cat -n
# Search for any refresh/loop that might call token endpoint multiple times
rg -n "FetchOAuth2TokenEndpointToken|refresh|retry" "$FILE" -SRepository: LerianStudio/flowker
Length of output: 5356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/executors/http/auth/token_fetcher.go"
rg -n "^\s*go\s|goroutine|WaitGroup|chan\s*<-" "$FILE" -S || true
# Show refreshToken implementation (bounded)
rg -n "func \\(f \\*TokenFetcher\\) refreshToken\\(" "$FILE"
sed -n '260,380p' "$FILE" | cat -nRepository: LerianStudio/flowker
Length of output: 5438
Rephrase tenant-capture guidance: race is unlikely given synchronous token fetch.
capturedTenant is written by the token server handler, but the HTTP executor path is synchronous (http.Client.Do + io.ReadAll completes before runner.Execute returns), so the assertion runs after the token response has been fully processed—this makes an actual race/flaky behavior unlikely. Synchronizing via the proposed channel/mutex is still a good way to make intent explicit (especially if this test is ever refactored to issue the token request asynchronously).
Proposed fix
func TestHTTPProviderAuthOAuth2TokenEndpointWithExtraParams(t *testing.T) {
- var capturedTenant string
+ tenantCh := make(chan string, 1)
@@
- capturedTenant = r.Form.Get("tenant")
+ tenantCh <- r.Form.Get("tenant")
@@
- assert.Equal(t, "acme", capturedTenant, "extra_params must reach the token endpoint")
+ select {
+ case tenant := <-tenantCh:
+ assert.Equal(t, "acme", tenant, "extra_params must reach the token endpoint")
+ case <-time.After(2 * time.Second):
+ t.Fatal("token endpoint was not called")
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/http_provider_auth_test.go` around lines 778 - 831, The
test currently relies on capturedTenant being set by the tokenSrv handler before
the assertion after runner.Execute; to make the intent explicit and avoid any
future flakiness, change the test to synchronize the token handler and the
assertion: create a channel (or a sync.WaitGroup) and have the tokenSrv handler
send the tenant value (or signal done) when it extracts r.Form.Get("tenant")
(instead of only writing to capturedTenant), then after runner.Execute but
before the assert, receive from that channel (or wait) to guarantee the handler
ran; refer to capturedTenant, tokenSrv (the httptest handler) and runner.Execute
to locate where to add the channel send/receive (or WaitGroup.Done/Wait).
7 tasks
…Endpoint Mirrors the directive already applied to TypeOIDCClientCredentials at line 32. The Type constant value is an auth-flow identifier (string discriminator used by the factory dispatch), not a credential value — same false-positive shape gosec G101 flags on both. Unblocks the CI required check on PR #36.
Collaborator
Author
|
Thanks @coderabbitai for the thorough review! 🙏 After verifying each finding against the existing OIDC providers ( The only item unique to this PR ( Per-item analysis
Real-world validationThis PR + its downstream DataRudder branch (#37) were smoke-tested end-to-end against
Next steps
Happy to discuss any of the above if my reading is off on a specific item. |
|
Tip For best results, initiate chat on the files or code changes.
The scoping decision (keep this PR focused on the new provider; track parity concerns separately with the product owner) is sound for an external collaborator contribution. Would you like me to open GitHub tracking issues for the platform-wide parity items so they don't get lost after this PR merges? I can open one consolidated issue (or separate ones) covering:
Just let me know and I'll create them assigned to you or |
dy-shimizu
changed the title
dy-shimizu
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new outbound auth provider
oauth2_token_endpointfor non-OIDC OAuth2 client_credentials flows. The driver is DataRudder/Delorean which exposes/api/v1/auth/loginacceptingclient_id+client_secretin form body and returning{access_token, token_type, expires_in}— RFC 6749-shaped but without an OIDC discovery document.This PR is prerequisite for the upcoming DataRudder provider (separate PR coming after this merges).
What's added
pkg/executors/http/auth/oauth2_token_endpoint.gopkg/executors/http/auth/token_fetcher.goFetchOAuth2TokenEndpointToken+fetchNewOAuth2TokenEndpointToken(routes via existingnewPinnedTokenRequestfor safehttp.Validate pre-flight) +buildOAuth2TokenEndpointDatapkg/executors/http/auth/types.goTypeOAuth2TokenEndpointconst +OAuth2TokenEndpointConfigstructpkg/executors/http/auth/factory.gocasearm +parseOAuth2TokenEndpointConfigpkg/executors/http/auth/errors.gopkg/model/executor_configuration{,_input,_test}.gooneof=tag extensionspkg/executors/http/http.goif/thenconfig blockapi/{docs.go,swagger.json,swagger.yaml,openapi.yaml}*_test.go(unit + integration)Hardening
extra_paramsdenylist — canonical fields (client_id,client_secret,scope,audience) cannot be overridden via map injection.token_typeallowlist — Bearer only (case-insensitive) or empty (defaults to Bearer); rejects MAC/DPoP/Basic withErrOAuth2TokenEndpointUnsupportedTokenType. Test asserts noaccess_tokenleakage into any header on rejection.access_tokenguard — defends against misbehaving IdPs returning{"access_token":"","token_type":"Bearer",...}. ReturnsErrOAuth2TokenEndpointEmptyAccessTokeninstead of emittingAuthorization: Bearer.oauth2te(distinct from OIDCcc/user).safehttp.Validatepre-flight + dial-time IP policy viasafehttp.NewClient-wrapped transport (consumes PR feat(infrastructure): defense-in-depth SSRF protection for outbound HTTP #24's helper, no reinvention).Test plan
make test-unit— PASS (includes 24 oauth2 unit tests)go test -tags=unit ./...— PASS (covers all packages)make test-integration— PASS (4 new TestHTTPProviderAuthOAuth2TokenEndpoint* scenarios + existing OIDC tests)make test-e2e— PASS (TestFullE2E + TestWebhookE2E full lifecycle)gofmt -l pkg/executors/http/auth/— cleango vet ./pkg/executors/http/auth/...— cleangolangci-lint run ./pkg/executors/http/auth/...— 0 issueshttps://inference.api-stg.delorean-ai.comvia downstream DataRudder branch —overallStatus: passed, all 3 stages green. Re-smoke after codereview fixes also passed.Notes for reviewers
Struct tags on
OAuth2TokenEndpointConfigintentionally omitted — the package does not invokevalidator.Structon this config. Validation runs via explicit if-checks inNewOAuth2TokenEndpointProvider. Docblock attypes.go:100-106documents the intent. (Sibling OIDC configs have decorative tags that also never run — that's pre-existing pattern.)fetchNewOAuth2TokenEndpointTokenroutes throughnewPinnedTokenRequest— matches the 3 OIDC fetch paths exactly (post-PR feat(infrastructure): defense-in-depth SSRF protection for outbound HTTP #24safehttpmigration). Pre-flightsafehttp.Validateenforces SSRF policy without URL pinning, keeping TLS SNI correct.No
grant_typeset by default in the form body — intentional Delorean accommodation (their/api/v1/auth/logindoes not require it). Callers needing strict RFC 6749 compliance setgrant_type=client_credentialsviaextra_params. Locked in byTestOAuth2TokenEndpointProvider_NoGrantTypeByDefault.API artifact regen via hand-patch —
make generate-docsis broken at HEAD due to a pre-existing malformed@Paramannotation ininternal/adapters/http/in/webhook/handler.go:83. Hand-patchedapi/*with narrow enum-only diffs to avoid touching out-of-scope files. Reported to platform team separately.Findings reported separately
During integration, several observations outside this PR's scope were communicated to @alexgarzao:
docs/connectivity-prober-guide.mdsilent on TLS SNI /safehttp.Validateadoptionsafehttp.Validateavailable since PR feat(infrastructure): defense-in-depth SSRF protection for outbound HTTP #24 but not adopted by Tracer/Midaz probersAuthorization: Bearer; etc.)make generate-docsbroken by webhook handler annotationmake test-unitdoesn't include-tags=unittests, so datarudder/midaz/tracer unit tests don't run in_pre-validations🤖 Generated with Claude Code