Skip to content

fix: fail loud when enable-csrf is set but csrf-key annotation is missing #440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shreemaan-abhishek wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: fail loud when enable-csrf is set but csrf-key annotation is missing #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5d92cf
fix: fail loud when enable-csrf is set but csrf-key is missing
shreemaan-abhishek Jul 19, 2026
2d1b3ba
fix: fail Ingress translation when an annotation handler errors
shreemaan-abhishek Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions internal/adc/translator/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ var ingressAnnotationParsers = map[string]annotations.IngressAnnotationsParser{
"UseRegex": regex.NewParser(),
}

func (t *Translator) TranslateIngressAnnotations(anno map[string]string) *IngressConfig {
func (t *Translator) TranslateIngressAnnotations(anno map[string]string) (*IngressConfig, error) {
if len(anno) == 0 {
return nil
return nil, nil
}
ing := &IngressConfig{}
if err := translateAnnotations(anno, ing); err != nil {
t.Log.Error(err, "failed to translate ingress annotations", "annotations", anno)
return nil, err
}
return ing
return ing, nil
}

func translateAnnotations(anno map[string]string, dst any) error {
Expand Down
7 changes: 6 additions & 1 deletion internal/adc/translator/annotations/plugins/csrf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
package plugins

import (
"fmt"

adctypes "github.com/apache/apisix-ingress-controller/api/adc"
"github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations"
)
Expand All @@ -39,7 +41,10 @@ func (c *csrf) Handle(e annotations.Extractor) (any, error) {

key := e.GetStringAnnotation(annotations.AnnotationsCsrfKey)
if key == "" {
return nil, nil
// csrf requested but the key is missing: fail loud instead of
// silently dropping the plugin and programming the route unprotected.

@jarvis9443 jarvis9443 Jul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still fails open at the route level. plugins.Parse logs handler errors and continues, then returns a nil error, so this error never reaches reconciliation and the route is still programmed without the csrf plugin. The linked finding explicitly requires translation to fail rather than only adding a log entry. Please propagate the handler error from plugins.Parse and cover the parser/reconcile path so enable-csrf=true cannot reconcile successfully without a key.

@shreemaan-abhishek shreemaan-abhishek Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and I have pushed the full fix.

Confirming your diagnosis: the error was swallowed and never reached reconciliation. I traced it and there were three swallow points, not one:

  1. plugins.Parse - logged the handler error, continue, returned a nil error.
  2. TranslateIngressAnnotations - logged the parser error and returned the config anyway.
  3. TranslateIngress - had no error to inspect, since the function above returned only a config.

One note on scope, because the diff is wider than the single change you asked for. Propagating from plugins.Parse alone would have made things worse: on error Parse returns nil for the entire plugin map, which also holds key-auth, basic-auth, ip-restriction and forward-auth. With points 2 and 3 still swallowing, the route would still have been programmed, now stripped of authentication and IP restrictions as well. A one-plugin gap would have become an all-plugins gap. So points 2 and 3 had to move together with point 1 for point 1 to be safe.

With all three propagating, TranslateIngress now returns the error, and the provider aborts at provider.go:154 before the sync task is built, so the route is never programmed and the failure surfaces on the reconcile.

Two things worth flagging for your review:

  • Behavior change beyond csrf. Any annotation handler error now fails translation. That includes the upstream parser, so an invalid upstream-scheme/retry/timeout annotation now fails the Ingress instead of being silently ignored. I think this is correct and the same bug class (a typo'd https silently fell back to plaintext http), but it will turn previously-"working" typo'd Ingresses red on upgrade. Happy to narrow it to plugin handlers only if you would rather not carry that.
  • I updated the existing invalid scheme case in TestTranslateIngressAnnotations to expect an error for that reason, and added parser-level cases covering both missing and empty csrf-key.

Full unit suite passes. The same change is in the upstream PR (apache/apisix-ingress-controller#2813).

return nil, fmt.Errorf("annotation %q is enabled but %q is missing or empty",
annotations.AnnotationsEnableCsrf, annotations.AnnotationsCsrfKey)
}

return &adctypes.CSRFConfig{
Expand Down
10 changes: 8 additions & 2 deletions internal/adc/translator/annotations/plugins/csrf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,16 @@ func TestCSRFHandler(t *testing.T) {
assert.Nil(t, err, "checking given error")
assert.Nil(t, out, "checking given output")

// Test with enable-csrf true but no key
// Test with enable-csrf true but no key: must fail loud, not drop silently
anno[annotations.AnnotationsEnableCsrf] = "true"
delete(anno, annotations.AnnotationsCsrfKey)
out, err = p.Handle(annotations.NewExtractor(anno))
assert.Nil(t, err, "checking given error")
assert.Error(t, err, "expecting an error when csrf-key is missing")
assert.Nil(t, out, "checking given output when key is missing")

// Test with enable-csrf true but empty key: same fail-loud behavior
anno[annotations.AnnotationsCsrfKey] = ""
out, err = p.Handle(annotations.NewExtractor(anno))
assert.Error(t, err, "expecting an error when csrf-key is empty")
assert.Nil(t, out, "checking given output when key is empty")
}
9 changes: 4 additions & 5 deletions internal/adc/translator/annotations/plugins/plugins.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package plugins

import (
logf "sigs.k8s.io/controller-runtime/pkg/log"
"fmt"

adctypes "github.com/apache/apisix-ingress-controller/api/adc"
"github.com/apache/apisix-ingress-controller/internal/adc/translator/annotations"
Expand All @@ -33,8 +33,6 @@ type PluginAnnotationsHandler interface {
}

var (
log = logf.Log.WithName("annotations").WithName("plugins").WithName("parser")

handlers = []PluginAnnotationsHandler{
NewRedirectHandler(),
NewRewriteHandler(),
Expand All @@ -60,8 +58,9 @@ func (p *plugins) Parse(e annotations.Extractor) (any, error) {
for _, handler := range handlers {
out, err := handler.Handle(e)
if err != nil {
log.Error(err, "Failed to handle annotation", "handler", handler.PluginName())
continue
// Fail closed: abort translation so the route is never programmed
// while a requested plugin is missing.
return nil, fmt.Errorf("%s: %w", handler.PluginName(), err)
}
if out != nil {
plugins[handler.PluginName()] = out
Expand Down
34 changes: 30 additions & 4 deletions internal/adc/translator/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,35 @@ func TestTranslateIngressAnnotations(t *testing.T) {
name string
anno map[string]string
expected *IngressConfig
wantErr bool
}{
{
name: "no matching annotations",
anno: map[string]string{"upstream": "value1"},
expected: &IngressConfig{},
},
{
name: "invalid scheme",
anno: map[string]string{annotations.AnnotationsUpstreamScheme: "invalid"},
expected: &IngressConfig{},
// enable-csrf without a key must fail translation, never reconcile
// into a route that silently lacks the csrf plugin.
name: "enable-csrf without csrf-key",
anno: map[string]string{
annotations.AnnotationsEnableCsrf: "true",
},
wantErr: true,
},
{
name: "enable-csrf with empty csrf-key",
anno: map[string]string{
annotations.AnnotationsEnableCsrf: "true",
annotations.AnnotationsCsrfKey: "",
},
wantErr: true,
},
{
// a typo'd scheme used to fall back to plaintext http silently
name: "invalid scheme",
anno: map[string]string{annotations.AnnotationsUpstreamScheme: "invalid"},
wantErr: true,
},
{
name: "http scheme",
Expand Down Expand Up @@ -343,8 +362,15 @@ func TestTranslateIngressAnnotations(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
translator := &Translator{}
result := translator.TranslateIngressAnnotations(tt.anno)
result, err := translator.TranslateIngressAnnotations(tt.anno)

if tt.wantErr {
assert.Error(t, err)
assert.Nil(t, result)
return
}

assert.NoError(t, err)
assert.NotNil(t, result)
assert.Equal(t, tt.expected, result)
})
Expand Down
5 changes: 4 additions & 1 deletion internal/adc/translator/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ func (t *Translator) TranslateIngress(

labels := label.GenLabel(obj)

config := t.TranslateIngressAnnotations(obj.Annotations)
config, err := t.TranslateIngressAnnotations(obj.Annotations)
if err != nil {
return nil, err
}

t.Log.V(1).Info("translating Ingress Annotations", "config", config)

Expand Down
Loading