fix(db): allow intentional deletion of all custom configs

[Marker689]

---

Problem

Users cannot delete their last remaining custom configuration (e.g. a ModSecurity rule exception) through the Web UI or API.

When a user selects the only custom config and clicks delete, the UI shows a success message, but the config row persists in the database. Refreshing the page reveals the config is still there.

Root Cause

The save_custom_configs() function in src/common/db/Database.py contains a data-loss guard (line 2412) that silently rejects empty payloads when rows still exist in the database:

if method in ("ui", "api") and not custom_configs:
    existing_count = session.query(Custom_configs).filter(Custom_configs.method == method).count()
    if existing_count > 0:
        self.logger.warning(
            "Refusing save_custom_configs: incoming method={method!r} payload is empty while {existing_count} ..."
        )
        return message  # Silently returns — no error propagated

The delete flow builds a list of "remaining" configs after filtering out the selected ones. When the last config is deleted, this list is empty. The guard then blocks the save operation, but the caller receives no error — the row simply stays in the database.

The guard was designed to protect against accidental data loss (e.g. a service edit that lost its in-memory config map). However, the delete flow is an intentional deletion that legitimately produces an empty remaining list.

Affected Code Paths

• Web UI: src/ui/app/routes/configs.py — delete_configs() function
• REST API: src/api/app/routers/configs.py — delete_configs() router endpoint

Both follow the same pattern: filter selected configs → call save_custom_configs(remaining, method) → guard blocks when remaining is empty.

---

Fix

Add an explicit allow_empty: bool = False parameter to save_custom_configs(). The guard condition becomes:

if method in ("ui", "api") and not custom_configs and not allow_empty:

The delete flows in both UI and API pass allow_empty=True to explicitly opt-in to empty payloads, while all other callers (scheduler, service edit, autoconf, save_config) keep the guard active by default.

Files Changed

File	Change
src/common/db/Database.py	Add allow_empty: bool = False parameter; update guard condition
src/ui/app/routes/configs.py	Pass allow_empty=True in delete_configs()
src/api/app/routers/configs.py	Pass allow_empty=True in delete_configs()

Safety

• Backward compatible: allow_empty=False is the default. All existing callers are unaffected.
• Explicit opt-in: Only the delete flows pass allow_empty=True. They are the only callers that intentionally produce empty remaining lists.
• Guard preserved: Service edits, scheduler, autoconf, and save_config.py all continue to benefit from the data-loss protection.

---

Validation

Pre-commit checks

black --check src/common/db/Database.py src/ui/app/routes/configs.py src/api/app/routers/configs.py  # PASS
flake8 --max-line-length=160 --ignore=E266,E402,E501,E722,W503 ...  # PASS (no output)
refurb ...  # PASS (no output)

Manual verification steps

1. Deploy a BunkerWeb instance with the Web UI
2. Create a single ModSecurity custom config (type: MODSEC, name: exceptions)
3. Select the config and click delete
4. Verify the config is actually removed from the database:

   sqlite3 /path/to/bunkerweb.db "SELECT COUNT(*) FROM bw_custom_configs WHERE method='ui';"
   -- Expected: 0 (was 1 before deletion)
   

5. Verify no configs remain after the operation

Regression check

The guard still fires for all non-delete callers:

• Service edit with lost config map → guard blocks (expected)
• Scheduler saving manual configs → guard blocks if empty (expected)
• Autoconf with disable_cleanup=True → guard bypassed (existing behavior)

[coderabbitai]

src/common

• Added allow_empty: bool = False to Database.save_custom_configs() and tightened the empty-payload guard so intentional empty saves are only accepted when explicitly permitted.

src/api

• Updated the custom config delete endpoint to pass allow_empty=True when persisting the post-delete state, allowing the last remaining config in a method bucket to be removed cleanly.

src/ui

• Updated the UI delete flow to pass allow_empty=True on save, fixing the case where deleting the final custom config previously reported success but left the row in place.

Security impact

• Low risk functional fix; no new exposure. This removes an incorrect protection only for explicit delete flows and preserves the existing safeguard for other callers.

User-visible behaviour

• Deleting the last remaining custom configuration now works from both Web UI and REST API.
• Previously successful delete operations now correctly persist the removal instead of leaving stale database rows.

Configuration / schema

• No configuration or schema changes.

Packaging / deployment

• No packaging or deployment impact.

Documentation / tests

• No documentation updates.
• No tests were updated in this change set.

Walkthrough

Database.save_custom_configs now accepts an allow_empty parameter that relaxes its empty-payload data-loss guard for ui/api methods. Both the API router and UI route delete_configs handlers now pass allow_empty=True when persisting remaining per-method configs.

Changes

Allow-empty flag for custom config deletion

Layer / File(s)	Summary
Database guard update src/common/db/Database.py	save_custom_configs gains a keyword-only allow_empty parameter (default False); the ui/api empty-payload data-loss guard now only blocks when allow_empty is False.
API and UI delete_configs callers src/api/app/routers/configs.py, src/ui/app/routes/configs.py	Both delete_configs handlers now call save_custom_configs(..., allow_empty=True) per method, allowing saves to proceed when the keep list is empty.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Poem

A guard once stood so firm and tight,
Blocking empty lists day and night.
Now with a flag, allow_empty true,
Deletes sail through, nothing askew.
Hop, hop, hooray — the configs clear! 🐇

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the fix and follows Conventional Commits with a db scope.
Description check	✅ Passed	The description matches the delete-last-custom-config fix and the affected UI/API routes.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/common/db/Database.py (2)

2403-2413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Guard comment is now stale/contradicted by the allow_empty path.

The comment claims: "Genuine 'remove all custom configs' actions delete rows individually through the UI/API, so by the time an empty payload reaches save_custom_configs there is nothing left to wipe and this guard is a no-op." That's no longer true — the whole point of this PR is that the UI/API delete flow does reach here with an empty payload when the last row is removed, and allow_empty=True is exactly how it opts out of the guard. Leaving the old rationale in place will mislead the next person touching this guard.

Please update the comment to explain the allow_empty escape hatch and why it's safe (caller has already computed the exact remaining set).

📝 Suggested comment update

-                # Data-loss guard (mirror of the save_config guards above): refuse the
-                # cleanup when a ui/api save_custom_configs call would wipe every
-                # method-owned custom config row while supplying nothing to replace
-                # them. An empty incoming list with existing rows almost always means
-                # the caller built an incomplete payload (route exception, form rebuild
-                # race, missing in-memory state). Genuine "remove all custom configs"
-                # actions delete rows individually through the UI/API, so by the time
-                # an empty payload reaches save_custom_configs there is nothing left
-                # to wipe and this guard is a no-op.
+                # Data-loss guard (mirror of the save_config guards above): refuse the
+                # cleanup when a ui/api save_custom_configs call would wipe every
+                # method-owned custom config row while supplying nothing to replace
+                # them. An empty incoming list with existing rows almost always means
+                # the caller built an incomplete payload (route exception, form rebuild
+                # race, missing in-memory state). Callers that have deliberately computed
+                # the full post-delete remaining set (e.g. the UI/API "delete custom
+                # config" routes deleting the last row of a method) pass
+                # allow_empty=True to bypass this guard intentionally.
                 if method in ("ui", "api") and not custom_configs and not allow_empty:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/common/db/Database.py` around lines 2403 - 2413, Update the stale guard
comment in the cleanup block near the `method in ("ui", "api") and not
custom_configs and not allow_empty` check so it matches the new `allow_empty`
behavior. The current rationale about UI/API “remove all custom configs” actions
being a no-op is no longer true; revise it to explain that `allow_empty`
intentionally bypasses the guard when the caller has already determined the
exact remaining set, such as when the last row is removed. Keep the comment
aligned with the `disable_cleanup` / `allow_empty` logic in
`save_custom_configs`.

---

2368-2390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New allow_empty param is correct but undocumented.

The keyword-only allow_empty: bool = False addition is logically sound and correctly gated at Line 2413. However, save_custom_configs's docstring ("""Save the custom configs in the database""") still doesn't mention this new, behaviour-altering flag. Given it deliberately bypasses a data-loss guard, a one-line note would help future callers understand when it's safe to pass True.

As per path instructions, "provide concise, accurate docstrings for public classes, functions, and methods."

📝 Suggested docstring addition

-        """Save the custom configs in the database"""
+        """Save the custom configs in the database.
+
+        Args:
+            allow_empty: When True, skips the ui/api empty-payload data-loss guard so an
+                         intentionally empty ``custom_configs`` list (e.g. deleting the
+                         last remaining custom config) is persisted instead of refused.
+        """

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/common/db/Database.py` around lines 2368 - 2390, The public method
save_custom_configs now accepts the keyword-only allow_empty flag, but its
docstring still only describes the save behavior without documenting this new
bypass for the empty-config guard. Update the docstring on save_custom_configs
to add a brief note explaining what allow_empty does and when callers should set
it to True, keeping the description concise and aligned with the existing method
contract.

Source: Path instructions

src/api/app/routers/configs.py (1)

385-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct wiring of allow_empty=True for the delete flow.

keep_by_method is built as a full snapshot of surviving method-owned rows before the per-method save_custom_configs calls, so passing allow_empty=True here matches the guard's intent — it only fires when the caller has genuinely computed an empty remaining set for that method (i.e. the last row was just deleted), not an incomplete payload.

One suggestion: this path modifying a previously-guarded, data-loss-sensitive code path would benefit from a regression test (e.g. deleting the sole remaining "api"-method config and asserting the row is actually gone, plus a control case confirming other callers still get the guard).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/api/app/routers/configs.py` around lines 385 - 416, The delete flow in
the config router is wiring `allow_empty=True` correctly, but this path is
data-loss-sensitive and should be covered by a regression test. Add a test
around the delete handler in `configs.py` that exercises the
`keep_by_method`/`save_custom_configs` flow by deleting the last remaining
config for a method and verifying it is removed, plus a control case proving
other `save_custom_configs` callers still enforce the empty-payload guard.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/api/app/routers/configs.py`:
- Around line 385-416: The delete flow in the config router is wiring
`allow_empty=True` correctly, but this path is data-loss-sensitive and should be
covered by a regression test. Add a test around the delete handler in
`configs.py` that exercises the `keep_by_method`/`save_custom_configs` flow by
deleting the last remaining config for a method and verifying it is removed,
plus a control case proving other `save_custom_configs` callers still enforce
the empty-payload guard.

In `@src/common/db/Database.py`:
- Around line 2403-2413: Update the stale guard comment in the cleanup block
near the `method in ("ui", "api") and not custom_configs and not allow_empty`
check so it matches the new `allow_empty` behavior. The current rationale about
UI/API “remove all custom configs” actions being a no-op is no longer true;
revise it to explain that `allow_empty` intentionally bypasses the guard when
the caller has already determined the exact remaining set, such as when the last
row is removed. Keep the comment aligned with the `disable_cleanup` /
`allow_empty` logic in `save_custom_configs`.
- Around line 2368-2390: The public method save_custom_configs now accepts the
keyword-only allow_empty flag, but its docstring still only describes the save
behavior without documenting this new bypass for the empty-config guard. Update
the docstring on save_custom_configs to add a brief note explaining what
allow_empty does and when callers should set it to True, keeping the description
concise and aligned with the existing method contract.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1c3c2f70-89d2-4ba4-a6fa-950689bf0c04

📥 Commits

Reviewing files that changed from the base of the PR and between 515efe9 and a1a65a9.

📒 Files selected for processing (3)

• src/api/app/routers/configs.py
• src/common/db/Database.py
• src/ui/app/routes/configs.py

📜 Review details

🧰 Additional context used

📓 Path-based instructions (7)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Python code must follow snake_case naming for modules and functions, PascalCase for classes, and be formatted with Black at 160 characters per line
Python linting with flake8 must ignore error codes: E266, E402, E501, E722, W503

**/*.py: Python code must use Black for formatting with 160 character line lengths
Python code must conform to Flake8 linting with ignores defined in .pre-commit-config.yaml
Python modules and functions must use snake_case naming convention
Python classes must use PascalCase naming convention

Files:

• src/api/app/routers/configs.py
• src/common/db/Database.py
• src/ui/app/routes/configs.py

---

⚙️ CodeRabbit configuration file

**/*.py: Follow BunkerWeb's Python standards and security posture:

• Use snake_case for functions and variables, PascalCase for classes, and provide concise, accurate docstrings for public classes, functions, and methods.
• Respect Black formatting with a 160-character line limit and the existing pre-commit conventions. Do not insist on adding type annotations to previously untyped code, but accept them when added consistently.
• Catch specific exceptions; never use bare except:. Catching Exception is acceptable only at explicit process boundaries (for example scheduler loops, outer job runners, worker entrypoints, or graceful-shutdown boundaries) when the code logs enough context and either re-raises, returns an error status, or terminates safely.
• Never use os.system, subprocess.*(..., shell=True), eval, or exec. Pass subprocess arguments as a list and prefer explicit binary paths for privileged operations.
• Do not use unsafe deserialisers (pickle, marshal, shelve, jsonpickle, dill) for untrusted data. Use yaml.safe_load() rather than unsafe YAML loading.
• Open files with an explicit encoding (normally utf-8) and use with statements for files, sockets, database sessions, and temporary resources.
• Use secrets for token generation and hmac.compare_digest for token, HMAC, or signature comparisons.
• For HTTP clients (requests, httpx): always set an explicit timeout, validate destination URLs to block RFC1918/loopback/link-local ranges (SSRF), disable automatic redirects to internal hosts, and be careful with proxy settings.
• For filesystem operations: resolve paths with Path.resolve() and verify they remain under the intended base directory before reading or writing (path traversal).
• Use defusedxml rather than stdlib XML parsers for untrusted XML.
• For SQLAlchemy, use bound parameters and safe query construction. Never call text() with f-strings or .format(), and flag .execute() calls built...

Files:

• src/api/app/routers/configs.py
• src/common/db/Database.py
• src/ui/app/routes/configs.py

src/api/app/routers/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

API service must use FastAPI with router-based architecture in src/api/app/routers/ for auth, instances, services, configs, plugins, jobs

Files:

• src/api/app/routers/configs.py

src/api/**/*.py

⚙️ CodeRabbit configuration file

src/api/**/*.py: src/api/ is the FastAPI service:

• Avoid blocking I/O in async endpoints and background tasks.
• Validate request models strictly and return precise HTTP status codes.
• Authentication, authorisation, CORS, and rate-limiting changes must preserve secure defaults.
• Propagate bounded timeouts to any outbound calls and avoid retry loops without caps, jitter, and logging.
• Be careful with streaming responses, file downloads, and proxy-like behaviour: validate content types, filenames, and upstream destinations.

Files:

• src/api/app/routers/configs.py

src/common/db/{model,Database}.py

📄 CodeRabbit inference engine (CLAUDE.md)

SQLAlchemy ORM models must be defined in src/common/db/model.py with Database.py wrapping high-level query methods

Files:

• src/common/db/Database.py

src/ui/app/routes/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Web UI must use Flask Blueprints for routing, with routers in src/ui/app/routes/ for configs, plugins, jobs, logs, instances, profile, etc.

Files:

• src/ui/app/routes/configs.py

src/ui/app/

📄 CodeRabbit inference engine (CLAUDE.md)

Web UI dependency injection must be centralized in src/ui/app/dependencies.py providing DB, DATA, BW_CONFIG, BW_INSTANCES_UTILS

Files:

• src/ui/app/routes/configs.py

src/ui/**/*.py

⚙️ CodeRabbit configuration file

src/ui/**/*.py: src/ui/ is the admin UI and related backend:

• State-changing routes must enforce CSRF protection where browser sessions are used.
• Session, remember-me, and auth-cookie changes must preserve Secure, HttpOnly, and appropriate SameSite behaviour.
• Redirect targets such as next parameters must be validated against an allowlist or local-path policy.
• For templates and forms, escape user-controlled data, validate uploads by type and size, and keep files outside the web root unless there is a deliberate reviewed exception.
• BunkerWeb uses bcrypt; flag any move towards weak password hashing or plaintext credential handling.

Files:

• src/ui/app/routes/configs.py

🔇 Additional comments (2)

src/common/db/Database.py (1)

2389-2389: 🎯 Functional Correctness

Guard bypass logic is correct.

and not allow_empty correctly short-circuits the empty-payload guard only when the caller opts in, while leaving the guard intact (default False) for every other caller. This is the right fix for the "can't delete the last custom config" bug.

Also applies to: 2413-2413

src/ui/app/routes/configs.py (1)

382-417: LGTM!