Comfy-Org / Comfy-Org/comfy-cli

PAT redaction from #342 is applied at one call site: `comfy distribution scan` re-leaks the token

Open
#748 0 comments 0 reactions 1 assignee Claimed by @sundar-svg View on GitHub
audit:security bug effort:small
Dominant language
Python
Stars
968
Forks
151
Avg merge
1d 9h
Merged PRs (30d)
77

Description

## Summary

Issue #342 ("Automatically erase github personal access token from url") was fixed in PR #376 by adding `_strip_url_credentials()` and calling it on the remote URL that `comfy node init` writes into `pyproject.toml`.

That guard is applied at **exactly one** call site. `comfy distribution scan`, added later in `comfy_cli/command/distribution.py`, reads `git remote get-url origin` raw and both **prints it to stdout** and **embeds it in the emitted distribution definition**. A custom node checked out over HTTPS-with-token therefore leaks the PAT again, in a new command, exactly the way `comfy node init` did before #342.

This is filed as a **closed-bug regression**, not as a new vulnerability class: the project already decided this string must be redacted, wrote the redactor, and shipped it. What is missing is the application of the existing rule to a new sink, plus a test that would have caught it.

## Where

Redactor, and its only call site:

- `comfy_cli/registry/config_parser.py:226` — `def _strip_url_credentials(url: str) -> str:`
- `comfy_cli/registry/config_parser.py:248` — the one and only call

A repo-wide control grep returns exactly those two hits:

```
$ git grep -n '_strip_url_credentials' origin/main -- comfy_cli/
comfy_cli/registry/config_parser.py:226:def _strip_url_credentials(url: str) -> str:
comfy_cli/registry/config_parser.py:248: git_remote_url = _strip_url_credentials(git_remote_url)
```

Unguarded sink, with the full path from read to disclosure:

| Step | Location |
|---|---|
| raw remote URL read, no redaction | `comfy_cli/command/distribution.py:196` |
| stored on the node record | `comfy_cli/command/distribution.py:201` |
| copied into the distribution definition | `comfy_cli/command/distribution.py:480` |
| **printed to stdout** in the scan table | `comfy_cli/command/distribution.py:607` |

Line 196 verbatim:

```python
repository = _git_output(entry, "remote", "get-url", "origin") if is_git else None
```

A third raw reader exists at `comfy_cli/command/outdated.py:382` (used for `ls-remote`); it does not appear to reach output, so it is noted rather than claimed.

## Reproduction

No install required — the leak is a pure string path.

```bash
# a custom node cloned with a token in the remote, which is what the PAT-in-URL
# workflow #342 was filed about actually produces
cd "$COMFY/custom_nodes"
git clone https://ghp_EXAMPLETOKEN0000000000000000000000@github.com/acme/my-node.git
cd "$COMFY" && comfy distribution scan
```

Observed: the `repository` column of the scan table prints
`https://ghp_EXAMPLETOKEN0000000000000000000000@github.com/acme/my-node.git`,
and the same string is written into the definition emitted for the builder.

Expected (the #342 contract): `https://github.com/acme/my-node.git`.

Control that the redactor is correct and simply not invoked here — `_strip_url_credentials` returns the stripped form for this exact input; `comfy node init` in the same working tree emits the redacted URL, `comfy distribution scan` does not.

## Why no test caught it

`tests/comfy_cli/registry/test_config_parser.py:914` parametrizes 12 credential shapes, but every case calls `_strip_url_credentials` **directly**. Nothing asserts that any particular *sink* is redacted, so a new sink cannot fail an existing test.

## Suggested fix

Route `distribution.py:196` (and `outdated.py:382`) through the existing helper, promoting it out of `registry/` to a shared location, then add a sink-level test rather than another unit test of the redactor:

- **Layer**: unit
- **File**: `tests/comfy_cli/command/test_distribution.py`
- **Asserts**: given a custom-node dir whose `git remote get-url origin` returns a token URL, the record produced by the scan and the row rendered at `:607` both contain `github.com/acme/my-node` and do **not** contain `ghp_`.

A stronger guard, since this is now the second occurrence of the same miss: assert repo-wide that no `get-url` result reaches output unredacted.

## Related, not duplicate: the redactor itself misses three shapes

Measured by executing the real `_strip_url_credentials` bytes from `origin/main` (extracted with `git show`, no reimplementation, stdlib only), with both a positive and a negative control:

```
CONTROL https+token -> https://github.com/acme/my-node.git token survives? False
CONTROL plain https -> https://github.com/acme/my-node.git token survives? False
VARIANT scp-style -> ghp_TOKEN@github.com:acme/my-node.git token survives? True
VARIANT query string-> https://github.com/acme/my-node.git?access_token=... token survives? True
VARIANT ssh://+token-> ssh://ghp_TOKEN@github.com/acme/my-node.git token survives? True
```

So even the guarded `comfy node init` path leaks for these three shapes:

- scp-style remotes have no URL scheme, so `config_parser.py:228` (`parsed.scheme in ("http", "https")`) skips them. This is the most realistic of the three for a git remote.
- `ssh://` URLs carry userinfo but fail the same scheme check.
- token-in-query-string is not considered at all.

None of the three is in the 12-case parametrize at `tests/comfy_cli/registry/test_config_parser.py:914`, which is why they are invisible today.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.