Skip to content

Commit 0d3b5df

Browse files
author
wildleo91
committed
fix(approval): WM banner shows analyst reason for column_removal/remove_csv/remove_rule (build 641)
The Whitelist Manager approval banner rendered "<action> by <analyst> -" with no reason text for column_removal, remove_csv, and remove_rule requests because the `pending_info` projection in `_get_csv_content` and `_action_get_pending_approvals` dropped the queue entry's `comment` field on the way to the frontend, while the auto-generated `description` field is empty for those action types. The frontend reads `pa.comment || pa.description || ""`, so both falsy = blank banner. Stayed dormant until build 640's clean demo-state seed because prior sessions were dominated by bulk_row_* requests where description IS auto-populated; the column_removal path with an analyst-typed comment was the first case where the missing field surfaced visibly. Fix: extracted the projection into `wl_approval.project_pending_info()` so both call sites share a single contract that includes `comment`. Pinned the contract with 15 unit tests covering: comment propagation, missing-comment defaulting, all 8 contract fields, RBAC payload-hiding for non-editors, and parametric coverage of all 10 banner-relevant action types. Verified end-to-end: REST `get_csv_content` returns the comment in pending_info[0]; browser load of WM with DR130 selected renders the banner with "Field deprecated by GRC team" after build deploy. Control Panel was unaffected because `get_approval_queue` returns the queue verbatim (no projection). Rollback: revert this commit, redeploy at build 640. No data migration needed — queue entries already carry the `comment` field; only the projection was stripping it.
1 parent a700c56 commit 0d3b5df

5 files changed

Lines changed: 253 additions & 20 deletions

File tree

CHANGELOG.md

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,90 @@ Detailed per-round entries below.
6464

6565
---
6666

67+
## Unreleased — 2026-05-07 (build 641, fix WM approval-banner blank reason)
68+
69+
### Bug — `comment` dropped from `pending_info` projection
70+
71+
The Whitelist Manager dashboard renders an approval banner when a CSV
72+
has a pending request (`<action> by <analyst> — <reason>`). On a
73+
freshly-seeded DR130 column-removal request, the banner displayed
74+
`column removal by analyst1 —` with **nothing after the dash** even
75+
though the Control Panel's Approval Queue showed the same request
76+
with `Analyst Reason: Field deprecated by GRC team` correctly.
77+
78+
**Root cause**: two endpoints (`_get_csv_content` for the WM page and
79+
`_action_get_pending_approvals` for the polling refresh) constructed
80+
their `pending_info` response inline with this exact field set:
81+
`request_id, action_type, description, analyst, timestamp,
82+
pending_highlight, payload`. **`comment` was not in the list.** The
83+
frontend banner at `wl_approval_ui.js:405` reads
84+
`pa.comment || pa.description || ""` — for `column_removal` /
85+
`remove_csv` / `remove_rule` the auto-`description` is empty by
86+
handler convention, so both fallbacks were falsy and the banner
87+
rendered as empty. The Control Panel was unaffected because it uses
88+
`get_approval_queue` which returns the queue entry verbatim.
89+
90+
**Why the bug stayed dormant**: the prior demo state had ~14 pending
91+
entries and 245 historical entries dominated by `bulk_row_addition`
92+
and `bulk_row_removal` requests, where `description` IS auto-
93+
populated by the handler. The blank-banner case was triggered only
94+
for action types where description is empty AND the analyst typed a
95+
free-form comment — exactly the path my build-640 demo-seed exercised
96+
on a clean state. Stale fuzz data had been masking it the whole time.
97+
98+
**Fix**:
99+
100+
1. Extract the shared projection into `wl_approval.project_pending_info`
101+
so both endpoints route through one place. Helper is library code
102+
(no `splunk.rest` import) and can be unit-tested directly.
103+
2. Add `"comment": entry.get("comment", "")` to the projection. The
104+
`.get` fallback covers older queue entries that may not have the
105+
field (forward-compatible upgrade path).
106+
3. Both call sites in `wl_handler.py` now read
107+
`[project_pending_info(p, has_edit=...) for p in queue]`
108+
single line replaces the prior inline dict literal.
109+
110+
**Tests**: `tests/unit/test_pending_info_projection.py` — 15 cases
111+
pinning the contract:
112+
- `comment` propagates for `column_removal` (the regression case)
113+
- Missing `comment` defaults to `""`, not `KeyError` or `None`
114+
- All 8 contract fields present (regression guard for future
115+
field drops)
116+
- `has_edit=False` strips `payload` + `pending_highlight`
117+
(RBAC contract)
118+
- `has_edit=True` exposes both
119+
- `comment` propagates for all 10 valid `action_type` values
120+
121+
**Verification**: REST endpoint smoke-test confirmed
122+
`comment: "Field deprecated by GRC team"` now appears in the
123+
`get_csv_content` response for DR130; browser-tested the WM page
124+
which now renders the banner with the analyst's reason text.
125+
126+
### Build
127+
128+
- `app.conf [install] build` 640 → 641
129+
- `whitelist_manager.js` urlArgs unchanged (no JS edits)
130+
131+
### Migration / rollback
132+
133+
- Revert: re-inline both projections in `wl_handler.py`
134+
(`_get_csv_content` ~line 1665, `_action_get_pending_approvals`
135+
~line 2336) and delete `project_pending_info` + the export
136+
from `__all__` in `wl_approval.py`. Tests under
137+
`tests/unit/test_pending_info_projection.py` would then need to
138+
be deleted or rewritten against the inline shape. Frontend
139+
banner reverts to its prior blank-on-column-removal behavior.
140+
141+
### Cleanup
142+
143+
- Removed `backups/2026-05-06/` (audit-index tarball + state
144+
JSONs + orphan CSVs + version snapshots from build-640 cleanup).
145+
Was already gitignored; just freed local disk. Re-generate any
146+
time via the seed-then-clean playbook documented in
147+
`tests/fixtures/demo-state/README.md`.
148+
149+
---
150+
67151
## Unreleased — 2026-05-06 (build 640, audit consistency + demo-state cleanup)
68152

69153
### Audit Trail consistency

bin/wl_approval.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
"check_conflicts",
5151
"cancel_conflicts",
5252
"generate_request_id",
53+
"project_pending_info",
5354
]
5455

5556
# Module-level constants
@@ -410,6 +411,36 @@ def get_pending_for_csv(csv_file: str) -> List[Dict]:
410411
return [e for e in queue if e.get("csv_file") == csv_file and e.get("status") == "pending"]
411412

412413

414+
def project_pending_info(entry: Dict[str, Any],
415+
has_edit: bool = True) -> Dict[str, Any]:
416+
"""Project a queue entry to the public-facing `pending_info` shape.
417+
418+
Used by the `_get_csv_content` and `_action_get_pending_approvals`
419+
endpoints in wl_handler.py. The frontend banner in
420+
`wl_approval_ui.js` reads ``pa.comment || pa.description`` to
421+
display the analyst's reason; for action types where the auto-
422+
description is empty (`column_removal`, `remove_csv`,
423+
`remove_rule`, etc.) the `comment` field is the only source.
424+
Build 641 added `comment` to this projection after a blank-banner
425+
bug surfaced on a cleanly-seeded DR130 column-removal request.
426+
427+
`has_edit` gates which fields a non-editor caller can see.
428+
Non-editors can see who/when/what but not the row-level highlight
429+
or payload (which can carry sensitive row data).
430+
"""
431+
return {
432+
"request_id": entry["request_id"],
433+
"action_type": entry["action_type"],
434+
"description": entry["description"],
435+
"comment": entry.get("comment", ""),
436+
"analyst": entry["analyst"],
437+
"timestamp": entry["timestamp"],
438+
"pending_highlight": entry.get("pending_highlight", {})
439+
if has_edit else {},
440+
"payload": entry.get("payload", {}) if has_edit else {},
441+
}
442+
443+
413444
def get_pending_for_rule(rule_name: str) -> List[Dict]:
414445
"""
415446
Get all pending approval requests for a specific detection rule.

bin/wl_handler.py

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@
194194
from wl_approval import (
195195
get_pending_for_csv, get_pending_for_rule, submit_approval,
196196
submit_dual_approval, check_approval_gate, expire_pending_approvals,
197-
check_conflicts, cancel_conflicts,
197+
check_conflicts, cancel_conflicts, project_pending_info,
198198
# Canonical request-ID generator (Phase 4 consolidation — CLAUDE.md
199199
# 2026-04-19). Prior to this, handler shipped its own version that
200200
# produced a different format, so the approval queue held IDs in
@@ -1663,15 +1663,8 @@ def _get_csv_content(self, request, csv_file, app_context, tz_offset="0"):
16631663

16641664
# Fetch pending approvals for this CSV
16651665
pending_approvals = _get_pending_for_csv(csv_file)
1666-
pending_info = [{
1667-
"request_id": p["request_id"],
1668-
"action_type": p["action_type"],
1669-
"description": p["description"],
1670-
"analyst": p["analyst"],
1671-
"timestamp": p["timestamp"],
1672-
"pending_highlight": p.get("pending_highlight", {}),
1673-
"payload": p.get("payload", {}),
1674-
} for p in pending_approvals]
1666+
pending_info = [project_pending_info(p, has_edit=True)
1667+
for p in pending_approvals]
16751668

16761669
try:
16771670
st = os.stat(path)
@@ -2327,15 +2320,8 @@ def _action_get_pending_approvals(self, request, query, user, roles):
23272320
csv_file = query.get("csv_file", "")
23282321
pending = get_pending_for_csv(csv_file)
23292322
has_edit = is_editor(roles)
2330-
pending_info = [{
2331-
"request_id": p["request_id"],
2332-
"action_type": p["action_type"],
2333-
"description": p["description"],
2334-
"analyst": p["analyst"],
2335-
"timestamp": p["timestamp"],
2336-
"pending_highlight": p.get("pending_highlight", {}) if has_edit else {},
2337-
"payload": p.get("payload", {}) if has_edit else {},
2338-
} for p in pending]
2323+
pending_info = [project_pending_info(p, has_edit=has_edit)
2324+
for p in pending]
23392325
return self._resp(200, {"pending_approvals": pending_info})
23402326

23412327
def _action_get_request_csv(self, request, query, user, roles):

default/app.conf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
[install]
77
is_configured = false
8-
build = 640
8+
build = 641
99

1010
[launcher]
1111
author = Security Engineering
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
"""
2+
Tests for `project_pending_info` — the queue-entry shape that
3+
`_get_csv_content` and `_action_get_pending_approvals` send to the
4+
WM frontend.
5+
6+
Origin: build 641 (2026-05-07). The WM approval banner displayed
7+
"<action> by <analyst> —" with no reason text for column_removal /
8+
remove_csv / remove_rule requests because the projection dropped
9+
the queue entry's `comment` field on the way to the frontend, while
10+
the auto-generated `description` field is empty for those action
11+
types. The frontend code at `wl_approval_ui.js:405` reads
12+
`pa.comment || pa.description || ""`, so both falsy → blank banner.
13+
14+
The Control Panel was unaffected because it uses the
15+
`get_approval_queue` action which returns the queue verbatim with
16+
no projection.
17+
18+
These tests pin the projection's contract so the regression cannot
19+
silently re-occur if someone adds another endpoint that constructs
20+
a similar shape inline.
21+
"""
22+
23+
import os
24+
import sys
25+
26+
import pytest
27+
28+
_BIN = os.path.join(os.path.dirname(__file__), "..", "..", "bin")
29+
sys.path.insert(0, os.path.abspath(_BIN))
30+
31+
from wl_approval import project_pending_info # noqa: E402
32+
33+
34+
@pytest.fixture
35+
def column_removal_entry():
36+
"""Realistic queue entry for an analyst-submitted column_removal.
37+
38+
Auto-`description` is empty (handler convention for this
39+
action type). The free-form analyst reason lives in `comment`.
40+
"""
41+
return {
42+
"request_id": "68d12bbb-8f37-4106-84d9-a5d34bd87a92",
43+
"timestamp": 1778025844,
44+
"analyst": "analyst1",
45+
"csv_file": "DR130_priv_escalation.csv",
46+
"app_context": "wl_manager",
47+
"detection_rule": "DR130_privilege_escalation",
48+
"action_type": "column_removal",
49+
"description": "",
50+
"comment": "Field deprecated by GRC team",
51+
"status": "pending",
52+
"payload": {},
53+
"expected_mtime": None,
54+
"pending_highlight": {},
55+
"resolved_by": None,
56+
"resolved_at": None,
57+
"rejection_reason": None,
58+
}
59+
60+
61+
def test_column_removal_comment_propagates(column_removal_entry):
62+
"""The build-641 regression: `comment` must reach the frontend.
63+
64+
If this test fails the WM banner will render "column removal by
65+
analyst1 —" with nothing after the dash.
66+
"""
67+
out = project_pending_info(column_removal_entry, has_edit=True)
68+
assert out["comment"] == "Field deprecated by GRC team"
69+
assert "comment" in out
70+
71+
72+
def test_missing_comment_yields_empty_string(column_removal_entry):
73+
"""Resilience: queue entries written by older app versions may
74+
not have a `comment` field at all. Projection must default to
75+
empty string rather than KeyError or None (the frontend's
76+
`||` short-circuit treats both empty string and undefined as
77+
falsy, which is the desired behavior)."""
78+
del column_removal_entry["comment"]
79+
out = project_pending_info(column_removal_entry, has_edit=True)
80+
assert out["comment"] == ""
81+
82+
83+
def test_required_fields_always_present(column_removal_entry):
84+
"""Pin the eight contract fields the frontend banner consumes.
85+
86+
Adding fields is fine; removing one breaks the WM page silently
87+
(no JS error, just a missing banner element)."""
88+
out = project_pending_info(column_removal_entry, has_edit=True)
89+
expected = {
90+
"request_id", "action_type", "description", "comment",
91+
"analyst", "timestamp", "pending_highlight", "payload",
92+
}
93+
assert set(out.keys()) == expected
94+
95+
96+
def test_non_editor_cannot_see_payload(column_removal_entry):
97+
"""RBAC contract: non-editors get who/when/what but not the
98+
row-level highlight or payload (which can carry CSV row content
99+
that the requester is asking to add/remove)."""
100+
column_removal_entry["payload"] = {"sensitive_row_data": "secret"}
101+
column_removal_entry["pending_highlight"] = {"row_keys": ["a"]}
102+
out = project_pending_info(column_removal_entry, has_edit=False)
103+
assert out["payload"] == {}
104+
assert out["pending_highlight"] == {}
105+
# But the public fields still flow through
106+
assert out["analyst"] == "analyst1"
107+
assert out["comment"] == "Field deprecated by GRC team"
108+
109+
110+
def test_editor_sees_payload_and_highlight(column_removal_entry):
111+
"""Symmetric check: editor view exposes payload + highlight."""
112+
column_removal_entry["payload"] = {"col": "ticket_id"}
113+
column_removal_entry["pending_highlight"] = {"col": "ticket_id"}
114+
out = project_pending_info(column_removal_entry, has_edit=True)
115+
assert out["payload"] == {"col": "ticket_id"}
116+
assert out["pending_highlight"] == {"col": "ticket_id"}
117+
118+
119+
@pytest.mark.parametrize("action_type", [
120+
"column_removal", "remove_csv", "remove_rule",
121+
"bulk_row_removal", "bulk_row_addition", "revert",
122+
"csv_import_replace", "bulk_row_edit",
123+
"create_csv", "create_rule",
124+
])
125+
def test_comment_propagates_for_all_action_types(
126+
column_removal_entry, action_type):
127+
"""The `comment` projection must work for every action type the
128+
frontend banner can render."""
129+
column_removal_entry["action_type"] = action_type
130+
out = project_pending_info(column_removal_entry, has_edit=True)
131+
assert out["comment"] == "Field deprecated by GRC team"
132+
assert out["action_type"] == action_type

0 commit comments

Comments
 (0)