Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Front-End for GitHub PR Generation #1134

Open
wants to merge 5 commits into
base: file-github-pr-resolver
Choose a base branch
from
Open

Front-End for GitHub PR Generation #1134

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e46701e
One more comment
freeqaz Feb 17, 2023
00ee832
Front-End for GitHub PR Generation
freeqaz Feb 17, 2023
e6f3cf9
Cleanup
freeqaz Feb 17, 2023
99ee3a0
Finish up the UI, fix some bugs
freeqaz Mar 3, 2023
4ea5a62
Attempt to fix CI failure
freeqaz Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lunatrace/bsl/backend/src/graphql-yoga/resolvers/create-pull-request-for-vulnerability.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { checkProjectIsAuthorizedOrThrow, isAuthenticated } from '../helpers/aut
type CreatePullRequestForVulnerabilityType = NonNullable<MutationResolvers['createPullRequestForVulnerability']>;

function splitGitHubRepoPath(gitHubRepo: string) {
// URL from GitHub of the repo, eg: git://github.com/lunasec-io/lunasec.git
const split = gitHubRepo.split('/');
const owner = split[split.length - 2];
const repo = split[split.length - 1].replace('.git', '');
Expand Down
51 changes: 48 additions & 3 deletions lunatrace/bsl/frontend/src/api/generated.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions lunatrace/bsl/frontend/src/api/graphql/mutateCreateGitHubPullRequestForVuln.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
mutation CreateGitHubPullRequestForVuln(
$project_id: uuid!,
$vulnerability_id: uuid!,
$old_package_slug: String!,
$new_package_slug: String!,
$package_manifest_path: String!
) {
createPullRequestForVulnerability(
project_id: $project_id,
vulnerability_id: $vulnerability_id,
old_package_slug: $old_package_slug,
new_package_slug: $new_package_slug,
package_manifest_path: $package_manifest_path
) {
success
pullRequestUrl
}
}
4 changes: 2 additions & 2 deletions lunatrace/bsl/frontend/src/pages/project/builds/details/VulnerablePackageListWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ export const VulnerablePackageListWrapper: React.FC<VulnerablePackageListWrapper
shouldIgnore,
build,
}) => {
// severity state for modern tree data, legacy has its own state and doesnt use this
// severity state for modern tree data, legacy has its own state and doesn't use this
const [severity, setSeverity] = useState<SeverityNamesOsv>('Critical');

// data for modern tree, legacy doesnt use this
// data for modern tree, legacy doesn't use this
const {
data: vulnerableReleasesData,
isLoading,
Expand Down
47 changes: 33 additions & 14 deletions ...ct/builds/details/vulnerable-packages/vulnerable-package-card/PackageUpdatablePopOver.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,19 @@
*
*/
import React from 'react';
import { NavLink, OverlayTrigger, Popover, Tooltip } from 'react-bootstrap';
import { Button, NavLink, OverlayTrigger, Popover, Tooltip } from 'react-bootstrap';
import { CopyBlock, tomorrowNight } from 'react-code-blocks';
import { BsGithub } from 'react-icons/bs';
import { FcUpload } from 'react-icons/fc';

import useBreakpoint from '../../../../../../hooks/useBreakpoint';
import { isDirectDep } from '../../../../../../utils/package';
import { VulnerablePackage } from '../types';
export const PackageUpdatablePopOver: React.FC<{ pkg: VulnerablePackage }> = ({ pkg }) => {

export const PackageUpdatablePopOver: React.FC<{
pkg: VulnerablePackage;
onClickUpdate: (pkg: VulnerablePackage) => void;
}> = ({ pkg, onClickUpdate }) => {
const trivialUpdateStatus = pkg.trivially_updatable;

if (!trivialUpdateStatus || trivialUpdateStatus === 'no') {
Expand All @@ -30,13 +35,21 @@ export const PackageUpdatablePopOver: React.FC<{ pkg: VulnerablePackage }> = ({
return (
<Popover className="vulnerablePackage-update-popover" {...props}>
<Popover.Header>Trivially Updatable </Popover.Header>
<Popover.Body>
A fix is available within the semver range this package was requested with, meaning that the{' '}
<strong>project lockfile</strong> is probably the only thing constraining the package to the vulnerable
version.
<hr className="m-1" />
<Popover.Body style={{ maxWidth: '500px' }}>
<div className="mb-1 ">
<Button variant="primary" size="lg" className="mb-1" onClick={() => onClickUpdate(pkg)}>
<BsGithub className="mb-1 me-1" /> {'Click to Patch Vulnerability'}
</Button>
</div>
Clicking this button will open a Pull Request to update the project&apos;s lockfile.
<hr className="mt-1 mb-1" />
<>
A fix is available within the semver range this package was requested with, meaning that the{' '}
<strong>project lockfile</strong> is likely constraining the package to the vulnerable version.
</>
<hr className="mt-1 mb-1" />
{isDirectDep(pkg) ? (
<>
<div>
This command will update the package:
<CopyBlock
text={`npm update ${pkg.release.package.name}`}
Expand All @@ -55,14 +68,20 @@ export const PackageUpdatablePopOver: React.FC<{ pkg: VulnerablePackage }> = ({
theme={tomorrowNight}
codeBlock
/>
</>
</div>
) : (
<>
<div>
This package is a <strong>transitive</strong> (deep) dependency. Due to constraints of NPM and Yarn,
updating it is only possible by manually deleting it from your project&apos;s lockfile (or deleting your
entire lockfile) and then re-running your package install command (npm install) to fetch the latest
version. Automated lockfile patching is currently in development by LunaTrace.
</>
updating it is only possible using a special tool like{' '}
<a
href="https://github.com/lunasec-io/lunasec/tree/master/lunatrace/npm-package-cli"
target="_blank"
rel="noreferrer"
>
@lunatrace/npm-package-cli
</a>{' '}
(which powers the automated patcher above).
</div>
)}
</Popover.Body>
</Popover>
Expand Down
4 changes: 3 additions & 1 deletion ...uilds/details/vulnerable-packages/vulnerable-package-card/VulnerablePackageCardHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ import { PackageUpdatablePopOver } from './PackageUpdatablePopOver';
interface VulnerablePackageCardHeaderProps {
ignored: boolean;
pkg: VulnerablePackage;
onClickUpdate: (pkg: VulnerablePackage) => void;
}

export const VulnerablePackageCardHeader: React.FunctionComponent<VulnerablePackageCardHeaderProps> = ({
pkg,
ignored,
onClickUpdate,
}) => {
const recommendedVersion = semver.rsort([...pkg.fix_versions])[0];
return (
Expand All @@ -50,7 +52,7 @@ export const VulnerablePackageCardHeader: React.FunctionComponent<VulnerablePack
{recommendedVersion}
</>
)}
<PackageUpdatablePopOver pkg={pkg} />
<PackageUpdatablePopOver pkg={pkg} onClickUpdate={onClickUpdate} />
<PackageManagerLink
packageName={pkg.release.package.name}
packageManager={pkg.release.package.package_manager}
Expand Down
25 changes: 22 additions & 3 deletions ...ject/builds/details/vulnerable-packages/vulnerable-package-card/VulnerablePackageMain.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ import React, { useState } from 'react';
import { Card, Dropdown, FloatingLabel, Form, FormControl, Spinner } from 'react-bootstrap';
import { BsThreeDotsVertical } from 'react-icons/bs';
import { useParams } from 'react-router-dom';
import semver from 'semver';

import api from '../../../../../../api';
import { ConfirmationDailog } from '../../../../../../components/ConfirmationDialog';
import {BuildDetailInfo, QuickViewProps} from '../../../types';
import { BuildDetailInfo, QuickViewProps } from '../../../types';
import { VulnerablePackage } from '../types';

import { PackageCardBody } from './PackageCardBody';
Expand All @@ -42,6 +43,8 @@ export const VulnerablePackageMain: React.FunctionComponent<VulnerablePackageMai
}) => {
const [showConfirmation, setShowConfirmation] = useState(false);
const [insertVulnIgnore, insertVulnIgnoreState] = api.useInsertIgnoredVulnerabilitiesMutation();
const [createGitHubPullRequestForVuln] = api.useCreateGitHubPullRequestForVulnMutation();

const [ignoreNote, setIgnoreNote] = useState('');
const { project_id } = useParams();
const [shouldFilterFindingsBySeverity, setShouldFilterFindingsBySeverity] = useState(true);
Expand All @@ -65,6 +68,8 @@ export const VulnerablePackageMain: React.FunctionComponent<VulnerablePackageMai

const allFindingsIgnored = findings.every((f) => f.ignored);

const recommendedVersion = semver.rsort([...pkg.fix_versions])[0];

const bulkIgnoreVulns = async () => {
if (!project_id) {
throw new Error('attempted to ignore a vuln but no project id is in the url');
Expand All @@ -82,7 +87,7 @@ export const VulnerablePackageMain: React.FunctionComponent<VulnerablePackageMai
// eslint-disable-next-line react/display-name
const customMenuToggle = React.forwardRef<
HTMLAnchorElement,
{ onClick: (e: React.MouseEvent<HTMLAnchorElement>) => void,children:React.ReactNode }
{ onClick: (e: React.MouseEvent<HTMLAnchorElement>) => void; children: React.ReactNode }
>(({ children, onClick }, ref) => (
<a
className="text-end position-absolute top-0 end-0 m-3 btn-white"
Expand Down Expand Up @@ -113,11 +118,25 @@ export const VulnerablePackageMain: React.FunctionComponent<VulnerablePackageMai
);
};

async function onClickUpdate(pkg: VulnerablePackage) {
const response = await createGitHubPullRequestForVuln({
project_id: build.project_id,
Copy link
Contributor

@factoidforrest factoidforrest Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel that this should almost certainly take an array of changes. Clicking this button 5 times and filing 5 PRs for a single build is just not usable, there's no reason to write it in an incomplete way as an MVP since that's missing the "viable" part, IMO. It's nice to have the option to do it both one at a time, or for everything. It's not like that will be significantly more difficult. That's a pretty easy change, from what I've seen so far. There are all the components for that in the frontend kicking around still, I think, since that's how I had it written before.

Also, the "recommended version" is, I don't think, necessarily what we want to update to. That is just the highest fix version as you have it, which is not necessarily the fix version that is the target for the update.

Take a look at the backend code that calculates what the potential patch version is and you'll see it returns the target updatable version to the frontend.

const triviallyUpdatableTo = precomputeVulnTriviallyUpdatableTo(node.range, vulnMeta);
This is what you should be targetting as an update version. So you already have what you need, you can just use that value.

type BuildData_AffectedByVulnerability {
    vulnerability: BuildData_Vulnerability!
    beneath_minimum_severity: Boolean!
    ranges: [BuildData_Range!]!
    ignored: Boolean!
    ignored_vulnerability: BuildData_IgnoredVulnerability
    trivially_updatable_to: String   <---- RIGHT HERE
    fix_versions: [String!]!
    path: String!
    adjustment: BuildData_Adjustment #optional
}

I think, very soon, we will want to change your backend implementation to use its own copy of the tree to make decisions, rather than just passing the data through the frontend. To build the "deep update" feature that I have mentioned several times, we need to be tree-aware anyway. But, for now, You should be able to get away with using the value that I'm passing here trivially_updatable_to

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind on that last tree part, but still yes on everything else. We can add the "parent update" logic to the backend as well and just pass that alongside the "trivially_updatable_to" that we are already passing here.

vulnerability_id: pkg.affected_by[0].vulnerability.id,
old_package_slug: `${pkg.release.package.name}@${pkg.release.version}`,
new_package_slug: `${pkg.release.package.name}@^${recommendedVersion}`,
// TODO: Add support for multiple paths...
package_manifest_path: pkg.paths[0],
});

// TODO: Make this actually put some HTML on the page.
console.log('pr response:', response);
Copy link
Contributor

@factoidforrest factoidforrest Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we should probably finish this feature before merging to master.

It would be nice to keep master in a deployable state at all times (not that we succeed, lol)

}

return (
<>
<Card className="vulnpkg-card">
{renderIgnoreUi()}
<VulnerablePackageCardHeader pkg={pkg} ignored={allFindingsIgnored} />
<VulnerablePackageCardHeader pkg={pkg} ignored={allFindingsIgnored} onClickUpdate={onClickUpdate} />
<PackageCardBody
findingsHiddenBySeverityCount={findingsHiddenBySeverityCount}
pkg={pkg}
Expand Down
2 changes: 1 addition & 1 deletion lunatrace/npm-package-cli/src/package/github-pr/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export async function replacePackageAndFileGitHubPullRequest(
throw new Error(`GitHub Error: Unable to create pull request for ${owner}/${repo}@${checkoutRef} in path: ${path}`);
}

if (pullRequest.status !== 200) {
if (pullRequest.status !== 201 && pullRequest.status !== 200) {
throw new Error(
`GitHub Error: Unable to create pull request for ${owner}/${repo}@${checkoutRef} in path: ${path} (Status: ${pullRequest.status})`
);
Expand Down