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

Omit adding the osd-version header when the Fetch request is to an external origin #3643

Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Vis Builder] Add metric to metric, bucket to bucket aggregation persistence ([#3495](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3495))
- Use mirrors to download Node.js binaries to escape sporadic 404 errors ([#3619](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3619))
- [Multiple DataSource] Refactor dev tool console to use opensearch-js client to send requests ([#3544](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3544))
- Add `osd-xsrf` header to all requests that incorrectly used `node-version` to satisfy XSRF protection ([#3643](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3643))
- [Data] Add geo shape filter field ([#3605](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3605))

### 🐛 Bug Fixes
Expand Down Expand Up @@ -110,6 +111,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Region Maps] Add ui setting to configure custom vector map's size parameter([#3399](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3399))
- [Search Telemetry] Fixes search telemetry's observable object that won't be GC-ed([#3390](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3390))
- Clean up and rebuild `@osd/pm` ([#3570](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3570))
- Omit adding the `osd-version` header when the Fetch request is to an external origin ([#3643](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3643))
- [Vega] Add Filter custom label for opensearchDashboardsAddFilter ([#3640](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3640))
- [Timeline] Fix y-axis label color in dark mode ([#3698](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3698))

Expand Down
14 changes: 13 additions & 1 deletion src/core/public/http/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import { omitBy } from 'lodash';
import { format } from 'url';
import { BehaviorSubject } from 'rxjs';
import { isRelativeUrl } from '@osd/std';

import {
IBasePath,
Expand Down Expand Up @@ -144,7 +145,6 @@ export class Fetch {
headers: removedUndefined({
'Content-Type': 'application/json',
...options.headers,
'osd-version': this.params.opensearchDashboardsVersion,
}),
};

Expand All @@ -158,6 +158,18 @@ export class Fetch {
fetchOptions.headers['osd-system-request'] = 'true';
}

/* `osd-version` is used on the server-side to make sure that an incoming request originated from a front-end
* of the same version; see core/server/http/lifecycle_handlers.ts
* `osd-xsrf` is to satisfy XSRF protection but is only meaningful to OpenSearch Dashboards.
*
* If the `url` equals `basePath`, starts with `basePath` + '/', or is relative, add `osd-version` and `osd-xsrf` headers.
*/
const basePath = this.params.basePath.get();
if (isRelativeUrl(url) || url === basePath || url.startsWith(`${basePath}/`)) {
fetchOptions.headers['osd-version'] = this.params.opensearchDashboardsVersion;
fetchOptions.headers['osd-xsrf'] = 'osd-fetch';
}

return new Request(url, fetchOptions as RequestInit);
}

Expand Down

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

Original file line number Diff line number Diff line change
Expand Up @@ -231,20 +231,23 @@ describe('core lifecycle handlers', () => {
.expect(200, 'ok');
});

// ToDo: Remove next; `osd-version` incorrectly used for satisfying XSRF protection
it('accepts requests with the version header', async () => {
await getSupertest(method.toLowerCase(), testPath)
.set(versionHeader, actualVersion)
.expect(200, 'ok');
});

// ToDo: Rename next; `osd-version` incorrectly used for satisfying XSRF protection
it('rejects requests without either an xsrf or version header', async () => {
await getSupertest(method.toLowerCase(), testPath).expect(400, {
statusCode: 400,
error: 'Bad Request',
message: 'Request must contain a osd-xsrf header.',
message: 'Request must contain the osd-xsrf header.',
});
});

// ToDo: Rename next; `osd-version` incorrectly used for satisfying XSRF protection
it('accepts whitelisted requests without either an xsrf or version header', async () => {
await getSupertest(method.toLowerCase(), whitelistedTestPath).expect(200, 'ok');
});
Expand Down
9 changes: 5 additions & 4 deletions src/core/server/http/lifecycle_handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ describe('xsrf post-auth handler', () => {
expect(result).toEqual('next');
});

// ToDo: Remove; `osd-version` incorrectly used for satisfying XSRF protection
it('accepts requests with version header', () => {
const config = createConfig({ xsrf: { whitelist: [], disableProtection: false } });
const handler = createXsrfPostAuthHandler(config);
Expand Down Expand Up @@ -129,7 +130,7 @@ describe('xsrf post-auth handler', () => {
expect(responseFactory.badRequest).toHaveBeenCalledTimes(1);
expect(responseFactory.badRequest.mock.calls[0][0]).toMatchInlineSnapshot(`
Object {
"body": "Request must contain a osd-xsrf header.",
"body": "Request must contain the osd-xsrf header.",
}
`);
expect(result).toEqual('badRequest');
Expand Down Expand Up @@ -199,7 +200,7 @@ describe('versionCheck post-auth handler', () => {
responseFactory = httpServerMock.createLifecycleResponseFactory();
});

it('forward the request to the next interceptor if header matches', () => {
it('forward the request to the next interceptor if osd-version header matches the actual version', () => {
const handler = createVersionCheckPostAuthHandler('actual-version');
const request = forgeRequest({ headers: { 'osd-version': 'actual-version' } });

Expand All @@ -212,7 +213,7 @@ describe('versionCheck post-auth handler', () => {
expect(result).toBe('next');
});

it('returns a badRequest error if header does not match', () => {
it('returns a badRequest error if osd-version header exists but does not match the actual version', () => {
const handler = createVersionCheckPostAuthHandler('actual-version');
const request = forgeRequest({ headers: { 'osd-version': 'another-version' } });

Expand All @@ -236,7 +237,7 @@ describe('versionCheck post-auth handler', () => {
expect(result).toBe('badRequest');
});

it('forward the request to the next interceptor if header is not present', () => {
it('forward the request to the next interceptor if osd-version header is not present', () => {
const handler = createVersionCheckPostAuthHandler('actual-version');
const request = forgeRequest({ headers: {} });

Expand Down
3 changes: 2 additions & 1 deletion src/core/server/http/lifecycle_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler
const hasVersionHeader = VERSION_HEADER in request.headers;
const hasXsrfHeader = XSRF_HEADER in request.headers;

// ToDo: Remove !hasVersionHeader; `osd-version` incorrectly used for satisfying XSRF protection
Copy link
Member

Choose a reason for hiding this comment

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

nit - todo comments should have issues opened to link to.

if (!isSafeMethod(request.route.method) && !hasVersionHeader && !hasXsrfHeader) {
return response.badRequest({ body: `Request must contain a ${XSRF_HEADER} header.` });
return response.badRequest({ body: `Request must contain the ${XSRF_HEADER} header.` });
}

return toolkit.next();
Expand Down
1 change: 1 addition & 0 deletions src/plugins/bfetch/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export class BfetchPublicPlugin
url: `${basePath}/${removeLeadingSlash(params.url)}`,
headers: {
'Content-Type': 'application/json',
'osd-xsrf': 'osd-bfetch',
'osd-version': version,
...(params.headers || {}),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ export const $setupXsrfRequestInterceptor = (version: string) => {
// Configure jQuery prefilter
$.ajaxPrefilter(({ osdXsrfToken = true }: any, originalOptions, jqXHR) => {
if (osdXsrfToken) {
jqXHR.setRequestHeader('osd-xsrf', 'osd-legacy');
// ToDo: Remove next; `osd-version` incorrectly used for satisfying XSRF protection
jqXHR.setRequestHeader('osd-version', version);
}
});
Expand All @@ -170,6 +172,8 @@ export const $setupXsrfRequestInterceptor = (version: string) => {
request(opts) {
const { osdXsrfToken = true } = opts as any;
if (osdXsrfToken) {
set(opts, ['headers', 'osd-xsrf'], 'osd-legacy');
// ToDo: Remove next; `osd-version` incorrectly used for satisfying XSRF protection
set(opts, ['headers', 'osd-version'], version);
}
return opts;
Expand Down