OF-3025: Refactor System Properties UI to remove inline scripts and improve table layout by MilanTyagi2004 · Pull Request #3237 · igniterealtime/Openfire

MilanTyagi2004 · 2026-03-28T16:42:58Z

This PR refactors the System Properties administrative page to ensure better Content Security Policy (CSP) compliance and fixes several UI rendering issues.

Changes included:

Removed Inline JavaScript: Extracted inline JS from system-properties.jsp into external files (system-properties.js and list-pager.js) to comply with strict CSP directives without requiring 'unsafe-inline'.
Fixed Data Layout Bugs: Corrected an issue where invisible whitespaces and line-breaks broke the edit/delete buttons, and moved dialog confirmation properties away from the <body> tag so they are no longer stripped out by SiteMesh.
Improved Table UX & CSS: Added overflow-wrap to property names to prevent horizontal scrolling on small screens. For excessively long property values (like complex CSP headers), added text-overflow: ellipsis to restrict the height to a single line, combined with title attributes so users can view the full un-truncated values on hover.

…tyles

MilanTyagi2004 · 2026-03-28T16:49:30Z

hi @guusdk , this is pr is for OF-3025
please review this and tell me if there is any change

thanku!

Copilot

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.jsp into new external scripts (system-properties.js, list-pager.js) and adds a mechanism in ListPager to 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 via onchange="submitForm();"). list-pager.js currently 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 JS change handler for these selects (either in system-properties.js or generically in list-pager.js) that calls ListPager.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.

Copilot · 2026-04-03T19:56:56Z

+<%-- Refactor; disable inline JS in ListPager and use list-pager.js --%>
+<% listPager.setInlineJsDisabled(true); %>


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.

Copilot · 2026-04-03T19:56:57Z

+<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"/>">


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>).

Copilot · 2026-04-03T19:56:57Z

        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)));


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.

Suggested change

: String.format(" onchange='return setPageSize(this.value);'", REQUEST_PARAMETER_KEY_PAGE_SIZE)));

: " onchange='return setPageSize(this.value);'"));

Copilot · 2026-04-03T19:56:57Z

+
+        // 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();
+                        }
+                    }
+                });
+            }
+        });


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.

Suggested change

// 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();

}

}

});

}

});

guusdk · 2026-04-03T19:58:48Z

Oof, this is a very large change set. That makes it hard to review this one.

MilanTyagi2004 · 2026-04-03T20:04:28Z

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.

guusdk

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!

MilanTyagi2004 · 2026-04-06T17:27:48Z

okay Guus,
i will do the changes.

OF-3025: Refactor System Properties UI to remove inline scripts and s…

661ba6b

…tyles

fix cicd

6fab07b

guusdk requested a review from Copilot April 3, 2026 19:50

Copilot started reviewing on behalf of guusdk April 3, 2026 19:51 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

MilanTyagi2004 and others added 2 commits April 4, 2026 21:38

OF-3025 fix change suggest by copilot

0972b39

Merge branch 'main' into OF-3025-Replace-Inline-style

4e1d9b7

This comment was marked as outdated.

Sign in to view

guusdk requested changes Apr 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OF-3025: Refactor System Properties UI to remove inline scripts and improve table layout#3237

OF-3025: Refactor System Properties UI to remove inline scripts and improve table layout#3237
MilanTyagi2004 wants to merge 4 commits intoigniterealtime:mainfrom
MilanTyagi2004:OF-3025-Replace-Inline-style

MilanTyagi2004 commented Mar 28, 2026

Uh oh!

MilanTyagi2004 commented Mar 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

guusdk commented Apr 3, 2026

Uh oh!

MilanTyagi2004 commented Apr 3, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

guusdk left a comment

Uh oh!

MilanTyagi2004 commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		<%-- Refactor; disable inline JS in ListPager and use list-pager.js --%>
		<% listPager.setInlineJsDisabled(true); %>

	: String.format(" onchange='return setPageSize(this.value);'", REQUEST_PARAMETER_KEY_PAGE_SIZE)));
	: " onchange='return setPageSize(this.value);'"));

Uh oh!

Conversation

MilanTyagi2004 commented Mar 28, 2026

Uh oh!

MilanTyagi2004 commented Mar 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

guusdk commented Apr 3, 2026

Uh oh!

MilanTyagi2004 commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

guusdk left a comment

Choose a reason for hiding this comment

Uh oh!

MilanTyagi2004 commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MilanTyagi2004 commented Apr 3, 2026 •

edited

Loading