snoopdave commented on code in PR #154:
URL: https://github.com/apache/roller/pull/154#discussion_r3779149400
##########
it-selenium/logs/roller-2026-08-11-165858.log.gz:
##########
Review Comment:
Please remove all of these logs from the PR
##########
app/src/main/resources/struts.xml:
##########
@@ -17,10 +17,26 @@
directory of this distribution.
-->
<!DOCTYPE struts PUBLIC
- "-//Apache Software Foundation//DTD Struts Configuration 2.5//EN"
- "http://struts.apache.org/dtds/struts-2.5.dtd";>
+ "-//Apache Software Foundation//DTD Struts Configuration 6.5//EN"
+ "https://struts.apache.org/dtds/struts-6.5.dtd";>
<struts>
+ <!-- Struts 7 denies OGNL access to any class outside this allowlist, which
+ otherwise blocks reads of Roller's own action and bean properties. -->
+ <constant name="struts.allowlist.packageNames"
+ value="org.apache.roller,java.util,java.lang"/>
+
+ <!-- Struts serves its bundled JS/CSS (tooltips, etc.) from this path,
which
+ must match the /struts/* filter-mapping in web.xml. Struts defaults it
+ to /static, which Roller does not map. -->
+ <constant name="struts.ui.staticContentPath" value="/struts"/>
+
+ <!-- Struts 7 only binds request parameters to properties annotated with
+ @StrutsParameter. Roller's actions predate that annotation, so without
+ this no form in the application can submit data. Annotating the action
+ beans would be the stricter alternative. -->
+ <constant name="struts.parameters.requireAnnotations" value="false"/>
Review Comment:
AI review flagged this change.
The alternative to disabling this is adding annotations to each form input
field? I wonder 1. how much of a security risk this is and 2. how much work we
would have to do to add annotations to every input field.
##########
app/src/main/webapp/WEB-INF/web.xml:
##########
@@ -1,8 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
-<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee";
+<web-app xmlns="https://jakarta.ee/xml/ns/jakartaee";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
- xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee
http://xmlns.jcp.org/xml/ns/javaee/web-app_4_0.xsd";
- version="4.0">
+ xsi:schemaLocation="https://jakarta.ee/xml/ns/jakartaee
https://jakarta.ee/xml/ns/jakartaee/web-app_6_0.xsd";
+ version="6.0">
Review Comment:
Should this be 6.1 instead of 6.0?
##########
app/src/test/resources/roller-jettyrun.properties:
##########
@@ -49,3 +50,13 @@ cache.sitewide.enabled=false
cache.weblogpage.enabled=false
cache.weblogfeed.enabled=false
cache.planet.enabled=false
+
+# Authentication defaults to Roller's own user database, which needs no extra
+# services. To develop against OIDC instead, start Keycloak with
Review Comment:
not critical, but should OIDC stuff live in the stacked PR 155 that follows
this?
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/util/UIAction.java:
##########
@@ -88,13 +87,14 @@ public abstract class UIAction extends ActionSupport
public void myPrepare() {
// no-op
}
-
- @Override
- public void setRequest(Map<String, Object> map) {
- this.salt = (String) map.get("salt");
- }
public String getSalt() {
+ if (salt == null) {
+ HttpServletRequest request =
ActionContext.getContext().getServletRequest();
Review Comment:
Direct static ActionContext lookups add thread-local and lifecycle coupling
and make these actions harder to unit test. Struts 7 provides the replacement
interfaces under org.apache.struts2.action—ServletRequestAware,
ServletResponseAware, and ParametersAware—just as this PR correctly uses the
new upload-aware API. Please use those injected APIs for this and the other
nine new ActionContext.getContext() call sites.
Problem exists at these locations as well:
app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroupSubs.java:78
app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetGroups.java:55
app/src/main/java/org/apache/roller/weblogger/planet/ui/PlanetConfig.java:84
app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalConfig.java:102,162
app/src/main/java/org/apache/roller/weblogger/ui/struts2/admin/GlobalCommentManagement.java:247
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/Members.java:61
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/FolderEdit.java:83
app/src/main/java/org/apache/roller/weblogger/ui/struts2/core/Register.java:438
##########
.gitignore:
##########
@@ -28,3 +28,4 @@ docker/postgresql-data
docker/roller-data
assembly-release/release.sh
it-selenium/overlays/
+logs/
Review Comment:
do we also need to ignore `docker/postgresql-16-data`?
--
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]
