Skip to content

Conversation

@lobsterkatie
Copy link
Member

This fixes a bug caused by #103268, which moved server-side fingerprinting before the call to normalize_stacktraces_for_grouping. While it's 99% true that they're unrelated processes (and therefore can happen in any order), the one exception to that is the handling of custom fingerprints which also include title information. In cases where the custom title includes any of the frame variables (function, module, package, or abs_path), the frame that gets used is the top in-app frame in the stacktrace. But in-app rules are applied as part of normalize_stacktraces_for_grouping, so having it not run until after the custom title is set is obviously a problem.

To fix this, the handling of such titles (in other words, the filling-in of the variables and adding of the result to the event) has been moved to live alongside the filling-in-of-the-variables which we do for the fingerprint itself, which is after in-app rules have been applied. A snapshot test illustrating the fix has also been added, showing that it's not the top frame but the top in-app frame which is used for the title.

(Why didn't I just switch the order of the two operations back, you ask? Because switching the order was the first step towards absorbing the normalization into variant calculation call which comes immediately after it, so server-side fingerprinting needed to get out of its original spot between them.)

@lobsterkatie lobsterkatie force-pushed the kmclb-fix-fingerprint-title-bug branch from fa97894 to d3e95f8 Compare November 15, 2025 21:59
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 15, 2025
@semgrep-code-getsentry
Copy link

Semgrep found 1 ssc-e23b0a49-86b3-48bc-b82c-03656ca7eaee finding:

  • src/sentry/workflow_engine/processors/detector.py

Risk: Affected versions of django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). The ORM methods QuerySet.filter(), QuerySet.exclude(), QuerySet.get() and the Q() class can be tricked into SQL injection when you pass a specially crafted dictionary via **kwargs that includes a malicious _connector entry. This bypasses the normal query parameterization and lets an attacker inject arbitrary SQL into the WHERE clause.

Fix: Upgrade this library to at least version 5.2.8 at sentry/uv.lock:305.

Reference(s): GHSA-frmv-pr5f-9mcr, CVE-2025-64459

@codecov
Copy link

codecov bot commented Nov 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #103425      +/-   ##
===========================================
- Coverage   80.78%    80.67%   -0.12%     
===========================================
  Files        9248      9243       -5     
  Lines      397172    394934    -2238     
  Branches    25158     25158              
===========================================
- Hits       320841    318596    -2245     
- Misses      75884     75891       +7     
  Partials      447       447              

@lobsterkatie lobsterkatie marked this pull request as ready for review November 15, 2025 22:59
@lobsterkatie lobsterkatie requested a review from a team as a code owner November 15, 2025 22:59
title to the event.
"""
custom_title_template = get_path(fingerprint_info, "matched_rule", "attributes", "title")
event_data = event.data.data
Copy link

Choose a reason for hiding this comment

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

Bug: Incorrectly accessing event.data.data instead of event.data on a NodeData object, leading to an AttributeError.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The _apply_custom_title_if_needed() function attempts to access event.data.data. However, event.data is already a NodeData object, which directly implements MutableMapping[str, Any]. NodeData does not have a nested .data attribute. This will cause an AttributeError at runtime when a custom fingerprint rule with a title template containing frame variables is processed, preventing custom titles from being applied and breaking a core feature of the pull request.

💡 Suggested Fix

Change event.data.data to event.data in the _apply_custom_title_if_needed() function, as event.data is already the desired mapping.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/grouping/api.py#L464

Potential issue: The `_apply_custom_title_if_needed()` function attempts to access
`event.data.data`. However, `event.data` is already a `NodeData` object, which directly
implements `MutableMapping[str, Any]`. `NodeData` does not have a nested `.data`
attribute. This will cause an `AttributeError` at runtime when a custom fingerprint rule
with a title template containing frame variables is processed, preventing custom titles
from being applied and breaking a core feature of the pull request.

Did we get this right? 👍 / 👎 to inform future reviews.

Reference_id: 2707029

Copy link
Member Author

Choose a reason for hiding this comment

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

This is sort of right, in that event.data could also be used (and is simpler, so I might as well switch to it), but it's wrong about event.data.data not being a thing. In fact, not only is event.data.data a thing, but event.data.data.data is also a thing.

Data storage in events is complicated. The inner containers go by various names:

image

Also, there are the aforementioned multiple layers, each of which has a different type:

image

And finally, both NodeData and EventDict have overloaded dict-type setting and getting to reach though the layers to the innermost dict, so using the first, second, or third level of .datas is functionally the same:

image

I used event.data.data because that's what was used in the code I moved, but the robot is right that it could be simplified to event.data, so I'll do that.

@lobsterkatie lobsterkatie merged commit 39ba245 into master Nov 17, 2025
95 of 97 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-fix-fingerprint-title-bug branch November 17, 2025 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants