-
Notifications
You must be signed in to change notification settings - Fork 0
Backend PR-2: CSRF Token Endpoint & Security Hardening #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add SecurityHeaders middleware for X-Frame-Options, X-Content-Type-Options, X-XSS-Protection, Referrer-Policy - Enable HSTS in production environment only - Register SecurityHeaders middleware globally for all requests - Verify CSRF token endpoint /sanctum/csrf-cookie accessibility - Confirm session cookies configured as httpOnly with sameSite=lax - Add 8 comprehensive CSRF protection tests - Add 6 comprehensive security headers tests - Update CHANGELOG.md with httpOnly cookie authentication features Part of: Epic httpOnly Cookie Authentication Migration (frontend#208) Fixes: #210
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements CSRF token endpoint and security hardening for httpOnly cookie-based SPA authentication. It adds a SecurityHeaders middleware to apply security headers globally, registers it in the application bootstrap, and includes tests for both security headers and CSRF protection functionality. However, the PR has several critical bugs in the test suite that prevent it from meeting its stated quality goals.
Key Changes:
- SecurityHeaders middleware with X-Frame-Options, X-Content-Type-Options, X-XSS-Protection, Referrer-Policy, and HSTS (production-only) headers
- Global middleware registration in bootstrap/app.php
- Test suites for security headers and CSRF protection
Critical Issues:
- Multiple tests use non-existent routes (
/v1/up,/v1/user) that will cause test failures - CSRF protection tests only verify configuration, not actual protection behavior
- CHANGELOG claims comprehensive test coverage despite these bugs
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/Http/Middleware/SecurityHeaders.php | Implements security headers middleware with appropriate headers for different environments |
| bootstrap/app.php | Registers SecurityHeaders middleware globally to cover all routes including Sanctum endpoints |
| tests/Feature/Middleware/SecurityHeadersTest.php | Tests security headers on various routes, but contains bug with non-existent /v1/up route |
| tests/Feature/Auth/CsrfProtectionTest.php | Tests CSRF configuration and cookie attributes, but contains bugs with non-existent routes and inadequate behavioral testing |
| CHANGELOG.md | Documents the feature addition, but overstates test comprehensiveness given the bugs in the test suite |
- Fix /v1/up -> /health in SecurityHeadersTest (actual health endpoint) - Fix /v1/user -> /v1/me in CsrfProtectionTest (actual authenticated user endpoint) - Clarify comment: security headers apply to API and Sanctum routes - All 14 tests passing after fixes Addresses Copilot review comments #1, #2, #3
Summary
Implements CSRF token endpoint and security hardening for httpOnly cookie-based SPA authentication. This PR configures the backend infrastructure needed for the frontend to migrate from localStorage tokens to secure httpOnly cookies.
Related Issues
Fixes #210
Part of: SecPal/frontend#208 (Epic: httpOnly Cookie Authentication Migration)
Changes
Security Headers Middleware (
app/Http/Middleware/SecurityHeaders.php)DENY(prevent clickjacking)nosniff(prevent MIME sniffing)1; mode=block(legacy browser XSS protection)strict-origin-when-cross-originmax-age=31536000; includeSubDomains(production only)Registered globally in
bootstrap/app.phpto apply to all routes including/sanctum/*.Session Configuration
httpOnly=true(prevents JavaScript access)sameSite=lax(CSRF protection)SESSION_SECURE_COOKIEenv var for HTTPS enforcementCSRF Protection
/sanctum/csrf-cookieendpoint verified accessibleTesting
tests/Feature/Auth/CsrfProtectionTest.php)tests/Feature/Middleware/SecurityHeadersTest.php)Quality Gates
✅ TDD: Tests written first, all passing
✅ PHPStan: No errors
✅ Pint: Code style compliant
✅ Test Coverage: 14 new tests, 34 auth tests total
✅ CHANGELOG: Updated
✅ 4-Pass Review: Completed 2x
✅ Preflight Script: All checks passed (466 tests)
Implementation Details
Why Global Middleware Registration?
Security headers middleware registered globally using
$middleware->append()instead of only on API routes because:/sanctum/csrf-cookieis a web route (not API route)Session Configuration Notes
Session configuration was completed in Backend PR-1 (#209):
.env.examplealready has all required variablesconfig/session.phpalready configured correctlyCSRF Token Flow
GET /sanctum/csrf-cookieXSRF-TOKENcookie (readable by JS)X-XSRF-TOKENheader in POST/PUT/PATCH/DELETEDependencies
Depends on:
Blocks:
Next Steps
After this PR merges:
Testing Instructions
References