fix(account): log a classified reason instead of the caught error on login failure

[Pixnop]

Summary

The LOGIN handler's catch logged getErrorMessage(error) at debug level. That message comes from whatever threw, and this catch is the only place in the launcher where a password, a two-factor code and a pre-login token are all in scope at once. redactSensitiveText only rewrites password: value, password=value and absolute paths, so a bare secret sitting in prose went through untouched, into the log file players paste into bug reports.

Nothing on this path puts a credential in an error message today. The point is that the line could not tell: the message came from code accountHandlers.ts does not own, so the property worth holding is "the message never reaches the log", not "no current thrower misbehaves".

Which mechanism, and why

The issue offered two. I took the first: drop the raw message and log a classified reason.

The per-call exact-match scrub is the one that looks safer and is not. It only catches the secret verbatim. A message that URL-encodes the body (the request body is a URLSearchParams, so + for space and %40 for @ is the likely shape), JSON-escapes it, changes its case, or truncates it at a length cap all defeat an exact match, and each of those is exactly what an HTTP client echoing a request body does. It also has to be re-applied by hand at every future call site that touches a secret, which is the kind of rule that holds until someone adds a third log line.

Dropping the message closes the whole class instead of enumerating it, and it matches what this file already does elsewhere: settle logs verdict.serverReason (a server enum) and verdict.diagnosis (a field name and a type, never a value), both chosen for exactly this reason, and the renderer already collapses every refusal into one message. It is also the smaller change.

loginFailureReason in src/ipc/handlers/loginFailureReason.ts maps the caught error onto a fixed vocabulary. It sits in the IPC layer rather than src/domain on purpose: its entire content is knowledge of what src/ipc/network.ts and src/ipc/accountStore.ts throw, and src/domain is forbidden from importing either (the no-restricted-imports rule in .eslintrc.cjs). It lives beside accountLoginOutcome.ts and loginRequestBody.ts, which are pure modules pulled out of accountHandlers.ts for the same testability reason.

Every logged value is a literal from this module

The first version of the classifier accepted code and name on shape, matching them against an anchored identifier pattern and splicing the accepted value into the token. Zaldaryon was right that this was not safe. Both properties are public and writable, so an identifier-shaped value proves nothing about where it came from: a screaming-snake-case password is a valid password, and Object.assign(new Error("boom"), { code: "PASSWORD123" }) produced network-PASSWORD123 in the log.

No value the error carries is copied any more. code and name are read as lookup keys into tables, and what gets logged is the table's own string:

• NETWORK_CODES lists the socket and TLS failures the login round trip can actually surface: the DNS, routing and peer codes http(s).request reports on error, plus the certificate ones (CERT_HAS_EXPIRED, UNABLE_TO_VERIFY_LEAF_SIGNATURE and the rest), which belong on the list because the login pass goes to https://auth3.vintagestory.at through the Node transport and a rejected certificate arrives on that same error event. A code that is not in the table logs network-other.

• STORAGE_CODES covers what the account-store write reports when the filesystem refuses it, grouped into storage-no-space, storage-permission, storage-locked and storage-io, with storage-other for the rest.

• ERROR_NAMES keeps the handful of built-in classes worth telling apart (Error, TypeError, RangeError, SyntaxError, ReferenceError, AbortError). Everything else is plain unclassified, with nothing appended.

• HTTP_STATUSES names the statuses the auth service actually answers with (http-unauthorized, http-forbidden, http-rate-limited, http-unavailable and the rest), keyed by the status parsed out of the message as a number.

Zaldaryon was right about the status too, and it is the same mistake one layer down. I had kept the three digits because the message is one this repository throws itself, so I treated the shape of the message as proof of the safety of its contents. It is not: network.ts:169 writes those digits from the response's own status line, and assertString accepts 503 as a password, so a player whose password is 503 had it written to the log the moment the auth service went down. Nothing unusual had to happen, and no thrower had to misbehave.

The status is now parsed with Number and used as a lookup key, exactly like code. What reaches the log is HTTP_STATUSES' own literal, and a status outside the table degrades to its range: http-4xx when the service rejected us, http-5xx when the service is broken, http-other for anything else, including a redirect this transport does not follow and the literal unknown when the response had no status line at all. I kept the named table over collapsing everything to the two ranges because telling a 401 from a 429 is the first split I want in a field report, and those names are literals in the module. No formatted number reaches a token now.

The network and storage split

The network- prefix was also wrong for half of what reached it. The handler's catch wraps the round trip and the account-store write alike, so an ENOSPC from the keyring write was reported as network-ENOSPC, which sends whoever reads the field report after the wrong problem.

The split lives at the call site that still knows the origin. settle's existing catch around saveAccountSecrets now rethrows as AccountStorageFailure, a marker class exported by the classifier, and the classifier checks that first: a wrapped error goes through the storage tables, everything else through the network ones. By the time the outer catch sees the error, an ENOSPC from a keyring write and an ENOSPC from a socket are indistinguishable, which is why the wrapping happens where it does rather than in the classifier. AccountStoreUnreadableError is unaffected: it is still caught above and answered with its own wire status.

What a maintainer can still tell from the log

The line reads Login failure reason: <token>. and the token separates:

• timeout, response-too-large, response-aborted: the transport's own limits.
• http-unauthorized, http-forbidden, http-rate-limited, http-unavailable, http-server-error and the rest of HTTP_STATUSES: what the auth service answered, so an outage is not read as a wrong password. http-4xx, http-5xx and http-other for a status outside the table.
• network-ENOTFOUND, network-ECONNRESET, network-CERT_HAS_EXPIRED and the rest of the allowlist: the socket-level cause. network-other for a code outside it.
• secure-storage-unavailable, no-system-password-store: the credentials were fine and the keyring is what failed.
• storage-no-space, storage-permission, storage-locked, storage-io, storage-other: the session was good and writing it to disk is what failed.
• unclassified-TypeError, unclassified-Error and the four other allowlisted class names: nothing recognised the failure, and the class is what is left. unclassified for anything else.
• non-error-throw: something that is not an Error was thrown.

A rejected credential never reaches this catch at all: it resolves through settle's bad-credentials arm, which keeps logging the service's own reason string. An unreadable response resolves through unreadable-response and keeps its diagnosis. Those two were already safe and are untouched, so a timeout, a refused credential, an unreadable response and a full disk remain four different lines.

The other handlers in this file

REMOVE_ACCOUNT has no catch, logs nothing, and takes no secret. Nothing to do.

One other getErrorMessage call remains in this file, in settle's AccountStoreUnreadableError branch. It is not the same shape: the instanceof guard immediately above it means the error is always an AccountStoreUnreadableError, whose message is a compile-time constant ("The account store is unreadable and its bytes could not be preserved"). No caller-supplied text can reach it, so there is no leak to close. It is worth a follow-up for a different reason, which I have left alone here rather than widen the diff: it logs that constant and drops the wrapped cause, so it duplicates the error line directly above it and tells you nothing about whether the copy failed on EACCES or on a full disk.

Type

• Bug fix

Checklist

• Targets dev, not main.
• npm run typecheck passes.
• npm run lint:ci passes.
• npm run format:check passes.
• npm run test:coverage passes, coverage at or above the floor in vitest.config.ts.
• npm run build:unpack passes.

Testing

tests/ipc/loginFailureReason.test.ts pins the mapping directly: an identifier-shaped secret in code (PASSWORD123, CORRECT_HORSE_9) and in name (CorrectHorseBatteryStaple) must appear nowhere in the answer, a storage failure must never be named as a network one, down to the same code (ETIMEDOUT) landing on two different tokens depending on which side raised it, and no reason may contain the digits of the status that produced it, checked across 401, 403, 429, 500, 503, 418, 599, 302 and unknown.

tests/ipc/accountHandlers.test.ts holds the property end to end rather than the implementation: it stubs the transport and the account store to throw, runs the real handler through the electron mock, and reads what actually reached electron-log at every level, after logManager's own redaction. Four tests are new there. One puts the secret in both mutable fields. One raises ENOSPC and EACCES from the store write and asserts the log names storage and no line says network. One logs in with the password 503 against a transport rejecting with status 503, and asserts no line carries that password anywhere, message or token, while the line still reads http-unavailable. One runs 401, 429 and 503 through and asserts the three land on three different tokens with none of their digits in the log. LEAKY_PASSWORD deliberately does not contain the word "password", since a value that did would be caught by the keyword pattern and would not exercise the hole.

Gate output, measured on the rebased head:

• Focused accountHandlers.test.ts and loginFailureReason.test.ts: 54 passed.
• npm run typecheck: passed for node, web and test configurations.
• npm run lint:ci: 0 errors, 15 pre-existing React hook warnings (unchanged from dev).
• npm run format:check: all matched files use Prettier code style.
• npm run test:coverage: 162 files, 2056 passed, 2 skipped. 93.44% statements, 90.30% branches, 92.71% functions, 94.99% lines, against floors of 87/85/85/89. Both changed source files are at 100% on all four.
• npm run build:unpack: passed, Electron 44.1.1, linux x64.
• The renderer-dom failures from the earlier local run did not reproduce here either: the full suite is green on this head, rebased onto 1d5280a3.

Mutants

Each mutant was applied to the committed tree on its own, the focused suite run, then the file restored with git checkout -- and the tree confirmed clean with git status.

#	Mutation	Result
1	code accepted by the identifier regex again, spliced into network-${code}	Red. 2 tests: "never logs the value of code, even when the secret is shaped exactly like a Node code" and "logs no credential when the secret is sitting in the error's own code and name".
2	Fallback interpolates the name again, unclassified-${error.name}	Red. 2 tests: "never logs the value of name, even when the secret is shaped exactly like a class name" and "logs no credential when the secret is sitting in the error's own code and name".
3	settle rethrows the store error unwrapped, so it reaches the network classifier	Red. 1 test: "reports a failed session write as storage, not as a network failure".
4	Storage codes looked up in NETWORK_CODES instead of STORAGE_CODES	Red. 4 tests, including "names a full disk as storage, not as a network failure" and the handler-level storage test.
5	Debug line logs ${getErrorMessage(error)} again	Red. 5 tests: all three leak tests plus both diagnosis tests.
6	STATUS_MESSAGE unanchored (^/$ removed)	Red. 1 test: "matches the status message whole, so a longer one is not sliced for its middle".
7	Non-Error branch returns String(error)	Red. 1 test: "says so when what was thrown is not an Error at all".
8	The status spliced back into http-status-${status[1]}, the exact line Zaldaryon flagged	Red. 6 tests, the leak assertion firing on Login failure reason: http-status-503. in "logs no credential when the password is the same digits as the HTTP status".
9	HTTP_STATUSES lookup dropped, so every status falls to its range	Red. 4 tests, including "still tells the HTTP failures apart without printing any of their digits": the token stops separating a rejected credential from an outage.

Related issues

Closes #352

[Zaldaryon]

Changes requested

The raw error message is no longer logged, and the focused regression suite passes. Two correctness gaps still prevent this from being safe to merge.

The new classifier must not interpolate mutable Error.code or Error.name values merely because they match a regular expression. Both fields can carry a valid-format credential, and the same outer catch also handles account-store failures. The inline comments show the concrete cases.

Please use explicit allowlists or fixed categories for every value that reaches the log, add regressions with identifier-shaped secrets in both mutable fields, and keep storage failures distinct from network failures. Then update the branch onto the current dev tip and rerun the required checks.

Verification

• Focused IPC tests: 44 passed.
• Local npm run typecheck: passed.
• Local npm run lint:ci: 0 errors and 15 existing React hook warnings.
• Local npm run format:check: passed.
• GitHub Actions run 33926229635 passed typecheck, lint, test, and both required platform builds. SonarCloud also passed as an informational check.
• The local full coverage run had three renderer-dom failures outside the changed area. The remote test matrix passed.
• The current remote dev tip is a6e91358, four commits ahead of the PR a071d898 base. The branch needs updating before re-review.

[Pixnop]

@Zaldaryon Your three findings were all correct, and the third one taught me something about my own reasoning: I had convinced myself that an anchored, length-capped pattern proved provenance, and it proves nothing at all. Shape is not origin. The branch is rebased onto the current dev tip and pushed.

Writable code. Gone as a source of text. NETWORK_CODES is now an explicit table of the socket and TLS failures this round trip can actually surface, mapping each key to its own literal token, and the classifier reads code only as a lookup key. What reaches the log is the table's string, never the error's value. A code outside the table logs network-other. The TLS entries are on the list because the login pass goes to https://auth3.vintagestory.at through the Node transport, so a certificate this machine will not accept lands on the same error event as everything else. Object.assign(new Error("boom"), { code: "PASSWORD123" }) now logs network-other.

The network- prefix over storage failures. Split at the boundary. settle's existing catch around saveAccountSecrets rethrows as AccountStorageFailure, a marker class the classifier checks first, and a wrapped error goes through STORAGE_MESSAGES and STORAGE_CODES instead: storage-no-space, storage-permission, storage-locked, storage-io, storage-other, next to the existing secure-storage-unavailable and no-system-password-store. I put the wrapping at the call site rather than in the classifier because it is the only place that still knows the origin: by the time the outer catch sees it, an ENOSPC from a keyring write and an ENOSPC from a socket are the same object. AccountStoreUnreadableError still short-circuits above, untouched.

Writable name. Same treatment. ERROR_NAMES allowlists the six built-in classes worth telling apart and everything else is plain unclassified, nothing appended. I kept the allowlist rather than collapsing to one token because separating a TypeError in our own parsing from an AbortError is the first thing I look for in a field report, and those names are literals in the module.

Tests: loginFailureReason.test.ts now asserts that an identifier-shaped secret in code and one in name appear nowhere in the answer, that a wrapped storage failure never gets a network token (including the same ETIMEDOUT landing on two different tokens depending on which side raised it), and accountHandlers.test.ts runs ENOSPC and EACCES from the store write through the real handler and reads what actually reached electron-log. The existing leak tests are unchanged and green.

Seven mutants, each applied alone to the committed tree and restored, are in the description; the three that reintroduce exactly what you flagged go red on the tests named after the leak or the misdiagnosis.

Gates on the rebased head: typecheck passes, lint:ci 0 errors and the same 15 React hook warnings, format:check clean, test:coverage 162 files and 2046 passed with 2 skipped at 93.41 / 90.20 / 92.68 / 94.98 against floors of 87/85/85/89, and build:unpack passes. Your three renderer-dom failures did not reproduce for me: the full local run was green, so they look environmental rather than related to this change, and I have not touched anything there.

[Zaldaryon]

Changes requested

The classifier still copies mutable HTTP status digits into the log.

At loginFailureReason.ts:182-183, the message Network request failed with status 503 becomes http-status-503. The network implementation creates that message at network.ts:169, and validation.ts:92-97 accepts 503 as a password. A login using password 503 can therefore write the password itself to the log.

Please replace the status-derived token with a fixed HTTP failure category, or another explicit allowlist that cannot carry arbitrary digits. Add a real handler regression with password 503 and a transport rejection carrying status 503, and assert that no log line contains the password.

The head is also six commits behind current dev (1d5280a3). Update the branch, rerun the five required checks, and resolve the three open review threads before requesting approval.

[Pixnop]

@Zaldaryon The finding is correct, and it is the same mistake as the last three, one layer down. I had kept the status digits because the message is one we throw ourselves, so I treated the provenance of the message as proof of the safety of its contents. It is not. network.ts:169 writes those digits from the response's own status line, assertString accepts 503 as a password, and a player whose password is 503 had it in the log the moment the auth service went down. No thrower had to misbehave and nothing unusual had to happen. My own rule was that nothing an error carries reaches the log, and digits parsed out of a message are carried bytes like any other.

The status is now parsed with Number and used as a lookup key into HTTP_STATUSES, exactly the treatment code already gets. What reaches the log is that table's own literal: http-bad-request, http-unauthorized, http-forbidden, http-not-found, http-request-timeout, http-rate-limited, http-server-error, http-bad-gateway, http-unavailable, http-gateway-timeout. A status outside the table degrades to its range, http-4xx for a service that rejected us and http-5xx for one that is broken, and http-other covers everything else, including a redirect this transport does not follow and the literal unknown when the response had no status line at all. No formatted number reaches a token.

I kept the named table rather than collapsing to the two ranges because telling a 401 from a 429 is the first split I want when reading a field report, and both of those are literals in the module, so the table costs nothing in safety. If you would rather have only the ranges, say so and I will cut it.

Regressions, both through the real handler: "logs no credential when the password is the same digits as the HTTP status" logs in with password 503 against a transport rejecting with status 503 and asserts no line carries 503 anywhere, message or token, while the reason still reads http-unavailable. "still tells the HTTP failures apart without printing any of their digits" runs 401, 429 and 503 and asserts three different tokens with none of their digits present. At the unit level, one test sweeps 401, 403, 429, 500, 503, 418, 599, 302 and unknown and asserts the reason never contains the status it came from.

Two mutants for this round, each applied alone to the committed tree, run, then restored with git checkout -- and the tree confirmed clean. Restoring http-status-${status[1]}, the exact line you flagged, goes red on six tests, with the leak assertion firing on Login failure reason: http-status-503.. Dropping the HTTP_STATUSES lookup so every status falls to its range goes red on four, because the token stops separating a rejected credential from an outage. The full table of nine is in the description.

Branch rebased onto 1d5280a3 and force-pushed. Gates on that head: typecheck passes for node, web and tests; lint:ci 0 errors and the same 15 pre-existing React hook warnings; format:check clean; test:coverage 162 files with 2056 passed and 2 skipped, at 93.44 / 90.30 / 92.71 / 94.99 against floors of 87/85/85/89; build:unpack passes on Electron 44.1.1 linux x64. The renderer-dom failures you saw locally did not reproduce here.

The three inline threads from the previous round are resolved. Each carries a reply naming the fix and the test that pins it.

[Pixnop]

Rebased onto the current dev tip (5fa0462, which carries #375). No source change; the five required checks are re-running on the new head.

[Zaldaryon]

Changes requested

The current classifier fixes the three data handling defects from the previous review. It now uses fixed lookup tables for mutable Error.code, mutable Error.name, HTTP status text, and storage failures. The focused IPC suite passes 54 tests.

The head is no longer current with dev. PR #375 merged into dev at 5fa04624a76e4433bf16fe86e1c73d51d657fe4d, while this PR still targets the previous base 1d5280a3db7efc6af7bb2adb32d7214530c1d446. Its required checks therefore do not cover the current integration tip.

Rebase fix/login-log-redaction onto the current dev, rerun typecheck, lint, test, build (ubuntu-latest), and build (windows-latest), then request a fresh review. I am not approving or merging this head until that freshness gate passes.

Verification

• Reviewed head: 4f0044f78a2170dc85a6358deffbc363b8e606ed.
• Current dev: 5fa04624a76e4433bf16fe86e1c73d51d657fe4d.
• Focused IPC tests passed: 54 cases.
• The previous required checks passed on the previous base.

[Pixnop]

Rebased onto the current dev tip (7a90aa3, which now carries #376). No source change; the required checks are re-running on the new head.

[Zaldaryon]

The findings from the last rounds are all closed, and the rebase onto 7a90aa3 changed nothing: git range-diff shows all three commits identical.

Every value loginFailureReason can return is a literal spelled out in src/ipc/handlers/loginFailureReason.ts: the NETWORK_MESSAGES, STORAGE_MESSAGES, HTTP_STATUSES, NETWORK_CODES, STORAGE_CODES and ERROR_NAMES map values, the http-4xx / http-5xx / http-other range buckets, and the bare network-other, unclassified and non-error-throw fallbacks. Nothing off the error is interpolated on any path. codeOf returns "" for a non-string, so a non-string code cannot become a lookup key, and STATUS_MESSAGE is anchored so a longer message is not sliced for its middle. The HTTP status is parsed with Number and used only as a HTTP_STATUSES key, so the digits never reach a token, which is the fix for the 503-as-password case.

The storage and network split is real: AccountStorageFailure is thrown at the one call site in accountHandlers.ts that still knows the origin, and loginFailureReason checks for it first. The test at tests/ipc/loginFailureReason.test.ts pins that the same ETIMEDOUT maps to network-ETIMEDOUT bare and storage-other wrapped.

The tests hold the property rather than the implementation. They use identifier-shaped secrets on purpose (PASSWORD123, CORRECT_HORSE_9) and assert the value appears nowhere in the answer, and accountHandlers.test.ts reads what actually reached electron-log after the main-process redaction. npx vitest run tests/ipc/loginFailureReason.test.ts tests/ipc/accountHandlers.test.ts is 54 passing. Full local gate is green: typecheck, lint:ci (0 errors, 15 pre-existing hook warnings), format:check, test:coverage at 93.43% statements above the 87 floor, build:unpack on Electron 44.1.1.

Approving.