From 43bfbf34c2a8903afe5458be46a98bc196d550f0 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 17 Jun 2026 11:33:27 -0400 Subject: [PATCH] Fix auth and race-condition flaws in shift commands Four confirmed findings from the 2026-06-17 security sweep: - register_user let any Slack user overwrite an extension already bound to a different user (account takeover). Add a DynamoDB ConditionExpression so a write only succeeds when the extension is unclaimed or already this user's; raise ExtensionAlreadyRegistered otherwise and surface a clear Slack message. - The `rate` subcommand was routed without the is_admin flag, so any user could set $0 pay rates. Gate _handle_rate on is_admin, matching the admin-command guard. - `/oncall pick` used a plain put_item (TOCTOU): two concurrent picks both won. Use the atomic claim_open_shift conditional claim so the loser gets an "already picked up" message. - swap-accept overwrote a shift independently claimed after the swap was initiated. Add reassign_if_held_by, a conditional write that only applies the swap while the override is still the requester's (or on the weekly fallback), and notify the accepter otherwise. Add tests for the register-ownership guard and the rate admin guard. Refs: INFRA --- src/shared/shared/schedule.py | 69 ++++++++++++++++++-- src/slack-bot/app.py | 62 ++++++++++++++++-- tests/slack_bot/test_handle_register_rate.py | 44 ++++++++++--- 3 files changed, 156 insertions(+), 19 deletions(-) diff --git a/src/shared/shared/schedule.py b/src/shared/shared/schedule.py index 4adad38..2d792a5 100644 --- a/src/shared/shared/schedule.py +++ b/src/shared/shared/schedule.py @@ -21,6 +21,15 @@ EASTERN = ZoneInfo("America/New_York") WEEKEND_DAYS = {"Saturday", "Sunday"} FALLBACK_EXTENSION = "100" + +class ExtensionAlreadyRegistered(Exception): + """Raised when a Slack user tries to claim an extension owned by another.""" + + def __init__(self, extension: str): + self.extension = extension + super().__init__(f"Extension {extension} is already registered to another user") + + # Defaults used when CONFIG omits the holiday-feature settings. DEFAULT_HOLIDAY_MULTIPLIER = Decimal("1.5") DEFAULT_HOLIDAY_QUEUE = "802" @@ -62,14 +71,32 @@ class ShiftSchedule: return None def register_user(self, slack_user_id: str, extension: str) -> dict | None: + """Link a Slack user to a roster extension. + + Returns the employee on success, ``None`` if the extension is not in the + roster, and raises :class:`ExtensionAlreadyRegistered` if the extension + is already bound to a *different* Slack user. Re-registering your own + extension is a no-op success, so this is safe to call idempotently. + """ employee = self.get_employee_by_extension(extension) if not employee: return None - self.table.update_item( - Key={"PK": "ROSTER", "SK": extension}, - UpdateExpression="SET slack_user_id = :sid", - ExpressionAttributeValues={":sid": slack_user_id}, - ) + try: + self.table.update_item( + Key={"PK": "ROSTER", "SK": extension}, + UpdateExpression="SET slack_user_id = :sid", + # Only allow the write when the extension is unclaimed + # (no/empty slack_user_id) or already claimed by this same user. + # Blocks one Slack user from hijacking another's extension. + ConditionExpression=( + "attribute_not_exists(slack_user_id) " + "OR slack_user_id = :empty " + "OR slack_user_id = :sid" + ), + ExpressionAttributeValues={":sid": slack_user_id, ":empty": ""}, + ) + except self.table.meta.client.exceptions.ConditionalCheckFailedException: + raise ExtensionAlreadyRegistered(extension) from None employee["slack_user_id"] = slack_user_id return employee @@ -123,6 +150,38 @@ class ShiftSchedule: except self.table.meta.client.exceptions.ConditionalCheckFailedException: return False + def reassign_if_held_by( + self, + date_str: str, + from_extension: str, + extension: str, + name: str, + shift_type: str = "night", + ) -> bool: + """Atomically reassign a shift only if it's still ``from_extension``'s. + + Used by the swap-accept path: the shift moves to the target only when + the override is still in the requester's name, or when no override + exists yet (weekly-schedule fallback, i.e. still the requester's by + default). Fails if the shift was independently claimed (e.g. a pickup) + by someone else after the swap was initiated. + """ + sk = f"{date_str}-DAY" if shift_type == "day" else date_str + try: + self.table.put_item( + Item={ + "PK": "OVERRIDE", + "SK": sk, + "extension": extension, + "name": name, + }, + ConditionExpression="attribute_not_exists(PK) OR extension = :from", + ExpressionAttributeValues={":from": from_extension}, + ) + return True + except self.table.meta.client.exceptions.ConditionalCheckFailedException: + return False + def mark_open(self, date_str: str, shift_type: str = "night") -> None: sk = f"{date_str}-DAY" if shift_type == "day" else date_str self.table.put_item( diff --git a/src/slack-bot/app.py b/src/slack-bot/app.py index cdbddba..64e31dc 100644 --- a/src/slack-bot/app.py +++ b/src/slack-bot/app.py @@ -36,6 +36,7 @@ from shared.ring_scheduler import update_queue_routing from shared.schedule import ( FALLBACK_EXTENSION, WEEKEND_DAYS, + ExtensionAlreadyRegistered, ShiftSchedule, determine_shift_type, ) @@ -383,7 +384,7 @@ def dispatch_oncall(command, respond, client, schedule, schedule_channel): elif text == "pay": _show_pay(respond, schedule) elif text.startswith("rate"): - _handle_rate(respond, schedule, text) + _handle_rate(respond, schedule, text, is_admin) elif text.startswith("register"): _handle_register(respond, schedule, user_id, text) elif text.startswith("pick"): @@ -554,7 +555,13 @@ def _show_pay(respond, schedule): ) -def _handle_rate(respond, schedule, text): +def _handle_rate(respond, schedule, text, is_admin): + # Setting pay rates is an admin-only operation; reading them is gated too + # since rates are sensitive payroll data. + if not is_admin: + respond(text="Rate commands are restricted. Contact an administrator.") + return + parts = text.split() # /oncall rate — show current rates if len(parts) == 1: @@ -625,7 +632,16 @@ def _handle_register(respond, schedule, user_id, text): return ext = parts[1].strip() - employee = schedule.register_user(user_id, ext) + try: + employee = schedule.register_user(user_id, ext) + except ExtensionAlreadyRegistered: + respond( + text=( + f"Extension {ext} is already registered to another person. " + "If this is your extension, ask an admin to clear it." + ) + ) + return if not employee: respond( text=f"Extension {ext} not found in the roster. Check `/oncall roster`." @@ -761,7 +777,24 @@ def _pick_regular( ) return - schedule.set_override(date_str, employee["extension"], employee["name"], shift_type) + # Already this employee's own shift — nothing to claim, just confirm. + already_mine = ( + bool(assignees) and assignees[0]["extension"] == employee["extension"] + ) + if not already_mine: + # Atomic conditional claim: only one of two concurrent pickers wins, so + # the loser is told it's taken instead of silently overwriting (TOCTOU). + claimed = schedule.claim_open_shift( + date_str, employee["extension"], employee["name"], shift_type + ) + if not claimed: + respond( + text=( + f"The *{date_label}*{shift_label} shift was just picked up by " + "someone else." + ) + ) + return if is_today(date_str) and _is_active_shift_type(shift_type): _update_3cx_routing(employee["extension"]) @@ -1164,9 +1197,26 @@ def handle_swap_accept(body, respond, client, schedule, schedule_channel): if _holiday_window_active(date_str): _set_holiday_queue_agents(schedule, date_str) else: - schedule.set_override( - date_str, swap["target_ext"], swap["target_name"], shift_type + # 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. + moved = schedule.reassign_if_held_by( + date_str, + swap["requester_ext"], + swap["target_ext"], + swap["target_name"], + shift_type, ) + if not moved: + schedule.clear_swap(date_str, shift_type) + respond( + replace_original=True, + blocks=build_swap_resolved_blocks( + "This shift was already picked up by someone else, so the " + "swap couldn't be applied." + ), + ) + return if is_today(date_str) and _is_active_shift_type(shift_type): _update_3cx_routing(swap["target_ext"]) schedule.mark_swap_verified(date_str, shift_type) diff --git a/tests/slack_bot/test_handle_register_rate.py b/tests/slack_bot/test_handle_register_rate.py index b262f7b..ef97a9a 100644 --- a/tests/slack_bot/test_handle_register_rate.py +++ b/tests/slack_bot/test_handle_register_rate.py @@ -18,47 +18,75 @@ class TestRegister: slackbot_app._handle_register(respond, schedule, "U_ALICE", "register") assert "Usage" in text_of(respond) + def test_register_reregister_self_is_idempotent( + self, slackbot_app, schedule, seed, respond, text_of + ): + seed.roster("114", "Alice") + slackbot_app._handle_register(respond, schedule, "U_ALICE", "register 114") + slackbot_app._handle_register(respond, schedule, "U_ALICE", "register 114") + assert "Linked" in text_of(respond) + assert schedule.get_employee_by_extension("114")["slack_user_id"] == "U_ALICE" + + def test_register_cannot_hijack_others_extension( + self, slackbot_app, schedule, seed, respond, text_of + ): + seed.roster("114", "Alice") + # Alice claims her extension first. + slackbot_app._handle_register(respond, schedule, "U_ALICE", "register 114") + # Mallory tries to claim Alice's extension — must be rejected. + slackbot_app._handle_register(respond, schedule, "U_MALLORY", "register 114") + assert "already registered" in text_of(respond).lower() + # The binding must still point at Alice, not Mallory. + assert schedule.get_employee_by_extension("114")["slack_user_id"] == "U_ALICE" + class TestRate: + def test_non_admin_rejected(self, slackbot_app, schedule, seed, respond, text_of): + seed.roster("114", "Alice") + slackbot_app._handle_rate(respond, schedule, "rate 114 90", False) + assert "restricted" in text_of(respond).lower() + # The rate must not have been changed by a non-admin. + assert schedule.get_shift_rate("114") == 0.0 + def test_show_rates(self, slackbot_app, schedule, seed, respond, text_of): seed.config(shift_rate="50") seed.roster("114", "Alice", shift_rate="75") - slackbot_app._handle_rate(respond, schedule, "rate") + slackbot_app._handle_rate(respond, schedule, "rate", True) text = text_of(respond) assert "$50.00" in text and "Alice" in text and "$75.00" in text def test_show_rates_no_custom(self, slackbot_app, schedule, seed, respond, text_of): seed.config(shift_rate="50") - slackbot_app._handle_rate(respond, schedule, "rate") + slackbot_app._handle_rate(respond, schedule, "rate", True) assert "No per-person rates" in text_of(respond) def test_set_default(self, slackbot_app, schedule, seed, respond, text_of): seed.config(shift_rate="50") - slackbot_app._handle_rate(respond, schedule, "rate default 60") + slackbot_app._handle_rate(respond, schedule, "rate default 60", True) assert schedule.get_shift_rate() == 60.0 assert "$60.00" in text_of(respond) def test_set_default_invalid_amount(self, slackbot_app, schedule, respond, text_of): - slackbot_app._handle_rate(respond, schedule, "rate default abc") + slackbot_app._handle_rate(respond, schedule, "rate default abc", True) assert "Invalid amount" in text_of(respond) def test_set_default_usage(self, slackbot_app, schedule, respond, text_of): - slackbot_app._handle_rate(respond, schedule, "rate default") + slackbot_app._handle_rate(respond, schedule, "rate default", True) assert "Usage" in text_of(respond) def test_set_per_employee(self, slackbot_app, schedule, seed, respond, text_of): seed.roster("114", "Alice") - slackbot_app._handle_rate(respond, schedule, "rate 114 90") + slackbot_app._handle_rate(respond, schedule, "rate 114 90", True) assert schedule.get_shift_rate("114") == 90.0 assert "Alice" in text_of(respond) and "$90.00" in text_of(respond) def test_set_per_employee_unknown(self, slackbot_app, schedule, respond, text_of): - slackbot_app._handle_rate(respond, schedule, "rate 999 90") + slackbot_app._handle_rate(respond, schedule, "rate 999 90", True) assert "not found" in text_of(respond).lower() def test_set_per_employee_strips_dollar_sign( self, slackbot_app, schedule, seed, respond ): seed.roster("114", "Alice") - slackbot_app._handle_rate(respond, schedule, "rate 114 $90") + slackbot_app._handle_rate(respond, schedule, "rate 114 $90", True) assert schedule.get_shift_rate("114") == 90.0