/engineering:code-review 2회차 · Claude Code · Opus 5.5 · 생각 깊이 중간 · 클로드가 쓴 review.md 원문 · 목록

Code Review: `shop.py` (쇼핑몰 주문 처리 모듈)

검토일: 2026-10-16 · 대상: shop.py (85줄, sqlite3) 아래 표에 "재현됨"으로 표시한 항목은 임시 DB를 만들어 실제로 실행해 확인했습니다. 실행 결과는 맨 아래 부록에 있습니다.

Summary

코드는 짧고 읽기 쉽고, 대부분의 쿼리에 파라미터 바인딩(?)을 써서 기본은 되어 있습니다. 하지만 돈·재고·개인정보를 다루는 경로에 바로 악용할 수 있는 결함이 여러 개 있습니다. SQL 인젝션으로 비밀번호 해시가 새고, 수량에 음수를 넣으면 잔액이 생기고, 남의 주문과 서버 파일을 읽을 수 있습니다. 이 상태로는 이번 주 실서비스에 올리면 안 됩니다.

Verdict

❌ Request Changes (머지 불가)

배포 전에 반드시 고칠 것: Critical 1~6번, High 7~9번. 고친 뒤에는 아래 "권장 테스트"를 추가하고 다시 리뷰하는 것을 권합니다.

Critical Issues

#	Line	Issue	Severity	재현
1	19	`search_products`: f-string으로 SQL을 조립해서 SQL 인젝션 발생. 검색어에 `' UNION SELECT id, email, password_hash FROM users --`를 넣으면 전체 회원의 이메일과 비밀번호 해시가 상품 목록인 것처럼 반환됨	🔴 Critical	✅
2	7	`ADMIN_TOKEN = "sk_live_..."`: 운영용(live) 비밀키가 소스에 하드코딩되어 있음. 이미 친구와 공유됐으니 유출된 것으로 보고 키 폐기·재발급이 필요함	🔴 Critical	—
3	46–66	`place_order`: `qty` 검증이 없음. `qty=-10`이면 재고 검사(`5 < -10`)를 통과한 뒤 재고가 +10, 잔액이 +10,000 됨. 결국 무한으로 돈을 만들 수 있음	🔴 Critical	✅
4	61–64	`place_order`: 잔액 확인 없이 차감함. 잔액이 0원인 사용자가 3,000원어치를 주문하면 잔액이 −3,000이 되고 주문은 성공함	🔴 Critical	✅
5	40–43	`get_order`: `current_user_id`를 받기만 하고 쓰지 않음. IDOR라서 주문 ID만 바꾸면 남의 주문을 볼 수 있음. 게다가 `SELECT *`로 모든 컬럼을 그대로 노출함	🔴 Critical	✅
6	77–80	`download_invoice`: 경로 탐색(Path Traversal) 가능. `../shop.py`나 `/etc/hosts`를 넣으면 서버의 아무 파일이나 읽힘. 절대경로를 넣으면 `os.path.join`이 `INVOICE_DIR`을 아예 무시함. 다운로드하는 사람이 그 송장의 주인인지도 확인하지 않음	🔴 Critical	✅

High

#	Line	Issue	Severity	재현
7	48–55	`place_order`: 재고를 조회한 뒤 따로 차감하는 구조(TOCTOU)라서 경쟁 상태가 생김. 동시에 들어온 주문 두 개가 둘 다 `stock=1`을 읽고 통과하면 초과 판매(재고가 음수)가 됨. SELECT 시점에는 트랜잭션이 아직 시작되지 않은 상태임	🟠 High	코드 분석
8	51	`place_order`: 상품이 없으면 `stock`이 `None`이라서 `TypeError: 'NoneType' object is not subscriptable`가 나고 500 에러가 됨	🟠 High	✅
9	25	`list_orders`: off-by-one. `offset = page * PAGE_SIZE` 때문에 `page=1`이 OFFSET 20부터 시작함. 주문이 20개 이하인 사용자는 1페이지가 빈 목록으로 나옴. `ORDER BY`도 없어서 페이지 순서가 보장되지 않음	🟠 High	✅
10	69–74	`apply_coupon`: ① `except Exception: pass` 때문에 오류가 나면 조용히 `None`을 반환하고, 이후 계산에서 터지거나 0원 처리될 수 있음. ② 할인율 범위 검증이 없어서 `percent=150`이면 −5,000원, 음수 percent면 오히려 가격이 오름	🟠 High	✅

Suggestions

#	Line	Suggestion	Category
11	11–14	모든 함수가 `get_db()`로 연결을 열고 닫지 않음. 요청이 몰리면 파일 핸들이 새고 `database is locked`가 날 수 있음. `contextlib.closing`이나 요청 단위 연결 관리로 바꿀 것	Performance / Resource
12	31–36	`list_orders`: 주문마다 `order_items`를 따로 조회하는 N+1 쿼리. `WHERE order_id IN (...)` 한 번이나 JOIN으로 묶을 것. `created_at`은 조회하고도 결과에 넣지 않음	Performance
13	83–84	`is_admin`: `==` 비교는 타이밍 공격에 취약함. `hmac.compare_digest`를 쓸 것 (2번에서 키를 환경변수로 옮기면서 함께 처리)	Security
14	63, 72	금액을 `float`로 계산함(`total * rate`). 원 단위 정수나 `Decimal`로 통일하고 반올림 규칙을 명시할 것	Correctness
15	5–6	`DB_PATH`, `INVOICE_DIR`이 상대경로라서 실행 위치(cwd)에 따라 다른 DB나 폴더를 보게 됨. 설정이나 환경변수에서 절대경로로 받을 것	Maintainability
16	—	테스트가 하나도 없음. 돈이 오가는 모듈이므로 최소한 아래 "권장 테스트"는 필요함	Test Coverage
17	53–65	예외가 나도 명시적인 `rollback`이 없음. 지금은 연결이 GC될 때 우연히 롤백되는 것에 기대고 있음. `with db:`로 트랜잭션 경계를 명시할 것	Correctness

수정 예시

1. SQL 인젝션 (L19)

def search_products(keyword):
    with closing(get_db()) as db:
        sql = "SELECT id, name, price FROM products WHERE name LIKE ? ESCAPE '\\'"
        escaped = keyword.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
        return [dict(r) for r in db.execute(sql, (f"%{escaped}%",)).fetchall()]

2. 비밀키 (L7, L83)

import hmac

ADMIN_TOKEN = os.environ["SHOP_ADMIN_TOKEN"]  # 기존 sk_live 키는 즉시 폐기·재발급

def is_admin(token):
    return isinstance(token, str) and hmac.compare_digest(token, ADMIN_TOKEN)

3·4·7·8·17. 주문 처리 (L46–66): 입력 검증과 원자적 차감

def place_order(user_id, product_id, qty):
    if not isinstance(qty, int) or qty <= 0:
        raise ValueError("수량은 1 이상의 정수여야 합니다")

    with closing(get_db()) as db:
        db.isolation_level = None          # 트랜잭션을 직접 제어
        db.execute("BEGIN IMMEDIATE")      # 쓰기 잠금을 먼저 잡아 동시 주문 직렬화
        try:
            product = db.execute(
                "SELECT price FROM products WHERE id = ?", (product_id,)
            ).fetchone()
            if product is None:
                raise ValueError("존재하지 않는 상품")
            amount = product["price"] * qty

            # 조건부 UPDATE: 재고가 충분할 때만 차감
            if db.execute(
                "UPDATE products SET stock = stock - ? WHERE id = ? AND stock >= ?",
                (qty, product_id, qty),
            ).rowcount != 1:
                raise ValueError("재고 부족")

            if db.execute(
                "UPDATE users SET balance = balance - ? WHERE id = ? AND balance >= ?",
                (amount, user_id, amount),
            ).rowcount != 1:
                raise ValueError("잔액 부족")

            order_id = db.execute(
                "INSERT INTO orders (user_id) VALUES (?)", (user_id,)
            ).lastrowid
            db.execute(
                "INSERT INTO order_items (order_id, product_id, qty, price) VALUES (?, ?, ?, ?)",
                (order_id, product_id, qty, product["price"]),
            )
            db.execute("COMMIT")
            return order_id
        except Exception:
            db.execute("ROLLBACK")
            raise

DB 레벨에서도 막아두면 더 안전합니다: CHECK (stock >= 0), CHECK (balance >= 0), CHECK (qty > 0).

5. IDOR (L40–43)

def get_order(order_id, current_user_id):
    with closing(get_db()) as db:
        row = db.execute(
            "SELECT id, user_id, created_at FROM orders WHERE id = ? AND user_id = ?",
            (order_id, current_user_id),
        ).fetchone()
        return dict(row) if row else None

6. 경로 탐색 (L77–80)

INVOICE_DIR = os.path.realpath(os.environ.get("SHOP_INVOICE_DIR", "invoices"))

def download_invoice(filename, current_user_id):
    path = os.path.realpath(os.path.join(INVOICE_DIR, filename))
    if os.path.commonpath([path, INVOICE_DIR]) != INVOICE_DIR:
        raise PermissionError("잘못된 경로")
    # + 해당 송장이 current_user_id의 주문인지 DB에서 확인
    #   (가장 좋은 방법은 파일명 대신 order_id를 받아 서버가 경로를 만드는 것)
    with open(path, "rb") as f:
        return f.read()

9. 페이지네이션 (L25)

page = max(1, int(page))
offset = (page - 1) * PAGE_SIZE
# SQL에 ORDER BY created_at DESC, id DESC 추가

10. 쿠폰 (L69–74)

def apply_coupon(total, coupon):
    percent = coupon.get("percent")
    if not isinstance(percent, (int, float)) or not 0 <= percent <= 100:
        raise ValueError("유효하지 않은 쿠폰")
    return total - (total * percent) // 100   # 정수 원 단위

쿠폰 dict를 클라이언트가 보내는 구조라면, 할인율은 반드시 서버 DB에서 쿠폰 코드로 조회해야 합니다.

권장 테스트 (머지 조건)

search_products에 ', %, UNION 페이로드를 넣어도 다른 테이블 데이터가 나오지 않는다
place_order에 qty가 0, 음수, 실수, 문자열이면 거부한다
잔액 부족, 재고 부족, 존재하지 않는 상품이면 각각 명확한 예외를 내고 DB 상태가 변하지 않는다
재고 1개인 상품에 동시 주문 2건을 넣으면 1건만 성공한다 (스레드/프로세스 2개로 검증)
get_order로 타인의 주문을 조회하면 None이 나온다
download_invoice에 ../, 절대경로, 심볼릭 링크를 넣으면 거부한다
list_orders(page=1)은 첫 20건을 반환하고 순서가 일정하다
apply_coupon에 0%, 100%, 101%, −1%, 키 누락을 넣었을 때 기대대로 동작한다

What Looks Good

search_products를 빼면 모든 쿼리가 파라미터 바인딩을 사용함
row_factory = sqlite3.Row로 컬럼명 기반 접근을 해서 가독성이 좋음
재고 차감을 stock = stock - ?처럼 DB 쪽에서 계산하는 방향 자체는 맞음 (조건만 추가하면 원자적으로 만들 수 있음)
주문 시점의 가격을 order_items.price에 스냅샷으로 저장함. 나중에 상품 가격이 바뀌어도 주문 내역이 보존되는 좋은 설계임
함수 하나가 기능 하나를 맡아서 구조가 단순하고 수정하기 쉬움

부록: 재현 결과

임시 디렉터리에 shop.py 사본과 테스트 DB를 만들어 실행한 결과입니다 (원본 파일과 DB는 건드리지 않았습니다).

1 SQLi: [{'id': 1, 'name': 'a@a.com', 'price': 'HASH_A'}, {'id': 1, 'name': 'pen', 'price': 1000},
         {'id': 2, 'name': 'b@b.com', 'price': 'HASH_B'}]
2 negative qty -> stock/balance: (15,) (10000,)        # 재고 5→15, 잔액 0→10000
3 no balance check -> user2 balance: (-3000,)
4 page=1 for user with 5 orders: []
5 IDOR (user 2 reads order 1): {'id': 1, 'user_id': 1, 'created_at': '...'}
6 missing product: TypeError 'NoneType' object is not subscriptable
7 coupon 150%: -5000.0 / missing key: None
8 traversal ../shop.py: b'"""\xec\x9e\x91...'           # shop.py 소스 읽힘
8 absolute /etc/hosts: b'##\n# Host Database\n#'

Code Review: shop.py (쇼핑몰 주문 처리 모듈)