fix(eco): Re-adds the Complete Installation button to integration config pages"#113076
Conversation
github-actions
Bot
added
the
Scope: Frontend
| }; | ||
|
|
||
| afterEach(() => { | ||
| window.history.replaceState({}, '', '/'); |
There was a problem hiding this comment.
You also shouldn't need this anymore
| const hasLinkedProjects = value.length > 0; | ||
|
|
||
| return ( | ||
| <Flex align="center" justify="between" gap="md"> |
There was a problem hiding this comment.
You may want to check what this looks like with a small window size to make sure the text and button still look right.
There was a problem hiding this comment.
Just updated the description with a new screenshot showing the mobile version. Looks like things wrap correctly.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return null; | ||
| } | ||
|
|
||
| if (!nextUrl.startsWith(nextButton.allowedDomain)) { |
There was a problem hiding this comment.
Bug: The startsWith() check for the next URL parameter is insufficient, creating an open redirect vulnerability. It allows redirects to attacker-controlled subdomains like https://vercel.com.evil.com.
Severity: HIGH
Suggested Fix
Parse the nextUrl using the URL constructor and perform an exact match on the hostname property against the allowed domain. For example: const parsedUrl = new URL(nextUrl); if (parsedUrl.hostname !== 'vercel.com') { ... }.
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/components/backendJsonFormAdapter/projectMapperAdapter.tsx#L247
Potential issue: The code uses `nextUrl.startsWith(nextButton.allowedDomain)` to
validate a redirect URL from the `next` query parameter. This check is insufficient as
it allows redirects to attacker-controlled subdomains, such as
`https://vercel.com.evil.com`, because it only checks the beginning of the string. An
attacker could craft a phishing link with a malicious `next` parameter, causing users
who click the "Complete on Vercel" button to be redirected to an attacker's site,
creating an open redirect vulnerability.
Did we get this right? 👍 / 👎 to inform future reviews.
| priority="primary" | ||
| icon={<IconOpen />} | ||
| disabled={!hasLinkedProjects} | ||
| href={nextUrl} | ||
| tooltipProps={{ | ||
| title: t('Please link at least one project to continue.'), | ||
| disabled: hasLinkedProjects, | ||
| }} | ||
| > | ||
| {nextButton.text} | ||
| </LinkButton> |
There was a problem hiding this comment.
Bug: The LinkButton receives both disabled and href props. If the component doesn't remove the href when disabled, users can still navigate to the link, bypassing the restriction.
Severity: MEDIUM
Suggested Fix
Conditionally render the href prop so it is undefined when the button is disabled. Alternatively, switch to using the internal LinkButton component which is known to handle this case correctly by removing the href when disabled.
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/components/backendJsonFormAdapter/projectMapperAdapter.tsx#L257-L269
Potential issue: The `LinkButton` component from the external `@sentry/scraps/button`
package is passed both an `href` and a `disabled` prop. It is uncertain whether this
external component removes the `href` attribute when the button is disabled. If it does
not, a user could bypass the disabled state by right-clicking the button and selecting
"Open in new tab", allowing them to navigate to the URL when they should not be able to.
The associated tests do not verify that the `href` is absent when the button is
disabled.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 9edec79. Configure here.
| return null; | ||
| } | ||
|
|
||
| if (!nextUrl.startsWith(nextButton.allowedDomain)) { |
There was a problem hiding this comment.
Domain validation bypassable via subdomain prefix matching
High Severity
The startsWith check for allowedDomain validation can be bypassed. If allowedDomain is https://vercel.com, a URL like https://vercel.com.evil.com/phishing also passes the check, enabling an open redirect. The validation needs to ensure the character after the allowed domain is a path separator (/), query (?), fragment (#), port (:), or end of string — or better yet, compare the parsed URL's origin against the allowed domain.
Reviewed by Cursor Bugbot for commit 9edec79. Configure here.
2 participants
Adds a missing "Next" button to our integration configuration pages, which appears to have been accidentally omitted in the ProjectMapper refactor in #110666
I've tested this using the Dev UI to ensure it completes installation as expected, and requires a single project mapping to be defined prior to allowing install.
Screenshot during testing
Mobile Screenshot:
Resolves ISWF-2405
Resolves #112345