diff --git a/.traefik.yml b/.traefik.yml index 858eacc..64fa6e2 100644 --- a/.traefik.yml +++ b/.traefik.yml @@ -60,14 +60,18 @@ testData: scopes: - roles - # Templated headers forwarded to backends. - # NOTE: use quadruple braces — the YAML parser collapses {{{{ → {{ so the - # Go template engine receives the correct expression. + # Templated headers forwarded to backends. Values are Go templates and must + # be written with plain double braces — the catalog analyzer feeds them to + # the plugin verbatim (issue #151; quadruple braces do NOT get collapsed). + # Only when your dynamic config file is rendered by Traefik's file-provider + # Go templating do the braces need escaping there, e.g. + # value: "{{`{{.Claims.email}}`}}" + # (see README "Templated headers"). headers: - name: X-User-Email - value: "{{{{.Claims.email}}}}" + value: "{{.Claims.email}}" - name: Authorization - value: "Bearer {{{{.AccessToken}}}}" + value: "Bearer {{.AccessToken}}" # Security headers (default profile is enabled out of the box) securityHeaders: diff --git a/README.md b/README.md index ff47644..3ad0c09 100644 --- a/README.md +++ b/README.md @@ -372,11 +372,11 @@ Forward identity to backends via Go templates over ID-token claims and tokens: ```yaml headers: - name: X-User-Email - value: "{{{{.Claims.email}}}}" + value: "{{.Claims.email}}" - name: Authorization - value: "Bearer {{{{.AccessToken}}}}" + value: "Bearer {{.AccessToken}}" - name: X-User-Roles - value: "{{{{range $i, $e := .Claims.roles}}}}{{{{if $i}}}},{{{{end}}}}{{{{$e}}}}{{{{end}}}}" + value: "{{range $i, $e := .Claims.roles}}{{if $i}},{{end}}{{$e}}{{end}}" ``` Available bindings: `.Claims.`, `.AccessToken`, `.IdToken` @@ -397,7 +397,7 @@ allowedClaims: - employee_id headers: - name: X-Employee-Id - value: "{{{{.Claims.employee_id}}}}" + value: "{{.Claims.employee_id}}" ``` Rendering the whole context (`{{.}}`, `{{$}}`), the whole claims map @@ -406,11 +406,16 @@ from accidentally forwarding raw tokens or unlisted claims. `range`/`with` must target a specific listed claim (e.g. `{{range .Claims.groups}}`); `get`/`default` are the only functions allowed. -> **Escape with quadruple braces.** If you see -> `can't evaluate field AccessToken in type bool`, Traefik's YAML parser ate -> your `{{ }}`. The fix that actually works is `{{{{ }}}}` — the YAML pass -> turns it into `{{ }}` for the Go template engine. Other escaping tricks -> (literal blocks, single quotes) do not work reliably. +> **File-provider users: escape the braces.** Traefik's file provider runs +> every dynamic configuration file through Go templating before the plugin +> sees it. Plain `{{.AccessToken}}` then fails with +> `can't evaluate field AccessToken in type bool`. Wrap the expression in a +> raw string so the file provider emits it literally: +> ``value: "{{`{{.Claims.email}}`}}"``. All other providers (Kubernetes CRD, +> Docker labels, Consul, ...) pass the value through untouched — use the plain +> form there. Quadruple braces (`{{{{ }}}}`) do not work anywhere: the file +> provider fails to parse them, and every other path hands them to the plugin +> verbatim, where template validation rejects them (issues #149, #151). ## Default downstream headers @@ -438,7 +443,7 @@ section — see [docs/CONFIGURATION.md](docs/CONFIGURATION.md#security-headers). | `No matching public key found` | JWKS endpoint down, or `kid` mismatch. | | `Access denied: Your email domain is not allowed` | User's domain not in `allowedUserDomains`. | | `Access denied: You do not have any of the allowed roles or groups` | Claims missing or not in `allowedRolesAndGroups`. | -| `can't evaluate field AccessToken in type bool` | Template not escaped — use `{{{{ }}}}`. | +| `can't evaluate field AccessToken in type bool` | File provider templated your header value — escape it: ``"{{`{{.AccessToken}}`}}"`` (see "Templated headers"). | | `tls: failed to verify certificate: x509: certificate signed by unknown authority` | Internal CA — set `caCertPath` / `caCertPEM`. | | `invalid handler type: ` | Env var name contains `API` — rename it. | | `false positive replay detected` | Multi-replica without Redis — see [Multi-replica deployments](#multi-replica-deployments). | diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 533d1f5..7dbb372 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -431,13 +431,13 @@ minimalHeaders: true # Only forwards X-Forwarded-User ```yaml headers: - name: "X-User-Email" - value: "{{{{.Claims.email}}}}" + value: "{{.Claims.email}}" - name: "X-User-ID" - value: "{{{{.Claims.sub}}}}" + value: "{{.Claims.sub}}" - name: "Authorization" - value: "Bearer {{{{.AccessToken}}}}" + value: "Bearer {{.AccessToken}}" - name: "X-User-Roles" - value: "{{{{range $i, $e := .Claims.roles}}}}{{{{if $i}}}},{{{{end}}}}{{{{$e}}}}{{{{end}}}}" + value: "{{range $i, $e := .Claims.roles}}{{if $i}},{{end}}{{$e}}{{end}}" ``` **Template Variables:** @@ -446,7 +446,14 @@ headers: - `{{.IdToken}}` / `{{.IDToken}}` - Raw ID token (both spellings work) - `{{.RefreshToken}}` - Raw refresh token -**Important:** Use double curly braces (`{{{{` and `}}}}`) to escape templates in YAML. +**Important — file provider only:** Traefik's file provider runs every dynamic +configuration file through Go templating before the plugin sees it, so plain +`{{ }}` fails there with `can't evaluate field AccessToken in type bool`. +Escape with a raw string: ``value: "{{`{{.Claims.email}}`}}"``. All other +providers (Kubernetes CRD, Docker labels, Consul, ...) pass the value through +untouched — use the plain form shown above. Do not use quadruple braces +(`{{{{ }}}}`); no configuration path collapses them, and template validation +rejects them (issues #149, #151). **Validation and the claims whitelist.** Header templates are parsed and validated when the middleware loads; an invalid or disallowed template stops the @@ -473,7 +480,7 @@ allowedClaims: - cost_center headers: - name: "X-Employee-Id" - value: "{{{{.Claims.employee_id}}}}" + value: "{{.Claims.employee_id}}" ``` Listed names apply to both direct access (`{{.Claims.employee_id}}`) and diff --git a/issue151_regression_test.go b/issue151_regression_test.go new file mode 100644 index 0000000..608daf7 --- /dev/null +++ b/issue151_regression_test.go @@ -0,0 +1,114 @@ +package traefikoidc + +import ( + "context" + "encoding/json" + "net/http" + "os" + "testing" + + "gopkg.in/yaml.v3" +) + +// Issue #151: the Traefik Plugin Catalog analyzer (piceus) failed v1.0.28 with +// "failed to run the plugin with Yaegi: invalid handler type: ". Two +// independent defects combined to produce that opaque report: +// +// 1. The analyzer feeds .traefik.yml testData to New() RAW — paerser and +// mapstructure perform no brace collapsing, so the quadruple-brace header +// values ("{{{{.Claims.email}}}}") reached Config.Validate() verbatim and +// failed template parsing (`unexpected "{" in command`). The quadruple-brace +// convention (commit 2d1b04c) was based on a false premise; YAML does not +// collapse braces. +// +// 2. New() tail-called `return NewWithContext(...)`, converting the returned +// (*TraefikOidc, error) to (http.Handler, error) in the return statement. +// Under yaegi v0.16.1 (the interpreter piceus and Traefik embed) that +// conversion zeroes BOTH results when the error path is taken, so piceus +// saw (nil, nil), skipped its error branch, and reported the handler-type +// message instead of the real validation error. Compiled Go had the sibling +// defect: the typed-nil *TraefikOidc made New() return a NON-nil +// http.Handler alongside a non-nil error. +// +// These tests pin both fixes natively (no interpreter needed): +// TestIssue151_ManifestTestDataPassesAnalyzer pins (1) and +// TestIssue151_NewReturnsUntypedNilHandlerOnInvalidConfig pins (2). + +// manifestTestData reads .traefik.yml and returns testData exactly as the +// analyzer sees it: parsed by a YAML parser, no template preprocessing. +func manifestTestData(t *testing.T) map[string]any { + t.Helper() + + raw, err := os.ReadFile(".traefik.yml") + if err != nil { + t.Fatalf("read .traefik.yml: %v", err) + } + + var manifest struct { + TestData map[string]any `yaml:"testData"` + } + if err := yaml.Unmarshal(raw, &manifest); err != nil { + t.Fatalf("parse .traefik.yml: %v", err) + } + if len(manifest.TestData) == 0 { + t.Fatal(".traefik.yml has no testData — the analyzer rejects manifests without it") + } + return manifest.TestData +} + +// TestIssue151_ManifestTestDataPassesAnalyzer decodes the manifest's testData +// into a Config the same way piceus does (defaults from CreateConfig, then the +// raw testData values on top; JSON round-trip stands in for mapstructure) and +// requires the full analyzer contract: Validate() passes and New() yields a +// working handler with no error. +func TestIssue151_ManifestTestDataPassesAnalyzer(t *testing.T) { + testData := manifestTestData(t) + + cfg := CreateConfig() + buf, err := json.Marshal(testData) + if err != nil { + t.Fatalf("marshal testData: %v", err) + } + if err := json.Unmarshal(buf, cfg); err != nil { + t.Fatalf("decode testData into Config: %v", err) + } + + if err := cfg.Validate(); err != nil { + t.Fatalf("manifest testData must pass Validate() — the catalog analyzer feeds it to New() raw (issue #151): %v", err) + } + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "issue151-analyzer") + if err != nil { + t.Fatalf("New() with manifest testData must succeed (issue #151): %v", err) + } + if h == nil { + t.Fatal("New() with manifest testData returned a nil handler") + } + if closer, ok := h.(interface{ Close() error }); ok { + _ = closer.Close() + } +} + +// TestIssue151_NewReturnsUntypedNilHandlerOnInvalidConfig requires New() to +// return an UNTYPED nil handler when construction fails. The previous +// `return NewWithContext(...)` tail call wrapped a typed-nil *TraefikOidc into +// the http.Handler interface (h != nil despite err != nil), and under yaegi the +// same statement zeroed the error as well, which is what turned every +// validation failure into Traefik's opaque "invalid handler type: ". +func TestIssue151_NewReturnsUntypedNilHandlerOnInvalidConfig(t *testing.T) { + cfg := issue149BaseConfig() + cfg.Headers = []TemplatedHeader{ + // Invalid template — the analyzer-side shape that triggered #151. + {Name: "X-User-Email", Value: `{{{{.Claims.email}}}}`}, + } + + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + h, err := New(context.Background(), next, cfg, "issue151-nil") + if err == nil { + t.Fatal("New() must reject an invalid header template") + } + if h != nil { + t.Fatalf("New() must return an untyped nil handler on error, got %T (typed nil leaks through the http.Handler interface and breaks the analyzer's error reporting)", h) + } +} diff --git a/main.go b/main.go index 10bb662..2d3409d 100644 --- a/main.go +++ b/main.go @@ -136,7 +136,20 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h telemetry.Send("traefikoidc", traefikoidcPluginVersion) } }) - return NewWithContext(ctx, config, next, name) + // Deliberately NOT `return NewWithContext(...)` (issue #151): that tail call + // converts (*TraefikOidc, error) to (http.Handler, error) in the return + // statement. Compiled, a failed construction then returns a typed-nil + // *TraefikOidc wrapped in a NON-nil http.Handler; under yaegi v0.16.1 (the + // interpreter Traefik and the plugin-catalog analyzer embed) the same + // statement zeroes BOTH results on the error path, so callers saw + // (nil, nil) and reported the opaque "invalid handler type: " instead + // of the real error. The explicit split returns an untyped nil handler and + // keeps the error intact in both worlds. + t, err := NewWithContext(ctx, config, next, name) + if err != nil { + return nil, err + } + return t, nil } // NewWithContext creates a new TraefikOidc middleware instance with proper context handling.