Add SQLite-backed user store#2
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def find_user(self, username): | ||
| # Look up a user by name. | ||
| query = "SELECT id, username FROM users WHERE username = '%s'" % username | ||
| cur = self.conn.execute(query) | ||
| return cur.fetchone() |
There was a problem hiding this comment.
find_user
The query is built via % username string formatting, making it directly injectable — e.g., passing ' OR '1'='1 dumps the full users table. Every other method in this file correctly uses the parameterized ? form; this one was missed.
| def find_user(self, username): | |
| # Look up a user by name. | |
| query = "SELECT id, username FROM users WHERE username = '%s'" % username | |
| cur = self.conn.execute(query) | |
| return cur.fetchone() | |
| def find_user(self, username): | |
| # Look up a user by name. | |
| cur = self.conn.execute( | |
| "SELECT id, username FROM users WHERE username = ?", (username,) | |
| ) | |
| return cur.fetchone() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: user_store.py
Line: 25-29
Comment:
**SQL Injection in `find_user`**
The query is built via `% username` string formatting, making it directly injectable — e.g., passing `' OR '1'='1` dumps the full users table. Every other method in this file correctly uses the parameterized `?` form; this one was missed.
```suggestion
def find_user(self, username):
# Look up a user by name.
cur = self.conn.execute(
"SELECT id, username FROM users WHERE username = ?", (username,)
)
return cur.fetchone()
```
How can I resolve this? If you propose a fix, please make it concise.| def _hash(self, password): | ||
| return hashlib.md5(password.encode()).hexdigest() |
There was a problem hiding this comment.
MD5 runs at billions of hashes/sec on commodity GPUs, and the lack of a per-user salt means the entire password column is crackable in bulk via rainbow tables. OWASP and NIST both require a purpose-built slow hash. Python's built-in hashlib.scrypt or the popular bcrypt library are drop-in alternatives.
| def _hash(self, password): | |
| return hashlib.md5(password.encode()).hexdigest() | |
| def _hash(self, password): | |
| # scrypt is available in Python 3.6+ with no extra dependencies. | |
| salt = hashlib.sha256(password.encode()).digest() # placeholder; use os.urandom(16) stored per-user | |
| return hashlib.scrypt(password.encode(), salt=salt, n=2**14, r=8, p=1).hex() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: user_store.py
Line: 15-16
Comment:
**Unsalted MD5 is unsafe for password storage**
MD5 runs at billions of hashes/sec on commodity GPUs, and the lack of a per-user salt means the entire password column is crackable in bulk via rainbow tables. OWASP and NIST both require a purpose-built slow hash. Python's built-in `hashlib.scrypt` or the popular `bcrypt` library are drop-in alternatives.
```suggestion
def _hash(self, password):
# scrypt is available in Python 3.6+ with no extra dependencies.
salt = hashlib.sha256(password.encode()).digest() # placeholder; use os.urandom(16) stored per-user
return hashlib.scrypt(password.encode(), salt=salt, n=2**14, r=8, p=1).hex()
```
How can I resolve this? If you propose a fix, please make it concise.| def __init__(self, db_path=":memory:"): | ||
| self.conn = sqlite3.connect(db_path) | ||
| self.conn.execute( | ||
| "CREATE TABLE IF NOT EXISTS users " | ||
| "(id INTEGER PRIMARY KEY, username TEXT UNIQUE, pw_hash TEXT)" |
There was a problem hiding this comment.
sqlite3.connect() returns a connection object that is not safe to share across threads by default. If this UserStore instance is ever shared between request-handling threads (common in any WSGI/ASGI server), concurrent calls to create_user, find_user, or verify will hit ProgrammingError: SQLite objects created in a thread can only be used in that same thread. Either pass check_same_thread=False and add your own lock, or create a connection per call, or use a connection pool.
Prompt To Fix With AI
This is a comment left during a code review.
Path: user_store.py
Line: 8-12
Comment:
**Single connection is not thread-safe**
`sqlite3.connect()` returns a connection object that is not safe to share across threads by default. If this `UserStore` instance is ever shared between request-handling threads (common in any WSGI/ASGI server), concurrent calls to `create_user`, `find_user`, or `verify` will hit `ProgrammingError: SQLite objects created in a thread can only be used in that same thread`. Either pass `check_same_thread=False` and add your own lock, or create a connection per call, or use a connection pool.
How can I resolve this? If you propose a fix, please make it concise.
| query = "SELECT id, username FROM users WHERE username = '%s'" % username | ||
| cur = self.conn.execute(query) |
There was a problem hiding this comment.
find_user. The username argument is interpolated directly into the query string via % formatting, allowing an attacker to inject arbitrary SQL (e.g. passing ' OR '1'='1 as the username would return every row). Both create_user and verify already use parameterized queries correctly — this method should do the same.
| query = "SELECT id, username FROM users WHERE username = '%s'" % username | |
| cur = self.conn.execute(query) | |
| cur = self.conn.execute( | |
| "SELECT id, username FROM users WHERE username = ?", (username,) | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: user_store.py
Line: 27-28
Comment:
SQL Injection vulnerability in `find_user`. The `username` argument is interpolated directly into the query string via `%` formatting, allowing an attacker to inject arbitrary SQL (e.g. passing `' OR '1'='1` as the username would return every row). Both `create_user` and `verify` already use parameterized queries correctly — this method should do the same.
```suggestion
cur = self.conn.execute(
"SELECT id, username FROM users WHERE username = ?", (username,)
)
```
How can I resolve this? If you propose a fix, please make it concise.| def _hash(self, password): | ||
| return hashlib.md5(password.encode()).hexdigest() |
There was a problem hiding this comment.
hashlib.scrypt (or the third-party bcrypt/argon2-cffi) which are purpose-built for this, slow by design, and incorporate a per-user salt automatically.
| def _hash(self, password): | |
| return hashlib.md5(password.encode()).hexdigest() | |
| def _hash(self, password: str) -> str: | |
| # hashlib.scrypt is in the stdlib (Python 3.6+) and is salt-aware. | |
| # For production, prefer bcrypt or argon2-cffi. | |
| salt = secrets.token_bytes(16) | |
| dk = hashlib.scrypt(password.encode(), salt=salt, n=2**14, r=8, p=1) | |
| # Store salt + derived key together so verify() can reconstruct it. | |
| return salt.hex() + ":" + dk.hex() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: user_store.py
Line: 15-16
Comment:
MD5 is unsuitable for password storage. It is a general-purpose, computationally cheap hash with no built-in salting, so a leaked database can be cracked in seconds with rainbow tables or GPU brute-force. Python's standard library ships `hashlib.scrypt` (or the third-party `bcrypt`/`argon2-cffi`) which are purpose-built for this, slow by design, and incorporate a per-user salt automatically.
```suggestion
def _hash(self, password: str) -> str:
# hashlib.scrypt is in the stdlib (Python 3.6+) and is salt-aware.
# For production, prefer bcrypt or argon2-cffi.
salt = secrets.token_bytes(16)
dk = hashlib.scrypt(password.encode(), salt=salt, n=2**14, r=8, p=1)
# Store salt + derived key together so verify() can reconstruct it.
return salt.hex() + ":" + dk.hex()
```
How can I resolve this? If you propose a fix, please make it concise.| def verify(self, username, password): | ||
| row = self.conn.execute( | ||
| "SELECT pw_hash FROM users WHERE username = ?", (username,) | ||
| ).fetchone() | ||
| if row is None: | ||
| return False | ||
| return row[0] == self._hash(password) |
There was a problem hiding this comment.
verify will break once _hash is fixed to use a salted algorithm. The current verify calls self._hash(password) again and does a direct equality check against the stored hash, which only works because MD5 is deterministic with no salt. Once you switch to a salted scheme (e.g. scrypt), each call to _hash produces a different output, so the comparison will always return False. The fix is to store the salt in the database alongside the hash and use hmac.compare_digest for the comparison, or use a library like bcrypt whose checkpw handles this transparently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: user_store.py
Line: 31-37
Comment:
**`verify` will break once `_hash` is fixed to use a salted algorithm.** The current `verify` calls `self._hash(password)` again and does a direct equality check against the stored hash, which only works because MD5 is deterministic with no salt. Once you switch to a salted scheme (e.g. scrypt), each call to `_hash` produces a different output, so the comparison will always return `False`. The fix is to store the salt in the database alongside the hash and use `hmac.compare_digest` for the comparison, or use a library like `bcrypt` whose `checkpw` handles this transparently.
How can I resolve this? If you propose a fix, please make it concise.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Adds a small
UserStorefor the demo app.create_user/verifyfor basic authfind_userlookup helperOpening this to have automated review run over the diff tomorrow.
🤖 Generated with Claude Code
Greptile Summary
This PR introduces a new
UserStoreclass backed by SQLite, providingcreate_user,verify, andfind_usermethods for a demo app. The implementation has two critical security vulnerabilities that must be resolved before this code is used in any context.find_userconstructs its query via Python%string formatting instead of parameterized placeholders, letting an attacker inject arbitrary SQL. The sibling methodscreate_userandverifyalready use?parameters correctly \u2014find_usershould follow the same pattern.hashlib.scrypt,bcrypt, orargon2-cffi) with a per-user salt is required.verifyafter fix: The currentverifymethod depends on MD5\u2019s determinism; switching to a salted algorithm will cause it to always returnFalseunless it is updated to re-extract the stored salt and use a constant-time comparison.Confidence Score: 1/5
Not safe to merge — the file introduces exploitable SQL injection and stores passwords with unsalted MD5.
Two concrete security defects are present in the only changed file:
find_useraccepts attacker-controlled input directly into a SQL string (classic injection), and_hashuses unsalted MD5 (a general-purpose hash with no salt, crackable with rainbow tables in seconds). Both affect core auth paths. Additionally, fixing_hashwithout also updatingverifywill cause every login check to silently returnFalse.user_store.py — all three methods need attention:
find_user(query construction),_hash(hashing algorithm), andverify(comparison logic).Security Review
user_store.pyline 27,find_user): User-suppliedusernameis interpolated into the SQL string via%formatting. An attacker can pass a crafted username (e.g.' OR '1'='1) to bypass the filter, dump all users, or corrupt data.user_store.pyline 16,_hash): Passwords are stored as bare MD5 digests with no salt. MD5 can be brute-forced at billions of hashes/second on commodity hardware and is fully covered by precomputed rainbow tables, meaning a stolen database immediately exposes all credentials.Important Files Changed
find_user(string-formatted query) and MD5 password hashing without a salt;verifyis also logically broken once the hash function is corrected.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller participant UserStore participant SQLite Caller->>UserStore: create_user(username, password) UserStore->>UserStore: _hash(password) → MD5 (⚠️ no salt) UserStore->>SQLite: INSERT INTO users (parameterized ✓) SQLite-->>UserStore: ok UserStore-->>Caller: (void) Caller->>UserStore: find_user(username) UserStore->>SQLite: "SELECT … WHERE username = '%s' (⚠️ SQL injection)" SQLite-->>UserStore: row or None UserStore-->>Caller: (id, username) or None Caller->>UserStore: verify(username, password) UserStore->>SQLite: "SELECT pw_hash WHERE username = ? (parameterized ✓)" SQLite-->>UserStore: pw_hash UserStore->>UserStore: _hash(password) → MD5 (⚠️ no salt) UserStore->>UserStore: "row[0] == computed_hash" UserStore-->>Caller: True / False%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller participant UserStore participant SQLite Caller->>UserStore: create_user(username, password) UserStore->>UserStore: _hash(password) → MD5 (⚠️ no salt) UserStore->>SQLite: INSERT INTO users (parameterized ✓) SQLite-->>UserStore: ok UserStore-->>Caller: (void) Caller->>UserStore: find_user(username) UserStore->>SQLite: "SELECT … WHERE username = '%s' (⚠️ SQL injection)" SQLite-->>UserStore: row or None UserStore-->>Caller: (id, username) or None Caller->>UserStore: verify(username, password) UserStore->>SQLite: "SELECT pw_hash WHERE username = ? (parameterized ✓)" SQLite-->>UserStore: pw_hash UserStore->>UserStore: _hash(password) → MD5 (⚠️ no salt) UserStore->>UserStore: "row[0] == computed_hash" UserStore-->>Caller: True / FalsePrompt To Fix All With AI
Reviews (2): Last reviewed commit: "Add SQLite-backed user store" | Re-trigger Greptile