Removed the admin secret key toggle so admin GET actions are always protected - #1256
Merged
Conversation
…rotected The admin/security/use_form_key toggle didn't trade one CSRF protection for another: turning it off dropped automatic GET protection and replaced it with a hand-maintained list of _setForcedFormKeyActions() / getUrlSecure() pairs that had already drifted out of sync (#1241). Admin urls now always carry the per-action secret key, so POST is always covered by the form key and GET by the secret key, and the whole opt-in list becomes dead code: - preDispatch() validates the form key on POST and the secret key otherwise, with no config branch - deleted _setForcedFormKeyActions(), _checkIsForcedFormKeyAction() and all ~56 controller call sites, most of which were the whole preDispatch override - deleted getUrlSecure()/getUrlSecureParams() and rewired their call sites to plain getUrl(), which now always mints the key - removed the system.xml field, its backend model, the config default and the isEnabledSecurityKeyUrl() helper; useSecretKey() now only reflects the per-url _nosecret opt-out (login flow, RSS) - upgrade script deletes stored admin/security/use_form_key rows - browser tests mint the secret key from the page's FORM_KEY via the new adminPathWithSecretKey() helper instead of disabling the protection Closes #1242
- LINK_GET_PARENTS shared LINK_GET_CHILDREN's value, making the parents branch of fetchLinkInformation() dead code; gave it its own value. The method's only live caller passes LINK_GET_CHILDREN, so behavior is unchanged. - $m[3] is always set in the cms/block directive callback (PHP only truncates trailing unmatched captures), so dropped the ?? on it.
…llback PHPStan's match-shape analysis proves the block_id capture of whichever alternation matched is non-empty, so the === '' branch could never run.
Maho_FeedManager_Model_Notifier builds feedmanager_feed/edit urls with _nosecret for failure emails and admin inbox notifications, but the controller never declared the action public, so the links bounced to the dashboard whenever secret keys were validated. Declaring the render-only edit action public completes the same pairing the RSS admin feeds use.
…posed Review follow-ups to the secret-key work: - Admin OAuth authorize confirm/reject were public GET actions that grant or reject a token from an attacker-controllable oauth_token, a CSRF hole. The admin button block now builds those urls through adminhtml/url so they carry the per-action secret key, and only index/simple stay public. - FeedManager made feedmanager_feed/edit public, but the edit page auto-starts generation from a generate=1 url param, so a cross-site GET could trigger it. The trigger now rides a one-shot session flag armed only by the key/POST protected save and generate actions. - RSS basic-auth polls re-run admin login every request; renewSecretUrls() ran unconditionally and flushed the admin menu cache on every poll. It is skipped again for keyless (RSS) logins. - getSecretKey() takes an optional form key so the browser-test helper reuses the real derivation instead of a copy. - Upgrade script uses deleteConfigData() with an exact path and declares strict_types; _getRequestUri() gains its native return type; trailing blank lines left by the removed preDispatch overrides are cleaned up.
…in review - The save-and-generate session flag now stores the feed id with a timestamp; the edit page consumes it clear-on-read and ignores flags older than 60 seconds, so a lost redirect can no longer start a stale generation on a later, unrelated visit. - login() no longer builds the keyed redirect url that request-less (RSS basic-auth) logins always discard, and its menu-flush guard uses useSecretKey() instead of respelling the same predicate. - preDispatch() tracks one key-validity flag instead of two mutually exclusive ones. - getSecretKey() parses the request path only when controller or action is missing. - The four browser tests share a single adminLoginAndVisit() helper, and the secret key url segment comes from SECRET_KEY_PARAM_NAME instead of a hard-coded 'key'.
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
2 participants
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.
Closes #1242.
The
admin/security/use_form_keytoggle didn't trade one CSRF protection for another: turning it off dropped automatic GET protection and replaced it with a hand-maintained list of_setForcedFormKeyActions()/getUrlSecure()pairs that had already drifted out of sync (#1241). Admin urls now always carry the per-action secret key, so POST is always covered by the form key, GET by the secret key, and the whole opt-in list is deleted.preDispatch()validates the form key on POST and the secret key on everything else, with no config branch.$_publicActionsremains the only exemption._setForcedFormKeyActions(),_checkIsForcedFormKeyAction()and all ~56 controller call sites; most were the controller's entirepreDispatch()override.getUrlSecure()/getUrlSecureParams()and rewired their call sites to plaingetUrl(), which now always mints the key.form_keynever appears in a url again.config.xmldefault andisEnabledSecurityKeyUrl().useSecretKey()now only reflects the per-url_nosecretopt-out (login flow, RSS), and the login-timerenewSecretUrls()/ redirect-policy checks lost their config branches.Mage_Coreupgrade script (2.1.0 → 2.1.1) deletes any storedadmin/security/use_form_keyrows, so installs that had the toggle off pick up the always-on behavior explicitly. Those installs lose bookmarkable admin deep links, which is the accepted cost from the issue.AdminReindexDialogTest,AdminQueryStringUrlsTest,AdminStoreSwitcherTest,AdminRelatedProductsGridTest) used to disable the toggle to deep-link into admin; they now mint the real secret key from the logged-in page'swindow.FORM_KEYvia a sharedadminPathWithSecretKey()helper, so they exercise the same validation real navigation does.AdminUrlFormKeyTestwas rewritten for the always-on behavior.Mage_CatalogIndex_Model_Data_Abstract::LINK_GET_PARENTSfrom 1 to 2: it previously sharedLINK_GET_CHILDREN's value, so any third-party caller passing it got children and now gets parents.Third-party compatibility: the deleted methods (
_setForcedFormKeyActions(),_checkIsForcedFormKeyAction(),getUrlSecure(),getUrlSecureParams(),turnOnSecretKey(),turnOffSecretKey(),isEnabledSecurityKeyUrl()) are removed without deprecation shims. Modules that call them (theturnOffSecretKey()pattern is common in custom RSS/export endpoints inherited from Magento 1) fail with "Call to undefined method" and must migrate to$_publicActions/_nosecret.$_publicActions: the one remaining exemption, documentedWith the toggle gone,
$_publicActionsis the only way an admin GET action skips secret key validation. Public actions are still behind the admin session cookie and ACL; "public" only means "no per-action key required". The rule for using it: the action must be render/read-only (the secret key defends against CSRF, which only matters for state-changing requests), and it exists for links that are generated outside any admin session, where a valid key is impossible in principle (the key derives from the recipient session's form key, unknowable at generation time). Every keyless (_nosecret) link generator is paired with the public action that receives it:Mage_Adminhtml_Sales_OrderControllerview,indexMage_Rss_Block_Order_New) links each order and the gridMage_Adminhtml_Catalog_ProductControllereditMage_Rss_Block_Catalog_NotifyStock) links each product's edit pageMage_Adminhtml_Catalog_Product_ReviewControllereditMage_Rss_Block_Catalog_Review) links each review's edit pageMaho_FeedManager_Adminhtml_Feedmanager_FeedControllereditMaho_FeedManager_Model_NotifierMaho_Ai_Adminhtml_AiControllertaskStatusThe FeedManager entry is a bug fix found while auditing this pairing: its notifier already sent
_nosecretlinks tofeedmanager_feed/edit, but the controller never declared the action public, so those links bounced to the dashboard on every install that validated keys (the default before this PR too). While there, the "Save & Generate" session flag gained a 60-second expiry with clear-on-read, so a lost redirect can no longer start a stale generation on a later visit to the edit page. The login-flow_nosecreturls (login, forgot/reset password) need no entry: validation only runs for logged-in sessions.The OAuth authorize
confirm/rejectactions also left the public list: the in-page flow carries the secret key in the url path, so it keeps working. The one degraded path is a Reject link in a stale logged-out tab after logging in elsewhere, which now redirects to the dashboard instead of rejecting the token; accepted as the cost of closing the CSRF hole.Note
Developed with the help of AI.
As part of our commitment to GenAI transparency, we flag pull requests produced with AI assistance alongside human work. As with every change in Maho, a maintainer reviews and validates it before merge, we never merge purely AI-generated changes. See the GenAI transparency section for details.