fix(aci): Selected project state should react to form changes#112434
fix(aci): Selected project state should react to form changes#112434
Conversation
| const project = projects.find(p => p.id === projectId); | ||
| if (!project) { | ||
| throw new Error( | ||
| `useDetectorFormProject: no project found for projectId "${projectId}"` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: The useDetectorFormProject hook throws an unhandled error if no project is found, crashing the component when an organization has zero projects or the projectId is invalid.
Severity: HIGH
Suggested Fix
Instead of throwing an error from the useDetectorFormProject hook, return undefined. Components using this hook should then check for an undefined project and render an appropriate error state, similar to the previous implementation which showed a <LoadingError> component.
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:
static/app/views/detectors/components/forms/common/useDetectorFormProject.tsx#L14-L19
Potential issue: The new `useDetectorFormProject` hook throws an unhandled error if a
project is not found for a given `projectId`. This occurs in realistic scenarios, such
as when a user in an organization with zero projects attempts to create a new detector.
In this case, `projectId` defaults to an empty string, `projects.find()` returns
`undefined`, and the hook throws an error that crashes the component. The previous
implementation handled this case gracefully by displaying a 'Project not found' message,
but this error handling was removed in the refactor.
Did we get this right? 👍 / 👎 to inform future reviews.
|
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
| result = Spans.run_table_query( | ||
| params=snuba_params, | ||
| query_string=build_escaped_term_filter("gen_ai.conversation.id", [conversation_id]), | ||
| query_string=f"gen_ai.conversation.id:{conversation_id}", |
There was a problem hiding this comment.
Unescaped conversation_id in Snuba query can cause incorrect results or parsing errors
The change removes proper value escaping from build_escaped_term_filter and uses direct f-string interpolation: f"gen_ai.conversation.id:{conversation_id}". If conversation_id (user-provided via URL path) contains spaces, the query parses as gen_ai.conversation.id:first_word second_word where second_word becomes unrelated free-text search, returning incorrect results silently. Parentheses cause parsing errors. While handle_query_errors() catches InvalidSearchQuery, silent wrong results are not caught. The old code quoted and escaped values to prevent this.
Verification
Read build_escaped_term_filter in query_utils.py (lines 15-24) - it escapes backslashes/quotes and wraps in double quotes. Checked search grammar in event_search.py line 166 - unquoted value = ~r"[^()\t\n ]*" means spaces/parens break parsing. Confirmed handle_query_errors() at line 110 catches exceptions but cannot detect semantically wrong queries from space-split values.
Identified by Warden sentry-backend-bugs · JZN-EWH
| if dataset != Dataset.EventsAnalyticsPlatform: | ||
| self._validate_snql_query(data, entity_subscription, projects) |
There was a problem hiding this comment.
EAP time window validation removed for MetricIssueDetectorValidator path
The removed call to _validate_time_window for Dataset.EventsAnalyticsPlatform was the only validation enforcing the 5-minute minimum time window when SnubaQueryValidator is used directly (via MetricIssueDetectorValidator). While AlertRuleSerializer has its own duplicate validation at lines 214-219, MetricIssueDetectorValidator at metric_issue_detector.py:194 uses SnubaQueryValidator(timeWindowSeconds=True) as a child serializer without any time window validation. This allows creating EAP metric issue detectors with invalid sub-5-minute time windows, which will cause subscription errors at runtime.
Verification
Read SnubaQueryValidator._validate_time_window (lines 384-398) which enforces 300-second minimum for EAP. Read AlertRuleSerializer.validate (lines 214-219) which has duplicate validation. Read MetricIssueDetectorValidator (metric_issue_detector.py:192-223) which uses SnubaQueryValidator directly without adding time window validation. Confirmed no other validation for EAP time windows via grep.
Identified by Warden sentry-backend-bugs · UJ5-PFJ
| uses: getsentry/craft/.github/workflows/changelog-preview.yml@f4889d04564e47311038ecb6b910fef6b6cf1363 # v2 | ||
| with: | ||
| comment: false | ||
| secrets: inherit |
There was a problem hiding this comment.
pull_request_target with secrets: inherit passes all secrets to external reusable workflow
This workflow uses pull_request_target trigger with secrets: inherit, which passes all repository secrets to the reusable workflow getsentry/craft/.github/workflows/changelog-preview.yml. If that external workflow checks out fork code (PR head ref) and executes it, an attacker could exfiltrate secrets by opening a malicious PR. The workflow also grants elevated permissions (contents: write, pull-requests: write, statuses: write) which amplifies potential impact.
Verification
Verified that workflow uses pull_request_target trigger (line 3), has secrets: inherit (line 21), and grants write permissions (lines 11-14). Cannot trace execution path into external workflow getsentry/craft/.github/workflows/changelog-preview.yml - the reusable workflow is SHA-pinned which is good practice, but the security ultimately depends on whether that workflow checks out fork code. Marking as MEDIUM confidence since the full attack path cannot be confirmed without access to the external workflow.
Identified by Warden gha-security-review · 2HA-UUB
Closes ISWF-2330