fix: fail loud when enable-csrf is set but csrf-key annotation is missing#440
Open
shreemaan-abhishek wants to merge 2 commits into
Open
fix: fail loud when enable-csrf is set but csrf-key annotation is missing#440shreemaan-abhishek wants to merge 2 commits into
shreemaan-abhishek wants to merge 2 commits into
Conversation
Previously, when the enable-csrf annotation was set to true but the csrf-key annotation was missing or empty, csrf.Handle returned (nil, nil), which is indistinguishable from the annotation being absent. The route was then programmed without the csrf plugin and the Ingress reconciled cleanly, so an endpoint the operator believed was protected ran unprotected with no error, event, or log. Return an error in this case so the parser surfaces it instead of silently dropping the security-relevant plugin. The error message does not echo the key value.
📝 WalkthroughWalkthroughChangesAnnotation error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-24T10:27:11Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-24T10:27:13Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-24T10:44:46Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- GatewayModifyListeners
result: failure
statistics:
Failed: 1
Passed: 11
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 1 test failures.
- core:
failedTests:
- GatewayModifyListeners
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures. |
jarvis9443
reviewed
Jul 24, 2026
| 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. |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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:
plugins.Parse- logged the handler error,continue, returned a nil error.TranslateIngressAnnotations- logged the parser error and returned the config anyway.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
upstreamparser, so an invalidupstream-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'dhttpssilently 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 schemecase inTestTranslateIngressAnnotationsto expect an error for that reason, and added parser-level cases covering both missing and emptycsrf-key.
Full unit suite passes. The same change is in the upstream PR (apache/apisix-ingress-controller#2813).
Returning an error from csrf.Handle was not enough to stop an
unprotected route from being programmed: the error was swallowed at
three separate points.
1. plugins.Parse logged the handler error and continued, returning 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.
Propagate the error through all three so TranslateIngress fails. The
provider aborts before building the sync task, so the route is no
longer programmed and the failure surfaces on the reconcile instead of
in a log line nobody reads.
This also stops an invalid upstream-scheme/retry/timeout annotation
from being silently ignored, which had the same silent-downgrade shape
(a typo'd "https" fell back to plaintext http).
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
2 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.
What this PR does
When the
enable-csrf: "true"annotation is set butcsrf-keyis missing or empty,csrf.Handlereturned(nil, nil)- indistinguishable from the annotation being absent. The route was programmed without the csrf plugin, the Ingress reconciled cleanly, and an endpoint the operator believed was CSRF-protected ran unprotected.Fix
Returning an error from
csrf.Handlealone was not enough, because the error was swallowed at three separate points:plugins.Parselogged the handler error,continued, and returned a nil error.TranslateIngressAnnotationslogged the parser error and returned the config anyway.TranslateIngresshad no error to inspect, since the function above returned only a config.All three now propagate.
TranslateIngressreturns the error and the provider aborts atprovider.go:154before the sync task is built, so the route is never programmed and the failure surfaces on the reconcile rather than in a log line.Fixing only point 1 would have been worse than the bug: on error
Parsereturns nil for the entire plugin map, which also holdskey-auth,basic-auth,ip-restrictionandforward-auth. With points 2 and 3 still swallowing, the route would still ship, now stripped of authentication too.The csrf error message does not echo the key value.
Behavior change worth reviewing
Any annotation handler error now fails translation, not just csrf. That includes the
upstreamparser, so an invalidupstream-scheme/retry/timeout annotation now fails the Ingress instead of being silently ignored. This is the same bug class (a typo'dhttpssilently fell back to plaintext http), but it will turn previously-"working" typo'd Ingresses red on upgrade. Can be narrowed to plugin handlers only if preferred.Test
TestCSRFHandler: missing-key and empty-key now return an error.TestTranslateIngressAnnotations: added parser-level cases for both, proving translation fails end to end. Updated the existinginvalid schemecase to expect an error.Fixes FINDING-031 (api7/rfcs#165). Synced with apache/apisix-ingress-controller#2813.
Summary by CodeRabbit