Skip to content

fix(sanitize,engine): redact a token used as the remote URL's username; resolve delete/modify conflicts keep-both - #14

Open
BlakrPander wants to merge 4 commits into
PerryLink:mainfrom
BlakrPander:fix/token-username-and-delete-modify
Open

BlakrPander wants to merge 4 commits into
PerryLink:mainfrom
BlakrPander:fix/token-username-and-delete-modify

Conversation

@BlakrPander

Copy link
Copy Markdown

Two independent bug fixes, one commit each. Happy to split this into two PRs if you would rather review them separately.

1. fix(sanitize): a token used as the remote URL's username was rendered verbatim

sanitizeRemote masked userinfo only when a password was present:

if (url.username !== '' && url.password !== '') {
  url.password = '***'
  return url.toString()          // ← also skipped the query redaction below
}

The most common non-interactive PAT form has no password at all — https://<token>@github.com/you/repo.git — so the token was returned untouched. That value is exactly what collectStatus puts in status.remote (lib/status.mjs:26), and renderStatus interpolates it with no further redaction (lib/render.mjs:21, reached from both /sync status and the sync_status tool). redactText is applied only to error messages on that path, never to remote, so there was no second line of defence.

Reproduced against the shipped 0.2.23 tarball:

https://ghp_AAAABBBBCCCCDDDDEEEEFFFF1234@github.com/o/r.git     -> unchanged (LEAK)
https://github_pat_11ABCDEFG...@github.com/o/r.git              -> unchanged (LEAK)
https://a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0@github.com/...  -> unchanged (LEAK)

This is worse here than in a typical plugin, because the leaked string lands in the session transcript — and this plugin mirrors the session store, so the token could then be committed into the synced repository the user is trying to keep private.

The fix keeps your "usernames are not secrets" intent (alice:***@ and scp syntax are unchanged, SECURITY.md's promise becomes true rather than newly narrowed):

  • A password-less username is masked only when it carries a token shape — a known prefix, >=16 hex digits, or a >=20-character separator-free multi-class string. git, oauth2, x-access-token, gitlab-ci-token and ordinary names stay visible.
  • TOKEN_PATTERN learns the families it had no coverage for: github_pat_ (the current default PAT form), the full gh[pousr]_ set, glpat-, gldt-, npm_, pypi-, AIza, ya29., SG., dop_v1_, shpat_, and three-segment JWTs.
  • The unparseable-remote branch returns redactText(trimmed) instead of echoing the input — which the module header already promised ("输入不合法时返回保守的脱敏文本").
  • The early return above also skipped the ?access_token= redaction; both redactions now always run.

2. fix(engine): a delete/modify conflict aborted the entire pull

When one side deletes a session file and the other modifies it, git registers only the surviving stages — a remote-side deletion leaves no stage 3, a local-side deletion leaves no stage 2 — and #resolveConflicts called checkoutStage unconditionally:

$ git merge --no-commit remote
CONFLICT (modify/delete): session.jsonl deleted in remote and modified in HEAD
$ git ls-files -u            # stage 1 + stage 2 only, no stage 3
$ git checkout --theirs -- session.jsonl
error: path 'session.jsonl' does not have their version

checkoutStage goes through #must, so the failure propagated to the catch, abortMerge() rolled the pull back, and sync stalled until the user resolved it by hand. Both directions were affected.

#resolveConflicts now reads each side's presence from the stage blobs first and treats a missing stage as "that side deleted the file": the deletion is kept, the surviving side is preserved (remote bytes still land in a fork file when there are any), and the divergence is still reported loudly through summary.diverged. Fork-ability is keyed on the remote side actually having bytes, so a deletion can no longer produce a bogus empty fork.

Same case in the encrypted backend, found while fixing the above

mergeTrees operated on undefined as if it were content:

  • THEIRS_ONLY with no remote bytes did merged.set(rel, undefined)
  • forkTheirs with no remote bytes inserted an undefined-valued fork key

writeTree writes every entry (lib/encrypted.mjs:73-77), so both became fs.writeFile(target, undefined) — a TypeError. An adopted deletion now leaves the merged tree (merged.delete(rel)) and an un-forkable deletion is counted as diverged with no fork entry.

Verification

New regression coverage: 5 sanitize cases, 1 mergeTrees case, 1 end-to-end two-device git case covering both delete/modify directions.

Reverting only lib/ and re-running the new tests — 6 of the 7 fail, so they genuinely pin these bugs (the 7th is the anti-over-redaction guard and correctly passes on both):

✖ mergeTrees handles remote deletions without ever storing undefined
✖ engine delete/modify conflicts stay keep-both instead of aborting the pull
✖ password-less token username is redacted (PAT-as-username)
✖ userinfo and query credentials are both redacted on the same URL
✖ modern token families are redacted in free text
✖ unparseable remote with an embedded token is redacted, not echoed
✔ benign password-less usernames stay visible

With the fixes in place, the full gate is green:

Gate Result
pnpm test 96/96 pass (baseline 89/89, no regressions)
pnpm run typecheck / typecheck:ci pass
pnpm run lint 3 warnings, unchanged from baseline
verify:self-contained / verify:artifacts / check:readmes pass

ARCHITECTURE.md records the deletion semantics in the merge table, and the five READMEs note that a deletion is a side too — the conflict bullet claimed the remote version is always preserved as fork files, which a deletion (having no bytes) cannot be.

One note on scope: I also confirmed the published npm tarball for 0.2.23 is byte-identical to main (all 18 shipped files), so these findings apply to the released artifact and not just to the checkout.

`sanitizeRemote` only masked userinfo when a password was present, so the
`https://<token>@host/…` form — the most common non-interactive PAT usage —
was rendered verbatim by `/sync status` and handed to the model by the
`sync_status` tool. Because this plugin mirrors session logs, a token seen
there could then be committed into the synced repository.

A password-less username now counts as a credential when it carries a token
shape: a known prefix, >=16 hex digits, or a >=20-character separator-free
multi-class string. Conventional logins (git, oauth2, x-access-token,
gitlab-ci-token) and ordinary names stay visible, so the documented
"username kept" behaviour for `user:pass@` and for scp syntax is unchanged.

Also here:

- `TOKEN_PATTERN` learns the families it had no coverage for: `github_pat_`
  (the current default PAT form), the full `gh[pousr]_` set, `glpat-`,
  `gldt-`, `npm_`, `pypi-`, `AIza`, `ya29.`, `SG.`, `dop_v1_`, `shpat_`,
  and three-segment JWTs.
- The unparseable-remote branch returns `redactText(trimmed)` instead of
  echoing the input, which is what this module's header already promised.
- The early return after masking `user:pass@` skipped the `?access_token=`
  redaction below it; both redactions now always run.

Verified: 6 of the 7 new sanitize tests fail against the previous code.
…he pull

When one side deletes a session file and the other modifies it, git registers
only the surviving stages — a remote-side deletion leaves no stage 3, a
local-side deletion leaves no stage 2 — so the unconditional
`checkoutStage('theirs'|'ours')` failed with `does not have their/our
version`, `abortMerge` rolled the whole pull back, and sync stalled until the
user resolved it by hand. Both directions were affected.

`#resolveConflicts` now reads each side's presence from the stage blobs first
and treats a missing stage as "that side deleted the file": the deletion is
kept, the surviving side is preserved (remote bytes still land in a fork file
when there are any), and the divergence is still reported loudly through
`summary.diverged`. Fork-ability is keyed on the remote side actually having
bytes, so a deletion can no longer produce a bogus empty fork.

The same case in the `encrypted` backend's in-memory `mergeTrees` inserted
`undefined` into the merged tree: `THEIRS_ONLY` with no remote bytes did
`merged.set(rel, undefined)`, and `forkTheirs` with no remote bytes inserted
an `undefined`-valued fork key. `writeTree` writes every entry, so both became
a `fs.writeFile(target, undefined)` TypeError. An adopted deletion now leaves
the merged tree and an un-forkable deletion is counted as `diverged` with no
fork entry.

ARCHITECTURE.md records the deletion semantics in the merge table, and the
five READMEs note that a deletion is a side too (the conflict bullet claimed
the remote version is always preserved as fork files, which a deletion — having
no bytes — cannot be).

Verified: both new end-to-end cases and the new `mergeTrees` case fail against
the previous code.
The mirror deleted every worktree file that was absent from `sessionRoot`, but a
worktree file has two possible origins: mirrored from the local store, or merged
in from the remote by a pull. `pull` never writes the live store (`sessionRoot`
is read-only to this plugin, by design), so a session that arrived from another
device was read as "deleted locally" and removed — along with a deletion commit
pushed back to the remote.

The result was that every pull was undone by the next mirror run, and a pull was
therefore useless on its own: two devices deleted each other's sessions in turn.

Reproduced with two devices over a real bare remote:

    A push          -> ok
    B pull          -> ok
      in B mirror?   true      <- pulled
      in B live?     false     <- never enters the live store, by design
    B pull again    -> ok
      in B mirror?   false     <- and it is gone again

`/sync status` alone was enough to trigger it, because status mirrors too.

The mirror now records the source's file list after every run in
`<repoDir>/.git/dsh-session-sync-mirror.json` — local state, never committed,
never synced — and deletes a file only when it is absent from the source **and**
present in that previous snapshot, i.e. one this device actually mirrored.
Anything else is reported as `remotePreserved` and kept.

When the snapshot cannot be read (first run, cleaned, corrupt, or `<repoDir>`
not yet a git worktree) nothing is deleted at all, so the failure direction is a
stale file rather than a lost one. Local deletion propagation is unchanged, and
still reaches a device that only ever held the remote copy (git resolves that as
a clean merge, so the non-owner adopts the deletion and does not resurrect it).

Verified: 4 of the new tests fail against the previous code, while the existing
"source deletions sync" test passes on both — the deletion semantics are not
weakened, only the misattribution is fixed.
@BlakrPander

Copy link
Copy Markdown
Author

Adding a third fix to this PR (f927981) — it is more serious than the other two, so I did not want to leave it as a separate follow-up.

fix(mirror): a pulled session was deleted by the next sync

The mirror deleted every worktree file absent from sessionRoot:

if (!targets.has(rel)) {          // targets = files present at the source
  await fs.unlink(absolute)
  deleted.push(rel)
}

But a worktree file has two origins: mirrored from the local store, or merged in from the remote by a pull. Since sessionRoot is read-only to this plugin (the mirror only ever calls listFiles/readFile on it), a session that arrived from another device is absent from the source by definition — so it was read as "deleted locally", removed, and the deletion was pushed back.

Two devices over a real bare remote, on the current release:

A push          -> ok
B pull          -> ok
  in B mirror?   true      <- pulled
  in B live?     false     <- never enters the live store, by design
B pull again    -> ok
  in B mirror?   false     <- gone again

/sync status triggers it on its own, because status() mirrors too (lib/engine.mjs:112), and because the deletion was then already staged in the worktree, the following push saw deleted: 0, skipped its commit guard, and left the removal uncommitted — an inconsistency that made the damage easy to miss.

Net effect on a two-device setup: every pull is undone by the next sync, so pull accomplishes nothing on its own, and the two devices end up deleting each other's sessions in turn. This is the headline "cross-device" feature, so I treated it as a correctness bug rather than a limitation.

The fix

The mirror now records the source's file list after every run in <repoDir>/.git/dsh-session-sync-mirror.json — local state, never committed, never synced — and deletes a file only when it is absent from the source and present in that previous snapshot, i.e. one this device actually mirrored. Everything else is reported as remotePreserved and kept.

  • Deletion propagation is unchanged, including to a device that only ever held the remote copy: git resolves that as a clean merge (base had it, ours unchanged, theirs deleted), so the non-owner adopts the deletion and does not resurrect it. Verified end-to-end.
  • The failure direction is safe. With no readable snapshot (first run, cleaned, corrupt, or <repoDir> not yet a git worktree) nothing is deleted at all — a stale file, never a lost one.
  • Fork files and host-private artifacts are untouched, as before.

Scope note

This deliberately does not make pull write into the live session store; the "this plugin never writes sessionRoot" boundary is worth keeping. It follows that a pulled session lives in the mirror until the user copies it into sessionRoot, and that the mirror keeps such a file indefinitely — both are "keep-both" behaviour, and both are documented in ARCHITECTURE.md.

Verification

Gate Result
pnpm test 100/100 (was 96; +4 new, 2 updated)
New tests vs. previous code 4 fail
Existing "source deletions sync" test passes on both — deletion semantics not weakened
typecheck / typecheck:ci / lint / verify:* / check:readmes pass

New e2e case: a pulled session survives every later sync, while real deletions still propagate (covers pull → status → push → remote round trip, then a genuine owner-side deletion propagating to the non-owner and staying gone).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant