Pentest fixes and hardening#321
Open
level09 wants to merge 90 commits into
Open
Conversation
Satisfies SLSA v1.2 Source track requirement for a documented Safe Expunging Process. Covers scope, permitted reasons, two-maintainer approval, procedure, and consumer notification.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bitnami removed bitnami/nginx:1.24 from Docker Hub in August 2025, breaking compose builds. Point to the bitnamilegacy mirror that Bitnami publishes for deprecated tags.
The celery-ocr service defined in docker-compose.yml builds with ROLE=celery-ocr, but flask/Dockerfile only handled 'flask' and 'celery' branches, so the image had no Python runtime and the container exited 127 with 'celery: not found'. Share the celery install path with celery-ocr, which needs the same deps.
The compose file defaulted ENV_FILE to .env, which is the local development file where POSTGRES_HOST=localhost. That mount made the bayanat container try to reach postgres via a local socket instead of the compose service. .env.docker already ships with the correct service hostnames (postgres, redis) and is the intended default for this compose file.
flask create-db builds the full schema from models, so running db upgrade after it conflicts on indexes the latest migrations try to create. Detect fresh vs existing DB via flask db current.
Swap ubuntu:24.04 for python:3.12-slim-bookworm in the runtime stage and use the official ghcr.io/astral-sh/uv image as the builder. Keeps the ROLE-based conditional install for celery-only deps (exiftool, ffmpeg) and drops incidental ubuntu bulk that is not actually used by the app. - builder stage installs only build headers; runtime stage installs only shared libraries (libpq5, pango, cairo, harfbuzz, libxml2, libxslt, libjpeg62-turbo, libopenjp2, libffi8, dejavu fonts) - libimage-exiftool-perl is kept in the builder because pyexifinfo's setup.py probes for the exiftool binary during wheel install - XDG_CACHE_HOME=/tmp/.cache silences fontconfig cache warnings during weasyprint PDF generation Verified end-to-end against a fresh compose stack: weasyprint PDF gen, psycopg2, pillow, lxml, exiftool, ffmpeg, nginx->uwsgi->Flask login all work. Image sizes drop ~160MB (bayanat) and ~210MB (celery).
The previous `service nginx status` check failed on the bitnami nginx image, which has no sysvinit, so the container always reported unhealthy even when nginx was serving correctly. Switch to a bash TCP probe against port 80 — no external tools required (curl and wget are not present in the image).
## Summary - Installer's systemd unit for `bayanat-celery` only subscribed to the default `celery` queue, but `ocr_single` tasks route to a dedicated `ocr` queue (`enferno/tasks/__init__.py`). - Any bulk OCR dispatched via the admin UI (`POST /admin/api/ocr/bulk`) or the `flask ocr process` CLI piled up in Redis with no consumer. - Single-media OCR (per-item UI button / `flask ocr extract`) was unaffected — it takes a sync path through `process_media_extraction_task()`. ## Fix Add `-Q celery,ocr` to the `ExecStart` line so the worker consumes both queues. ## Affected versions - v4.0.0 (GA install command ships the broken unit). ## Upgrade path ### Fresh install Nothing special — new installs get the fixed unit. ### Existing v4.0.0 installs (in-place patch, no reinstall needed) ``` sudo sed -i.bak 's|worker --autoscale 2,5 -B$|& -Q celery,ocr|' /etc/systemd/system/bayanat-celery.service sudo systemctl daemon-reload sudo systemctl restart bayanat-celery ``` Verify with `sudo journalctl -u bayanat-celery --since "30 seconds ago" | grep queues` — should list both `celery` and `ocr`. ## Test plan - [x] Reproduced on auto-update-simplified deployment: 6 tasks stuck in `ocr` queue with 0 processed, worker idle. - [x] Applied the same fix to prod2 manually: queue drains, `ocr_single` tasks processed end-to-end via Google Vision API. - [x] All 4 test media processed successfully (confidence 24-98%, zero failures). - [ ] Tag `v4.0.1` after merge and update docs installer URL to pin v4.0.1.
Bulletin/actor/incident history routes only checked the global view_*_history permission, never the per-item access of the parent record. A user with history-view permission but no group access to a restricted item could read its full revision payload via the history API. Resolve the parent entity and gate on current_user.can_access(parent) before the history query. Log denied attempts to Activity. Location history is unaffected (Location has no role-based access).
The PUT /api/extraction/<id> endpoint resolved the Extraction row directly by ID without re-checking the parent Media's group membership, mirroring the GET sibling. Any DA/Admin could overwrite text/status on extractions in groups they don't belong to, and the success response leaked the full text+history payload back. Resolve the parent Media, gate on current_user.can_access(media), and trim the success response to to_compact_dict so even authorised calls don't echo the full text/history block.
api_csv_analyze, api_xls_sheet and api_xls_analyze concatenated the caller-supplied filename onto IMPORT_DIR with no containment check, so '../../../../app/.env' resolved outside the import directory and the resulting file content was returned as parsed CSV. With Admin credentials this leaked SECRET_KEY, SECURITY_TOTP_SECRETS and SECURITY_PASSWORD_SALT. Add _resolve_import_path() that joins via werkzeug.safe_join, resolves symlinks, and asserts the candidate is inside IMPORT_DIR. Reject with 400 on traversal or missing filename, with a warning log.
The /login endpoint had no server-side throttle: Flask-Limiter was only attached to /csrf, the session-scoped failure counter is reset by starting a new session, and reCAPTCHA is off by default. An attacker could issue an unbounded number of POSTs against /login. Add a Redis-backed throttle keyed independently on (username) and (ip) with a 15-minute sliding window: 10 failures per username, 30 per IP. Throttle check runs in before_app_request for POST /login and returns 429 once either ceiling is hit. Counters increment in after_app_request on failed login and clear on success. Failed attempts are logged via the regular logger; reCAPTCHA stays as an optional secondary friction layer.
Interactive CRUD runs Bulletin.description / ActorProfile.description through SanitizedField, but the import helpers wrote raw, untrusted strings straight to those columns. Both fields are rendered with v-html in BulletinCard and ActorProfiles, so an attacker-controlled CSV cell or external metadata payload could carry stored XSS that fires when an analyst opens the record. Wrap the import-side writes in sanitize_string() so the same bleach allowlist used by SanitizedField applies before persistence. Covers sheet_import.set_description (actor profile), and the three bulletin.description sinks in media_import (update_description, video info description, text_content).
One test file covering 001 / 002 / 004 / 007 / 008. Each test mirrors the auditor's PoC payload so a future regression flips the test. Mix of e2e (history endpoint, extraction PUT, traversal endpoints) and unit (login throttle helpers, sanitize_string) depending on what was practical to wire through fixtures. 7ASec retest map: run 'uv run pytest tests/test_pentest_fixes.py -v'.
The previous fix reinvented sliding-window rate limiting on top of raw Redis incr/expire. Replace it with two stacked Flask-Limiter decorators applied to security.login view post-registration: one keyed by username, one by IP. Storage, TTL, sliding window, 429 response and X-RateLimit-* headers all come from the existing limiter setup. Limits live in settings.py (LOGIN_RATE_LIMIT_PER_USERNAME and LOGIN_RATE_LIMIT_PER_IP), env-overridable, no UI exposure. TestConfig uses tighter values so the e2e test trips quickly. Drops the custom helpers in rate_limit_utils.py, the throttle code in user/views.py, the _FakeRedis test class, three throttle helper unit tests, and the weak resolver unit test. Adds one e2e test that posts bad credentials repeatedly until 429. Net: -94 lines, leaning on a battle-tested library.
The installer created the bayanat role with createuser -s (full superuser) and inserted 'local all bayanat trust' into pg_hba.conf. Any local OS user could then open a socket connection and claim the bayanat role with no password and full superuser privileges. Drop -s on createuser so the role is a plain owner. Create pg_trgm and postgis as the postgres superuser at install time so the app role never needs CREATE EXTENSION privilege. Replace the 'trust' pg_hba rule with 'peer' so only processes running as the bayanat OS user can authenticate as the bayanat PG role. Idempotent for upgrades: existing installs get ALTER USER ... NOSUPERUSER and the trust rule is removed before the peer rule is inserted.
Celery export tasks (PDF / JSON / CSV / media) iterated the caller-supplied IDs and serialised every row, with no per-record group check. A user with can_export but no group access could submit restricted IDs in an export request; once an admin approved it (the approval UI gives no signal that items cross access boundaries), the requester received the full payload. Add _accessible_items helper that yields only rows the export requester is authorised to see (mirrors current_user.can_access on the direct GET path) and route all three generator tasks plus the media-attachment task through it. CSV path uses an inline check because each iteration mutates a DataFrame. Restricted items get a warning log; missing requester yields nothing (fail closed). UI surface (admin approval warning when items are out-of-group) deferred to a follow-up; this commit closes the data leak.
…r CLI The /api/create-admin route was reachable over the network without authentication during the install window. The supported quick-install script started uWSGI + Caddy before the operator visited the wizard, so the first network client could claim a fully privileged admin account. The same hole reopened any time the admin role had zero users. Drop the route entirely (and its sibling /api/check-admin). The installer now provisions the admin out-of-band: it generates a high-entropy password and runs 'flask install --username admin --password ...' before the service is exposed, then prints the credentials once for the operator. The setup wizard now requires an authenticated Admin session and starts from the language step. flask install gains --username and --password options for non-interactive use; if username is given without a password it generates and prints one. Drive-by: pre-commit ruff dropped 4 pre-existing unused imports (Bulletin, DynamicField, DateHelper, celery_app) and one redundant f-string in commands.py.
The credentials banner already prints username + password, but the operator still has to construct the login URL themselves. Add the fully-qualified /login URL to the banner and explain what the wizard covers after sign-in, so the post-install handoff is one copy + one click.
The native installer (bayanat script) provisions the initial admin via flask install before the service is reachable. The Docker entrypoint was not aligned: it ran create-db on a fresh volume but never created an admin, leaving Pattern A's network surface (wizard requires auth, no admin = no login) effectively unusable for compose users. Add a flask install --username admin call after schema creation. With no --password supplied the command generates a random one and prints the credentials banner to stdout, which docker-compose logs flask captures so the operator can retrieve it after first start.
Two issues from review: - The entrypoint comment referenced 'docker-compose logs flask' but the service in docker-compose.yml is named bayanat. - The deployment docs still told operators to run 'flask install' after startup, which now silently no-ops because the entrypoint bootstrapped the admin already, leaving them without credentials. Fix the comment and rewrite the docs: the first-run password is in 'docker-compose logs bayanat | grep -A4 "Generated password"'; the CLI fallback only mints a fresh password if no admin exists.
Reviewer flagged that the native installer's _bootstrap_admin generated
the password in shell and passed it via 'flask install --password "$pw"',
exposing it through /proc/<pid>/cmdline and 'ps' for the lifetime of
the child.
Add a --password-stdin flag to the flask install CLI (mirroring the
docker login --password-stdin pattern). The installer now feeds the
password over a pipe:
printf '%s\n' "$pw" | flask install --username admin --password-stdin
The password never appears in argv or environ. The Docker entrypoint
already used the safer shape (flask install --username admin generates
and prints) and is unchanged.
f-string substitution into postgresql:// and redis:// URLs broke when passwords contained URL-special characters (/, @, #, +, =). Surfaced during Docker bootstrap testing: a base64 password with '/' caused 'Port could not be cast to integer' at app boot. quote(safe='') around POSTGRES_USER/POSTGRES_PASSWORD/REDIS_PASSWORD fixes Postgres URI plus all four Redis URLs (broker, result, session, cache). Verified roundtrip via redis.from_url decode.
## Description Brings back the native PDF viewer (iframe) for normal viewing and keeps the canvas viewer for OCR. Default: native viewer (text selection, search, zoom, etc.) OCR dialog: still uses canvas renderer No changes to OCR pipeline ## How to Test Open a PDF → verify it uses the browser native pdf viewer with their own tools (marker, zoom, etc) Open OCR dialog → verify pdf canvas viewer + OCR still work Check fullscreen works Check images and DOCX still work ## Jira ID (if applicable) [BYNT-1679](https://syriajustice.atlassian.net/browse/BYNT-1679) [BYNT-1679]: https://syriajustice.atlassian.net/browse/BYNT-1679?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Nidal Alhariri <level09@gmail.com>
## Summary Patch release addressing 11 open Dependabot advisories in `uv.lock` (high + medium severity). All fixes are minor/patch-level dependency bumps. No code changes. | Package | From | To | Severity | Advisory | |---|---|---|---|---| | lxml | 6.0.2 | 6.1.0 | **high** | [GHSA-pp7h-53gx-mx7r](GHSA-pp7h-53gx-mx7r) — XXE in iterparse/ETCompatXMLParser | | pillow | 12.1.1 | 12.2.0 | **high** | [GHSA-2vfv-wwj6-7q47](https://github.com/advisories/GHSA-2vfv-wwj6-7q47) — FITS GZIP decompression bomb | | pypdf | 6.10.0 | 6.10.2 | medium | 3 advisories — RAM exhaustion via crafted PDFs | | python-dotenv | 1.2.1 | 1.2.2 | medium | symlink-following in set_key | | Mako | 1.3.10 | 1.3.11 | medium | path traversal in TemplateLookup | | pytest (dev) | 9.0.2 | 9.0.3 | medium | vulnerable tmpdir handling | `pip <= 26.0.1` (alert #159) is also flagged but no patched version is available yet upstream — left for a follow-up. Frontend advisories (`dompurify`, `vite`, `uuid` in `docs/package-lock.json`) are scoped to the docs site and tracked separately. ## Test plan - [x] `uv sync --extra dev` clean - [x] `uv run pytest -q` — **773 passed, 4 skipped** in 29.5 s - [ ] After merge, bump `production` and tag v4.0.2
Contributor
|
BAY-01-015 looks fixed. I tested by changing the export ID in the URL/API to another user’s export. The server returned 403 Forbidden, so the export metadata is no longer exposed. Minor note: 403 vs 404 can still hint that an export ID exists, but that is low impact and not the main issue. Unrelated UX issues noticed while testing:
|
Bulletin/Actor to_dict() embedded full media metadata (filename, etag, title, OCR extraction text) in the medias array for any user who could read the parent, even those blocked from the media endpoints by _require_media_access. A non-Admin without can_access_media therefore still received the data the direct endpoints deny them. Gate the medias loop on a shared can_view_media() helper that mirrors the _require_media_access policy (Admin or can_access_media) and trusts CLI/Celery via has_request_context, matching check_history_access.
The single-OCR endpoint requires can_edit (assignment boundary) since OCR writes an extraction, but /api/ocr/bulk scoped items only by can_access (visibility), letting a non-Admin with can_access_media OCR-mutate media they can read but are not assigned to edit. Post-filter the resolved media ids by can_edit for non-Admins. The query-level access filter can't express can_edit (needs the parent's assignment + status), so it is done in Python; Admins skip the filter.
Contributor
|
BAY-01-018 looks fixed for the main reported issue. I tested a bulk Bulletin update as a Mod with:
Only Bulletin 12 was updated. Activity Monitor only showed Bulletin 12, and only Bulletin 12 got a new revision history entry. Separate issue found while testing Incident bulk updates: Test:
Result: So the original restricted-ID logging issue seems fixed, but assignRelated can still create duplicate history entries for related Bulletins. |
Contributor
|
BAY-01-022 looks fixed for the normal Bulletin update API, but I found a remaining bulk-update edge case. Test:
So the normal edit path is protected, but bulk update can still mutate peer-review-locked Bulletins. |
## Summary
Web/upload imports could silently get stuck in **Pending** with no error
log on the import row, even though the celery worker logged a traceback.
We hit this in production when Postgres dropped an idle connection:
`etl_process_file` raised `PendingRollbackError`, the original exception
was masked, and the import row was never marked Failed.
Two fixes:
1. **Defensive error handling in `etl_process_file`**
(`enferno/tasks/data_import.py`)
- Call `db.session.rollback()` before reusing the session in the except
branch. Otherwise a poisoned session causes the fail-marker query to
raise, masking the real exception and leaving the row stuck in Pending.
- Guard the fail-marker itself so a secondary failure logs and doesn't
replace the original exception.
2. **Connection health checks on the SQLAlchemy engine**
(`enferno/settings.py`)
- `pool_pre_ping=True`: ping each pooled connection before use, so
workers don't get handed a dropped connection.
- `pool_recycle=300` (overridable via `SQLALCHEMY_POOL_RECYCLE`): cycle
connections every 5 min, well under typical NAT/firewall idle timeouts.
Together: the first fix makes the symptom impossible regardless of root
cause; the second stops the underlying class of error from happening in
the first place.
## Observed in production
```
PendingRollbackError: Can't reconnect until invalid transaction is rolled back.
File "enferno/tasks/data_import.py", line 33, in etl_process_file
log = db.session.get(DataImport, data_import_id)
```
The line number points inside the except clause; the original exception
inside `MediaImport(...)` / `di.process(file)` was lost. We also saw
`OperationalError: server closed the connection unexpectedly` on the
same worker pool earlier the same day, confirming the connection-drop
pattern.
## Test plan
- [ ] Run existing test suite (`uv run pytest`) on a Postgres with a low
idle timeout and confirm no regressions.
- [ ] Trigger a web import and verify normal happy path still produces a
bulletin and `Ready` status.
- [ ] Force a transient DB error during `etl_process_file` (e.g. kill
the worker's connection) and confirm the import row ends in `Failed`
with the original exception in the log instead of `Pending`.
## Summary - `PERMANENT_SESSION_LIFETIME` is currently hardcoded to 3600s. This makes it configurable via the `SESSION_LIFETIME` environment variable, defaulting to 3600 (no behavior change for existing installs). - Lets operators tune the idle session timeout (Flask refreshes the cookie/Redis TTL on every request, so this effectively controls idle expiry) without code changes. ## Test plan - [ ] Unset `SESSION_LIFETIME` → sessions expire after 3600s of inactivity (unchanged default). - [x] Set `SESSION_LIFETIME=300` in `.env` → sessions expire after 5 min of inactivity. - [x] Service starts cleanly with the env var present and absent.
## Summary PR #312 replaced the `request.referrer` check on `/admin/api/media/chunk` with `request.form.get(\"source\") == \"import\"`. Dropzone drops its `params` option on chunked POSTs, so the form field is missing on every chunk request, `import_upload` is always `False`, and in S3 mode the chunk endpoint takes the **S3 push + local delete** branch. The import ETL then runs and finds no local file: ``` ValueError: The filename given was either non existent or was a directory File \"enferno/data_import/utils/media_import.py\", line 447, in upload_import info = exiflib.get_json(filepath)[0] ``` Every import in S3 mode has been failing since #312 merged. ## Fix Move `source=import` from Dropzone's `params` to the URL query string and check `request.args` instead of `request.form`. Query params live on the URL and ride along on every chunk POST regardless of how Dropzone shapes the body. Same security posture as the form-body check — both come from the client. The actual access control is the `current_user.has_role(\"Admin\")` gate three lines down, which is unchanged. ## Test plan - [ ] Upload a video via the media import tool with `FILESYSTEM_LOCAL=False`. Confirm the file lands in `enferno/media/` locally for ETL, then ends up in S3 after `MediaImport.upload()`. - [ ] Upload a video via the media import tool with `FILESYSTEM_LOCAL=True`. Confirm normal local-only behavior still works. - [ ] Confirm normal (non-import) Dropzone uploads under Bulletin/Actor still go straight to S3 in S3 mode. - [ ] Confirm a non-admin user can't bypass the extension check by hitting `/admin/api/media/chunk?source=import` directly (admin role gate still applies).
# Conflicts: # enferno/commands.py # enferno/settings.py # uv.lock
## Summary In-app visual redaction for document and image media. An analyst draws boxes over a PDF or scan, Bayanat burns them in permanently and saves a clean redacted copy as a **new** Media row. The original is never modified. ## How it works - **Engine** (`enferno/utils/redaction_utils.py`): PDFs via PyMuPDF `add_redact_annot` + `apply_redactions` + `scrub` (destructive: content removed, not overlaid); images via Pillow flatten to JPEG (EXIF dropped). - **Endpoint** `POST /api/media/<id>/redact` (Admin/DA): reads source bytes, burns, writes a new file, creates the Media row plus a `media_redaction` audit row (source, regions, user), logs an Activity. Existing redactions can also be deleted (#354). - **Available on bulletins and actors**: redact icon on the media card next to the OCR icon. - **Many copies of one doc**: redacted copies are grouped by their source media id and collapsed behind a "Show N redactions" toggle, so a document redacted once per detainee does not clutter the record. Optional per-copy label keeps them findable; copies can be edited in place. - **Migrations**: `e4f2a9c8d7b1` (additive `media_redaction` table) and `68396035f041` (`original_media_id` link). Both additive with clean downgrades, no data risk. - **Docs**: Redaction guide included. ## Status Ready for review and merge. UX has been finalized with the team and the flow has been validated on staging (sta.bayanat.org). ## Review focus - Irreversibility: PDF text/content actually gone under boxes; scanned image-only pages blanked per region (not the whole page); image EXIF dropped. - Original Media row/file immutability (redaction always produces a new row). - Access control on the endpoint (`@roles_accepted("Admin","DA")` + `can_access`). --------- Co-authored-by: Daniel Apodaca <apodacaduron@gmail.com> Co-authored-by: Daniel Apodaca <47479471+apodacaduron@users.noreply.github.com>
apodacaduron
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes for the 7ASec BAY-01 engagement, on top of baseline
8615ff11b. One commit per finding, taggedfix(BAY-01-NNN):so the retest maps fixes bygit log --grep BAY-01 pentest-fixes.42 of 43 fixed. Open:
BAY-01-017(release signing), a product decision, not a code gap.Finding to commit
017 is unaddressed pending the signing decision. minisign keypair exists, public key pinned; manual
sudo bayanat updatestays as the fallback.Partials worth flagging
020: filename enumeration closed (opaque names). Per-item auth on/api/serve/inline/<filename>is a separate parent-binding change, deferred.028issue 6: passwords are out of argv (in-container config file +REDISCLI_AUTH). Delivery stays via env vars because Compose can't mount env-sourced secrets intoread_onlycontainers.031: app DB role stripped of superuser/createdb/createrole/replication and CONNECT revoked from PUBLIC, but keeps table ownership because dynamic-fields runsALTER TABLEat runtime.Retest
Backend regression tests:
uv run pytest tests/test_pentest_fixes.py. No frontend harness in this repo; 034 verified by review. Each commit message has root cause and where to verify. Fresh install recommended (005 changed the install flow: the installer now generates and prints the admin password once).