fix: Avoid IndexError in match_regex_list() - #6896
fix: Avoid IndexError in match_regex_list()#6896alexander-alderman-webb wants to merge 1 commit into
IndexError in match_regex_list()#6896Conversation
| if not substring_matching and (not item_matcher or item_matcher[-1] != "$"): | ||
| item_matcher += "$" |
There was a problem hiding this comment.
Bug: The fix for an IndexError causes an empty string in regex_list to be treated as a pattern that matches everything, leading to unintended consequences.
Severity: HIGH
Suggested Fix
The function match_regex_list should handle empty strings in regex_list explicitly. Instead of converting "" to "$", it should either skip empty strings (possibly with a warning) or only match them against empty input strings. This will prevent the unintended "match-all" behavior.
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: sentry_sdk/utils.py#L1739-L1740
Potential issue: The change in `match_regex_list` prevents an `IndexError` when an item
in `regex_list` is an empty string. However, it introduces a new issue. An empty string
pattern `""` is converted to `"$"` which, when used with `re.search`, matches the end of
any string. Consequently, if a configuration list like `trace_propagation_targets` or
`exclude_beat_tasks` contains an empty string, `match_regex_list` will return `True` for
every input. This could lead to all HTTP requests having trace propagation enabled, or
all Celery beat tasks being excluded from monitoring, failing silently instead of
crashing.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
that's what I would expect with an empty regex
Codecov Results 📊✅ 98739 passed | ❌ 2 failed | ⏭️ 6806 skipped | Total: 105547 | Pass Rate: 93.55% | Execution Time: 359m 31s 📊 Comparison with Base Branch
➕ New Tests (2)View new tests
➖ Removed Tests (1)View removed tests
❌ Failed Tests
|
Chenghao999
left a comment
There was a problem hiding this comment.
Good minimal fix for the crash. One semantic point to consider: with this change an empty matcher becomes "$", and re.search("$", item) matches every string — so if regex_list ever contains an empty pattern, match_regex_list will now return True for any item instead of crashing. If an empty pattern should be ignored rather than match everything, an explicit if not item_matcher: continue would express that intent.
I also checked the substring_matching path: it only appends/reads the tail, no equivalent item_matcher[0] access, so this is the only crash site. LGTM otherwise.
Description
This approach still allows matching empty strings.
Issues
Closes #6504
Reminders
uv run ruff.feat:,fix:,ref:,meta:)