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.

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

feat(cve): add compare cves feature for 2 images #420

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 5 additions & 6 deletions playwright.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const { devices } = require('@playwright/test');
*/
// require('dotenv').config();


/**
* @see https://playwright.dev/docs/test-configuration
* @type {import('@playwright/test').PlaywrightTestConfig}
Expand Down Expand Up @@ -53,24 +52,24 @@ const config = {
use: {
...devices['Desktop Chrome'],
ignoreHTTPSErrors: true
},
}
},

{
name: 'firefox',
use: {
...devices['Desktop Firefox'],
ignoreHTTPSErrors: true
},
}
},

{
name: 'webkit',
use: {
...devices['Desktop Safari'],
ignoreHTTPSErrors: true
},
},
}
}

/* Test against mobile viewports. */
// {
Expand Down Expand Up @@ -102,7 +101,7 @@ const config = {
],

/* Folder for test artifacts such as screenshots, videos, traces, etc. */
outputDir: 'test-results/',
outputDir: 'test-results/'

/* Run your local dev server before starting the tests */
// webServer: {
Expand Down
2 changes: 1 addition & 1 deletion src/__tests__/TagPage/VulnerabilitiesDetails.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const mockCVEList = {
LowCount: 1,
MediumCount: 1,
HighCount: 1,
CriticalCount: 1,
CriticalCount: 1
},
CVEList: [
{
Expand Down
18 changes: 18 additions & 0 deletions src/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,19 @@
},
allVulnerabilitiesForRepo: (name) =>
`/v2/_zot/ext/search?query={CVEListForImage(image: "${name}"){Tag Page {TotalCount ItemCount} CVEList {Id Title Description Severity Reference PackageList {Name InstalledVersion FixedVersion}}}}`,
cveDiffForImages: (minuend = {}, subtrahend = {}, { pageNumber = 1, pageSize = 3 }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

'minuend' and 'subtrahend' don't sound very suggestive of what the parameter represents. Is there a better name we can use perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I struggled finding a better name, minuend means "from which you subtract" and subtrahend "how much you subtract". These are the most concise terms I know but I agree that they're quite mathematical and unfriendly

Copy link
Contributor

@andaaron andaaron Mar 5, 2024

Choose a reason for hiding this comment

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

Add a comment in the JS code explaining these.

let imageInput = (img) => {

Check warning on line 108 in src/api.js

View check run for this annotation

Codecov / codecov/patch

src/api.js#L107-L108

Added lines #L107 - L108 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the value for the variable doesn't change, please use 'const' for declaration

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we will want to reuse this imageInput function in the future for other endpoints. We should declare it outside 'cveDiffForImages'

let digest = img.digest ? `, digest: ${img.digest}` : '';
let platform = img.platform ? `, platform: {os: ${img.platform.os}, arch: ${img.platform.arch}}` : '';
return `{repo: "${img.repo}", tag: "${img.tag}"${digest}${platform}}`;

Check warning on line 111 in src/api.js

View check run for this annotation

Codecov / codecov/patch

src/api.js#L111

Added line #L111 was not covered by tests
};
let query = `/v2/_zot/ext/search?query={CVEDiffListForImages(minuend: ${imageInput(

Check warning on line 113 in src/api.js

View check run for this annotation

Codecov / codecov/patch

src/api.js#L113

Added line #L113 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the 'query' variable, we can just return the interpolated string directly

minuend
)}, subtrahend: ${imageInput(subtrahend)}, requestedPage: {limit:${pageSize} offset:${
(pageNumber - 1) * pageSize
}}) {Minuend Subtrahend CVEList{Id Title Description Severity Reference PackageList {Name InstalledVersion FixedVersion}} Summary {Count UnknownCount LowCount MediumCount HighCount CriticalCount} Page {TotalCount ItemCount}}}`;
return query;

Check warning on line 118 in src/api.js

View check run for this annotation

Codecov / codecov/patch

src/api.js#L118

Added line #L118 was not covered by tests
},
imageListWithCVEFixed: (cveId, repoName, { pageNumber = 1, pageSize = 3 }, filter = {}) => {
let filterParam = '';
if (filter.Os || filter.Arch) {
Expand Down Expand Up @@ -150,6 +163,11 @@
const paginationParam = `requestedPage: {limit:${pageSize} offset:${(pageNumber - 1) * pageSize} sortBy:RELEVANCE}`;
return `/v2/_zot/ext/search?query={GlobalSearch(${searchParam}, ${paginationParam}) {Images {RepoName Tag}}}`;
},
imageSuggestionsWithPlatforms: ({ searchQuery = '""', pageNumber = 1, pageSize = 15 }) => {

Check warning on line 166 in src/api.js

View check run for this annotation

Codecov / codecov/patch

src/api.js#L166

Added line #L166 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a point for this endpoint? Can we not achieve this already with the 'globalSearch'?

const searchParam = searchQuery !== '' ? `query:"${searchQuery}"` : `query:""`;
const paginationParam = `requestedPage: {limit:${pageSize} offset:${(pageNumber - 1) * pageSize} sortBy:RELEVANCE}`;
return `/v2/_zot/ext/search?query={GlobalSearch(${searchParam}, ${paginationParam}) {Images {RepoName Tag Manifests {Platform {Os Arch}}}}}`;

Check warning on line 169 in src/api.js

View check run for this annotation

Codecov / codecov/patch

src/api.js#L168-L169

Added lines #L168 - L169 were not covered by tests
},
referrers: ({ repo, digest, type = '' }) =>
`/v2/_zot/ext/search?query={Referrers(repo: "${repo}" digest: "${digest}" type: "${type}"){MediaType ArtifactType Size Digest Annotations{Key Value}}}`,
bookmarkToggle: (repo) => `/v2/_zot/ext/userprefs?repo=${repo}&action=toggleBookmark`,
Expand Down
15 changes: 13 additions & 2 deletions src/components/Header/SearchSuggestion.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ const useStyles = makeStyles(() => ({
position: 'relative',
zIndex: 1150
},
searchContainerUnstretched: {
display: 'inline-block',
backgroundColor: '#2B3A4E',
boxShadow: '0 0.313rem 0.625rem rgba(131, 131, 131, 0.08)',
borderRadius: '0.625rem',
position: 'relative',
zIndex: 1150
},
searchContainerFocused: {
backgroundColor: '#FFFFFF'
},
Expand Down Expand Up @@ -107,7 +115,7 @@ const useStyles = makeStyles(() => ({
}
}));

function SearchSuggestion({ setSearchCurrentValue = () => {} }) {
function SearchSuggestion({ setSearchCurrentValue = () => {}, stretch = true }) {
const [searchQuery, setSearchQuery] = useState('');
const [suggestionData, setSuggestionData] = useState([]);
const [queryParams] = useSearchParams();
Expand Down Expand Up @@ -269,7 +277,10 @@ function SearchSuggestion({ setSearchCurrentValue = () => {} }) {
};

return (
<div className={`${classes.searchContainer} ${isComponentFocused && classes.searchContainerFocused}`}>
<div
className={`${stretch ? classes.searchContainer : classes.searchContainerUnstretched}
${isComponentFocused && classes.searchContainerFocused}`}
>
<Stack
className={`${classes.search} ${isComponentFocused && classes.searchFocused} ${
isFailedSearch && !isLoading && classes.searchFailed
Expand Down
154 changes: 154 additions & 0 deletions src/components/Tag/Tabs/CompareImages.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
import React, { useState, useMemo } from 'react';

// components
import { Button, Modal, Typography, Stack, Box } from '@mui/material';
import makeStyles from '@mui/styles/makeStyles';

import { api, endpoints } from 'api';
import { host } from 'host';
import ImageSelector from 'components/Tag/Tabs/ImageSelector';
import VulnerabilitiyCard from 'components/Shared/VulnerabilityCard';
import { isEmpty } from 'lodash';
import { mapCVEInfo } from 'utilities/objectModels';

const useStyles = makeStyles(() => ({
imagesInputBox: {
padding: '0.5rem',
marginBottom: '0.5rem'
},
input: {
color: '#464141',
'&::placeholder': {
opacity: '1'
}
},
searchInputBase: {
width: '90%',
paddingLeft: '1rem',
border: '1px solid black',
height: 40,
color: 'rgba(0, 0, 0, 0.6)'
},
compareImagesPopup: {
position: 'absolute',
top: '50%',
left: '50%',
transform: 'translate(-50%, -50%)',
backgroundColor: 'white',
border: '2px solid #000',
padding: '1rem'
},
compareButton: {
paddingLeft: '0.5rem'
}
}));

function CompareImages({ name, tag, platform }) {
const classes = useStyles();
const [open, setOpen] = useState(false);
const [minuend, setMinuend] = useState('');
const [subtrahend, setSubtrahend] = useState('');
const [cveData, setCVEData] = useState([]);
const handleOpen = () => setOpen(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can just inline the handleOpen and handleClose functions if we don't want to add additional logic

const handleClose = () => setOpen(false);
const handleMinuendInput = (value) => {
setMinuend(value);

Check warning on line 55 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L55

Added line #L55 was not covered by tests
};
const handleSubtrahendInput = (value) => {
setSubtrahend(value);

Check warning on line 58 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L58

Added line #L58 was not covered by tests
};
const abortController = useMemo(() => new AbortController(), []);

const imageCVECompare = (minuend, subtrahend) => {
api

Check warning on line 63 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L63

Added line #L63 was not covered by tests
.get(
`${host()}${endpoints.cveDiffForImages(minuend, subtrahend, { pageNumber: 1, pageSize: 9 })}`,
abortController.signal
)
.then((diffResponse) => {
const cveListData = mapCVEInfo(diffResponse.data.data.CVEDiffListForImages.CVEList);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should have some checks here before passing data to mapCVEInfo, make sure the response actually contains data

setCVEData(cveListData);

Check warning on line 70 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L68-L70

Added lines #L68 - L70 were not covered by tests
})
.catch((e) => {
console.error(e);

Check warning on line 73 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L72-L73

Added lines #L72 - L73 were not covered by tests
});
};

const getImageRefComponents = (image) => {
if (image.includes('@')) {
let components = image.split('@');
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer const over let

return [components[0], '', components[1]];

Check warning on line 80 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L79-L80

Added lines #L79 - L80 were not covered by tests
} else if (image.includes(':')) {
let components = image.split(':');
return [components[0], components[1], ''];

Check warning on line 83 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L82-L83

Added lines #L82 - L83 were not covered by tests
}

return [image, '', ''];

Check warning on line 86 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L86

Added line #L86 was not covered by tests
};

const handleCompare = () => {
let [minuendRepo, minuendTag] = getImageRefComponents(minuend);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this call will achieve what we want. You are only taking the first 2 values returned by the getImageRefComponents call, so in the return-case on line 80, the components[1] value will be lost

let [subtrahendRepo, subtrahendTag] = getImageRefComponents(subtrahend);
imageCVECompare({ repo: minuendRepo, tag: minuendTag }, { repo: subtrahendRepo, tag: subtrahendTag });

Check warning on line 92 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L90-L92

Added lines #L90 - L92 were not covered by tests
};

const renderCVEs = () => {
return !isEmpty(cveData) ? (
cveData.map((cve, index) => {
return <VulnerabilitiyCard key={index} cve={cve} name={name} platform={platform} expand={true} />;

Check warning on line 98 in src/components/Tag/Tabs/CompareImages.jsx

View check run for this annotation

Codecov / codecov/patch

src/components/Tag/Tabs/CompareImages.jsx#L97-L98

Added lines #L97 - L98 were not covered by tests
})
) : (
<></>
);
};

return (
<div>
<Button variant="outlined" onClick={handleOpen}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this component is a modal popup, then control over when the modal is shown should be given to the parent component, meaning: take out the button that opens it, that should be in the parent component

Compare
</Button>
<Modal
open={open}
onClose={handleClose}
aria-labelledby="modal-modal-title"
aria-describedby="modal-modal-description"
>
<Stack direction="column" className={classes.compareImagesPopup}>
<Typography variant="h6" component="h2">
Compare the vulnerabilities of 2 images
</Typography>
<Stack direction="row" spacing={2} className={classes.imagesInputBox}>
<ImageSelector
setSearchCurrentValue={handleMinuendInput}
name={name}
tag={tag}
// digest={digest}
// platform={platform}
/>
<ImageSelector setSearchCurrentValue={handleSubtrahendInput} name={name} tag={tag} />
</Stack>
<Box className={classes.compareButton}>
<Button variant="outlined" onClick={handleCompare}>
Compare
</Button>
</Box>
<Box
sx={{
mb: 2,
display: 'flex',
flexDirection: 'column',
height: 300,
overflow: 'hidden',
overflowY: 'scroll'
// justifyContent="flex-end" # DO NOT USE THIS WITH 'scroll'
}}
>
{renderCVEs()}
</Box>
</Stack>
</Modal>
</div>
);
}

export default CompareImages;
Loading
Loading