mirror of
https://github.com/Sea-Haven-Industries/afterhours-shift-manager.git
synced 2026-10-06 12:22:02 +00:00
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.
This commit is contained in:
parent
03ea8cbdd8
commit
401a561ae2
2 changed files with 48 additions and 3 deletions
|
|
@ -1197,6 +1197,27 @@ def handle_swap_accept(body, respond, client, schedule, schedule_channel):
|
||||||
if _holiday_window_active(date_str):
|
if _holiday_window_active(date_str):
|
||||||
_set_holiday_queue_agents(schedule, date_str)
|
_set_holiday_queue_agents(schedule, date_str)
|
||||||
else:
|
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
|
# 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
|
# weekly fallback). If it was independently claimed after the swap was
|
||||||
# initiated, abort instead of silently overwriting the new holder.
|
# initiated, abort instead of silently overwriting the new holder.
|
||||||
|
|
|
||||||
|
|
@ -42,8 +42,11 @@ def _channels(client):
|
||||||
class TestAccept:
|
class TestAccept:
|
||||||
@freeze_time(MON)
|
@freeze_time(MON)
|
||||||
def test_applies_override_marks_verified_notifies(
|
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")
|
_pending(schedule, "2026-06-01")
|
||||||
slackbot_app.handle_swap_accept(
|
slackbot_app.handle_swap_accept(
|
||||||
_accept_body("2026-06-01"), respond, client, schedule, "C_TEST"
|
_accept_body("2026-06-01"), respond, client, schedule, "C_TEST"
|
||||||
|
|
@ -58,8 +61,9 @@ class TestAccept:
|
||||||
|
|
||||||
@freeze_time(MON)
|
@freeze_time(MON)
|
||||||
def test_future_shift_no_3cx(
|
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
|
_pending(schedule, "2026-06-03") # Wednesday
|
||||||
slackbot_app.handle_swap_accept(
|
slackbot_app.handle_swap_accept(
|
||||||
_accept_body("2026-06-03"), respond, client, schedule, "C_TEST"
|
_accept_body("2026-06-03"), respond, client, schedule, "C_TEST"
|
||||||
|
|
@ -116,7 +120,27 @@ class TestAccept:
|
||||||
routing_spy.assert_not_called()
|
routing_spy.assert_not_called()
|
||||||
|
|
||||||
@freeze_time(MON)
|
@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")
|
_pending(schedule, "2026-06-06", shift_type="day")
|
||||||
slackbot_app.handle_swap_accept(
|
slackbot_app.handle_swap_accept(
|
||||||
_accept_body("2026-06-06", suffix="_day"),
|
_accept_body("2026-06-06", suffix="_day"),
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue