Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions qlty-check/src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
use rand::{distributions::Alphanumeric, Rng};
use rand::rngs::OsRng;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unused import: rand::rngs::OsRng [clippy:unused_imports]

Suggested change
use rand::rngs::OsRng;


pub fn generate_random_id(length: usize) -> String {
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

rand::thread_rng()
.sample_iter(&Alphanumeric)
.take(length)
.map(char::from)
.collect()
// Use a URL/filename-safe alphabet to avoid control chars, path separators, etc.
const ALPHABET: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZ\nabcdefghijklmnopqrstuvwxyz\n0123456789-_";

let mut buf = vec![0u8; length];
getrandom(&mut buf)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cannot find function getrandom in this scope [clippy:E0425]


let mut id = String::with_capacity(length);
for byte in buf {
let idx = (byte as usize) % ALPHABET.len();
id.push(ALPHABET[idx] as char);
}

id
}
31 changes: 31 additions & 0 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from qlty_check.src.utils import generate_random_id

def test_generate_random_id_happy_path():
"""Test that generate_random_id produces a string of the correct length"""
result = generate_random_id(10)
assert isinstance(result, str)
assert len(result) == 10

def test_generate_random_id_edge_cases():
"""Test generate_random_id with edge case lengths"""
# Test with length 0
result = generate_random_id(0)
assert isinstance(result, str)
assert len(result) == 0

# Test with larger length
result = generate_random_id(100)
assert isinstance(result, str)
assert len(result) == 100

def test_generate_random_id_different_outputs():
"""Test that generate_random_id produces different outputs on subsequent calls"""
result1 = generate_random_id(10)
result2 = generate_random_id(10)
assert result1 != result2

# Ensure both are valid strings of same length
assert isinstance(result1, str)
assert isinstance(result2, str)
assert len(result1) == 10
assert len(result2) == 10