fix: avoid format strings in FrontendUtils.console - #25048
Conversation
FrontendUtils.console() passed a caller-supplied format string to String.format(), which static analysis flags as CWE-134. The ANSI color constants (YELLOW, RED, GREEN, BRIGHT_BLUE) are changed to hold only the color escape prefix, a new ANSI_RESET constant holds the reset sequence, and console() now concatenates color, message and reset instead of formatting the message, so the text is always printed literally. Fixes vaadin#9263
|
|
|
@hej090224 could you please sign the CLA? Otherwise we cannot proceed with this PR. |
|
@mcollovati Apologies for the delayed response — I missed the CLA notification. I'll sign the CLA now via the link above. |
console(String, String) let the color and message arguments be swapped by mistake since both were plain strings. The ANSI color constants (YELLOW, RED, GREEN, BRIGHT_BLUE) are replaced by a nested AnsiColor enum holding each color's escape code and reset sequence, so console(AnsiColor, String) can no longer confuse the two arguments at compile time. AnsiColor.wrap(String) builds the escaped message via plain concatenation, same as before. AnsiColor is nested in FrontendUtils, matching how other enums in this package (e.g. BrowserLiveReload.Backend) are scoped close to their single use; it is public only because the sole external caller, AbstractDevServerRunner in the vaadin-dev-server module, needs to reference it. Also corrects YELLOW from color code 111 (a light blue, not yellow) to 220, a normal warning yellow, as suggested in review. Fixes vaadin#9263 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8EXia4muFka28BmN6niq4
|
Updated (24c2c41) to address @caalador's review:
Tests: Ready for another look whenever convenient. |
|
Hi, the latest GitHub Formatter and Flow Validation workflows are now passing, and the rerun of the TeamCity Gradle Tests also succeeded. The remaining API-DIFF labeling, Validation, Formatter, and Validation-SlowTests statuses still point to the earlier failed TeamCity builds. Since the PR head has not changed, could the remaining TeamCity checks be rerun when convenient? Please also let me know if the API-DIFF check requires a maintainer-applied label for the |
|
Thank you for the contribution. |
#25309) This PR cherry-picks changes from the original PR #25048 to branch 25.2. --- #### Original PR description > ## Description > > `FrontendUtils.console()` passed a caller-controlled `format` parameter > straight into `String.format()`, which static analysis tools flag as > CWE-134 (uncontrolled format string), since the method's contract does not > guarantee the argument is a compile-time constant. > > Additionally, review feedback pointed out that even after removing > `String.format()`, `console(String, String)` still let the color and > message arguments be swapped by mistake, since both were plain `String`s. > > ## Changes > > - `flow-server/.../internal/FrontendUtils.java`: > - The ANSI color `String` constants (`YELLOW`, `RED`, `GREEN`, > `BRIGHT_BLUE`) and `ANSI_RESET` are replaced by a nested `AnsiColor` > enum. Each constant holds its own escape code, and > `AnsiColor.wrap(String message)` concatenates the code, the literal > message, and the reset sequence. > - `console(String format, Object message)` is now > `console(AnsiColor ansiColor, String message)`; its body is > `System.out.print(ansiColor.wrap(message));`. The color and message > arguments can no longer be confused at compile time, and the message is > never interpreted as a format string. > - `AnsiColor` is nested inside `FrontendUtils` (matching how other > single-purpose enums are scoped in this package, e.g. > `BrowserLiveReload.Backend`); it's `public` only because the sole > external caller needs to reference the constants across modules. > - `YELLOW`'s color code is corrected from 111 (a light blue, not yellow) > to 220 (a normal warning yellow), per review. > - `vaadin-dev-server/.../AbstractDevServerRunner.java`: the three > `FrontendUtils.console(...)` call sites now pass > `FrontendUtils.AnsiColor.GREEN`/`RED` instead of the old string > constants. > - `flow-server/.../internal/FrontendUtilsTest.java`: updated to the new > enum-based API and extended with a test asserting the reset sequence is > appended exactly once. > > ## Testing > > `FrontendUtilsTest` verifies: > - a plain message is wrapped with the given color and the reset sequence > - embedded newlines in the message are preserved exactly > - messages containing `%s`, `%c`, `%n`, and a literal `%` are printed > verbatim rather than being interpreted as format specifiers > - the reset sequence is appended exactly once > > Verification commands run locally: > - `mvn -pl flow-server -am -DskipITs -Dtest=FrontendUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test` → **4/4 passed** > - `mvn -pl vaadin-dev-server -am -DskipITs -DskipTests install` → **BUILD SUCCESS** (confirms the only caller module compiles against the new `AnsiColor`-based signature) > - `mvn -pl flow-server spotless:check` → **BUILD SUCCESS** > - `mvn -pl flow-server checkstyle:check` → fails, but with the same pre-existing, unrelated parser error in `flow-server/.../signals/shared/impl/MutableTreeRevision.java` (a record pattern in `instanceof` that the bundled checkstyle grammar can't parse); this file is untouched by this PR and the failure reproduces on `upstream/main` without this change. > > Fixes #9263 > Co-authored-by: 한의준 <fc49854985@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Artur Signell <artur@vaadin.com> Co-authored-by: caalador <mikael.grankvist@vaadin.com>
#25310) This PR cherry-picks changes from the original PR #25048 to branch 25.1. --- #### Original PR description > ## Description > > `FrontendUtils.console()` passed a caller-controlled `format` parameter > straight into `String.format()`, which static analysis tools flag as > CWE-134 (uncontrolled format string), since the method's contract does not > guarantee the argument is a compile-time constant. > > Additionally, review feedback pointed out that even after removing > `String.format()`, `console(String, String)` still let the color and > message arguments be swapped by mistake, since both were plain `String`s. > > ## Changes > > - `flow-server/.../internal/FrontendUtils.java`: > - The ANSI color `String` constants (`YELLOW`, `RED`, `GREEN`, > `BRIGHT_BLUE`) and `ANSI_RESET` are replaced by a nested `AnsiColor` > enum. Each constant holds its own escape code, and > `AnsiColor.wrap(String message)` concatenates the code, the literal > message, and the reset sequence. > - `console(String format, Object message)` is now > `console(AnsiColor ansiColor, String message)`; its body is > `System.out.print(ansiColor.wrap(message));`. The color and message > arguments can no longer be confused at compile time, and the message is > never interpreted as a format string. > - `AnsiColor` is nested inside `FrontendUtils` (matching how other > single-purpose enums are scoped in this package, e.g. > `BrowserLiveReload.Backend`); it's `public` only because the sole > external caller needs to reference the constants across modules. > - `YELLOW`'s color code is corrected from 111 (a light blue, not yellow) > to 220 (a normal warning yellow), per review. > - `vaadin-dev-server/.../AbstractDevServerRunner.java`: the three > `FrontendUtils.console(...)` call sites now pass > `FrontendUtils.AnsiColor.GREEN`/`RED` instead of the old string > constants. > - `flow-server/.../internal/FrontendUtilsTest.java`: updated to the new > enum-based API and extended with a test asserting the reset sequence is > appended exactly once. > > ## Testing > > `FrontendUtilsTest` verifies: > - a plain message is wrapped with the given color and the reset sequence > - embedded newlines in the message are preserved exactly > - messages containing `%s`, `%c`, `%n`, and a literal `%` are printed > verbatim rather than being interpreted as format specifiers > - the reset sequence is appended exactly once > > Verification commands run locally: > - `mvn -pl flow-server -am -DskipITs -Dtest=FrontendUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test` → **4/4 passed** > - `mvn -pl vaadin-dev-server -am -DskipITs -DskipTests install` → **BUILD SUCCESS** (confirms the only caller module compiles against the new `AnsiColor`-based signature) > - `mvn -pl flow-server spotless:check` → **BUILD SUCCESS** > - `mvn -pl flow-server checkstyle:check` → fails, but with the same pre-existing, unrelated parser error in `flow-server/.../signals/shared/impl/MutableTreeRevision.java` (a record pattern in `instanceof` that the bundled checkstyle grammar can't parse); this file is untouched by this PR and the failure reproduces on `upstream/main` without this change. > > Fixes #9263 > Co-authored-by: 한의준 <fc49854985@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Artur Signell <artur@vaadin.com> Co-authored-by: caalador <mikael.grankvist@vaadin.com>
Description
FrontendUtils.console()passed a caller-controlledformatparameterstraight into
String.format(), which static analysis tools flag asCWE-134 (uncontrolled format string), since the method's contract does not
guarantee the argument is a compile-time constant.
Additionally, review feedback pointed out that even after removing
String.format(),console(String, String)still let the color andmessage arguments be swapped by mistake, since both were plain
Strings.Changes
flow-server/.../internal/FrontendUtils.java:Stringconstants (YELLOW,RED,GREEN,BRIGHT_BLUE) andANSI_RESETare replaced by a nestedAnsiColorenum. Each constant holds its own escape code, and
AnsiColor.wrap(String message)concatenates the code, the literalmessage, and the reset sequence.
console(String format, Object message)is nowconsole(AnsiColor ansiColor, String message); its body isSystem.out.print(ansiColor.wrap(message));. The color and messagearguments can no longer be confused at compile time, and the message is
never interpreted as a format string.
AnsiColoris nested insideFrontendUtils(matching how othersingle-purpose enums are scoped in this package, e.g.
BrowserLiveReload.Backend); it'spubliconly because the soleexternal caller needs to reference the constants across modules.
YELLOW's color code is corrected from 111 (a light blue, not yellow)to 220 (a normal warning yellow), per review.
vaadin-dev-server/.../AbstractDevServerRunner.java: the threeFrontendUtils.console(...)call sites now passFrontendUtils.AnsiColor.GREEN/REDinstead of the old stringconstants.
flow-server/.../internal/FrontendUtilsTest.java: updated to the newenum-based API and extended with a test asserting the reset sequence is
appended exactly once.
Testing
FrontendUtilsTestverifies:%s,%c,%n, and a literal%are printedverbatim rather than being interpreted as format specifiers
Verification commands run locally:
mvn -pl flow-server -am -DskipITs -Dtest=FrontendUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test→ 4/4 passedmvn -pl vaadin-dev-server -am -DskipITs -DskipTests install→ BUILD SUCCESS (confirms the only caller module compiles against the newAnsiColor-based signature)mvn -pl flow-server spotless:check→ BUILD SUCCESSmvn -pl flow-server checkstyle:check→ fails, but with the same pre-existing, unrelated parser error inflow-server/.../signals/shared/impl/MutableTreeRevision.java(a record pattern ininstanceofthat the bundled checkstyle grammar can't parse); this file is untouched by this PR and the failure reproduces onupstream/mainwithout this change.Fixes #9263