New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
False positive for jsonwebtoken.sign with a dummy password used as a secret key #16360
Comments
Thank you for the suggestion. I have forwarded this to the relevant team and they are working on it! |
This should be fixed by #16417 once that pull request is merged. |
Hi 👋 I just merged #16417 which I think should solve your issue. Thanks for the nice report. |
Thanks for all the fantastic work on this @erik-krogh, really appreciate it! |
@erik-krogh is ignoring dummy passwords really the right way to fix this? |
@intrigus-lgtm This is an area where I've seen our developers struggle the most with CodeQL - we have a large number of repos using the CodeQL Default Setup for JavaScript/TypeScript and - if I remember right - by default it includes unit tests. At least one made around 50+ commits trying to figure out the right magic pattern to put a mock value in a unit test so they can get CodeQL to show 'green' on their JWT unit tests. Arguably they could have tried a different approach, but I'm happy any day a developer tries to write a unit test 😁 I'm not a fan of dummy passwords either. I haven't tested this, but I believe that if someone uses Is there any way that the query can be made more robust by somehow excluding unit test code that "stays within the unit tests" without requiring a file-based exclude via the CodeQL configuration? Everyone's seen some integration with "real" passwords making outbound calls to servers or allowing inbound calls when perf tests are running 🤨. |
This is precisely the problem of ignoring hardcoded dummy passwords: the fact that they're "dummy" doesn't mean they aren't a problem if used in real systems (real world example). Alerts appearing in test code is a different issue, and IMHO we shouldn't try to solve it in individual queries, but rather at the presentation layer. See how the Code Scanning documentation recommends handling it, for exmaple: (Note the specific "Used in tests" reason when dismissing an alert.) |
Description of the false positive
The rule 'js/hardcoded-credentials' does not exclude dummy/stub passwords passed as JWT signing secrets.
The documentation for jsonwebtoken.sign states that the second parameter (
secretOrPrivateKey
) can be either a passphrase or a cryptographic key. Although the query usesPasswordHeuristics::isDummyPassword
to exclude common dummy/stub values typically found in unit tests, it only does this for credential sinks of the types 'password', 'credentials', or 'token'.Because the
secretOrPrivateKey
parameter has two possible types, the implementation in CodeQL likely had to pick one and chose something related to a private key - the effect of which blocksPasswordHeuristics::isDummyPassword
from running and causes false positives for unit test code using hardcoded secret passphrases.Code samples or links to source code
Code reproducing the issue is attached. The hardcoded signing secret
'example'
is a value listed in PasswordHeuristics::isDummyPassword.sample.zip
The text was updated successfully, but these errors were encountered: