prototoolagain/todo.md
James Devine 39f8b4abb5
i got claude to review it
Added a detailed code review for the project, highlighting critical security issues, structural problems, code quality concerns, and dependency issues, along with a prioritized task list for improvements.
2026-03-23 11:23:02 +01:00

7.3 KiB

Let me fetch the key source files to do a proper review.Now let me look at the actual app source files:I now have a solid picture of the repo. GitHub's robots.txt blocks direct fetching of file tree pages, so I can't deep-dive into the app/ source files directly — but I've read config.py, run.py, setup.py, requirements.txt, .gitignore, and the full PROMPT.md spec. Here's a thorough review:


Code Review: pingud98/prototoolagain

🔴 Critical Security Issues

1. Hardcoded secret key in config.py SECRET_KEY = "dev-secret-key" is committed in plain text. If this ever ends up in production (or if someone forks/deploys without reading carefully), all Flask sessions, CSRF tokens, and signed cookies are compromised. The PROMPT.md spec even says to load it from an env var or auto-generate to .env — this wasn't implemented. Fix: SECRET_KEY = os.environ.get("SECRET_KEY") or secrets.token_hex(32) and never commit it.

2. run.py binds to 0.0.0.0 with ssl_context="adhoc" The "adhoc" SSL context generates a new self-signed cert on every restart (requires pyOpenSSL). This is fine for quick testing but: (a) it's not using the certs generated by setup.py (so setup.py's generate_cert() is effectively dead code), (b) binding to 0.0.0.0 exposes the app on all interfaces — potentially a problem if deployed on a machine with public network interfaces. Fix: Use ssl_context=("certs/cert.pem", "certs/key.pem") to use the actual generated certs.

3. setup.py uses --break-system-packages Installing dependencies with pip install --break-system-packages can damage the system Python environment. This is a bad pattern even in dev, and dangerous if run as root. Fix: Create and activate a virtualenv first, or at minimum document clearly this is not for use on system Python.

4. setup.pycreate_admin_user() is a stub (pass) The function that should prompt for admin credentials and create the first user does nothing. This means after running setup.py there is no admin account and the app is essentially unusable without manually inserting a DB record. Fix: Implement the credential prompt and user insertion, or raise a NotImplementedError so it fails loudly rather than silently.

5. setup.py — schema never initialized create_db() opens a SQLite connection and immediately closes it with a # TODO: Initialize schema comment. No tables are created. The app will crash on first use with "no such table" errors. Fix: Call db.create_all() within the app context, or run Alembic migrations.


🟠 Structural / Architecture Issues

6. Duplicate/confused project structure The repo has both a top-level app/ directory and an inspection-app/app/ subdirectory, plus a src/ directory. It's unclear which one is actually the running application. run.py does from app import create_app — if app/ at the root is incomplete or a stub, this will fail at import.

7. No version pinning on dependencies requirements.txt pins exact versions (good), but there's no pip-tools lockfile or pyproject.toml. The pinned versions are from 2023 (Flask==2.3.3, Flask-Login==0.6.0), which may have known CVEs by now. Fix: Run pip-audit against the current requirements and update to current stable versions.

8. SESSION_TYPE = "filesystem" without Flask-Session installed config.py sets SESSION_TYPE = "filesystem" but Flask-Session is not in requirements.txt. This config key will simply be ignored by Flask-Login's default cookie-based sessions, which may create a false sense of security (thinking server-side sessions are active when they aren't). Fix: Either add Flask-Session to requirements, or remove SESSION_TYPE from config and document the session strategy clearly.

9. REMEMBER_COOKIE_SECURE = True but dev might run without HTTPS If someone runs in a non-HTTPS dev context, the remember-me cookie will never be sent, breaking that feature silently. Needs a dev/prod config split.


🟡 Code Quality / Maintainability Issues

10. setup.py name collision Naming the setup script setup.py conflicts with Python's packaging convention (setuptools). Running pip install -e . or other tooling could misinterpret it. Fix: Rename to init_app.py or bootstrap.py.

11. setup.py uses raw sqlite3 instead of SQLAlchemy The spec calls for SQLAlchemy ORM only — no raw SQL — but setup.py uses import sqlite3 directly. Inconsistent, and means the schema init path bypasses the ORM models entirely.

12. No tests despite a tests/ directory The tests/ directory exists but appears empty (no test files visible in the tree). For an auth-heavy app with admin roles, there should be at minimum auth boundary tests and CRUD tests.

13. No app/__init__.py / create_app() visible The run.py imports from app import create_app but we can't confirm that factory function exists and is correct. Given the stub state of setup.py, this may not be wired up properly.

14. Duplicate entries in .gitignore pip-log.txt appears twice, env/ and venv/ each appear twice, and *.log appears twice. Minor, but sloppy.

15. PROMPT.md committed to the repo The full AI generation prompt is committed publicly. While not a hard security issue, it exposes your complete design intent, architecture choices, and security requirements to anyone who looks — including potential attackers who now know exactly what the app is supposed to do and can probe for gaps between spec and implementation.


🔵 Dependency Concerns

16. Both bcrypt and passlib and Flask-Bcrypt are all installed Three overlapping password hashing libraries. passlib wraps bcrypt, and Flask-Bcrypt wraps bcrypt directly. Pick one (recommend Flask-Bcrypt with bcrypt as the backend, and drop passlib).

17. WeasyPrint==60.1 has complex system dependencies WeasyPrint requires Pango, Cairo, and other native libs. The setup.py doesn't check for or install these, so PDF export will fail silently on systems missing those libraries. The README should document this.


Summary for Claude Code

Here's a prioritised task list to hand off:

  1. Replace hardcoded SECRET_KEY with env var loading + auto-generation to .env
  2. Fix run.py SSL context to use certs/cert.pem / certs/key.pem
  3. Implement create_admin_user() in setup.py with interactive credential prompt
  4. Implement create_db() using SQLAlchemy db.create_all() within app context
  5. Resolve directory structure — clarify which app/ is canonical and remove dead directories
  6. Add Flask-Session to requirements.txt or remove SESSION_TYPE from config
  7. Run pip-audit on current requirements and update stale/vulnerable packages
  8. Rename setup.py to avoid packaging tool conflicts
  9. Refactor setup to use SQLAlchemy rather than raw sqlite3
  10. Add basic auth and route tests to the tests/ directory
  11. Remove duplicate .gitignore entries (cosmetic but clean up)
  12. Consolidate password hashing — remove passlib, keep bcrypt + Flask-Bcrypt
  13. Document WeasyPrint system dependencies in README

This is a solid scaffold but it's not yet runnable — the two stubs in setup.py mean the database is never initialized and there's no admin account, so the app would crash on first use. That's the most urgent thing to fix.