fix: fail loud when enable-csrf is set but csrf-key annotation is missing by shreemaan-abhishek · Pull Request #440 · api7/api7-ingress-controller

shreemaan-abhishek · 2026-07-19T09:04:42Z

What this PR does

Fixes FINDING-031 (api7/rfcs#165).

When the enable-csrf: "true" annotation is set but the csrf-key annotation is missing or empty, csrf.Handle returned (nil, nil) in internal/adc/translator/annotations/plugins/csrf.go - indistinguishable from the annotation being absent.

The result: the route is programmed without the csrf plugin, the Ingress reconciles cleanly, and an endpoint the operator believed was CSRF-protected runs unprotected, with no error, event, or log.

Fix

Return an error when enable-csrf is enabled but csrf-key is missing/empty. The plugin parser already logs handler errors at error level (plugins.go), so the misconfiguration is now surfaced loudly instead of silently dropping a security-relevant plugin. The error message does not echo the key value.

The handler interface doc already states callers should "judge whether Handle is failed by the second error value", so this aligns the csrf handler with the intended contract.

Test

Updated TestCSRFHandler to assert that both the missing-key and empty-key cases now return an error.

go test ./internal/adc/translator/annotations/plugins/ -run TestCSRFHandler

Synced with the upstream apisix-ingress-controller PR carrying the same change.

Summary by CodeRabbit

Bug Fixes
- CSRF configuration now reports an error when CSRF protection is enabled without a valid CSRF key.
- Prevents incomplete CSRF configuration from being silently ignored.

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.

coderabbitai · 2026-07-19T09:05:02Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0c57c44c-ecb7-4cfd-830c-80cf113f3e7a

📥 Commits

Reviewing files that changed from the base of the PR and between 5f84a00 and a5d92cf.

📒 Files selected for processing (2)

internal/adc/translator/annotations/plugins/csrf.go
internal/adc/translator/annotations/plugins/csrf_test.go

📝 Walkthrough

Walkthrough

Changes

CSRF validation

Layer / File(s)	Summary
Fail-loud CSRF key validation `internal/adc/translator/annotations/plugins/csrf.go`, `internal/adc/translator/annotations/plugins/csrf_test.go`	CSRF handling now returns an error when enabled without a non-empty key, with tests covering both missing and empty annotations.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
E2e Test Quality Review	⚠️ Warning	Only unit coverage was added; no e2e test exercises the annotation→reconcile→APISIX flow or missing/empty csrf-key behavior.	Add an e2e case that creates a misconfigured Ingress with enable-csrf=true and missing/empty csrf-key, then assert the observed reconcile/log/plugin outcome.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: erroring when CSRF is enabled without a csrf-key annotation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Security Check	✅ Passed	No security regressions: CSRF now fails closed; parser logs only handler name and error, with no key value leakage.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/csrf-fail-loud

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-19T09:17:42Z

conformance test report - apisix mode

apiVersion: gateway.networking.k8s.io/v1
date: "2026-07-19T09:17:39Z"
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.

github-actions · 2026-07-19T09:18:45Z

conformance test report - apisix-standalone mode

apiVersion: gateway.networking.k8s.io/v1
date: "2026-07-19T09:18:42Z"
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:
    result: partial
    skippedTests:
    - HTTPRouteHTTPSListener
    statistics:
      Failed: 0
      Passed: 32
      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 partially succeeded with 1 test skips. 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.

github-actions · 2026-07-19T09:37:12Z

conformance test report

apiVersion: gateway.networking.k8s.io/v1
date: "2026-07-19T09:37:09Z"
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:
    failedTests:
    - HTTPRouteBackendProtocolWebSocket
    result: failure
    skippedTests:
    - HTTPRouteRedirectPortAndScheme
    statistics:
      Failed: 1
      Passed: 10
      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 failed with 1 test
    failures.
- core:
    failedTests:
    - GatewayModifyListeners
    - TLSRouteSimpleSameNamespace
    result: failure
    statistics:
      Failed: 2
      Passed: 9
      Skipped: 0
  name: GATEWAY-TLS
  summary: Core tests failed with 2 test failures.

shreemaan-abhishek self-assigned this Jul 20, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix: fail loud when enable-csrf is set but csrf-key annotation is missing#440
shreemaan-abhishek wants to merge 1 commit into
masterfrom
fix/csrf-fail-loud

shreemaan-abhishek commented Jul 19, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 19, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

shreemaan-abhishek commented Jul 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Fix

Test

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 19, 2026

conformance test report - apisix mode

Uh oh!

github-actions Bot commented Jul 19, 2026

conformance test report - apisix-standalone mode

Uh oh!

github-actions Bot commented Jul 19, 2026

conformance test report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

shreemaan-abhishek commented Jul 19, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 19, 2026 •

edited

Loading