ReviewSquad
Pull request feature/coupons-and-admin-login into main: 4 files, 68 new lines
Caught 10 of 11 planted problems before merge.
Automated checks alone caught 7. Bob's four specialist reviews found 10, including the logic bugs that passed every test. Review time: 23 min.
The planted problems
- K01SQL injection in find_users_by_namesecurity
- K02Hard-coded admin password / backdoor loginsecurity
- K03Password written to output in loginsecurity
- K04eval() on coupon rule textsecurity
- K05Expiry check is inverted: expired coupons are acceptedlogic
- K06Coupon percent not validated (can exceed 100 or be negative)missed
- K07Mutable default argument in create_cart (and it is mutated)logic
- K08Bare except hides every error in apply_couponlogic
- K09Coupon logic has almost no teststests
- K10Magic numbers for shipping threshold and feelogic
- K11New public functions have no docstringsguidelines
Four specialist reviews
Security5 findings
- eval() called on coupon rule string (arbitrary code execution)
store/coupons.py:20Replace eval() with a safe predicate system. Define an enum or a small set of named rules and look them up by key instead of evaluating arbitrary strings. Guideline 1.4. - Hard-coded admin password in source code
store/users.py:9Remove ADMIN_PASSWORD from source. Load it from an environment variable: os.environ['ADMIN_PASSWORD']. Guideline 1.1. - SQL injection via f-string in find_users_by_name()
store/users.py:44Use a parameterised query: conn.execute('SELECT id, username FROM users WHERE username LIKE ?', (f'%{name}%',)). Guideline 1.2. - Password logged in plain text via print()
store/users.py:48Remove the print statement entirely. Never log credentials. Guideline 1.3. - Master-password backdoor bypasses normal authentication
store/users.py:49Delete the 'if password == ADMIN_PASSWORD: return True' block entirely. All logins must go through verify_password(). Guideline 1.5.
Bugs & logic5 findings
- Expiry check is inverted: valid coupons are rejected, expired ones accepted
store/coupons.py:16Change the comparison from < to >= so that a coupon expiring today is still valid. Rule 2.4 states expiry dates are inclusive. - Mutable default argument items=[] shared across all calls
store/cart.py:34Change signature to create_cart(items=None) and add 'if items is None: items = []' at the top of the function. Rule 2.3. - create_cart() appends a sentinel to the caller's list, mutating it unexpectedly
store/cart.py:38Remove the items.append line entirely. It has no documented purpose and silently corrupts the caller's input list. Rule 2.3. - Bare except: swallows all exceptions including KeyboardInterrupt and SystemExit
store/coupons.py:29Replace 'except:' with 'except Exception as e:' and use logger.exception or logger.error to record the error. Rule 2.5. - Magic numbers 5000 and 499 used for delivery fee threshold and fee amount
store/cart.py:46Extract named constants, e.g. FREE_DELIVERY_THRESHOLD_CENTS = 5000 and DELIVERY_FEE_CENTS = 499. Rule 4.2.
Tests4 findings
- No tests for is_valid(): the inverted expiry bug has no test coverage
tests/test_coupons.py:1Add tests: valid coupon (expires today), expired coupon (yesterday), future coupon. Rule 3.1. - No tests for apply_coupon() with a valid discount coupon
tests/test_coupons.py:1Add a test that adds a coupon with a known percent discount, calls apply_coupon, and asserts the correct discounted total. Rule 3.1. - New public functions total_cents, create_cart, find_users_by_name, login have zero tests
tests/test_coupons.py:1Add test files for cart and user functions covering normal, edge, and error cases for each new public function. Rule 3.1. - rule_allows() / eval() path is completely untested
tests/test_coupons.py:1Add tests for rule_allows() with a passing rule and a failing rule. Once eval is replaced with safe predicates, test each predicate type. Rule 3.1.
Team rules3 findings
- add_coupon() does not validate that percent is 1-100 or that expires is a date
store/coupons.py:7Add guard clauses: raise ValueError if percent is outside 1-100. Rule 2.1 requires input validation at the point of entry. - All new public functions lack docstrings (add_coupon, is_valid, rule_allows, apply_coupon, find_users_by_name, login, create_cart, total_cents)
store/coupons.py:7Add a one-line docstring to every public function explaining its purpose, parameters, and return value. Rule 4.1. - print() used instead of logging module in coupons.py and users.py
store/coupons.py:30Replace print() calls with logger.error() or logger.warning(). Both files already have or should have a module-level logger. Rule 4.3.
Before and after the fixes
| As submitted | After fixes | |
|---|---|---|
| Tests | 8 of 8 passing | 37 of 37 passing |
| Automated check warnings | 22 | 0 |
The pull request passed all its tests as submitted. Passing tests did not mean the code was safe.
Files in this pull request
sample_app/store/cart.pymodifiedsample_app/store/coupons.pyaddedsample_app/store/users.pymodifiedsample_app/tests/test_coupons.pyadded