learning-website-framework1-9  Exercise 3: Review Your Own Login Code
=================================================================

The site already has one real account system: the Anime Vault (a separate PHP
and MySQL application in astro-site/public/anime). Review it against a short
checklist, by script first, then by reading the findings.

Checklist the script applies:
1. every admin page calls requireAdmin() or requireLogin()
2. every page that handles POST data verifies a CSRF token
3. no SQL is built by putting variables inside a query() or exec() string
4. no request value is printed without escaping

Save as audit_php.py:

import os, re, sys

def audit_file(path):
    text = open(path, encoding="utf-8", errors="ignore").read()
    rel = path
    findings = []
    is_admin = f"{os.sep}admin{os.sep}" in path
    # 1. every admin page must call requireAdmin() (or at least requireLogin())
    if is_admin and "requireAdmin(" not in text and "requireLogin(" not in text:
        findings.append("admin page without requireAdmin()/requireLogin()")
    # 2. a page that handles POST data should verify the CSRF token
    handles_post = "$_POST" in text or "REQUEST_METHOD" in text
    if handles_post and "verifyCsrf(" not in text:
        findings.append("handles POST but never calls verifyCsrf()")
    # 3. SQL built with variables instead of prepared statements
    for m in re.finditer(r"->(query|exec)\(\s*([\"'])(.*?)\2", text, re.S):
        if "$" in m.group(3):
            findings.append(f"{m.group(1)}() with a variable inside the SQL")
    # 4. output of a request value without escaping (rough check)
    for m in re.finditer(r"echo\s+\$_(GET|POST|REQUEST)\[", text):
        findings.append("echo of a request value without escaping")
    counts = {
        "prepare": len(re.findall(r"->prepare\(", text)),
        "raw query/exec": len(re.findall(r"->(query|exec)\(", text)),
    }
    return findings, counts

if __name__ == "__main__":
    root = sys.argv[1]
    total = {"prepare": 0, "raw query/exec": 0}
    files = flagged = 0
    for dirpath, dirnames, filenames in os.walk(root):
        for name in sorted(filenames):
            if not name.endswith(".php"):
                continue
            files += 1
            p = os.path.join(dirpath, name)
            findings, counts = audit_file(p)
            for k in total:
                total[k] += counts[k]
            if findings:
                flagged += 1
                print(os.path.relpath(p, root).replace(os.sep, "/"))
                for f in findings:
                    print("   -", f)
    print()
    print(f"{files} PHP files, {flagged} with findings")
    print(f"prepared statements: {total['prepare']}, raw query()/exec() calls: {total['raw query/exec']}")

Run it:

  python audit_php.py "<path to>/astro-site/public/anime"

Output (checked by running it):

login.php
   - handles POST but never calls verifyCsrf()
admin/delete_anime.php
   - handles POST but never calls verifyCsrf()
admin/reviews.php
   - query() with a variable inside the SQL
admin/sidebar.php
   - admin page without requireAdmin()/requireLogin()

23 PHP files, 4 with findings
prepared statements: 60, raw query()/exec() calls: 14

Reading each finding (I opened every flagged file)
--------------------------------------------------
1. login.php, "never calls verifyCsrf()": FALSE POSITIVE. It checks the token
   inline with hash_equals(csrfToken(), $token), which is the right comparison.
2. admin/delete_anime.php, same message: FALSE POSITIVE for the check (it uses
   hash_equals too), but note a design point: the delete can be triggered by a
   GET link carrying the token in the address. A token-protected GET is better
   than none, but state-changing actions belong in POST requests, because
   addresses end up in logs, history and Referer headers.
3. admin/reviews.php, "query() with a variable": harmless here. The values
   are $perPage (fixed at 30) and $offset, calculated from an (int) cast of the
   page number, so no text from the visitor reaches the SQL. Using a prepared
   statement would still be the more robust habit.
4. admin/sidebar.php, "no requireAdmin()": FALSE POSITIVE. It is an include
   file (a menu fragment) used by pages that do call requireAdmin().

Result: no real vulnerability found by this check. What the code does well, as
seen while reading it: bcrypt hashing with cost 12, prepared statements in 60
places, a session cookie with HttpOnly, SameSite=Lax and Secure on HTTPS, a new
session id at login (session_regenerate_id), and a per-session CSRF token.
This is a sound base to copy if shared accounts are ever added.

The check is a heuristic. It cannot prove the code safe: it did not look at
file uploads, output escaping in every template, rate limiting of logins, or
password reset flows. A passing scan is a starting point for a proper review.

WHY THIS WORKS AS AN ANSWER
---------------------------
Before designing a new account system, look at the one you already run. The
script gives a first pass in seconds, and reading each finding separates real
problems from false alarms, which is the honest way to use any scanner.