feat: Platform Improvements v2 - Reporting, Backup, and Rule Validation#65
feat: Platform Improvements v2 - Reporting, Backup, and Rule Validation#65TerrifiedBug merged 8 commits intomainfrom
Conversation
Implements Phase 2 and Phase 3 features from platform improvements roadmap: Advanced Reporting (Task 10): - PDF/CSV export for Alert Summary reports with date filtering - PDF/CSV export for Rule Coverage reports - Reports include severity distribution, status breakdown, ATT&CK techniques Config Import/Export (Task 11): - Full configuration backup to JSON including rules, correlations, webhooks - Import with conflict resolution (skip, overwrite, rename) - Dry-run preview mode before applying imports Bug fixes for export functionality: - Fix enum value handling for source, operator, origin fields - Parse YAML content to extract tags for rule coverage reports - Improve Settings backup UI with styled file upload area - Make export buttons consistent (outline variant) - Remove unused date picker from Rule export (rules are not time-based) Additional features implemented: - Correlation rule snooze functionality - User notification preferences - Exception grouping by group_id - IP allowlist and rate limiting for API - WebSocket improvements for real-time updates - Index pattern field discovery enhancements
- Fix notification_settings always showing "Will Update" by comparing values before marking as updated. Now correctly shows "Will Skip" when no changes are detected. - Fix "Multiple rows were found" error when importing rules with duplicate titles. Now gracefully skips and reports an error message explaining that multiple rules exist with the same title. - Apply same fix to correlation rules import for consistency.
- Add unique constraint to Rule.title field in model - Create migration that deduplicates existing rules by appending a number suffix to duplicates (e.g., "Rule Name (2)") - Prevents future duplicate rule titles This fixes import errors where multiple rules with the same title caused "Multiple rows were found" errors.
When creating or updating a rule with a title that already exists, return a 409 Conflict with a clear message instead of a 500 error. The error message explains: "A rule with the title 'X' already exists. Please choose a different title."
Frontend: - Add debounced title check (500ms) when typing rule title - Show error message below title input when duplicate detected - Highlight input border red when duplicate - Show loading spinner while checking - Disable Save button if title is duplicate Backend: - Add /rules/check-title endpoint to check title availability - Supports exclude_id parameter for edit mode (don't flag own title)
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Remove user-controlled data from console.log statements to prevent log injection attacks: - Log only alert ID instead of full alert data - Remove server message content from connection log - Remove close reason from disconnect log (keep only code)
Frontend (use-websocket.ts): - Remove console.log that logged message.type and alert_id - These values come from parsed WebSocket data (user-controlled) Backend (correlation.py): - Use parameterized logging with only rule ID (UUID) - Remove user-controlled name, sigma_field, and entity_value from log
|
|
||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = '04bf62g048c6' |
Check notice
Code scanning / CodeQL
Unused global variable Note
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = '04bf62g048c6' | ||
| down_revision: Union[str, None] = '03ae51f937b5' |
Check notice
Code scanning / CodeQL
Unused global variable Note
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| # revision identifiers, used by Alembic. | ||
| revision: str = '04bf62g048c6' | ||
| down_revision: Union[str, None] = '03ae51f937b5' | ||
| branch_labels: Union[str, Sequence[str], None] = None |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, unused global variables should either be removed or renamed to clearly indicate that they are intentionally unused (e.g., _unused_branch_labels). However, for Alembic migrations, only specific globals are actually consulted by Alembic. branch_labels is optional and only needed when you want to associate labels with a revision; setting it to None has no runtime effect.
The best way to fix this without changing behavior is to delete the branch_labels assignment line entirely. Since its value is None, its presence does not affect Alembic’s behavior; if Alembic needs labels, they would have to be non-None. Removing that line will satisfy CodeQL’s unused-global check and will not change the functionality of the migration. All other revision identifiers (revision, down_revision, depends_on) remain unchanged.
Concretely:
- Edit
backend/alembic/versions/04bf62g048c6_add_correlation_rule_snooze.py. - Remove line 17 (
branch_labels: Union[str, Sequence[str], None] = None). - No new imports or helper functions are required.
| @@ -14,7 +14,6 @@ | ||
| # revision identifiers, used by Alembic. | ||
| revision: str = '04bf62g048c6' | ||
| down_revision: Union[str, None] = '03ae51f937b5' | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None | ||
|
|
||
|
|
| revision: str = '04bf62g048c6' | ||
| down_revision: Union[str, None] = '03ae51f937b5' | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, an unused global variable should either be removed (if it has no side-effectful right-hand side) or renamed to one of the accepted “unused” patterns if it is present only for documentation or template completeness. Here, depends_on is purely declarative, has no side effects, and is set to None, so we can safely change its name without affecting runtime behavior.
To fix the issue without changing functionality, rename depends_on to something that clearly indicates it is intentionally unused, such as _unused_depends_on. This preserves the information for readers that the migration has no dependencies while satisfying the static analysis rule. The change is in backend/alembic/versions/04bf62g048c6_add_correlation_rule_snooze.py, at the definition on line 18. No imports, methods, or additional definitions are required.
| @@ -15,7 +15,7 @@ | ||
| revision: str = '04bf62g048c6' | ||
| down_revision: Union[str, None] = '03ae51f937b5' | ||
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None | ||
| _unused_depends_on: Union[str, Sequence[str], None] = None | ||
|
|
||
|
|
||
| def upgrade() -> None: |
|
|
||
|
|
||
| # revision identifiers, used by Alembic. | ||
| revision: str = '05cg73h159d7' |
Check notice
Code scanning / CodeQL
Unused global variable Note
Copilot Autofix
AI about 1 month ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| revision: str = "07ip_allowlist_ratelimit" | ||
| down_revision: str = "06dh84i260e8" | ||
| branch_labels: str | Sequence[str] | None = None | ||
| depends_on: str | Sequence[str] | None = None |
Check notice
Code scanning / CodeQL
Unused global variable Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the issue without changing functionality, we should keep depends_on but make it clear that it is intentionally a public module-level symbol. The recommended mechanism per the rule description is to include such globals in the module’s __all__ list, which counts as an explicit “use”. This avoids renaming Alembic’s expected variable and does not affect runtime behavior.
Concretely, in backend/alembic/versions/07ip_allowlist_ratelimit_add.py, add a module-level __all__ sequence that lists the Alembic-identifying globals (revision, down_revision, branch_labels, depends_on) and the upgrade/downgrade functions. Place this right after the existing revision identifier declarations (around lines 16–21). No new imports or other definitions are needed; __all__ is a plain list of strings.
| @@ -19,7 +19,16 @@ | ||
| branch_labels: str | Sequence[str] | None = None | ||
| depends_on: str | Sequence[str] | None = None | ||
|
|
||
| __all__ = [ | ||
| "revision", | ||
| "down_revision", | ||
| "branch_labels", | ||
| "depends_on", | ||
| "upgrade", | ||
| "downgrade", | ||
| ] | ||
|
|
||
|
|
||
| def upgrade() -> None: | ||
| # Add IP allowlist column | ||
| op.add_column( |
|
|
||
| """ | ||
| from alembic import op | ||
| import sqlalchemy as sa |
Check notice
Code scanning / CodeQL
Unused import Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix an unused import, the general approach is to remove the import statement (or specific imported name) that is not referenced anywhere in the file. This eliminates unnecessary dependencies and resolves static analysis warnings without changing runtime behaviour.
In this specific case, the import sqlalchemy as sa on line 9 of backend/alembic/versions/08jq_add_rule_title_unique.py is never used. The rest of the migration uses op and text directly. The best fix is to delete that single import line and leave the from alembic import op and from sqlalchemy import text imports intact. No other methods, imports, or definitions are needed, and no changes to the upgrade or downgrade functions are required.
| @@ -6,7 +6,6 @@ | ||
|
|
||
| """ | ||
| from alembic import op | ||
| import sqlalchemy as sa | ||
| from sqlalchemy import text | ||
|
|
||
| # revision identifiers, used by Alembic. |
| if exc.get("operator"): | ||
| try: | ||
| operator = ExceptionOperator(exc["operator"]) | ||
| except ValueError: |
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, empty except blocks should either be removed (letting the exception propagate), narrowed further, or augmented to at least log or otherwise handle the error. Here we want to preserve current behavior (fall back to defaults when parsing fails) but avoid completely silent failures.
The best targeted fix is to add logging inside the two except ValueError: blocks in _import_exceptions, capturing the invalid value and clarifying that a default is being used. This keeps the existing functional behavior (defaults are already set before the try/except) while ensuring that configuration/data issues surface in logs.
Concretely, in backend/app/api/export.py:
- Add an
import loggingat the top alongside the other imports. - In the
except ValueError:afteroperator = ExceptionOperator(exc["operator"]), replacepasswith alogging.warning(...)call that mentions the invalid operator, and that the default is used. - In the
except ValueError:aftergroup_id = uuid.UUID(exc["group_id"]), similarly replacepasswith alogging.warning(...)about the invalidgroup_idand fallback behavior.
No new methods or complex structures are needed; just the standard library logging import and two logging calls.
| @@ -9,6 +9,7 @@ | ||
| from datetime import datetime | ||
| from enum import Enum | ||
| from typing import Annotated, Any | ||
| import logging | ||
|
|
||
| from fastapi import APIRouter, Depends, File, HTTPException, Response, UploadFile | ||
| from fastapi.responses import StreamingResponse | ||
| @@ -752,14 +753,23 @@ | ||
| try: | ||
| operator = ExceptionOperator(exc["operator"]) | ||
| except ValueError: | ||
| pass | ||
| logging.warning( | ||
| "Invalid exception operator '%s' for rule %s; using default '%s'.", | ||
| exc.get("operator"), | ||
| rule_id, | ||
| operator, | ||
| ) | ||
|
|
||
| group_id = uuid.uuid4() | ||
| if exc.get("group_id"): | ||
| try: | ||
| group_id = uuid.UUID(exc["group_id"]) | ||
| except ValueError: | ||
| pass | ||
| logging.warning( | ||
| "Invalid exception group_id '%s' for rule %s; using new UUID.", | ||
| exc.get("group_id"), | ||
| rule_id, | ||
| ) | ||
|
|
||
| new_exc = RuleException( | ||
| rule_id=rule_id, |
| if exc.get("group_id"): | ||
| try: | ||
| group_id = uuid.UUID(exc["group_id"]) | ||
| except ValueError: |
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, empty except blocks should either (a) re-raise or convert the exception, or (b) at least log or comment why it is being intentionally ignored. Here, we want to keep the current fallback behavior (default operator and random group ID) but add proper handling.
Best fix: add logging within each except ValueError: block to record that an invalid operator or group_id was encountered, including the offending value. This way, imports still succeed with defaults, but operators can investigate logs when something is wrong. To implement this:
- Add a
loggingimport at the top ofbackend/app/api/export.py. - In the first
except ValueError:(around lines 752–755), log a warning includingexc.get("operator"). - In the second
except ValueError:(around lines 759–762), log a warning includingexc.get("group_id").
No new external dependencies are needed; logging is from the standard library.
| @@ -9,6 +9,7 @@ | ||
| from datetime import datetime | ||
| from enum import Enum | ||
| from typing import Annotated, Any | ||
| import logging | ||
|
|
||
| from fastapi import APIRouter, Depends, File, HTTPException, Response, UploadFile | ||
| from fastapi.responses import StreamingResponse | ||
| @@ -752,14 +753,23 @@ | ||
| try: | ||
| operator = ExceptionOperator(exc["operator"]) | ||
| except ValueError: | ||
| pass | ||
| logging.warning( | ||
| "Invalid exception operator '%s' for rule %s; using default '%s'.", | ||
| exc.get("operator"), | ||
| rule_id, | ||
| ExceptionOperator.EQUALS, | ||
| ) | ||
|
|
||
| group_id = uuid.uuid4() | ||
| if exc.get("group_id"): | ||
| try: | ||
| group_id = uuid.UUID(exc["group_id"]) | ||
| except ValueError: | ||
| pass | ||
| logging.warning( | ||
| "Invalid exception group_id '%s' for rule %s; using new UUID.", | ||
| exc.get("group_id"), | ||
| rule_id, | ||
| ) | ||
|
|
||
| new_exc = RuleException( | ||
| rule_id=rule_id, |
| parsed = yaml.safe_load(yaml_content) | ||
| if parsed and isinstance(parsed, dict): | ||
| return parsed.get("tags", []) or [] | ||
| except Exception: |
Check notice
Code scanning / CodeQL
Empty except Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, empty except blocks should either be removed (letting exceptions propagate) or should explicitly handle the error, commonly by logging it and/or converting it to a controlled fallback. Here, we want to preserve the current behavior of returning [] on failure but avoid silently discarding information.
The best targeted fix is to replace the bare except Exception: pass with an except Exception as exc: that logs the parsing error using a module-level logger. This way, any malformed YAML or unexpected runtime error is recorded, but the function continues to return [] as before, so existing functionality (including API behavior and return values) is unchanged. Concretely, in backend/app/api/reports.py, we should: (1) import logging near the top of the file, (2) define a logger like logger = logging.getLogger(__name__), and (3) update the get_tags_from_yaml helper so that it logs an error message in the except block instead of just passing.
| @@ -13,6 +13,7 @@ | ||
| from enum import Enum | ||
| from typing import Annotated, Any | ||
|
|
||
| import logging | ||
| import yaml | ||
| from fastapi import APIRouter, Depends, HTTPException | ||
| from fastapi.responses import StreamingResponse | ||
| @@ -26,6 +27,8 @@ | ||
| from app.models.rule import Rule | ||
| from app.models.user import User | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| router = APIRouter(prefix="/reports", tags=["reports"]) | ||
|
|
||
|
|
||
| @@ -480,8 +483,8 @@ | ||
| parsed = yaml.safe_load(yaml_content) | ||
| if parsed and isinstance(parsed, dict): | ||
| return parsed.get("tags", []) or [] | ||
| except Exception: | ||
| pass | ||
| except Exception as exc: | ||
| logger.warning("Failed to parse tags from rule YAML content: %s", exc) | ||
| return [] | ||
|
|
||
| # Severity distribution |
Summary
This PR implements Phase 2 and Phase 3 features from the platform improvements roadmap, plus several bug fixes discovered during testing.
Advanced Reporting (Task 10)
Config Import/Export (Task 11)
Rule Title Uniqueness
Bug Fixes
Additional Features
Test plan