Skip to content

perf(csrf,redirect): share scheme/host matching and skip url.Parse on the hot path - #4449

Merged
gaby merged 2 commits into
mainfrom
perf/csrf-origin-no-urlparse
Jun 24, 2026
Merged

perf(csrf,redirect): share scheme/host matching and skip url.Parse on the hot path#4449
gaby merged 2 commits into
mainfrom
perf/csrf-origin-no-urlparse

Conversation

@ReneWerner87

Copy link
Copy Markdown
Member

Summary

schemeAndHostMatch + normalizeSchemeHost were duplicated verbatim in two
security-relevant spots:

  • the CSRF middleware (Origin/Referer validation), and
  • the core redirect logic (Redirect.Back's open-redirect prevention).

Each called url.Parse twice per request only to split host and port. This PR
extracts the shared logic into a new internal/schemehost package and adds a
fast path that handles clean <host> / <host>:<port> values directly,
avoiding the url.Parse allocation. Anything unusual (userinfo, path,
percent-encoding, bracketed IPv6, control characters, empty/invalid port) falls
back to the unchanged url.Parse path.

The security-critical top-level url.Parse(origin) / url.Parse(referer)
(input validation -> ErrOriginInvalid) is untouched.

Equivalence / safety

  • Differential test (Test_normalizeSchemeHost_matchesReference): the fast
    path is compared against a copy of the previous url.Parse-based
    implementation across a broad, adversarial corpus.
  • Fuzz target (FuzzNormalizeSchemeHost): 6M+ executions, zero divergence.
  • Explicit contract table (Test_normalizeSchemeHost) and Test_Match cover
    the documented behavior. Full CSRF suite and root redirect tests pass.

Benchmarks

benchstat, count=6, Apple M2 Pro:

Benchmark allocs/op B/op ns/op
schemehost.Match 6 -> 2 (-67%) 368 -> 32 -78%
normalizeSchemeHost/noport 3 -> 1 (-67%) 184 -> 16 -80%
normalizeSchemeHost/port 2 -> 0 (-100%) 168 -> 0 -88%

The optimization now benefits both the CSRF unsafe-request path and
Redirect.Back.

Notes

normalizeOrigin / subdomain are also duplicated (between CSRF and CORS) but
have diverged in their validation rules, so they are intentionally left in
place; unifying them is a separate change.

🤖 Generated with Claude Code

… the hot path

schemeAndHostMatch + normalizeSchemeHost were duplicated verbatim in the CSRF
middleware (Origin/Referer validation) and the core redirect logic (Back's
open-redirect prevention), each calling url.Parse twice per request only to
split host and port.

Extract them into a shared internal/schemehost package and add a fast path that
handles clean "<host>" / "<host>:<port>" values directly, avoiding url.Parse.
Anything unusual (userinfo, path, percent-encoding, bracketed IPv6, control
chars, empty/invalid port) falls back to the unchanged url.Parse path. The
security-critical top-level url.Parse(origin)/url.Parse(referer) is untouched.

Equivalence is guarded by a differential test against a copy of the previous
implementation and a fuzz target (FuzzNormalizeSchemeHost, 6M+ execs, zero
divergence). normalizeOrigin/subdomain (which have diverged between csrf and
cors) are intentionally left in place.

Benchmarks (benchstat, count=6, Apple M2 Pro):
  schemehost.Match              6 -> 2 allocs, 368 -> 32 B, -78% ns
  normalizeSchemeHost/noport    3 -> 1 allocs, 184 -> 16 B, -80% ns
  normalizeSchemeHost/port      2 -> 0 allocs, 168 -> 0 B,  -88% ns

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2b04e88b-ad7a-40f8-849b-4dcaef41de50

📥 Commits

Reviewing files that changed from the base of the PR and between 1a6182f and 4a98d98.

📒 Files selected for processing (6)
  • internal/schemehost/schemehost.go
  • internal/schemehost/schemehost_test.go
  • middleware/csrf/csrf.go
  • middleware/csrf/helpers.go
  • middleware/csrf/helpers_test.go
  • redirect.go
💤 Files with no reviewable changes (2)
  • middleware/csrf/helpers_test.go
  • middleware/csrf/helpers.go

Walkthrough

A new internal/schemehost package is introduced with an exported Match function that normalizes scheme and host (including default port handling and IPv6 bracketing) for same-origin comparison. Duplicate implementations of this logic previously residing in middleware/csrf/helpers.go and redirect.go are removed and replaced with calls to schemehost.Match.

Changes

Shared schemehost package and migration

Layer / File(s) Summary
internal/schemehost implementation
internal/schemehost/schemehost.go
New package with exported Match, normalizeSchemeHost (fast-path + url.Parse fallback), classifyHostPort character validator, allDigits, and normalizeSchemeHostViaParse for bracketed IPv6 and default-port canonicalization.
internal/schemehost tests, benchmarks, and fuzz
internal/schemehost/schemehost_test.go
Adds a url.Parse-based reference normalizer, adversarial host corpus, table-driven tests for normalizeSchemeHost and Match, benchmarks, and a fuzz target that checks the fast-path against the reference across http, https, ftp, and empty schemes.
CSRF middleware migrated to schemehost.Match
middleware/csrf/csrf.go, middleware/csrf/helpers.go, middleware/csrf/helpers_test.go
Imports schemehost in csrf.go; replaces originMatchesHost and refererMatchesHost same-origin checks with schemehost.Match; removes schemeAndHostMatch and normalizeSchemeHost helpers and their tests.
Redirect.Back migrated to schemehost.Match
redirect.go
Imports schemehost; replaces schemeAndHostMatch call in Redirect.Back with schemehost.Match; removes local schemeAndHostMatch and normalizeRedirectSchemeHost functions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • gofiber/fiber#3904: Directly related — both PRs modify CSRF origin/referer scheme+host matching to handle case normalization and implicit default ports, with this PR replacing the logic introduced there via the new shared internal/schemehost.Match.
  • gofiber/fiber#4204: Related — both modify refererMatchesHost in middleware/csrf/csrf.go; #4204 strips path/query before trusted-origin checks while this PR replaces the scheme+host normalization used in the same function.

Suggested labels

codex

Suggested reviewers

  • efectn
  • sixcolors
  • gaby

Poem

🐇 Hop, hop, I found a mess — three helpers, all the same!
I bundled scheme and host matching under one clear name.
The CSRF guard and redirect path now share a single call,
Default ports, IPv6 brackets — handled once for all.
Less code to sniff, more carrots left — a tidy warren hall! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the shared scheme/host matching optimization across CSRF and redirect hot paths.
Description check ✅ Passed The description covers purpose, safety, and benchmarks, though it omits the issue reference and several optional template sections.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 perf/csrf-origin-no-urlparse

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.59%. Comparing base (db4c533) to head (e3d2259).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4449      +/-   ##
==========================================
+ Coverage   91.58%   91.59%   +0.01%     
==========================================
  Files         134      134              
  Lines       13548    13494      -54     
==========================================
- Hits        12408    12360      -48     
+ Misses        725      723       -2     
+ Partials      415      411       -4     
Flag Coverage Δ
unittests 91.59% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

github-actions[bot]

This comment was marked as outdated.

@gaby
gaby merged commit ad44335 into main Jun 24, 2026
20 checks passed
@gaby
gaby deleted the perf/csrf-origin-no-urlparse branch June 24, 2026 13:50
@github-project-automation github-project-automation Bot moved this to Done in v3 Jun 24, 2026
@ReneWerner87 ReneWerner87 modified the milestones: v3, v3.4.0 Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants