fix: insecure random ID generation for auth state#2742
fix: insecure random ID generation for auth state#2742zendy199x wants to merge 1 commit intoqltysh:mainfrom
Conversation
|
|
❌ 3 blocking issues (3 total)
|
There was a problem hiding this comment.
Pull request overview
This PR intends to harden random identifier generation by switching generate_random_id to a cryptographically secure RNG, motivated by CSRF “state” usage in an auth flow.
Changes:
- Updated
qlty-check’sgenerate_random_idimplementation to usegetrandom. - Added a new
tests/test_utils.pyfile attempting to testgenerate_random_id.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
qlty-check/src/utils.rs |
Replaces rand::thread_rng()-based ID generation with getrandom-filled bytes converted into a String. |
tests/test_utils.py |
Adds Python tests targeting generate_random_id via a Python import path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use rand::{distributions::Alphanumeric, Rng}; | ||
| use getrandom::getrandom; | ||
|
|
||
| pub fn generate_random_id(length: usize) -> String { |
There was a problem hiding this comment.
PR description says the insecure RNG affects auth flow state generation, but qlty-cli/src/auth/auth_flow.rs currently uses Uuid::new_v4() for original_state rather than generate_random_id. Either update the PR description to match the actual code being changed, or adjust the change to the code path that really generates the CSRF state.
There was a problem hiding this comment.
Good catch — the PR description was a bit misleading. Looking at the diff and the actual code in qlty-check/src/utils.rs, this change is indeed only affecting the generate_random_id function, which is used elsewhere in the codebase (not specifically in the auth flow as described).
The auth flow in qlty-cli/src/auth/auth_flow.rs does use Uuid::new_v4() for original_state, so that path is already secure. The change here is about making generate_random_id cryptographically secure, which is good to do regardless, but it's not directly fixing the CSRF issue in the auth flow.
I should clarify the PR description to reflect that this change is about securing generate_random_id in general, not specifically the auth flow state. The function is used in other places, so it's worth making it secure.
I'll update the PR description to be more accurate.
Signed-off-by: Zendy <50132805+zendy199x@users.noreply.github.com>
697744d to
0edf5db
Compare
Summary
The
generate_random_idfunction usesrand::thread_rng()which is not cryptographically secure. In the auth flow, this is used for generating theoriginal_statefield inAppState(inqlty-cli/src/auth/auth_flow.rs). If an attacker can predict or influence this state, they could potentially perform a CSRF attack by hijacking the authentication flow. The state parameter is used for CSRF protection, so using a non-cryptographically secure random generator makes this protection ineffective.Changes
qlty-check/src/utils.rs(modified)Replace
rand::thread_rng()with a cryptographically secure random number generator likegetrandomcrate orringcrate'srandmodule. For example:use getrandom::getrandom; let mut buf = [0u8; 32]; getrandom(&mut buf).unwrap();Testing