[Tyrie Vella (tyrielv)]

Problem and Context

git credential fill writes its result as key=value lines:

protocol=https
host=example.com
username=someone
password=<plaintext secret>

Four problems let credential data reach logs and telemetry, or let the wrong value be used.

• On a parse failure, GitProcess.TryGetCredential and GitProcess.TryGetCertificatePassword added the raw stdout to the activity metadata. A helper can return a secret and still fail the parse, for example when the last line has no terminating newline. The secret then reached the log.
• On a nonzero exit, the failure warning carried the full stderr with the telemetry keyword. Git echoes an unparseable helper line to stderr as warning: invalid credential line: <raw line>, so a secret fragment could leave the machine.
• The same stderr text was returned as the error message. The caller puts it in an exception, and the retry handler reports the final failure of a request with the telemetry keyword.
• ParseValue did not check for an absent key. It added the prefix length to a failed IndexOf (-1), so output with no username= line gave a username of =https. The caller treated that as success. It also matched a key anywhere in the output, so a longer key such as oauth_password= or a value that contains password= could supply the value.

Changes

• On parse failure, both methods add OutputKeys instead of Output. GetCredentialOutputKeys maps each output line to a key name from the git credential protocol, <other> for any other key, or <malformed> for a line with no key. A value, or free text before the first =, cannot reach the metadata. The summary lists at most 32 lines and reports <truncated> when the captured output was truncated.
• When the password is missing or empty, a warning with the telemetry keyword reports OutputKeys, the exit code, and the length and truncation flag of stderr. The failure with a nonzero exit reports the same exit code and stderr length. No stderr text goes to telemetry. The hasVisibleConsoleWindow diagnostic stays in these events.
• The text of stderr goes to the local log only. The error message that is returned to callers names the exit code and no longer carries stderr. A credential manager timeout also writes its captured stderr to the local log.
• ParseValue returns null when the key is absent. It reads output like git: a line ends at \n, the last line need not end with one, a repeated key takes its last value, and a key matches only at the start of a line. GetCredentialOutputKeys uses the same definition of a line.
• TryGetCredential treats an absent username as empty and requires a non-empty password, because a helper that returns only a token does not send a username. GitAuthentication accepts an empty username and builds the basic-auth string as :<password>. A credential without a password fails with the reason "Git credential fill returned no password".
• GitAuthentication splits a stored credential at the first :, so a password that contains : can be approved. It encodes and decodes the credential as UTF-8. ASCII replaced every other character with ?.
• Tests use a sentinel secret and check every traced event. They cover both methods and the boundary of GitAuthentication: a failing exit with secret-bearing stderr and the returned error message, a timeout, CRLF and bare-CR separators, empty output, unknown, whitespace and uppercase keys, the key limit, truncated output, the success path, line-start matching, repeated keys, a missing or empty password, a missing username, non-ASCII credentials, and the approval of a credential with an empty username or a : in the password.

Known gaps and deliberate decisions

• The log keeps stderr. The text of a failed credential fill is in the local log file. The error message that a user sees no longer includes it, so a user must read the log to see what the credential helper wrote.
• Unknown keys appear as <other>. A key that is not in the git credential protocol list does not appear by name. This keeps free text out of the output at the cost of some diagnostic detail.
• The telemetry field is renamed. The failure-path field Output is now OutputKeys. No consumer of the old field exists in this repository.
• An empty username is now accepted. A helper that sends an explicit empty username= line, or none, now authenticates with :<password>. Before, an explicit empty username failed with "Got back empty credentials", and an absent username produced a wrong value that was used for authentication. Azure DevOps and a GVFS cache server both returned success (HTTP 200) for a valid token with an empty username and with a non-empty one. Neither treated an invalid token as success. The checks used a Microsoft Entra token, not a PAT.
• Empty lines are skipped. Git stops reading credential output at the first empty line. ParseValue skips empty lines and keeps reading, so it accepts every output that git accepts.

[Nathan Baird (ShiningMassXAcc)]

Reviewed the diff, built it, and ran the full unit suite. The core security property holds: I verified it two independent ways — hand-tracing every path, and separately by executing the exact method body against adversarial inputs and running real git credential fill (2.55.0.vfs.0.10) against a deliberately hostile credential helper. I found no secret-leakage path, and I'm stating that explicitly rather than manufacturing one.

The returned string's alphabet is closed: every element is either a compile-time literal (<malformed>, <other>, ,) or a key that passed CredentialOutputKeyAllowlist.Contains under StringComparer.Ordinal — byte-exact, so no culture or case folding can admit a crafted key. Things I specifically tried that all degrade safely: leading whitespace before the key, trailing whitespace before =, = at index 0, no = at all, multiple = (first wins, value never emitted), a 200-char key, and homoglyph keys (Kelvin sign, accented characters) — <other> or <malformed> in every case. Line splitting is correct too: RemoveEmptyEntries drops the zero-length run between \r and \n, so CRLF and bare-CR both parse cleanly.

Ordinal rather than OrdinalIgnoreCase is the right call — git's credential keys are lowercase by spec, and an uppercase variant degrading to <other> fails in the conservative direction.

The tests are a good shape: AssertNoSecret sweeping all recorded metadata rather than just the one field, and putting the sentinel in the key position in the first case to prove free text before = is suppressed. The MockTracer change is purely additive and I confirmed nothing existing regressed.

Validation on my end: clean build, 0 warnings, and 985/990 unit tests passing versus 977/982 on master — so +8 tests, all green, zero regressions (the 5 PostIndexChangedHookTests failures are environmental and identical on both branches).

Four things below. None of them is a regression this PR introduces, so I don't consider any of it merge-blocking — but the first one is worth a decision before this goes in.

---

1. GVFS/GVFS.Common/Git/GitProcess.cs:300 (and :367) — the redaction landed on the log-only sink, not the telemetry one

This is the one I'd most like your read on, because I think the asymmetry inverts the intent slightly.

The path this PR sanitizes is activity.Stop(metadata), on an activity started by StartActivity("TryGetCredential", EventLevel.Informational). That overload resolves to Keywords.None (JsonTracer.cs:255), so OutputKeys was only ever being written to the local log file. Meanwhile the failure path a few lines up is the one that leaves the machine:

tracer.RelatedWarning(
    errorData,
    "Git could not get credentials: " + gitCredentialOutput.Errors,
    Keywords.Network | Keywords.Telemetry);

TelemetryDaemonEventListener gates on that keyword, so this stderr text ships off-box while the now-sanitized OutputKeys does not.

That matters because git itself will put secret-bearing text into that stderr. credential_read reflects any line it cannot parse back verbatim as warning: invalid credential line: <raw line>. So a helper that emits a password containing an embedded newline produces a secret fragment on stderr and exits 128 — which is ExitCodeIsFailure, i.e. exactly this branch. Reproduced locally against git version 2.55.0.vfs.0.10 with a helper emitting a bare line:

EXITCODE=128
--- STDERR ---
fatal: Cannot prompt because user interactivity has been disabled.
warning: invalid credential line: <raw helper line appears here verbatim>
fatal: unable to get password from user

The same text is also assigned to errorMessage (:302 / :364), which flows on to GitAuthentication.cs:295 and into the GitObjectsHttpException message at HttpRequestor.cs:117.

Small related note: the new doc comment at :1101 says values "never reach telemetry or the log" — true of this method, but the sibling failure path undercuts the sentence as written.

I'm not asking you to solve the whole stderr problem here; the narrow scope is the right call. But two things would be good before this merges:

1. Dropping Keywords.Telemetry from these two call sites is a very small change and closes the off-box half of the stated goal.
2. Either way — could we get a tracking issue? I searched and there's no existing issue covering credential stderr redaction, and the description says "no follow-up is planned yet", so the remaining half would otherwise have nothing holding it.

For completeness: :231 (reject) and :259 (approve) also trace result.Errors, but through the format-string RelatedWarning overload, which routes to Keywords.None — local log only, so materially lower risk than these two.

2. GVFS/GVFS.Common/Git/GitProcess.cs:1106-1129 — GetCredentialOutputKeys is an unbounded, amplifying transform

credential fill runs with parseStdOutLine: null, so stdout is buffered whole through BoundedGitOutputBuffer(MaxCapturedStdOutChars) — and MaxCapturedStdOutChars is 134,217,728 (:38). The helper doesn't consult OutputTruncated.

Each non-whitespace line becomes up to 12 output chars (<malformed> plus separator), so a 2-char line amplifies 6x. I measured it by running the method body over a 400,000-char input of repeated single-char lines: 2,399,999 chars out, ratio 5.99997. Projected at the cap that's roughly 768M chars (~1.5 GiB as UTF-16) for the joined string, plus an intermediate Split array of ~67M string references.

Worth flagging that this is a net worsening versus the previous behaviour: metadata.Add("Output", ...) stored an existing reference and allocated nothing new, whereas this allocates a fresh object graph several times the size of the source. It also runs against the convention this file documents at :18-37 and implements in BoundedGitOutputBuffer (:1288+), whose stated rationale is preventing "a noisy git command from growing an unbounded StringBuilder until GVFS.Mount hits an OutOfMemoryException". And it's on the parse-failure path, which is precisely where unusually noisy output is most likely in the first place.

Suggestion: cap the lines summarized (.Take(N) with a +K more marker) and/or short-circuit when OutputTruncated is set, mirroring the truncation-marker pattern already in BoundedGitOutputBuffer. A handful of key names is all the diagnostic value this field really carries.

3. GVFS/GVFS.UnitTests/Common/Git/GitProcessCredentialTests.cs — the suite can't observe the path in finding 1

Two gaps worth closing:

• AssertNoSecret scans only StartActivityTracer.StoppedActivityMetadata. It never scans RelatedWarningEvents / RelatedErrorEvents / RelatedInfoEvents — which is exactly where the stderr warning from finding 1 lands. Every case also passes string.Empty for stderr and Result.SuccessCode, so the ExitCodeIsFailure branch is never exercised at all. The suite proves the fixed path is clean but asserts nothing about the unfixed one. A case with a failing exit code and secret-bearing stderr, plus extending AssertNoSecret to the related-event collections, would cover it.
• The "success path omits OutputKeys" assertion exists for TryGetCredential but not for TryGetCertificatePassword. Both call sites got the same treatment, so it'd be good to pin both.

Also currently unprotected by regression tests, though I confirmed the method handles them correctly: leading/trailing whitespace around a key, and uppercase key variants. Those are the load-bearing guards, so they're worth locking in.

4. GVFS/GVFS.Common/Git/GitProcess.cs:1113 — IEnumerable<string> keys

CONTRIBUTING.md (lines 190-192) asks for explicit types over interfaces: "Prefer concrete/explicit types to interfaces (e.g. prefer List to IList)." Materializing to List<string> would also give a natural place to apply the bound from finding 2.

[Tyrie Vella (tyrielv)]

Note

Machine-drafted reply, reviewed and approved by Tyrie Vella (@tyrielv) before posting.

Thank you for the careful review. All four points were right. I addressed them in 541a5d75.

1. Stderr and telemetry. I confirmed that the failed credential fill warning carried stderr with Keywords.Telemetry, and that OutputKeys only reached the local log. I did not drop the keyword, because the same event carries hasVisibleConsoleWindow, which we want in telemetry. The telemetry event now has no stderr. A second warning with the stderr text goes to the local log only. Tests cover both methods with a failing exit and secret-bearing stderr. Stderr is still in the local log and in errorMessage, and the description now says so. A tracking issue is no longer needed for the telemetry half.
2. Unbounded summary. GetCredentialOutputKeys scans line by line without Split, lists at most 32 lines, and returns <truncated> when OutputTruncated is set.
3. Tests. AssertNoSecret now covers every traced event. I added the failing-exit stderr case, the certificate success path, whitespace and uppercase keys, and the ParseValue cases.
4. IEnumerable<string>. It is gone with the rewrite in point 2.

I also made ParseValue match only at the start of a line, expanded the key allowlist, and fixed approval of a password that contains :.

[Nathan Baird (ShiningMassXAcc)]

Review pass over this PR. 3 comment(s).

These findings concern code the diff does not touch directly, so GitHub cannot anchor them inline:

GVFS/GVFS.Common/Git/GitProcess.cs (around line 1276) - LOW

ParseValue returns the first line-start match. Git's credential_read overwrites the field on each repeated key, so the last value wins. A helper emitting password= twice gets different answers from git and from GVFS.

Probably not worth changing behavior over, but worth either matching git or stating the first-wins choice in the comment and locking it with a test, since this is now the authoritative parser for this output.

[Tyrie Vella (tyrielv)]

Note

Machine-drafted reply, reviewed and approved by Tyrie Vella (@tyrielv) before posting.

Thank you, all three were right. I fixed them in 40397235, and the branch is rebased onto current master.

1. First vs last. ParseValue now returns the last match, as git does. I ran git credential fill against a helper that emits password twice. Git writes the last one once and ends every line, so GVFS does not see duplicates in practice, but the parser no longer disagrees with git. There is a test.
2. Telemetry. You were right that OutputKeys rode on the activity stop event, which carries no telemetry keyword. When the password is missing, we now emit a warning with Keywords.Telemetry that carries OutputKeys, the exit code, and the stderr length and truncation flag. The failure warning also gets the exit code and stderr length. No stderr text goes to telemetry.
3. One definition of a line. GetCredentialOutputKeys and ParseValue now both end a line at \n, accept an unterminated final line, and do not treat a bare \r as a separator, which matches git. My tests had forced failures by dropping the final newline, which git never produces. They now use an absent password instead.

[Nathan Baird (ShiningMassXAcc)]

Re-review of 40397235. Both of my earlier threads are genuinely fixed here — the fix is explained in a top-level comment, which means the threads still read as unanswered in the Files view. Replying inline on each thread (even just "fixed in 4039723") would make that visible to anyone scanning the PR.

One High finding below: I believe the raw-stderr-to-telemetry property the PR sets out to close is still open on one path. The rest are smaller.

Credential-leak audit of the new code — clean except one: TraceCredentialFillFailure no leak · TraceCredentialFillMissingPassword no leak (closed OutputKeys alphabet) · timeout branch no leak · activity.Stop(metadata) no leak (the old raw-stdout dump is gone) · GitProcess.cs ~403 leaks via the errorMessage out-param (detail inline).

CI is green across every leg.

[Tyrie Vella (tyrielv)]

Note

Machine-drafted reply, reviewed and approved by Tyrie Vella (@tyrielv) before posting.

Thank you for the re-review. I addressed all eight comments in 0a3f274b, and the branch is rebased onto current master.

• High, returned errorMessage. You were right. I followed the chain: the message becomes the exception message, and StandardErrorHandler reports a final failure through RelatedError, which always adds Keywords.Telemetry. The message now names the exit code and says to see the log. The stderr text stays in the local log. I changed the certificate path the same way and added a test at the GitAuthentication boundary, so AssertStderrIsOnlyLocal is no longer the only guard.
• Empty password. Fixed with IsNullOrEmpty, so both layers agree and the empty case gets the same diagnostic. Tests cover both methods.
• ASCII encoding. Now UTF-8 on both sides, with a non-ASCII round-trip test through approve.
• Dropping the username guard. This is deliberate, and the PR keeps it. The old guard did not mean "no username is ever sent": an absent username= produced a wrong value (=https) that was still sent, and an explicit empty one failed. The servers for this protocol are Azure DevOps and its GVFS cache servers, and I tested both: a valid token with an empty username and with a non-empty one both returned 200, and an invalid token was not treated as success. Those checks used an Entra token, not a PAT. The description records this.
• Timeout stderr. A timeout now also writes its captured stderr to the local log.
• ParseValue cost and empty lines. Both are documented in a remark. I checked that git stops at the first empty line, and ParseValue accepts a superset of what git accepts.
• Using order. The order already matches GitSslTests and Sha1IdTests. The one difference at line 1 was the UTF-8 BOM they have, which I added. If you meant something else, please tell me.