OF-3025: Refactor System Properties UI to remove inline scripts and improve table layout#3237
Conversation
|
hi @guusdk , this is pr is for OF-3025 thanku! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the admin “System Properties” page to improve CSP compliance by removing inline JavaScript, while also addressing table/layout issues for long property names/values and improving pagination plumbing.
Changes:
- Extracts inline JS from
system-properties.jspinto new external scripts (system-properties.js,list-pager.js) and adds a mechanism inListPagerto disable inline JS generation. - Adjusts System Properties table rendering (wrapping/ellipsis, tooltips for truncated values) and fixes action icon handling via
data-*attributes. - Tweaks a flaky test timeout for outgoing server session initialization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java | Increases session initialize-timeout to reduce test flakiness. |
| xmppserver/src/main/webapp/system-properties.jsp | Removes inline handlers, adds external scripts, adjusts table CSS/markup, and refactors pagination form. |
| xmppserver/src/main/webapp/js/system-properties.js | Implements click handling for edit/encrypt/delete/search and edit form submit actions. |
| xmppserver/src/main/webapp/js/list-pager.js | Adds CSP-friendly ListPager navigation logic (pagination + page size + form submission). |
| xmppserver/src/main/java/org/jivesoftware/util/ListPager.java | Adds inlineJsDisabled flag and alters generated HTML/JS to support CSP-friendly operation. |
Comments suppressed due to low confidence (1)
xmppserver/src/main/webapp/system-properties.jsp:137
- The filter
<select>controls (e.g.searchDefaultValue) no longer auto-submit when their value changes (previously done viaonchange="submitForm();").list-pager.jscurrently only auto-submits on Enter and when an input becomes empty, so changing a select to a non-empty value won’t refresh results unless the user clicks the search icon. Add a JSchangehandler for these selects (either insystem-properties.jsor generically inlist-pager.js) that callsListPager.submitForm().
<select id="searchDefaultValue">
<option value="" <c:if test="${search.defaultValue == ''}">selected</c:if>><fmt:message key="server.properties.search_all"/></option>
<option value="unchanged" <c:if test="${search.defaultValue == 'unchanged'}">selected</c:if>><fmt:message key="server.properties.default.search_unchanged"/></option>
<option value="changed" <c:if test="${search.defaultValue == 'changed'}">selected</c:if>><fmt:message key="server.properties.default.search_changed"/></option>
<option value="unknown" <c:if test="${search.defaultValue == 'unknown'}">selected</c:if>><fmt:message key="server.properties.default.unknown"/></option>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <%-- Refactor; disable inline JS in ListPager and use list-pager.js --%> | ||
| <% listPager.setInlineJsDisabled(true); %> |
There was a problem hiding this comment.
listPager.setInlineJsDisabled(true) is executed after ${listPager.pageSizeSelection} / ${listPager.pageLinks} have already been rendered. As a result, those HTML fragments will still include inline onchange/onclick handlers, which strict CSP will block, and which also prevents list-pager.js from taking over (it skips elements that already have onchange/onclick). Move the setInlineJsDisabled(true) call to before any use of pageSizeSelection/pageLinks (or set this flag in the servlet before forwarding) so that generated markup contains no inline handlers.
| <form method="post" id="actionForm" | ||
| data-encrypt-confirm="<fmt:message key="server.properties.encrypt_confirm"/>" | ||
| data-delete-confirm="<fmt:message key="server.properties.delete_confirm"/>"> |
There was a problem hiding this comment.
The confirmation messages are injected into HTML attributes using <fmt:message .../> without escaping. If a translation ever contains characters like " or &, it can break the attribute value (or, depending on content, create injection risk). Prefer storing the message in a variable and outputting it with XML/HTML escaping (e.g., via <c:out>).
| final StringBuilder sb = new StringBuilder(); | ||
| sb.append(String.format("<select name='%s' onchange='return setPageSize(this.value);'>", REQUEST_PARAMETER_KEY_PAGE_SIZE)); | ||
| sb.append(String.format("<select name='%s'%s>", REQUEST_PARAMETER_KEY_PAGE_SIZE, inlineJsDisabled ? "" | ||
| : String.format(" onchange='return setPageSize(this.value);'", REQUEST_PARAMETER_KEY_PAGE_SIZE))); |
There was a problem hiding this comment.
This String.format call passes an extra argument that is never used (the format string has no placeholders). It’s harmless but confusing; remove the unused argument to make it clear what is being formatted.
| : String.format(" onchange='return setPageSize(this.value);'", REQUEST_PARAMETER_KEY_PAGE_SIZE))); | |
| : " onchange='return setPageSize(this.value);'")); |
|
|
||
| // Global keydown for Enter on search inputs | ||
| const searchInputs = ["searchName", "searchValue", "searchDescription"]; | ||
| searchInputs.forEach(id => { | ||
| const input = document.getElementById(id); | ||
| if (input) { | ||
| input.addEventListener('keydown', function(e) { | ||
| if (e.keyCode === 13) { | ||
| if (typeof ListPager !== 'undefined') { | ||
| ListPager.submitForm(); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
list-pager.js already binds Enter-to-submit behavior for all data-additional-form-fields (including searchName, searchValue, searchDescription). Adding another keydown listener here duplicates submissions and can trigger multiple form.submit() calls for a single Enter key press. Consider removing this block and relying on list-pager.js, or call e.preventDefault()/e.stopPropagation() to ensure only one submit is triggered.
| // Global keydown for Enter on search inputs | |
| const searchInputs = ["searchName", "searchValue", "searchDescription"]; | |
| searchInputs.forEach(id => { | |
| const input = document.getElementById(id); | |
| if (input) { | |
| input.addEventListener('keydown', function(e) { | |
| if (e.keyCode === 13) { | |
| if (typeof ListPager !== 'undefined') { | |
| ListPager.submitForm(); | |
| } | |
| } | |
| }); | |
| } | |
| }); |
|
Oof, this is a very large change set. That makes it hard to review this one. |
sorry Guus, i know that large changes are headache to review, as you know UI changes are generally large. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
guusdk
left a comment
guusdk
left a comment
There was a problem hiding this comment.
Thank you for the effort, but I am still going to ask you to split this in separate PRs. The PR description now says that this is fixing three different issues. Please make those three different PRs, so that they can be evaluated one-by-one.
This PR is making changes to ListPager. That is used on many different pages, not just the one that is being changed in this PR. Have you verified that these changes do not break other pages?
I'm not sure why the change in LocalOutgoingServerSessionTest is included. Does that relate to this PR?
The PR contains a couple of commits that need to be fixed - they are not very concise. Perhaps you should squash some of them (but also split things up over different PR's, as suggested above).
The current PR can no longer be merged, probably because other PRs got merged that change the base of Openfire that this was created on. Please re-base on the latest HEAD of the Main branch and address any merge conflicts!
|
okay Guus, |
|
@coderabbitai |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refactors the system-properties page to extract inline JavaScript into external modules and adds configurable inline JS disabling to the ListPager utility. The ListPager class gains an Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xmppserver/src/main/webapp/js/system-properties.js`:
- Around line 94-109: The submitActionForm function uses bracket-access like
form["action"], which risks DOM clobbering; change all accesses to the form
controls to use the form.elements collection (e.g., form.elements["action"],
form.elements["key"], form.elements["value"], form.elements["encrypt"]) when
setting .value and .disabled so the inputs are referenced via
HTMLFormControlsCollection rather than as named properties on the form element.
In `@xmppserver/src/main/webapp/system-properties.jsp`:
- Around line 262-264: The data-null-value attribute is computed from
property.displayValue but must reflect whether the stored value is absent
(property.valueAsSaved); change the data-null-value expression to use
property.valueAsSaved (e.g. data-null-value="<c:out
value='${property.valueAsSaved == null}'/>") so doEdit() sees true when no value
is stored and avoids preloading the "none" placeholder; keep the other data-*
attributes unchanged.
- Around line 111-114: The JSP uses nested JSTL tags inside HTML attributes
(e.g., the input element with id="searchName" uses value="<c:out
value="${search.name}"/>"), which breaks compilation; fix by replacing JSTL tag
usage inside attributes with direct EL (e.g., value="${search.name}"), and for
any <fmt:message> used inside attributes, pre-bind the message with <fmt:message
key="..." var="someVar"/> then reference it as value="${fn:escapeXml(someVar)}";
also add the functions taglib declaration (xmlns or <%@ taglib prefix="fn"
uri="http://java.sun.com/jsp/jstl/functions" %>) so you can call fn:escapeXml()
to safely escape attribute content.
In
`@xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java`:
- Around line 241-243: Remove the redundant immediate overwrite of the
initialise-timeout in the test setup: in LocalOutgoingServerSessionTest (look
for the JiveGlobals.setProperty calls setting
"xmpp.server.session.initialise-timeout"), delete the first call that sets it to
5 so only the intended 10 timeout assignment remains; this clarifies test intent
and avoids dead code in the test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39b42310-ea9a-401b-a9e4-12879ce83775
📒 Files selected for processing (5)
xmppserver/src/main/java/org/jivesoftware/util/ListPager.javaxmppserver/src/main/webapp/js/list-pager.jsxmppserver/src/main/webapp/js/system-properties.jsxmppserver/src/main/webapp/system-properties.jspxmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java
| function submitActionForm(action, key, value, encrypt) { | ||
| const form = document.getElementById("actionForm"); | ||
| form["action"].value = action; | ||
| form["key"].value = key; | ||
| if (typeof value !== "undefined") { | ||
| form["value"].value = value; | ||
| form["value"].disabled = false; | ||
| } else { | ||
| form["value"].disabled = true; | ||
| } | ||
| if (typeof encrypt !== "undefined") { | ||
| form["encrypt"].value = encrypt; | ||
| form["encrypt"].disabled = false; | ||
| } else { | ||
| form["encrypt"].disabled = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the HTML DOM, when a form contains <input name="action">, is form["action"] guaranteed to return that input, or can it resolve to HTMLFormElement.action? What is the standards-compliant way to access the control?
💡 Result:
Not guaranteed: if your form contains a named control with name="action", then form['action'] / form.action will resolve to (or be overwritten by) that control, not to HTMLFormElement.action. MDN explicitly notes that named inputs “can overwrite native properties if they share the same name (e.g., a form with an input named action will have its action property return that input instead of the form's action HTML attribute)” [1]. The HTML standard also describes the named-property lookup behavior that maps name/id to elements (“To determine the value of a named property name for a form element…” returning candidates based on matching id/name) [2], and warns about DOM clobbering as a general hazard of using names that match built-in properties [3]. Standards-compliant way to access what you want: 1) If you mean the form’s action URL attribute: use the dedicated IDL attribute/property on HTMLFormElement: form.action (but avoid name/id clashes by not naming controls “action”) [4][5]. 2) If you mean the specific control : access it through the form controls collection, not via the clobberable named-property access. MDN recommends using form.elements to avoid ambiguity between an element name and a form property [6], and the HTMLFormElement.elements collection allows you to access a particular control by its name/id [7]. Example: const inputAction = form.elements['action']; Practical security/robustness guidance: don’t use input names that collide with built-in form properties (action, method, elements, etc.). The HTML standard calls this out as a DOM clobbering source of security issues [3].
Citations:
- 1: https://wiki.developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement
- 2: https://html.spec.whatwg.org/multipage/forms.html
- 3: https://html.spec.whatwg.org/dev/form-control-infrastructure.html
- 4: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/action
- 5: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/form
- 6: https://dev.mozilla.org/en-US/docs/Web/API/HTMLFormElement
- 7: https://developer.mozilla.org/docs/Web/API/HTMLFormElement/elements
🏁 Script executed:
# Search for the actionForm element definition in HTML files
find . -name "*.html" -o -name "*.jsp" | head -20Repository: igniterealtime/Openfire
Length of output: 1042
🏁 Script executed:
# More specifically, search for actionForm in the file that likely contains the form
rg "actionForm" --type html --type jsp -A 20 -B 2Repository: igniterealtime/Openfire
Length of output: 95
🏁 Script executed:
# Also check if there's a JSP file in the webapp directory that might define this form
fd "system-properties" --type fRepository: igniterealtime/Openfire
Length of output: 167
🏁 Script executed:
cat -n xmppserver/src/main/webapp/system-properties.jsp | head -200Repository: igniterealtime/Openfire
Length of output: 11177
🏁 Script executed:
# Search for actionForm specifically in the JSP file
rg "actionForm" xmppserver/src/main/webapp/system-properties.jsp -A 30 -B 5Repository: igniterealtime/Openfire
Length of output: 671
Use form.elements[...] to access hidden form controls.
The hidden input fields (action, key, value, encrypt) are properly accessible via form["action"] due to DOM named property resolution. However, this pattern causes DOM clobbering—the named inputs shadow the HTMLFormElement's native properties. For clarity and standards compliance, use form.elements["action"] and similar consistently.
Suggested fix
function submitActionForm(action, key, value, encrypt) {
const form = document.getElementById("actionForm");
- form["action"].value = action;
- form["key"].value = key;
+ form.elements["action"].value = action;
+ form.elements["key"].value = key;
if (typeof value !== "undefined") {
- form["value"].value = value;
- form["value"].disabled = false;
+ form.elements["value"].value = value;
+ form.elements["value"].disabled = false;
} else {
- form["value"].disabled = true;
+ form.elements["value"].disabled = true;
}
if (typeof encrypt !== "undefined") {
- form["encrypt"].value = encrypt;
- form["encrypt"].disabled = false;
+ form.elements["encrypt"].value = encrypt;
+ form.elements["encrypt"].disabled = false;
} else {
- form["encrypt"].disabled = true;
+ form.elements["encrypt"].disabled = true;
}
form.submit();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/webapp/js/system-properties.js` around lines 94 - 109,
The submitActionForm function uses bracket-access like form["action"], which
risks DOM clobbering; change all accesses to the form controls to use the
form.elements collection (e.g., form.elements["action"], form.elements["key"],
form.elements["value"], form.elements["encrypt"]) when setting .value and
.disabled so the inputs are referenced via HTMLFormControlsCollection rather
than as named properties on the form element.
| <input type="search" | ||
| id="searchName" | ||
| size="40" | ||
| size="20" | ||
| value="<c:out value="${search.name}"/>"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Finds the same invalid pattern elsewhere in this JSP.
rg -n '="\s*<(c:out|fmt:message)\b' xmppserver/src/main/webapp/system-properties.jspRepository: igniterealtime/Openfire
Length of output: 2517
🏁 Script executed:
#!/bin/bash
# Check for existing taglib imports in the JSP file
head -30 xmppserver/src/main/webapp/system-properties.jsp | grep -n "taglib"Repository: igniterealtime/Openfire
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Check all taglib imports in the JSP file
grep -n "taglib" xmppserver/src/main/webapp/system-properties.jspRepository: igniterealtime/Openfire
Length of output: 204
Fix the invalid JSP tag nesting inside HTML attributes.
Lines 114, 126, 147, 155, and approximately 17 additional occurrences contain the pattern value="<c:out value="${search.name}"/>", which is not valid JSP syntax. JSTL tags cannot be nested inside quoted HTML attribute values; this prevents the JSP from compiling.
Use EL expressions directly in attribute values instead: value="${search.name}". For <fmt:message> invocations, pre-bind the message to a variable using <fmt:message key="..." var="varName"/> first, then reference it with EL. Add the functions taglib to enable fn:escapeXml() for proper attribute escaping.
Example fix pattern
+<%@ taglib prefix="fn" uri="http://java.sun.com/jsp/jstl/functions" %>
...
- value="<c:out value="${search.name}"/>"/>
+ value="${fn:escapeXml(search.name)}"/>+<fmt:message key="global.save_property" var="savePropertyLabel"/>
- <input type="button" id="savePropertyBtn" value="<fmt:message key="global.save_property" />">
+ <input type="button" id="savePropertyBtn" value="${fn:escapeXml(savePropertyLabel)}">🧰 Tools
🪛 PMD (7.24.0)
[High] 114-114: Parse Error: ParseException: Parse exception in file 'xmppserver/src/main/webapp/system-properties.jsp' at line 114, column 59: Encountered <IN_TAG_ERROR: """>.
Was expecting one of:
<ATTR_NAME> ...
">" ...
"/>" ...
"=" ...
(Parse Error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/webapp/system-properties.jsp` around lines 111 - 114, The
JSP uses nested JSTL tags inside HTML attributes (e.g., the input element with
id="searchName" uses value="<c:out value="${search.name}"/>"), which breaks
compilation; fix by replacing JSTL tag usage inside attributes with direct EL
(e.g., value="${search.name}"), and for any <fmt:message> used inside
attributes, pre-bind the message with <fmt:message key="..." var="someVar"/>
then reference it as value="${fn:escapeXml(someVar)}"; also add the functions
taglib declaration (xmlns or <%@ taglib prefix="fn"
uri="http://java.sun.com/jsp/jstl/functions" %>) so you can call fn:escapeXml()
to safely escape attribute content.
| data-hidden="<c:out value='${property.hidden}'/>" | ||
| data-encrypted="<c:out value='${property.encrypted}'/>" | ||
| data-null-value="<c:out value='${property.displayValue == null}'/>" |
There was a problem hiding this comment.
Derive data-null-value from valueAsSaved, not displayValue.
The table treats property.valueAsSaved == null as the “no stored value” case, but this flag is computed from property.displayValue == null. When an unset property renders as none, doEdit() will treat that placeholder as real data and preload "none" into the textarea.
Suggested fix
- data-null-value="<c:out value='${property.displayValue == null}'/>"
+ data-null-value="${property.valueAsSaved == null}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xmppserver/src/main/webapp/system-properties.jsp` around lines 262 - 264, The
data-null-value attribute is computed from property.displayValue but must
reflect whether the stored value is absent (property.valueAsSaved); change the
data-null-value expression to use property.valueAsSaved (e.g.
data-null-value="<c:out value='${property.valueAsSaved == null}'/>") so doEdit()
sees true when no value is stored and avoids preloading the "none" placeholder;
keep the other data-* attributes unchanged.
| JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(5)); | ||
| // Avoid CI jitter causing intermittent setup timeouts. | ||
| JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(10)); |
There was a problem hiding this comment.
Remove the immediately-overwritten timeout assignment.
Line 241 sets xmpp.server.session.initialise-timeout to 5, but Line 243 overwrites it with 10 before any intervening logic. The first write is effectively dead and makes test intent unclear.
Suggested change
- JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(5));
// Avoid CI jitter causing intermittent setup timeouts.
JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(10));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(5)); | |
| // Avoid CI jitter causing intermittent setup timeouts. | |
| JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(10)); | |
| // Avoid CI jitter causing intermittent setup timeouts. | |
| JiveGlobals.setProperty("xmpp.server.session.initialise-timeout", Long.toString(10)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java`
around lines 241 - 243, Remove the redundant immediate overwrite of the
initialise-timeout in the test setup: in LocalOutgoingServerSessionTest (look
for the JiveGlobals.setProperty calls setting
"xmpp.server.session.initialise-timeout"), delete the first call that sets it to
5 so only the intended 10 timeout assignment remains; this clarifies test intent
and avoids dead code in the test setup.
This PR refactors the System Properties administrative page to ensure better Content Security Policy (CSP) compliance and fixes several UI rendering issues.
Changes included:
system-properties.jspinto external files (system-properties.jsandlist-pager.js) to comply with strict CSP directives without requiring'unsafe-inline'.<body>tag so they are no longer stripped out by SiteMesh.overflow-wrapto property names to prevent horizontal scrolling on small screens. For excessively long property values (like complex CSP headers), addedtext-overflow: ellipsisto restrict the height to a single line, combined withtitleattributes so users can view the full un-truncated values on hover.