Fix/#3167 fixed escaping fields #3204
Conversation
…dEvents.jsp, eventHandlers.jsp, mailingLists.jsp, emailRecipients.js, common.js)
- Created universal converter XssBeanConverter; - Change annotation name from AllowHtml to NoEscape; - Corrected escaped pointlinks;
…xed_escaping_fields
- Created class XssBeanConverterUtils; - Added test: XssBeanConverterUtilsTest and class: TestGetExtendedName for test; - Moved NoEscape and XssBeanConverter to org.scada_lts.web.dwr.security; - Added converted RecipientListEntryBean and IntValuePair in dwr.xml;
Java Script Mocha Unit Test Results268 tests 268 ✅ 3s ⏱️ Results for commit 8f90b7d. ♻️ This comment has been updated with latest results. |
Java JUnit Test Results3 476 tests 3 476 ✅ 52s ⏱️ Results for commit 8f90b7d. ♻️ This comment has been updated with latest results. |
| @JsonRemoteProperty | ||
| private String name; | ||
| @JsonRemoteProperty | ||
| @JsonRemoteProperty @NoEscape |
There was a problem hiding this comment.
I added this annotation here, but it probably shouldn't be here, because it's a structure representing js scripts executed on the server side, using the rhino engine, it's not a script executed on the client/browser side.
…xed_escaping_fields
- Fixed XssProtectUtils.unescapeNewLineAndWhitespace, XssProtectUtils.escapeNewLineAndWhitespace
…scaping_fields_in_mailing_lists_maintenance_events_point_links_publishers
…nt links, publishers
… into fix/#3206_Fixed_escaping_fields_in_Event_handlers_Users_profiles_watch_lists
…nt links, publishers
…, watch lists" This reverts commit 371ad44.
…lists: - Fixed double escaped alias in: MaintenanceEventVO, ScheduledEventVO (use NoEscape annotation); - Fixed escaped after save event handler;
…lists: - Removed use NoEscape annotation in: MaintenanceEventVO.alias, ScheduledEventVO.alias - Unescaped in getDescription string fields from: MaintenanceEventVO, ScheduledEventVO because returned LocalizableMessage object escaped by converter; - Corrected maintenanceEvents.jsp, scheduledEvents.jsp use unescapeHtml;
…lists: - Corrected: MaintenanceEventVO.getDescription, ScheduledEventVO.getDescription;
…ds_in_Event_handlers_Users_profiles_watch_lists #3206 Fixed escaping fields in Event handlers, Users profiles, watch lists
…scaping_fields_in_mailing_lists_maintenance_events_point_links_publishers
…nt links, publishers: - Added convert definitions for UserEntryJson, AddressEntryJson; - Fixed escape add address;
… into fix/#3197_CVE-2021-26829_Mitigation_Guidance_System_settings
- Corrected default path;
…gation_Guidance_System_settings #3197 CVE-2021-26829 Mitigation Guidance [System settings]
…nt links, publishers
… into fix/#3209_Fixed_escaping_fields_in_mailing_lists_maintenance_events_point_links_publishers
…nt links, publishers: - Corrected all publisher types;
…ds_in_mailing_lists_maintenance_events_point_links_publishers #3209 Fixed escaping fields in mailing lists, maintenance events, point links, publishers
…nt links, publishers: - Corrected editPachube.jsp, editPersistent.jsp;
…ds_in_mailing_lists_maintenance_events_point_links_publishers #3209 Fixed escaping fields in mailing lists, maintenance events, poi…
- Fixed displaying of Link component as well as scripts on scripts list in button (script) component in graphical views - Fixed displaying script names in scripting view
| } | ||
|
|
||
| public String createLinkContent() { | ||
| String safeHref = escapeHtml(link == null ? "" : link); |
There was a problem hiding this comment.
String escapedHref = link == null ? "" : escapeHtml(link);
|
|
||
| public String createLinkContent() { | ||
| String safeHref = escapeHtml(link == null ? "" : link); | ||
| String safeText = escapeHtml(text == null ? "" : text); |
There was a problem hiding this comment.
String escapedText = text == null ? "" : escapeHtml(text);
| private void createLink() { | ||
| String content = createLinkContent(); | ||
| setContent(content); | ||
| setContent(createLinkContent()); |
There was a problem hiding this comment.
What is the reason for this change?
I would go back to how it was, if there is no need to change something, then we probably won't touch it.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request implements comprehensive XSS (Cross-Site Scripting) protection across the application by introducing a centralized DWR converter system, security utilities for escaping/unescaping HTML entities, new Vue.js components for safe text handling, and updates to JSP views to properly display previously escaped content. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser/Vue
participant DWRConv as DWR Converter<br/>(XssBeanConverter)
participant Utils as XssBeanConverterUtils
participant Backend as Backend Service
participant DB as Database
rect rgb(200, 220, 255)
note over Client,DB: Outbound Data Flow (Server → Client)
Backend->>DWRConv: convertOutbound(DataObject)
DWRConv->>Utils: convertObjectEscaped(DataObject)
Utils->>Utils: Recursively escape all<br/>string fields
Utils-->>DWRConv: Escaped DataObject
DWRConv-->>Client: JSON with escaped HTML
Client->>Client: unescapeHtml/unescapeVueHtml<br/>for display
end
rect rgb(220, 255, 220)
note over Client,DB: Inbound Data Flow (Client → Server)
Client->>DWRConv: submitForm(JSON data)
DWRConv->>Utils: convertObjectUnescaped(JSONData)
Utils->>Utils: Recursively unescape<br/>string fields
Utils-->>DWRConv: Unescaped DataObject
DWRConv-->>Backend: convertInbound(DataObject)
Backend->>DB: store(DataObject)
DB-->>Backend: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The review requires understanding of the new reflection-based Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/org/scada_lts/dao/cache/UsersProfileDaoWithCache.java (2)
24-26: Remove redundant.map(UsersProfileVO::new)call.Since
selectProfilesnow creates freshUsersProfileVOinstances (lines 42-45), the additional.map(UsersProfileVO::new)on line 25 creates a redundant second copy of each object.♻️ Remove the redundant mapping
public Optional<UsersProfileVO> selectProfileById(int profileId) { - return selectProfiles(0, Integer.MAX_VALUE).stream().filter(a -> a.getId() == profileId).map(UsersProfileVO::new).findAny(); + return selectProfiles(0, Integer.MAX_VALUE).stream().filter(a -> a.getId() == profileId).findAny(); }
28-31: Remove redundant.map(UsersProfileVO::new)call.Similar to
selectProfileById, this method now performs double-wrapping sinceselectProfilesalready creates fresh instances.♻️ Remove the redundant mapping
public Optional<UsersProfileVO> selectProfileByXid(String profileXid) { - return selectProfiles(0, Integer.MAX_VALUE).stream().filter(a -> a.getXid().equals(profileXid)).map(UsersProfileVO::new).findAny(); + return selectProfiles(0, Integer.MAX_VALUE).stream().filter(a -> a.getXid().equals(profileXid)).findAny(); }WebContent/WEB-INF/jsp/pointEdit/pointProperties.jsp (1)
52-68: Adddescriptionfield to DataPointVO DWR converter include list for consistency.The
namefield (line 52) is included in the DataPointVO XssBean converter configuration (dwr.xmlline 160), so usinghtmlEscape="false"correctly prevents double-escaping when Spring processes the bound value.However, the
descriptionfield (line 62) is not in the DWR converter include list. This means the backend XssBeanConverter never escapes this field, makinghtmlEscape="false"unnecessary and creating an inconsistent escaping approach between the two fields. Both fields should either:
- Be included in the converter's include list with
htmlEscape="false"to prevent double-escaping, or- Be excluded from the converter and rely solely on
<c:out>for escapingSince
nameis already included in the converter, adddescriptionto maintain uniform backend escaping of all user-facing text fields.src/org/scada_lts/mango/service/SystemSettingsService.java (1)
633-657: Fix null/empty handling beforevalidatePath(...)to avoid NPE + preserve “clear to default” behaviorBoth
saveResourceGraphicsPathMiscandsaveResourceUploadsPathMisccan throw onnullinput (validation runs before the null check). Also, empty string now always fails early, even though the subsequent branch suggests it should be allowed.Proposed fix (apply similarly to uploads)
public void saveResourceGraphicsPathMisc(String webResourceGraphicsPath, DwrResponseI18n response) { - boolean validated = PathSecureUtils.ValidationPaths.validatePath(webResourceGraphicsPath, a -> true); - if(!validated) { - response.addContextualMessage(SystemSettingsDAO.WEB_RESOURCE_GRAPHICS_PATH, "validate.invalidValue"); - } else if (webResourceGraphicsPath != null && (StringUtils.isEmpty(webResourceGraphicsPath) + if (webResourceGraphicsPath == null) { + response.addContextualMessage(SystemSettingsDAO.WEB_RESOURCE_GRAPHICS_PATH, "validate.invalidValue"); + return; + } + // Preserve existing "empty means default" behavior + if (StringUtils.isEmpty(webResourceGraphicsPath)) { + saveResourceGraphicsPathMisc(webResourceGraphicsPath); + return; + } + + boolean validated = PathSecureUtils.ValidationPaths.validatePath(webResourceGraphicsPath, a -> true); + if(!validated) { + response.addContextualMessage(SystemSettingsDAO.WEB_RESOURCE_GRAPHICS_PATH, "validate.invalidValue"); + } else if (webResourceGraphicsPath != null && (StringUtils.isEmpty(webResourceGraphicsPath) || (webResourceGraphicsPath.endsWith("graphics") || webResourceGraphicsPath.endsWith("graphics" + File.separator)))) { saveResourceGraphicsPathMisc(webResourceGraphicsPath); } else { response.addContextualMessage(SystemSettingsDAO.WEB_RESOURCE_GRAPHICS_PATH, "systemsettings.webresource.graphics.path.wrong", File.separator); } }WebContent/WEB-INF/jsp/maintenanceEvents.jsp (1)
71-108: Add null-safety checks to unescapeHtml calls for potentially null fields.The
unescapeHtmlfunction converts null/undefined to the strings "null"/"undefined" rather than handling them gracefully. Fields likexid,alias,activeCron, andinactiveCroncan be null when a new MaintenanceEventVO is created, causing incorrect string values to be displayed.Proposed fix
- $set("xid", unescapeHtml(me.xid)); + $set("xid", unescapeHtml(me.xid || "")); ... - $set("alias", unescapeHtml(me.alias)); + $set("alias", unescapeHtml(me.alias || "")); ... - $set("activeCron", unescapeHtml(me.activeCron)); + $set("activeCron", unescapeHtml(me.activeCron || "")); ... - $set("inactiveCron", unescapeHtml(me.inactiveCron)); + $set("inactiveCron", unescapeHtml(me.inactiveCron || ""));WebContent/WEB-INF/jsp/scheduledEvents.jsp (1)
74-104: Use empty string fallback for unescapeHtml calls to avoid setting form fields to literal "null"/"undefined" strings.
If se.xid, se.alias, se.activeCron, or se.inactiveCron are null/undefined, unescapeHtml silently converts them to the strings "null" or "undefined", which will then populate the form elements with unwanted text instead of empty values.Proposed patch
- $set("xid", unescapeHtml(se.xid)); - $set("alias", unescapeHtml(se.alias)); + $set("xid", unescapeHtml(se.xid || "")); + $set("alias", unescapeHtml(se.alias || "")); ... - $set("activeCron", unescapeHtml(se.activeCron)); + $set("activeCron", unescapeHtml(se.activeCron || "")); ... - $set("inactiveCron", unescapeHtml(se.inactiveCron)); + $set("inactiveCron", unescapeHtml(se.inactiveCron || ""));WebContent/WEB-INF/jsp/eventHandlers.jsp (1)
516-535: UsetextContentinstead ofinnerHTMLto prevent XSS when writing unescaped content to the DOM.The call to
unescapeHtml(handler.message)reverses HTML entity escaping (e.g.,<script>→<script>). When this decoded value is passed to$set, which delegates todwr.util.setValueusing innerHTML, the browser interprets the decoded content as executable code. PrefertextContentorinnerTextto safely insert plain text.Suggested fix
- else - $set(handler.id +"Msg", unescapeHtml(handler.message)); + else { + var el = $(handler.id + "Msg"); + if (el) el.textContent = unescapeHtml((handler && handler.message) || ""); + }scadalts-ui/src/views/DataObjects/Scripting/index.vue (1)
420-432: Avoid mixed escaping rules:scriptis unescaped on save butnameisn’t.
This is a common source of double-escaping regressions (especially if backend converters now escape outbound). Consider unescapingnamein the payload as well (or unescaping nothing ifEscapedTextareaalready returns raw).Proposed fix (if `EscapedTextarea` stores escaped values)
const payload = { ...this.scriptForm, + name: unescapeVueHtml(this.scriptForm.name || ''), script: unescapeVueHtml(this.scriptForm.script || ''), };
🤖 Fix all issues with AI agents
In @src/com/serotonin/mango/vo/report/ReportVO.java:
- Around line 877-878: Avoid NPE and encoded-semicolon bypass by treating null
names and decoding URL-encoded input before validation: replace the direct use
of name in the validateFilename and contains checks with a safe, decoded value
(e.g., safeName = name == null ? "" : name; decodedName =
URLDecoder.decode(safeName, StandardCharsets.UTF_8)); use
validateFilename(decodedName + ".csv") and check decodedName for ';' (and
optionally check for the encoded form "%3B" case-insensitively) before calling
response.addContextualMessage("name", "validate.invalidValue"); ensure you never
concatenate a literal "null" into the filename and handle
UnsupportedEncodingException if not using StandardCharsets.
In @src/org/scada_lts/utils/PathSecureUtils.java:
- Around line 58-65: normalizePath currently returns null on failure which
causes a NullPointerException at call sites like UploadBackgroundFileUtilsTest
where .toString() is invoked without a null check; change the API to return
Optional<Path> instead: update normalizePath(String) to return Optional<Path>,
wrap successful result in Optional.of(path) and return Optional.empty() on
exception (logging the error), then update all callers (notably
UploadBackgroundFileUtilsTest) to consume the Optional safely (e.g., use
map(Path::toString).orElseThrow(...) or orElse/ifPresent) so no call invokes
methods on a possibly-null Path.
In @src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java:
- Around line 26-36: convertObjectEscaped and convertObjectUnescaped (and
similar blocks) can NPE or throw for simple types; update doConvertIf usage to
first return null for null input, return simple types unchanged, and apply
escape/unescape only for Strings. Specifically, in
convertObjectEscaped/convertObjectUnescaped and the other occurrences around
doConvertIf/isSimpleJavaType/doConvert/convertObject, add a null check and a
simple-type branch that returns the original value (but for instance-of String,
call escapeIfString/unescapeIfString), ensuring doConvert is only invoked for
complex types.
- Around line 68-85: The conversion loop currently swallows conversion failures
by returning null (causing the original unescaped value to be used) and/or only
logging; change the logic so any exception or failed conversion during
convert.apply(...) results in throwing a ScadaMarshallException (wrap underlying
exception and include stacktrace) instead of falling back silently — update the
try/catch in the loop around field.setAccessible(...) / convert.apply(...) to
rethrow new ScadaMarshallException(originObject.getClass(), e) (or throw a
RuntimeException wrapping that) and remove the silent-null fallback; apply the
same change to the other similar blocks referenced (lines ~148-151, 165-168,
182-185, 197-200) and ensure NoEscape handling remains only for successfully
converted non-null results.
- Around line 60-88: doConvert does only shallow conversion — nested bean fields
and collection/map/array elements are left unprocessed; implement deep
conversion by recursively applying the conversion to complex values: for
non-primitive/non-String objects, call doConvert (or convertObject) on the
nested object, and for arrays/Collections/Maps iterate their elements and
convert each element/value, creating new containers as needed. Add cycle
detection using an IdentityHashMap<Object, Object> visited map passed through
recursive calls to avoid infinite loops and reuse created instances. Preserve
NoEscape handling on fields and ensure field.set uses the converted nested
instance when available; apply the same recursive logic to the similar code at
the other location referenced (lines 108-131).
- Around line 96-106: newInstanceEmpty is brittle: stop assuming specific
constructor signatures and avoid converting Map keys. Update newInstanceEmpty to
(1) handle Collection and Map types explicitly by creating safe empty instances
(e.g., ArrayList for Collection, HashMap/LinkedHashMap for Map) instead of
calling getConstructor(Collection/Map.class); (2) keep the existing
EmptyInstance and LocalizableMessage branches, and for other types attempt a
no-arg constructor via getDeclaredConstructor().setAccessible(true) but if
instantiation fails return a safe fallback (the original object or a documented
empty placeholder) rather than null; and (3) when converting maps, do not
transform keys — use the original key when putting into the output map (avoid
out.put(convert.apply(key), ...)), only convert values. Reference:
newInstanceEmpty, EmptyInstance, LocalizableMessage, and the map conversion site
that uses out.put(convert.apply(key), ...).
In @src/org/scada_lts/web/security/dwr/XssLocalizableMessageConverter.java:
- Around line 18-29: convertInbound currently takes the result of
StringConverter.convertInbound (a String) into convertObjectUnescaped which
rejects simple types and throws ScadaMarshallException; modify convertInbound so
after calling super.convertInbound(paramType, iv, inctx) you check if the
returned 'converted' is a String and, if so, unescape it directly (e.g. call
XssProtectUtils.unescapeHtml on converted) instead of calling
convertObjectUnescaped; for non-String types continue to call
convertObjectUnescaped(converted).
In @WebContent/WEB-INF/jsp/pointHierarchySLTS.jsp:
- Around line 827-832: The dialog uses an undefined UI string and an
out-of-scope error variable: add a new message key (e.g., messages.folderNotAdd
or messages.pointHierarchySLTS.folderNotAdd) to the messages object and to i18n
properties, and replace the out-of-scope errorThrown reference in the dialog
HTML with a defined value (either a new messages key like
messages.errorThrown/messageGeneric or the actual error parameter from the AJAX
error callback); ensure the dialog construction in the branch that checks
titleNewNode uses only defined message keys/variables.
- Around line 820-825: The endpoint handlers for
/pointHierarchy/new/{newParentId}/{newTitle} and the edit counterpart accept
newTitle raw and lack server-side validation; update the controller/service that
handles these routes to validate and sanitize newTitle (enforce non-empty and
max length, verify/normalize character encoding, reject or strip dangerous
characters, and apply XSS escaping via XssProtectUtils.escapeHtml() before
persisting), implement these checks where the TODO at line 161 indicates
incomplete validation, and return clear 4xx responses on validation failure
instead of persisting invalid input.
In @WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp:
- Around line 214-218: The renderer that returns "<span>" + data.pointType +
"</span>" (the anonymous function producing the pointType cell) does not escape
untrusted content; change it to use the existing escapeHtml helper (same as used
for data.pointName and data.parameterName) so the output becomes "<span>" +
escapeHtml(data.pointType) + "</span>" to prevent XSS.
- Line 38: The code constructs objects with type:
unescapeHtml(list[i].dataTypeMessage) and later renders data.pointType directly
into the DOM, creating an XSS risk; update the renderer that returns "<span>" +
data.pointType + "</span>" to instead call escapeHtml(data.pointType) (i.e.,
wrap data.pointType with escapeHtml) so the value is safely escaped before
insertion; apply the same fix to the equivalent renderers in editPersistent.jsp
and editPachube.jsp that output data.pointType.
In @WebContent/WEB-INF/jsp/reports.jsp:
- Around line 160-169: The inline onclick handlers embed ri.name directly which
can break the JS string or enable XSS; change the rendering to stop injecting
raw ri.name into the onclick and instead put the values into data attributes on
the <img> (e.g., data-name and data-id) and update the handlers to read from the
element (create helper functions like exportDataFromElement,
exportEventDataFromElement, exportUserCommentsFromElement that call
exportData/exportEventData/exportUserComments after decoding the data-name), or
alternatively properly escape ri.name for JS string context before embedding it.
- Around line 338-340: The updateReport function assigns untrusted name to DOM
via $("r"+ id +"Name").innerHTML which enables XSS; change the assignment to use
textContent (i.e., $("r"+ id +"Name").textContent = name) or run name through an
HTML-escaping sanitizer before assigning to innerHTML so the value is rendered
as plain text rather than executable HTML.
In @WebContent/WEB-INF/jsp/users.jsp:
- Line 326: The line using $("u"+ user.id +"Username").innerHTML =
user.username; introduces an XSS risk by injecting raw HTML; change to assign
the username as plain text (use textContent or equivalent) so the DOM receives
untrusted data safely, i.e., replace the innerHTML assignment with a text-safe
assignment for the element referenced by $("u"+ user.id +"Username"); if the
value must contain trusted HTML, explicitly sanitize or unescape it before
inserting (perform sanitization at the client using a vetted sanitizer or
confirm backend-provided HTML is safe) and document why innerHTML is necessary
when touching the user.username insertion.
🟡 Minor comments (6)
WebContent/WEB-INF/jsp/scripting.jsp-106-106 (1)
106-106: Inconsistent HTML handling for script name display.Line 106 assigns
se.nametoinnerHTML, but line 122 shows that script names are HTML-escaped on the server (requiringunescapeHtml()for editing). This will cause HTML entities to display literally in the script list.Suggested fix
Use
textContentfor safe text assignment (recommended):- $("se"+ se.id +"Name").innerHTML = se.name; + $("se"+ se.id +"Name").textContent = se.name;Or unescape if HTML rendering is needed:
- $("se"+ se.id +"Name").innerHTML = se.name; + $("se"+ se.id +"Name").innerHTML = unescapeHtml(se.name);WebContent/WEB-INF/jsp/watchList.jsp-154-157 (1)
154-157: UseunescapeHtml()when assigningpoint.valuetoinnerHTMLat line 157.Line 123 explicitly unescapes point values with
unescapeHtml()for display, but line 157 assignspoint.valuedirectly toinnerHTMLwithout unescaping. This inconsistency causes HTML entities in point names to render literally instead of being decoded properly in the tree node title.Change line 157 from:
spanNode.innerHTML = point.value;to:
spanNode.innerHTML = unescapeHtml(point.value);src/org/scada_lts/web/security/dwr/NoEscape.java-5-9 (1)
5-9: Fix javadoc inconsistency.The javadoc mentions "field or method parameter", but the
@Targetannotation only includesElementType.FIELD. Either update the javadoc to remove "or method parameter" or addElementType.PARAMETERto the@Targetif method parameters should also be supported.📝 Proposed fix
/** - * Marks a field or method parameter as safe to carry raw HTML. + * Marks a field as safe to carry raw HTML. * Use sparingly: only for server-generated HTML fragments you really intend to render via innerHTML. */test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsExceptionTest.java-30-42 (1)
30-42: Test method names reference wrong exception type.The method names mention
IllegalArgumentExceptionbut the tests expectScadaMarshallException:
when_convertObjectEscaped_for_object_with_type_no_source_then_IllegalArgumentExceptionwhen_convertObjectUnescaped_for_object_with_type_no_source_then_IllegalArgumentExceptionThis creates confusion between the method name and actual behavior.
Suggested fix
- public void when_convertObjectEscaped_for_object_with_type_no_source_then_IllegalArgumentException() throws ScadaMarshallException { + public void when_convertObjectEscaped_for_simple_type_then_ScadaMarshallException() throws ScadaMarshallException { - public void when_convertObjectUnescaped_for_object_with_type_no_source_then_IllegalArgumentException() throws ScadaMarshallException { + public void when_convertObjectUnescaped_for_simple_type_then_ScadaMarshallException() throws ScadaMarshallException {WebContent/WEB-INF/jsp/mailingLists.jsp-232-232 (1)
232-232: Potential double-escaping issue with referenceAddress mutation.The
addressEntryobject is added toeditingMailingList.entrieson line 231, then itsreferenceAddressis escaped on line 232. Since JavaScript objects are passed by reference, this mutation affects the entry in the array. WhencreateRecipList()(lines 146-150) sends entries back to the server, the escaped value is transmitted. If the server escapes again on save, this could cause double-escaping on subsequent edits.Consider escaping only for display purposes:
Suggested fix
editingMailingList.entries[editingMailingList.entries.length] = addressEntry; - addressEntry.referenceAddress = escapeHtml(addressEntry.referenceAddress); - appendAddressEntry(addressEntry); + appendAddressEntry({...addressEntry, referenceAddress: escapeHtml(addressEntry.referenceAddress)});Or pass the escaped value only to the display function without mutating the original entry.
Committable suggestion skipped: line range outside the PR's diff.
scadalts-ui/src/views/DataObjects/Scripting/index.vue-30-33 (1)
30-33:decodeName()is fine for safe display, but ensure search/sort semantics are intentional.
You’re unescaping only for the rendered cell; filtering/sorting still uses the (possibly escaped) underlyingitem.name, which can feel inconsistent for users searching for</&content. If you want “what you see is what you search”, consider normalizing the search source too.Also applies to: 453-455
🧹 Nitpick comments (9)
src/org/scada_lts/mango/service/SystemSettingsService.java (1)
471-483: Consider whethervalidatePath(path, a -> true)provides enough assuranceBecause the predicate always returns
true,validatePathis only doing normalization/string checks (not existence). If you intended existence checks, passFiles::exists; if not, consider simplifyingvalidatePath’s signature to avoid implying existence validation.Also applies to: 485-497
src/org/scada_lts/web/security/EmptyInstance.java (1)
3-5: Consider using generics for type safety.The return type
Objectrequires callers to cast, which can fail at runtime. Consider using a generic type parameter for better type safety.♻️ Proposed refactor with generics
-public interface EmptyInstance { - Object newInstanceEmpty(); +public interface EmptyInstance<T> { + T newInstanceEmpty(); }This would allow implementations like:
public class DataPointVO implements EmptyInstance<DataPointVO> { @Override public DataPointVO newInstanceEmpty() { return new DataPointVO(-1, -1, -1); } }WebContent/WEB-INF/jsp/maintenanceEvents.jsp (1)
55-60: Nice: unescaping option labels before addOptions improves UX; just harden against missing data.Optional hardening
- var ds = response.data.dataSources.map(function(o) { - return { key: o.key, value: unescapeHtml(o.value) }; - }); + var ds = (response.data && response.data.dataSources ? response.data.dataSources : []).map(function(o) { + return { key: o.key, value: unescapeHtml((o && o.value) || "") }; + }); dwr.util.addOptions("dataSourceId", ds, "key", "value");WebContent/WEB-INF/jsp/pointLinks.jsp (1)
29-44: Standardize toletand add optional null-safety guards.The codebase already uses ES6 syntax (
let, arrow functions) across 26+ JSP files, so ES5 rewrite is not necessary. However, lines 31–32 mixletwith thevardeclarations above (line 26–27). For consistency, useletthroughout. Additionally, add defensive null checks forp.valueas optional hardening:Suggested refactor
- let srcOpts = response.sourcePoints.map(p => ({ key: p.key, value: unescapeHtml(p.value) })); - let tgtOpts = response.targetPoints.map(p => ({ key: p.key, value: unescapeHtml(p.value) })); + let srcOpts = (response.sourcePoints || []).map(p => ({ + key: p.key, + value: unescapeHtml((p && p.value) || "") + })); + let tgtOpts = (response.targetPoints || []).map(p => ({ + key: p.key, + value: unescapeHtml((p && p.value) || "") + }));scadalts-ui/src/utils/common.js (1)
66-83: Consider handlingnull/undefinedinputs explicitly.
String(null)returns"null"andString(undefined)returns"undefined", which may not be the intended behavior when unescaping. Consider adding a guard:💡 Suggested improvement
export function unescapeVueHtml(str) { + if (str == null) return ''; let s = String(str);src/org/scada_lts/web/security/dwr/ScadaMarshallException.java (1)
5-17: Consider addingserialVersionUID.Since exceptions are
Serializable, adding an explicitserialVersionUIDensures consistent serialization across JVM versions.💡 Suggested improvement
public class ScadaMarshallException extends MarshallException { + private static final long serialVersionUID = 1L; public ScadaMarshallException(Class paramType) {src/com/serotonin/mango/web/dwr/beans/DataPointBean.java (1)
98-153: EmptyInstance implementation for XSS converter support.The
newInstanceEmpty()method correctly implements theEmptyInstanceinterface to support the XSS bean conversion mechanism. The anonymousPointLocatorVOprovides stub implementations returning safe defaults (null, 0, false).Consider extracting the anonymous
PointLocatorVOto a private static nested class or a separateEmptyPointLocatorVOclass if this pattern is reused elsewhere, as it would improve readability and testability.test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsExceptionTest.java (1)
24-28: Unusedtypeparameter in constructor.The
typeparameter is passed to the constructor but never stored or used. It appears in the test name via@Parameterized.Parameters(name = "{index}: value: {0}, type: {1}")for display purposes only.If the type is only needed for test naming, consider removing it from the constructor signature, or document why it's passed but unused:
- public XssBeanConverterUtilsExceptionTest(Object value, Class<?> type) { + public XssBeanConverterUtilsExceptionTest(Object value, Class<?> typeForDisplay) { this.value = value; + // typeForDisplay is used only in @Parameters name template }test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsEscapedTest.java (1)
13-59: Make assertions field-specific instead of relying on VOequals().
This will keep the test robust even ifequals()changes (or isn’t implemented). Also assert result type for clarity.Example adjustment
Object result = XssBeanConverterUtils.convertObjectEscaped(objectUnescaped); -Assert.assertEquals(objectEscaped, result); +Assert.assertTrue(result instanceof ContextualizedScriptVO); +Assert.assertEquals(objectEscaped.getScript(), ((ContextualizedScriptVO) result).getScript());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
WebContent/WEB-INF/dwr.xmlWebContent/WEB-INF/jsp/compoundEvents.jspWebContent/WEB-INF/jsp/dataSourceEdit/editHttpReceiver.jspWebContent/WEB-INF/jsp/dataSourceEdit/editSql.jspWebContent/WEB-INF/jsp/eventHandlers.jspWebContent/WEB-INF/jsp/include/settingsEditor.jspWebContent/WEB-INF/jsp/mailingLists.jspWebContent/WEB-INF/jsp/maintenanceEvents.jspWebContent/WEB-INF/jsp/pointEdit/pointProperties.jspWebContent/WEB-INF/jsp/pointHierarchySLTS.jspWebContent/WEB-INF/jsp/pointLinks.jspWebContent/WEB-INF/jsp/publisherEdit.jspWebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspWebContent/WEB-INF/jsp/publisherEdit/editPachube.jspWebContent/WEB-INF/jsp/publisherEdit/editPersistent.jspWebContent/WEB-INF/jsp/publisherList.jspWebContent/WEB-INF/jsp/reports.jspWebContent/WEB-INF/jsp/scheduledEvents.jspWebContent/WEB-INF/jsp/scripting.jspWebContent/WEB-INF/jsp/users.jspWebContent/WEB-INF/jsp/usersProfiles.jspWebContent/WEB-INF/jsp/watchList.jspWebContent/WEB-INF/tags/page.tagbuild.gradlescadalts-ui/src/apps/App.vuescadalts-ui/src/components/common/EscapedTextarea.vuescadalts-ui/src/components/datasources/MetaDataSource/point.vuescadalts-ui/src/utils/common.jsscadalts-ui/src/views/DataObjects/Scripting/index.vuescadalts-ui/src/views/System/SystemSettings/index.vuesrc/br/org/scadabr/view/component/LinkComponent.javasrc/com/serotonin/mango/vo/DataPointVO.javasrc/com/serotonin/mango/vo/event/EventTypeVO.javasrc/com/serotonin/mango/vo/event/MaintenanceEventVO.javasrc/com/serotonin/mango/vo/event/ScheduledEventVO.javasrc/com/serotonin/mango/vo/report/ReportVO.javasrc/com/serotonin/mango/web/dwr/PointLinksDwr.javasrc/com/serotonin/mango/web/dwr/XssDataPointBeanConverter.javasrc/com/serotonin/mango/web/dwr/XssDataPointVoConverter.javasrc/com/serotonin/mango/web/dwr/beans/DataPointBean.javasrc/org/scada_lts/dao/cache/UsersProfileDaoWithCache.javasrc/org/scada_lts/mango/service/SystemSettingsService.javasrc/org/scada_lts/utils/PathSecureUtils.javasrc/org/scada_lts/web/security/EmptyInstance.javasrc/org/scada_lts/web/security/XssProtectUtils.javasrc/org/scada_lts/web/security/dwr/NoEscape.javasrc/org/scada_lts/web/security/dwr/ScadaMarshallException.javasrc/org/scada_lts/web/security/dwr/XssBeanConverter.javasrc/org/scada_lts/web/security/dwr/XssBeanConverterUtils.javasrc/org/scada_lts/web/security/dwr/XssLocalizableMessageConverter.javatest/org/scada_lts/web/security/dwr/XssBeanConverterUtilsEscapedTest.javatest/org/scada_lts/web/security/dwr/XssBeanConverterUtilsExceptionTest.javatest/org/scada_lts/web/security/dwr/XssBeanConverterUtilsTest.javatest/org/scada_lts/web/security/dwr/XssBeanConverterUtilsTestsSuite.java
💤 Files with no reviewable changes (3)
- src/com/serotonin/mango/web/dwr/XssDataPointBeanConverter.java
- src/com/serotonin/mango/web/dwr/PointLinksDwr.java
- src/com/serotonin/mango/web/dwr/XssDataPointVoConverter.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T22:12:13.613Z
Learnt from: Limraj
Repo: SCADA-LTS/Scada-LTS PR: 3239
File: WebContent/WEB-INF/jsp/eventHandlers.jsp:275-282
Timestamp: 2026-01-06T22:12:13.613Z
Learning: In WebContent/WEB-INF/jsp/eventHandlers.jsp, when editing an existing event handler in showHandlerEdit(), targetPoint being null/undefined is considered an invalid state (data integrity violation) because if a handler exists with a targetPointId, the corresponding point is guaranteed to exist in allPoints. The maintainer has asserted this architectural invariant.
Applied to files:
WebContent/WEB-INF/jsp/pointLinks.jspWebContent/WEB-INF/jsp/dataSourceEdit/editHttpReceiver.jspWebContent/WEB-INF/jsp/pointEdit/pointProperties.jspWebContent/WEB-INF/jsp/pointHierarchySLTS.jspWebContent/WEB-INF/jsp/publisherEdit/editPachube.jspWebContent/WEB-INF/jsp/eventHandlers.jspWebContent/WEB-INF/jsp/compoundEvents.jspWebContent/WEB-INF/jsp/publisherEdit/editPersistent.jspWebContent/WEB-INF/jsp/include/settingsEditor.jsp
🧬 Code graph analysis (13)
src/com/serotonin/mango/vo/report/ReportVO.java (1)
src/org/scada_lts/utils/PathSecureUtils.java (2)
PathSecureUtils(25-325)ValidationPaths(257-324)
test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsTestsSuite.java (2)
test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsTest.java (1)
RunWith(34-136)test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsExceptionTest.java (1)
RunWith(7-44)
src/org/scada_lts/web/security/dwr/XssBeanConverter.java (1)
src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java (1)
XssBeanConverterUtils(20-218)
src/br/org/scadabr/view/component/LinkComponent.java (1)
scadalts-ui/src/utils/common.js (1)
escapeHtml(85-90)
test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsExceptionTest.java (1)
src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java (1)
XssBeanConverterUtils(20-218)
src/com/serotonin/mango/vo/event/ScheduledEventVO.java (2)
src/org/scada_lts/web/security/XssProtectUtils.java (1)
XssProtectUtils(8-54)scadalts-ui/src/utils/common.js (1)
unescapeHtml(60-64)
src/org/scada_lts/utils/PathSecureUtils.java (1)
src/com/serotonin/mango/util/LoggingUtils.java (1)
LoggingUtils(34-300)
src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java (2)
src/com/serotonin/mango/util/LoggingUtils.java (1)
LoggingUtils(34-300)src/org/scada_lts/web/security/XssProtectUtils.java (1)
XssProtectUtils(8-54)
src/org/scada_lts/mango/service/SystemSettingsService.java (3)
src/org/scada_lts/utils/PathSecureUtils.java (2)
PathSecureUtils(25-325)ValidationPaths(257-324)src/org/scada_lts/utils/SystemSettingsUtils.java (1)
SystemSettingsUtils(25-817)src/org/scada_lts/dao/SystemSettingsDAO.java (1)
SystemSettingsDAO(50-496)
src/org/scada_lts/web/security/dwr/XssLocalizableMessageConverter.java (1)
src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java (1)
XssBeanConverterUtils(20-218)
src/com/serotonin/mango/vo/event/MaintenanceEventVO.java (2)
src/org/scada_lts/web/security/XssProtectUtils.java (1)
XssProtectUtils(8-54)scadalts-ui/src/utils/common.js (1)
unescapeHtml(60-64)
src/org/scada_lts/web/security/XssProtectUtils.java (1)
scadalts-ui/src/utils/common.js (2)
escapeHtml(85-90)unescapeHtml(60-64)
test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsEscapedTest.java (1)
src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java (1)
XssBeanConverterUtils(20-218)
🔇 Additional comments (48)
src/org/scada_lts/dao/cache/UsersProfileDaoWithCache.java (1)
41-46: Good consistency improvement, but creates redundancy in callers.The change makes
selectProfilesconsistent withselectUserProfileByUserId(lines 35-37) and ensures fresh VO instances are returned, which aligns with the XSS protection work in this PR. However, this introduces redundant wrapping inselectProfileByIdandselectProfileByXid(already flagged above).WebContent/WEB-INF/jsp/pointHierarchySLTS.jsp (1)
818-844: Improved error handling and user experience.The new implementation properly handles the user flow by:
- Disabling controls and showing a spinner during the operation
- Validating input before making the AJAX call (early return pattern)
- Re-enabling controls on both success and error paths
This is a good improvement over the previous immediate AJAX call approach.
src/org/scada_lts/utils/PathSecureUtils.java (1)
73-76: LGTM: null-guards added to contain failures fromnormalizePathThe added
normalizedPath == nullchecks innormalizePath(Path, ...),getAbsoluteResourcePath, and thepath != nullguard in image-path collection correctly prevent downstream NPEs and avoid adding invalid paths.Also applies to: 192-203, 210-226
WebContent/WEB-INF/jsp/dataSourceEdit/editSql.jsp (1)
129-131: LGTM! Proper HTML unescaping for form field population.The use of
unescapeHtml()is appropriate here. Server-side data is HTML-escaped for storage, so unescaping when populating editable form fields ensures users see the original values they entered.WebContent/WEB-INF/jsp/watchList.jsp (2)
123-123: LGTM! Proper unescaping for point names.The
unescapeHtml()call correctly reverses server-side HTML escaping, and the value is later safely assigned viatextContent(line 350), which prevents XSS.
661-664: LGTM! Proper output escaping with c:out.The use of
<c:out>for JSP EL expressions in HTML attributes correctly prevents XSS injection.WebContent/WEB-INF/jsp/publisherEdit/editPersistent.jsp (1)
31-31: LGTM! Correct round-trip HTML escaping.Line 31 correctly unescapes server data when loading, and line 79 re-escapes when rendering to HTML. This pattern prevents both double-escaping (which would show HTML entities to users) and XSS injection.
Also applies to: 79-79
WebContent/WEB-INF/jsp/scripting.jsp (1)
121-123: LGTM! Proper HTML unescaping for form fields.The use of
unescapeHtml()correctly reverses server-side HTML escaping when populating editable form fields.WebContent/WEB-INF/jsp/dataSourceEdit/editHttpReceiver.jsp (1)
185-187: LGTM! Proper HTML unescaping for form field population.The selective use of
unescapeHtml()for string fields (parameterName, binary0Value) while leaving the numeric field (dataTypeId) untouched is correct and consistent with the XSS protection framework.WebContent/WEB-INF/tags/page.tag (2)
33-35: LGTM: Proper XSS protection in page title.Using
<c:out>correctly escapes theinstanceDescriptionHeadervalue, preventing potential XSS attacks through malicious instance descriptions.
223-225: LGTM: Consistent XSS protection in link text.The same escaping pattern is correctly applied to the visible link text, maintaining consistency with the title section.
src/org/scada_lts/web/security/XssProtectUtils.java (3)
10-23: LGTM: Well-organized centralized entity mapping.The static map provides a clean, maintainable way to handle newline and whitespace character escaping. The non-overlapping entity codes ensure that iteration order doesn't affect correctness.
27-31: LGTM: Correct escaping order maintained.The refactored implementation correctly applies standard HTML escaping before custom newline/whitespace escaping, ensuring proper handling of special characters.
33-53: LGTM: Correct bidirectional escape/unescape implementation.The new
unescapeHtmlmethod properly reverses the escaping order (custom entities first, then standard HTML), ensuring round-trip correctness. The map-driven approach maintains consistency between escaping and unescaping logic.src/br/org/scadabr/view/component/LinkComponent.java (1)
52-60: LGTM: Proper XSS protection for link component.The implementation correctly escapes both the href attribute and link text before HTML construction, with proper null handling. This prevents XSS attacks through malicious link or text content.
WebContent/WEB-INF/jsp/include/settingsEditor.jsp (1)
157-164: No action needed. TheunescapeHtmlfunction is correctly available in scope.The function is defined in
WebContent/resources/common.js(lines 1314-1318) and is loaded globally viapage.tag(line 96) before any JSP that includessettingsEditor.jspruns. The inline script comment at line 74 appropriately documents this dependency.src/com/serotonin/mango/vo/event/ScheduledEventVO.java (1)
143-200: Verify thatLocalizableMessage.getLocalizedMessage()properly escapes parameters when rendering.The code unescapes
alias,activeCron, andinactiveCronbefore passing them as parameters toLocalizableMessage. This pattern assumesLocalizableMessage.getLocalizedMessage()escapes these parameters during rendering. If it does not, unescaped user-controlled values could be rendered unsafely, reintroducing XSS vulnerabilities.This approach is used consistently in both
ScheduledEventVOandMaintenanceEventVO. The security of this pattern depends entirely on the externalseroUtils.jarlibrary properly escaping parameters during message localization and rendering.WebContent/WEB-INF/jsp/usersProfiles.jsp (1)
151-151: LGTM!The unescaping is correctly applied before populating the input field. Since HTML input fields automatically handle text content safely, unescaping the server-escaped value here is the right approach.
WebContent/WEB-INF/jsp/compoundEvents.jsp (1)
99-99: LGTM!Correctly unescaping the HTML-escaped backend value before populating the input field. This follows the established pattern and maintains security.
WebContent/WEB-INF/jsp/publisherEdit/editPachube.jsp (2)
30-30: LGTM!Correctly unescaping the HTML-escaped value when building the
allPointsarray. This ensures the data is in its original form for processing and display in select options.
80-80: Good security pattern: Escaping before innerHTML assignment.This demonstrates the correct approach: explicitly escaping user-controlled content before rendering via
innerHTML. This prevents XSS while allowing safe HTML structure in the table cell.WebContent/WEB-INF/jsp/eventHandlers.jsp (2)
284-290: Alias unescape for an input field is reasonable here.
(Just ensure unescapeHtml is null-safe as suggested elsewhere.)
223-240: Verify handler data structure and references. The review identifiesunescapeHtml(handler.message)as vulnerable, but this field doesn't exist in the EventHandlerVO serialization; onlyalias,id,handlerType, anddisabledare present in the handler response. Confirm whether this is dead code, a bug, or whether the intended field washandler.alias(which is used correctly elsewhere with $set).Likely an incorrect or invalid review comment.
build.gradle (1)
186-252: Both test suite classes resolve correctly:
org.scada_lts.web.beans.validation.css.CssValidatorTestsSuiteexists attest/org/scada_lts/web/beans/validation/css/CssValidatorTestsSuite.javaorg.scada_lts.web.security.dwr.XssBeanConverterUtilsTestsSuiteexists attest/org/scada_lts/web/security/dwr/XssBeanConverterUtilsTestsSuite.javaNo issues found with the test filter configuration.
src/org/scada_lts/web/security/dwr/XssBeanConverter.java (1)
9-22: Clean implementation of XSS-safe DWR converter.The converter correctly escapes outbound data (server→client) and unescapes inbound data (client→server), delegating marshalling to the parent
BeanConverter. The exception types are compatible sinceScadaMarshallExceptionextendsMarshallException.scadalts-ui/src/views/System/SystemSettings/index.vue (2)
185-201: Appropriate use ofEscapedTextareafor XSS-safe input.Switching to
EscapedTextareafor fields that may contain escaped HTML content is consistent with the PR's escaping improvements. Therows,auto-grow, and@focusoutbindings are correctly preserved.
299-305: LGTM!The import and component registration are correctly structured.
WebContent/WEB-INF/jsp/reports.jsp (1)
206-216: Good use ofencodeURIComponentfor URL path encoding.Properly encoding the name in URLs prevents path traversal and special character issues in filenames.
src/com/serotonin/mango/vo/event/EventTypeVO.java (1)
34-36: LGTM!The
EmptyInstanceimplementation enables the XSS converter utilities to instantiateEventTypeVOduring object conversion. Using-1sentinel values is consistent with the existing constructor patterns in this class.Also applies to: 177-181
WebContent/WEB-INF/jsp/publisherList.jsp (1)
37-39: No action required. TheunescapeHtmlfunction is properly available throughresources/common.js, which is included globally via thepage.tagfile used by this JSP. The function is already in use elsewhere in the codebase with no issues.Likely an incorrect or invalid review comment.
src/com/serotonin/mango/vo/event/MaintenanceEventVO.java (2)
34-34: LGTM!The static import is appropriate for the unescaping operations added in this file.
310-346: Incorrect assumption: fields are not stored in escaped form.Verification shows no HTML escaping occurs when persisting these fields to the database. The
MaintenanceEventsDwrclass passes raw parameters directly to setters (lines 109, 119, 126), and the DAO persists them without escaping. TheunescapeHtml()calls ingetDescription()lack a corresponding escaping step at storage time, suggesting either:
- Unnecessary defensive unescaping that masks a real escaping gap, or
- Missing escaping logic in the persistence layer
The valid concerns remain:
- Null handling:
unescapeHtml(null)converts to"", notnull, which could affect message formatting if null is meaningful.- Performance:
getDescription()unescapes on every call; caching after first access could help if this method is called frequently.Recommend clarifying the escaping strategy: either remove the unescaping (if fields genuinely aren't escaped), or add escaping at the storage layer to match the retrieval logic.
Likely an incorrect or invalid review comment.
WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp (1)
123-123: LGTM!The escaping of
data.keyanddata.valuewhen rendering table rows correctly prevents XSS attacks.Also applies to: 167-167
test/org/scada_lts/web/security/dwr/XssBeanConverterUtilsTestsSuite.java (1)
1-13: LGTM!The test suite correctly aggregates the three test classes using JUnit 4's
@RunWith(Suite.class)pattern. This provides a convenient entry point for running all XssBeanConverterUtils-related tests.src/com/serotonin/mango/vo/DataPointVO.java (3)
53-53: LGTM!The import is necessary for implementing the
EmptyInstanceinterface.
66-66: LGTM!Adding the
EmptyInstanceinterface is consistent with the broader PR pattern of enabling empty-instance creation across VO/Bean types for DWR converter support.
1170-1173: LGTM!The
newInstanceEmpty()implementation correctly returns a newDataPointVOwith sentinel values (-1) for the required constructor parameters, providing a valid empty instance for the converter framework.scadalts-ui/src/components/datasources/MetaDataSource/point.vue (2)
319-328: LGTM - EscapedTextarea integration for XSS protection.The replacement of
v-textareawithEscapedTextareaproperly handles HTML-escaped script content. The component maintains all necessary bindings (validation rules, model, ref, events).Minor note:
rows=3works but:rows="3"would be more explicit for numeric prop binding in Vue.
387-392: LGTM - Proper component import and registration.The
EscapedTextareacomponent is correctly imported and registered alongside existing components.WebContent/WEB-INF/jsp/publisherEdit.jsp (2)
98-98: LGTM - Proper XSS protection with<c:out>escaping.Using
<c:out>to escape the publisher name prevents XSS attacks from user-supplied content in form field values.
106-144: Consistent escaping applied across all form fields.All dynamic values (
xid,enabled,cacheWarningSize,changesOnly,sendSnapshot,snapshotSendPeriods,snapshotSendPeriodType) are properly escaped with<c:out>, ensuring consistent XSS protection throughout the form.scadalts-ui/src/apps/App.vue (2)
21-22: LGTM - Safe unescaping withv-textbinding.Using
v-textwithunescapeVueHtml()is the correct approach—it decodes HTML entities for display while treating the result as text content (not HTML), preventing XSS. This properly complements the server-side escaping.
88-88: Proper import and method exposure for template usage.The
unescapeVueHtmlutility is correctly imported and exposed in methods, making it available for template bindings.Also applies to: 178-178
WebContent/WEB-INF/jsp/mailingLists.jsp (2)
50-50: LGTM - Proper unescaping for form field display.The mailing list name is correctly unescaped before setting it in the form field, complementing server-side escaping.
101-104: LGTM - Usernames properly unescaped for dropdown display.Using function accessors with
unescapeHtml()for the username display ensures escaped server data is rendered correctly in the select options.scadalts-ui/src/components/common/EscapedTextarea.vue (1)
1-51: Well-designed wrapper component for escaped content handling.The
EscapedTextareacomponent correctly:
- Decodes HTML entities for display using
unescapeVueHtml- Emits raw user input (server handles escaping on save)
- Forwards attributes and non-input listeners to the inner
v-textarea- Provides a
validate()method that delegates to the inner component for form validation support- Handles null values gracefully in both directions
The
inheritAttrs: falsewith explicit attribute forwarding is the correct Vue 2.x pattern for transparent wrapper components.scadalts-ui/src/views/DataObjects/Scripting/index.vue (1)
88-89: VerifyEscapedTextareav-model semantics + prop passthrough (rows,autoGrow) before relying on it.
IfEscapedTextareaemits escaped text, you likely need to unescape bothnameandscriptbefore persisting; if it emits raw text, thenunescapeVueHtml()on save could corrupt intended literals like<. Please confirm component behavior.Also applies to: 136-144, 201-207
WebContent/WEB-INF/dwr.xml (1)
23-27: Please verify DWR converter resolution order/precedence with the addedxssBeanmappings.
If DWR applies the first matching converter (rather than “most specific”), earlier wildcard mappings likeconverter="bean" match="com.serotonin.mango.vo.event.handlers.*"could prevent the laterxssBeanentries from ever running. This would reintroduce “some objects escaped, some not” regressions.Also applies to: 113-115, 141-148, 151-164, 174-184, 192-195, 218-220, 242-262, 267-270
| if(!PathSecureUtils.ValidationPaths.validateFilename(name + ".csv") || name.contains(";")) | ||
| response.addContextualMessage("name", "validate.invalidValue"); |
There was a problem hiding this comment.
Fix potential NPE + encoded semicolon bypass in report name validation
name.contains(";") will throw when name is null (and name + ".csv" can become "null.csv" and pass). Also, %3B bypasses the semicolon check since only the decoded form contains ;.
Proposed fix
public void validateRun(DwrResponseI18n response, User user) {
if (StringUtils.isEmpty(name))
response.addContextualMessage("name", "reports.validate.required");
if (StringUtils.isLengthGreaterThan(name, 100))
response.addContextualMessage("name", "reports.validate.longerThan100");
- if(!PathSecureUtils.ValidationPaths.validateFilename(name + ".csv") || name.contains(";"))
- response.addContextualMessage("name", "validate.invalidValue");
+ if (!StringUtils.isEmpty(name)) {
+ try {
+ String decodedName = PathSecureUtils.decodePath(name);
+ if (!PathSecureUtils.ValidationPaths.validateFilename(name + ".csv") || decodedName.contains(";")) {
+ response.addContextualMessage("name", "validate.invalidValue");
+ }
+ } catch (Exception ex) {
+ // treat decode/validation failures as invalid input
+ response.addContextualMessage("name", "validate.invalidValue");
+ }
+ }📝 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.
| if(!PathSecureUtils.ValidationPaths.validateFilename(name + ".csv") || name.contains(";")) | |
| response.addContextualMessage("name", "validate.invalidValue"); | |
| public void validateRun(DwrResponseI18n response, User user) { | |
| if (StringUtils.isEmpty(name)) | |
| response.addContextualMessage("name", "reports.validate.required"); | |
| if (StringUtils.isLengthGreaterThan(name, 100)) | |
| response.addContextualMessage("name", "reports.validate.longerThan100"); | |
| if (!StringUtils.isEmpty(name)) { | |
| try { | |
| String decodedName = PathSecureUtils.decodePath(name); | |
| if (!PathSecureUtils.ValidationPaths.validateFilename(name + ".csv") || decodedName.contains(";")) { | |
| response.addContextualMessage("name", "validate.invalidValue"); | |
| } | |
| } catch (Exception ex) { | |
| // treat decode/validation failures as invalid input | |
| response.addContextualMessage("name", "validate.invalidValue"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/com/serotonin/mango/vo/report/ReportVO.java around lines 877 - 878,
Avoid NPE and encoded-semicolon bypass by treating null names and decoding
URL-encoded input before validation: replace the direct use of name in the
validateFilename and contains checks with a safe, decoded value (e.g., safeName
= name == null ? "" : name; decodedName = URLDecoder.decode(safeName,
StandardCharsets.UTF_8)); use validateFilename(decodedName + ".csv") and check
decodedName for ';' (and optionally check for the encoded form "%3B"
case-insensitively) before calling response.addContextualMessage("name",
"validate.invalidValue"); ensure you never concatenate a literal "null" into the
filename and handle UnsupportedEncodingException if not using StandardCharsets.
| public static Path normalizePath(String path) { | ||
| return Paths.get(normalizeSeparator(path)).toFile().getAbsoluteFile().toPath().normalize(); | ||
| try { | ||
| return Paths.get(normalizeSeparator(path)).toFile().getAbsoluteFile().toPath().normalize(); | ||
| } catch (Exception exception) { | ||
| LOG.error(LoggingUtils.exceptionInfo(exception)); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=java -C2 'PathSecureUtils\.normalizePath\s*\('Repository: SCADA-LTS/Scada-LTS
Length of output: 1213
🏁 Script executed:
sed -n '100,115n' test/org/scada_lts/utils/UploadBackgroundFileUtilsTest.javaRepository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
cat -n test/org/scada_lts/utils/UploadBackgroundFileUtilsTest.java | sed -n '100,115p'Repository: SCADA-LTS/Scada-LTS
Length of output: 707
🏁 Script executed:
# Let's search for any other potential call sites more broadly
rg -n --type=java 'normalizePath' | grep -v 'public static Path normalizePath' | grep -v '//'Repository: SCADA-LTS/Scada-LTS
Length of output: 1150
Unsafe null handling confirmed in test file call site
normalizePath(String) returns null on error, but UploadBackgroundFileUtilsTest.java:106 calls .toString() on the result without null-checking, which will throw NullPointerException if the path normalization fails. The method at PathSecureUtils.java:193 does handle the null return safely with an explicit check. Either fix the test site to null-check, or refactor to use Optional<Path> to enforce null-safety at compile time.
🤖 Prompt for AI Agents
In @src/org/scada_lts/utils/PathSecureUtils.java around lines 58 - 65,
normalizePath currently returns null on failure which causes a
NullPointerException at call sites like UploadBackgroundFileUtilsTest where
.toString() is invoked without a null check; change the API to return
Optional<Path> instead: update normalizePath(String) to return Optional<Path>,
wrap successful result in Optional.of(path) and return Optional.empty() on
exception (logging the error), then update all callers (notably
UploadBackgroundFileUtilsTest) to consume the Optional safely (e.g., use
map(Path::toString).orElseThrow(...) or orElse/ifPresent) so no call invokes
methods on a possibly-null Path.
| public static Object convertObjectEscaped(Object value) throws ScadaMarshallException { | ||
| return doConvertIf(value, isSimpleJavaType().negate(), | ||
| XssBeanConverterUtils::doConvert, | ||
| object -> convertObject(object, XssBeanConverterUtils::escapeIfString)); | ||
| } | ||
|
|
||
| public static Object convertObjectUnescaped(Object value) throws ScadaMarshallException { | ||
| return doConvertIf(value, isSimpleJavaType().negate(), | ||
| XssBeanConverterUtils::doConvert, | ||
| object -> convertObject(object, XssBeanConverterUtils::unescapeIfString)); | ||
| } |
There was a problem hiding this comment.
Handle null and “simple” types without throwing (current code can NPE / break callers).
convertObjectEscaped/Unescaped(null) will NPE, and passing a String/primitive wrapper throws ScadaMarshallException (but some call sites appear to want string unescape). Safer behavior: null -> null, simple types -> return as-is, String -> escape/unescape.
Proposed fix
public static Object convertObjectEscaped(Object value) throws ScadaMarshallException {
- return doConvertIf(value, isSimpleJavaType().negate(),
- XssBeanConverterUtils::doConvert,
- object -> convertObject(object, XssBeanConverterUtils::escapeIfString));
+ if (value == null) return null;
+ if (value instanceof String) return XssProtectUtils.escapeHtml((String) value);
+ if (isSimpleJavaType().test(value)) return value;
+ return doConvertIf(value, o -> true,
+ XssBeanConverterUtils::doConvert,
+ object -> convertObject(object, XssBeanConverterUtils::escapeIfString));
}
public static Object convertObjectUnescaped(Object value) throws ScadaMarshallException {
- return doConvertIf(value, isSimpleJavaType().negate(),
- XssBeanConverterUtils::doConvert,
- object -> convertObject(object, XssBeanConverterUtils::unescapeIfString));
+ if (value == null) return null;
+ if (value instanceof String) return XssProtectUtils.unescapeHtml((String) value);
+ if (isSimpleJavaType().test(value)) return value;
+ return doConvertIf(value, o -> true,
+ XssBeanConverterUtils::doConvert,
+ object -> convertObject(object, XssBeanConverterUtils::unescapeIfString));
}Also applies to: 38-53, 55-58
🤖 Prompt for AI Agents
In @src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java around lines
26 - 36, convertObjectEscaped and convertObjectUnescaped (and similar blocks)
can NPE or throw for simple types; update doConvertIf usage to first return null
for null input, return simple types unchanged, and apply escape/unescape only
for Strings. Specifically, in convertObjectEscaped/convertObjectUnescaped and
the other occurrences around
doConvertIf/isSimpleJavaType/doConvert/convertObject, add a null check and a
simple-type branch that returns the original value (but for instance-of String,
call escapeIfString/unescapeIfString), ensuring doConvert is only invoked for
complex types.
| private static Object doConvert(Object originObject, Function<Object, Object> convert) { | ||
| Object object; | ||
| try { | ||
| object = newInstanceEmpty(originObject); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(new ScadaMarshallException(originObject.getClass(), e)); | ||
| } | ||
| List<Field> fields = getAllFields(originObject); | ||
| for(Field field: fields) { | ||
| if(!isStatic(field.getModifiers())) { | ||
| try { | ||
| field.setAccessible(true); | ||
| Object value = field.get(originObject); | ||
| if(value != null) { | ||
| Object converted = convert.apply(value); | ||
| if(converted != null && !field.isAnnotationPresent(NoEscape.class)) | ||
| field.set(object, converted); | ||
| else | ||
| field.set(object, value); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(new ScadaMarshallException(originObject.getClass(), e)); | ||
| } finally { | ||
| field.setAccessible(false); | ||
| } | ||
| } | ||
| } | ||
| return object; | ||
| } |
There was a problem hiding this comment.
Conversion is shallow (nested beans won’t be escaped/unescaped) — likely defeats the goal for many VO graphs.
For any field/collection element that is a complex object (not String/collection/map/array), convertObject() returns it unchanged, so its internal String fields remain unprocessed. If you intended deep conversion, you’ll need recursion + cycle detection (e.g., IdentityHashMap visited).
Also applies to: 108-131
🤖 Prompt for AI Agents
In @src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java around lines
60 - 88, doConvert does only shallow conversion — nested bean fields and
collection/map/array elements are left unprocessed; implement deep conversion by
recursively applying the conversion to complex values: for
non-primitive/non-String objects, call doConvert (or convertObject) on the
nested object, and for arrays/Collections/Maps iterate their elements and
convert each element/value, creating new containers as needed. Add cycle
detection using an IdentityHashMap<Object, Object> visited map passed through
recursive calls to avoid infinite loops and reuse created instances. Preserve
NoEscape handling on fields and ensure field.set uses the converted nested
instance when available; apply the same recursive logic to the similar code at
the other location referenced (lines 108-131).
| for(Field field: fields) { | ||
| if(!isStatic(field.getModifiers())) { | ||
| try { | ||
| field.setAccessible(true); | ||
| Object value = field.get(originObject); | ||
| if(value != null) { | ||
| Object converted = convert.apply(value); | ||
| if(converted != null && !field.isAnnotationPresent(NoEscape.class)) | ||
| field.set(object, converted); | ||
| else | ||
| field.set(object, value); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(new ScadaMarshallException(originObject.getClass(), e)); | ||
| } finally { | ||
| field.setAccessible(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t swallow conversion failures (returning null hides security gaps).
Returning null on conversion errors means fields fall back to original (unescaped) values with only a log line (without stacktrace). For XSS protection utilities, “fail open” is risky; prefer throwing ScadaMarshallException (or at least logging with stacktrace + explicit fallback choice).
Also applies to: 148-151, 165-168, 182-185, 197-200
🤖 Prompt for AI Agents
In @src/org/scada_lts/web/security/dwr/XssBeanConverterUtils.java around lines
68 - 85, The conversion loop currently swallows conversion failures by returning
null (causing the original unescaped value to be used) and/or only logging;
change the logic so any exception or failed conversion during convert.apply(...)
results in throwing a ScadaMarshallException (wrap underlying exception and
include stacktrace) instead of falling back silently — update the try/catch in
the loop around field.setAccessible(...) / convert.apply(...) to rethrow new
ScadaMarshallException(originObject.getClass(), e) (or throw a RuntimeException
wrapping that) and remove the silent-null fallback; apply the same change to the
other similar blocks referenced (lines ~148-151, 165-168, 182-185, 197-200) and
ensure NoEscape handling remains only for successfully converted non-null
results.
| for (var i=0; i<list.length; i++) | ||
| allPoints[allPoints.length] = { | ||
| id: list[i].id, name: list[i].extendedName, enabled: list[i].enabled, type: list[i].dataTypeMessage}; | ||
| id: list[i].id, name: unescapeHtml(list[i].extendedName), enabled: list[i].enabled, type: unescapeHtml(list[i].dataTypeMessage)}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find PublisherEditDwr.java and examine the initSender method
fd -type f -name '*PublisherEditDwr.java' --exec cat -n {} \;Repository: SCADA-LTS/Scada-LTS
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Also examine the editHttpSender.jsp file, particularly around the lines mentioned (38, 43, 48, 53, 70, 214-218)
fd -type f -name 'editHttpSender.jsp' --exec wc -l {} \;Repository: SCADA-LTS/Scada-LTS
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Find the JSP file and read the relevant sections
fd -type f -name 'editHttpSender.jsp' --exec cat -n {} \; | head -250Repository: SCADA-LTS/Scada-LTS
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Find PublisherEditDwr.java
fd PublisherEditDwr.javaRepository: SCADA-LTS/Scada-LTS
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find editHttpSender.jsp
fd editHttpSender.jspRepository: SCADA-LTS/Scada-LTS
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for initSender method in Java files
rg -n -A 30 'public.*initSender' --type-listRepository: SCADA-LTS/Scada-LTS
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Simpler search for initSender
rg -n 'initSender' -A 20Repository: SCADA-LTS/Scada-LTS
Length of output: 8503
🏁 Script executed:
#!/bin/bash
# Read the entire editHttpSender.jsp file
cat -n WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspRepository: SCADA-LTS/Scada-LTS
Length of output: 24838
🏁 Script executed:
#!/bin/bash
# Search for DwrResponseI18n to understand escaping
rg -n 'DwrResponseI18n' -A 10 -B 5Repository: SCADA-LTS/Scada-LTS
Length of output: 50376
🏁 Script executed:
#!/bin/bash
# Check if there's any XssBeanConverter or escaping applied
rg -n 'XssBeanConverter|escapeHtml|unescapeHtml' WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspRepository: SCADA-LTS/Scada-LTS
Length of output: 1132
🏁 Script executed:
#!/bin/bash
# Check the data flow - where pointType comes from
grep -n "pointType" WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jspRepository: SCADA-LTS/Scada-LTS
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Search for escapeHtml utility to understand its implementation
rg -n "function.*escapeHtml|escapeHtml\s*=" --context 3Repository: SCADA-LTS/Scada-LTS
Length of output: 7460
Wrap data.pointType with escapeHtml() to prevent XSS.
Line 216 outputs data.pointType in a span element without escaping, creating a cross-site scripting vulnerability. The value comes from list[i].dataTypeMessage which is HTML-escaped by the server (line 38), unescaped for storage in the JavaScript object (line 38: type: unescapeHtml(list[i].dataTypeMessage)), and then rendered directly to the DOM without re-escaping.
Compared to the adjacent pointName field on line 214 which correctly uses escapeHtml(data.pointName), the pointType output must be similarly escaped before DOM insertion.
Fix required at line 216:
// Before:
function(data) { return "<span>" + data.pointType + "</span>"; },
// After:
function(data) { return "<span>" + escapeHtml(data.pointType) + "</span>"; },Also applies to: editPersistent.jsp line 81, editPachube.jsp line 82 (both render data.pointType without escaping).
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp at line 38, The code
constructs objects with type: unescapeHtml(list[i].dataTypeMessage) and later
renders data.pointType directly into the DOM, creating an XSS risk; update the
renderer that returns "<span>" + data.pointType + "</span>" to instead call
escapeHtml(data.pointType) (i.e., wrap data.pointType with escapeHtml) so the
value is safely escaped before insertion; apply the same fix to the equivalent
renderers in editPersistent.jsp and editPachube.jsp that output data.pointType.
| function(data) { return "<span>" + escapeHtml(data.pointName) + "</span>"; }, | ||
| function(data) { return "<img src='images/"+ (data.enabled ? "brick_go" : "brick_stop") +".png'/>"; }, | ||
| function(data) { return data.pointType; }, | ||
| function(data) { return "<span>" + data.pointType + "</span>"; }, | ||
| function(data) { | ||
| return "<input type='text' value='"+ data.parameterName +"' "+ | ||
| return "<input type='text' value='"+ escapeHtml(data.parameterName) +"' "+ |
There was a problem hiding this comment.
Missing escaping for pointType on Line 216.
Line 214 correctly escapes data.pointName, and Line 218 escapes data.parameterName, but Line 216 renders data.pointType without escaping. If pointType contains user-controlled or untrusted content, this could create an XSS vulnerability.
🔒 Proposed fix
- function(data) { return "<span>" + data.pointType + "</span>"; },
+ function(data) { return "<span>" + escapeHtml(data.pointType) + "</span>"; },📝 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.
| function(data) { return "<span>" + escapeHtml(data.pointName) + "</span>"; }, | |
| function(data) { return "<img src='images/"+ (data.enabled ? "brick_go" : "brick_stop") +".png'/>"; }, | |
| function(data) { return data.pointType; }, | |
| function(data) { return "<span>" + data.pointType + "</span>"; }, | |
| function(data) { | |
| return "<input type='text' value='"+ data.parameterName +"' "+ | |
| return "<input type='text' value='"+ escapeHtml(data.parameterName) +"' "+ | |
| function(data) { return "<span>" + escapeHtml(data.pointName) + "</span>"; }, | |
| function(data) { return "<img src='images/"+ (data.enabled ? "brick_go" : "brick_stop") +".png'/>"; }, | |
| function(data) { return "<span>" + escapeHtml(data.pointType) + "</span>"; }, | |
| function(data) { | |
| return "<input type='text' value='"+ escapeHtml(data.parameterName) +"' "+ |
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/publisherEdit/editHttpSender.jsp around lines 214 -
218, The renderer that returns "<span>" + data.pointType + "</span>" (the
anonymous function producing the pointType cell) does not escape untrusted
content; change it to use the existing escapeHtml helper (same as used for
data.pointName and data.parameterName) so the output becomes "<span>" +
escapeHtml(data.pointType) + "</span>" to prevent XSS.
| var result = "<img src='images/bullet_down.png' class='ptr' title='<spring:message code="reports.export"/>' "+ | ||
| "onclick='exportData(\""+ escapeHtml(ri.name) +"\", "+ ri.id +")'/>"; | ||
| "onclick='exportData(\""+ ri.name +"\", "+ ri.id +")'/>"; | ||
|
|
||
| if (ri.includeEvents != <c:out value="<%= ReportVO.EVENTS_NONE %>"/>) | ||
| result += "<img src='images/flag_white.png' class='ptr' title='<spring:message code="reports.eventExport"/>' "+ | ||
| "onclick='exportEventData(\""+ escapeHtml(ri.name) +"\", "+ ri.id +")'/>"; | ||
| "onclick='exportEventData(\""+ ri.name +"\", "+ ri.id +")'/>"; | ||
|
|
||
| if (ri.includeUserComments) | ||
| result += "<img src='images/comment.png' class='ptr' title='<spring:message code="reports.userCommentExport"/>' "+ | ||
| "onclick='exportUserComments(\""+ escapeHtml(ri.name) +"\", "+ ri.id +")'/>"; | ||
| "onclick='exportUserComments(\""+ ri.name +"\", "+ ri.id +")'/>"; |
There was a problem hiding this comment.
Quote characters in ri.name can break onclick handlers or enable XSS.
If ri.name contains " or </script>, the inline JavaScript in onclick will break or become exploitable. Consider escaping the name for JavaScript string context or using a data attribute approach.
💡 Safer approach using data attributes
Instead of embedding the name directly in onclick, store it in a data attribute and retrieve it in the handler:
// In the row rendering function:
function(ri) {
var result = "<img src='images/bullet_down.png' class='ptr' title='<spring:message code="reports.export"/>' "+
"data-name='"+ encodeURIComponent(ri.name) +"' data-id='"+ ri.id +"' onclick='exportDataFromElement(this)'/>";
// ... rest of the code
}
// New handler function:
function exportDataFromElement(el) {
var name = decodeURIComponent(el.getAttribute('data-name'));
var id = el.getAttribute('data-id');
exportData(name, id);
}🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/reports.jsp around lines 160 - 169, The inline
onclick handlers embed ri.name directly which can break the JS string or enable
XSS; change the rendering to stop injecting raw ri.name into the onclick and
instead put the values into data attributes on the <img> (e.g., data-name and
data-id) and update the handlers to read from the element (create helper
functions like exportDataFromElement, exportEventDataFromElement,
exportUserCommentsFromElement that call
exportData/exportEventData/exportUserComments after decoding the data-name), or
alternatively properly escape ri.name for JS string context before embedding it.
| function updateReport(id, name) { | ||
| let escapedName = escapeHtml(name); | ||
| $("r"+ id +"Name").innerHTML = escapedName; | ||
| $("r"+ id +"Name").innerHTML = name; | ||
| } |
There was a problem hiding this comment.
Potential XSS vulnerability: raw name assigned to innerHTML.
Setting innerHTML directly with an unescaped name allows script injection if the name contains malicious HTML (e.g., <img onerror=alert(1)>). Use textContent instead or escape the value.
🔒 Proposed fix
function updateReport(id, name) {
- $("r"+ id +"Name").innerHTML = name;
+ $("r"+ id +"Name").textContent = name;
}📝 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.
| function updateReport(id, name) { | |
| let escapedName = escapeHtml(name); | |
| $("r"+ id +"Name").innerHTML = escapedName; | |
| $("r"+ id +"Name").innerHTML = name; | |
| } | |
| function updateReport(id, name) { | |
| $("r"+ id +"Name").textContent = name; | |
| } |
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/reports.jsp around lines 338 - 340, The updateReport
function assigns untrusted name to DOM via $("r"+ id +"Name").innerHTML which
enables XSS; change the assignment to use textContent (i.e., $("r"+ id
+"Name").textContent = name) or run name through an HTML-escaping sanitizer
before assigning to innerHTML so the value is rendered as plain text rather than
executable HTML.
| function updateUser(response) { | ||
| var user = response.data ? response.data.user : response.user; | ||
| $("u"+ user.id +"Username").textContent = user.username; | ||
| $("u"+ user.id +"Username").innerHTML = user.username; |
There was a problem hiding this comment.
Potential XSS vulnerability: Using innerHTML without explicit escaping.
Switching from textContent to innerHTML introduces XSS risk. If user.username contains malicious HTML or JavaScript, it will be executed in the browser context.
While the PR introduces XSS protection via backend converters, relying solely on backend escaping for innerHTML is risky:
- If backend escaping is bypassed or incomplete, this becomes an XSS vector
- If usernames are intentionally HTML-escaped on backend,
innerHTMLwill display the escape sequences literally (e.g.,<script>)
🔒 Recommended fix
Option 1 (preferred): Revert to textContent for safe text rendering
- $("u"+ user.id +"Username").innerHTML = user.username;
+ $("u"+ user.id +"Username").textContent = user.username;Option 2: If HTML content is intentional, explicitly escape
- $("u"+ user.id +"Username").innerHTML = user.username;
+ $("u"+ user.id +"Username").innerHTML = escapeHtml(user.username);Option 3: If displaying backend-escaped content, unescape first
- $("u"+ user.id +"Username").innerHTML = user.username;
+ $("u"+ user.id +"Username").textContent = unescapeHtml(user.username);📝 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.
| $("u"+ user.id +"Username").innerHTML = user.username; | |
| $("u"+ user.id +"Username").textContent = user.username; |
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/jsp/users.jsp at line 326, The line using $("u"+ user.id
+"Username").innerHTML = user.username; introduces an XSS risk by injecting raw
HTML; change to assign the username as plain text (use textContent or
equivalent) so the DOM receives untrusted data safely, i.e., replace the
innerHTML assignment with a text-safe assignment for the element referenced by
$("u"+ user.id +"Username"); if the value must contain trusted HTML, explicitly
sanitize or unescape it before inserting (perform sanitization at the client
using a vetted sanitizer or confirm backend-provided HTML is safe) and document
why innerHTML is necessary when touching the user.username insertion.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.