-
Notifications
You must be signed in to change notification settings - Fork 0
chore: extend /health with hash distribution & export_stream flag #45
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
Conversation
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving security and observability within the application. It introduces a transparent password rehash mechanism to upgrade older bcrypt hashes to Argon2id upon user login, enhancing overall security. Concurrently, the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends the /health endpoint with additional metrics and monitoring capabilities while adding password rehashing functionality for security improvements.
- Adds hash distribution metrics (bcrypt vs argon2 counts) to health endpoint for password security monitoring
- Introduces export_stream feature flag exposure in health status
- Implements transparent password rehashing from bcrypt to Argon2id during login with configurable toggle
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| jive-api/src/main.rs | Extends health endpoint with hash distribution metrics and export_stream feature flag |
| jive-api/src/handlers/auth.rs | Adds password rehashing logic from bcrypt to Argon2id with environment variable control |
| docs/EXPORT_STREAMING_BENCH.md | Documents benchmark procedures for export streaming feature |
| README.md | Updates documentation with password rehashing configuration instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "SELECT COUNT(*) FILTER (WHERE password_hash LIKE '$2%') AS b,\n COUNT(*) FILTER (WHERE password_hash LIKE '$argon2%') AS a FROM users" | ||
| ).fetch_one(&state.pool).await { |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The SQL query string has inconsistent formatting with a literal newline and extra spaces. Consider normalizing the whitespace or using a multiline string for better readability.
| "SELECT COUNT(*) FILTER (WHERE password_hash LIKE '$2%') AS b,\n COUNT(*) FILTER (WHERE password_hash LIKE '$argon2%') AS a FROM users" | |
| ).fetch_one(&state.pool).await { | |
| r#" | |
| SELECT | |
| COUNT(*) FILTER (WHERE password_hash LIKE '$2%') AS b, | |
| COUNT(*) FILTER (WHERE password_hash LIKE '$argon2%') AS a | |
| FROM users | |
| "# |
| "SELECT COUNT(*) FILTER (WHERE password_hash LIKE '$2%') AS b,\n COUNT(*) FILTER (WHERE password_hash LIKE '$argon2%') AS a FROM users" | ||
| ).fetch_one(&state.pool).await { | ||
| use sqlx::Row; (row.try_get("b").unwrap_or(0), row.try_get("a").unwrap_or(0)) | ||
| } else { (0,0) }; |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Missing space after comma in tuple literal. Should be (0, 0) for consistent formatting.
| } else { (0,0) }; | |
| } else { (0, 0) }; |
| let argon2 = Argon2::default(); | ||
| let salt = SaltString::generate(&mut OsRng); |
Copilot
AI
Sep 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating new Argon2 instance and generating salt on every bcrypt login creates unnecessary overhead. Consider reusing the Argon2 instance or moving this logic outside the rehash condition to avoid repeated instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces some valuable enhancements. The addition of password hash distribution to the /health endpoint is great for monitoring the migration from bcrypt to Argon2. The opportunistic password re-hashing on login is a solid security improvement, and making it configurable via an environment variable is a good touch. The new benchmark document for export streaming is also a great practice.
I have a couple of suggestions to improve code clarity and robustness, mainly related to parsing the environment variable and simplifying a database query block. Overall, this is a well-executed change.
| let enable_rehash = std::env::var("REHASH_ON_LOGIN") | ||
| .map(|v| matches!(v.as_str(), "1" | "true" | "TRUE")) | ||
| .unwrap_or(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for parsing the REHASH_ON_LOGIN environment variable is case-sensitive for the value "true" (it accepts "true" and "TRUE" but not "True"), which can be surprising. It's better to perform a case-insensitive comparison for boolean-like strings to make it more robust and user-friendly.
| let enable_rehash = std::env::var("REHASH_ON_LOGIN") | |
| .map(|v| matches!(v.as_str(), "1" | "true" | "TRUE")) | |
| .unwrap_or(true); | |
| let enable_rehash = std::env::var("REHASH_ON_LOGIN") | |
| .map(|v| v == "1" || v.eq_ignore_ascii_case("true")) | |
| .unwrap_or(true); |
| let (bcrypt_count, argon2_count) = if let Ok(row) = sqlx::query( | ||
| "SELECT COUNT(*) FILTER (WHERE password_hash LIKE '$2%') AS b,\n COUNT(*) FILTER (WHERE password_hash LIKE '$argon2%') AS a FROM users" | ||
| ).fetch_one(&state.pool).await { | ||
| use sqlx::Row; (row.try_get("b").unwrap_or(0), row.try_get("a").unwrap_or(0)) | ||
| } else { (0,0) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is functionally correct, but it's quite dense. The if let ... else combined with an inline use sqlx::Row; statement makes it harder to read. You can refactor this for better clarity using a more functional style. The use sqlx::Row; is also redundant as it's already imported at the top of the file.
let (bcrypt_count, argon2_count) = sqlx::query(
"SELECT COUNT(*) FILTER (WHERE password_hash LIKE '$2%') AS b,\n COUNT(*) FILTER (WHERE password_hash LIKE '$argon2%') AS a FROM users"
)
.fetch_one(&state.pool)
.await
.map(|row| (row.try_get("b").unwrap_or(0), row.try_get("a").unwrap_or(0)))
.unwrap_or((0, 0));
Adds bcrypt/argon2 hash distribution metrics and export_stream feature flag to /health response. Non-breaking, best-effort queries.