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

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 codestore/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 authenticationstore/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 acceptedstore/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 callsstore/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 unexpectedlystore/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 SystemExitstore/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 amountstore/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 coveragetests/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 coupontests/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 teststests/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 untestedtests/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 datestore/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.pystore/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 submittedAfter fixes
Tests8 of 8 passing37 of 37 passing
Automated check warnings220

The pull request passed all its tests as submitted. Passing tests did not mean the code was safe.

Files in this pull request