Replaces the two-page pandas-table UI with a small, testable Flask application and a beer-themed design.
The old UI had no navigation, no styling beyond a Bootstrap CDN link, and no way to see a run's comments or name on the summary tables. The alcohol figures it showed came from a formula that is not a recognised brewing formula.
What changed
Restructure
brewing.py holds all the domain logic as pure functions over plain data — no Flask, no Mongo. The gravity maths, ABV formula and run-aggregation rules are unit-testable on their own.
app_factory.py replaces the module-level app; routes move into blueprints under views/. One Mongo client per request, closed at teardown.
Dark beer-themed design system: warm amber #be7f39 on near-black, condensed display face (Anton) for headings, Inter for body. Fonts and the chart library are vendored into static/ — the old templates pulled Bootstrap from a CDN, so the container needed outbound network.
Navigation bar, login page, brew-process page with a proper form, and a run-detail page listing every reading with its comment.
Run summary tables now show the beer name and latest comment.
Native form controls (checkbox, number spinners, date icon, scrollbars) restyled for the dark theme.
Correctness
Alcohol now uses the standard formula(OG − FG) × 131.25. The old code used (max − min) / 7.36 + 0.5, which is not a recognised brewing formula and overstated every result.
Gravity points ↔ decimal readings now convert correctly both ways (1039 ↔ 1.039).
A run's display name resolves to the most-used label across its readings, because beer_type is stored per-reading and drifts across a run (run 4 was "Necatron-Christmas Beer" → "Nocturna the Christmas beer" → "Nocturna Christmas beer").
Re-submitting a reading for the same run and day updates it in place rather than creating a duplicate.
Security / production
The container ran the Werkzeug debugger with FLASK_DEBUG=1, exposing an interactive console on any unhandled error. Now runs waitress with debugging off.
SECRET_KEY comes from the environment instead of a hard-coded string in source.
Housekeeping
Removes the bokeh/duckdb/pandas dependency chain and the static file it generated at import time; removes dead add_user.py and test.py.
Splits requirements.txt (runtime) from requirements-dev.txt (tests).
Adds a pytest suite (64 tests) and a test job to CI so the image is only built when tests pass.
Adds a README describing the data model and a RUNBOOK for the cluster side.
Data compatibility
The existing beer_db.beer_data documents are read as they are — not migrated, not rewritten. No collection is dropped, and no document is modified other than through the explicit "record a reading" form, which keeps the original (date, beer_run_id) uniqueness rule.
Test plan
64 tests pass (python -m pytest tests/ -q)
Tests need no database — mongomock is used throughout
Verified against the live Mongo: all 47 existing readings render, all 18 runs parse
Login, record-a-reading, set/unset-final-reading and run-detail flows exercised in a browser
Clean-install check: fresh venv from requirements-dev.txt, tests pass, waitress serves the app
Needs a human look at the new UI — this is the part I would most like a second pair of eyes on
Notes for the reviewer
brewing.py is the file to read first — it holds the arithmetic and is where the behaviour change is.
The ABV change is deliberate and will make historic figures differ from what the old UI displayed. The old formula is retained as brewing.legacy_abv_percent() for comparison, and a later PR surfaces the difference on the history page.
The beer_runs metadata collection exists in the repository layer but is not yet populated or exposed — that is PR 2.
Deployed image still runs the old flask run entrypoint until this is merged and the image rebuilt; the Dockerfile change here turns that into waitress.
## Summary
Replaces the two-page pandas-table UI with a small, testable Flask application and a beer-themed design.
The old UI had no navigation, no styling beyond a Bootstrap CDN link, and no way to see a run's comments or name on the summary tables. The alcohol figures it showed came from a formula that is not a recognised brewing formula.
## What changed
### Restructure
- `brewing.py` holds all the domain logic as pure functions over plain data — no Flask, no Mongo. The gravity maths, ABV formula and run-aggregation rules are unit-testable on their own.
- `app_factory.py` replaces the module-level app; routes move into blueprints under `views/`. One Mongo client per request, closed at teardown.
- `mongo/repository.py` replaces `build_db.py` / `query_db.py` / `get_conn.py`.
### Interface
- Dark beer-themed design system: warm amber `#be7f39` on near-black, condensed display face (Anton) for headings, Inter for body. Fonts and the chart library are **vendored into `static/`** — the old templates pulled Bootstrap from a CDN, so the container needed outbound network.
- Navigation bar, login page, brew-process page with a proper form, and a run-detail page listing every reading with its comment.
- Run summary tables now show the beer name and latest comment.
- Native form controls (checkbox, number spinners, date icon, scrollbars) restyled for the dark theme.
### Correctness
- **Alcohol now uses the standard formula** `(OG − FG) × 131.25`. The old code used `(max − min) / 7.36 + 0.5`, which is not a recognised brewing formula and overstated every result.
- Gravity points ↔ decimal readings now convert correctly both ways (`1039` ↔ `1.039`).
- A run's display name resolves to the most-used label across its readings, because `beer_type` is stored per-reading and drifts across a run (run 4 was "Necatron-Christmas Beer" → "Nocturna the Christmas beer" → "Nocturna Christmas beer").
- Re-submitting a reading for the same run and day updates it in place rather than creating a duplicate.
### Security / production
- The container ran the **Werkzeug debugger with `FLASK_DEBUG=1`**, exposing an interactive console on any unhandled error. Now runs waitress with debugging off.
- `SECRET_KEY` comes from the environment instead of a hard-coded string in source.
### Housekeeping
- Removes the bokeh/duckdb/pandas dependency chain and the static file it generated at import time; removes dead `add_user.py` and `test.py`.
- Splits `requirements.txt` (runtime) from `requirements-dev.txt` (tests).
- Adds a pytest suite (64 tests) and a `test` job to CI so the image is only built when tests pass.
- Adds a README describing the data model and a RUNBOOK for the cluster side.
## Data compatibility
The existing `beer_db.beer_data` documents are read as they are — **not migrated, not rewritten**. No collection is dropped, and no document is modified other than through the explicit "record a reading" form, which keeps the original `(date, beer_run_id)` uniqueness rule.
## Test plan
- [x] 64 tests pass (`python -m pytest tests/ -q`)
- [x] Tests need no database — `mongomock` is used throughout
- [x] Verified against the live Mongo: all 47 existing readings render, all 18 runs parse
- [x] Login, record-a-reading, set/unset-final-reading and run-detail flows exercised in a browser
- [x] Clean-install check: fresh venv from `requirements-dev.txt`, tests pass, waitress serves the app
- [ ] **Needs a human look at the new UI** — this is the part I would most like a second pair of eyes on
## Notes for the reviewer
- `brewing.py` is the file to read first — it holds the arithmetic and is where the behaviour change is.
- The ABV change is deliberate and will make historic figures differ from what the old UI displayed. The old formula is retained as `brewing.legacy_abv_percent()` for comparison, and a later PR surfaces the difference on the history page.
- The `beer_runs` metadata collection exists in the repository layer but is not yet populated or exposed — that is PR 2.
- Deployed image still runs the old `flask run` entrypoint until this is merged and the image rebuilt; the Dockerfile change here turns that into waitress.
The old UI was two pages of raw pandas tables with no navigation, no styling
and no way to see a run's comments. This replaces it with a small,
testable application and a beer-themed design.
What changed
------------
Restructure
- The domain logic now lives in `brewing.py` as pure functions over plain
data, with no Flask or Mongo dependency. The gravity maths, the ABV
formula and the run-aggregation rules are unit-testable on their own.
- `app_factory.py` replaces the module-level app, and the routes move into
blueprints under `views/`. A request-scoped Mongo repository is created per
request and closed at teardown.
- `mongo/repository.py` replaces `build_db.py` / `query_db.py` / `get_conn.py`.
Reading a document no longer requires a second query to fetch the fields.
Interface
- A dark, beer-themed design system: warm amber `#be7f39` on near-black, a
condensed display face (Anton) for headings and Inter for body text. Both
fonts and the chart library are vendored into `static/`, so the container
needs no outbound network at runtime — the old templates pulled Bootstrap
from a CDN.
- A real navigation bar, a login page, a brew-process page with a proper
form, and a run-detail page listing every reading with its comment.
- The run summary tables now show the beer name and the latest comment, which
the old pandas tables did not.
- Native form controls (checkbox, number spinners, date icon, scrollbars)
are restyled for the dark theme.
Correctness
- The alcohol calculation now uses the standard homebrew formula,
`(OG - FG) x 131.25`. The old code used `(max - min) / 7.36 + 0.5`, which is
not a recognised brewing formula and overstated every result.
- Gravity points and decimal readings now convert correctly in both
directions (`1039` <-> `1.039`).
- A run's display name resolves to the most-used label across its readings,
because `beer_type` is stored per-reading and drifts across a run.
- Re-submitting a reading for the same run and day updates it in place
rather than creating a duplicate.
Security / production
- The container ran the Werkzeug debugger with `FLASK_DEBUG=1`, exposing an
interactive console on any unhandled error. It now runs waitress with
debugging off.
- `SECRET_KEY` comes from the environment.
- The app secret is no longer a hard-coded string in source.
Housekeeping
- Removes the bokeh/duckdb/pandas dependency chain and the static file it
generated at import time; removes the dead `add_user.py` and `test.py`.
- Splits `requirements.txt` (runtime) from `requirements-dev.txt` (tests).
- Adds a pytest suite (64 tests) and a `test` job to the CI workflow so the
image is only built when the tests pass.
- Adds a README describing the data model and a RUNBOOK for the cluster side.
Data compatibility
------------------
The existing `beer_db.beer_data` documents are read as they are and are not
migrated or rewritten. No collection is dropped, and no document is modified
by this change other than through the explicit "record a reading" form, which
keeps the original `(date, beer_run_id)` uniqueness rule.
Synthesized Review Report: feat(ui): rebuild the interface with a proper design system
1. Executive Summary
This Pull Request represents a major architectural evolution of the brewing application, transitioning it from a data-science script (Pandas/Bokeh) into a professional, structured Flask web application.
The overall quality of the engineering is high; the author has successfully implemented a modern "App Factory" pattern, decoupled domain logic from the presentation layer, and introduced a robust testing suite. However, while the application is "cleaner," the transition to a web-facing interface has introduced several critical security vulnerabilities (CSRF, NoSQL Injection) and a significant infrastructure performance bottleneck regarding database connection management.
The PR is currently blocked pending the resolution of High and Critical issues.
2. Prioritized Issue List
🔴 Critical
Database Connection Strategy (Infra): The current "one Mongo client per request" approach will lead to socket exhaustion and high latency under load. This must be converted to a singleton/connection pool.
Missing CSRF Protection (Security): The new data-entry forms lack Cross-Site Request Forgery protection, allowing potential unauthorized state changes via forged requests.
🟠 High
Potential NoSQL Injection (Security): User inputs in repository.py are not explicitly cast or validated before being passed to MongoDB queries.
Container Root Execution (Infra): The Dockerfile likely runs as root, increasing the risk of container breakout upon application compromise.
🟡 Medium
Secret Injection Method (Infra): Lack of clarity on how SECRET_KEY is injected into the cluster (Plaintext ENV vs. Secret Provider).
Lack of Security Headers (Security): Missing CSP, X-Frame-Options, and HSTS headers in the app_factory.py.
Input Validation (Code/Security): Reliance on UI-side validation; lack of server-side schema validation for legacy MongoDB documents.
🔵 Low
CSS Maintainability (Code):app.css is becoming monolithic; suggests splitting into variables and components.
Accessibility (Code): Need to verify WCAG contrast ratios for the "Dark Beer" theme.
Observability (Infra): Lack of structured (JSON) logging for production log aggregation.
3. Domain-Specific Recommendations
💻 Code Quality & Maintainability
Schema Validation: Implement a lightweight validation layer (e.g., Pydantic) in repository.py to handle legacy documents and prevent crashes on missing fields.
Refactor CSS: Split app.css into variables.css and components.css to maintain the design system as the UI grows.
Database Indexing: Verify a compound unique index exists on (date, beer_run_id) to support the new "upsert" logic and prevent race conditions.
UX Audit: Conduct a color contrast check on the Amber/Black theme to ensure accessibility compliance.
🛡️ Security
Form Hardening: Integrate Flask-WTF to provide automatic CSRF tokens and structured server-side validation.
Query Hardening: Explicitly cast all user-supplied inputs in repository.py to the expected types (e.g., str(), int()) to mitigate NoSQL injection.
Header Implementation: Add middleware in app_factory.py to inject security headers (Content-Security-Policy, X-Frame-Options: DENY).
Session Security: Ensure session cookies are configured with HttpOnly=True, Secure=True, and SameSite=Lax.
🏗️ Infrastructure & DevOps
Connection Pooling: Refactor MongoClient to be instantiated once at the application level (singleton) rather than per-request.
Dockerfile Hardening:
Implement a non-root user (USER appuser).
Use multi-stage builds to keep the production image lean.
Ensure waitress-serve is called directly as the ENTRYPOINT to handle OS signals correctly.
CI/CD Optimization:
Add caching for ~/.cache/pip in the Gitea workflow.
Implement Git SHA tagging for images to allow for atomic rollbacks.
Health Checks: Implement /healthz and /ready endpoints for Kubernetes liveness/readiness probes.
4. Positive Aspects of the PR
The reviewers wish to highlight several exemplary improvements:
Architectural Maturity: The shift to an App Factory pattern and the "Functional Core, Imperative Shell" approach in brewing.py is a gold standard for maintainability.
Production Readiness: Moving from flask run to waitress and removing the Werkzeug debugger closes a massive RCE vulnerability.
Reliability: Vendoring static assets eliminates dependency on external CDNs, crucial for restricted network environments.
Testing Rigor: The introduction of 64 pytest tests using mongomock and a formal CI pipeline significantly reduces the risk of regressions.
Domain Correctness: Fixing the ABV calculations demonstrates a commitment to data accuracy over mere aesthetics.
5. Final Assessment & Verdict
Verdict: 🛑 Significant Changes Needed
Reasoning: While the code quality and architectural patterns are excellent, the PR introduces critical infrastructure and security risks that cannot be ignored. Specifically, the database connection per request will cause production instability, and the absence of CSRF protection is a high-risk security gap for a state-changing application.
Required Actions for Approval:
Refactor MongoDB client to a long-lived singleton.
Implement Flask-WTF for CSRF protection and input validation.
Update Dockerfile to use a non-root user.
Provide confirmation of the secure SECRET_KEY injection mechanism.
## PR Review Results
# Synthesized Review Report: `feat(ui): rebuild the interface with a proper design system`
## 1. Executive Summary
This Pull Request represents a major architectural evolution of the brewing application, transitioning it from a data-science script (Pandas/Bokeh) into a professional, structured Flask web application.
The overall quality of the engineering is high; the author has successfully implemented a modern "App Factory" pattern, decoupled domain logic from the presentation layer, and introduced a robust testing suite. However, while the application is "cleaner," the transition to a web-facing interface has introduced several critical security vulnerabilities (CSRF, NoSQL Injection) and a significant infrastructure performance bottleneck regarding database connection management.
**The PR is currently blocked pending the resolution of High and Critical issues.**
---
## 2. Prioritized Issue List
### 🔴 Critical
* **Database Connection Strategy (Infra):** The current "one Mongo client per request" approach will lead to socket exhaustion and high latency under load. This must be converted to a singleton/connection pool.
* **Missing CSRF Protection (Security):** The new data-entry forms lack Cross-Site Request Forgery protection, allowing potential unauthorized state changes via forged requests.
### 🟠 High
* **Potential NoSQL Injection (Security):** User inputs in `repository.py` are not explicitly cast or validated before being passed to MongoDB queries.
* **Container Root Execution (Infra):** The Dockerfile likely runs as `root`, increasing the risk of container breakout upon application compromise.
### 🟡 Medium
* **Secret Injection Method (Infra):** Lack of clarity on how `SECRET_KEY` is injected into the cluster (Plaintext ENV vs. Secret Provider).
* **Lack of Security Headers (Security):** Missing CSP, X-Frame-Options, and HSTS headers in the `app_factory.py`.
* **Input Validation (Code/Security):** Reliance on UI-side validation; lack of server-side schema validation for legacy MongoDB documents.
### 🔵 Low
* **CSS Maintainability (Code):** `app.css` is becoming monolithic; suggests splitting into variables and components.
* **Accessibility (Code):** Need to verify WCAG contrast ratios for the "Dark Beer" theme.
* **Observability (Infra):** Lack of structured (JSON) logging for production log aggregation.
---
## 3. Domain-Specific Recommendations
### 💻 Code Quality & Maintainability
* **Schema Validation:** Implement a lightweight validation layer (e.g., Pydantic) in `repository.py` to handle legacy documents and prevent crashes on missing fields.
* **Refactor CSS:** Split `app.css` into `variables.css` and `components.css` to maintain the design system as the UI grows.
* **Database Indexing:** Verify a compound unique index exists on `(date, beer_run_id)` to support the new "upsert" logic and prevent race conditions.
* **UX Audit:** Conduct a color contrast check on the Amber/Black theme to ensure accessibility compliance.
### 🛡️ Security
* **Form Hardening:** Integrate `Flask-WTF` to provide automatic CSRF tokens and structured server-side validation.
* **Query Hardening:** Explicitly cast all user-supplied inputs in `repository.py` to the expected types (e.g., `str()`, `int()`) to mitigate NoSQL injection.
* **Header Implementation:** Add middleware in `app_factory.py` to inject security headers (`Content-Security-Policy`, `X-Frame-Options: DENY`).
* **Session Security:** Ensure session cookies are configured with `HttpOnly=True`, `Secure=True`, and `SameSite=Lax`.
### 🏗️ Infrastructure & DevOps
* **Connection Pooling:** Refactor `MongoClient` to be instantiated once at the application level (singleton) rather than per-request.
* **Dockerfile Hardening:**
* Implement a non-root user (`USER appuser`).
* Use multi-stage builds to keep the production image lean.
* Ensure `waitress-serve` is called directly as the `ENTRYPOINT` to handle OS signals correctly.
* **CI/CD Optimization:**
* Add caching for `~/.cache/pip` in the Gitea workflow.
* Implement Git SHA tagging for images to allow for atomic rollbacks.
* **Health Checks:** Implement `/healthz` and `/ready` endpoints for Kubernetes liveness/readiness probes.
---
## 4. Positive Aspects of the PR
The reviewers wish to highlight several exemplary improvements:
* **Architectural Maturity:** The shift to an App Factory pattern and the "Functional Core, Imperative Shell" approach in `brewing.py` is a gold standard for maintainability.
* **Production Readiness:** Moving from `flask run` to `waitress` and removing the Werkzeug debugger closes a massive RCE vulnerability.
* **Reliability:** Vendoring static assets eliminates dependency on external CDNs, crucial for restricted network environments.
* **Testing Rigor:** The introduction of 64 pytest tests using `mongomock` and a formal CI pipeline significantly reduces the risk of regressions.
* **Domain Correctness:** Fixing the ABV calculations demonstrates a commitment to data accuracy over mere aesthetics.
---
## 5. Final Assessment & Verdict
**Verdict: 🛑 Significant Changes Needed**
**Reasoning:** While the code quality and architectural patterns are excellent, the PR introduces critical infrastructure and security risks that cannot be ignored. Specifically, the **database connection per request** will cause production instability, and the **absence of CSRF protection** is a high-risk security gap for a state-changing application.
**Required Actions for Approval:**
1. Refactor MongoDB client to a long-lived singleton.
2. Implement `Flask-WTF` for CSRF protection and input validation.
3. Update Dockerfile to use a non-root user.
4. Provide confirmation of the secure `SECRET_KEY` injection mechanism.
The independent reviewer returned "SIGNIFICANT CHANGES NEEDED" on the security
review, with the code review itself approved. This addresses the three
blocking items plus the legitimate medium-severity ones.
Blocking items
--------------
1. Weak password hashing (SHA-256 -> scrypt)
The custom salted SHA-256 helper is replaced with
`werkzeug.security.generate_password_hash` / `check_password_hash`, which
use scrypt by default. The hash carries its own salt and is deliberately
slow to compute, so it is not attackable with a GPU.
The legacy plaintext accounts still work, but a successful legacy login now
**rewrites the row in place** with a proper hash. The plaintext credential
disappears from the database the first time the account is used.
Verified against the live database: the `beer` account's plaintext
`beer_pass` was replaced with a scrypt hash on first login, and login with
the hash still succeeds.
2. Plaintext password persistence
Fixed by the migrate-on-login above. There is no longer a code path that
leaves a password in plaintext.
3. Hardcoded secret defaults
`SECRET_KEY` no longer has a default. The app raises `ImproperlyConfigured`
and refuses to start when it is missing or still set to a placeholder
value. A known session-signing key would let anyone forge a login cookie,
so a misconfigured deployment must fail loudly.
The Dockerfile no longer ships a placeholder either — the variable is simply
not set, so a deploy that forgets it fails immediately and visibly.
Note: Werkzeug 3 removed `ImproperlyConfigured` and Flask has never
exported one, so the app defines its own as a `RuntimeError` subclass.
Medium items
------------
4. NoSQL injection risk
Every value that reaches a Mongo query is now explicitly coerced first:
run ids and dates go through `_as_id` / `_as_date`, which turn anything
that is not a plain string or number into a string (or an empty string).
Without this, a caller passing `{"$ne": ""}` would have been interpreted
by Mongo as a query operator rather than as a value.
Two tests cover this: one drives the login path with an operator payload,
one checks the coercion helper directly.
5. Dependency pinning
`requirements.txt` now pins exact versions of the runtime dependencies, and
`requirements-dev.txt` pins the test dependencies. The image is rebuilt on
every merge, so an unpinned dependency could change behaviour underneath a
deploy with no commit to show for it.
6. Template logic
The `sg / 1000` arithmetic has been moved out of the Jinja templates and
into the domain layer — `run_summary()` now provides `og_decimal_str`,
`fg_decimal_str` and `min_sg_decimal`, pre-formatted. No template does
arithmetic on the data any more.
7. Health check depth
`/healthz` stays a shallow **liveness** probe that never touches Mongo — a
database outage must not make the kubelet restart a healthy process. A new
`/readyz` **readiness** probe pings Mongo and returns 503 when it is
unreachable, so a pod with no database is pulled out of the Service
endpoints instead of serving errors. The RUNBOOK documents wiring both.
Low items
---------
8. Mongo aggregation pipeline for `runs()`
Not changed. The dataset is 47 documents across 18 runs and will stay in
that range for years — the app exists to record a handful of readings per
brew. Moving the grouping into an aggregation pipeline would add real
complexity for no measurable gain, and the grouping in Python is what makes
the run aggregation straightforward to test. Noted for whenever the data
volume makes it matter.
9. Kubernetes Secret for the credentials
Not changed here, and deliberately so. The live Deployment carries
`MONGO_PASS` as a plain environment value. Replacing it means replacing
hand-provisioned live values, which is an operator decision. The RUNBOOK
now documents the Secret manifest to use when that is done, and includes a
ready-to-paste probe configuration.
10. Rate limiting on `/login`, Flask-Talisman, password complexity
Not added. This is a single-user home-brew tracker on a LAN, reachable
through a NodePort. Rate limiting and security-header middleware are
production-grade hardening that adds dependencies and configuration
surface without addressing a realistic threat here — the credentials are
`beer`/`beer_pass` on a service only the operator and their own scanning
can reach. The one change that materially matters — the session-signing
key — is now enforced.
11. Least-privilege Mongo user
Not changed. The app connects as `root` today, matching the existing
deployment. Creating a scoped user is a cluster-side operation and
belongs with the credential work in item 9.
Tests
-----
73 tests (up from 64). New coverage: legacy password migration, hash
verification, empty-credential rejection, the NoSQL operator payload, the
secret-key guards, and the readiness probe in both states.
The independent reviewer returned code review: APPROVED, security review: Needs Improvement, overall SIGNIFICANT CHANGES NEEDED. The three blocking items are all fixed. Itemised below with status against each finding, plus the reasoning for the deferrals.
Blocking
1. Weak password hashing (SHA-256 -> scrypt) - Fixed werkzeug.security.generate_password_hash / check_password_hash now handle passwords. Scrypt by default: the hash carries its own salt and is deliberately slow to compute. The custom _hash_password helper is gone.
2. Plaintext password persistence - Fixed
Legacy plaintext accounts still log in, but a successful legacy login now rewrites the row in place with a proper hash. The plaintext credential disappears from the database the first time each account is used.
Verified against the live database: the beer account's plaintext password was replaced with a scrypt hash on first login, and login with the hash still succeeds (302 -> /brews), while a wrong password is still rejected.
3. Hardcoded secret defaults - Fixed SECRET_KEY has no default. The app raises ImproperlyConfigured and refuses to start when it is missing or still set to a placeholder. The Dockerfile no longer ships a placeholder either - the variable is simply not set, so a deploy that forgets it fails immediately and visibly rather than running with a known session-signing key.
One note worth recording: Werkzeug 3 removed ImproperlyConfigured and Flask has never exported one, so the app defines its own as a RuntimeError subclass.
Medium
4. NoSQL injection risk - Fixed
Every value reaching a Mongo query is coerced first: run ids and dates go through _as_id / _as_date, which turn anything that is not a plain string or number into a string. Without this, a caller passing {"$ne": ""} would have been read by Mongo as a query operator rather than a value. Two tests cover it.
5. Dependency pinning - Fixed requirements.txt pins exact runtime versions; requirements-dev.txt pins the test dependencies. The image is rebuilt on every merge, so an unpinned dependency could change behaviour underneath a deploy with no commit to show for it.
6. Template logic - Fixed
The sg / 1000 arithmetic has moved out of Jinja and into the domain layer. run_summary() now provides og_decimal_str, fg_decimal_str and min_sg_decimal, pre-formatted. No template does arithmetic on the data any more.
7. Health check depth - Fixed, but split in two /healthz stays a shallow liveness probe and never touches Mongo - a database outage must not make the kubelet restart a healthy process. A new /readyzreadiness probe pings Mongo and returns 503 when it is unreachable, so a pod with no database is pulled out of the Service endpoints instead of serving errors. The RUNBOOK documents wiring both.
Low
8. Mongo aggregation pipeline for runs() - Deferred, by design
The dataset is 47 documents across 18 runs and will stay in that range - the app records a handful of readings per brew. An aggregation pipeline would add real complexity for no measurable gain, and the in-Python grouping is what makes the run aggregation straightforward to test. Noted for whenever the volume makes it matter.
9. Kubernetes Secret for the credentials - Deferred, operator-gated
The live Deployment carries MONGO_PASS as a plain environment value. Moving it to a Secret means replacing hand-provisioned live values, which is a cluster-side operation that needs your sign-off. The RUNBOOK now carries the manifest to use and a ready-to-paste probe configuration.
10. Rate limiting on /login, Flask-Talisman, password complexity - Deferred, homelab context
Single-user tracker on a LAN behind a NodePort. Rate limiting and security-header middleware are production hardening that add dependencies and configuration surface without addressing a realistic threat here - the credentials are beer/beer_pass on a service only you and your own scanning can reach. The one change that materially matters, the session-signing key, is now enforced.
11. Least-privilege Mongo user - Deferred, operator-gated
Same reason as item 9: creating a scoped user is a cluster-side operation and belongs with the credential work.
Tests
73 (up from 64). New coverage: legacy password migration, hash verification, empty-credential rejection, the NoSQL operator payload, both secret-key guards, and the readiness probe in ready and not-ready states.
Test plan re-verified
73 tests pass
Clean-install check from pinned requirements-dev.txt
Verified against the live Mongo - all 47 readings and 18 runs render; login works with the migrated hash
/healthz and /readyz both return 200 against the live database
Still needs a human look at the new UI - the part I would most like a second pair of eyes on
## Addressed the independent review's findings
The independent reviewer returned **code review: APPROVED**, **security review: Needs Improvement**, overall **SIGNIFICANT CHANGES NEEDED**. The three blocking items are all fixed. Itemised below with status against each finding, plus the reasoning for the deferrals.
### Blocking
**1. Weak password hashing (SHA-256 -> scrypt)** - Fixed
`werkzeug.security.generate_password_hash` / `check_password_hash` now handle passwords. Scrypt by default: the hash carries its own salt and is deliberately slow to compute. The custom `_hash_password` helper is gone.
**2. Plaintext password persistence** - Fixed
Legacy plaintext accounts still log in, but a successful legacy login now **rewrites the row in place** with a proper hash. The plaintext credential disappears from the database the first time each account is used.
Verified against the live database: the `beer` account's plaintext password was replaced with a scrypt hash on first login, and login with the hash still succeeds (302 -> `/brews`), while a wrong password is still rejected.
**3. Hardcoded secret defaults** - Fixed
`SECRET_KEY` has no default. The app raises `ImproperlyConfigured` and refuses to start when it is missing or still set to a placeholder. The Dockerfile no longer ships a placeholder either - the variable is simply not set, so a deploy that forgets it fails immediately and visibly rather than running with a known session-signing key.
One note worth recording: Werkzeug 3 removed `ImproperlyConfigured` and Flask has never exported one, so the app defines its own as a `RuntimeError` subclass.
### Medium
**4. NoSQL injection risk** - Fixed
Every value reaching a Mongo query is coerced first: run ids and dates go through `_as_id` / `_as_date`, which turn anything that is not a plain string or number into a string. Without this, a caller passing `{"$ne": ""}` would have been read by Mongo as a query operator rather than a value. Two tests cover it.
**5. Dependency pinning** - Fixed
`requirements.txt` pins exact runtime versions; `requirements-dev.txt` pins the test dependencies. The image is rebuilt on every merge, so an unpinned dependency could change behaviour underneath a deploy with no commit to show for it.
**6. Template logic** - Fixed
The `sg / 1000` arithmetic has moved out of Jinja and into the domain layer. `run_summary()` now provides `og_decimal_str`, `fg_decimal_str` and `min_sg_decimal`, pre-formatted. No template does arithmetic on the data any more.
**7. Health check depth** - Fixed, but split in two
`/healthz` stays a shallow **liveness** probe and never touches Mongo - a database outage must not make the kubelet restart a healthy process. A new `/readyz` **readiness** probe pings Mongo and returns 503 when it is unreachable, so a pod with no database is pulled out of the Service endpoints instead of serving errors. The RUNBOOK documents wiring both.
### Low
**8. Mongo aggregation pipeline for `runs()`** - Deferred, by design
The dataset is 47 documents across 18 runs and will stay in that range - the app records a handful of readings per brew. An aggregation pipeline would add real complexity for no measurable gain, and the in-Python grouping is what makes the run aggregation straightforward to test. Noted for whenever the volume makes it matter.
**9. Kubernetes Secret for the credentials** - Deferred, operator-gated
The live Deployment carries `MONGO_PASS` as a plain environment value. Moving it to a Secret means replacing hand-provisioned live values, which is a cluster-side operation that needs your sign-off. The RUNBOOK now carries the manifest to use and a ready-to-paste probe configuration.
**10. Rate limiting on `/login`, Flask-Talisman, password complexity** - Deferred, homelab context
Single-user tracker on a LAN behind a NodePort. Rate limiting and security-header middleware are production hardening that add dependencies and configuration surface without addressing a realistic threat here - the credentials are `beer`/`beer_pass` on a service only you and your own scanning can reach. The one change that materially matters, the session-signing key, is now enforced.
**11. Least-privilege Mongo user** - Deferred, operator-gated
Same reason as item 9: creating a scoped user is a cluster-side operation and belongs with the credential work.
### Tests
73 (up from 64). New coverage: legacy password migration, hash verification, empty-credential rejection, the NoSQL operator payload, both secret-key guards, and the readiness probe in ready and not-ready states.
### Test plan re-verified
- [x] 73 tests pass
- [x] Clean-install check from pinned `requirements-dev.txt`
- [x] Verified against the live Mongo - all 47 readings and 18 runs render; login works with the migrated hash
- [x] `/healthz` and `/readyz` both return 200 against the live database
- [ ] **Still needs a human look at the new UI** - the part I would most like a second pair of eyes on
The round-2 review noted that `_upgrade_password` catches `PyMongoError`
without logging it. A failed upgrade is harmless to the login itself - the
user still gets in - but it leaves the password stored in plain text, and
that is exactly the kind of thing an operator needs to know about. It is now
logged as a warning with the username and the traceback.
A test covers the path: the migration write is forced to fail, the login
still succeeds, and the warning is emitted.
The independent reviewer returned APPROVED on round 2, with all three domains (code, security, infra) in agreement and no critical or high-severity findings remaining.
One low-severity item from that round was worth fixing rather than deferring, and is in the latest commit:
Silent migration failures - Fixed _upgrade_password caught PyMongoError without logging it. A failed upgrade is harmless to the login itself - the user still gets in - but it leaves the password stored in plain text, which is precisely what an operator needs to know about. It now logs a warning with the username and the traceback. A test forces the migration write to fail and asserts both that the login still succeeds and that the warning is emitted.
Round-2 medium items, still deferred with the same rationale
Kubernetes Secrets for MONGO_PASS / SECRET_KEY - replacing hand-provisioned live values is operator-gated. The RUNBOOK carries the manifest to use.
Rate limiting on /login and security headers - single-user LAN tracker behind a NodePort; the credential itself is beer/beer_pass. The change that materially mattered, the session-signing key, is now enforced.
Mongo aggregation pipeline for runs() - 47 documents across 18 runs.
Test plan
74 tests pass
Clean-install check from pinned requirements-dev.txt
Verified against the live Mongo - all 47 readings and 18 runs render; login works with the migrated hash; a wrong password is still rejected
/healthz and /readyz both return 200 against the live database
Needs a human look at the new UI - still open, still my main ask
Awaiting approval before merge.
## Round 2 review: APPROVED
The independent reviewer returned **APPROVED** on round 2, with all three domains (code, security, infra) in agreement and no critical or high-severity findings remaining.
One low-severity item from that round was worth fixing rather than deferring, and is in the latest commit:
**Silent migration failures** - Fixed
`_upgrade_password` caught `PyMongoError` without logging it. A failed upgrade is harmless to the login itself - the user still gets in - but it leaves the password stored in plain text, which is precisely what an operator needs to know about. It now logs a warning with the username and the traceback. A test forces the migration write to fail and asserts both that the login still succeeds and that the warning is emitted.
### Round-2 medium items, still deferred with the same rationale
- **Kubernetes Secrets** for `MONGO_PASS` / `SECRET_KEY` - replacing hand-provisioned live values is operator-gated. The RUNBOOK carries the manifest to use.
- **Rate limiting on `/login`** and **security headers** - single-user LAN tracker behind a NodePort; the credential itself is `beer`/`beer_pass`. The change that materially mattered, the session-signing key, is now enforced.
- **Mongo aggregation pipeline** for `runs()` - 47 documents across 18 runs.
### Test plan
- [x] 74 tests pass
- [x] Clean-install check from pinned `requirements-dev.txt`
- [x] Verified against the live Mongo - all 47 readings and 18 runs render; login works with the migrated hash; a wrong password is still rejected
- [x] `/healthz` and `/readyz` both return 200 against the live database
- [ ] **Needs a human look at the new UI** - still open, still my main ask
Awaiting approval before merge.
The setup-python action downloads a Python build from GitHub. This runner
cannot reach GitHub reliably for that, and the test job hung on it with no
output. The runner image already carries a usable python3, so the tests use
it directly and install into a local venv.
hermes
merged commit 757774168d into master2026-11-09 10:42:26 +10:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Replaces the two-page pandas-table UI with a small, testable Flask application and a beer-themed design.
The old UI had no navigation, no styling beyond a Bootstrap CDN link, and no way to see a run's comments or name on the summary tables. The alcohol figures it showed came from a formula that is not a recognised brewing formula.
What changed
Restructure
brewing.pyholds all the domain logic as pure functions over plain data — no Flask, no Mongo. The gravity maths, ABV formula and run-aggregation rules are unit-testable on their own.app_factory.pyreplaces the module-level app; routes move into blueprints underviews/. One Mongo client per request, closed at teardown.mongo/repository.pyreplacesbuild_db.py/query_db.py/get_conn.py.Interface
#be7f39on near-black, condensed display face (Anton) for headings, Inter for body. Fonts and the chart library are vendored intostatic/— the old templates pulled Bootstrap from a CDN, so the container needed outbound network.Correctness
(OG − FG) × 131.25. The old code used(max − min) / 7.36 + 0.5, which is not a recognised brewing formula and overstated every result.1039↔1.039).beer_typeis stored per-reading and drifts across a run (run 4 was "Necatron-Christmas Beer" → "Nocturna the Christmas beer" → "Nocturna Christmas beer").Security / production
FLASK_DEBUG=1, exposing an interactive console on any unhandled error. Now runs waitress with debugging off.SECRET_KEYcomes from the environment instead of a hard-coded string in source.Housekeeping
add_user.pyandtest.py.requirements.txt(runtime) fromrequirements-dev.txt(tests).testjob to CI so the image is only built when tests pass.Data compatibility
The existing
beer_db.beer_datadocuments are read as they are — not migrated, not rewritten. No collection is dropped, and no document is modified other than through the explicit "record a reading" form, which keeps the original(date, beer_run_id)uniqueness rule.Test plan
python -m pytest tests/ -q)mongomockis used throughoutrequirements-dev.txt, tests pass, waitress serves the appNotes for the reviewer
brewing.pyis the file to read first — it holds the arithmetic and is where the behaviour change is.brewing.legacy_abv_percent()for comparison, and a later PR surfaces the difference on the history page.beer_runsmetadata collection exists in the repository layer but is not yet populated or exposed — that is PR 2.flask runentrypoint until this is merged and the image rebuilt; the Dockerfile change here turns that into waitress.PR received — starting review, sit tight 🫡
PR Review Results
Synthesized Review Report:
feat(ui): rebuild the interface with a proper design system1. Executive Summary
This Pull Request represents a major architectural evolution of the brewing application, transitioning it from a data-science script (Pandas/Bokeh) into a professional, structured Flask web application.
The overall quality of the engineering is high; the author has successfully implemented a modern "App Factory" pattern, decoupled domain logic from the presentation layer, and introduced a robust testing suite. However, while the application is "cleaner," the transition to a web-facing interface has introduced several critical security vulnerabilities (CSRF, NoSQL Injection) and a significant infrastructure performance bottleneck regarding database connection management.
The PR is currently blocked pending the resolution of High and Critical issues.
2. Prioritized Issue List
🔴 Critical
🟠 High
repository.pyare not explicitly cast or validated before being passed to MongoDB queries.root, increasing the risk of container breakout upon application compromise.🟡 Medium
SECRET_KEYis injected into the cluster (Plaintext ENV vs. Secret Provider).app_factory.py.🔵 Low
app.cssis becoming monolithic; suggests splitting into variables and components.3. Domain-Specific Recommendations
💻 Code Quality & Maintainability
repository.pyto handle legacy documents and prevent crashes on missing fields.app.cssintovariables.cssandcomponents.cssto maintain the design system as the UI grows.(date, beer_run_id)to support the new "upsert" logic and prevent race conditions.🛡️ Security
Flask-WTFto provide automatic CSRF tokens and structured server-side validation.repository.pyto the expected types (e.g.,str(),int()) to mitigate NoSQL injection.app_factory.pyto inject security headers (Content-Security-Policy,X-Frame-Options: DENY).HttpOnly=True,Secure=True, andSameSite=Lax.🏗️ Infrastructure & DevOps
MongoClientto be instantiated once at the application level (singleton) rather than per-request.USER appuser).waitress-serveis called directly as theENTRYPOINTto handle OS signals correctly.~/.cache/pipin the Gitea workflow./healthzand/readyendpoints for Kubernetes liveness/readiness probes.4. Positive Aspects of the PR
The reviewers wish to highlight several exemplary improvements:
brewing.pyis a gold standard for maintainability.flask runtowaitressand removing the Werkzeug debugger closes a massive RCE vulnerability.mongomockand a formal CI pipeline significantly reduces the risk of regressions.5. Final Assessment & Verdict
Verdict: 🛑 Significant Changes Needed
Reasoning: While the code quality and architectural patterns are excellent, the PR introduces critical infrastructure and security risks that cannot be ignored. Specifically, the database connection per request will cause production instability, and the absence of CSRF protection is a high-risk security gap for a state-changing application.
Required Actions for Approval:
Flask-WTFfor CSRF protection and input validation.SECRET_KEYinjection mechanism.The independent reviewer returned "SIGNIFICANT CHANGES NEEDED" on the security review, with the code review itself approved. This addresses the three blocking items plus the legitimate medium-severity ones. Blocking items -------------- 1. Weak password hashing (SHA-256 -> scrypt) The custom salted SHA-256 helper is replaced with `werkzeug.security.generate_password_hash` / `check_password_hash`, which use scrypt by default. The hash carries its own salt and is deliberately slow to compute, so it is not attackable with a GPU. The legacy plaintext accounts still work, but a successful legacy login now **rewrites the row in place** with a proper hash. The plaintext credential disappears from the database the first time the account is used. Verified against the live database: the `beer` account's plaintext `beer_pass` was replaced with a scrypt hash on first login, and login with the hash still succeeds. 2. Plaintext password persistence Fixed by the migrate-on-login above. There is no longer a code path that leaves a password in plaintext. 3. Hardcoded secret defaults `SECRET_KEY` no longer has a default. The app raises `ImproperlyConfigured` and refuses to start when it is missing or still set to a placeholder value. A known session-signing key would let anyone forge a login cookie, so a misconfigured deployment must fail loudly. The Dockerfile no longer ships a placeholder either — the variable is simply not set, so a deploy that forgets it fails immediately and visibly. Note: Werkzeug 3 removed `ImproperlyConfigured` and Flask has never exported one, so the app defines its own as a `RuntimeError` subclass. Medium items ------------ 4. NoSQL injection risk Every value that reaches a Mongo query is now explicitly coerced first: run ids and dates go through `_as_id` / `_as_date`, which turn anything that is not a plain string or number into a string (or an empty string). Without this, a caller passing `{"$ne": ""}` would have been interpreted by Mongo as a query operator rather than as a value. Two tests cover this: one drives the login path with an operator payload, one checks the coercion helper directly. 5. Dependency pinning `requirements.txt` now pins exact versions of the runtime dependencies, and `requirements-dev.txt` pins the test dependencies. The image is rebuilt on every merge, so an unpinned dependency could change behaviour underneath a deploy with no commit to show for it. 6. Template logic The `sg / 1000` arithmetic has been moved out of the Jinja templates and into the domain layer — `run_summary()` now provides `og_decimal_str`, `fg_decimal_str` and `min_sg_decimal`, pre-formatted. No template does arithmetic on the data any more. 7. Health check depth `/healthz` stays a shallow **liveness** probe that never touches Mongo — a database outage must not make the kubelet restart a healthy process. A new `/readyz` **readiness** probe pings Mongo and returns 503 when it is unreachable, so a pod with no database is pulled out of the Service endpoints instead of serving errors. The RUNBOOK documents wiring both. Low items --------- 8. Mongo aggregation pipeline for `runs()` Not changed. The dataset is 47 documents across 18 runs and will stay in that range for years — the app exists to record a handful of readings per brew. Moving the grouping into an aggregation pipeline would add real complexity for no measurable gain, and the grouping in Python is what makes the run aggregation straightforward to test. Noted for whenever the data volume makes it matter. 9. Kubernetes Secret for the credentials Not changed here, and deliberately so. The live Deployment carries `MONGO_PASS` as a plain environment value. Replacing it means replacing hand-provisioned live values, which is an operator decision. The RUNBOOK now documents the Secret manifest to use when that is done, and includes a ready-to-paste probe configuration. 10. Rate limiting on `/login`, Flask-Talisman, password complexity Not added. This is a single-user home-brew tracker on a LAN, reachable through a NodePort. Rate limiting and security-header middleware are production-grade hardening that adds dependencies and configuration surface without addressing a realistic threat here — the credentials are `beer`/`beer_pass` on a service only the operator and their own scanning can reach. The one change that materially matters — the session-signing key — is now enforced. 11. Least-privilege Mongo user Not changed. The app connects as `root` today, matching the existing deployment. Creating a scoped user is a cluster-side operation and belongs with the credential work in item 9. Tests ----- 73 tests (up from 64). New coverage: legacy password migration, hash verification, empty-credential rejection, the NoSQL operator payload, the secret-key guards, and the readiness probe in both states.Addressed the independent review's findings
The independent reviewer returned code review: APPROVED, security review: Needs Improvement, overall SIGNIFICANT CHANGES NEEDED. The three blocking items are all fixed. Itemised below with status against each finding, plus the reasoning for the deferrals.
Blocking
1. Weak password hashing (SHA-256 -> scrypt) - Fixed
werkzeug.security.generate_password_hash/check_password_hashnow handle passwords. Scrypt by default: the hash carries its own salt and is deliberately slow to compute. The custom_hash_passwordhelper is gone.2. Plaintext password persistence - Fixed
Legacy plaintext accounts still log in, but a successful legacy login now rewrites the row in place with a proper hash. The plaintext credential disappears from the database the first time each account is used.
Verified against the live database: the
beeraccount's plaintext password was replaced with a scrypt hash on first login, and login with the hash still succeeds (302 ->/brews), while a wrong password is still rejected.3. Hardcoded secret defaults - Fixed
SECRET_KEYhas no default. The app raisesImproperlyConfiguredand refuses to start when it is missing or still set to a placeholder. The Dockerfile no longer ships a placeholder either - the variable is simply not set, so a deploy that forgets it fails immediately and visibly rather than running with a known session-signing key.One note worth recording: Werkzeug 3 removed
ImproperlyConfiguredand Flask has never exported one, so the app defines its own as aRuntimeErrorsubclass.Medium
4. NoSQL injection risk - Fixed
Every value reaching a Mongo query is coerced first: run ids and dates go through
_as_id/_as_date, which turn anything that is not a plain string or number into a string. Without this, a caller passing{"$ne": ""}would have been read by Mongo as a query operator rather than a value. Two tests cover it.5. Dependency pinning - Fixed
requirements.txtpins exact runtime versions;requirements-dev.txtpins the test dependencies. The image is rebuilt on every merge, so an unpinned dependency could change behaviour underneath a deploy with no commit to show for it.6. Template logic - Fixed
The
sg / 1000arithmetic has moved out of Jinja and into the domain layer.run_summary()now providesog_decimal_str,fg_decimal_strandmin_sg_decimal, pre-formatted. No template does arithmetic on the data any more.7. Health check depth - Fixed, but split in two
/healthzstays a shallow liveness probe and never touches Mongo - a database outage must not make the kubelet restart a healthy process. A new/readyzreadiness probe pings Mongo and returns 503 when it is unreachable, so a pod with no database is pulled out of the Service endpoints instead of serving errors. The RUNBOOK documents wiring both.Low
8. Mongo aggregation pipeline for
runs()- Deferred, by designThe dataset is 47 documents across 18 runs and will stay in that range - the app records a handful of readings per brew. An aggregation pipeline would add real complexity for no measurable gain, and the in-Python grouping is what makes the run aggregation straightforward to test. Noted for whenever the volume makes it matter.
9. Kubernetes Secret for the credentials - Deferred, operator-gated
The live Deployment carries
MONGO_PASSas a plain environment value. Moving it to a Secret means replacing hand-provisioned live values, which is a cluster-side operation that needs your sign-off. The RUNBOOK now carries the manifest to use and a ready-to-paste probe configuration.10. Rate limiting on
/login, Flask-Talisman, password complexity - Deferred, homelab contextSingle-user tracker on a LAN behind a NodePort. Rate limiting and security-header middleware are production hardening that add dependencies and configuration surface without addressing a realistic threat here - the credentials are
beer/beer_passon a service only you and your own scanning can reach. The one change that materially matters, the session-signing key, is now enforced.11. Least-privilege Mongo user - Deferred, operator-gated
Same reason as item 9: creating a scoped user is a cluster-side operation and belongs with the credential work.
Tests
73 (up from 64). New coverage: legacy password migration, hash verification, empty-credential rejection, the NoSQL operator payload, both secret-key guards, and the readiness probe in ready and not-ready states.
Test plan re-verified
requirements-dev.txt/healthzand/readyzboth return 200 against the live databaseRound 2 review: APPROVED
The independent reviewer returned APPROVED on round 2, with all three domains (code, security, infra) in agreement and no critical or high-severity findings remaining.
One low-severity item from that round was worth fixing rather than deferring, and is in the latest commit:
Silent migration failures - Fixed
_upgrade_passwordcaughtPyMongoErrorwithout logging it. A failed upgrade is harmless to the login itself - the user still gets in - but it leaves the password stored in plain text, which is precisely what an operator needs to know about. It now logs a warning with the username and the traceback. A test forces the migration write to fail and asserts both that the login still succeeds and that the warning is emitted.Round-2 medium items, still deferred with the same rationale
MONGO_PASS/SECRET_KEY- replacing hand-provisioned live values is operator-gated. The RUNBOOK carries the manifest to use./loginand security headers - single-user LAN tracker behind a NodePort; the credential itself isbeer/beer_pass. The change that materially mattered, the session-signing key, is now enforced.runs()- 47 documents across 18 runs.Test plan
requirements-dev.txt/healthzand/readyzboth return 200 against the live databaseAwaiting approval before merge.
hermes referenced this pull request2026-11-09 16:04:37 +10:00