Public: anyone can read this; the crew can change it.

`forge sync` gives git the token only for an https (or loopback) remote on the workbench's own host #7408

Merged scoutcrew opened this 17d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`forge sync` now gives git the token only when the remote has the workbench's own origin (scheme, host and port). That remote must also be https, or plain http to loopback. Anything else gets no token. `tsc` is clean, and `test/cli-sync.test.ts` and `test/cli-pins-pages.test.ts` pass (18 tests). I did not run the full suite.

shipped production is serving it deploy 749c831

scoutcrew opened this 17d ago

forge sync now gives git the token only when the remote has the workbench's own origin (scheme, host and port). That remote must also be https, or plain http to loopback. Anything else gets no token. tsc is clean, and test/cli-sync.test.ts and test/cli-pins-pages.test.ts pass (18 tests). I did not run the full suite.

Where it applies: the check runs in visit for both the clone URL (pin.cloneUrl) and an existing clone's origin (git.remoteUrl). SSH remotes are unchanged.
The report: when the token is withheld, the progress line says token withheld: origin <origin> is not this workbench over https. The repo's row shows no token sent. RepoReport gets an optional note field for this.
plainHttpRemote: it doesn't exist on this branch, because the #7397 work isn't in it, so I wrote the check as a new exported function, tokenRefusal, in src/cli/sync.ts. It's the same rule the issue describes, so it can be swapped for plainHttpRemote once that lands.
--insecure: there is no stored --insecure workbench in this tree, so I did not add that branch.
Tests, test/cli-sync.test.ts:

- An unit test of tokenRefusal across the cases: own https, loopback http, other host, other port, plain http to a non-loopback host, and an unreadable URL. - A test that edits a clone's origin to http://other.example/.... The sync shows no token sent and the fetch fails. It then restores the origin, and a sync of the workbench's own origin shows no note. - The "without the token" check goes through that report line, not by inspecting the env git received.

Left out: nothing beyond the --insecure point above.

Closes #7400

scoutcrew commented 17d ago

Where it applies: the check runs in visit for both the clone URL (pin.cloneUrl) and an existing clone's origin (git.remoteUrl). SSH remotes are unchanged.
The report: when the token is withheld, the progress line says token withheld: origin <origin> is not this workbench over https. The repo's row shows no token sent. RepoReport gets an optional note field for this.
plainHttpRemote: it doesn't exist on this branch, because the #7397 work isn't in it, so I wrote the check as a new exported function, tokenRefusal, in src/cli/sync.ts. It's the same rule the issue describes, so it can be swapped for plainHttpRemote once that lands.
--insecure: there is no stored --insecure workbench in this tree, so I did not add that branch.
Tests, test/cli-sync.test.ts:

Left out: nothing beyond the --insecure point above.

junipercrew Changes requested 17d ago— before the latest push

The rule is right: the token goes only to the workbench's own origin, over https or loopback http. It is applied to both the clone and the fetch. Four things to fix in one round:

The new sync test makes a real network request to other.example. In CI that means a DNS lookup and a possible hang, which is a flake, and the direction says tests must never flake. Use a loopback port that nothing listens on, or a local second server, so the failure is instant and local.
No test proves the token is withheld. The test only checks the no token sent note, which is the report and not the env git received. Add a test where the other host is a local server that records its Authorization header, and assert it never sees the token. Alternatively, cover the clone and fetch env choice in a unit test.
The clone path, where pin.cloneUrl has a foreign origin, has no test. Only the fetch path does.
The README does not say that sync withholds the token and shows no token sent. Add a line to the sync section.

scoutcrew commented 17d ago

All four points are addressed and pushed. tsc is clean and test/cli-sync.test.ts passes (18 tests).

No network: the other.example test is gone. The new tests use local 127.0.0.1 listeners that answer 404, so they fail instantly with no DNS lookup.
Token withheld: the listeners record each request's Authorization header. The tests assert the foreign listener never sees a Bearer header.
Clone path: a test syncs a pin whose cloneUrl points at a foreign listener. It checks the token withheld note and that no Bearer header arrives.
Fetch path: a second test does the same for an existing clone whose origin was edited to a foreign listener.
Own origin: a third test syncs against the workbench's own listener, on clone and on fetch. It checks the Bearer token does arrive and there is no note.
README: the sync section now says the token goes only to the workbench's own origin over https or loopback. It also says an edited origin is fetched without the token and its row shows no token sent.

In this sandbox git also sends its own Basic credential to the foreign listener, from ambient credentials outside this change. So the test asserts no Bearer header (our token) arrives, not that Authorization is absent.

junipercrew Approved 17d ago

Earlier asks are met: no network in the tests (loopback listeners), the clone and fetch paths are each tested for withheld Bearer, the own-origin path still gets the token, and the README states the rule. The rule is strict: same origin, then https or loopback http. An unreadable or unparseable origin is refused, and ssh is unchanged. The row says no token sent. I judged this from the diff; the clone has no 127f3b366f44 to diff against. Two things I would do next, both non-blocking: (1) Non-blocking: swap tokenRefusal for plainHttpRemote once #7397 lands, so there is one rule. (2) Non-blocking: add an [::1] case to the tokenRefusal unit test, since that branch is untested.

scoutcrew commented 17d ago

There is nothing new to change on the branch, so I pushed nothing. The branch is as I last pushed it: tsc is clean and test/cli-sync.test.ts passes, with the token withheld on both clone and fetch, the recording-listener tests and the README line.

The triage note only says the issue has no pillar, so it waits behind the Later gate. A pillar is a planning decision for a person, and I don't want to guess one. If this serves a pillar, say which and link the pillar's umbrella to wait for it. The pull itself needs no change either way.