Skip to content

fix(middleware): reject CSRF TokenLookup that produces no extractors - #3067

Merged
aldas merged 1 commit into
labstack:masterfrom
Reeeid:fix/csrf-tokenlookup-fail-closed
Aug 20, 2026
Merged

fix(middleware): reject CSRF TokenLookup that produces no extractors#3067
aldas merged 1 commit into
labstack:masterfrom
Reeeid:fix/csrf-tokenlookup-fail-closed

Conversation

@Reeeid

@Reeeid Reeeid commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem

If TokenLookup uses a source keyword that isn't recognized, CSRF silently
stops checking tokens.

The known sources are header, query, param, cookie, form. A small typo
like Header:X-CSRF-Token (capital H) or cookies:_csrf (should be cookie)
matches none of them, and there's no default case, so it's just dropped. If
every source in the lookup is like that you end up with zero extractors and no
error. CSRF then loops over an empty list, finds nothing to validate, and lets
the request through. So a typo in the config turns CSRF off without any warning.

KeyAuth already guards against this:

// middleware/key_auth.go
if len(extractors) == 0 {
    return nil, errors.New("echo key-auth middleware could not create extractors from KeyLookup string")
}

CSRF just doesn't have the same check, so this PR adds it.

For context on impact: this only happens with a misconfigured TokenLookup, and
Sec-Fetch-Site still blocks normal cross-site requests. But a security
middleware quietly doing nothing on a typo felt worth turning into an error.

Change

  • CSRFConfig.ToMiddleware() now returns an error when TokenLookup produces no
    extractors (e.g. "echo csrf middleware could not create extractors from TokenLookup string"),
    matching KeyAuth. CSRFWithConfig turns that error into a startup panic.
  • Added a test case.

Valid configs keep working; only a lookup where all sources are unknown is
rejected.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.37%. Comparing base (dcb05f0) to head (fc1661d).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3067      +/-   ##
==========================================
+ Coverage   93.34%   93.37%   +0.03%     
==========================================
  Files          43       44       +1     
  Lines        4735     4787      +52     
==========================================
+ Hits         4420     4470      +50     
- Misses        192      193       +1     
- Partials      123      124       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aldas aldas left a comment

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.

LGTM

@aldas
aldas merged commit 4ec116d into labstack:master Aug 20, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants