mraible commented on code in PR #227:
URL: https://github.com/apache/roller/pull/227#discussion_r4402290354
##########
app/src/main/webapp/WEB-INF/jsps/editor/Pings.jsp:
##########
@@ -69,13 +69,15 @@
<td class="rollertable" align="center" >
<s:if test="pingStatus[#pingTarget.id]">
<s:url var="disableUrl" action="pings!disable" >
+ <s:param name="salt" value="%{salt}" />
Review Comment:
`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.
##########
CHANGES.md:
##########
@@ -58,6 +58,11 @@
- **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
Review Comment:
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.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/ValidateSaltInterceptor.java:
##########
@@ -33,13 +35,23 @@
import com.opensymphony.xwork2.interceptor.AbstractInterceptor;
/**
- * Validates salts after Struts has wrapped a multipart form request.
+ * Validates salts after Struts has wrapped a multipart form request, and on
+ * requests other than POST that call an action method which changes data.
*/
public class ValidateSaltInterceptor extends AbstractInterceptor implements
StrutsStatics {
private static final long serialVersionUID = 2446434402795510394L;
private static final Log log =
LogFactory.getLog(ValidateSaltInterceptor.class);
+ /**
+ * 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(
+ "execute", "input", "view", "search", "query", "edit", "cancel",
Review Comment:
`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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
