Repository navigation
Conversation
Drop 8 accidental request.form dumps that wrote entire submitted forms to data/log/app.log (6 logging.debug in app.py, logging.info in app.py and filter_functions, logging.info(request.form.items()) in flowtable_functions). Log hygiene / avoid leaking form field values. The single-field extra_items log is kept. 363 tests pass; bandit clean.
Development mode enables the Werkzeug debugger (RCE). The container never sets FLASK_ENV (defaults to the waitress serve path), so this is only a risk if an operator explicitly enables Development on a network-reachable host. Add a one-line caution in examples/.env.prod. No code change.
Set X-Frame-Options: SAMEORIGIN (clickjacking), X-Content-Type-Options: nosniff, and Referrer-Policy on every response; emit HSTS only when served over HTTPS (reusing SESSION_COOKIE_SECURE). CSP intentionally deferred — a strict policy would break inline scripts/handlers and needs a separate tested change. Adds tests. 365 pass; bandit clean.
read_user_data_file silently deleted+overwrote the current config when called with a snapshot name (diff=False) -- a 'read' that mutates data. Extract that into an explicit restore_snapshot(filename, snapshot); make read_user_data_file a pure read (lazy schema-migration writes kept). select_firewall_config now calls restore_snapshot only for an actual snapshot. Behavior-preserving; adds restore + non-destructive-read tests. 368 tests pass; bandit clean.
Bare 'except Exception: return {}' made a Mongo read error look like an
empty config, which guard-creating write paths could then overwrite (data
loss), and it hid real bugs. Catch pymongo.errors.PyMongoError, log it, and
re-raise; stop catching bare Exception. Inert in healthy operation; a DB
outage now fails loud instead of risking data loss. Adds a propagation
test. 369 pass; bandit clean.
Mirror the chain path: guard-create ip_version/filters/filter/rule-order before use, so adding a rule to a nonexistent filter (crafted or out-of-order request) no longer raises KeyError. Adds a test. 370 pass.
decrypt_file wrote the plaintext private key to data/tmp/ with a 6-char non-crypto random name and default (world-readable) perms. Switch to tempfile.mkstemp(dir=data/tmp) — crypto-random name, 0o600 owner-only. Remove now-unused random/string imports. Close the Paramiko cleanup gap: if ssh.connect fails, remove the temp key rather than leaving it staged. Update docs/ssh-credential-handling.md. 370 pass; bandit clean.
logging>=0.4.9.6 and uuid>=1.30 are Python-2-era PyPI backports of stdlib modules (they import cStringIO / use long() and would crash on import under Python 3). They were dormant (stdlib precedes site-packages) but a latent landmine. All import logging / import uuid usage is stdlib-only. Remove both and re-lock. 370 tests pass.
Build context was the repo root with no .dockerignore, sending ~1.2 GB to the daemon each build (data/ alone ~1 GB, plus .venv and .git) and exposing data/ (auth.db, keys, logs) to the daemon. Add .dockerignore excluding version control, local envs, runtime data, caches, and non-runtime dirs. Switch ADD globs to COPY of directories (predictable semantics; no __pycache__ shipped). Base-image pinning intentionally left on latest per maintainer (wants updates/fixes each build). Build-only change.
Replace 3x 'os.environ["DISABLE_REGISTRATION"] == "False"' with a registration_enabled() helper: os.environ.get (no KeyError when unset), case-insensitive truthy check, defaults to enabled to match the shipped config. Adds parsing tests. 372 pass; bandit clean.
Prevent unguarded-index 500s app-wide. Add global 404/500 handlers (500 logs the exception and renders a friendly error.html instead of a stack trace) and a @requires_firewall decorator on the 25 routes that read session[firewall_name] (redirect to the config page with a prompt when no firewall is selected, instead of KeyError 500). Missing request.form[...] already yields Werkzeug 400. Adds tests. 375 pass; bandit clean.
post_telemetry and check_version had no timeout, so a hung endpoint could stall login (both run during process_login). Add timeout=5.0 so they fail fast into the existing except. 375 pass.
Replace subprocess.run(['cp', ...]) with shutil.copy: portable (no cp on Windows), no spawned process, and drops the subprocess import + 3 nosec markers (B404/B603/B607). 78 data-file tests pass; bandit clean.
Wrap the 5 open() sites in try/except OSError with sensible defaults: - app.py import-time .version -> FWGUI_VERSION=0.0.0 + warning (was a hard startup crash if .version missing) - startup banner -> 'unknown' - check_version() -> local_version 0.0.0 - get_version() -> 0.0.0 - get_instance_id() -> '' (telemetry degrades) Behavior unchanged when files exist. 375 pass; bandit clean.
Replace the private logging._nameToLevel with the public getLevelNamesMapping() (3.11+; requires-python is >=3.12.8). Same validation. 375 pass.
download_config and download_json returned raw strings (text/html, inline). Return via send_file with proper mimetype, as_attachment, and download_name (<firewall>.conf / .json) so they download instead of rendering inline. Tests assert the attachment headers. 375 pass.
register_user and change_password now require passwords >= 8 chars (MIN_PASSWORD_LENGTH); register_user validates email against a basic regex. Enforced only on set, so existing users and logins are unaffected (no lockouts). Adds tests. 378 pass; bandit clean.
Ignore directories at dir-level (data/, dump/, jupyter/, tmp/, reference/) and add .ipynb_checkpoints/ so stray files can't be accidentally committed. Removed local-only clutter from the working tree (.coverage, dump/, tmp/, jupyter/, .ipynb_checkpoints/); none were git-tracked.
Apply @requires_firewall to snapshot_diff_choose and snapshot_diff_display and drop their copy-pasted 'No firewall selected' blocks; they now redirect to the config page with a flash like every other firewall-requiring route. Only display_config keeps the inline selection view. 378 pass.
test_allows_when_firewall_selected hit /group_view, which reads MongoDB; it passed locally (Mongo running) but failed in CI (no Mongo). The guard's decision is already covered by the redirect test (which returns before any Mongo access). 377 pass.
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
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.
Follow-up to v2.4.0. Resolves the remaining MEDIUM findings and a batch of LOW ones from the code review. All changes verified: 378 tests pass, bandit clean.
Security / hardening
after_requestsetsX-Frame-Options: SAMEORIGIN,X-Content-Type-Options: nosniff,Referrer-Policy; HSTS gated to HTTPS (SESSION_COOKIE_SECURE). CSP intentionally deferred (would break inline template scripts).decrypt_filenow usestempfile.mkstemp(crypto-random name,0o600) instead of a 6-charrandomname + world-readable perms; the temp key is removed even whenssh.connectfails.DISABLE_REGISTRATION— robust parsing (no KeyError when unset, case-insensitive).Robustness / correctness
read_user_data_file— split the destructive snapshot-restore into an explicitrestore_snapshot(); the read is now pure. DB errors raise (PyMongoError) instead of being swallowed into{}(which risked overwriting real config).add_filter_rule_to_data— guard-create nested structure (mirrors the chain path) so adding a rule to a missing filter can't KeyError.@requires_firewallon the 25 routes that need a selected firewall (redirect instead of KeyError 500). Also dedups the copy-pasted "No firewall selected" blocks..version/instance.idreads guarded with defaults (a missing.versionno longer crashes startup).Hygiene / build
request.formdumps).logging/uuidPython-2 backport dependencies..dockerignore(drops ~1 GB of context incl.data/); switch DockerfileADDglobs toCOPY.shutil.copyinstead ofsubprocess cp; publiclogging.getLevelNamesMapping(); config/JSON downloads now sent as file attachments..gitignore; drop local clutter.Notes
latest(maintainer wants updates each build). Rate limiting deferred (needs ProxyFix + deployment-specific config).