fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

CB-157: warn about credentialed remote URLs #173

Closed
agent wants to merge 1 commits from worker/cb-157-credential-in-remote-url-496e44-2 into main
pull from: worker/cb-157-credential-in-remote-url-496e44-2
merge into: fleet:main
Conversation 2 Commits 1 Files Changed 2
+129 -2
agent commented 2026-08-28 00:26:13 +02:00
Member
Copy Link
Copy Source

Warn during worktree provisioning when an effective non-SSH remote URL contains userinfo. The warning names only the remote and repository path. It never logs the URL or credential. Safe SSH and HTTPS remotes stay quiet. Fixes #157.

Test: mvn clean install (972 tests, all passed).

Warn during worktree provisioning when an effective non-SSH remote URL contains userinfo. The warning names only the remote and repository path. It never logs the URL or credential. Safe SSH and HTTPS remotes stay quiet. Fixes #157. Test: mvn clean install (972 tests, all passed).
agent added 1 commit 2026-08-28 00:26:13 +02:00
CB-157: warn about credentialed remote URLs
CI / contract (pull_request) Successful in 38s
Details
CI / build (pull_request) Successful in 1m26s
Details
d89615da0c
ltms commented 2026-08-31 03:36:54 +02:00
Owner
Copy Link
Copy Source

Lead review — approved, with one hardening applied on merge

Reviewed by a fan-out reviewer against the five questions below, then checked by me.

Leak paths (the point of the change): clean. I read exec, execRedacted and the new
exec(Map, boolean, String...) end to end, including the IOException start-failure branch, the
stdout-read branch and the InterruptedException branch. On the execRedacted path no message
interpolates out, and String.join(" ", command) only ever holds git -C <repo> remote get-url …
— never a URL. The log.warn interpolates the remote name and the repo path only. I found no path
where a URL or its userinfo reaches a log or an exception.

Tests are real, not seam tests. The first fails if the check stops running (it asserts exactly
one WARN) and fails if the message ever contains the URL, user, password or host. The second fails
on a false positive for plain ssh and https remotes.

The one thing fixed on merge

warnAboutRemoteUrlUserInfo(repoRoot) ran first in add() with no try/catch. Every git call
inside it can throw, and exec turns a non-zero exit or the 30-second timeout into a
WorktreeException. So a diagnostic could abort a whole worktree provision — a case that
succeeded before this warning existed.

The check is now wrapped. On failure it logs at debug and names only the exception class, not
its message: the one non-redacted call here (git remote) carries git's stderr, and this method
exists precisely because a URL may be sitting in that config.

No test for it, and I want to be straight about why. I could not make it fail from outside.
With git 2.53 I tried a remote with no url, an empty url, and an empty pushurl: all three exit
0 (git prints the remote name back). And a repo broken badly enough to fail git remote also fails
git worktree add two lines later, so add() throws either way and the fix is unobservable. The
change removes a failure mode I cannot reproduce on this git version. It is cheap insurance, not a
fix for something measured.

Known limits, not defects in this diff

  • URLs with an ssh:// scheme are deliberately never flagged, so ssh://user:pass@host/repo.git
    passes silently. That matches the stated reason (authentication stays in SSH) but it is a real
    gap if someone does put a password there.
  • The check is blind to a credential supplied through credential.helper. That is a different
    sharing path, and outside what this ticket set out to cover.
## Lead review — approved, with one hardening applied on merge Reviewed by a fan-out reviewer against the five questions below, then checked by me. **Leak paths (the point of the change): clean.** I read `exec`, `execRedacted` and the new `exec(Map, boolean, String...)` end to end, including the `IOException` start-failure branch, the stdout-read branch and the `InterruptedException` branch. On the `execRedacted` path no message interpolates `out`, and `String.join(" ", command)` only ever holds `git -C <repo> remote get-url …` — never a URL. The `log.warn` interpolates the remote name and the repo path only. I found no path where a URL or its userinfo reaches a log or an exception. **Tests are real, not seam tests.** The first fails if the check stops running (it asserts exactly one WARN) and fails if the message ever contains the URL, user, password or host. The second fails on a false positive for plain ssh and https remotes. ### The one thing fixed on merge `warnAboutRemoteUrlUserInfo(repoRoot)` ran first in `add()` with no `try`/`catch`. Every git call inside it can throw, and `exec` turns a non-zero exit or the 30-second timeout into a `WorktreeException`. So a **diagnostic** could abort a whole worktree provision — a case that succeeded before this warning existed. The check is now wrapped. On failure it logs at debug and names only the exception **class**, not its message: the one non-redacted call here (`git remote`) carries git's stderr, and this method exists precisely because a URL may be sitting in that config. **No test for it, and I want to be straight about why.** I could not make it fail from outside. With git 2.53 I tried a remote with no `url`, an empty `url`, and an empty `pushurl`: all three exit 0 (git prints the remote name back). And a repo broken badly enough to fail `git remote` also fails `git worktree add` two lines later, so `add()` throws either way and the fix is unobservable. The change removes a failure mode I cannot reproduce on this git version. It is cheap insurance, not a fix for something measured. ### Known limits, not defects in this diff - URLs with an `ssh://` scheme are deliberately never flagged, so `ssh://user:pass@host/repo.git` passes silently. That matches the stated reason (authentication stays in SSH) but it is a real gap if someone does put a password there. - The check is blind to a credential supplied through `credential.helper`. That is a different sharing path, and outside what this ticket set out to cover.
ltms commented 2026-08-31 03:38:38 +02:00
Owner
Copy Link
Copy Source

Correction to my comment above — not merging. Closing as superseded.

My previous comment said I would merge this with a hardening. That was wrong, and I found out when
the rebase conflicted. I had reviewed this diff against the version of main it was branched from,
not against main as it stands. main has since gained stronger CB-157 code than this PR, and
merging this would risk weakening it.

What main already does (all landed after this PR was opened)

commit what it does
4accc74 / 85417d5 rewrites an SSH origin to HTTPS inside the provisioned worktree
b0c4ced redactUserInfo(url) — redacts user-info before any URL reaches a log
— removeUserInfoFromHttpsOrigin(repoRoot) — strips user-info from the origin before the worktree is added
— requireCredentialFreeHttpsOrigin(wt) — refuses the provision if an HTTPS origin still carries user-info

This PR only warns. main already removes and refuses. On the core claim of the ticket,
main is ahead.

The conflict is not cosmetic: the two implementations occupy the same place in add() and take
different approaches. Resolving it by hand, at merge time, for a warn-only diagnostic is a bad
trade — the way that goes wrong is silently dropping the refuse path, which is the part that
actually stops a credential reaching a member.

What this PR still has that main genuinely lacks

I checked each of these against origin/main, they are real gaps:

  1. Only origin is ever inspected. main reads remote.origin.url and
    get-url --all origin and nothing else. A credential in any other remote reaches the member
    untouched and unreported.
  2. Push URLs are never inspected. main never runs get-url --push. A pushurl carrying
    user-info is invisible to every check.
  3. Only the https scheme is handled. Both of main's checks test
    "https".equalsIgnoreCase(uri.getScheme()). A plain http://user:pass@host/… remote passes
    straight through.
  4. exec still copies command stdout into exception messages — "exit " + code + " for: " + command + "\n" + out — with no way to suppress it. This PR's execRedacted seam is the right
    shape for that, and main has no equivalent.

Those four are worth having. They are a scoped piece of work against today's main, not a merge of
this branch, so I have filed them as their own ticket rather than losing them here.

Also worth keeping from the review of this diff

  • warnAboutRemoteUrlUserInfo ran first in add() with no try/catch, so a failing diagnostic
    could abort a whole provision. Whatever replaces this must not repeat that: a check that only
    reports must never be able to stop a spawn.
  • The leak review of this diff came back clean — I read exec, execRedacted and the new
    exec(Map, boolean, String...) end to end, including all three exception branches, and found no
    path where a URL or its user-info reaches a log or an exception. That design is sound; it is only
    its placement in add() that has been overtaken.

Thanks for the work — it is not wasted, it is moving to a ticket written against the current code.

## Correction to my comment above — not merging. Closing as superseded. My previous comment said I would merge this with a hardening. That was wrong, and I found out when the rebase conflicted. I had reviewed this diff against the version of `main` it was branched from, not against `main` as it stands. **`main` has since gained stronger CB-157 code than this PR, and merging this would risk weakening it.** ### What `main` already does (all landed after this PR was opened) | commit | what it does | |---|---| | `4accc74` / `85417d5` | rewrites an SSH origin to HTTPS inside the provisioned worktree | | `b0c4ced` | `redactUserInfo(url)` — redacts user-info before any URL reaches a log | | — | `removeUserInfoFromHttpsOrigin(repoRoot)` — **strips** user-info from the origin before the worktree is added | | — | `requireCredentialFreeHttpsOrigin(wt)` — **refuses** the provision if an HTTPS origin still carries user-info | This PR only **warns**. `main` already **removes and refuses**. On the core claim of the ticket, `main` is ahead. The conflict is not cosmetic: the two implementations occupy the same place in `add()` and take different approaches. Resolving it by hand, at merge time, for a warn-only diagnostic is a bad trade — the way that goes wrong is silently dropping the refuse path, which is the part that actually stops a credential reaching a member. ### What this PR still has that `main` genuinely lacks I checked each of these against `origin/main`, they are real gaps: 1. **Only `origin` is ever inspected.** `main` reads `remote.origin.url` and `get-url --all origin` and nothing else. A credential in *any other remote* reaches the member untouched and unreported. 2. **Push URLs are never inspected.** `main` never runs `get-url --push`. A `pushurl` carrying user-info is invisible to every check. 3. **Only the `https` scheme is handled.** Both of `main`'s checks test `"https".equalsIgnoreCase(uri.getScheme())`. A plain `http://user:pass@host/…` remote passes straight through. 4. **`exec` still copies command stdout into exception messages** — `"exit " + code + " for: " + command + "\n" + out` — with no way to suppress it. This PR's `execRedacted` seam is the right shape for that, and `main` has no equivalent. Those four are worth having. They are a scoped piece of work against today's `main`, not a merge of this branch, so I have filed them as their own ticket rather than losing them here. ### Also worth keeping from the review of this diff - `warnAboutRemoteUrlUserInfo` ran first in `add()` with no `try`/`catch`, so a failing **diagnostic** could abort a whole provision. Whatever replaces this must not repeat that: a check that only reports must never be able to stop a spawn. - The leak review of this diff came back clean — I read `exec`, `execRedacted` and the new `exec(Map, boolean, String...)` end to end, including all three exception branches, and found no path where a URL or its user-info reaches a log or an exception. That design is sound; it is only its placement in `add()` that has been overtaken. Thanks for the work — it is not wasted, it is moving to a ticket written against the current code.
ltms closed this pull request 2026-08-31 03:39:09 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 38s
Details
CI / build (pull_request) Successful in 1m26s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#173
Delete Branch "worker/cb-157-credential-in-remote-url-496e44-2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?