Repository navigation
Conversation
generate_config walked every top-level key of the config document and
filtered non-config keys with a deny-list that omitted "tag". Snapshot
documents carry a free-text tag (tag_snapshot), so ip_version became
"tag" and user_data["tag"] was a string. The guards inside the loop are
`in` tests, which against a string degrade from a key check to a
substring check: a tag mentioning chains, filters or groups passed the
guard and the next line indexed a string with a string.
File "package/generate_config.py", line 304, in generate_config
for fw_chain in user_data[ip_version]["chains"]:
TypeError: string indices must be integers, not 'str'
Only View Snapshot Diff hit this -- process_diff is the sole caller that
generates config from snapshot documents; the create-snapshot path
deletes tag before writing current. Not a 2.4.x regression: the
deny-list never listed tag.
Replace it with an allow-list over ("ipv4", "ipv6"). The loop body
already branches only on those two, so this is behaviour-preserving and
immune to any other top-level key. Output verified byte-identical to the
previous code across all 31 documents in a real database. The bulk of
the diff is the body dropping one indent level; read it with
`git diff -w`.
Adds a parametrized regression test that fails on the previous code.
difflib puts its change-navigation links in an unlabeled column and writes them as bare single letters -- "f" (first change), "n" (next change), "t" (top) -- which are easy to miss in a long config diff. - _LabeledHtmlDiff overrides _convert_flags to word the links as "First change" / "Next change" / "Back to top", and blanks difflib's built-in legend, which documented the single-letter wording. - process_diff heads both navigation columns with "Jump" (difflib leaves those header cells empty). - The links render as pills so they stand out against the config lines, and the columns stay as narrow as the text allows. - New "Jump to First Change" toolbar button. It follows the first link in the table rather than a hard-coded href, because difflib generates the anchor prefix per request. - The page legend now explains the Jump columns. _legend and _convert_flags are difflib internals; verified unchanged on Python 3.14 (the container interpreter) as well as 3.12.
uv.lock's fw-gui version was stale at 2.4.1; resolving the project brought it in line with pyproject.toml.
Tags were stored, displayed and edited in ways that had already caused two production bugs, and loading a snapshot silently discarded the working copy. Storage: - write_user_data_file now pops the snapshot-only keys (firewall, snapshot, tag) from a "current" write and $unsets them on the stored document. $set alone left an already-leaked tag in place forever, which is how a tag reached generate_config's ip-version loop and 500'd the diff page. - New set_snapshot_tag() writes only the tag field instead of read-modify-writing the whole configuration document. No upsert, so an unresolvable name is a no-op rather than a ghost snapshot; an empty tag $unsets the key so "untagged" stays representable. - tag_snapshot() is now a thin request wrapper: validates both names with is_safe_name(), trims the tag and caps it at 100 characters, and flashes instead of raising when the snapshot does not exist. - list_snapshots() projects the three fields it renders; it was pulling entire configuration documents on nearly every page render. Safety net on load: - New create_snapshot() copies current into a new snapshot document, with an optional tag. select_firewall_config calls it before restore_snapshot so the working copy the restore overwrites survives, tagged "auto-snapshot before reloading snapshot". The manual create path uses it too. - Snapshot names have one-second granularity and are the only identifier for a snapshot, so _unique_snapshot_name() steps the timestamp forward until the name is free; previously a same-second create overwrote the existing snapshot via the upsert. UI: - /snapshots (snapshot_manage.html) replaces the tag-only page: one row per snapshot with the tag editable inline plus Save Tag, Load, Diff vs Current and Delete, each button reusing an existing endpoint via formaction. - The sidebar shows the tag as text (or "(no tag)") instead of hiding it in a hover tooltip, and links to Manage Snapshots. - The diff chooser labels each option "timestamp — tag" and lets snapshot_1 be the current working copy. - /snapshot_tag_create redirects to /snapshots for old bookmarks. Existing installs need no migration: snapshot documents are unchanged, and a current document that already leaked a tag is cleaned on its next write.
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.
Summary
Three related pieces of snapshot work, plus the version bump to 2.4.3.
Snapshot diff 500 (
ad3bd6c)A tag string sitting at the top level of a configuration document was treated as an IP version by
generate_config, which crashed withTypeError: string indices must be integers.generate_confignow allow-listsipv4/ipv6, and the storage fix below removes the cause.Diff page navigation (
7223f54)The jump-to-change links are now obvious and their columns are labeled.
Snapshot tags reworked (
edd9a1f)Storage.
write_user_data_filepops the snapshot-only keys (firewall,snapshot,tag) from acurrentwrite and$unsets them —$setalone left an already-leaked tag in place forever. Newset_snapshot_tag()writes only thetagfield instead of read-modify-writing the whole configuration document, with noupsert(an unresolvable name is a no-op, not a ghost snapshot) and$unseton an empty tag so "untagged" stays representable.tag_snapshot()became a thin request wrapper that validates both names withis_safe_name(), trims and caps the tag at 100 characters, and flashes instead of raising for a missing snapshot.list_snapshots()now projects the three fields it renders rather than pulling entire configuration documents on nearly every page render.UI.
/snapshotsreplaces the tag-only page: one row per snapshot with the tag editable inline plus Save Tag, Load, Diff vs Current and Delete, each button reusing an existing endpoint viaformaction. The sidebar shows the tag as text (or(no tag)) instead of hiding it in a hover tooltip. The diff chooser labels optionstimestamp — tagand lets either side be the current working copy.Auto-snapshot before loading (
edd9a1f)New
create_snapshot()copies current into a new snapshot document with an optional tag.select_firewall_configcalls it beforerestore_snapshot, so the working copy a restore would overwrite survives as a snapshot taggedauto-snapshot before reloading snapshot. Loading a snapshot by mistake is now recoverable._unique_snapshot_name()steps the one-second-granularity timestamp forward until it is free, fixing a pre-existing hole where two snapshots taken in the same second silently overwrote each other via the upsert.Existing installs
No migration and no schema version bump. Snapshot documents are unchanged; a current document that already leaked a
tagis cleaned on its next write./snapshot_tag_createredirects to/snapshots. Auto-snapshots accumulate one per load with no pruning — worth revisiting if snapshot lists get long.Testing
pytest— 474 passedbandit -c pyproject.toml -r .— 0 issues at every severity