CB-189: cover every remote, both URLs, and any non-SSH scheme in the credential check #195

2026-08-31T04:18:15+02:00

agent commented

2026-08-31 04:18:15 +02:00

Fixes #189.

GitWorktrees's credential check only ever inspected origin's HTTPS fetch URL. Four gaps closed, additively (the existing origin/https strip-and-refuse behaviour is unchanged):

Enumerate every remote via git remote, not just origin.
Check both fetch (get-url --all) and push (get-url --push --all) URLs.
Treat any scheme with non-empty user-info as a finding, not just https (excludes ssh://, git+ssh://, ssh+git:// deliberately — there the user part selects an account and authentication stays in SSH).
Added execRedacted, an exec variant that never copies captured stdout into a WorktreeException message, since several callers' stdout is itself a URL.

Both hard constraints from the issue are honored: the new reportRemoteUrlsWithUserInfo check is wrapped end-to-end so it can never abort a provision (git calls inside it can throw), and on any failure it logs only the exception's class, never its message — the enumerating git remote call is not redacted, and its stderr can itself hold the URL.

Tests added in GitWorktreesTest (all real git, TempDir-based, matching the existing style):

aCredentialedUrlOnANonOriginRemoteIsReported
aCredentialedPushUrlIsReported
anHttpUrlWithCredentialsIsReported
anSshRemoteAndACleanHttpsRemoteProduceNoReport

Each uses a logback ListAppender on GitWorktrees's own logger and asserts both that the expected report fires and that no captured log message contains the URL, user, password, or host. The existing origin strip-and-refuse tests are untouched and still pass.

I proved each of the four checks by temporarily disabling it and watching its test fail, then reverted (see PR description / worker report for the exact failure text); all sabotage has been reverted before this commit.

Build: mvn clean install — Tests run: 1016, Failures: 0, Errors: 0, Skipped: 0. BUILD SUCCESS. GitWorktreesTest: 29/29.

Fixes #189. GitWorktrees's credential check only ever inspected origin's HTTPS fetch URL. Four gaps closed, additively (the existing origin/https strip-and-refuse behaviour is unchanged): 1. Enumerate every remote via git remote, not just origin. 2. Check both fetch (get-url --all) and push (get-url --push --all) URLs. 3. Treat any scheme with non-empty user-info as a finding, not just https (excludes ssh://, git+ssh://, ssh+git:// deliberately — there the user part selects an account and authentication stays in SSH). 4. Added execRedacted, an exec variant that never copies captured stdout into a WorktreeException message, since several callers' stdout is itself a URL. Both hard constraints from the issue are honored: the new reportRemoteUrlsWithUserInfo check is wrapped end-to-end so it can never abort a provision (git calls inside it can throw), and on any failure it logs only the exception's class, never its message — the enumerating git remote call is not redacted, and its stderr can itself hold the URL. Tests added in GitWorktreesTest (all real git, TempDir-based, matching the existing style): - aCredentialedUrlOnANonOriginRemoteIsReported - aCredentialedPushUrlIsReported - anHttpUrlWithCredentialsIsReported - anSshRemoteAndACleanHttpsRemoteProduceNoReport Each uses a logback ListAppender on GitWorktrees's own logger and asserts both that the expected report fires and that no captured log message contains the URL, user, password, or host. The existing origin strip-and-refuse tests are untouched and still pass. I proved each of the four checks by temporarily disabling it and watching its test fail, then reverted (see PR description / worker report for the exact failure text); all sabotage has been reverted before this commit. Build: mvn clean install — Tests run: 1016, Failures: 0, Errors: 0, Skipped: 0. BUILD SUCCESS. GitWorktreesTest: 29/29.

agent added 1 commit 2026-08-31 04:18:15 +02:00

CB-189: cover every remote, both URLs, and any non-SSH scheme in the credential check

CI / build (pull_request) Successful in 1m7s

Details

CI / contract (pull_request) Successful in 1m7s

Details

d1fd5700f5

GitWorktrees only ever inspected origin's HTTPS fetch URL for embedded credentials. A
credential on any other remote, on a pushurl, or on a plain http:// URL passed through
unreported. Adds an additive, reporting-only check that enumerates every remote and both
its fetch and push URLs, flagging non-empty user-info on any non-SSH-family scheme.

The existing origin/https strip-and-refuse behaviour is untouched. The new check is
wrapped so it can never abort a provision, and on failure logs only the exception's
class, never its message, since the enumerating `git remote` call is not redacted.
Also adds execRedacted, an exec variant that never copies captured stdout into a
WorktreeException message, for commands whose stdout may itself be a credentialed URL.

agent added 1 commit 2026-08-31 04:29:51 +02:00

CB-189 review fix: redact the three pre-existing URL-reading exec calls

CI / contract (pull_request) Successful in 1m11s

Details

CI / build (pull_request) Successful in 1m42s

Details

6fc301d62c

Review found that execRedacted was applied only to the new remote-enumeration code
and left three pre-existing calls reading remote.origin.url through the plain,
unredacted exec: removeUserInfoFromHttpsOrigin, requireCredentialFreeHttpsOrigin, and
configureHttpsUrlRewriteForSshOrigin. A non-zero exit or timeout on any of those could
still have copied the credentialed URL into a WorktreeException message. Switches all
three to execRedacted; the set-url write in removeUserInfoFromHttpsOrigin is left on
plain exec with a comment explaining why (it writes the already-stripped URL, not a
read).

Widens the shared exec(Map, boolean, String...) overload to package-private, the same
test-seam pattern already used by the afterWorktreeAdded constructor parameter, and
adds a test that drives it directly with a synthetic failing command whose stdout
carries a marker (passed via env, not argv, so the always-printed command line can't
carry it) and asserts the marker never reaches the exception message.

ltms merged commit a49e96835a into main

2026-08-31 04:32:15 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fleet/fleetd#195