Copilot commented on code in PR #156:
URL:
https://github.com/apache/maven-jarsigner-plugin/pull/156#discussion_r3645413602
##########
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java:
##########
@@ -373,7 +373,9 @@ protected String getCommandlineInfo(final Commandline
commandLine) {
}
String commandLineInfo = commandLine.toString();
- commandLineInfo = StringUtils.replace(commandLineInfo, this.storepass,
"'*****'");
+ if (this.storepass != null) {
+ commandLineInfo = StringUtils.replace(commandLineInfo,
this.storepass, "'*****'");
+ }
Review Comment:
Consider guarding against empty-string passwords as well as null. If
`storepass` can be `\"\"`, some `StringUtils.replace` implementations can
behave unexpectedly (e.g., repeated insertions or excessive work). A safer
check is to only redact when the password is non-empty (e.g.,
`isNotEmpty`/`isNotBlank` depending on the StringUtils in use).
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -247,7 +247,7 @@ public JarsignerSignMojo(
protected String getCommandlineInfo(final Commandline commandLine) {
String commandLineInfo = commandLine != null ? commandLine.toString()
: null;
- if (commandLineInfo != null) {
+ if (commandLineInfo != null && this.keypass != null) {
commandLineInfo = StringUtils.replace(commandLineInfo,
this.keypass, "'*****'");
}
Review Comment:
Same as `storepass`: consider also excluding empty-string `keypass` values,
not just null. Redacting an empty search string can lead to pathological
behavior depending on the `StringUtils.replace` implementation, so it’s safer
to only redact when `keypass` is non-empty.
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerVerifyMojoTest.java:
##########
@@ -133,6 +136,22 @@ public void testVerifyFailure() throws Exception {
containsString(RESULT_ERROR.getCommandline().toString()));
}
+ /**
+ * Verifies that {@code getCommandlineInfo} does not throw when
+ * {@code storepass} is {@code null} (the default).
+ */
+ @Test
+ public void testGetCommandlineInfoWithNullStorepass() throws Exception {
+ JarsignerVerifyMojo mojo = mojoTestCreator.configure(configuration);
Review Comment:
This test currently relies on the fixture default that `storepass` is null,
and only asserts non-null output. To make the regression more robust,
explicitly set `storepass` to null on the configured mojo (if a setter/field
access exists in the test harness) and assert the returned `info` equals
`cmd.toString()` (i.e., no redaction occurred and, most importantly, no
exception was thrown).
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoTest.java:
##########
@@ -185,6 +188,22 @@ public void testDontProcessMainArtifact() throws Exception
{
verify(jarSigner, never()).execute(any()); // Should not try to sign
anything
}
+ /**
+ * Verifies that {@code getCommandlineInfo} does not throw when
+ * {@code keypass} is {@code null} (the default).
+ */
+ @Test
+ public void testGetCommandlineInfoWithNullKeypass() throws Exception {
+ JarsignerSignMojo mojo = mojoTestCreator.configure(configuration);
Review Comment:
Same improvement as the verify-mojo test: explicitly ensure `keypass` is
null on the mojo instead of relying on defaults, and assert `info` equals
`cmd.toString()` to make the regression clearer and less dependent on fixture
configuration.
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerVerifyMojoTest.java:
##########
@@ -133,6 +136,22 @@ public void testVerifyFailure() throws Exception {
containsString(RESULT_ERROR.getCommandline().toString()));
}
+ /**
+ * Verifies that {@code getCommandlineInfo} does not throw when
+ * {@code storepass} is {@code null} (the default).
+ */
+ @Test
+ public void testGetCommandlineInfoWithNullStorepass() throws Exception {
+ JarsignerVerifyMojo mojo = mojoTestCreator.configure(configuration);
+ Shell shell = new Shell();
+ Commandline cmd = new Commandline(shell);
+ cmd.setExecutable("jarsigner");
+ cmd.addArguments("my-project.jar", "myalias");
+
+ String info = mojo.getCommandlineInfo(cmd);
+ assertNotNull(info);
Review Comment:
This test currently relies on the fixture default that `storepass` is null,
and only asserts non-null output. To make the regression more robust,
explicitly set `storepass` to null on the configured mojo (if a setter/field
access exists in the test harness) and assert the returned `info` equals
`cmd.toString()` (i.e., no redaction occurred and, most importantly, no
exception was thrown).
##########
src/test/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojoTest.java:
##########
@@ -185,6 +188,22 @@ public void testDontProcessMainArtifact() throws Exception
{
verify(jarSigner, never()).execute(any()); // Should not try to sign
anything
}
+ /**
+ * Verifies that {@code getCommandlineInfo} does not throw when
+ * {@code keypass} is {@code null} (the default).
+ */
+ @Test
+ public void testGetCommandlineInfoWithNullKeypass() throws Exception {
+ JarsignerSignMojo mojo = mojoTestCreator.configure(configuration);
+ Shell shell = new Shell();
+ Commandline cmd = new Commandline(shell);
+ cmd.setExecutable("jarsigner");
+ cmd.addArguments("my-project.jar", "myalias");
+
+ String info = mojo.getCommandlineInfo(cmd);
+ assertNotNull(info);
Review Comment:
Same improvement as the verify-mojo test: explicitly ensure `keypass` is
null on the mojo instead of relying on defaults, and assert `info` equals
`cmd.toString()` to make the regression clearer and less dependent on fixture
configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
