fix(errors): close redacted credential values that end in a backslash - #222
Open
rohanpoudel2 wants to merge 2 commits into
Open
fix(errors): close redacted credential values that end in a backslash#222rohanpoudel2 wants to merge 2 commits into
rohanpoudel2 wants to merge 2 commits into
Conversation
`redactQuotedCredentialValues` decided whether a quote closed a credential
value by comparing the run of backslashes in front of it for equality with
the run in front of the opening quote:
if (delimiter - preceding === openingSlashes) {
A backslash at the end of the value is itself escaped, so it contributes a
whole extra escape level in front of the closing delimiter. The run is then
longer than the opening run, the equality fails, and the scan treats the real
delimiter as an escaped quote and keeps going. It closes at the next quote it
finds, which belongs to the following key, so the separator between them is
consumed:
{"password":"p@ss\\","errno":-2,"address":"10.0.0.5"}
{"password":"[redacted]"errno":-2,"address":"10.0.0.5"}
The result no longer parses as JSON. When no later quote matches either, the
unterminated branch takes over and everything after the credential is dropped,
so the host, port and errno an operator needs are lost from the diagnostic.
Close the value when the run exceeds the opening run by complete escape
levels rather than when the two are equal, and emit only the delimiter's own
escape backslashes, since the rest of the run is the tail of the value being
replaced. Redaction only ever became less accurate, never more permissive:
an odd run still marks an escaped quote and is still skipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #221
Problem
redactQuotedCredentialValuesdecided whether a quote closed a credential value by comparing backslash runs for equality:A backslash at the end of the value is itself escaped, so it contributes a whole extra escape level in front of the closing delimiter. The run is then longer than the opening run, the equality fails, and the real delimiter is misread as an escaped quote. The scan continues to the next quote — which belongs to the following key — and consumes the separator between them:
That output no longer parses as JSON, which is exactly the invariant
tests-ts/cli.test.tsalready pins for nesting depths 1–3. When no later quote matches at all, the unterminated branch takes over and everything after the credential is deleted:Change
Close the value when the run exceeds the opening run by complete escape levels rather than when the two are equal:
At each encoding depth the opening run is
2^d - 1, so one literal backslash costsopeningSlashes + 1characters and an escaped quote costs one further level. The period therefore generalises across depths: depth 0 closes on an even run, depth 1 on(n - 1) % 4 === 0, depth 3 on(n - 3) % 8 === 0.The replacement also now emits only the delimiter's own escape backslashes —
message.slice(delimiter - openingSlashes, delimiter + 1)— because with the corrected rule the rest of the run is the tail of the value being replaced and must not survive into the output.This never weakens redaction
An odd run still marks an escaped quote and is still skipped, so the scan cannot stop early inside a value. Closing earlier than before only ever happens at a position that genuinely ends the string under backslash-escaping rules. Redaction became more accurate, not more permissive.
The existing pinned behaviours are unchanged, including
password="correct horse battery staplestill redacting to end of input.Verification
A harness round-trips 13 secrets through real
JSON.stringifyat depths 1–3 and asserts three things per case: the output still parses, no fragment of the secret survives, and sibling fields are preserved.main: 18 failures, every one a secret ending in a backslash (p@ss\,\,\\,\\\,end\,mix\"\end).ALL OK: parses, no leak, siblings preserved.The regression test added to
tests-ts/cli.test.tsfollows the existing round-trip loop directly above it, adding a siblingaddressfield so it fails if the separator is eaten. Against the unfixed code it fails withSyntaxError: JSON Parse error: Expected '}'; with the fix it passes.Full suite: 717 pass / 5 skip / 0 fail (the new assertions extend an existing test, so the count is unchanged).
pnpm run typesandpnpm run formatare clean.