Skip to content

Add CSRF protection via Rack::Protection::AuthenticityToken#444

Open
mattimustang wants to merge 1 commit into
ytti:masterfrom
mattimustang:fix/csrf-protection
Open

Add CSRF protection via Rack::Protection::AuthenticityToken#444
mattimustang wants to merge 1 commit into
ytti:masterfrom
mattimustang:fix/csrf-protection

Conversation

@mattimustang

Copy link
Copy Markdown

Summary

  • Add Rack::Session::Cookie (SameSite=Strict, HttpOnly) to establish the session required for CSRF token storage
  • Add Rack::Protection::AuthenticityToken to validate tokens on all non-safe requests (POST, PUT, DELETE, PATCH); skipped in the test environment so existing rack/test suites are unaffected without needing to be rewritten
  • Add authenticity_token hidden input to the conf_search form in layout.haml (present on every page) and to the version-diff comparison form in diffs.haml
  • Add three regression tests in spec/web/csrf_spec.rb: CSRF token present in conf_search form, CSRF token present in diffs form, session cookie carries SameSite=Strict

Fixes CWE-352 / CVSSv4.0 6.3 — CSRF on all state-changing endpoints.

Why Rack::Session::Cookie is always on (including test)

Rack::Protection::AuthenticityToken stores and retrieves the CSRF token via the Rack session (session[:csrf]). Without a session middleware the template call Rack::Protection::AuthenticityToken.token(session) would crash on a nil session. The session middleware must be present whenever a page is rendered, including in tests.

Why Rack::Protection::AuthenticityToken is skipped in test

The validation middleware is the piece that returns 403 for requests without a valid token. Every existing POST/PUT test (put '/node/next/...', etc.) was written before CSRF protection existed and sends no token. Enabling validation in test would break the entire suite. The correctness of the validation logic is rack-protection's own responsibility (covered by its own tests); what we verify here is our side of the contract — that forms embed the token and the cookie carries the right attributes.

Notes on GET state-changing endpoints

/reload and GET /node/next/:node are state-changing GET routes that cannot be protected by form tokens. The SameSite=Strict session cookie provides defence-in-depth for same-site-restricted environments, but full protection would require converting these to POST endpoints (a separate, larger change).

Test plan

  • Run rake test — 42 runs, 0 failures
  • Visit /nodes — confirm the conf_search form submits successfully (token round-trips correctly)
  • Visit a diff page — confirm the "Get Diffs!" form submits successfully
  • Confirm browser DevTools shows rack.session cookie with SameSite=Strict and HttpOnly

Sinatra::Base subclasses do not enable Rack::Protection by default.
This commit adds explicit CSRF protection for all state-changing endpoints.

- Add Rack::Session::Cookie (SameSite=Strict, HttpOnly) to establish the
  session needed for CSRF token storage
- Add Rack::Protection::AuthenticityToken to validate tokens on all
  non-safe requests (POST, PUT, DELETE, PATCH); skipped in the test
  environment so existing rack/test suites are unaffected
- Add authenticity_token hidden input to the conf_search form in
  layout.haml (present on every page) and to the version-diff form in
  diffs.haml
- Add three regression tests: CSRF token present in conf_search form,
  CSRF token present in diffs form, session cookie carries SameSite=Strict

Fixes CWE-352 / CVSSv4.0 6.3 (CSRF on state-changing endpoints).
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.

1 participant