OF-3025: Replace inline <style> element in system-properties.jsp#3326
OF-3025: Replace inline <style> element in system-properties.jsp#3326MilanTyagi2004 wants to merge 2 commits into
Conversation
MilanTyagi2004
commented
May 9, 2026
MilanTyagi2004
commented
- Add inlineJsDisabled flag to ListPager to conditionally suppress inline onclick/onchange attributes and pageFunctions output (defaults to false, so all existing pages are unaffected)
- Extract page-specific JavaScript to js/system-properties.js using event delegation instead of inline onclick/onchange handlers
- Extract ListPager client-side pagination logic to js/list-pager.js
- Replace inline <script> block with external script includes
- Use data-* attributes to pass property metadata to JavaScript
- Use c:set/c:out for safely passing localized confirm messages
- Replace scriptlet getHiddenFields() with EL expression
- Improve table layout: use word-wrap for property names, add title tooltips for truncated values, reduce search input sizes
…pliance - Add inlineJsDisabled flag to ListPager to conditionally suppress inline onclick/onchange attributes and pageFunctions output (defaults to false, so all existing pages are unaffected) - Extract page-specific JavaScript to js/system-properties.js using event delegation instead of inline onclick/onchange handlers - Extract ListPager client-side pagination logic to js/list-pager.js - Replace inline <script> block with external script includes - Use data-* attributes to pass property metadata to JavaScript - Use c:set/c:out for safely passing localized confirm messages - Replace scriptlet getHiddenFields() with EL expression - Improve table layout: use word-wrap for property names, add title tooltips for truncated values, reduce search input sizes
📝 WalkthroughWalkthroughThis PR migrates inline JavaScript to external scripts for Content Security Policy compliance. The ListPager Java utility class gains an inlineJsDisabled flag that suppresses onchange and onclick attributes plus script block generation. Two new client-side modules replace the suppressed functionality: list-pager.js handles pagination via event delegation, and system-properties.js replaces page-specific inline handlers for editing and deleting properties. The system-properties.jsp page integrates these changes by disabling ListPager's inline JS, loading the external scripts, removing inline handlers, and using data-* attributes to expose property state instead of passing arguments inline. Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
Hi @guusdk, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xmppserver/src/main/webapp/js/list-pager.js (1)
100-105: 💤 Low value
keyCodeis deprecated; prefere.key.
e.keyCodeis deprecated in favor ofKeyboardEvent.key. While still supported, using the modern API improves readability and future-proofs the code.♻️ Suggested fix
function inputFieldOnKeyDownEventListener(e) { - if (e.keyCode === 13) { + if (e.key === 'Enter') { submitForm(); return false; } }🤖 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/list-pager.js` around lines 100 - 105, Replace the deprecated use of e.keyCode in inputFieldOnKeyDownEventListener with the modern KeyboardEvent.key API: check for e.key === 'Enter' (or e.key === "Enter") instead of e.keyCode === 13, call submitForm() and return false as before; update any related comments or tests that reference keyCode to reflect the new check.xmppserver/src/main/webapp/js/system-properties.js (1)
121-135: ⚡ Quick winAction detection via image
srcis fragile.Identifying edit/encrypt/delete actions by checking for substrings in
src(e.g.,'edit-16x16.gif') is brittle—renaming or replacing icons would silently break functionality. Consider adding explicitdata-action="edit"attributes to the clickable images for more robust identification.♻️ Suggested improvement
In JSP, add data attributes:
<img class="clickable" src="images/edit-16x16.gif" data-action="edit" ...> <img class="clickable" src="images/add-16x16.gif" data-action="encrypt" ...> <img class="clickable" src="images/delete-16x16.gif" data-action="delete" ...>Then in JS:
- var src = target.getAttribute('src') || ''; - if (src.indexOf('edit-16x16.gif') > -1) { + var action = target.getAttribute('data-action'); + if (action === 'edit') { var hidden = target.getAttribute('data-hidden') === 'true'; ... - } else if (src.indexOf('add-16x16.gif') > -1) { + } else if (action === 'encrypt') { doEncrypt(target); - } else if (src.indexOf('delete-16x16.gif') > -1) { + } else if (action === 'delete') { doDelete(target);🤖 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 121 - 135, Replace fragile icon-src checks in the click handler with an explicit data-action attribute: instead of checking target.getAttribute('src').indexOf(...), read target.getAttribute('data-action') and switch on values like "edit", "encrypt", "delete" to call doEdit(target,...), doEncrypt(target) and doDelete(target); keep the existing alt 'Search' check and ListPager.submitForm() behavior unchanged, and update the JSP that renders the images to include data-action="edit|encrypt|delete" so the JS logic (in system-properties.js) reliably finds actions via data-action rather than filename substrings.
🤖 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.
Nitpick comments:
In `@xmppserver/src/main/webapp/js/list-pager.js`:
- Around line 100-105: Replace the deprecated use of e.keyCode in
inputFieldOnKeyDownEventListener with the modern KeyboardEvent.key API: check
for e.key === 'Enter' (or e.key === "Enter") instead of e.keyCode === 13, call
submitForm() and return false as before; update any related comments or tests
that reference keyCode to reflect the new check.
In `@xmppserver/src/main/webapp/js/system-properties.js`:
- Around line 121-135: Replace fragile icon-src checks in the click handler with
an explicit data-action attribute: instead of checking
target.getAttribute('src').indexOf(...), read target.getAttribute('data-action')
and switch on values like "edit", "encrypt", "delete" to call
doEdit(target,...), doEncrypt(target) and doDelete(target); keep the existing
alt 'Search' check and ListPager.submitForm() behavior unchanged, and update the
JSP that renders the images to include data-action="edit|encrypt|delete" so the
JS logic (in system-properties.js) reliably finds actions via data-action rather
than filename substrings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 289b21b6-2b23-47c2-a4ba-bc1ece99d5f1
📒 Files selected for processing (4)
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.jsp
Fishbowler
left a comment
Fishbowler
left a comment
There was a problem hiding this comment.
Some initial comments. I haven't done any testing yet.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xmppserver/src/main/webapp/system-properties.jsp (1)
34-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the remaining
<style>block out of the JSP.If OF-3025 is meant to make this page CSP-safe, this inline CSS still forces
style-srcto allow inline content (or require a hash/nonce), so the page is not fully externalized yet. Moving these rules into an existing stylesheet would finish the job cleanly.🤖 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 34 - 58, The inline <style> block in system-properties.jsp (containing .nameColumn, .valueColumn, .hidden, img.clickable rules) breaks CSP by requiring inline styles; move these CSS rules into an external stylesheet (e.g., the existing site stylesheet used by the page), remove the <style> block from system-properties.jsp, and ensure the page includes that stylesheet (or add the selectors to an existing CSS file) so the classes used by system-properties.js and the JSP are styled without inline CSS.
🤖 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/system-properties.jsp`:
- Around line 110-125: Several input/option/title attributes embed <c:out> with
nested double quotes causing invalid quoting; update each occurrence (e.g., the
inputs with id="searchName" and id="searchValue", the plugin value, the
search.description input, and title attributes referencing property.valueAsSaved
and property.defaultDisplayValue) so the inner ${...} expression uses single
quotes inside the c:out value attribute (change value="<c:out value="${...}"/>
to value="<c:out value='${...}'/>" and similarly for title attributes), ensuring
all occurrences match the existing safe pattern used by the data-* attributes.
---
Outside diff comments:
In `@xmppserver/src/main/webapp/system-properties.jsp`:
- Around line 34-58: The inline <style> block in system-properties.jsp
(containing .nameColumn, .valueColumn, .hidden, img.clickable rules) breaks CSP
by requiring inline styles; move these CSS rules into an external stylesheet
(e.g., the existing site stylesheet used by the page), remove the <style> block
from system-properties.jsp, and ensure the page includes that stylesheet (or add
the selectors to an existing CSS file) so the classes used by
system-properties.js and the JSP are styled without inline CSS.
🪄 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: 644274dd-b189-4d62-ba96-9f3256bf2553
📒 Files selected for processing (4)
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.jsp
🚧 Files skipped from review as they are similar to previous changes (2)
- xmppserver/src/main/webapp/js/list-pager.js
- xmppserver/src/main/webapp/js/system-properties.js
| <input type="search" | ||
| id="searchName" | ||
| size="40" | ||
| size="20" | ||
| value="<c:out value="${search.name}"/>"/> | ||
| <img src="images/search-16x16.png" | ||
| width="16" height="16" | ||
| class="clickable" | ||
| alt="Search" title="Search" | ||
| style="vertical-align: middle;" | ||
| onclick="submitForm();" | ||
| > | ||
| </td> | ||
| <td nowrap> | ||
| <input type="search" | ||
| id="searchValue" | ||
| size="40" | ||
| size="20" | ||
| value="<c:out value="${search.value}"/>"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="xmppserver/src/main/webapp/system-properties.jsp"
sed -n '108,130p' "$file"
rg -n '="<c:out value="\$\{[^"]+\}"\s*/>"' "$file"Repository: igniterealtime/Openfire
Length of output: 1652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="xmppserver/src/main/webapp/system-properties.jsp"
echo "=== Searching for data-* attributes ==="
rg -n 'data-[a-z-]+=' "$file" | head -20
echo -e "\n=== Context around lines 113 and 125 ==="
sed -n '110,130p' "$file"
echo -e "\n=== Context around line 146 ==="
sed -n '144,148p' "$file"
echo -e "\n=== All occurrences of c:out with nested quotes ==="
rg -n '<c:out value=' "$file" | head -20Repository: igniterealtime/Openfire
Length of output: 3896
Fix all nested <c:out> quoting patterns in input, option, and title attributes.
The file has nested double-quoted expressions in multiple locations that need correction. Data-* attributes in this file (lines 261–263, 374–375) already use the safer single-quoted pattern: `data-attr="<c:out value='${...}'/>". Apply the same pattern to all occurrences:
Lines to fix
- Line 113:
value="<c:out value="${search.name}"/>"/>→value="<c:out value='${search.name}'/>"/> - Line 125:
value="<c:out value="${search.value}"/>"/>→value="<c:out value='${search.value}'/>"/> - Line 146:
value="<c:out value="${plugin}"/>"→value="<c:out value='${plugin}'/>" - Line 154:
value="<c:out value="${search.description}"/>"/>→value="<c:out value='${search.description}'/>"/> - Line 210:
title="<c:out value="${property.valueAsSaved}"/>"→title="<c:out value='${property.valueAsSaved}'/>" - Line 226:
title="<c:out value="${property.defaultDisplayValue}"/>"→title="<c:out value='${property.defaultDisplayValue}'/>"
Expected result: rg -n '="<c:out value="\$\{[^"]+\}"\s*/>"' xmppserver/src/main/webapp/system-properties.jsp returns no matches.
🧰 Tools
🪛 PMD (7.24.0)
[High] 113-113: Parse Error: ParseException: Parse exception in file 'xmppserver/src/main/webapp/system-properties.jsp' at line 113, 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 110 - 125,
Several input/option/title attributes embed <c:out> with nested double quotes
causing invalid quoting; update each occurrence (e.g., the inputs with
id="searchName" and id="searchValue", the plugin value, the search.description
input, and title attributes referencing property.valueAsSaved and
property.defaultDisplayValue) so the inner ${...} expression uses single quotes
inside the c:out value attribute (change value="<c:out value="${...}"/> to
value="<c:out value='${...}'/>" and similarly for title attributes), ensuring
all occurrences match the existing safe pattern used by the data-* attributes.
Improve readability of ListPager HTML generation
MilanTyagi2004
force-pushed
the
OF-3025-clean
branch
from
6fb0098 to
145db90
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
xmppserver/src/main/webapp/system-properties.jsp (1)
110-154:⚠️ Potential issue | 🟠 MajorFix the remaining nested
<c:out>attribute quoting.These attributes still nest double quotes inside double-quoted HTML attributes, so the JSP remains invalid and the PMD parse error on Line 113 is still reproducible. Use the same single-quoted
value='${...}'pattern already used by thedata-*attributes.Also applies to: 210-226
🤖 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 110 - 154, The JSP has nested double quotes inside HTML attributes for multiple inputs/selects (see elements with id="searchName", id="searchValue", id="searchPlugin", id="searchDescription" and the <option> values inside the <c:forEach>), causing invalid JSP and PMD parse errors; update those value and option attributes to use the single-quoted pattern (e.g. value='${...}' or value='${plugin}') or otherwise remove nested <c:out> double quotes so the expressions are quoted with single quotes, and ensure <option> values (including the <c:forEach> generated options) follow the same quoting style; keep the existing logic (selected checks with <c:if>) unchanged.
🤖 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.
Duplicate comments:
In `@xmppserver/src/main/webapp/system-properties.jsp`:
- Around line 110-154: The JSP has nested double quotes inside HTML attributes
for multiple inputs/selects (see elements with id="searchName",
id="searchValue", id="searchPlugin", id="searchDescription" and the <option>
values inside the <c:forEach>), causing invalid JSP and PMD parse errors; update
those value and option attributes to use the single-quoted pattern (e.g.
value='${...}' or value='${plugin}') or otherwise remove nested <c:out> double
quotes so the expressions are quoted with single quotes, and ensure <option>
values (including the <c:forEach> generated options) follow the same quoting
style; keep the existing logic (selected checks with <c:if>) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd4ec85f-19bd-4596-8059-1ef7ffd6514a
📒 Files selected for processing (4)
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.jsp
🚧 Files skipped from review as they are similar to previous changes (2)
- xmppserver/src/main/webapp/js/system-properties.js
- xmppserver/src/main/java/org/jivesoftware/util/ListPager.java
|
hi @Fishbowler |
2 participants
