From 401a561ae2385d2d9ea08c6eb1a96e84d8c22de4 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 17 Jun 2026 12:17:43 -0400 Subject: [PATCH] fix: re-validate shift holder on swap-accept (sh-security-review RIHB-1) reassign_if_held_by trusted 'no override row' as 'still the requester's', but a weekly-held shift also has no override row. An admin clear or weekly edit between swap-init and accept could move the shift to a third party with no override, letting the accept steal it (CWE-367, confirmed HIGH). Re-resolve the current holder at accept and abort if it is no longer the requester. Adds regression test + seeds the holder in existing accept tests. --- src/slack-bot/app.py | 21 +++++++++++++++ tests/slack_bot/test_swap_accept_decline.py | 30 ++++++++++++++++++--- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/slack-bot/app.py b/src/slack-bot/app.py index 64e31dc..bdd4cd6 100644 --- a/src/slack-bot/app.py +++ b/src/slack-bot/app.py @@ -1197,6 +1197,27 @@ def handle_swap_accept(body, respond, client, schedule, schedule_channel): if _holiday_window_active(date_str): _set_holiday_queue_agents(schedule, date_str) else: + # Re-resolve the current holder at accept time. reassign_if_held_by + # below trusts "no override row" as "still the requester's", but a + # weekly-held shift also has no override row — so an admin clear or a + # weekly-schedule edit between swap-init and accept could move the shift + # to a third party without ever creating an override, and the bare + # conditional write would not catch it. Confirm the requester is still + # the resolved holder before reassigning. + accept_day_name = datetime.strptime(date_str, "%Y-%m-%d").strftime("%A") + current_ext, _, _ = schedule.resolve_shift( + date_str, accept_day_name, shift_type + ) + if current_ext != swap["requester_ext"]: + schedule.clear_swap(date_str, shift_type) + respond( + replace_original=True, + blocks=build_swap_resolved_blocks( + "This shift is no longer assigned to the person who " + "requested the swap, so it couldn't be applied." + ), + ) + return # Only move the shift if it's still the requester's (or still on the # weekly fallback). If it was independently claimed after the swap was # initiated, abort instead of silently overwriting the new holder. diff --git a/tests/slack_bot/test_swap_accept_decline.py b/tests/slack_bot/test_swap_accept_decline.py index 6498ee6..0d779e3 100644 --- a/tests/slack_bot/test_swap_accept_decline.py +++ b/tests/slack_bot/test_swap_accept_decline.py @@ -42,8 +42,11 @@ def _channels(client): class TestAccept: @freeze_time(MON) def test_applies_override_marks_verified_notifies( - self, slackbot_app, schedule, respond, client, routing_spy + self, slackbot_app, schedule, seed, respond, client, routing_spy ): + # Requester holds the shift via the weekly schedule (production + # invariant: swap-create validates the requester is the holder). + seed.weekly("Monday", "114", "Alice") _pending(schedule, "2026-06-01") slackbot_app.handle_swap_accept( _accept_body("2026-06-01"), respond, client, schedule, "C_TEST" @@ -58,8 +61,9 @@ class TestAccept: @freeze_time(MON) def test_future_shift_no_3cx( - self, slackbot_app, schedule, respond, client, routing_spy + self, slackbot_app, schedule, seed, respond, client, routing_spy ): + seed.weekly("Wednesday", "114", "Alice") _pending(schedule, "2026-06-03") # Wednesday slackbot_app.handle_swap_accept( _accept_body("2026-06-03"), respond, client, schedule, "C_TEST" @@ -116,7 +120,27 @@ class TestAccept: routing_spy.assert_not_called() @freeze_time(MON) - def test_weekend_day_suffix(self, slackbot_app, schedule, respond, client): + def test_aborts_if_requester_no_longer_holder( + self, slackbot_app, schedule, seed, respond, client, routing_spy + ): + # RIHB-1 regression: requester held via weekly, swap pending to Bob, + # then the holder changes to Carol (e.g. weekly edit / admin reassign) + # before Bob accepts. The accept must NOT steal the shift from Carol. + seed.weekly("Monday", "114", "Alice") + _pending(schedule, "2026-06-01") + # Holder is now Carol (116), not the requester Alice (114). + seed.weekly("Monday", "116", "Carol") + slackbot_app.handle_swap_accept( + _accept_body("2026-06-01"), respond, client, schedule, "C_TEST" + ) + # No override written to Bob; the swap is cleared and 3CX untouched. + assert schedule.get_override("2026-06-01") is None + assert "no longer assigned" in _resolved_text(respond).lower() + routing_spy.assert_not_called() + + @freeze_time(MON) + def test_weekend_day_suffix(self, slackbot_app, schedule, seed, respond, client): + seed.weekly("Saturday", "114", "Alice", shift_type="day") _pending(schedule, "2026-06-06", shift_type="day") slackbot_app.handle_swap_accept( _accept_body("2026-06-06", suffix="_day"),