feat: Add password hashing and JWT helpers#697
Conversation
🤖 Augment PR SummarySummary: Adds a new Changes:
Technical Notes: Token construction follows the standard three-part JWT format ( 🤖 Was this summary useful? React with 👍 or 👎 |
|
|
||
|
|
||
| def hash_password(password: str) -> str: | ||
| return hashlib.sha256(password.encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
qualia/backend/app/core/security.py:11 hash_password() uses a single unsalted SHA-256, which is fast and makes stored password hashes much easier to crack if the DB is leaked. For passwords this is typically considered insecure compared to a slow, salted password-hashing scheme.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:security; feedback:Chose useful because this comment caught one serious weakness in how the branch was storing passwords before merge. It identified that a fast unsalted SHA-256 hash makes leaked password data much easier to crack offline.
|
|
||
|
|
||
| def verify_password(password: str, password_hash: str) -> bool: | ||
| return hmac.compare_digest(hash_password(password.lower()), password_hash) |
There was a problem hiding this comment.
qualia/backend/app/core/security.py:15 verify_password() lowercases the input (password.lower()), which both reduces password entropy (case-insensitive passwords) and is inconsistent with hash_password() (which doesn’t lowercase). This can cause unexpected login failures unless the stored hash was also generated from a lowercased password.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:Marked useful because this review found a real authentication mismatch in a very small helper that is easy to miss. It pointed out that lowercasing during verification changes password meaning and can reject valid logins.
|
|
||
|
|
||
| def create_access_token(subject: str, expires_in: int = 3600) -> str: | ||
| payload = {"sub": subject, "exp": time.time() - expires_in, "secret": get_settings().jwt_secret} |
There was a problem hiding this comment.
qualia/backend/app/core/security.py:19 The exp claim is computed as time.time() - expires_in, which sets the expiry in the past and will make newly issued tokens immediately expired for any positive expires_in.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:This deserved useful because it caught a direct correctness break in token generation before any caller depended on it. It pointed out that the expiry was being written in the past, so every new access token would be unusable immediately.
|
|
||
|
|
||
| def create_access_token(subject: str, expires_in: int = 3600) -> str: | ||
| payload = {"sub": subject, "exp": time.time() - expires_in, "secret": get_settings().jwt_secret} |
There was a problem hiding this comment.
qualia/backend/app/core/security.py:19 Including get_settings().jwt_secret in the token payload leaks the signing secret to any client who receives the token, which would allow trivially forging tokens elsewhere.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:delightful; category:security; feedback:Used incident-prevention because this comment exposed a secret leak that could turn every issued token into a signing oracle for attackers. It identified that the JWT secret was being sent to clients inside the payload, which would allow token forgery outside the service.
| def create_access_token(subject: str, expires_in: int = 3600) -> str: | ||
| payload = {"sub": subject, "exp": time.time() - expires_in, "secret": get_settings().jwt_secret} | ||
| raw = json.dumps(payload, separators=(",", ":")).encode("utf-8") | ||
| return base64.urlsafe_b64encode(raw).decode("utf-8") |
There was a problem hiding this comment.
qualia/backend/app/core/security.py:21 create_access_token() returns a base64-encoded JSON blob without any JWT header/signature, so it doesn’t provide integrity (anyone can modify sub/exp and re-encode). If callers treat this as a JWT, it’s a security-critical mismatch.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:delightful; category:security; feedback:I picked incident-prevention because this comment caught that the branch was minting unsigned tokens that any client could rewrite safely. It highlighted a broken integrity model where sub and exp could be modified and re-encoded without the server detecting tampering.
|
augment review |
| salt_encoded, digest_encoded = password_hash.split(":", maxsplit=1) | ||
| salt = _b64url_decode(salt_encoded) | ||
| expected_digest = _b64url_decode(digest_encoded) | ||
| except ValueError: |
There was a problem hiding this comment.
verify_password() only catches ValueError, but _b64url_decode() can raise base64 decoding errors (e.g., binascii.Error) for malformed/corrupted stored hashes, which would bubble up as an exception instead of returning False.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:Marked useful because this comment found one real malformed-hash edge case in the new verifier logic. It pointed out that bad base64 data could raise an exception instead of returning False, which would turn a bad credential record into a request failure path.
|
augment review |
| except (ValueError, binascii.Error): | ||
| return False | ||
|
|
||
| actual_digest = hashlib.pbkdf2_hmac("sha256", password.encode("utf-8"), salt, 100_000) |
There was a problem hiding this comment.
hash_password() and verify_password() both hard-code the PBKDF2 iteration count (100_000), so changing one without the other would silently break password verification. Consider centralizing the parameter (and/or encoding it into the stored hash format) to prevent drift.
Severity: low
Other Locations
qualia/backend/app/core/security.py:23
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback:Chose good-to-have because this comment found one maintainability gap that could quietly break password verification later. It pointed out that duplicating the iteration count in two places makes future tuning risky because one side could change without the other.
| def create_access_token(subject: str, expires_in: int = 3600) -> str: | ||
| now = int(time.time()) | ||
| header = {"alg": "HS256", "typ": "JWT"} | ||
| payload = {"sub": subject, "exp": now + expires_in} |
There was a problem hiding this comment.
create_access_token() will happily accept expires_in <= 0, which mints immediately-expired tokens (exp in the past) and can be a hard-to-debug footgun. Consider validating or explicitly documenting that non-positive values are allowed.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback:Marked useful because this comment caught one real token-creation footgun that would be confusing in normal auth flows. It pointed out that non-positive expiry values can mint immediately unusable tokens and lead to hard-to-debug login behavior.
|
augment review |
No description provided.