Skip to content

Security + performance hardening from production/enterprise audit#70

Closed
ibuilder wants to merge 7 commits into
mainfrom
chore/audit-hardening
Closed

Security + performance hardening from production/enterprise audit#70
ibuilder wants to merge 7 commits into
mainfrom
chore/audit-hardening

Conversation

@ibuilder

Copy link
Copy Markdown
Owner

Implements the safe, low-risk hardening items from the production/enterprise audit. All changes are source-local; no architectural rewrites. Each item cites file:line evidence from the audit.

Performance

  • nginx precompressed assets + API JSON compression (`apps/web/nginx.conf`): enable `gzip_static on;` to serve the Vite-emitted `.gz` siblings directly (incl. the ~7 MB `thatopen` chunk) with zero runtime CPU, plus `gzip on;` so proxied `/api/` JSON is compressed. `brotli_static` intentionally omitted — stock `nginx:alpine` has no `ngx_brotli` module; `.br` siblings still ship for a future brotli-enabled image.
  • Subset-IFC export temp cleanup (`bim.py`): attach `BackgroundTask(os.unlink, out)` to the `FileResponse` so throwaway `subset-*.ifc` no longer leaks in `/tmp`.
  • Blocking IO off the event loop (`bim.py`): wrap `storage.put` (add_attachment), `bundle_io.import_bundle`, `bcf_io.import_bcfzip`, and `clash_import.import_clash_xlsx/xml` in `run_in_threadpool` (mirrors the existing `authoring.py` pattern). Stops MinIO/zip/XML work from stalling uvicorn workers.
  • Bounded background-publish concurrency (`authoring.py`): replace per-request `threading.Thread` with a module-level `ThreadPoolExecutor(max_workers=AEC_PUBLISH_WORKERS)` (default 2). All 13 publish call sites funnel through `_publish_bg`, so one pool governs the whole app; excess publishes queue instead of exhausting CPU/RAM.

Security

  • RVT upload path traversal (`convert.py:63-65`, MEDIUM): sanitize `UploadFile.filename` with `basename` + regex before joining into the temp dir. Pre-existing, gated behind auth+APS.
  • Hide APS subprocess stderr (`convert.py:72-73`): log full stderr server-side, return a generic 502 to the caller.
  • Content-Disposition header injection (`serving.py` + `convert.py` + `bim.py`): new shared `content_disposition()` helper strips path + CR/LF, quotes an ASCII fallback, and adds an RFC 5987 `filename*=UTF-8''` form for non-ASCII names. Applied to `.frag`/`.xyz` responses and all `range_response()` downloads.
  • BCF decompression-bomb guard (`bcf_io.py`): new `_open_bcfzip()` checks each `ZipInfo.file_size` and the cumulative total against `AEC_MAX_UNZIP_MB` (default 512), raising 413 on oversized archives and 422 on bad zips. `defusedxml` parsing untouched.

Tests

  • `test_serving.py` — `content_disposition()` path traversal / CRLF / non-ASCII / blank cases.
  • `test_bcf.py` — decompression-bomb rejection (both `import_bcfzip` and `parse_records_bcfzip`).
  • `test_subset_export.py` — asserts no `subset-*.ifc` remains after the response.
  • 16 related backend suites green; `npm run build -w apps/web` green with `.gz`/`.br` siblings confirmed. ruff clean on changed files.

Deferred (separate PRs)

`/metrics` auth (breaks Prometheus scrapers; needs opt-in path), compose CPU/mem limits (OOM risk for legitimate conversion), Dockerfile digest pinning (needs exact platform digests), `npm audit fix` dev/docs chain (lockfile churn), and the architectural items (Alembic migrations, OpenTelemetry tracing, Sentry alerting, on-demand rendering, warm converter worker, chunk splitting, point-cloud worker, streaming uploads/downloads, batch portfolio aggregates, and product gaps: clash detection, IFC version-compare, public API, SOC 2).

```task
file:apps/web/nginx.conf
file:services/api/src/aec_api/bcf_io.py
file:services/api/src/aec_api/routers/authoring.py
file:services/api/src/aec_api/routers/bim.py
file:services/api/src/aec_api/routers/convert.py
file:services/api/src/aec_api/serving.py
```

ibuilder and others added 7 commits July 22, 2026 15:05
The Vite build already emits .gz/.br siblings but nginx served neither and had
no gzip on, so the ~7 MB thatopen chunk shipped raw. Enable gzip_static (serves
the precompressed .gz for hashed JS/CSS/wasm) and gzip on with gzip_types so
proxied /api/ JSON is compressed too. brotli_static is intentionally omitted:
the stock nginx:alpine image has no ngx_brotli module.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add a shared content_disposition() helper that strips path components and
CR/LF, emits a quoted ASCII fallback plus an RFC 5987 filename*=UTF-8'' form
for non-ASCII names, and route range_response() (attachment + model downloads)
through it. A caller-controlled filename can no longer inject header fields or
crash latin-1 header encoding.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Sanitize the attacker-controlled UploadFile.filename before joining it to the
  temp dir so a crafted name can't traverse outside it (path traversal).
- Log the converter subprocess stderr server-side and return a generic 502 so
  internal APS/converter detail no longer leaks to the caller.
- Build the .frag / .xyz download Content-Disposition via the shared helper.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…vent loop

- Attach a BackgroundTask(os.unlink) to the subset.ifc FileResponse so the
  server-chosen throwaway file no longer leaks in /tmp.
- Wrap the blocking storage.put / zip+XML importers (add_attachment, bcf_import,
  coordination import xlsx/xml, import_bundle) in run_in_threadpool, mirroring
  the existing pattern in authoring.py so a big upload can't stall the loop.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The raw upload is capped by AEC_MAX_UPLOAD_MB, but a tiny .bcfzip can still
expand to gigabytes in RAM. Add _open_bcfzip() to check each entry's declared
uncompressed size and the cumulative total against AEC_MAX_UNZIP_MB (default
512), rejecting oversized archives with 413. defusedxml parsing is unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Every publish spawned a raw daemon Thread, so a burst of uploads could launch
N heavy ifcopenshell converts at once and exhaust CPU/RAM. Submit run_publish
to a module-level ThreadPoolExecutor sized by AEC_PUBLISH_WORKERS (default 2);
excess publishes queue. All 13 call sites funnel through _publish_bg, so this
single pool governs the whole app.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ibuilder added a commit that referenced this pull request Jul 23, 2026
ibuilder added a commit that referenced this pull request Jul 23, 2026
…ion bump + CHANGELOG

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ibuilder

Copy link
Copy Markdown
Owner Author

Landed in v0.3.586 (merged into main with the full suite re-verified — 313/313 green). Thanks!

@ibuilder ibuilder closed this Jul 23, 2026
@ibuilder
ibuilder deleted the chore/audit-hardening branch July 23, 2026 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant