#3227 Update DWR library to version 3.0 #3230
Conversation
Java Script Mocha Unit Test Results268 tests 268 ✅ 3s ⏱️ Results for commit 76d6d89. ♻️ This comment has been updated with latest results. |
Java JUnit Test Results3 440 tests 3 440 ✅ 51s ⏱️ Results for commit 76d6d89. ♻️ This comment has been updated with latest results. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request upgrades Direct Web Remoting (DWR) from version 2.0 to 3.0. Changes include updating the XML configuration doctype, refactoring converter method signatures to match DWR 3.0 specifications, adding JavaScript compatibility aliases for DWR 2 fallback behavior, delegating WebContext filter logic to a helper class, and introducing a new JSP custom tag for JSON serialization. Changes
Possibly related issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WebContent/WEB-INF/dwr.xml (1)
253-293: Missing import and malformed import syntax in DWR signatures block.Two syntax errors must be fixed:
- Line 279:
import br.org.scadabr.web.dwr.UsersProfilesDwris missing a semicolon- Line 292:
SystemSettingsDwr.saveSystemEventAlarmLevels(...)references a class with no import statement (missingimport com.serotonin.mango.web.dwr.SystemSettingsDwr;)Both are required for DWR 3 compatibility.
Minimal fix (inside the CDATA block)
- import br.org.scadabr.web.dwr.UsersProfilesDwr + import br.org.scadabr.web.dwr.UsersProfilesDwr; + import com.serotonin.mango.web.dwr.SystemSettingsDwr;
🤖 Fix all issues with AI agents
In @src/com/serotonin/web/taglib/DwrConvertTag.java:
- Around line 14-16: Replace usage of the internal log4j JsonUtils.quoteAsString
and raw Number.toString in DwrConvertTag by wiring in Jackson's ObjectMapper:
create/configure a shared ObjectMapper (or use existing project one) and call
writeValueAsString(...) to produce JSON strings instead of manual quoting;
before serializing Numbers, detect Float/Double and if
Double.isNaN/Double.isInfinite (or Float equivalents) handle them
deterministically (e.g., serialize as null or throw a clear
IllegalArgumentException) so invalid JSON tokens like NaN/Infinity are not
emitted; also configure the ObjectMapper/JsonFactory with a CharacterEscapes
implementation (or use StdSerializer that escapes HTML-sensitive chars) to
ensure '<', '>', '&' are escaped for safe embedding in HTML/script; finally
remove imports of org.apache.logging.log4j.core.util.JsonUtils and replace all
calls in DwrConvertTag (the methods that previously used JsonUtils.quoteAsString
and Number.toString) with the new ObjectMapper-based serialization and the
NaN/Infinity guard.
- Around line 18-47: The DwrConvertTag (class DwrConvertTag) is declared in the
TLD as tag name "convert" but no JSP usage was found; either add example
usages/docs showing where to use <escape:convert> (e.g., demonstrate import of
the escape taglib and use within a <script type="application/json"> or other
appropriate context) and a short JSP snippet in the project docs or a sample
JSP, or remove the tag and its TLD entry if truly unused; update the TLD
(escape.tld) and project docs or delete the DwrConvertTag class accordingly to
keep code and configuration in sync.
In @WebContent/WEB-INF/dwr.xml:
- Line 19: The DOCTYPE line in dwr.xml references an external DTD ("-//GetAhead
Limited//DTD Direct Web Remoting 3.0//EN" "http://getahead.org/dwr/dwr30.dtd")
which can cause XXE/SSRF and runtime failures; update your configuration to
either (a) remove the external system URL and point the DOCTYPE to a bundled
local copy of dwr30.dtd included with the application, or (b) keep the PUBLIC id
but configure your XML parser/Factory to disable external entity/DTD loading and
enable a local EntityResolver/resource resolver that returns the bundled
dwr30.dtd; ensure the change targets the XML parsing code that reads dwr.xml (or
the deployment parser) so external fetching is prevented.
🧹 Nitpick comments (1)
WebContent/resources/common.js (1)
26-43: Global$alias is helpful, but consider avoiding clobbering any pre-existing non-function$.
Right now, any existingwindow.$that isn’t a function gets overwritten; that’s usually fine, but it can still be a surprising global collision.Safer guard (only define if truly absent)
(function () { - if (typeof window.$ === 'function') { + if (window.$ != null) { return; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
WebContent/WEB-INF/lib/dwr.jaris excluded by!**/*.jar
📒 Files selected for processing (6)
WebContent/WEB-INF/dwr.xmlWebContent/resources/common.jssrc/com/serotonin/mango/web/dwr/OpcUaDataTypeConverter.javasrc/com/serotonin/mango/web/dwr/ProtocolVersionConverter.javasrc/com/serotonin/mango/web/filter/WebContextFilter.javasrc/com/serotonin/web/taglib/DwrConvertTag.java
🔇 Additional comments (3)
src/com/serotonin/mango/web/dwr/OpcUaDataTypeConverter.java (1)
12-18: The code is correct. TheconvertInbound(Class, InboundVariable)signature matches DWR 3'sEnumConvertercontract, andLocalUtil.urlDecode()is the correct method for decoding inbound values—DWR's ownLocaleConverteruses the same pattern. The@Overrideannotation confirms proper inheritance.src/com/serotonin/mango/web/dwr/ProtocolVersionConverter.java (1)
12-18: The implementation is correct. DWR 3 converters useLocalUtil.urlDecode()to decode inbound values, which is what bothProtocolVersionConverterandOpcUaDataTypeConverteralready do. The signatureconvertInbound(Class paramType, InboundVariable iv)withMarshallExceptionis consistent with the DWR 3EnumConverterSPI and follows the same pattern as other DWR 3 implementations (e.g.,LocaleConverter).src/com/serotonin/mango/web/filter/WebContextFilter.java (1)
30-54: Delegate toDwrWebContextFilteris the correct approach for DWR 3.Filter ordering in web.xml ensures WebContext is initialized before request processing. Extensive codebase usage of
WebContextFactory.get()confirms the implementation works correctly—no prior manual set/unset behavior was relied upon by application code. The simplification from custom lifecycle management to delegation is valid and reduces maintenance burden.
| import org.apache.logging.log4j.core.util.JsonUtils; | ||
| import com.serotonin.web.i18n.I18NUtils; | ||
| import com.serotonin.web.i18n.LocalizableMessage; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for project dependencies - look for pom.xml or gradle.build
git ls-files | grep -E "(pom\.xml|build\.gradle|ivy\.xml)" | head -5Repository: SCADA-LTS/Scada-LTS
Length of output: 100
🏁 Script executed:
# Check the full DwrConvertTag.java file
find . -name "DwrConvertTag.java" -type fRepository: SCADA-LTS/Scada-LTS
Length of output: 112
🏁 Script executed:
# Check for taglib descriptor files
git ls-files | grep -E "\.(tld|xml)$" | grep -i tag | head -10Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
# Check main build.gradle for log4j-core dependency
cat build.gradle | grep -A 50 -B 5 -i "log4j"Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
# Check the full DwrConvertTag.java file
cat -n src/com/serotonin/web/taglib/DwrConvertTag.javaRepository: SCADA-LTS/Scada-LTS
Length of output: 5900
🏁 Script executed:
# Read the full build.gradle to check dependencies
cat build.gradle | head -200Repository: SCADA-LTS/Scada-LTS
Length of output: 6341
🏁 Script executed:
# Search for any mention of log4j or json libraries more broadly
rg "log4j|jackson|gson|json-p|org\.json" build.gradleRepository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
# Check if there's any usage of JsonUtils in the codebase
rg "JsonUtils" --type javaRepository: SCADA-LTS/Scada-LTS
Length of output: 570
🏁 Script executed:
# Check what's in WEB-INF/lib to see if log4j-core is there
find WebContent/WEB-INF/lib -name "*log4j*" 2>/dev/null | head -20Repository: SCADA-LTS/Scada-LTS
Length of output: 291
🏁 Script executed:
# Check the lib directory as well
find lib -name "*log4j*" 2>/dev/null | head -20Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
# Search for any JSON-related libraries in the project that might be better options
find WebContent/WEB-INF/lib lib -name "*.jar" 2>/dev/null | grep -iE "(jackson|gson|json)" | head -20Repository: SCADA-LTS/Scada-LTS
Length of output: 484
🏁 Script executed:
# Check if there are any .tld files at all
find . -name "*.tld" -o -name "*.xml" | grep -i taglib 2>/dev/null | head -10Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
# Verify JsonUtils.quoteAsString escaping by checking log4j source or docs
# Check if there are any tests or usages that show what characters it escapes
rg "quoteAsString" -A 5 -B 5 --type javaRepository: SCADA-LTS/Scada-LTS
Length of output: 783
🏁 Script executed:
# Check if Jackson ObjectMapper is used elsewhere in the codebase
rg "ObjectMapper|Jackson|writeValueAsString" --type java | head -15Repository: SCADA-LTS/Scada-LTS
Length of output: 2305
Replace log4j-core's internal JsonUtils with Jackson and fix Number serialization.
The project includes log4j-core-2.16.0.jar and jackson-databind-2.17.2.jar. Using JsonUtils.quoteAsString() is problematic because:
- It's an internal API (
org.apache.logging.log4j.core.util.*), not part of log4j's public contract, while Jackson is already the established pattern in the codebase (seeSystemSettingsUtilsand test usage). Number.toString()(line 57–59) emits invalid JSON: values likeNaN,Infinity, and-Infinityare not valid JSON numbers.- Limited HTML-safe escaping: The current escaping does not encode
<,>, or&, risking XSS if JSON is embedded in HTML/script contexts without extra precautions.
Use Jackson's ObjectMapper (already integrated and available via the project's dependency chain). Jackson correctly rejects or safely handles NaN/Infinity, and allows configuration of proper JSON escaping for both syntax and HTML contexts.
🤖 Prompt for AI Agents
In @src/com/serotonin/web/taglib/DwrConvertTag.java around lines 14 - 16,
Replace usage of the internal log4j JsonUtils.quoteAsString and raw
Number.toString in DwrConvertTag by wiring in Jackson's ObjectMapper:
create/configure a shared ObjectMapper (or use existing project one) and call
writeValueAsString(...) to produce JSON strings instead of manual quoting;
before serializing Numbers, detect Float/Double and if
Double.isNaN/Double.isInfinite (or Float equivalents) handle them
deterministically (e.g., serialize as null or throw a clear
IllegalArgumentException) so invalid JSON tokens like NaN/Infinity are not
emitted; also configure the ObjectMapper/JsonFactory with a CharacterEscapes
implementation (or use StdSerializer that escapes HTML-sensitive chars) to
ensure '<', '>', '&' are escaped for safe embedding in HTML/script; finally
remove imports of org.apache.logging.log4j.core.util.JsonUtils and replace all
calls in DwrConvertTag (the methods that previously used JsonUtils.quoteAsString
and Number.toString) with the new ObjectMapper-based serialization and the
NaN/Infinity guard.
| public class DwrConvertTag extends TagSupport { | ||
|
|
||
| private Object obj; | ||
|
|
||
| public DwrConvertTag() { | ||
| // no-op | ||
| } | ||
|
|
||
| public void setObj(Object obj) { | ||
| this.obj = obj; | ||
| } | ||
|
|
||
| @Override | ||
| public int doStartTag() throws JspException { | ||
| try { | ||
| JspWriter out = pageContext.getOut(); | ||
| String json = toJson(obj); | ||
| out.write(json); | ||
| return SKIP_BODY; | ||
| } catch (IOException e) { | ||
| throw new JspException("Error writing DwrConvertTag JSON content", e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void release() { | ||
| super.release(); | ||
| this.obj = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tag registration and usage
rg -n --fixed-strings "DwrConvertTag" -C2
rg -n --glob='**/*.tld' -C2 "com.serotonin.web.taglib.DwrConvertTag"
rg -n --glob='**/*.jsp' -C2 "dwrConvert" "DwrConvertTag"Repository: SCADA-LTS/Scada-LTS
Length of output: 1917
🏁 Script executed:
#!/bin/bash
# View the full TLD descriptor entry for the convert tag
cat -n WebContent/WEB-INF/tld/escape.tld | sed -n '550,570p'
# Search for actual usage in JSP files
fd --type f -e jsp -e jspx | head -20 | xargs -I {} rg -l "convert" {} 2>/dev/nullRepository: SCADA-LTS/Scada-LTS
Length of output: 839
🏁 Script executed:
#!/bin/bash
# Find JSP files and search for the convert tag usage
# The tag would typically be used with a namespace prefix like <escape:convert ...>
find . -name "*.jsp" -o -name "*.jspx" | xargs rg -l "convert" 2>/dev/null | head -10
# Also search explicitly for the tag pattern
rg -n "escape:convert|<convert" --type jsp 2>/dev/null | head -20Repository: SCADA-LTS/Scada-LTS
Length of output: 420
🏁 Script executed:
#!/bin/bash
# Search for escape:convert tag usage in JSP files
rg -n "escape:convert" --type jsp 2>/dev/null
# Also check for the tag declaration in JSP files
rg -n "<%@ taglib.*escape" --type jsp 2>/dev/null | head -5Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if any JSP files actually import the escape taglib
rg -n "taglib.*escape" --type jsp 2>/dev/null | head -10
# Search for any test files or usage
find . -name "*Test*.java" -o -name "*test*.java" | xargs rg -l "DwrConvertTag\|escape:convert" 2>/dev/null
# Check for documentation or examples
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs rg -l "DwrConvertTag\|dwrConvert" 2>/dev/nullRepository: SCADA-LTS/Scada-LTS
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Simpler search for taglib imports
rg "taglib" --type jsp 2>/dev/null | grep -i escape | head -10
# Check for any JSP importing the escape TLD
rg -n "escape.tld\|uri=\".*escape" --type jsp 2>/dev/null | head -10
# Count occurrences of "convert" in JSP files to understand context
rg -n "convert" --type jsp 2>/dev/null | head -20Repository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check web.xml for TLD references
rg -n "escape.tld" --type xml 2>/dev/null
# Check if there's a web.xml file
find . -name "web.xml" -type f | head -3
# Look for taglib-uri declarations
rg -n "taglib-uri" --type xml 2>/dev/null | head -20
# Check escape.tld header to see URI
head -20 WebContent/WEB-INF/tld/escape.tldRepository: SCADA-LTS/Scada-LTS
Length of output: 963
🏁 Script executed:
#!/bin/bash
# List all TLD files
ls -la WebContent/WEB-INF/tld/
# Verify which file contains the convert tag
rg -l "convert" WebContent/WEB-INF/tld/Repository: SCADA-LTS/Scada-LTS
Length of output: 335
🏁 Script executed:
#!/bin/bash
# Check if escape.tld is referenced in web.xml
cat WebContent/WEB-INF/web.xml | grep -i "escape\|serotonin" | head -20
# Check the URI declared in escape.tld
head -15 WebContent/WEB-INF/tld/escape.tld | grep -i uri
# Search for JSP imports with the URI pattern
rg -n "http://www.serotoninsoftware.com" --type jsp 2>/dev/nullRepository: SCADA-LTS/Scada-LTS
Length of output: 1757
🏁 Script executed:
#!/bin/bash
# Check escape.tld for its declared URI
grep -n "uri" WebContent/WEB-INF/tld/escape.tld | head -3
# Search JSP files for taglib imports with escape or serotonin URIs
rg -n "<%@ taglib" --type jsp 2>/dev/null | head -30Repository: SCADA-LTS/Scada-LTS
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for taglib directives in JSP using different pattern
rg "<%@.*taglib" --type jsp 2>/dev/null | head -20
# Also search for URI mappings in web.xml
grep -n "taglib-uri\|taglib-location" WebContent/WEB-INF/web.xml 2>/dev/nullRepository: SCADA-LTS/Scada-LTS
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Final check: search for any reference to "dwrConvert" or "dwr" that might indicate usage
rg -i "dwr" --type jsp --type java 2>/dev/null | head -20
# Also check comments in the TLD
grep -A 2 -B 2 "convert" WebContent/WEB-INF/tld/escape.tldRepository: SCADA-LTS/Scada-LTS
Length of output: 215
The tag IS properly registered in the TLD but appears unused in JSP pages.
The tag is correctly wired in WebContent/WEB-INF/tld/escape.tld (lines 552–561) with the name convert, required obj attribute, and proper configuration. However, no usage of this tag (e.g., <escape:convert> or imports of the escape taglib) was found in any JSP files. If this tag is genuinely needed, add JSP usage examples or documentation specifying its output context (e.g., only within <script type="application/json"> blocks). If unused, consider removing it.
🤖 Prompt for AI Agents
In @src/com/serotonin/web/taglib/DwrConvertTag.java around lines 18 - 47, The
DwrConvertTag (class DwrConvertTag) is declared in the TLD as tag name "convert"
but no JSP usage was found; either add example usages/docs showing where to use
<escape:convert> (e.g., demonstrate import of the escape taglib and use within a
<script type="application/json"> or other appropriate context) and a short JSP
snippet in the project docs or a sample JSP, or remove the tag and its TLD entry
if truly unused; update the TLD (escape.tld) and project docs or delete the
DwrConvertTag class accordingly to keep code and configuration in sync.
WebContent/WEB-INF/dwr.xml
Outdated
| along with this program. If not, see http://www.gnu.org/licenses/. | ||
| --> | ||
| <!DOCTYPE dwr PUBLIC "-//GetAhead Limited//DTD Direct Web Remoting 2.0//EN" "http://getahead.org/dwr/dwr20.dtd"> | ||
| <!DOCTYPE dwr PUBLIC "-//GetAhead Limited//DTD Direct Web Remoting 3.0//EN" "http://getahead.org/dwr/dwr30.dtd"> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
DWR 3.0 Direct Web Remoting dwr.xml DOCTYPE DTD reference specification
💡 Result:
Use this DOCTYPE in dwr.xml for DWR 3.0:
(Older examples also reference "http://getahead.org/dwr/dwr30.dtd" with the same public id.) [1][2]
🌐 Web query:
getahead.org dwr30.dtd external DTD remote fetch best practices
💡 Result:
Short answer: avoid letting XML parsers fetch external DTDs (like getahead.org/dwr30.dtd) from the network. Either disable external DTD/ENTITY loading or resolve DTDs locally and tightly control when/what the parser may fetch. Key practices:
- Disable DTDs or external-entity resolution in your parser (best): disallow DOCTYPE, disable external entities and external DTD loading. [1][2][3]
- If DTDs are required, use a local, versioned copy (vendor bundle or internal repository) and configure the parser to resolve only that local store (catalog resolver/whitelist). Do not permit arbitrary HTTP fetches. [6][2]
- Set parser attributes to block network access: e.g. set XMLConstants.ACCESS_EXTERNAL_DTD and ACCESS_EXTERNAL_SCHEMA to "" or set apache feature http://apache.org/xml/features/nonvalidating/load-external-dtd to false. Also enable secure-processing features. [2][3][4]
- Harden entity handling: disable entity expansion or limit expansions to prevent Billion‑Laughs/DoS. [1][3]
- Network/operational controls: block outbound HTTP(S) from parse-time processes (egress rules) or allow only trusted hosts; log/alert any attempts to fetch external DTDs. [1][5]
- Input validation and safe libraries: treat incoming XML as untrusted, validate against schemas that do not require external fetches and prefer parsers/libraries built with XXE protections (or use JSON instead if feasible). [1][3][5]
- Fail-safe defaults and testing: audit code for DOCTYPE/entity handling, add unit tests and security tests that exercise XXE/SSRF and remote-DTD cases. Monitor dependencies for references to remote DTDs like getahead.org/dwr30.dtd and replace them. [1][4]
Sources: OWASP XML-External-Entity cheat sheet; Oracle Java XML guidance; Semgrep/secure-coding guidance; SecureCodingPractices article; general DTD pitfalls. [1][2][3][4][5]
References
[1] OWASP XML External Entity Prevention Cheat Sheet.
[2] Oracle — Java Technology and XML (reduce external entity/DTD cost and controls).
[3] Semgrep / XML Security guidance (disable DTDs / external entities).
[4] SecureCodingPractices — Avoid XXE in Java parsers (features/attributes examples).
[5] Article on DTD pitfalls and availability/performance/security trade-offs.
Disable external DTD loading or use local DTD resolution.
While the PUBLIC identifier is correct for DWR 3.0, fetching the DTD from an external URL (http://getahead.org/dwr/dwr30.dtd) introduces security risks (XXE/SSRF vulnerabilities) and operational fragility (network failures, offline/restricted environments). Configure your XML parser to disable external DTD/entity loading, or replace with a local, versioned copy of the DTD.
🤖 Prompt for AI Agents
In @WebContent/WEB-INF/dwr.xml at line 19, The DOCTYPE line in dwr.xml
references an external DTD ("-//GetAhead Limited//DTD Direct Web Remoting
3.0//EN" "http://getahead.org/dwr/dwr30.dtd") which can cause XXE/SSRF and
runtime failures; update your configuration to either (a) remove the external
system URL and point the DOCTYPE to a bundled local copy of dwr30.dtd included
with the application, or (b) keep the PUBLIC id but configure your XML
parser/Factory to disable external entity/DTD loading and enable a local
EntityResolver/resource resolver that returns the bundled dwr30.dtd; ensure the
change targets the XML parsing code that reads dwr.xml (or the deployment
parser) so external fetching is prevented.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.