#184 Change CSRF implementation to default to using SessionStorage instead of CacheStorage to remove 'page expired' errors and make extensible. #238
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… of CacheStorage to remove 'page expired' errors and make extensible.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors CBWIRE's CSRF protection to use a custom, extensible implementation based on cbstorages instead of relying on the cbcsrf module. The change addresses the "Page Expired" errors that occurred with cbcsrf's cache-based approach by defaulting to session-based storage, while maintaining flexibility for distributed deployments. The implementation introduces an interface-based design that allows users to choose between session storage (default, OWASP-recommended) or cache storage (for clusters), or even create custom implementations.
Key Changes:
- Introduced
ICSRFStorageinterface and two built-in implementations:SessionCSRFStorage(default) andCacheCSRFStorage - Refactored
TokenServiceto use lazy-loaded storage implementations via the new interface - Added
csrfEnabledandcsrfServicesettings to ModuleConfig for user configuration - Replaced
cbcsrfdependency withcbstoragesin box.json
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| models/interfaces/ICSRFStorage.cfc | Defines the contract for CSRF storage implementations with 5 core methods (set, get, exists, delete, clear) |
| models/services/csrf/SessionCSRFStorage.cfc | Session-based storage implementation using cbstorages sessionStorage - the new default |
| models/services/csrf/CacheCSRFStorage.cfc | Cache-based storage implementation for distributed systems that don't require sessions |
| models/services/TokenService.cfc | Refactored to delegate storage to configurable ICSRFStorage implementations via lazy loading |
| models/CBWIREController.cfc | Updated to use TokenService instead of cbcsrf, with conditional CSRF verification based on csrfEnabled setting |
| ModuleConfig.cfc | Added csrfEnabled and csrfService configuration settings with detailed documentation |
| box.json | Updated dependencies from cbcsrf to cbstorages, including install path changes |
| test-harness/tests/specs/unit/services/csrf/SessionCSRFStorageSpec.cfc | Comprehensive test suite for SessionCSRFStorage (6 tests) |
| test-harness/tests/specs/unit/services/csrf/CacheCSRFStorageSpec.cfc | Comprehensive test suite for CacheCSRFStorage (6 tests) |
| test-harness/tests/specs/unit/services/TokenServiceSpec.cfc | Enhanced test suite for TokenService with 7 tests covering generation, verification, and rotation |
| test-harness/layouts/Main.cfm | Minor cleanup removing version number from header text |
| .claude/settings.local.json | Added Claude AI configuration file for development tooling permissions |
Contributor
|
@grantcopley I've opened a new pull request, #250, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: grantcopley <1197835+grantcopley@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
@grantcopley I've opened a new pull request, #251, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: grantcopley <1197835+grantcopley@users.noreply.github.com>
Co-authored-by: grantcopley <1197835+grantcopley@users.noreply.github.com>
Co-authored-by: grantcopley <1197835+grantcopley@users.noreply.github.com>
Co-authored-by: grantcopley <1197835+grantcopley@users.noreply.github.com>
Add test coverage for CSRF-disabled scenarios
Use getApplicationMetadata() for cross-platform session detection in TokenService
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Files Created
- Defines the contract for CSRF storage implementations
- 5 methods: set(), get(), exists(), delete(), clear()
models/services/csrf/SessionCSRFStorage.cfc
- Default OWASP-recommended storage
- Uses sessionStorage@cbstorages
- Requires sessions enabled
models/services/csrf/CacheCSRFStorage.cfc
- For distributed/clustered systems
- Uses cacheStorage@cbstorages
- No session requirement (uses cookie/URL fallback)
- test-harness/tests/specs/unit/services/csrf/SessionCSRFS
torageSpec.cfc
- test-harness/tests/specs/unit/services/csrf/CacheCSRFSto
rageSpec.cfc
Files Modified
- Added csrfEnabled: true setting
- Added csrfService: "SessionCSRFStorage@cbwire" setting
- Refactored to use lazy-loaded ICSRFStorage
implementation
- Maintains same public API (backward compatible)
- Conditional CSRF verification based on csrfEnabled
- generateCSRFToken() returns empty string when disabled
- Added cleanup before each test
- Complete documentation of new architecture
- Configuration examples
- Custom implementation guide
Test Results
✅ All 305 tests passing
Key Features
✅ Interface-based design - Clean, extensible architecture
✅ Two built-in implementations - Session (default) and
Cache (distributed)
✅ User-configurable - Simple module settings
✅ User-extensible - Can implement custom storage (Redis,
DynamoDB, etc.)
✅ OWASP-compliant only - No insecure cookie-based storage
✅ Backward compatible - No breaking changes
✅ Zero-config default - Works out-of-box with
SessionCSRFStorage
✅ Comprehensive documentation - Full guide with examples
Configuration Example
moduleSettings = {
"cbwire": {
"csrfEnabled": true,
"csrfService": "SessionCSRFStorage@cbwire" // or
CacheCSRFStorage@cbwire
}
};
The implementation solves the "Page Expired" issue while providing flexibility for different deployment scenarios!