fix: drop the masked key rendering, clearing CodeQL alert #3 - #21
Merged
Conversation
Alert #1 closed on the merge of #18. Alert #3 opened against the replacement, at src/components/web3forms.tsx:182, same rule, js/clear-text-logging. This is not a repeat of the same mistake. The previous version printed the key in full. This one printed four leading and four trailing characters. But "fewer characters" is not the same as "no characters", and the alert is pointing at something real. Which field CodeQL could actually taint --------------------------------------- describeKey returned four fields. Only one of them carried the value's content: length a number, from .length not tainted trimmedWhitespace a comparison result not tainted note one of a fixed set of literals, not tainted (implicit selected by branching flow only) masked key.slice(0,4) + key.slice(-4) TAINTED, via slice So the one field the analyser objects to is the one field genuinely carrying key material. That alignment is the useful signal: the tool was right, and the comment claiming the value was "described, never reproduced" was not quite true, because eight characters of it were being reproduced. Removing it costs nothing ------------------------- Every distinction the branch exists to draw comes from the other three fields: empty length 0, trimmedWhitespace false whitespace-padded length 0, trimmedWhitespace TRUE placeholder text not hex-and-dashes truncated hex-and-dashes but not 36 characters Verified across all five cases: each remains distinguishable, and no four character run of a real key appears anywhere in the output. The masked value was only ever an aid to identifying WHICH key arrived, and it could not do that job here anyway: a valid-but-wrong key passes UUID.test and never reaches this branch. Not dismissed ------------- The alert is resolved by removing the flow, not by marking it a false positive. Calling it one would have required arguing that eight characters of a credential are not credential material, which is the kind of claim this repository has already had to walk back twice. Two stale comments describing the removed masking are corrected in the same change, found by grepping rather than by memory. Verified -------- lint, tsc --noEmit and build all clean.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Closes CodeQL alert #3,
js/clear-text-logging,src/components/web3forms.tsx:182.Alert #1 closed on the merge of #18. #3 opened against the replacement, same rule.
This is not the same mistake twice, but the alert is right
The version before #18 printed the key in full. #18 printed four leading and four trailing characters. Fewer characters is not no characters, and CodeQL is pointing at something real.
Which field could actually be tainted
describeKeyreturned four fields. Only one carried the value's content:length.length→ numbertrimmedWhitespacenotemaskedkey.slice(0, 4)+key.slice(-4)String.prototype.sliceThe one field the analyser objects to is the one field genuinely carrying key material. That alignment is the useful signal here: the tool was right, and #18's comment — "The value is described, never reproduced" — was not quite true, because eight characters of it were being reproduced.
Removing it costs nothing
Every distinction the branch exists to draw survives without it:
Empty and whitespace-only are separated by
trimmedWhitespace, which is the only signal that can do it once length is 0. Placeholder and truncated bynote. Verified across all five, with an assertion that no four-character run of a real key appears anywhere in the output.The masked value was only ever an aid to identifying which key arrived — and it could not do that job here anyway. A valid-but-wrong key passes
UUID.testand never reaches this branch.Not dismissed
The alert is resolved by removing the flow, not by marking it a false positive.
Calling it one would have required arguing that eight characters of a credential are not credential material. That is precisely the shape of claim this repository has already had to walk back twice — the rotate claim, and the "not a UUID therefore not a credential" comment that #18 itself corrected. Dismissing would have been a third.
Also
Two stale comments describing the removed masking are corrected in the same change, found by grepping rather than by memory. Leaving them would have reproduced the exact failure mode that let the rotate claim survive its first correction cycle.
Verified
npm run lint,npx tsc --noEmitandnpm run buildall clean. Nosliceorrepeaton the key value remains anywhere in the file.One honest caveat
I cannot run CodeQL here, so I cannot prove in advance that the alert closes. The reasoning is that
slicewas the only taint-propagating step, and removing it should cut the dataflow. The confirmation is the alert closing after merge — if it does not, the remaining flow is through.lengthor the branch-selectednote, and that would be worth knowing.