Require POST for action methods that change data by snoopdave · Pull Request #227 · apache/roller

snoopdave · 2026-10-27T21:13:25Z

Action methods that change data now need a POST:

A request other than a POST that calls an action method other than a display method (execute, input, view, search, query, edit, cancel and a few redirect targets) gets 405 Method Not Allowed. Every POST still needs the salt.
The links that changed data by GET (ping enable, disable and send, global ping target enable and disable, accepting or declining a weblog invitation, and the installer's bootstrap link) are now POST forms, and return to the page afterwards.
Saving a common ping target needs a POST.

Tests: ValidateSaltInterceptorTest and PingTargetEditTest. Full build and tests pass on JDK 11. On a fresh Jetty install, a GET to each of those URLs got 405, and the forms bootstrapped Roller, enabled pings and accepted an invitation; ping now was not tried.

Links that change data (ping settings, invitation replies) now carry the salt.

snoopdave

🤖 GLM: PR-Review — 1 inline comment posted.

snoopdave · 2026-10-27T21:24:10Z

+     * Action methods that only display data. Links and redirects may reach
+     * these without a salt; any other method needs a POST or a salt.
+     */
+    static final Set<String> DISPLAY_METHODS = Set.of(


Important: The fresh-install click-through breaks with this change: CreateDatabase.jsp:36 and UpgradeDatabase.jsp:54 render installer.tryBootstrapping as a plain GET link (<a href="{0}"> around <s:url action="install!bootstrap"/>), bootstrap is not in DISPLAY_METHODS, and the link carries no salt — so completing a fresh installation by clicking that link now throws. Same class of miss as the menu/pings links this PR fixes. Either add salt params to those two s:urls (the pages render a salt) or route the installer flow through POST.

🤖Claude: fixed by adding the salt to the install!bootstrap URL in CreateDatabase.jsp and UpgradeDatabase.jsp. Checked on a fresh Jetty install: token, create tables, then the bootstrap link completes setup and lands on the front page (6e13c0c).

snoopdave · 2026-10-27T21:24:11Z

🤖 GLM: PR-Review — General Issues

The following issues were found but cannot be attached to a specific line in the diff:

Important: CHANGES.md has no entry for this change. Other PRs in the 6.1.7 batch each carry one. Worth a note: URL-reachable action methods that change data now require POST or a valid salt, and the affected GET links (main-menu invitation accept/decline, ping enable/disable/ping-now) now carry per-page salt parameters — so bookmarks to those URLs stop working after upgrading.

Strengths (not action items): the allowlist approach with tests for all three paths (GET without salt rejected, GET with salt accepted, display methods exempt) is a clean class-level fix rather than a per-action patch, and firstSave / activate genuinely are display/entry-point methods, so their presence in the exempt set checks out against their implementations.

…NGES.md

snoopdave · 2026-10-27T21:59:28Z

🤖Claude: fixed by adding a CHANGES.md entry under 6.1.7 behaviour changes (6e13c0c).

mraible

Salting the GET links is a good fix. One data-changing path still gets through, though, and the refreshed pages run into a consumed salt. Details are inline.

mraible · 2026-10-28T15:07:08Z

+     * these without a salt; any other method needs a POST or a salt.
+     */
+    static final Set<String> DISPLAY_METHODS = Set.of(
+            "execute", "input", "view", "search", "query", "edit", "cancel",


PingTargetEdit.execute() validates, saves and flushes a ping target, and commonPingTargetAdd/commonPingTargetEdit allow only execute. That means a GET like commonPingTargetAdd.rol?bean.name=x&bean.pingUrl=... still creates or repoints a ping target without a salt. Spring CSRF is disabled and ValidateSaltFilter only checks POST, so nothing else catches it. Could the save move to a save method, or could those two actions require POST?

🤖Claude: fixed by having PingTargetEdit take the request method (like GlobalConfig) and save only on a POST. Any other method returns the input result without saving. The modal already saves by AJAX POST, so the page works as before. New PingTargetEditTest checks that a GET does not save or flush and that a POST does; the GET case fails without the change. Also checked on Jetty: a GET to commonPingTargetAdd.rol with bean parameters adds nothing, and the modal POST still adds the target. A sweep of the other exempt methods found no others that save. (Local change, in the next push.)

mraible · 2026-10-28T15:07:08Z

    <td class="rollertable" align="center" >
        <s:if test="pingStatus[#pingTarget.id]">
            <s:url var="disableUrl" action="pings!disable" >
+                <s:param name="salt" value="%{salt}" />


pings!enable, disable and pingNow render the list at the salted URL, and the salt is single-use, so refreshing the page (or using a second link from the same page in another tab) shows "Security Violation". Redirecting to pings after the change would keep the used salt out of the address bar. The same applies to the PingTargets and MainMenu links.

🤖Claude: fixed by returning a new done result from pings!enable/disable/pingNow, commonPingTargets!enable/disable and menu!accept/decline. The result redirects to the plain page (pings.rol?weblog=…, commonPingTargets.rol, menu.rol), and those three actions add the Struts store interceptor in AUTOMATIC mode, so the ping result and the invite messages survive the redirect and show once. Checked on Jetty: each link returns a 302 without the salt in Location, the message appears after the redirect, and a refresh shows the page with no error. While checking this I found that the invite links double-escaped & (s:url and then s:property), so inviteId never arrived; they now use s:a href="%{…}" like the pings pages. Opening a second link from the same, already-used page still needs a reload, because every link on a page shares one salt. (Local change, in the next push.)

mraible · 2026-10-28T15:07:08Z

 - **Startup fails if the XML-RPC parser cannot be configured.** Roller used to
  log an error and continue. It now stops at startup, so check the log if a
  custom XML parser is on the classpath.
+- **Links and URLs that change data need the form token.** A request other


This says every non-POST request that changes data needs the salt, but because execute is exempt, the ping target add/edit GET above still doesn't need one. Either close that path or narrow this entry to the links the PR covers.

🤖Claude: fixed by closing the ping target path (see the PingTargetEdit reply), so the entry holds as written. I also added that saving a common ping target needs a POST, and that the links return to the page without the salt in the address. (Local change, in the next push.)

…ix invite links Claude-Session: https://claude.ai/code/session_01C5X1Z1eoSH7o25gZskPGzb

mraible

Saving a common ping target now needs a POST, the modal still saves over AJAX POST, and the link actions redirect so a refresh doesn't resend a used salt. LGTM.

snoopdave · 2026-10-29T21:25:00Z

GET should be safe, so I'd rather not accept a salt on GET. Can we make these actions POST-only instead?

Put @HttpPost on the methods that change data: Pings.enable/disable/pingNow, PingTargets.enable/disable, MainMenu.accept/decline, Install.bootstrap and PingTargetEdit.save. Add the httpMethod interceptor to rollerStack. This also replaces the ServletRequestAware check in PingTargetEdit.
Change those links to one-button POST forms with <s:hidden name="salt"/>. ValidateSaltFilter already checks the salt on every POST.
Remove the GET branch and DISPLAY_METHODS from ValidateSaltInterceptor. A list based on method names will drift. It already lists activate, which enables an account. That one is fine only because the activation code works as a secret, so make it an explicit exception.
Keep the redirect-after-POST with store.

This keeps the salt out of URLs, logs and Referer, and link prefetchers can't trigger changes. Please also check that @HttpPost works with our !method URLs.

A request other than a POST to an action method outside the display list now gets 405 Method Not Allowed. The ping, common ping target and invitation links are one-button POST forms, and the installer's bootstrap link submits a POST form. Every POST still needs the salt, which ValidateSaltFilter checks.

snoopdave · 2026-10-30T02:08:00Z

🤖Claude: fixed, with one change from the plan. Struts 2.5.29 (our 6.1.x version) has no httpMethod interceptor or @HttpPost; they arrived in Struts 6. So ValidateSaltInterceptor now does it: a request other than a POST to a method outside DISPLAY_METHODS gets 405 Method Not Allowed (with Allow: POST), salt or not. I kept the list rather than an opt-in annotation, so a method nobody marked is still refused on GET; its comment now names activate as the one GET that changes data, guarded by its one-time code.

Ping enable/disable/ping now, common ping target enable/disable, and invitation accept/decline are one-button POST forms with the page salt. They still redirect back to the page afterwards (done + store).
The installer message keeps its link (it is in 5 translations); the link now submits a POST form, with a plain button inside <noscript>.
ValidateSaltFilter checks the salt on every POST, as before. PingTargetEdit keeps its POST check, since execute is on the display list.
ValidateSaltInterceptorTest: GET refused with and without a salt, POST invoked. Full app tests pass (423).

Checked on a fresh Jetty install: GET to install!bootstrap, pings!enable, commonPingTargets!disable and menu!accept each got 405; the POST forms bootstrapped Roller, enabled a weblog and a common ping target, and accepted an invitation, each redirecting back. Ping now was not tried (it calls an external server). (817d290)

…hods # Conflicts: # CHANGES.md

Require POST or a salt for action methods that change data

5db71ca

Links that change data (ping settings, invitation replies) now carry the salt.

snoopdave added the 6.1.7 label Oct 27, 2026

snoopdave commented Oct 27, 2026

View reviewed changes

Add the salt to the installer bootstrap links; note the change in CHA…

6e13c0c

…NGES.md

mraible reviewed Oct 28, 2026

View reviewed changes

Save common ping targets only on POST; redirect after link actions; f…

0e7c8d3

…ix invite links Claude-Session: https://claude.ai/code/session_01C5X1Z1eoSH7o25gZskPGzb

mraible approved these changes Oct 29, 2026

View reviewed changes

snoopdave changed the title ~~Require POST or a salt for action methods that change data~~ Require POST for action methods that change data Oct 30, 2026

snoopdave added 2 commits October 29, 2026 22:24

Merge remote-tracking branch 'origin/roller-6.1.x' into ui-action-met…

740dd30

…hods # Conflicts: # CHANGES.md

Merge remote-tracking branch 'origin/roller-6.1.x' into ui-action-met…

3eb9e66

…hods # Conflicts: # CHANGES.md

snoopdave merged commit 8263661 into roller-6.1.x Oct 30, 2026
6 checks passed

Conversation

snoopdave commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snoopdave left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snoopdave commented Oct 27, 2026

Uh oh!

snoopdave commented Oct 27, 2026

Uh oh!

mraible left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mraible left a comment

Choose a reason for hiding this comment

Uh oh!

snoopdave commented Oct 29, 2026

Uh oh!

snoopdave commented Oct 30, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snoopdave commented Oct 27, 2026 •

edited

Loading