Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 34 additions & 28 deletions src/firetower/slack_app/handlers/mitigated.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
build_incident_lifecycle_modal,
build_incident_update_data,
get_incident_from_channel,
notify_submission_error,

Check warning on line 13 in src/firetower/slack_app/handlers/mitigated.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[RQB-88Y] `Tag.objects.create()` bypasses `clean()`, enabling concurrent case-variant duplicate tags (additional location)

The comment in `_resolve_tag_values` (utils.py) claims `Tag.clean()` guards case-insensitive uniqueness, but Django's `objects.create()` never calls `clean()`. Two concurrent inline-creation requests for the same name with different capitalizations (e.g. `"Foo"` and `"foo"`) both pass the `filter(type=tag_type, name__iexact=name).first()` pre-check before either commits, then both succeed at the DB level because `unique_together = [("name", "type")]` is case-sensitive, producing duplicate case-variant tags. The same-case concurrent path is covered by the DB constraint (raises IntegrityError), but the cross-case path is not.
parse_incident_form_values,
validate_lifecycle_form,
)
Expand Down Expand Up @@ -49,7 +50,8 @@


def handle_mitigated_submission(ack: Any, body: dict, view: dict, client: Any) -> None:
form = parse_incident_form_values(view)
# Parse without resolving tags so validation touches no DB before ack().
form = parse_incident_form_values(view, resolve_tags=False)
channel_id = view.get("private_metadata", "")

errors = validate_lifecycle_form(form)
Expand All @@ -59,39 +61,43 @@

ack()

incident = get_incident_from_channel(channel_id)
if not incident:
logger.error("Mitigated submission: no incident for channel %s", channel_id)
return
user_id = body["user"]["id"]

captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"])
if not captain_user:
logger.error(
"Could not resolve Slack user %s to a Firetower user",
form["captain_slack_id"],
)
client.chat_postMessage(
channel=channel_id,
text="Failed to resolve the selected captain to a Firetower user.",
# Work runs after ack() so the modal closes immediately; any failure is
# reported back to the submitter as an ephemeral message.
try:
incident = get_incident_from_channel(channel_id)
captain_user = (
get_or_create_user_from_slack_id(form["captain_slack_id"])
if incident
else None
)
return
if incident is None or captain_user is None:
notify_submission_error(client, channel_id, user_id)
return

data = build_incident_update_data(
form, IncidentStatus.MITIGATED, captain_user.email
)
# Resolve/create inline tags only after validation passed.
form = parse_incident_form_values(view)

acting_user = get_or_create_user_from_slack_id(body["user"]["id"])
serializer = IncidentWriteSerializer(
instance=incident, data=data, partial=True, context={"acting_user": acting_user}
)
if not serializer.is_valid():
logger.error("Mitigated update failed: %s", serializer.errors)
client.chat_postMessage(
channel=channel_id,
text=f"Failed to update incident: {serializer.errors}",
data = build_incident_update_data(
form, IncidentStatus.MITIGATED, captain_user.email
)
acting_user = get_or_create_user_from_slack_id(user_id)
serializer = IncidentWriteSerializer(
instance=incident,
data=data,
partial=True,
context={"acting_user": acting_user},
)
if not serializer.is_valid():
logger.error("Mitigated update failed: %s", serializer.errors)
notify_submission_error(client, channel_id, user_id)
return
serializer.save()
except Exception:
logger.exception("Failed to process mitigated submission")
notify_submission_error(client, channel_id, user_id)
return
serializer.save()

incident_url = f"{settings.FIRETOWER_BASE_URL}/{incident.incident_number}"
client.chat_postMessage(
Expand Down
37 changes: 32 additions & 5 deletions src/firetower/slack_app/handlers/new_incident.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
from firetower.integrations.services.slack import escape_slack_text
from firetower.slack_app.handlers.utils import (
_DEFAULT_SEVERITY,
CREATE_TAG_PREFIX,
_extract_title,
build_incident_form_blocks,
parse_incident_form_values,
)
Expand Down Expand Up @@ -170,6 +172,10 @@
"affected_region_tags": TagType.AFFECTED_REGION,
}

# Tag types users can create inline from the lifecycle modals. impact_type stays

Check warning on line 175 in src/firetower/slack_app/handlers/new_incident.py

View check run for this annotation

@sentry/warden / warden: find-bugs

`Tag.objects.create()` bypasses `clean()`, enabling concurrent case-variant duplicate tags

The comment in `_resolve_tag_values` (utils.py) claims `Tag.clean()` guards case-insensitive uniqueness, but Django's `objects.create()` never calls `clean()`. Two concurrent inline-creation requests for the same name with different capitalizations (e.g. `"Foo"` and `"foo"`) both pass the `filter(type=tag_type, name__iexact=name).first()` pre-check before either commits, then both succeed at the DB level because `unique_together = [("name", "type")]` is case-sensitive, producing duplicate case-variant tags. The same-case concurrent path is covered by the DB constraint (raises IntegrityError), but the cross-case path is not.
# curated since it powers dashboards, so it is intentionally excluded.
INLINE_CREATABLE_TAG_TYPES = {TagType.AFFECTED_SERVICE, TagType.AFFECTED_REGION}


def handle_tag_options(ack: Any, payload: dict) -> None:
close_old_connections()
Expand All @@ -185,11 +191,31 @@
if keyword:
qs = qs.filter(name__icontains=keyword)

matches = list(qs.order_by("name")[:100])
options = [
{"text": {"type": "plain_text", "text": tag.name}, "value": tag.name}
for tag in qs.order_by("name")[:100]
for tag in matches
]
ack(options=options)

stripped = keyword.strip()
if tag_type in INLINE_CREATABLE_TAG_TYPES and stripped:
exact_match = Tag.objects.filter(type=tag_type, name__iexact=stripped).exists()
if not exact_match:
# Put "+ Create" first so it survives the 100-option Slack cap even
# when the keyword already matches a full page of existing tags.
options.insert(
0,
{
"text": {
"type": "plain_text",
"text": f'+ Create "{stripped}"',
},
Comment thread
spalmurray marked this conversation as resolved.
"value": f"{CREATE_TAG_PREFIX}{stripped}",
},
)
Comment thread
cursor[bot] marked this conversation as resolved.

# Slack rejects block_suggestion responses with more than 100 options.
ack(options=options[:100])


def handle_new_command(ack: Any, body: dict, command: dict, respond: Any) -> None:
Expand Down Expand Up @@ -259,16 +285,15 @@
def handle_new_incident_submission(
ack: Any, body: dict, view: dict, client: Any
) -> None:
form = parse_incident_form_values(view)

values = view.get("state", {}).get("values", {})
private_selections = (
values.get("private_block", {}).get("is_private", {}).get("selected_options")
or []
)
is_private = any(opt.get("value") == "private" for opt in private_selections)

if not form["title"]:
title = _extract_title(view)
if not title:
ack(
response_action="errors",
errors={"title_block": "This field is required."},
Expand All @@ -279,12 +304,14 @@

slack_user_id = body.get("user", {}).get("id", "")

try:
form = parse_incident_form_values(view)
Comment thread
github-actions[bot] marked this conversation as resolved.
incident = _create_incident_via_db(form, slack_user_id, is_private, client)
except (OperationalError, InterfaceError):
logger.exception(
"Database unreachable during incident creation from Slack modal"

Check warning on line 312 in src/firetower/slack_app/handlers/new_incident.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[ST4-AWC] Inline tags created before serializer validation can be orphaned in the DB (additional location)

Tags are created by `parse_incident_form_values(view)` (the `resolve_tags=True` re-parse inside the `try` block) before `serializer.is_valid()` is called; if validation fails or any subsequent exception is caught, `Tag.objects.create()` calls have already committed and the tags persist with `approved=False` but are never associated with any incident.
)
form = parse_incident_form_values(view, resolve_tags=False)
form_data = {
"title": form["title"],
"severity": form["severity"] or _DEFAULT_SEVERITY.value,
Expand Down
74 changes: 40 additions & 34 deletions src/firetower/slack_app/handlers/resolved.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
build_incident_lifecycle_modal,
build_incident_update_data,
get_incident_from_channel,
notify_submission_error,

Check warning on line 11 in src/firetower/slack_app/handlers/resolved.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[RQB-88Y] `Tag.objects.create()` bypasses `clean()`, enabling concurrent case-variant duplicate tags (additional location)

The comment in `_resolve_tag_values` (utils.py) claims `Tag.clean()` guards case-insensitive uniqueness, but Django's `objects.create()` never calls `clean()`. Two concurrent inline-creation requests for the same name with different capitalizations (e.g. `"Foo"` and `"foo"`) both pass the `filter(type=tag_type, name__iexact=name).first()` pre-check before either commits, then both succeed at the DB level because `unique_together = [("name", "type")]` is case-sensitive, producing duplicate case-variant tags. The same-case concurrent path is covered by the DB constraint (raises IntegrityError), but the cross-case path is not.
parse_incident_form_values,
validate_lifecycle_form,
)
Expand Down Expand Up @@ -47,7 +48,8 @@


def handle_resolved_submission(ack: Any, body: dict, view: dict, client: Any) -> None:
form = parse_incident_form_values(view)
# Parse without resolving tags so validation touches no DB before ack().
form = parse_incident_form_values(view, resolve_tags=False)
channel_id = view.get("private_metadata", "")

errors = validate_lifecycle_form(form)
Expand All @@ -57,43 +59,47 @@

ack()

incident = get_incident_from_channel(channel_id)
if not incident:
logger.error("Resolved submission: no incident for channel %s", channel_id)
return
user_id = body["user"]["id"]

captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"])
if not captain_user:
logger.error(
"Could not resolve Slack user %s to a Firetower user",
form["captain_slack_id"],
# Work runs after ack() so the modal closes immediately; any failure is
# reported back to the submitter as an ephemeral message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concerned about hitting 3s ack timeout without this

try:
incident = get_incident_from_channel(channel_id)
captain_user = (
get_or_create_user_from_slack_id(form["captain_slack_id"])
if incident
else None
)
client.chat_postMessage(
channel=channel_id,
text="Failed to resolve the selected captain to a Firetower user.",
)
return

severity = form["severity"]
if severity in ("P0", "P1", "P2"):
target_status = IncidentStatus.POSTMORTEM
else:
target_status = IncidentStatus.DONE

data = build_incident_update_data(form, target_status, captain_user.email)

acting_user = get_or_create_user_from_slack_id(body["user"]["id"])
serializer = IncidentWriteSerializer(
instance=incident, data=data, partial=True, context={"acting_user": acting_user}
)
if not serializer.is_valid():
logger.error("Resolved update failed: %s", serializer.errors)
client.chat_postMessage(
channel=channel_id,
text=f"Failed to resolve incident: {serializer.errors}",
if incident is None or captain_user is None:
notify_submission_error(client, channel_id, user_id)
return

# Resolve/create inline tags only after validation passed.
form = parse_incident_form_values(view)

severity = form["severity"]
if severity in ("P0", "P1", "P2"):
target_status = IncidentStatus.POSTMORTEM
else:
target_status = IncidentStatus.DONE

data = build_incident_update_data(form, target_status, captain_user.email)
acting_user = get_or_create_user_from_slack_id(user_id)
serializer = IncidentWriteSerializer(
instance=incident,

Check warning on line 89 in src/firetower/slack_app/handlers/resolved.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[ST4-AWC] Inline tags created before serializer validation can be orphaned in the DB (additional location)

Tags are created by `parse_incident_form_values(view)` (the `resolve_tags=True` re-parse inside the `try` block) before `serializer.is_valid()` is called; if validation fails or any subsequent exception is caught, `Tag.objects.create()` calls have already committed and the tags persist with `approved=False` but are never associated with any incident.
data=data,
partial=True,
context={"acting_user": acting_user},
)
if not serializer.is_valid():
logger.error("Resolved update failed: %s", serializer.errors)
notify_submission_error(client, channel_id, user_id)
return
Comment thread
spalmurray marked this conversation as resolved.
serializer.save()
except Exception:
logger.exception("Failed to process resolved submission")
notify_submission_error(client, channel_id, user_id)
return
serializer.save()

client.chat_postMessage(
channel=channel_id,
Expand Down
81 changes: 43 additions & 38 deletions src/firetower/slack_app/handlers/update_incident.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from firetower.incidents.serializers import IncidentWriteSerializer
from firetower.slack_app.handlers.utils import (
get_incident_from_channel,
notify_submission_error,
parse_incident_form_values,
)

Expand Down Expand Up @@ -232,7 +233,8 @@
def handle_update_incident_submission(
ack: Any, body: dict, view: dict, client: Any
) -> None:
form = parse_incident_form_values(view)
# Parse without resolving tags so validation touches no DB before ack().
form = parse_incident_form_values(view, resolve_tags=False)
channel_id = view.get("private_metadata", "")

values = view.get("state", {}).get("values", {})
Expand All @@ -251,48 +253,51 @@

ack()

incident = get_incident_from_channel(channel_id)
if not incident:
logger.error("Update submission: no incident for channel %s", channel_id)
return

data: dict[str, Any] = {
"title": form["title"],
"description": form["description"],
"impact_summary": form["impact_summary"],
"is_private": is_private,
"impact_type_tags": form["impact_type_tags"],
"affected_service_tags": form["affected_service_tags"],
"affected_region_tags": form["affected_region_tags"],
}
if form["severity"]:
data["severity"] = form["severity"]

if form["captain_slack_id"]:
captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"])
if captain_user:
data["captain"] = captain_user.email

acting_user = get_or_create_user_from_slack_id(body["user"]["id"])
serializer = IncidentWriteSerializer(
instance=incident, data=data, partial=True, context={"acting_user": acting_user}
)
if not serializer.is_valid():
logger.error("Incident update validation failed: %s", serializer.errors)
client.chat_postMessage(
channel=channel_id,
text=f"Failed to update incident: {serializer.errors}",
)
return
user_id = body["user"]["id"]

# Work runs after ack() so the modal closes immediately; any failure is
# reported back to the submitter as an ephemeral message.
try:
incident = get_incident_from_channel(channel_id)
if incident is None:
notify_submission_error(client, channel_id, user_id)
return

# Resolve/create inline tags only after validation passed.
form = parse_incident_form_values(view)

data: dict[str, Any] = {
"title": form["title"],
"description": form["description"],
"impact_summary": form["impact_summary"],
"is_private": is_private,
"impact_type_tags": form["impact_type_tags"],
"affected_service_tags": form["affected_service_tags"],
"affected_region_tags": form["affected_region_tags"],
}
if form["severity"]:
data["severity"] = form["severity"]

if form["captain_slack_id"]:
captain_user = get_or_create_user_from_slack_id(form["captain_slack_id"])
if captain_user:
data["captain"] = captain_user.email

acting_user = get_or_create_user_from_slack_id(user_id)
serializer = IncidentWriteSerializer(
instance=incident,
data=data,
partial=True,
context={"acting_user": acting_user},
)
if not serializer.is_valid():
logger.error("Incident update validation failed: %s", serializer.errors)
notify_submission_error(client, channel_id, user_id)
return

Check warning on line 296 in src/firetower/slack_app/handlers/update_incident.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Inline tags created before serializer validation can be orphaned in the DB

Tags are created by `parse_incident_form_values(view)` (the `resolve_tags=True` re-parse inside the `try` block) before `serializer.is_valid()` is called; if validation fails or any subsequent exception is caught, `Tag.objects.create()` calls have already committed and the tags persist with `approved=False` but are never associated with any incident.
serializer.save()
except Exception:
logger.exception("Failed to update incident from Slack modal")
client.chat_postMessage(
channel=channel_id,
text="Something went wrong updating the incident. Please try again.",
)
notify_submission_error(client, channel_id, user_id)
return

client.chat_postMessage(
Expand Down
Loading
Loading