Skip to content

ref: Use getApiURl() inside of static/app/view/{dashboards,discover}/*#106738

Merged
ryan953 merged 2 commits intomasterfrom
ryan953/ref-getApiUrl-view-dashboards
Jan 23, 2026
Merged

ref: Use getApiURl() inside of static/app/view/{dashboards,discover}/*#106738
ryan953 merged 2 commits intomasterfrom
ryan953/ref-getApiUrl-view-dashboards

Conversation

@ryan953
Copy link
Member

@ryan953 ryan953 commented Jan 21, 2026

@ryan953 ryan953 requested review from a team as code owners January 21, 2026 22:59
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 21, 2026
projectIdOrSlug: projectSlug!,
eventId: eventId!,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: The code unsafely splits eventSlug without validating it contains a colon. A malformed URL can cause eventId to be undefined, leading to an invalid API request.
Severity: MEDIUM

Suggested Fix

Before splitting eventSlug, add a guard to validate that it contains a colon. If the format is invalid, handle the error gracefully instead of proceeding with an unsafe split and non-null assertion.

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/discover/eventDetails.tsx#L43

Potential issue: The `eventSlug` parameter from the URL is split by a colon to extract
`projectSlug` and `eventId`. However, there is no validation to ensure the slug contains
a colon. If a user navigates to a URL where `eventSlug` is missing a colon (e.g., a
malformed or manually entered URL), `eventSlug.split(':')` will result in `eventId`
being `undefined`. A non-null assertion `eventId!` is then used, which passes
`undefined` to `getApiUrl`. This constructs an invalid API endpoint like
`/organizations/.../events/project:undefined/`, leading to a 404 error.

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

Copy link
Member

Choose a reason for hiding this comment

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

IMO this is fine. Is the slug is malformed we've got bigger problems, and catching a Sentry exception here would be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take special care with this one, i think some decent type aware parsing will do the trick instead of letting it fail. it shouldn't be failing at all, just TS needs to know what's up

Copy link
Member Author

Choose a reason for hiding this comment

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

it's matching against: r"^(?P<organization_id_or_slug>[^/]+)/events/(?P<project_id_or_slug>[^/]+):(?P<event_id>(?:\d+|[A-Fa-f0-9-]{32,36}))/$",
https://github.com/getsentry/sentry/blob/master/src/sentry/api/urls.py#L1708

the colon is mandatory in there. the JS won't throw, the only thing that can happen is the response fails with 404.

That should be covered because we're properly looking at the error return value. I think this is as good as before.

Copy link
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

@ryan953 ryan953 requested a review from a team January 21, 2026 23:22
Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! There's one thing I agree with the robot on, and one thing I do not 👍🏻

projectIdOrSlug: projectSlug!,
eventId: eventId!,
},
}
Copy link
Member

Choose a reason for hiding this comment

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

IMO this is fine. Is the slug is malformed we've got bigger problems, and catching a Sentry exception here would be fine

@ryan953 ryan953 requested a review from gggritso January 23, 2026 00:07
@ryan953 ryan953 merged commit 89b74bc into master Jan 23, 2026
53 checks passed
@ryan953 ryan953 deleted the ryan953/ref-getApiUrl-view-dashboards branch January 23, 2026 17:24
JonasBa pushed a commit that referenced this pull request Jan 27, 2026
…}/* (#106738)

See: https://gist.github.com/ryan953/af5458fa4f89ae42cc19e8ea266b7e9a

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants