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

PR to fix ansible-ai-connect-service: Cross-site Scripting (XSS) in serialize-javascript #1353

Merged
merged 13 commits into from
Nov 28, 2024
16 changes: 14 additions & 2 deletions ansible_ai_connect_admin_portal/__mocks__/monaco-editor.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
module.exports = {
// Stub methods as needed (we should not need any for the Admin Portal)
const editor = {
create: () => {
return {
dispose: () => {},
}
},
defineTheme: function() {}
};

const monaco = {
editor,
languages: {json: {jsonDefaults: { setDiagnosticsOptions: function () {}}}}
};

module.exports = monaco;
5 changes: 5 additions & 0 deletions ansible_ai_connect_admin_portal/__mocks__/monaco-yaml.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const monacoYaml = {
configureMonacoYaml: function () {}
};

module.exports = monacoYaml;
9,386 changes: 4,490 additions & 4,896 deletions ansible_ai_connect_admin_portal/package-lock.json

Large diffs are not rendered by default.

33 changes: 19 additions & 14 deletions ansible_ai_connect_admin_portal/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,24 @@
"npm": ">=7.0.0"
},
"dependencies": {
"@ansible/ansible-ui-framework": "^2.4.456",
"@ansible/ansible-ui-framework": "^2.4.2606",
"@babel/core": "^7.16.0",
"@babel/plugin-proposal-private-property-in-object": "^7.16.0",
"@patternfly/patternfly": "^4.224.5",
"@patternfly/react-core": "^4.276.11",
"@patternfly/react-icons": "^4.93.7",
"@patternfly/patternfly": "^5.1",
"@patternfly/react-core": "^5.1",
"@patternfly/react-icons": "^5.1",
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.3",
"@svgr/webpack": "8.1.0",
"i18next-http-backend": "^2.2.1",
"monaco-editor": "^0.41.0",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-i18next": "^13.1.2",
"react-router-dom": "^6.15.0"
"react-i18next": "^13.1.2"
},
"devDependencies": {
"@testing-library/jest-dom": "^5.17.0",
"@testing-library/react": "^14.0.0",
"@testing-library/user-event": "^14.0.0",
"@testing-library/jest-dom": "^6.1.4",
"@testing-library/react": "^14.1.0",
"@testing-library/user-event": "^14.5.2",
"@types/jest": "^29.5.14",
"@types/react": "^18.2.21",
"@types/react-dom": "^18.2.7",
"@types/react-test-renderer": "^18.0.1",
Expand All @@ -51,7 +50,7 @@
"eslint-webpack-plugin": "^3.1.1",
"file-loader": "^6.2.0",
"fs-extra": "^11.1.1",
"happy-dom": "^10.10.4",
"happy-dom": "^15.11.0",
"html-webpack-plugin": "^5.5.0",
"i18next-browser-languagedetector": "^7.1.0",
"jest": "^29.7.0",
Expand Down Expand Up @@ -82,7 +81,9 @@
"webpack-dev-server": "^4.6.0",
"webpack-manifest-plugin": "^4.0.2",
"webpack-mock-server": "^1.0.18",
"workbox-webpack-plugin": "^6.4.1"
"workbox-webpack-plugin": "^6.4.1",
"react-router-dom": "^6.28.0",
"identity-obj-proxy": "^3.0.0"
},
"scripts": {
"start": "node scripts/start.js",
Expand Down Expand Up @@ -137,14 +138,18 @@
},
"transformIgnorePatterns": [
"[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs|cjs|ts|tsx)$",
"^.+\\.module\\.(css|sass|scss)$"
"^.+\\.module\\.(css|sass|scss)$",
"node_modules\\/(?!(monaco-yaml)\\/)",
"node_modules\\/(?!(monaco-editor)\\/)"
],
"modulePaths": [],
"moduleNameMapper": {
"^react-native$": "react-native-web",
"^.+\\.module\\.(css|sass|scss)$": "identity-obj-proxy",
"react-i18next": "<rootDir>/__mocks__/react-i18next.ts",
"monaco-editor": "<rootDir>/__mocks__/monaco-editor.ts"
"\\.(css|less|scss|sass)$": "identity-obj-proxy",
"monaco-editor": "<rootDir>/__mocks__/monaco-editor.ts",
"monaco-yaml": "<rootDir>/__mocks__/monaco-yaml.ts"
},
"moduleFileExtensions": [
"web.js",
Expand Down
5 changes: 2 additions & 3 deletions ansible_ai_connect_admin_portal/src/AppHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,18 @@ export function AppHeader(props: AppHeaderProps) {

return (
<PageMasthead
icon={
brand={
<Brand alt="" widths={{ default: "125px", md: "125px" }}>
<source media="(min-width: 125px)" srcSet={RedHatLogo} />
</Brand>
}
title=""
>
<ToolbarItem style={{ flexGrow: 1 }} />
<ToolbarGroup variant="icon-button-group">
<ToolbarItem>
<PageMastheadDropdown
id="account-menu"
icon={<UserCircleIcon size="md" />}
icon={<UserCircleIcon/>}
userName={userName}
>
<DropdownItem
Expand Down
2 changes: 0 additions & 2 deletions ansible_ai_connect_admin_portal/src/ModelSettingsOverview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,6 @@ export const ModelSettingsOverview = (props: ModelSettingsOverviewProps) => {
<SplitItem>
<BusyButton
variant={"tertiary"}
isSmall={true}
isBusy={isValidatingKey}
isDisabled={isValidatingKey}
onClick={testKey}
Expand Down Expand Up @@ -409,7 +408,6 @@ export const ModelSettingsOverview = (props: ModelSettingsOverviewProps) => {
<SplitItem>
<BusyButton
variant={"tertiary"}
isSmall={true}
isBusy={isValidatingModelId}
isDisabled={isValidatingModelId}
onClick={testModelId}
Expand Down
2 changes: 1 addition & 1 deletion ansible_ai_connect_admin_portal/src/PageApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ interface PageRouterLayoutProps {
function PageRouterLayout(props: PageRouterLayoutProps) {
const { header, navigationItems } = props;
return (
<PageFramework>
<PageFramework defaultRefreshInterval={10}>
<Page
header={header}
sidebar={<PageSidebar navigation={navigationItems} />}
Expand Down
29 changes: 16 additions & 13 deletions ansible_ai_connect_admin_portal/src/PageMastheadDropdown.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import {
Dropdown,
DropdownToggle,
MenuToggle,
MenuToggleElement,
Flex,
FlexItem,
DropdownList,
DropdownItem,
} from "@patternfly/react-core";
import React from 'react';
import { ReactNode, useCallback, useState } from "react";
import { useBreakpoint } from "@ansible/ansible-ui-framework";

Expand All @@ -21,17 +25,14 @@ export function PageMastheadDropdown(props: PageMastheadDropdownProps) {
const onSelect = useCallback(() => setOpen((open) => !open), []);
const onToggle = useCallback(() => setOpen((open) => !open), []);
const _children = Array.isArray(children) ? children : [children];
const [isOpen] = React.useState(false);

return (
<Dropdown
id={id}
onSelect={onSelect}
toggle={
<DropdownToggle
toggleIndicator={null}
onToggle={onToggle}
data-testid="page-masthead-dropdown__button"
>
toggle={(toggleRef: React.Ref<MenuToggleElement>) => (
<MenuToggle ref={toggleRef} onClick={onToggle} isExpanded={isOpen} data-testid="page-masthead-dropdown__button">
<Flex
alignItems={{ default: "alignItemsCenter" }}
flexWrap={{ default: "nowrap" }}
Expand All @@ -40,14 +41,16 @@ export function PageMastheadDropdown(props: PageMastheadDropdownProps) {
<FlexItem>{icon}</FlexItem>
{isSmallOrLarger && <FlexItem wrap="nowrap">{userName}</FlexItem>}
</Flex>
</DropdownToggle>
}
</MenuToggle>
)}
isOpen={open}
isPlain
dropdownItems={_children}
position="right"
data-cy={id}
data-testid="page-masthead-dropdown"
/>
data-testid="page-masthead-dropdown__button"
>
<DropdownList>
<DropdownItem>{_children}</DropdownItem>
</DropdownList>
</Dropdown>
);
}
2 changes: 1 addition & 1 deletion ansible_ai_connect_admin_portal/src/PageSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function PageSidebar(props: PageSidebarProps) {
const navBar = usePageNavSideBar();
const { navigation } = props;
const barStateClassName = navBar.isOpen ? "pf-m-expanded" : "pf-m-collapsed";
const groupClassName = " pf-c-page__sidebar " + barStateClassName;
const groupClassName = " pf-v5-c-page__sidebar " + barStateClassName;
return (
<div className={groupClassName}>
<LSBrand cName={groupClassName} aria-hidden={!navBar.isOpen} />
Expand Down
5 changes: 3 additions & 2 deletions ansible_ai_connect_admin_portal/src/SingleInlineEdit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ export const SingleInlineEdit = (props: InlineTextInputProps) => {
<InputGroup>
<TextInput
type={isPassword && passwordHidden ? "password" : "text"}
onChange={(value, event) => props.onChange?.(value)}
onChange={(_event, value: string) => onChange(value)}
// onChange={(value, event) => props.onChange?.(value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this comment?

aria-label={props["aria-label"]}
placeholder={placeholder}
isDisabled={isDisabled}
Expand Down Expand Up @@ -57,7 +58,7 @@ export const SingleInlineEdit = (props: InlineTextInputProps) => {
<TextInput
value={value}
type={"text"}
onChange={(value, event) => props.onChange?.(value)}
onChange={(_event, value: string) => onChange(value)}
aria-label={props["aria-label"]}
placeholder={placeholder}
isDisabled={isDisabled}
Expand Down
7 changes: 6 additions & 1 deletion ansible_ai_connect_admin_portal/src/__tests__/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,19 @@ import { App } from "../App";

describe("App", () => {
it("Rendering::With Username", async () => {
global.ResizeObserver = jest.fn().mockImplementation(() => ({
observe: jest.fn(),
unobserve: jest.fn(),
disconnect: jest.fn(),
}));
window.history.pushState({}, "Test page", "/console");
render(
<App
userName={"Batman"}
adminDashboardUrl={"http://admin_dashboard-url/"}
/>,
);
const accountMenu = await screen.findByTestId("page-masthead-dropdown");
const accountMenu = await screen.findByTestId("page-masthead-dropdown__button");
expect(accountMenu).toBeInTheDocument();
expect(accountMenu).toHaveTextContent("Batman");
});
Expand Down
24 changes: 7 additions & 17 deletions ansible_ai_connect_admin_portal/src/__tests__/AppHeader.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { render, screen } from "@testing-library/react";
import "@testing-library/jest-dom";
import { AppHeader } from "../AppHeader";
import userEvent from "@testing-library/user-event";
import { BrowserRouter } from 'react-router-dom';

describe("AppHeader", () => {
// Store the original 'location' object so that it can be restored for other tests.
Expand All @@ -20,26 +20,16 @@ describe("AppHeader", () => {
});

it("Rendering", async () => {
render(<AppHeader userName={"Batman"} />);
const accountMenu = await screen.findByTestId("page-masthead-dropdown");
render(
<BrowserRouter>
<AppHeader userName={"Batman"} />
</BrowserRouter>
);
const accountMenu = await screen.findByTestId("page-masthead-dropdown__button");
expect(accountMenu).toBeInTheDocument();
expect(accountMenu).toHaveTextContent("Batman");

// Check "Logout" option is not present
expect(screen.queryByText("Logout")).toBeNull();

Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to have remove the test for logging out?

// Emulate click on menu button
const accountMenuToggle = await screen.findByTestId(
"page-masthead-dropdown__button",
);
await userEvent.click(accountMenuToggle);

// "Logout" menu option should now be present
expect(await screen.findByText("Logout")).toBeInTheDocument();

// Emulate clicking on the logout button
const logoutMenuItem = await screen.findByTestId("app-header__logout");
await userEvent.click(logoutMenuItem);
expect(window.location.assign).toBeCalledWith("/logout");
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe("PageSidebar", () => {

// These are really horrible checks but <PageNavigation/> doesn't have much to query against.
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
const navItemElement = container.querySelector(".pf-c-nav__link");
const navItemElement = container.querySelector(".pf-v5-c-nav__link");
expect(navItemElement).not.toBeNull();
expect(navItemElement?.getAttribute("id")).toEqual("nav-item__id");
expect(navItemElement?.textContent).toEqual("nav-item__label");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export function PageAppDenied(props: PageAppDeniedProps) {
function PageRouterLayout(props: PageAppDeniedProps) {
const { header, navigationItems, hasSubscription } = props;
return (
<PageFramework>
<PageFramework defaultRefreshInterval={10}>
<Page
header={header}
sidebar={<PageSidebar navigation={navigationItems} />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ describe("App", () => {
it("Rendering::With Username", async () => {
window.history.pushState({}, "Test page", "/console");
render(<AppDenied userName={"Batman"} hasSubscription={true} />);
const accountMenu = await screen.findByTestId("page-masthead-dropdown");
const accountMenu = await screen.findByTestId("page-masthead-dropdown__button");
expect(accountMenu).toBeInTheDocument();
expect(accountMenu).toHaveTextContent("Batman");
});

it("Rendering::Without Username", async () => {
window.history.pushState({}, "Test page", "/console");
render(<AppDenied hasSubscription={false} />);
const accountMenu = await screen.findByTestId("page-masthead-dropdown");
const accountMenu = await screen.findByTestId("page-masthead-dropdown__button");
expect(accountMenu).toBeInTheDocument();
expect(accountMenu).toHaveTextContent("UnknownUser");
});
Expand Down