False positive for jsonwebtoken.sign with a dummy password used as a secret key

[ebickle]

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 uses PasswordHeuristics::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 blocks PasswordHeuristics::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

[ginsbach]

Thank you for the suggestion. I have forwarded this to the relevant team and they are working on it!

[aibaars]

This should be fixed by https://github.com/github/codeql/pull/16417 once that pull request is merged.

[erik-krogh]

Hi 👋

I just merged https://github.com/github/codeql/pull/16417 which I think should solve your issue. If you have feedback and other comments just let me know, nothing is set in stone.

Thanks for the nice report.

[ebickle]

Thanks for all the fantastic work on this @erik-krogh, really appreciate it!

[intrigus-lgtm]

@erik-krogh is ignoring dummy passwords really the right way to fix this? I remember that @atorralba recently did pretty much the opposite change for Go: https://github.com/github/codeql/pull/15924

[ebickle]

@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 12345678 as a hardcoded password (or any value from the dummy list) in real production code, the CodeQL query won't trigger an alert.

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 🤨.

[atorralba]

I haven't tested this, but I believe that if someone uses 12345678 as a hardcoded password (or any value from the dummy list) in real production code, the CodeQL query won't trigger an alert.

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:

https://docs.github.com/en/code-security/code-scanning/managing-code-scanning-alerts/triaging-code-scanning-alerts-in-pull-requests#dismissing-an-alert-on-your-pull-request

(Note the specific "Used in tests" reason when dismissing an alert.)