security: CSRF protection, POST enforcement, cookie allowlist#95
Open
rhoerr wants to merge 5 commits intovpietri:mainfrom
Open
security: CSRF protection, POST enforcement, cookie allowlist#95rhoerr wants to merge 5 commits intovpietri:mainfrom
rhoerr wants to merge 5 commits intovpietri:mainfrom
Conversation
- Implement CsrfAwareActionInterface on all 5 action controllers - Add HttpPostActionInterface to restrict to POST method - Allowlist cookie names in Cookie controller (C1) - Remove devadmin config key from ConfigUpdate (C3) - Fix exception catches to use \Throwable (H9) - Update toolbar JS to send POST with FORM_KEY Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix unreachable else branch by adding explicit $file existence check - Fix typo: "reseted" → "reset" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The comment explains WHY the Cache-Control header is set (to control varnish, fastly, and built-in php page caches), which is not obvious from the code alone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Both validateForCsrf() and createCsrfValidationException() returned null, which is identical to the default behavior for any action that does NOT implement CsrfAwareActionInterface. HttpPostActionInterface alone is sufficient — it restricts routes to POST, and the framework's CsrfValidator applies standard form_key validation to all POST actions by default. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Curly/smart quotes (U+2018, U+2019) were introduced in the catch handler string. Replace with straight ASCII quotes for consistency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
CsrfAwareActionInterfaceon all 5 action controllers (Cache, CacheCss, ConfigUpdate, Cookie, Log/Reset)HttpPostActionInterfaceto restrict state-changing actions to POST method onlyqdb_db_profiler,qdb_db_profiler_backtrace, andqdb_appearanceare permitted (C1)devadminconfig key from ConfigUpdate controller — this allowed disabling admin password lifetime/enforcement from the toolbar (C3)\Throwableinstead of unqualifiedException(H9)toolbar.phtml) to send POST requests withFORM_KEYfor CSRF protectionSecurity Findings Addressed
Exceptioncatches missErrorsubclassesTest plan
form_keyare rejecteddevadminconfig option is no longer available🤖 Generated with Claude Code