pingu_98/prototoolagain
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

i got claude to review it

Browse source 
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.
This commit is contained in:
James Devine 2026-03-23 11:23:02 +01:00 committed by GitHub
parent 796c4b7910
commit 39f8b4abb5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 100 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

100
todo.md Normal file
View file

@ -0,0 +1,100 @@
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.py` — `create_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.