Features/thymeleaf module #60
Conversation
WalkthroughAdds Thymeleaf UI module and JSP alternative, migrates Maven modules and Jetty to EE10 BOM, introduces JettyCustomizer SPI, makes WebSocket path configurable, refactors server startup into a Jetty Server subclass with Spring-driven web app creation, removes a DateTime utility, and adds many Thymeleaf templates, static assets and Docker tooling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as Main
participant App as Application
participant Server as JettyServer (extends Server)
participant Ctx as SteveAppContext
participant Jetty as Jetty Core
participant Spring as Spring Contexts
Main->>App: new Application(config, logRetriever)
Main->>App: start()
App->>App: print log file path (if present)
App->>Server: serverStart()
activate Server
Server->>Server: doStart()
Server->>Jetty: configure HTTP/HTTPS connectors
Server->>Jetty: install WS defaults (idle timeout, max msg size)
Server->>Ctx: createHandlers()
activate Ctx
Ctx->>Spring: create root context (+JettyCustomizer, config beans)
Ctx->>Spring: create web context (DispatcherServlet, Security)
Ctx-->>Server: HandlerCollection
deactivate Ctx
Server-->>App: started
deactivate Server
App->>Server: join()
sequenceDiagram
autonumber
participant Client as CP (WebSocket client)
participant HTTP as HTTP Server
participant HS as OcppWebSocketHandshakeHandler
participant Conf as SteveConfiguration.Paths
Client->>HTTP: GET /{wsPathInfix}/.../{chargeBoxId}
HTTP->>HS: doHandshake(request)
HS->>Conf: getWsPathInfix()
HS->>HS: getLastBitFromUrl(path, requestURI)
HS-->>HTTP: accept with extracted chargeBoxId
sequenceDiagram
autonumber
participant User as Manager UI
participant Page as Operation Page
participant Ajax as /manager/ajax/*
User->>Page: Select Charge Point
Page->>Ajax: GET {cp}/connectorIds
Ajax-->>Page: [id...]
Page->>Page: populate #connectorId options (enable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
juherr
force-pushed
the
features/thymeleaf-module
branch
from
46ae970 to
f3554fc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 93
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
steve/src/test/java/de/rwth/idsg/steve/OperationalTestSoapOCPP16.java (2)
573-585: Bug: contradicting assertion causes IndexOutOfBoundsException.You assert the sampled values list is empty and then access index 0. Flip the assertion (and, since we always create a single SampledValue per MeterValue, assert size 1 for clarity).
Apply:
- assertThat(sampledValues).isEmpty(); + assertThat(sampledValues).hasSize(1); var thisValueFound = false; // and check, if it can be found in the DB for (var values : details.getValues()) { if (values.getValue().equals(sampledValues.get(0).getValue())) { thisValueFound = true; break; } }
120-131: Ensure Javadoc link resolves for OcppWebSocketUpgrader
Add an import for the actual package of OcppWebSocketUpgrader or switch the tag to its fully qualified name (e.g.{@link com.example.pkg.OcppWebSocketUpgrader}) so that doclint can find it.steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/ConcurrentWebSocketHandler.java (2)
63-67: Use per-session buffer limit and avoid recreating magic numbers here.Compute buffer size with the helper and pass it to the decorator.
- return sessions.computeIfAbsent( - session.getId(), - s -> new ConcurrentWebSocketSessionDecorator(session, SEND_TIME_LIMIT, BUFFER_SIZE_LIMIT)); + return sessions.computeIfAbsent( + session.getId(), + s -> new ConcurrentWebSocketSessionDecorator(session, SEND_TIME_LIMIT, calcBufferLimit(session)));
59-61: Guard against null decorated session on close
Fallback to the rawsessionifsessions.remove(...)returns null to prevent NPE:- this.onClose(sessions.remove(session.getId()), closeStatus); + var decorated = sessions.remove(session.getId()); + this.onClose(decorated != null ? decorated : session, closeStatus);steve/src/main/java/de/rwth/idsg/steve/SteveAppContext.java (1)
145-156: Compilation error:HashSet.newHashSet(…)doesn’t exist in JDK.Use the constructor (or Guava’s
Sets.newHashSetWithExpectedSizeif available).Apply:
- var redirectSet = HashSet.<String>newHashSet(3); + var redirectSet = new HashSet<String>(3);Also avoid generating
//whenpathis/:- if (!"".equals(path)) { + if (!"".equals(path) && !"/".equals(path)) { redirectSet.add(path + "/"); }
| <dependency> | ||
| <groupId>ch.qos.logback</groupId> | ||
| <artifactId>logback-classic</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t pin a logging backend in core; make logback non-transitive
Core libraries should depend on slf4j-api only. Make logback optional so downstream runnable modules decide the backend.
Apply:
- <dependency>
- <groupId>ch.qos.logback</groupId>
- <artifactId>logback-classic</artifactId>
- </dependency>
+ <dependency>
+ <groupId>ch.qos.logback</groupId>
+ <artifactId>logback-classic</artifactId>
+ <optional>true</optional>
+ </dependency>🏁 Script executed:
#!/bin/bash
# Check for multiple SLF4J bindings across modules
rg -n -C2 -g '**/pom.xml' 'logback-classic|slf4j-simple|log4j-to-slf4j|jul-to-slf4j'Length of output: 1065
🏁 Script executed:
#!/bin/bash
# Check for explicit slf4j-api declaration in steve-core/pom.xml
rg -n -C2 '<artifactId>slf4j-api</artifactId>' steve-core/pom.xmlLength of output: 210
Mark logback-classic as optional in steve-core/pom.xml
Replace the direct logback binding so core only brings in slf4j-api by default:
--- steve-core/pom.xml
@@ -61,3 +61,5 @@
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
+ <optional>true</optional>
</dependency>📝 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.
| <dependency> | |
| <groupId>ch.qos.logback</groupId> | |
| <artifactId>logback-classic</artifactId> | |
| </dependency> | |
| <dependency> | |
| <groupId>ch.qos.logback</groupId> | |
| <artifactId>logback-classic</artifactId> | |
| <optional>true</optional> | |
| </dependency> |
🤖 Prompt for AI Agents
In steve-core/pom.xml around lines 61 to 64, the pom currently declares
logback-classic as a regular dependency which forces a concrete SLF4J binding
into consumers; mark that dependency optional so the module only brings in
slf4j-api by default by adding an <optional>true</optional> element to the
logback-classic dependency (or remove it and let consumers choose their
binding), leaving slf4j-api as the provided API.
| public String getLocation() { | ||
| return contextPath + managerMapping + "/home"; | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle nullable contextPath and slash normalization in getLocation().
Current code risks "null..." and double slashes.
- public String getLocation() {
- return contextPath + managerMapping + "/home";
- }
+ public String getLocation() {
+ String ctx = (contextPath == null || "/".equals(contextPath)) ? "" : contextPath;
+ if (!managerMapping.startsWith("/")) {
+ // Ensure managerMapping is absolute
+ ctx = ctx + "/";
+ }
+ String base = (ctx.endsWith("/")) ? ctx.substring(0, ctx.length() - 1) : ctx;
+ return base + managerMapping + "/home";
+ }Add unit tests for contextPath = null, "", and "/foo".
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/SteveConfiguration.java around
lines 52-55, getLocation() currently concatenates contextPath and managerMapping
directly which can produce "null" or double slashes; normalize by treating a
null contextPath as empty string, strip any trailing slash from contextPath,
ensure it has a leading slash only if non-empty, and join pieces so you never
produce duplicate slashes (e.g., ensure managerMapping begins with a single '/'
or use a safe join). Update getLocation() to apply this normalization and always
return a canonical path ending with "/home". Add unit tests covering contextPath
= null, "" and "/foo" to assert expected outputs (no "null" and no double
slashes).
| public String getWsPathInfix() { | ||
| return routerEndpointPath + websocketMapping; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize join for WebSocket path infix.
Avoid missing or duplicate slashes.
- public String getWsPathInfix() {
- return routerEndpointPath + websocketMapping;
- }
+ public String getWsPathInfix() {
+ String left = routerEndpointPath.endsWith("/") ? routerEndpointPath.substring(0, routerEndpointPath.length()-1) : routerEndpointPath;
+ String right = websocketMapping.startsWith("/") ? websocketMapping : "/" + websocketMapping;
+ return left + right;
+ }📝 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.
| public String getWsPathInfix() { | |
| return routerEndpointPath + websocketMapping; | |
| } | |
| public String getWsPathInfix() { | |
| String left = routerEndpointPath.endsWith("/") | |
| ? routerEndpointPath.substring(0, routerEndpointPath.length() - 1) | |
| : routerEndpointPath; | |
| String right = websocketMapping.startsWith("/") | |
| ? websocketMapping | |
| : "/" + websocketMapping; | |
| return left + right; | |
| } |
🤖 Prompt for AI Agents
In steve-core/src/main/java/de/rwth/idsg/steve/SteveConfiguration.java around
lines 56 to 58, the getWsPathInfix() simply concatenates routerEndpointPath and
websocketMapping which can produce missing or duplicate slashes; fix by
normalizing both parts before joining: trim any trailing slash from
routerEndpointPath and any leading slash from websocketMapping, then return the
two parts joined with a single "/" between them (ensure the result starts with
"/" if routerEndpointPath is empty or originally started with "/"); also handle
null or empty websocketMapping gracefully by returning the normalized
routerEndpointPath or "/" as appropriate.
| $(() => { | ||
| /** @type {JQuery} */ | ||
| const $menuItems = $('#dm-menu a'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Delegate clicks and initialize from URL hash.
Avoid stale bindings when menu items are injected later and show the correct panel on first load.
- const $menuItems = $('#dm-menu a');
+ const $menuItems = $('#dm-menu a');
@@
- $menuItems.on('click', onMenuClick);
+ $('#dm-menu').on('click', 'a', onMenuClick);
+
+ // Initial state: from hash or first item
+ const initial =
+ (window.location.hash || '').replace(/^#/, '') ||
+ $menuItems.first().attr('name') ||
+ ($menuItems.first().attr('href') || '').replace(/^#/, '');
+ if (initial) {
+ const $panel = $(`#${initial}`);
+ if ($panel.length) {
+ $panel.show().siblings().hide();
+ $menuItems.removeClass('highlight');
+ $menuItems
+ .filter(`[name="${initial}"],[href="#${initial}"]`)
+ .addClass('highlight');
+ }
+ }Also applies to: 45-46
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/static/js/script.js around lines 23-26
(and also apply same change to lines 45-46), the click handlers are bound
directly to $('#dm-menu a') which can become stale if menu items are injected
later and the initial panel isn't selected from the URL hash; change to
delegated event handling (e.g., bind click to a stable parent like '#dm-menu' or
document) so dynamically injected anchors are handled, and add initialization
logic on load to read location.hash (or default) and trigger the same panel
activation code so the correct panel is shown on first load.
| const onMenuClick = (e) => { | ||
| e.preventDefault(); | ||
|
|
||
| const attr = $(e.currentTarget).attr('name'); | ||
| const $attr = $(`#${attr}`); | ||
|
|
||
| $attr.siblings().hide(); | ||
| $attr.show(); | ||
|
|
||
| $menuItems.removeClass('highlight'); | ||
| $link.addClass('highlight'); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix undefined $link; harden target lookup and guard missing panels.
$current code references $link without defining it and assumes a valid name/panel. Patch below fixes the crash, supports href="#id" fallback, guards empty targets, and syncs the URL hash.
- const onMenuClick = (e) => {
- e.preventDefault();
-
- const attr = $(e.currentTarget).attr('name');
- const $attr = $(`#${attr}`);
-
- $attr.siblings().hide();
- $attr.show();
-
- $menuItems.removeClass('highlight');
- $link.addClass('highlight');
- };
+ const onMenuClick = (e) => {
+ e.preventDefault();
+ const $link = $(e.currentTarget);
+ const attr =
+ $link.attr('name') ||
+ ($link.attr('href') || '').replace(/^#/, '');
+ if (!attr) return;
+ const $panel = $(`#${attr}`);
+ if (!$panel.length) return;
+ $panel.show().siblings().hide();
+ $menuItems.removeClass('highlight');
+ $link.addClass('highlight');
+ if (location && location.hash !== `#${attr}`) location.hash = attr;
+ };📝 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.
| const onMenuClick = (e) => { | |
| e.preventDefault(); | |
| const attr = $(e.currentTarget).attr('name'); | |
| const $attr = $(`#${attr}`); | |
| $attr.siblings().hide(); | |
| $attr.show(); | |
| $menuItems.removeClass('highlight'); | |
| $link.addClass('highlight'); | |
| }; | |
| const onMenuClick = (e) => { | |
| e.preventDefault(); | |
| const $link = $(e.currentTarget); | |
| const attr = | |
| $link.attr('name') || | |
| ($link.attr('href') || '').replace(/^#/, ''); | |
| if (!attr) return; | |
| const $panel = $(`#${attr}`); | |
| if (!$panel.length) return; | |
| $panel.show().siblings().hide(); | |
| $menuItems.removeClass('highlight'); | |
| $link.addClass('highlight'); | |
| if (location && location.hash !== `#${attr}`) location.hash = attr; | |
| }; |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/static/js/script.js around lines 32 to
43, the click handler references an undefined $link, assumes a valid name-based
target, and doesn't guard missing panels; fix by deriving $link from the event
target (e.g. $(e.currentTarget)), resolve the target id by trying the element's
name attribute first and falling back to parsing its href hash, check that the
resolved target selector exists before operating (return early if not), then
perform the siblings hide/show and menu highlight using the defined $link, and
finally update location.hash to keep the URL in sync.
| <div class="content"> | ||
| <div th:if="${param.error}" class="error">Your name or password is incorrect.</div> | ||
| <section><span>Sign In</span></section> | ||
| <form method="POST" th:action="@{/manager/signin}"> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify login endpoint matches Spring Security config.
Ensure the authentication filter’s loginProcessingUrl is set to “/manager/signin” (or map this form to the configured URL).
🏁 Script executed:
#!/bin/bash
# Check security config for loginProcessingUrl or formLogin().loginProcessingUrl(...)
rg -nP --type=java -C3 '\bformLogin\s*\(|loginProcessingUrl\s*\('
rg -n '/manager/signin' -g '!**/target/**'Length of output: 1866
Configure loginProcessingUrl to match form action
Add .loginProcessingUrl(prefix + "/signin") to the formLogin config so Spring Security handles POST /manager/signin instead of default /login.
Location: steve/src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java:130
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/signin.html around line 12,
the form posts to /manager/signin but the Spring Security formLogin uses the
default processing URL; update the formLogin configuration in
steve/src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java (around
line 130) to call .loginProcessingUrl(prefix + "/signin") (or the exact manager
prefix used) so Spring Security handles POST /manager/signin instead of the
default /login.
| <tr><td>Name:</td><td><input type="text" name="username" id="username" required /></td></tr> | ||
| <tr><td>Password:</td><td><input type="password" name="password" id="password" required /></td></tr> | ||
| <tr><td></td><td id="add_space"><input type="submit" value="Sign In"></td></tr> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add labels and autocomplete hints; set autofocus.
Improves accessibility and UX on credential fields.
- <tr><td>Name:</td><td><input type="text" name="username" id="username" required /></td></tr>
- <tr><td>Password:</td><td><input type="password" name="password" id="password" required /></td></tr>
- <tr><td></td><td id="add_space"><input type="submit" value="Sign In"></td></tr>
+ <tr>
+ <td><label for="username">Username:</label></td>
+ <td><input type="text" name="username" id="username" required autocomplete="username" autofocus /></td>
+ </tr>
+ <tr>
+ <td><label for="password">Password:</label></td>
+ <td><input type="password" name="password" id="password" required autocomplete="current-password" /></td>
+ </tr>
+ <tr><td></td><td id="add_space"><input type="submit" value="Sign In"></td></tr>📝 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.
| <tr><td>Name:</td><td><input type="text" name="username" id="username" required /></td></tr> | |
| <tr><td>Password:</td><td><input type="password" name="password" id="password" required /></td></tr> | |
| <tr><td></td><td id="add_space"><input type="submit" value="Sign In"></td></tr> | |
| <tr> | |
| <td><label for="username">Username:</label></td> | |
| <td><input type="text" name="username" id="username" required autocomplete="username" autofocus /></td> | |
| </tr> | |
| <tr> | |
| <td><label for="password">Password:</label></td> | |
| <td><input type="password" name="password" id="password" required autocomplete="current-password" /></td> | |
| </tr> | |
| <tr><td></td><td id="add_space"><input type="submit" value="Sign In"></td></tr> |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/signin.html around lines 14
to 16, the username and password inputs lack associated <label> elements,
autocomplete attributes, and autofocus on the username field; update the markup
to add <label for="username"> and <label for="password"> referencing the
respective input ids, add autocomplete="username" on the username input and
autocomplete="current-password" on the password input, and set autofocus on the
username input so screen readers and browsers can correctly associate labels,
enable credential autofill, and focus the username field on page load.
| $(document).ready( | ||
| setTimeout(function(){ | ||
| window.location.reload(1); | ||
| }, 5000)); | ||
| </script> |
There was a problem hiding this comment.
Bug: $(document).ready called with a number, not a function.
setTimeout(...) returns an ID; pass a function to ready that sets the timer.
- $(document).ready(
- setTimeout(function(){
- window.location.reload(1);
- }, 5000));
+ $(document).ready(function () {
+ setTimeout(function () {
+ window.location.reload(true);
+ }, 5000);
+ });📝 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.
| $(document).ready( | |
| setTimeout(function(){ | |
| window.location.reload(1); | |
| }, 5000)); | |
| </script> | |
| $(document).ready(function () { | |
| setTimeout(function () { | |
| window.location.reload(true); | |
| }, 5000); | |
| }); |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/taskResult.html around lines
12 to 16, the code calls $(document).ready with the return value of setTimeout
(a numeric timer ID) instead of a function; change it so ready receives a
function that itself calls setTimeout (e.g., pass an anonymous function or use
$(function(){ ... }) that invokes setTimeout to reload after 5000ms) so that a
function is executed when the document is ready rather than passing the timer
ID.
| model.addAttribute("systemTime", Instant.now()); | ||
| model.addAttribute("systemTimeZone", ZoneId.systemDefault()); | ||
| model.addAttribute("releaseReport", releaseCheckService.check()); | ||
| log.warn(info.toString()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid noisy WARN and use structured logging
Logging EndpointInfo on every GET at WARN will spam logs and may leak environment details. Prefer DEBUG and parameterized logging.
Apply:
- log.warn(info.toString());
+ log.debug("EndpointInfo: {}", info);📝 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.
| log.warn(info.toString()); | |
| log.debug("EndpointInfo: {}", info); |
🤖 Prompt for AI Agents
In
steve-ui/src/main/java/de/rwth/idsg/steve/web/controller/AboutSettingsController.java
around line 81, replace the noisy log.warn(info.toString()) with a parameterized
debug log and avoid calling toString() directly; change to
log.debug("EndpointInfo: {}", info) (or log.debug("EndpointInfo: {}",
maskedInfo) if any fields are sensitive), ensure expensive info serialization is
guarded by isDebugEnabled() or done via a masked DTO that excludes environment
secrets, and remove or lower the log level so GET requests no longer spam WARN
logs or leak environment details.
| "/webjars/**", | ||
| config.getPaths().getSoapMapping() + "/**", | ||
| OcppWebSocketConfiguration.PATH_INFIX + "**", | ||
| config.getPaths().getWsPathInfix() + "/**", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limit WS permitAll to GET only
Reduce exposure by restricting to the WebSocket handshake method.
Apply:
- config.getPaths().getWsPathInfix() + "/**",
+ HttpMethod.GET, config.getPaths().getWsPathInfix() + "/**",📝 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.
| config.getPaths().getWsPathInfix() + "/**", | |
| HttpMethod.GET, config.getPaths().getWsPathInfix() + "/**", |
🤖 Prompt for AI Agents
In steve/src/main/java/de/rwth/idsg/steve/config/SecurityConfiguration.java
around line 74, the WebSocket handshake path currently uses a blanket permitAll
for all methods; narrow it to only allow GET (the handshake) by changing the
matcher to an HTTP-method-specific one (e.g. use antMatchers(HttpMethod.GET,
config.getPaths().getWsPathInfix() + "/**").permitAll() or equivalent), and add
the required import for HttpMethod if missing.
| registry.addHandler( | ||
| handshakeHandler.getDummyWebSocketHandler(), | ||
| config.getPaths().getWsPathInfix() + "/*") | ||
| .setHandshakeHandler(handshakeHandler) | ||
| .setAllowedOrigins("*"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restrict WebSocket allowed origins.
setAllowedOrigins("*") enables any origin. Prefer explicit origins or config-driven patterns.
- .setAllowedOrigins("*");
+ // Prefer patterns or a configured list
+ .setAllowedOriginPatterns(config.getSecurity().getAllowedWsOrigins());If no such config exists, introduce one or restrict to known UI origins.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/config/OcppWebSocketConfiguration.java
around lines 71 to 76, the WebSocket mapping currently calls
setAllowedOrigins("*"), which allows any origin; replace this with a
config-driven, explicit allowed-origins value (e.g. config.getAllowedOrigins())
or introduce a new property if it doesn't exist, validate/parse it
(comma-separated list or patterns) and pass the resulting origin array to
setAllowedOrigins; ensure a sensible default (restrict to known UI host(s) or
fail fast) and avoid using the wildcard.
| public static String getLastBitFromUrl(String path, String input) { | ||
| if (Strings.isNullOrEmpty(input)) { | ||
| return ""; | ||
| } | ||
|
|
||
| var substring = OcppWebSocketConfiguration.PATH_INFIX; | ||
|
|
||
| var index = input.indexOf(substring); | ||
| var index = input.indexOf(path); | ||
| if (index == -1) { | ||
| return ""; | ||
| } | ||
| return input.substring(index + substring.length()); | ||
| return input.substring(index + path.length()); | ||
| } |
There was a problem hiding this comment.
getLastBitFromUrl returns more than the last segment when extra path parts exist
Ensure only the next segment after the infix is returned (the chargeBoxId), ignoring trailing segments.
- public static String getLastBitFromUrl(String path, String input) {
+ public static String getLastBitFromUrl(String path, String input) {
if (Strings.isNullOrEmpty(input)) {
return "";
}
-
- var index = input.indexOf(path);
- if (index == -1) {
- return "";
- }
- return input.substring(index + path.length());
+ int start = input.indexOf(path);
+ if (start == -1) {
+ return "";
+ }
+ int from = start + path.length();
+ // Skip any leading slash just in case
+ if (from < input.length() && input.charAt(from) == '/') {
+ from++;
+ }
+ int next = input.indexOf('/', from);
+ return next == -1 ? input.substring(from) : input.substring(from, next);
}📝 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.
| public static String getLastBitFromUrl(String path, String input) { | |
| if (Strings.isNullOrEmpty(input)) { | |
| return ""; | |
| } | |
| var substring = OcppWebSocketConfiguration.PATH_INFIX; | |
| var index = input.indexOf(substring); | |
| var index = input.indexOf(path); | |
| if (index == -1) { | |
| return ""; | |
| } | |
| return input.substring(index + substring.length()); | |
| return input.substring(index + path.length()); | |
| } | |
| public static String getLastBitFromUrl(String path, String input) { | |
| if (Strings.isNullOrEmpty(input)) { | |
| return ""; | |
| } | |
| int start = input.indexOf(path); | |
| if (start == -1) { | |
| return ""; | |
| } | |
| int from = start + path.length(); | |
| // Skip any leading slash just in case | |
| if (from < input.length() && input.charAt(from) == '/') { | |
| from++; | |
| } | |
| int next = input.indexOf('/', from); | |
| return next == -1 | |
| ? input.substring(from) | |
| : input.substring(from, next); | |
| } |
🤖 Prompt for AI Agents
In
steve-ocpp-websocket/src/main/java/de/rwth/idsg/steve/ocpp/ws/OcppWebSocketHandshakeHandler.java
around lines 136 to 146, getLastBitFromUrl currently returns everything after
the infix path, which can include multiple trailing segments; change it to
return only the next path segment (the chargeBoxId). After locating the infix,
take the substring starting at index + path.length(), trim any leading '/'
characters, then split that remainder on '/' and return the first element (or
empty string if none); preserve the existing null/empty checks and handle cases
where the infix is not present by returning empty string.
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-webmvc</artifactId> | ||
| <exclusions> | ||
| <!-- http://docs.spring.io/spring/docs/current/spring-framework-reference/html/overview.html#overview-not-using-commons-logging --> | ||
| <exclusion> | ||
| <groupId>commons-logging</groupId> | ||
| <artifactId>commons-logging</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <groupId>org.springframework</groupId> | ||
| <artifactId>spring-jcl</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Revisit exclusions of commons-logging and spring-jcl.
Spring uses spring-jcl for logging. Excluding it may break logging if no alternative bridge is present. Verify it’s still on the classpath via another dependency or drop this exclusion.
🏁 Script executed:
#!/bin/bash
set -e
# search all POM XMLs for spring-jcl, commons-logging, jcl-over-slf4j, slf4j-api, or logback-classic
rg -nP -C2 --glob '**/*.xml' '<artifactId>(spring-jcl|commons-logging|jcl-over-slf4j|slf4j-api|logback-classic)</artifactId>'Length of output: 4030
Restore JCL support in steve-ui-thymeleaf/pom.xml
- The
<exclusion>ofspring-jcl(andcommons-logging) from Spring-WebMVC removes Spring’s built-in JCL implementation without providing a JCL‐to‐SLF4J bridge. - Either drop those exclusions or add
org.slf4j:jcl-over-slf4jto this module to ensure JCL API classes remain on the classpath.
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/pom.xml around lines 32-45 the POM currently excludes
commons-logging and spring-jcl from the spring-webmvc dependency which removes
Spring’s JCL implementation; either remove those two <exclusion> entries so
spring-jcl remains on the classpath, or keep the exclusions but add an explicit
dependency on org.slf4j:jcl-over-slf4j (compile scope) to provide a JCL-to-SLF4J
bridge; update the dependencies section accordingly and ensure only one of these
two fixes is applied to avoid duplicate JCL implementations.
| <dependency> | ||
| <groupId>org.webjars</groupId> | ||
| <artifactId>jquery</artifactId> | ||
| <version>2.0.3</version> | ||
| </dependency> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Upgrade jQuery; 2.0.3 has known vulnerabilities.
Move to jQuery 3.x (and ensure compatibility of jQuery UI and plugins). Confirm via SCA before merging.
Upgrade jQuery dependency to 3.x
In steve-ui-thymeleaf/pom.xml (lines 66–70), bump org.webjars:jquery from 2.0.3 to the latest 3.x (e.g. 3.7.0), verify jQuery UI/plugins still work, and re-run SCA to confirm all vulnerabilities are resolved.
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/pom.xml around lines 66 to 70, the pom currently pins
org.webjars:jquery to 2.0.3; update the <version> to the latest 3.x (for example
3.7.0), then run your build and QA: verify any jQuery UI/plugins used in the
project are compatible and update their versions or adapters if needed, run the
application tests and manual UI flows that use jQuery, and re-run your
SCA/vulnerability scan to confirm the upgrade resolves reported issues.
| public ThymeleafViewResolver viewResolver() { | ||
| var viewResolver = new ThymeleafViewResolver(); | ||
| viewResolver.setTemplateEngine(templateEngine()); | ||
| return viewResolver; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set UTF-8 on the Thymeleaf view resolver.
Prevents charset issues on non-UTF defaults.
public ThymeleafViewResolver viewResolver() {
var viewResolver = new ThymeleafViewResolver();
viewResolver.setTemplateEngine(templateEngine());
+ viewResolver.setCharacterEncoding("UTF-8");
return viewResolver;
}📝 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.
| public ThymeleafViewResolver viewResolver() { | |
| var viewResolver = new ThymeleafViewResolver(); | |
| viewResolver.setTemplateEngine(templateEngine()); | |
| return viewResolver; | |
| } | |
| public ThymeleafViewResolver viewResolver() { | |
| var viewResolver = new ThymeleafViewResolver(); | |
| viewResolver.setTemplateEngine(templateEngine()); | |
| viewResolver.setCharacterEncoding("UTF-8"); | |
| return viewResolver; | |
| } |
🤖 Prompt for AI Agents
In
steve-ui-thymeleaf/src/main/java/de/rwth/idsg/steve/ui/config/ThymeleafConfiguration.java
around lines 76 to 80, the ThymeleafViewResolver is created without configuring
UTF-8, which can cause charset issues; update the viewResolver to set the
character encoding to "UTF-8" (via setCharacterEncoding) and also set the
content type to include the UTF-8 charset (e.g., "text/html; charset=UTF-8")
before returning the resolver.
| $("#chargePointSelectList").change(function() { | ||
| var cp = $(this).find("option:selected").text(); | ||
| var url = /*[[@{/manager/ajax/}]]*/ '/manager/ajax/'; | ||
| $.getJSON(url + cp + "/connectorIds", function(data) { | ||
| var options = ""; | ||
| $.each(data, function() { | ||
| options += "<option value='" + this + "'>" + this + "</option>"; | ||
| }); | ||
| var select = $("#connectorId"); | ||
| select.prop("disabled", false); | ||
| select.html(options); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same AJAX hardening as op15: encode cp, use value(), safe creation, error handling
Mirror the secure pattern to prevent routing bugs and XSS.
- $("#chargePointSelectList").change(function() {
- var cp = $(this).find("option:selected").text();
- var url = /*[[@{/manager/ajax/}]]*/ '/manager/ajax/';
- $.getJSON(url + cp + "/connectorIds", function(data) {
- var options = "";
- $.each(data, function() {
- options += "<option value='" + this + "'>" + this + "</option>";
- });
- var select = $("#connectorId");
- select.prop("disabled", false);
- select.html(options);
- });
- });
+ $("#chargePointSelectList").change(function() {
+ var cp = $(this).val() || $(this).find("option:selected").text();
+ var url = /*[[@{/manager/ajax/}]]*/ '/manager/ajax/';
+ var $select = $("#connectorId");
+ $select.prop("disabled", true).empty();
+ $select.append(new Option("Not for a specific connector", ""));
+ $.getJSON(url + encodeURIComponent(cp) + "/connectorIds")
+ .done(function(data) {
+ data.forEach(function(id) { $select.append(new Option(String(id), id)); });
+ $select.prop("disabled", false);
+ })
+ .fail(function() {
+ $select.empty().append(new Option("Failed to load connectors", "")).prop("disabled", true);
+ });
+ });📝 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.
| $("#chargePointSelectList").change(function() { | |
| var cp = $(this).find("option:selected").text(); | |
| var url = /*[[@{/manager/ajax/}]]*/ '/manager/ajax/'; | |
| $.getJSON(url + cp + "/connectorIds", function(data) { | |
| var options = ""; | |
| $.each(data, function() { | |
| options += "<option value='" + this + "'>" + this + "</option>"; | |
| }); | |
| var select = $("#connectorId"); | |
| select.prop("disabled", false); | |
| select.html(options); | |
| }); | |
| }); | |
| $("#chargePointSelectList").change(function() { | |
| var cp = $(this).val() || $(this).find("option:selected").text(); | |
| var url = /*[[@{/manager/ajax/}]]*/ '/manager/ajax/'; | |
| var $select = $("#connectorId"); | |
| // disable while loading and clear previous options | |
| $select.prop("disabled", true).empty(); | |
| // provide a default placeholder option | |
| $select.append(new Option("Not for a specific connector", "")); | |
| $.getJSON(url + encodeURIComponent(cp) + "/connectorIds") | |
| .done(function(data) { | |
| // safely append each connector ID | |
| data.forEach(function(id) { | |
| $select.append(new Option(String(id), id)); | |
| }); | |
| $select.prop("disabled", false); | |
| }) | |
| .fail(function() { | |
| // show error state | |
| $select.empty() | |
| .append(new Option("Failed to load connectors", "")) | |
| .prop("disabled", true); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/op16/ReserveNow.html around
lines 25 to 37, the AJAX call builds the URL and option HTML unsafely and lacks
error handling; change to read the selected option value via .val() (not
.text()), URL-encode the cp when appending to the endpoint, build option
elements using DOM APIs/text nodes or properly escape values instead of string
concatenation to avoid XSS, and add a .fail or error callback to the $.getJSON
call to handle network/errors and disable/clear the connector select on failure.
| <form th:action="@{/manager/settings}" th:object="${settingsForm}" method="post"> | ||
|
|
There was a problem hiding this comment.
Add CSRF token to POST form
If CSRF is enabled (Spring Security default), this form will be rejected without a token.
- <form th:action="@{/manager/settings}" th:object="${settingsForm}" method="post">
+ <form th:action="@{/manager/settings}" th:object="${settingsForm}" method="post">
+ <input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}"/>📝 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.
| <form th:action="@{/manager/settings}" th:object="${settingsForm}" method="post"> | |
| <form th:action="@{/manager/settings}" th:object="${settingsForm}" method="post"> | |
| <input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}"/> |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/settings.html around lines
16-17, the POST form is missing a CSRF token so requests will be rejected when
Spring Security CSRF protection is enabled; add a hidden input bound to the CSRF
parameter and token (e.g. include an input with th:name="${_csrf.parameterName}"
and th:value="${_csrf.token}") inside the form so the CSRF token is submitted
with the POST, or alternatively enable the Thymeleaf Spring Security dialect
CSRF helper if configured.
| <td>Heartbeat Interval:</td> | ||
| <td><input type="text" th:field="*{heartbeat}"/></td> | ||
| </tr> | ||
| <tr> | ||
| <td colspan="2"><i> | ||
| The time interval in <b>minutes</b> for how often a charge point <br/> should request the current time from SteVe.<br/> | ||
| The value 0 requests clients to use reasonable default values. | ||
| </i></td> | ||
| </tr> | ||
| <tr> | ||
| <td>Expiration:</td> | ||
| <td><input type="text" th:field="*{expiration}"/></td> | ||
| </tr> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use semantic input types and constraints for numeric/email fields
Improve validation and UX: number types with min/step, and email/autocomplete hints.
- <td><input type="text" th:field="*{heartbeat}"/></td>
+ <td><input type="number" th:field="*{heartbeat}" min="0" step="1" inputmode="numeric"/></td>
...
- <td><input type="text" th:field="*{expiration}"/></td>
+ <td><input type="number" th:field="*{expiration}" min="0" step="1" inputmode="numeric"/></td>
...
- <tr><td>Port:</td><td><input type="text" th:field="*{port}"/></td></tr>
- <tr><td>From:</td><td><input type="text" th:field="*{from}"/></td></tr>
- <tr><td>User name:</td><td><input type="text" th:field="*{username}"/></td></tr>
- <tr><td>Password:</td><td><input type="password" th:field="*{password}"/></td></tr>
+ <tr><td>Port:</td><td><input type="number" th:field="*{port}" min="1" max="65535" step="1" inputmode="numeric"/></td></tr>
+ <tr><td>From:</td><td><input type="email" th:field="*{from}" autocomplete="email"/></td></tr>
+ <tr><td>User name:</td><td><input type="text" th:field="*{username}" autocomplete="username"/></td></tr>
+ <tr><td>Password:</td><td><input type="password" th:field="*{password}" autocomplete="current-password"/></td></tr>Also applies to: 54-57
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/settings.html around lines 21
to 33 (and similarly lines 54 to 57), the inputs for numeric settings use plain
text which hurts validation and UX; change these to semantic numeric inputs
(type="number") with appropriate attributes such as min="0" and step="1" (or
step="0.1" if fractional values allowed), add sensible placeholder/title
attributes, and include autocomplete hints where applicable (e.g.,
autocomplete="email" for email fields elsewhere). Ensure Thymeleaf th:field
bindings remain intact and update any server-side parsing/validation if needed
to accept numbers from numeric inputs.
| <td th:text="${task.origin}"></td> | ||
| <td th:data-sort-value="${task.start.time}" th:text="${task.start}"></td> | ||
| <td th:data-sort-value="${task.end.time}" th:text="${task.end}"></td> | ||
| <td th:text="${task.responseCount} + ' / ' + ${task.requestCount}"></td> |
There was a problem hiding this comment.
Fix invalid Thymeleaf expression for “Responses / Requests”.
Use a single expression or literal substitution; current double ${...} will fail.
- <td th:text="${task.responseCount} + ' / ' + ${task.requestCount}"></td>
+ <td th:text="|${task.responseCount} / ${task.requestCount}|"></td>📝 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.
| <td th:text="${task.responseCount} + ' / ' + ${task.requestCount}"></td> | |
| <td th:text="|${task.responseCount} / ${task.requestCount}|"></td> |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/tasks.html around line 34,
the Thymeleaf attribute uses two ${...} expressions which is invalid; replace it
with a single expression such as th:text="${task.responseCount + ' / ' +
task.requestCount}" so the entire concatenation is evaluated inside one ${...}.
| app.serverStart(); | ||
| } |
There was a problem hiding this comment.
main() should join or delegate to start(); current code may return immediately.
main() calls serverStart() and exits; use start() to print log path and join the Jetty thread.
- Application app = new Application(config, logFileRetriever);
- app.serverStart();
+ Application app = new Application(config, logFileRetriever);
+ app.start();📝 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.
| app.serverStart(); | |
| } | |
| Application app = new Application(config, logFileRetriever); | |
| app.start(); | |
| } |
🤖 Prompt for AI Agents
In steve/src/main/java/de/rwth/idsg/steve/Application.java around lines 67 to
68, main currently calls app.serverStart() which may return immediately and let
the JVM exit; replace that call with app.start() (or delegate to the existing
start() method) so that the application prints the log path and joins/blocks on
the Jetty thread. Ensure main invokes the blocking start() method (or forwards
to it) instead of serverStart() so the process remains alive and logging
behavior is preserved.
| public void addResourceHandlers(final ResourceHandlerRegistry registry) { | ||
| registry.addResourceHandler("/static/**").addResourceLocations("static/"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix static resource location (missing leading slash; add classpath fallback).
Without a leading slash, Spring may fail to resolve resources relative to the webapp root. Recommend also adding classpath fallback for packaged JARs.
- registry.addResourceHandler("/static/**").addResourceLocations("static/");
+ registry.addResourceHandler("/static/**")
+ .addResourceLocations("/static/", "classpath:/static/");📝 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.
| public void addResourceHandlers(final ResourceHandlerRegistry registry) { | |
| registry.addResourceHandler("/static/**").addResourceLocations("static/"); | |
| } | |
| public void addResourceHandlers(final ResourceHandlerRegistry registry) { | |
| registry.addResourceHandler("/static/**") | |
| .addResourceLocations("/static/", "classpath:/static/"); | |
| } |
🤖 Prompt for AI Agents
In steve-ui-jsp/src/main/java/de/rwth/idsg/steve/ui/config/JspConfiguration.java
around lines 57 to 59, the ResourceHandlerRegistry is using "static/" which
lacks the leading slash and has no classpath fallback; change the resource
locations to include a leading slash and a classpath fallback (e.g. "/static/"
and "classpath:/static/") so resources resolve from the webapp root and from
packaged JARs.
| background:-webkit-gradient(linear, left top, left bottom, color-stop(0.05, #ededed), color-stop(1, #dfdfdf)); | ||
| background:-moz-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | ||
| background:-webkit-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | ||
| background:-o-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | ||
| background:-ms-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | ||
| background:linear-gradient(to bottom, #ededed 5%, #dfdfdf 100%); | ||
| filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#ededed', endColorstr='#dfdfdf',GradientType=0); | ||
| background-color:#ededed; | ||
| border:1px solid #b0b0b0; | ||
| display:inline-block; | ||
| color:#575757; | ||
| text-decoration:none; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove legacy gradient declarations that break linters/parsers.
The IE filter and multiple vendor-prefixed background declarations trigger parse errors and duplicate-property warnings. Modernize to a single background-image with a fallback color.
- background:-webkit-gradient(linear, left top, left bottom, color-stop(0.05, #ededed), color-stop(1, #dfdfdf));
- background:-moz-linear-gradient(top, #ededed 5%, #dfdfdf 100%);
- background:-webkit-linear-gradient(top, #ededed 5%, #dfdfdf 100%);
- background:-o-linear-gradient(top, #ededed 5%, #dfdfdf 100%);
- background:-ms-linear-gradient(top, #ededed 5%, #dfdfdf 100%);
- background:linear-gradient(to bottom, #ededed 5%, #dfdfdf 100%);
- filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#ededed', endColorstr='#dfdfdf',GradientType=0);
- background-color:#ededed;
+ background-color: #ededed;
+ background-image: linear-gradient(to bottom, #ededed 5%, #dfdfdf 100%);📝 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.
| background:-webkit-gradient(linear, left top, left bottom, color-stop(0.05, #ededed), color-stop(1, #dfdfdf)); | |
| background:-moz-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | |
| background:-webkit-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | |
| background:-o-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | |
| background:-ms-linear-gradient(top, #ededed 5%, #dfdfdf 100%); | |
| background:linear-gradient(to bottom, #ededed 5%, #dfdfdf 100%); | |
| filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#ededed', endColorstr='#dfdfdf',GradientType=0); | |
| background-color:#ededed; | |
| border:1px solid #b0b0b0; | |
| display:inline-block; | |
| color:#575757; | |
| text-decoration:none; | |
| } | |
| background-color: #ededed; | |
| background-image: linear-gradient(to bottom, #ededed 5%, #dfdfdf 100%); | |
| border: 1px solid #b0b0b0; | |
| display: inline-block; | |
| color: #575757; | |
| text-decoration: none; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 127-127: Unexpected value or character.
Expected one of:
(parse)
[error] 122-122: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
background is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/static/css/style.css around lines 121
to 133, remove the legacy vendor-prefixed gradient declarations and the IE
filter that cause linter/parser errors and duplicate-property warnings; replace
them with a single modern background-image: linear-gradient(...) declaration and
keep a solid background-color fallback (and retain the existing border, display,
color, and text-decoration rules), ensuring background-color appears before the
background-image to act as a fallback for older browsers.
| background:-webkit-gradient(linear, left top, left bottom, color-stop(0.05, #dfdfdf), color-stop(1, #ededed)); | ||
| background:-moz-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | ||
| background:-webkit-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | ||
| background:-o-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | ||
| background:-ms-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | ||
| background:linear-gradient(to bottom, #dfdfdf 5%, #ededed 100%); | ||
| filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#dfdfdf', endColorstr='#ededed',GradientType=0); | ||
| background-color:#dfdfdf; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same here: hover state uses duplicate/legacy background declarations.
Consolidate to a single modern declaration.
- background:-webkit-gradient(linear, left top, left bottom, color-stop(0.05, #dfdfdf), color-stop(1, #ededed));
- background:-moz-linear-gradient(top, #dfdfdf 5%, #ededed 100%);
- background:-webkit-linear-gradient(top, #dfdfdf 5%, #ededed 100%);
- background:-o-linear-gradient(top, #dfdfdf 5%, #ededed 100%);
- background:-ms-linear-gradient(top, #dfdfdf 5%, #ededed 100%);
- background:linear-gradient(to bottom, #dfdfdf 5%, #ededed 100%);
- filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#dfdfdf', endColorstr='#ededed',GradientType=0);
- background-color:#dfdfdf;
+ background-color: #dfdfdf;
+ background-image: linear-gradient(to bottom, #dfdfdf 5%, #ededed 100%);📝 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.
| background:-webkit-gradient(linear, left top, left bottom, color-stop(0.05, #dfdfdf), color-stop(1, #ededed)); | |
| background:-moz-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | |
| background:-webkit-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | |
| background:-o-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | |
| background:-ms-linear-gradient(top, #dfdfdf 5%, #ededed 100%); | |
| background:linear-gradient(to bottom, #dfdfdf 5%, #ededed 100%); | |
| filter:progid:DXImageTransform.Microsoft.gradient(startColorstr='#dfdfdf', endColorstr='#ededed',GradientType=0); | |
| background-color:#dfdfdf; | |
| } | |
| /* …other declarations above… */ | |
| background-color: #dfdfdf; | |
| background-image: linear-gradient(to bottom, #dfdfdf 5%, #ededed 100%); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 141-141: Unexpected value or character.
Expected one of:
(parse)
[error] 136-136: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
background is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/static/css/style.css around lines 135
to 143, the hover state's background contains multiple legacy vendor-prefixed
and filter declarations; consolidate them by removing redundant vendor-prefixed
gradient lines and the old IE filter, keeping a single modern background
(linear-gradient) plus a fallback background-color if needed, and ensure the
remaining declaration is last so it takes precedence.
| input[type="submit"]:disabled, input[type="submit"]:hover:disabled, input[type="submit"]:active:disabled { | ||
| cursor: not-allowed; | ||
| background:-webkit-gradient(linear, left top, left bottom, from(#a5a5a5), to(#5b5b5b)); | ||
| background:-moz-linear-gradient(top, #a5a5a5, #5b5b5b); | ||
| background:-o-linear-gradient(top, #a5a5a5, #5b5b5b); | ||
| background:-ms-linear-gradient(top, #a5a5a5, #5b5b5b); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disabled submit button: remove duplicate/legacy gradient declarations.
This also triggers duplicate-property warnings.
- background:-webkit-gradient(linear, left top, left bottom, from(#a5a5a5), to(#5b5b5b));
- background:-moz-linear-gradient(top, #a5a5a5, #5b5b5b);
- background:-o-linear-gradient(top, #a5a5a5, #5b5b5b);
- background:-ms-linear-gradient(top, #a5a5a5, #5b5b5b);
+ background-image: linear-gradient(to bottom, #a5a5a5, #5b5b5b);📝 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.
| input[type="submit"]:disabled, input[type="submit"]:hover:disabled, input[type="submit"]:active:disabled { | |
| cursor: not-allowed; | |
| background:-webkit-gradient(linear, left top, left bottom, from(#a5a5a5), to(#5b5b5b)); | |
| background:-moz-linear-gradient(top, #a5a5a5, #5b5b5b); | |
| background:-o-linear-gradient(top, #a5a5a5, #5b5b5b); | |
| background:-ms-linear-gradient(top, #a5a5a5, #5b5b5b); | |
| } | |
| input[type="submit"]:disabled, | |
| input[type="submit"]:hover:disabled, | |
| input[type="submit"]:active:disabled { | |
| cursor: not-allowed; | |
| background-image: linear-gradient(to bottom, #a5a5a5, #5b5b5b); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 345-345: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
background is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
steve-ui-thymeleaf/src/main/resources/static/css/style.css lines 342-348: the
disabled submit selector currently contains multiple duplicate/legacy gradient
declarations causing duplicate-property warnings—remove the outdated
vendor-specific gradient lines (including -webkit-gradient,
-moz-linear-gradient, -o-linear-gradient, -ms-linear-gradient) and replace them
with a single modern, standards-compliant linear-gradient declaration (and
optionally a simple solid-color fallback) while keeping the cursor:not-allowed
rule intact.
| <form th:if="${ta.stopValue == null}" th:action="@{/manager/transactions/stop/{id}(id=${ta.id})}" method="post"> | ||
| <input type="submit" class="redSubmit" title="Manually set this transaction from 'active' to 'stopped'. The actual transaction is not affected by this!" value="Stop"> | ||
| </form> | ||
| </td> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include CSRF token in POST “Stop” form.
Spring Security will reject the request without it; also mitigates CSRF.
- <form th:if="${ta.stopValue == null}" th:action="@{/manager/transactions/stop/{id}(id=${ta.id})}" method="post">
+ <form th:if="${ta.stopValue == null}" th:action="@{/manager/transactions/stop/{id}(id=${ta.id})}" method="post">
+ <input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}"/>
<input type="submit" class="redSubmit" title="Manually set this transaction from 'active' to 'stopped'. The actual transaction is not affected by this!" value="Stop">
</form>📝 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.
| <form th:if="${ta.stopValue == null}" th:action="@{/manager/transactions/stop/{id}(id=${ta.id})}" method="post"> | |
| <input type="submit" class="redSubmit" title="Manually set this transaction from 'active' to 'stopped'. The actual transaction is not affected by this!" value="Stop"> | |
| </form> | |
| </td> | |
| <form th:if="${ta.stopValue == null}" | |
| th:action="@{/manager/transactions/stop/{id}(id=${ta.id})}" | |
| method="post"> | |
| <input type="hidden" | |
| th:name="${_csrf.parameterName}" | |
| th:value="${_csrf.token}"/> | |
| <input type="submit" | |
| class="redSubmit" | |
| title="Manually set this transaction from 'active' to 'stopped'. The actual transaction is not affected by this!" | |
| value="Stop"> | |
| </form> |
🤖 Prompt for AI Agents
In steve-ui-thymeleaf/src/main/resources/templates/data-man/transactions.html
around lines 108 to 111, the POST form that stops a transaction is missing the
CSRF token so Spring Security will reject the request; add a hidden CSRF input
inside the form using Thymeleaf variables (e.g. <input type="hidden"
th:name="${_csrf.parameterName}" th:value="${_csrf.token}"/>), ensuring the
token is present on all POST forms that change state.
| <tr> | ||
| <td>Connector ID (integer):</td> | ||
| <td><input type="text" th:field="*{connectorId}" placeholder="if empty, 0 = charge point as a whole"/></td> | ||
| </tr> | ||
| <tr> | ||
| <td>Availability Type:</td> | ||
| <td> | ||
| <select th:field="*{availType}"> | ||
| <option th:each="t : ${availTypes}" th:value="${t}" th:text="${t.value}"></option> | ||
| </select> | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain numeric inputs with type=number and min/step.
Prevents non-numeric submission and reduces server-side validation burden.
-<input type="text" th:field="*{connectorId}" .../>
+<input type="number" th:field="*{connectorId}" min="0" step="1" .../>
-<input type="text" th:field="*{durationInSeconds}"/>
+<input type="number" th:field="*{durationInSeconds}" min="0" step="1"/>
-<input type="text" th:field="*{stackLevel}"/>
+<input type="number" th:field="*{stackLevel}" min="0" step="1"/>Also applies to: 217-231, 501-506
🤖 Prompt for AI Agents
In
steve-ui-thymeleaf/src/main/resources/templates/op-forms/_operationFragments.html
around lines 66 to 77 (also apply same change to lines 217-231 and 501-506), the
connectorId input is a text field and should be constrained as a numeric input;
change the input to type="number" and add appropriate attributes such as min="0"
and step="1" (and add matching attributes for any other integer fields at the
referenced ranges) so the browser enforces numeric entry and prevents
non-numeric submissions.
| <profile> | ||
| <id>with-jsp</id> | ||
| <dependencies> | ||
| <dependency> | ||
| <groupId>de.rwth.idsg</groupId> | ||
| <artifactId>steve-ui-jsp</artifactId> | ||
| </dependency> | ||
| </dependencies> | ||
| </profile> | ||
| <profile> | ||
| <id>with-thymeleaf</id> | ||
| <activation> | ||
| <activeByDefault>true</activeByDefault> | ||
| </activation> | ||
| <dependencies> | ||
| <dependency> | ||
| <groupId>de.rwth.idsg</groupId> | ||
| <artifactId>steve-ui-thymeleaf</artifactId> | ||
| </dependency> | ||
| </dependencies> | ||
| </profile> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Profiles are not mutually exclusive; -Pwith-jsp will still keep Thymeleaf active.
with-thymeleaf is activeByDefault; enabling with-jsp adds JSP on top, potentially registering both view resolvers and causing ambiguity. Make them exclusive.
Options:
- Remove activeByDefault and require explicit
-Pwith-thymeleafor-Pwith-jsp. - Or gate profiles via properties:
- <profile>
- <id>with-thymeleaf</id>
- <activation>
- <activeByDefault>true</activeByDefault>
- </activation>
+ <profile>
+ <id>with-thymeleaf</id>
+ <activation>
+ <property>
+ <name>!with-jsp</name>
+ </property>
+ </activation>If negation isn’t desired, switch to a single property, e.g., -Dui=jsp:
<activation>
<property>
<name>ui</name>
<value>thymeleaf</value>
</property>
</activation>Then activate JSP with -Dui=jsp.
🤖 Prompt for AI Agents
In steve/pom.xml around lines 285-305 the two profiles are not mutually
exclusive because with-thymeleaf is activeByDefault, so activating -Pwith-jsp
leaves Thymeleaf enabled and can register conflicting view resolvers; fix by
making profiles mutually exclusive: either remove activeByDefault and require
explicit profile selection (use -Pwith-thymeleaf or -Pwith-jsp), or replace
activation with a single property-based activation (e.g., use a property name
like "ui" and activate thymeleaf when ui=thymeleaf and jsp when ui=jsp), or add
property-gated negation so enabling with-jsp disables with-thymeleaf; update the
profile activation blocks accordingly and ensure CI/build docs mention the
chosen activation flag.
| public JettyServer(SteveConfiguration config, LogFileRetriever logFileRetriever) { | ||
| super(createThreadPool()); | ||
| this.config = config; | ||
| this.logFileRetriever = logFileRetriever; | ||
| this.info = new EndpointInfo(config); | ||
| this.addEventListener(updateWsDefault()); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Register WS defaults on the WebAppContext, not on Server.
ServletContextListener should be attached to the web app’s context to guarantee contextInitialized is called for that context.
Apply:
- this.addEventListener(updateWsDefault());
+ // moved to WebAppContext to ensure proper ServletContext lifecycleAnd after creating the app context:
- var steveAppContext = new SteveAppContext(config, logFileRetriever, info);
- this.setHandler(steveAppContext.createHandlers());
+ var steveAppContext = new SteveAppContext(config, logFileRetriever, info);
+ steveAppContext.getWebAppContext().addEventListener(updateWsDefault());
+ this.setHandler(steveAppContext.createHandlers());Also applies to: 139-143
🏁 Script executed:
#!/bin/bash
set -eux
# locate JettyServer.java
fd JettyServer.java
# print lines around the constructor snippet
sed -n '60,100p' steve/src/main/java/de/rwth/idsg/steve/JettyServer.java
# find updateWsDefault references
grep -R "updateWsDefault" -n steve/src/main/java/de/rwth/idsg/steve/JettyServer.java
# locate SteveAppContext.java
fd SteveAppContext.java
# print its initial content
sed -n '1,200p' steve/src/main/java/de/rwth/idsg/steve/SteveAppContext.java
# check for getWebAppContext method
grep -R "getWebAppContext" -n steve/src/main/java/de/rwth/idsg/steve/SteveAppContext.javaLength of output: 8676
🏁 Script executed:
#!/bin/bash
set -eux
# Print lines around the second updateWsDefault invocation (lines ~139-143)
sed -n '130,160p' steve/src/main/java/de/rwth/idsg/steve/JettyServer.java
# Print the updateWsDefault static method body (lines ~180-260)
sed -n '180,260p' steve/src/main/java/de/rwth/idsg/steve/JettyServer.javaLength of output: 4430
Register WS defaults on the WebAppContext, not on Server
Remove the this.addEventListener(updateWsDefault()); call from the JettyServer constructor and instead attach the listener directly to the WebAppContext returned by SteveAppContext. For example:
--- a/steve/src/main/java/de/rwth/idsg/steve/JettyServer.java
+++ b/steve/src/main/java/de/rwth/idsg/steve/JettyServer.java
@@ constructor (around line 77)
- this.addEventListener(updateWsDefault());
+ // moved to WebAppContext to ensure ServletContextListener firesAnd in doStart() (around lines 139–143):
- var steveAppContext = new SteveAppContext(config, logFileRetriever, info);
- this.setHandler(steveAppContext.createHandlers());
+ var steveAppContext = new SteveAppContext(config, logFileRetriever, info);
+ steveAppContext.getWebAppContext().addEventListener(updateWsDefault());
+ this.setHandler(steveAppContext.createHandlers());Apply the same adjustment for the second listener registration.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In steve/src/main/java/de/rwth/idsg/steve/JettyServer.java around lines 77 to
83, remove the call this.addEventListener(updateWsDefault()) from the
JettyServer constructor and instead register that listener on the WebAppContext
instance returned by SteveAppContext (locate the method/variable that
creates/returns the WebAppContext). Also in doStart() around lines 139 to 143,
find the second addEventListener call noted in the review and move that
registration so it is invoked on the same WebAppContext rather than on the
Server; ensure the WebAppContext is created/available before attaching both
listeners and replace server-level listener registrations with
context.addEventListener(...).
| if (config.getJetty().isHttpsEnabled()) { | ||
| server.addConnector(httpsConnector(httpConfig)); | ||
| this.addConnector(createHttpsConnector( | ||
| this, | ||
| config.getJetty().getServerHost(), | ||
| config.getJetty().getHttpPort(), | ||
| httpConfig, | ||
| config.getJetty().getKeyStorePath(), | ||
| config.getJetty().getKeyStorePassword())); | ||
| } |
There was a problem hiding this comment.
HTTPS connector is bound to the HTTP port.
createHttpsConnector is invoked with getHttpPort() instead of getHttpsPort(), so HTTPS will listen on the wrong port.
Apply:
- this.addConnector(createHttpsConnector(
- this,
- config.getJetty().getServerHost(),
- config.getJetty().getHttpPort(),
- httpConfig,
- config.getJetty().getKeyStorePath(),
- config.getJetty().getKeyStorePassword()));
+ this.addConnector(createHttpsConnector(
+ this,
+ config.getJetty().getServerHost(),
+ config.getJetty().getHttpsPort(),
+ httpConfig,
+ config.getJetty().getKeyStorePath(),
+ config.getJetty().getKeyStorePassword()));📝 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 (config.getJetty().isHttpsEnabled()) { | |
| server.addConnector(httpsConnector(httpConfig)); | |
| this.addConnector(createHttpsConnector( | |
| this, | |
| config.getJetty().getServerHost(), | |
| config.getJetty().getHttpPort(), | |
| httpConfig, | |
| config.getJetty().getKeyStorePath(), | |
| config.getJetty().getKeyStorePassword())); | |
| } | |
| if (config.getJetty().isHttpsEnabled()) { | |
| this.addConnector(createHttpsConnector( | |
| this, | |
| config.getJetty().getServerHost(), | |
| config.getJetty().getHttpsPort(), | |
| httpConfig, | |
| config.getJetty().getKeyStorePath(), | |
| config.getJetty().getKeyStorePassword())); | |
| } |
🤖 Prompt for AI Agents
In steve/src/main/java/de/rwth/idsg/steve/JettyServer.java around lines 126 to
134, the HTTPS connector is being created with config.getJetty().getHttpPort()
so HTTPS will bind to the HTTP port; change the argument to
config.getJetty().getHttpsPort() when calling createHttpsConnector so the
connector listens on the configured HTTPS port.
| var host = "treibhaus.informatik.rwth-aachen.de"; | ||
| var ips = new ArrayList<String>(); | ||
|
|
||
| try { | ||
| ips.add(InetAddress.getLocalHost().getHostAddress()); | ||
| } catch (Exception e) { | ||
| // fail silently | ||
| } | ||
|
|
||
| try { | ||
| Socket socket = new Socket(); | ||
| try (var socket = new Socket()) { | ||
| socket.connect(new InetSocketAddress(host, 80)); | ||
| ips.add(socket.getLocalAddress().getHostAddress()); | ||
| } catch (Exception e) { | ||
| // fail silently |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded external host + cleartext TCP for IP discovery.
Using a fixed domain (treibhaus.informatik.rwth-aachen.de) is brittle and the TCP connect on port 80 can hang without timeout. Prefer the UDP technique (8.8.8.8) and local interfaces only.
Apply:
- var host = "treibhaus.informatik.rwth-aachen.de";
var ips = new ArrayList<String>();
@@
- try (var socket = new Socket()) {
- socket.connect(new InetSocketAddress(host, 80));
- ips.add(socket.getLocalAddress().getHostAddress());
- } catch (Exception e) {
- // fail silently
- }
+ // skip TCP connect trick (brittle, may hang, and adds external dependency)📝 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.
| var host = "treibhaus.informatik.rwth-aachen.de"; | |
| var ips = new ArrayList<String>(); | |
| try { | |
| ips.add(InetAddress.getLocalHost().getHostAddress()); | |
| } catch (Exception e) { | |
| // fail silently | |
| } | |
| try { | |
| Socket socket = new Socket(); | |
| try (var socket = new Socket()) { | |
| socket.connect(new InetSocketAddress(host, 80)); | |
| ips.add(socket.getLocalAddress().getHostAddress()); | |
| } catch (Exception e) { | |
| // fail silently | |
| var ips = new ArrayList<String>(); | |
| try { | |
| ips.add(InetAddress.getLocalHost().getHostAddress()); | |
| } catch (Exception e) { | |
| // fail silently | |
| } | |
| // skip TCP connect trick (brittle, may hang, and adds external dependency) |
🧰 Tools
🪛 ast-grep (0.38.6)
[info] 256-256: "Detected use of a Java socket that is not encrypted. As a result, the
traffic could be read by an attacker intercepting the network traffic. Use
an SSLSocket created by 'SSLSocketFactory' or 'SSLServerSocketFactory'
instead."
Context: new Socket()
Note: [CWE-319] Cleartext Transmission of Sensitive Information [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(unencrypted-socket-java)
🤖 Prompt for AI Agents
In steve/src/main/java/de/rwth/idsg/steve/JettyServer.java around lines 248 to
261, the code uses a hard-coded host "treibhaus.informatik.rwth-aachen.de" and a
blocking TCP connect to port 80 to discover the local IP, which is brittle and
can hang; replace this with a non-blocking UDP technique (e.g., create a
DatagramSocket, connect it to 8.8.8.8:53, then read socket.getLocalAddress())
or, alternatively, enumerate local network interfaces
(NetworkInterface.getNetworkInterfaces()) and collect non-loopback, IPv4
addresses, and ensure all socket operations set short timeouts and swallow only
expected exceptions while logging debug info; update the ips collection logic to
use one of these safer methods and remove the hard-coded host and plain TCP
connect.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
.dockerignore (1)
17-21: Good baseline; add env/secrets and Eclipse artifacts.Prevent accidental secret/context bloat and IDE noise.
Apply:
**/target/ !.mvn/ !mvnw !mvnw.cmd +.env +.env.* +.dockerignore +.classpath +.project +.settings/ +**/.idea/ +**/.vscode/ +*.logDockerfile (1)
70-76: Harden runtime image (non-root, healthcheck).Improve security and operability.
FROM eclipse-temurin:21 WORKDIR /app ENV LANG=C.UTF-8 LC_ALL=C.UTF-8 +RUN adduser --system --home /nonexistent --uid 10001 appuser @@ EXPOSE ${PORT} EXPOSE 8443 -CMD ["sh", "-c", "exec java -XX:MaxRAMPercentage=85 -jar /app/steve.jar"] +HEALTHCHECK --interval=30s --timeout=5s --retries=5 CMD wget -qO- http://localhost:${PORT}/actuator/health || exit 1 +USER 10001 +CMD ["sh", "-c", "exec java -XX:+ExitOnOutOfMemoryError -XX:MaxRAMPercentage=85 -jar /app/steve.jar"]Adjust health endpoint/path if different.
docker-compose.yml (3)
14-14: Port 3306 exposure may conflict on hosts with MySQL.Keep, but document the collision or use a different host port (e.g., 13306:3306).
- - "3306:3306" + - "13306:3306"Update DB_PORT accordingly if you adopt this.
39-41: depends_on only affects runtime, not image build.Avoid giving the impression it orders the build. Add README guidance for the build-order requirement.
I can propose README instructions or a Makefile target (build-db, build-app, up) if helpful.
43-46: Volume mount path doesn’t affect the final image.The app runs from /app; mounting .:/code is unused. If you intended hot-reload, run a dev image from the builder stage or adjust the mount.
- volumes: - - .:/code + # For dev-only hot-reload, switch to builder stage image and mount sources. + # volumes: + # - .:/code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.dockerignore(1 hunks)Dockerfile(1 hunks)docker-compose.yml(1 hunks)steve/Dockerfile(0 hunks)
💤 Files with no reviewable changes (1)
- steve/Dockerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T07:17:42.970Z
Learnt from: juherr
PR: juherr/evolve#62
File: steve/README.md:119-121
Timestamp: 2025-08-29T07:17:42.970Z
Learning: SteVe project uses jOOQ which requires live database connection during Maven build process to run Flyway migrations and generate Java code from database schema in the generate-sources phase.
Applied to files:
Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-24.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-24.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.4.30)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.6.14)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.5.21)
- GitHub Check: build (ubuntu-22.04, 24, mysql:8.0)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 21, mariadb:10.3)
- GitHub Check: build (ubuntu-22.04, 24, mariadb:10.4.30)
🔇 Additional comments (3)
Dockerfile (1)
53-59: Verify whether -Dhttp.port and -Ddb. are needed at runtime too.*You pass them to Maven during build, but the final java -jar doesn’t receive them. If the app reads these only at runtime, connectivity will fail.
If required, inject at runtime via entrypoint:
-CMD ["sh", "-c", "exec java -XX:+ExitOnOutOfMemoryError -XX:MaxRAMPercentage=85 -jar /app/steve.jar"] +ENV DB_HOST=db DB_PORT=3306 DB_DATABASE=stevedb DB_USER=steve +# DB_PASSWORD from secret/environment +CMD ["sh", "-c", "exec java -XX:+ExitOnOutOfMemoryError -XX:MaxRAMPercentage=85 \ + -Dhttp.port=${PORT} -Dserver.host=0.0.0.0 \ + -Ddb.ip=${DB_HOST} -Ddb.port=${DB_PORT} -Ddb.schema=${DB_DATABASE} \ + -Ddb.user=${DB_USER} -Ddb.password=${DB_PASSWORD} \ + -jar /app/steve.jar"]If not needed, ignore.
docker-compose.yml (2)
20-26: Healthcheck LGTM.Appropriate probe for MariaDB readiness.
45-46: Expose ports align with Dockerfile; good.No action.
| build: | ||
| context: . | ||
| args: | ||
| DB_HOST: host.docker.internal | ||
| DB_PORT: 3306 | ||
| DB_DATABASE: stevedb | ||
| DB_USER: steve | ||
| DB_PASSWORD: changeme | ||
| image: steve:test |
There was a problem hiding this comment.
Build-time DB args embed secrets and don’t ensure DB is up before build.
Two issues:
- Secrets in build args leak to image history/logs.
- Compose builds images before starting services; builder won’t reach db even with host.docker.internal.
Consider:
- Remove build args here; build after db is up (two-step workflow), passing secrets via build secrets.
- Or add a dedicated “codegen” service (depends_on: db:healthy) to run mvn generate-sources, then build the image without DB coupling.
| # copy Maven wrapper if present | ||
| cp mvnw /code/; \ | ||
| mkdir -p /code/.mvn && cp -R .mvn/. /code/.mvn/; \ | ||
| # copy all pom.xml preserving directory structure | ||
| while IFS= read -r -d "" p; do \ | ||
| mkdir -p /code/"$(dirname "$p")"; \ | ||
| cp "$p" /code/"$p"; \ | ||
| done < <(find . -name pom.xml -type f -print0); \ |
There was a problem hiding this comment.
“If present” comment is false; cp will fail when files are absent.
Guard wrapper/.mvn copies to avoid breaking builds in forks/CI without mvnw.
- # copy Maven wrapper if present
- cp mvnw /code/; \
- mkdir -p /code/.mvn && cp -R .mvn/. /code/.mvn/; \
+ # copy Maven wrapper if present
+ [ -f mvnw ] && cp mvnw /code/ || true; \
+ if [ -d .mvn ]; then mkdir -p /code/.mvn && cp -R .mvn/. /code/.mvn/; fi; \📝 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.
| # copy Maven wrapper if present | |
| cp mvnw /code/; \ | |
| mkdir -p /code/.mvn && cp -R .mvn/. /code/.mvn/; \ | |
| # copy all pom.xml preserving directory structure | |
| while IFS= read -r -d "" p; do \ | |
| mkdir -p /code/"$(dirname "$p")"; \ | |
| cp "$p" /code/"$p"; \ | |
| done < <(find . -name pom.xml -type f -print0); \ | |
| # copy Maven wrapper if present | |
| [ -f mvnw ] && cp mvnw /code/ || true; \ | |
| if [ -d .mvn ]; then mkdir -p /code/.mvn && cp -R .mvn/. /code/.mvn/; fi; \ | |
| # copy all pom.xml preserving directory structure | |
| while IFS= read -r -d "" p; do \ | |
| mkdir -p /code/"$(dirname "$p")"; \ | |
| cp "$p" /code/"$p"; \ | |
| done < <(find . -name pom.xml -type f -print0); \ |
🤖 Prompt for AI Agents
In Dockerfile around lines 12 to 19, the comments claim "if present" but the cp
commands unconditionally copy mvnw and .mvn which will fail if they don't exist;
guard these copies by checking existence first: only cp mvnw if the file exists,
only create /code/.mvn and cp .mvn contents if the .mvn directory exists, and
keep the find+while loop for pom.xml as-is (or ensure it safely handles no
results); update the Dockerfile to perform -f/-d tests (or use shell
conditionals) before each cp to avoid build failures in forks/CI without
mvnw/.mvn.
| ARG DB_HOST | ||
| ARG DB_PORT | ||
| ARG DB_DATABASE | ||
| ARG DB_USER | ||
| ARG DB_PASSWORD | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
DB credentials as build args leak into image history.
Build-time args are visible in layer history and logs; don’t pass secrets this way.
Use BuildKit secrets for the password and minimal-privilege codegen user:
-ARG DB_PASSWORD
+ARG DB_PASSWORD # kept for local dev, prefer secret in CIAnd in the build step:
-RUN --mount=type=cache,target=/root/.m2 \
+RUN --mount=type=cache,target=/root/.m2 \
+ --mount=type=secret,id=dbpass \
dockerize -wait tcp://${DB_HOST}:${DB_PORT} -timeout 60s \
&& ./mvnw clean package \
@@
- -Ddb.password="${DB_PASSWORD}" \
+ -Ddb.password="$(cat /run/secrets/dbpass || echo ${DB_PASSWORD})" \Document building with: docker build --secret id=dbpass,src=./db.pass …
| RUN --mount=type=cache,target=/root/.m2 \ | ||
| dockerize -wait tcp://${DB_HOST}:${DB_PORT} -timeout 60s \ | ||
| && ./mvnw clean package \ | ||
| -DskipTests \ | ||
| -Dhttp.port=${PORT} \ | ||
| -Dserver.host="0.0.0.0" \ | ||
| -Ddb.ip="${DB_HOST}" \ | ||
| -Ddb.port=${DB_PORT} \ | ||
| -Ddb.schema="${DB_DATABASE}" \ | ||
| -Ddb.user="${DB_USER}" \ | ||
| -Ddb.password="${DB_PASSWORD}" \ | ||
| -Dserver.gzip.enabled=false \ | ||
| -Dappender="CONSOLE" \ | ||
| -Djdk.tls.client.protocols="TLSv1,TLSv1.1,TLSv1.2" | ||
|
|
There was a problem hiding this comment.
Build depends on a running DB; docker compose can’t guarantee DB is up before build.
With dockerize in the builder stage, docker compose up --build will likely fail because images are built before services start. This conflicts with SteVe’s jOOQ/Flyway codegen requirement for a live DB (per retrieved learnings).
Leverage the learning: SteVe needs DB during mvn generate-sources.
Options:
- Simplest workflow: document a 2-step sequence:
- docker compose up -d db
- docker build --build-arg DB_* …
- docker compose up app
- Or add a compose “codegen” service that depends_on db:healthy and runs mvn -Ddb.* generate-sources, stores generated-sources in a volume, then let Dockerfile build without DB.
- Or supply a cached “generated-sources” artifact via a separate stage/volume.
If you keep dockerize in Dockerfile, remove build from compose for app to avoid misleading users.
🤖 Prompt for AI Agents
In Dockerfile around lines 49-63, the build currently waits for a running DB via
dockerize which fails because images are built before compose starts services;
remove the dockerize wait from the Dockerfile and instead (choose one) either:
1) document a two-step build workflow (bring up the db first, then build the
image with DB_* build-args, then start the app), or 2) add a separate "codegen"
service in docker-compose that depends_on db:healthy and runs mvn -Ddb.*
generate-sources writing generated-sources into a shared volume which the app
build stage consumes, or 3) create a prior CI/build stage that produces and
caches generated-sources/artifact which the Dockerfile copies in; implement one
of these fixes and ensure the Dockerfile no longer blocks on DB at build time
(or remove app from compose build to avoid misleading users).
| -Dserver.gzip.enabled=false \ | ||
| -Dappender="CONSOLE" \ | ||
| -Djdk.tls.client.protocols="TLSv1,TLSv1.1,TLSv1.2" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t enable TLS 1.0/1.1.
These are deprecated and should not be forced on the build JVM.
- -Djdk.tls.client.protocols="TLSv1,TLSv1.1,TLSv1.2"
+ -Djdk.tls.client.protocols="TLSv1.2,TLSv1.3"📝 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.
| -Dserver.gzip.enabled=false \ | |
| -Dappender="CONSOLE" \ | |
| -Djdk.tls.client.protocols="TLSv1,TLSv1.1,TLSv1.2" | |
| -Dserver.gzip.enabled=false \ | |
| -Dappender="CONSOLE" \ | |
| -Djdk.tls.client.protocols="TLSv1.2,TLSv1.3" |
🤖 Prompt for AI Agents
In Dockerfile around lines 60 to 62, the JVM system property
jdk.tls.client.protocols is forcing deprecated TLSv1 and TLSv1.1; remove those
protocols so only modern TLS versions are allowed (e.g., use TLSv1.2 and TLSv1.3
or omit the property to use the JVM defaults). Update the
-Djdk.tls.client.protocols value to exclude TLSv1 and TLSv1.1 and ensure it
specifies only supported secure protocols.
Summary by CodeRabbit
New Features
Improvements
Chores