Mask masked arguments in ArgumentListBuilder#toStringWithQuote

[LHMQ878]

ArgumentListBuilder.toString() replaces masked arguments with ******. toStringWithQuote() was character-for-character identical to it except that it iterated args directly and so never consulted the mask:

public String toStringWithQuote() {
    StringBuilder buf = new StringBuilder();
    for (String arg : args) {              // <-- no index, so no mask lookup
        if (!buf.isEmpty())  buf.append(' ');
        if (arg.indexOf(' ') >= 0 || arg.isEmpty())
            buf.append('"').append(arg).append('"');
        else
            buf.append(arg);
    }
    return buf.toString();
}

Both methods exist to render the command line for a human, and this one's Javadoc says it is for "informational/logging purposes" — which is precisely where a masked argument must not appear.

This is not only theoretical. Around fourteen plugins call toStringWithQuote(), and at least one has a live leak. In mercurial-plugin, HgExe.findHgExe puts the credentials into the builder:

b.add("--config");
b.addMasked("auth.jenkins.password=" + upc.getPassword().getPlainText());

and HgExe.popen logs the same builder when the command fails:

} else {
    listener.error("Failed to run " + args.toStringWithQuote());

So any failing hg invocation writes the password into the build log in plain text. Other callers pass it to LOG.log(...) (xshell-plugin does this in three places), which lands the same content in the Jenkins system log.

The fix makes the mask apply, and folds the two renderings into one private method so that a future change to the masking of one cannot silently miss the other — the divergence is how this arose in the first place. For a builder with no masked arguments the output is byte-identical to before.

Testing done

Two tests added to ArgumentListBuilderTest:

• toStringWithQuoteMasksMaskedArguments — reproduces the mercurial-plugin shape (hg --config <masked password> clone) and asserts both that the output is hg --config ****** clone and that the secret does not appear anywhere in it.
• toStringWithQuoteQuotesTheSameWayAsToString — asserts the two methods agree and that the space/empty-argument quoting is unchanged (cmd "has space" "" ****** plain).

Control experiment, running the new tests against the unmodified ArgumentListBuilder:

toStringWithQuoteMasksMaskedArguments()      FAILED
  Expected: is "hg --config ****** clone"
       but: was "hg --config auth.jenkins.password=s3cr3t clone"
toStringWithQuoteQuotesTheSameWayAsToString() FAILED
  Expected: is "cmd \"has space\" \"\" ****** plain"
       but: was "cmd \"has space\" \"\" \"secret value\" plain"

With the fix applied, all 13 enabled tests in ArgumentListBuilderTest pass (the 14th is the pre-existing @Disabled testToWindowsCommandMasked). The 11 pre-existing tests pass either way, so nothing that previously asserted this behaviour changes — there was no coverage of the masking of this method. Notably testToWindowsCommand, which asserts exact toString() output including a ******, is unaffected.

Screenshots (UI changes only)

Before

After

Proposed changelog entries

• Replace masked arguments with ****** in ArgumentListBuilder#toStringWithQuote, which previously printed them in plain text; plugins that log this string, such as Mercurial, no longer leak credentials into the build log.

Proposed changelog category

/label bug

Proposed upgrade guidelines

N/A

Submitter checklist

• The issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood.
• There is automated testing or an explanation as to why this change has no tests.
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate. (No new public API; the one new method is private.)
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable. (N/A)
• UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. (N/A)
• For dependency updates, there are links to external changelogs and, if possible, full differentials. (N/A)
• For new APIs and extension points, there is a link to at least one consumer. (N/A)

[Copilot]

Pull request overview

This pull request fixes a credentials-leak risk by ensuring ArgumentListBuilder#toStringWithQuote() applies argument masking (replacing masked args with ******) the same way as toString(), which is important because toStringWithQuote() is used for logging/informational output.

Changes:

• Refactors toString() and toStringWithQuote() to share a single rendering implementation that applies masking consistently.
• Updates Javadoc to explicitly document masking behavior for toStringWithQuote().
• Adds JUnit tests to verify masking and quoting behavior for toStringWithQuote().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
core/src/main/java/hudson/util/ArgumentListBuilder.java	Makes toStringWithQuote() consult the mask (via shared renderForDisplay()), preventing masked args from leaking in logs.
core/src/test/java/hudson/util/ArgumentListBuilderTest.java	Adds regression tests covering masking and quoting equivalence between toStringWithQuote() and toString().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[LHMQ878]

The second half is right and is fixed in 95c46267. The first half is not — addMasked does not rewrite its argument, so the assertion is not vacuous.

On the masking claim. The premise is that the argument passed to addMasked "is already auth.jenkins.******". It isn't. addMasked(String) stores the string verbatim and only records an index in a BitSet:

public void addMasked(String string) {      // ArgumentListBuilder.java:435
    add(string, true);
}

public ArgumentListBuilder add(String a, boolean mask) {   // :90
    if (a != null) {
        if (mask) {
            this.mask.set(args.size());     // index recorded; `a` untouched
        }
        args.add(a);
    }
    return this;
}

So after addMasked("auth.jenkins.password=s3cr3t"), args holds that exact string including s3cr3t, and mask holds its index. Substitution happens only at render time, in the method under test. s3cr3t is a live substring of the input, and asserting its absence from the output is exactly the leak check.

The control experiment in the PR description shows this directly — against the unmodified ArgumentListBuilder, where the render ignores the mask, the test fails with:

Expected: is "hg --config ****** clone"
     but: was "hg --config auth.jenkins.password=s3cr3t clone"

s3cr3t is present in the output. A vacuous assertion could not produce that. Both new tests fail before the fix and pass after it, which is the property that makes them regression tests.

On recomputing the render. Fair, and worth doing for a reason beyond tidiness: with two separate calls a reader has to know they are deterministic before the pair of assertions means anything together. Hoisting it makes the not(containsString(...)) check demonstrably run against the same string the is(...) check just matched:

String rendered = builder.toStringWithQuote();
assertThat(rendered, is("hg --config ****** clone"));
assertThat(rendered, not(containsString("s3cr3t")));

Same in toStringWithQuoteQuotesTheSameWayAsToString, where builder.toString() stays a separate call since comparing the two renderings is the point of that test. No assertion changed meaning; 13/13 enabled tests still pass.

[LHMQ878]

The red Tests / linux-jdk21 here is an unrelated flake, not a regression from this PR. Recording the evidence so nobody has to re-derive it.

The failure is hudson.cli.Security3630Test.testConcurrentCliSessionPairing — the CLI full-duplex HTTP session-pairing stress test. It has no connection to this change: the test file references neither ArgumentListBuilder nor toStringWithQuote, and this PR touches only ArgumentListBuilder.java plus its own test. There is no code path from argument quoting to FullDuplexHttpService's session map.

The same commit is green on the other two matrix legs:

leg	result
Linux - JDK 25	success
Windows - JDK 25	success
Linux - JDK 21	failure (Security3630Test)
ath-linux-jdk21-firefox	success

A real defect in argument masking would not be JDK-21-only while passing on JDK 25 on both OSes. Everything else is green too — CheckStyle, SpotBugs, Java Compiler, JavaDoc, Code Coverage, ESLint, Stylelint (13 of 15 checks pass; the second red, Jenkins, is the aggregate reporting Unstable for the same test).

The test has a known flake history. #27087, "Refactor Security3630Test to eliminate ProcessBuilder flakiness", merged 2026-07-17 — the most recent commit to touch this file. So it was already recognised as flaky and had one fix attempt; this looks like residual flakiness from the same source rather than something new. The test is concurrent by construction (CONCURRENCY threads × ITERATIONS, all released simultaneously from a CountDownLatch), and its own body concedes the timing sensitivity:

} catch (Exception e) {
    // Expected under heavy concurrent load: timeouts, connection resets, 500s.
}

I also checked the 24 most recently updated open PRs from other authors and none currently show this failure, so I'm not claiming it's failing for everyone right now — only that it is timing-dependent, has a documented flake history, and cannot be reached from this diff.

Happy to rebase to pick up a fresh CI run if that's the easiest way to confirm, or to leave it if a maintainer would rather just re-run the one leg.

[MarkEWaite]

Happy to rebase to pick up a fresh CI run if that's the easiest way to confirm, or to leave it if a maintainer would rather just re-run the one leg.

No need. I have permission to run the job again. I've started a

[LHMQ878]

Thanks for re-running it — the new build came back green, so the Security3630Test failure was indeed the flake and not this change.

continuous-integration/jenkins/pr-head is now success on 95c4626 with no code change in between, which is the cleanest possible confirmation: same commit, same diff, different result.

Nothing outstanding from my side — the PR is ready whenever you'd like to take another look.