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

CB-157 follow-up: the remote-URL credential checks only see origin, only https, and never a push URL #189

New Issue
Closed
opened 2026-08-31 03:39:04 +02:00 by ltms · 0 comments
ltms commented 2026-08-31 03:39:04 +02:00
Owner
Copy Link
Copy Source

Follow-up to #157, written against main as it stands today (23ada19). Supersedes PR #173, which
was branched before the current CB-157 code landed and could not be merged without risking it.

GitWorktrees now strips user-info from the origin before adding a worktree
(removeUserInfoFromHttpsOrigin) and refuses the provision if it is still there afterwards
(requireCredentialFreeHttpsOrigin). That is the right shape. But both checks are narrower than the
threat, and the narrowing is invisible — nothing warns that a URL was never looked at.

A member's worktree shares its parent repository's git config. So every URL below is readable by
every member.

The four gaps

1. Only origin is inspected. Both checks are hardcoded to that one remote:

remote.origin.url                    (removeUserInfoFromHttpsOrigin, line ~139, ~142)
git remote get-url --all origin      (requireCredentialFreeHttpsOrigin, line ~164, ~167)

A credential in upstream, fork, mirror or any other remote passes through untouched and
unreported.

2. Push URLs are never inspected. Nothing runs git remote get-url --push. A
remote.<name>.pushurl carrying user-info is invisible to every check we have.

3. Only the https scheme is handled. Both checks test
"https".equalsIgnoreCase(uri.getScheme()), so http://user:pass@host/repo.git passes straight
through. Plain http is worse than https, not better.

4. exec copies command stdout into exception messages, with no way to suppress it:

throw new WorktreeException("exit " + code + " for: " + String.join(" ", command)
        + (out.isBlank() ? "" : "\n" + out));

Several callers run git config --get remote.origin.url and git remote get-url, whose stdout is
a URL. Callers log exceptions, so a failing command on one of those paths can put a URL in a log.
b0c4ced fixed the direct log calls with redactUserInfo; the exception path was not covered.

What to build

  • Enumerate every remote (git remote), and for each check both the fetch URLs
    (get-url --all) and the push URLs (get-url --push --all).
  • Treat any scheme with non-empty user-info as a finding, not just https. Exclude ssh://,
    git+ssh:// and ssh+git:// deliberately and say why in a comment — there the user part selects
    an account and authentication stays in SSH.
  • Add a redacting exec variant so a command whose stdout may be a URL can never copy it into an
    exception message. PR #173's execRedacted seam is the right shape; take it from there.
  • Keep the existing strip-and-refuse behaviour for origin exactly as it is. Do not replace it
    with a warning.     Removing and refusing is stronger than reporting, and that is the part that
    actually stops a credential reaching a member.

Two constraints, both learned the hard way

A diagnostic must never be able to abort a provision. In PR #173 the new check ran first in
add() with no try/catch, and every git call inside it can throw — exec turns a non-zero exit
or the 30-second timeout into a WorktreeException. Any reporting-only code added here must be
wrapped, and its failure logged, never propagated. The strip-and-refuse path is different: that one
is supposed to stop the provision.

Never log the exception's message on that wrapped path — log the exception class only. The
enumerating call (git remote) is not redacted, and its stderr comes from a config that may hold
the very URL this code exists to find.

Acceptance

  • A repo with a credentialed URL on a non-origin remote is reported.
  • A repo with a credentialed pushurl is reported.
  • A repo with an http://user:pass@… remote is reported.
  • A normal ssh:// remote and a credential-free https:// remote produce no report at all.
  • No test's captured log or exception message contains the URL, the user part, the password, or the
    host — assert on their absence explicitly, the way GitWorktreesTest already does.
  • The existing origin strip-and-refuse tests still pass unchanged.
  • Prove each check by removing it and watching its test fail. A test that passes with the check
    deleted is testing the seam, not the behaviour — that shape has shipped dead here three times
    (CB-586, CB-611, and the four routing defects in CB-185).
Follow-up to #157, written against `main` as it stands today (`23ada19`). Supersedes PR #173, which was branched before the current CB-157 code landed and could not be merged without risking it. `GitWorktrees` now strips user-info from the origin before adding a worktree (`removeUserInfoFromHttpsOrigin`) and refuses the provision if it is still there afterwards (`requireCredentialFreeHttpsOrigin`). That is the right shape. But both checks are narrower than the threat, and the narrowing is invisible — nothing warns that a URL was never looked at. A member's worktree shares its parent repository's git config. So every URL below is readable by every member. ## The four gaps **1. Only `origin` is inspected.** Both checks are hardcoded to that one remote: ``` remote.origin.url (removeUserInfoFromHttpsOrigin, line ~139, ~142) git remote get-url --all origin (requireCredentialFreeHttpsOrigin, line ~164, ~167) ``` A credential in `upstream`, `fork`, `mirror` or any other remote passes through untouched and unreported. **2. Push URLs are never inspected.** Nothing runs `git remote get-url --push`. A `remote.<name>.pushurl` carrying user-info is invisible to every check we have. **3. Only the `https` scheme is handled.** Both checks test `"https".equalsIgnoreCase(uri.getScheme())`, so `http://user:pass@host/repo.git` passes straight through. Plain http is worse than https, not better. **4. `exec` copies command stdout into exception messages, with no way to suppress it:** ```java throw new WorktreeException("exit " + code + " for: " + String.join(" ", command) + (out.isBlank() ? "" : "\n" + out)); ``` Several callers run `git config --get remote.origin.url` and `git remote get-url`, whose stdout is a URL. Callers log exceptions, so a failing command on one of those paths can put a URL in a log. `b0c4ced` fixed the direct log calls with `redactUserInfo`; the exception path was not covered. ## What to build - Enumerate **every** remote (`git remote`), and for each check both the fetch URLs (`get-url --all`) and the push URLs (`get-url --push --all`). - Treat **any** scheme with non-empty user-info as a finding, not just `https`. Exclude `ssh://`, `git+ssh://` and `ssh+git://` deliberately and say why in a comment — there the user part selects an account and authentication stays in SSH. - Add a redacting `exec` variant so a command whose stdout may be a URL can never copy it into an exception message. PR #173's `execRedacted` seam is the right shape; take it from there. - Keep the existing strip-and-refuse behaviour for `origin` exactly as it is. **Do not replace it with a warning.** Removing and refusing is stronger than reporting, and that is the part that actually stops a credential reaching a member. ## Two constraints, both learned the hard way **A diagnostic must never be able to abort a provision.** In PR #173 the new check ran first in `add()` with no `try`/`catch`, and every git call inside it can throw — `exec` turns a non-zero exit or the 30-second timeout into a `WorktreeException`. Any reporting-only code added here must be wrapped, and its failure logged, never propagated. The strip-and-refuse path is different: that one is *supposed* to stop the provision. **Never log the exception's message on that wrapped path** — log the exception class only. The enumerating call (`git remote`) is not redacted, and its stderr comes from a config that may hold the very URL this code exists to find. ## Acceptance - A repo with a credentialed URL on a **non-origin** remote is reported. - A repo with a credentialed **`pushurl`** is reported. - A repo with an **`http://user:pass@…`** remote is reported. - A normal `ssh://` remote and a credential-free `https://` remote produce **no** report at all. - No test's captured log or exception message contains the URL, the user part, the password, or the host — assert on their absence explicitly, the way `GitWorktreesTest` already does. - The existing `origin` strip-and-refuse tests still pass unchanged. - Prove each check by removing it and watching its test fail. A test that passes with the check deleted is testing the seam, not the behaviour — that shape has shipped dead here three times (CB-586, CB-611, and the four routing defects in CB-185).
ltms closed this issue 2026-08-31 04:32:15 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#189
Delete Branch "%!s()"

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?