Skip to content

Commit 8d6e1ab

Browse files
joshuali925joshuali925
authored
[2.x] Restrict chromium requests (#435)
* Fix regex validation, detect iframe, embed, object tags Signed-off-by: Joshua Li <[email protected]> * Disallow redirection to non-localhost urls Signed-off-by: Joshua Li <[email protected]> * Disallow connection to non-allowlisted urls Signed-off-by: Joshua Li <[email protected]> * Disable JIT Signed-off-by: Joshua Li <[email protected]> * Fix workflow Signed-off-by: Joshua Li <[email protected]> * Try to fix CI Signed-off-by: Joshua Li <[email protected]> * Fix localstorage logic Signed-off-by: Joshua Li <[email protected]> Signed-off-by: Joshua Li <[email protected]>
1 parent 1b25f94 commit 8d6e1ab

File tree

7 files changed

+125
-24
lines changed

7 files changed

+125
-24
lines changed

‎.github/workflows/dashboards-reports-test-and-build-workflow.yml

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ jobs:
2020
with:
2121
repository: opensearch-project/Opensearch-Dashboards
2222
ref: ${{ env.OPENSEARCH_VERSION }}
23-
path: dashboards-reports/OpenSearch-Dashboards
23+
path: OpenSearch-Dashboards
2424

2525
- name: Get node version
2626
id: versions_step
2727
run:
28-
echo "::set-output name=node_version::$(node -p "(require('./OpenSearch-Dashboards/package.json').engines.node).match(/[.0-9]+/)[0]")"
28+
echo "::set-output name=node_version::$(node -p "(require('../OpenSearch-Dashboards/package.json').engines.node).match(/[.0-9]+/)[0]")"
2929

3030
- name: Setup Node
3131
uses: actions/setup-node@v1
@@ -35,13 +35,13 @@ jobs:
3535

3636

3737
- name: Move Dashboards Reports to Plugins Dir
38-
run: mv dashboards-reports OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
38+
run: mv dashboards-reports ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
3939

4040
- name: Add Chromium Binary to Reporting for Testing
4141
run: |
4242
sudo apt update
4343
sudo apt install -y libnss3-dev fonts-liberation libfontconfig1
44-
cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
44+
cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
4545
wget https://github.com/opendistro-for-elasticsearch/kibana-reports/releases/download/chromium-1.12.0.0/chromium-linux-x64.zip
4646
unzip chromium-linux-x64.zip
4747
rm chromium-linux-x64.zip
@@ -51,25 +51,25 @@ jobs:
5151
with:
5252
timeout_minutes: 30
5353
max_attempts: 3
54-
command: cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn osd bootstrap
54+
command: cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn osd bootstrap
5555

5656
- name: Test
5757
uses: nick-invision/retry@v1
5858
with:
5959
timeout_minutes: 30
6060
max_attempts: 3
61-
command: cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn test --coverage
61+
command: cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}; yarn test --coverage
6262

6363
- name: Upload coverage
6464
uses: codecov/codecov-action@v1
6565
with:
6666
flags: dashboards-reports
67-
directory: OpenSearch-Dashboards/plugins/
67+
directory: ../OpenSearch-Dashboards/plugins/
6868
token: ${{ secrets.CODECOV_TOKEN }}
6969

7070
- name: Build Artifact
7171
run: |
72-
cd OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
72+
cd ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}
7373
yarn build
7474
7575
cd build
@@ -103,16 +103,16 @@ jobs:
103103
uses: actions/upload-artifact@v1
104104
with:
105105
name: dashboards-reports-linux-x64
106-
path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-x64.zip
106+
path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-x64.zip
107107

108108
- name: Upload Artifact For Linux arm64
109109
uses: actions/upload-artifact@v1
110110
with:
111111
name: dashboards-reports-linux-arm64
112-
path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-arm64.zip
112+
path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-linux-arm64.zip
113113

114114
- name: Upload Artifact For Windows
115115
uses: actions/upload-artifact@v1
116116
with:
117117
name: dashboards-reports-windows-x64
118-
path: OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-windows-x64.zip
118+
path: ../OpenSearch-Dashboards/plugins/${{ env.PLUGIN_NAME }}/build/${{ env.ARTIFACT_NAME }}-${{ env.OPENSEARCH_PLUGIN_VERSION }}-windows-x64.zip

‎dashboards-reports/server/routes/utils/__tests__/visualReportHelper.test.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,9 @@ describe('test create visual report', () => {
5555
reportParams as ReportParamsSchemaType,
5656
mockHtmlPath,
5757
mockLogger,
58-
mockHeader
58+
mockHeader,
59+
undefined,
60+
/^(data:image|file:\/\/)/
5961
);
6062
expect(fileName).toContain(`${reportParams.report_name}`);
6163
expect(fileName).toContain('.png');
@@ -71,7 +73,9 @@ describe('test create visual report', () => {
7173
reportParams as ReportParamsSchemaType,
7274
mockHtmlPath,
7375
mockLogger,
74-
mockHeader
76+
mockHeader,
77+
undefined,
78+
/^(data:image|file:\/\/)/
7579
);
7680
expect(fileName).toContain(`${reportParams.report_name}`);
7781
expect(fileName).toContain('.pdf');

‎dashboards-reports/server/routes/utils/constants.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ const ipv6Regex = /(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:)
9393
const localhostRegex = /localhost:([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])/g;
9494
const iframeRegex = /iframe/g;
9595

96+
export const ALLOWED_HOSTS = /^(0|0.0.0.0|127.0.0.1|localhost|(.*\.)?(opensearch.org|aws.a2z.com))$/;
97+
9698
export const replaceBlockedKeywords = (htmlString: string) => {
9799
// replace <ipv4>:<port>
98100
htmlString = htmlString.replace(ipv4Regex, BLOCKED_KEYWORD);

‎dashboards-reports/server/routes/utils/visual_report/style.css

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ body {
44
padding: 0;
55
}
66

7+
iframe, embed, object {
8+
display: none !important;
9+
}
10+
711
/* nice padding + matches Kibana default UI colors you could also set this to inherit if
812
the wrapper gets inserted inside a kibana section. I might also remove the manual text color here as well, potentially */
913
.reportWrapper {

‎dashboards-reports/server/routes/utils/visual_report/visualReportHelper.ts

Lines changed: 66 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
SELECTOR,
1515
CHROMIUM_PATH,
1616
SECURITY_CONSTANTS,
17+
ALLOWED_HOSTS,
1718
} from '../constants';
1819
import { getFileName } from '../helpers';
1920
import { CreateReportResultType } from '../types';
@@ -27,7 +28,8 @@ export const createVisualReport = async (
2728
queryUrl: string,
2829
logger: Logger,
2930
extraHeaders: Headers,
30-
timezone?: string
31+
timezone?: string,
32+
validRequestProtocol = /^(data:image)/,
3133
): Promise<CreateReportResultType> => {
3234
const {
3335
core_params,
@@ -76,6 +78,8 @@ export const createVisualReport = async (
7678
'--no-zygote',
7779
'--single-process',
7880
'--font-render-hinting=none',
81+
'--js-flags="--jitless --no-opt"',
82+
'--disable-features=V8OptimizeJavascript',
7983
],
8084
executablePath: CHROMIUM_PATH,
8185
ignoreHTTPSErrors: true,
@@ -84,6 +88,32 @@ export const createVisualReport = async (
8488
},
8589
});
8690
const page = await browser.newPage();
91+
92+
await page.setRequestInterception(true);
93+
let localStorageAvailable = true;
94+
page.on('request', (req) => {
95+
// disallow non-allowlisted connections. urls with valid protocols do not need ALLOWED_HOSTS check
96+
if (
97+
!validRequestProtocol.test(req.url()) &&
98+
!ALLOWED_HOSTS.test(new URL(req.url()).hostname)
99+
) {
100+
if (req.isNavigationRequest() && req.redirectChain().length > 0) {
101+
localStorageAvailable = false;
102+
logger.error(
103+
'Reporting does not allow redirections to outside of localhost, aborting. URL received: ' +
104+
req.url()
105+
);
106+
} else {
107+
logger.warn(
108+
'Disabled connection to non-allowlist domains: ' + req.url()
109+
);
110+
}
111+
req.abort();
112+
} else {
113+
req.continue();
114+
}
115+
});
116+
87117
page.setDefaultNavigationTimeout(0);
88118
page.setDefaultTimeout(100000); // use 100s timeout instead of default 30s
89119
// Set extra headers that are needed
@@ -93,13 +123,25 @@ export const createVisualReport = async (
93123
logger.info(`original queryUrl ${queryUrl}`);
94124
await page.goto(queryUrl, { waitUntil: 'networkidle0' });
95125
// should add to local storage after page.goto, then access the page again - browser must have an url to register local storage item on it
96-
await page.evaluate(
97-
/* istanbul ignore next */
98-
(key) => {
99-
localStorage.setItem(key, 'false');
100-
},
101-
SECURITY_CONSTANTS.TENANT_LOCAL_STORAGE_KEY
102-
);
126+
try {
127+
await page.evaluate(
128+
/* istanbul ignore next */
129+
(key) => {
130+
try {
131+
if (
132+
localStorageAvailable &&
133+
typeof localStorage !== 'undefined' &&
134+
localStorage !== null
135+
) {
136+
localStorage.setItem(key, 'false');
137+
}
138+
} catch (err) {}
139+
},
140+
SECURITY_CONSTANTS.TENANT_LOCAL_STORAGE_KEY
141+
);
142+
} catch (err) {
143+
logger.error(err);
144+
}
103145
await page.goto(queryUrl, { waitUntil: 'networkidle0' });
104146
logger.info(`page url ${page.url()}`);
105147

@@ -162,9 +204,24 @@ export const createVisualReport = async (
162204
// wait for dynamic page content to render
163205
await waitForDynamicContent(page);
164206

165-
await addReportStyle(page);
166207
await addReportHeader(page, keywordFilteredHeader);
167208
await addReportFooter(page, keywordFilteredFooter);
209+
await addReportStyle(page);
210+
211+
// this causes UT to fail in github CI but works locally
212+
try {
213+
const numDisallowedTags = await page.evaluate(
214+
() =>
215+
document.getElementsByTagName('iframe').length +
216+
document.getElementsByTagName('embed').length +
217+
document.getElementsByTagName('object').length
218+
);
219+
if (numDisallowedTags > 0) {
220+
throw Error('Reporting does not support "iframe", "embed", or "object" tags, aborting');
221+
}
222+
} catch (error) {
223+
logger.error(error);
224+
}
168225

169226
// create pdf or png accordingly
170227
if (reportFormat === FORMAT.pdf) {

‎dashboards-reports/server/utils/__tests__/validationHelper.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
REPORT_TYPE,
1111
TRIGGER_TYPE,
1212
} from '../../routes/utils/constants';
13-
import { validateReport, validateReportDefinition } from '../validationHelper';
13+
import { isValidRelativeUrl, validateReport, validateReportDefinition } from '../validationHelper';
1414

1515
const SAMPLE_SAVED_OBJECT_ID = '3ba638e0-b894-11e8-a6d9-e546fe2bba5f';
1616
const createReportDefinitionInput: ReportDefinitionSchemaType = {
@@ -152,7 +152,41 @@ describe('test input validation', () => {
152152
`saved object with id dashboard:${SAMPLE_SAVED_OBJECT_ID} does not exist`
153153
);
154154
});
155+
156+
test('validation against query_url', async () => {
157+
const urls: [string, boolean][] = [
158+
['/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=', true],
159+
[
160+
'/_plugin/kibana/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
161+
true,
162+
],
163+
[
164+
'/_dashboards/app/dashboards#/view/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
165+
true,
166+
],
167+
[
168+
'/_dashboards/app/dashboards#/edit/7adfa750-4c81-11e8-b3d7-01146121b73d?_g=',
169+
true,
170+
],
171+
[
172+
'/app/observability-dashboards?security_tenant=private#/notebooks/NYdlPIIB0-fJ8Bh1nLdW?view=output_only',
173+
true,
174+
],
175+
[
176+
'/app/notebooks-dashboards?view=output_only&security_tenant=private#/M4dlPIIB0-fJ8Bh1nLc7?security_tenant=private',
177+
true,
178+
],
179+
[
180+
'/_dashboards/app/visualize&security_tenant=/.%2e/.%2e/.%2e/.%2e/_dashboards?#/view/id',
181+
false,
182+
],
183+
];
184+
expect(urls.map((url) => isValidRelativeUrl(url[0]))).toEqual(
185+
urls.map((url) => url[1])
186+
);
187+
});
155188
});
189+
156190
// TODO: merge this with other mock clients used in testing, to create some mock helpers file
157191
const mockOpenSearchClient = (mockSavedObjectIds: string[]) => {
158192
const client = {

‎dashboards-reports/server/utils/validationHelper.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ export const isValidRelativeUrl = (relativeUrl: string) => {
3737
export const regexDuration = /^(-?)P(?=\d|T\d)(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)([DW]))?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+(?:\.\d+)?)S)?)?$/;
3838
export const regexEmailAddress = /\S+@\S+\.\S+/;
3939
export const regexReportName = /^[\w\-\s\(\)\[\]\,\_\-+]+$/;
40-
export const regexRelativeUrl = /^\/(_plugin\/kibana\/|_dashboards\/)?app\/(dashboards|visualize|discover|observability-dashboards|notebooks-dashboards\?view=output_only)([?&]security_tenant=.+|)#\/(notebooks\/|view\/|edit\/)?[^\/]+$/;
40+
export const regexRelativeUrl = /^\/(_plugin\/kibana\/|_dashboards\/)?app\/(dashboards|visualize|discover|observability-dashboards|notebooks-dashboards\?view=output_only(&security_tenant=.+)?)(\?security_tenant=.+)?#\/(notebooks\/|view\/|edit\/)?[^\/]+$/;
4141

4242
export const validateReport = async (
4343
client: ILegacyScopedClusterClient,

0 commit comments

Comments
 (0)