Skip to content

JS: add a js/empty-password-in-configuration-file query #7632

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

Merged
merged 4 commits into from
Jan 24, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jan 18, 2022

Evaluation looks clean.

Inspired by this C# query.
The implementation mostly reuses the logic from our existing PasswordInConfigurationFile.ql query.

@erik-krogh erik-krogh added WIP This is a work-in-progress, do not merge yet! Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jan 18, 2022
@erik-krogh erik-krogh removed WIP This is a work-in-progress, do not merge yet! Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jan 19, 2022
@erik-krogh erik-krogh marked this pull request as ready for review January 19, 2022 09:51
@erik-krogh erik-krogh requested a review from a team as a code owner January 19, 2022 09:51
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the qhelp has the right length. It would be too patronising to add examples for this kind of security issue.

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
esbena
esbena previously approved these changes Jan 21, 2022
@esbena esbena added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jan 21, 2022
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text generally looks okay but there seem to be some inconsistencies, which I've commented on.


</overview>
<recommendation>
<p>Choose a proper password and encrypt it if it has to stored in a configuration file.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth including the alternative possibility of storing the password in a secure store and referring to it in the configuration file?

Also - "proper" seems like a strange choice of word. Would "strong" be better here?


</overview>
<recommendation>
<p>Choose a proper password and encrypt it if it has to stored in a configuration file.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>Choose a proper password and encrypt it if it has to stored in a configuration file.</p>
<p>Choose a proper password and encrypt it if it has to be stored in a configuration file.</p>

erik-krogh and others added 2 commits January 24, 2022 13:37
Co-authored-by: Felicity Chapman <felicitymay@github.com>
Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates

@codeql-ci codeql-ci merged commit 8d1e22b into github:main Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants