From 40a6419dccfefa4a6e38e91cef110a741e12da7b Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Thu, 27 Feb 2020 11:23:10 +0100 Subject: [PATCH 01/11] add xsrfRequired flag to a route definition interface --- src/core/server/http/http_server.ts | 7 +++++-- src/core/server/http/lifecycle_handlers.ts | 6 +++++- src/core/server/http/router/index.ts | 1 + src/core/server/http/router/request.ts | 9 ++++++++- src/core/server/http/router/route.ts | 9 +++++++++ 5 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index 025ab2bf56ac2d..b1dc2eacefdff9 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -27,7 +27,7 @@ import { adoptToHapiOnPostAuthFormat, OnPostAuthHandler } from './lifecycle/on_p import { adoptToHapiOnPreAuthFormat, OnPreAuthHandler } from './lifecycle/on_pre_auth'; import { adoptToHapiOnPreResponseFormat, OnPreResponseHandler } from './lifecycle/on_pre_response'; -import { IRouter } from './router'; +import { IRouter, KibanaRouteState } from './router'; import { SessionStorageCookieOptions, createCookieSessionStorageFactory, @@ -148,8 +148,10 @@ export class HttpServer { this.log.debug(`registering route handler for [${route.path}]`); // Hapi does not allow payload validation to be specified for 'head' or 'get' requests const validate = ['head', 'get'].includes(route.method) ? undefined : { payload: true }; - const { authRequired = true, tags, body = {} } = route.options; + const { authRequired = true, tags, body = {}, xsrfRequired = true } = route.options; const { accepts: allow, maxBytes, output, parse } = body; + const kibanaRouteState: KibanaRouteState = { xsrfRequired }; + this.server.route({ handler: route.handler, method: route.method, @@ -157,6 +159,7 @@ export class HttpServer { options: { // Enforcing the comparison with true because plugins could overwrite the auth strategy by doing `options: { authRequired: authStrategy as any }` auth: authRequired === true ? undefined : false, + app: kibanaRouteState, tags: tags ? Array.from(tags) : undefined, // TODO: This 'validate' section can be removed once the legacy platform is completely removed. // We are telling Hapi that NP routes can accept any payload, so that it can bypass the default diff --git a/src/core/server/http/lifecycle_handlers.ts b/src/core/server/http/lifecycle_handlers.ts index ee877ee031a2bb..3d7a565dac64fd 100644 --- a/src/core/server/http/lifecycle_handlers.ts +++ b/src/core/server/http/lifecycle_handlers.ts @@ -31,7 +31,11 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler const { whitelist, disableProtection } = config.xsrf; return (request, response, toolkit) => { - if (disableProtection || whitelist.includes(request.route.path)) { + if ( + disableProtection || + whitelist.includes(request.route.path) || + request.route.options.xsrfRequired === false + ) { return toolkit.next(); } diff --git a/src/core/server/http/router/index.ts b/src/core/server/http/router/index.ts index 32663d1513f36b..06efce3e979c52 100644 --- a/src/core/server/http/router/index.ts +++ b/src/core/server/http/router/index.ts @@ -24,6 +24,7 @@ export { KibanaRequestEvents, KibanaRequestRoute, KibanaRequestRouteOptions, + KibanaRouteState, isRealRequest, LegacyRequest, ensureRawRequest, diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 703571ba53c0a3..7bb84369dc1273 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -18,7 +18,7 @@ */ import { Url } from 'url'; -import { Request } from 'hapi'; +import { Request, ApplicationState } from 'hapi'; import { Observable, fromEvent, merge } from 'rxjs'; import { shareReplay, first, takeUntil } from 'rxjs/operators'; @@ -30,6 +30,12 @@ import { RouteValidator, RouteValidatorFullConfig } from './validator'; const requestSymbol = Symbol('request'); +/** + * @internal + */ +export interface KibanaRouteState extends ApplicationState { + xsrfRequired: boolean; +} /** * Route options: If 'GET' or 'OPTIONS' method, body options won't be returned. * @public @@ -184,6 +190,7 @@ export class KibanaRequest< const options = ({ authRequired: request.route.settings.auth !== false, + xsrfRequired: (request.app as KibanaRouteState).xsrfRequired, tags: request.route.settings.tags || [], body: ['get', 'options'].includes(method) ? undefined diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index 4439a80b1eac71..92991a978c39b3 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -108,6 +108,15 @@ export interface RouteConfigOptions { */ authRequired?: boolean; + /** + * Defines xsrf protection for a route: + * - true. Requires an incoming request to contain `kbn-xsrf` header. + * - false. Disables xsrf protection. + * + * Set to true by default + */ + xsrfRequired?: boolean; + /** * Additional metadata tag strings to attach to the route. */ From 6b3b4475e5fa893edf4962a5e7bab708ec3c55eb Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Thu, 27 Feb 2020 11:23:25 +0100 Subject: [PATCH 02/11] update tests --- src/core/server/http/http_server.mocks.ts | 5 ++++ .../lifecycle_handlers.test.ts | 11 +++++++ .../server/http/lifecycle_handlers.test.ts | 29 +++++++++++++++++-- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index c586cf6a9825fa..6051d3f08301ec 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -29,6 +29,7 @@ import { RouteMethod, KibanaResponseFactory, RouteValidationSpec, + KibanaRouteState, } from './router'; import { OnPreResponseToolkit } from './lifecycle/on_pre_response'; import { OnPostAuthToolkit } from './lifecycle/on_post_auth'; @@ -43,6 +44,7 @@ interface RequestFixtureOptions

{ method?: RouteMethod; socket?: Socket; routeTags?: string[]; + kibanaRouteState?: KibanaRouteState; validation?: { params?: RouteValidationSpec

; query?: RouteValidationSpec; @@ -60,11 +62,13 @@ function createKibanaRequestMock

({ socket = new Socket(), routeTags, validation = {}, + kibanaRouteState = { xsrfRequired: true }, }: RequestFixtureOptions = {}) { const queryString = stringify(query, { sort: false }); return KibanaRequest.from( createRawRequestMock({ + app: kibanaRouteState, headers, params, query, @@ -105,6 +109,7 @@ function createRawRequestMock(customization: DeepPartial = {}) { return merge( {}, { + app: { xsrfRequired: true } as any, headers: {}, path: '/', route: { settings: {} }, diff --git a/src/core/server/http/integration_tests/lifecycle_handlers.test.ts b/src/core/server/http/integration_tests/lifecycle_handlers.test.ts index f4c5f16870c7ed..89995f1f4cf14e 100644 --- a/src/core/server/http/integration_tests/lifecycle_handlers.test.ts +++ b/src/core/server/http/integration_tests/lifecycle_handlers.test.ts @@ -36,6 +36,7 @@ const versionHeader = 'kbn-version'; const xsrfHeader = 'kbn-xsrf'; const nameHeader = 'kbn-name'; const whitelistedTestPath = '/xsrf/test/route/whitelisted'; +const xsrfDisabledTestPath = '/xsrf/test/route/disabled'; const kibanaName = 'my-kibana-name'; const setupDeps = { context: contextServiceMock.createSetupContract(), @@ -188,6 +189,12 @@ describe('core lifecycle handlers', () => { return res.ok({ body: 'ok' }); } ); + ((router as any)[method.toLowerCase()] as RouteRegistrar)( + { path: xsrfDisabledTestPath, validate: false, options: { xsrfRequired: false } }, + (context, req, res) => { + return res.ok({ body: 'ok' }); + } + ); }); await server.start(); @@ -235,6 +242,10 @@ describe('core lifecycle handlers', () => { it('accepts whitelisted requests without either an xsrf or version header', async () => { await getSupertest(method.toLowerCase(), whitelistedTestPath).expect(200, 'ok'); }); + + it('accepts requests on a route with disabled xsrf protection', async () => { + await getSupertest(method.toLowerCase(), whitelistedTestPath).expect(200, 'ok'); + }); }); }); }); diff --git a/src/core/server/http/lifecycle_handlers.test.ts b/src/core/server/http/lifecycle_handlers.test.ts index 48a6973b741ba0..a80e432e0d4cb7 100644 --- a/src/core/server/http/lifecycle_handlers.test.ts +++ b/src/core/server/http/lifecycle_handlers.test.ts @@ -24,7 +24,7 @@ import { } from './lifecycle_handlers'; import { httpServerMock } from './http_server.mocks'; import { HttpConfig } from './http_config'; -import { KibanaRequest, RouteMethod } from './router'; +import { KibanaRequest, RouteMethod, KibanaRouteState } from './router'; const createConfig = (partial: Partial): HttpConfig => partial as HttpConfig; @@ -32,12 +32,14 @@ const forgeRequest = ({ headers = {}, path = '/', method = 'get', + kibanaRouteState, }: Partial<{ headers: Record; path: string; method: RouteMethod; + kibanaRouteState: KibanaRouteState; }>): KibanaRequest => { - return httpServerMock.createKibanaRequest({ headers, path, method }); + return httpServerMock.createKibanaRequest({ headers, path, method, kibanaRouteState }); }; describe('xsrf post-auth handler', () => { @@ -142,6 +144,29 @@ describe('xsrf post-auth handler', () => { expect(toolkit.next).toHaveBeenCalledTimes(1); expect(result).toEqual('next'); }); + + it('accepts requests if xsrf protection on a route is disabled', () => { + const config = createConfig({ + xsrf: { whitelist: [], disableProtection: false }, + }); + const handler = createXsrfPostAuthHandler(config); + const request = forgeRequest({ + method: 'post', + headers: {}, + path: '/some-path', + kibanaRouteState: { + xsrfRequired: false, + }, + }); + + toolkit.next.mockReturnValue('next' as any); + + const result = handler(request, responseFactory, toolkit); + + expect(responseFactory.badRequest).not.toHaveBeenCalled(); + expect(toolkit.next).toHaveBeenCalledTimes(1); + expect(result).toEqual('next'); + }); }); }); From b466852aa8c5607adef39c665283b053fe1f951e Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Thu, 27 Feb 2020 11:29:51 +0100 Subject: [PATCH 03/11] deprecate server.xsrf.whitelist It meant to be used for IdP endpoints only, which we are going to refactor to disable xsrf requirement per a specific endpoint. --- .../config/deprecation/core_deprecations.test.ts | 13 +++++++++++++ .../server/config/deprecation/core_deprecations.ts | 11 +++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/core/server/config/deprecation/core_deprecations.test.ts b/src/core/server/config/deprecation/core_deprecations.test.ts index b40dbdc1b66519..c4a3c74e6c296f 100644 --- a/src/core/server/config/deprecation/core_deprecations.test.ts +++ b/src/core/server/config/deprecation/core_deprecations.test.ts @@ -81,6 +81,19 @@ describe('core deprecations', () => { }); }); + describe('xsrfDeprecation', () => { + it('logs a warning if server.xsrf.whitelist is set', () => { + const { messages } = applyCoreDeprecations({ + server: { xsrf: { whitelist: ['/path'] } }, + }); + expect(messages).toMatchInlineSnapshot(` + Array [ + "It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].It will be removed in 8.0 release. Instead, supply the \\"kbn-xsrf\\" header.", + ] + `); + }); + }); + describe('rewriteBasePath', () => { it('logs a warning is server.basePath is set and server.rewriteBasePath is not', () => { const { messages } = applyCoreDeprecations({ diff --git a/src/core/server/config/deprecation/core_deprecations.ts b/src/core/server/config/deprecation/core_deprecations.ts index 4fa51dcd5a0825..36f6c565d9259c 100644 --- a/src/core/server/config/deprecation/core_deprecations.ts +++ b/src/core/server/config/deprecation/core_deprecations.ts @@ -38,6 +38,16 @@ const dataPathDeprecation: ConfigDeprecation = (settings, fromPath, log) => { return settings; }; +const xsrfDeprecation: ConfigDeprecation = (settings, fromPath, log) => { + if (has(settings, 'server.xsrf.whitelist')) { + log( + 'It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].' + + 'It will be removed in 8.0 release. Instead, supply the "kbn-xsrf" header.' + ); + } + return settings; +}; + const rewriteBasePathDeprecation: ConfigDeprecation = (settings, fromPath, log) => { if (has(settings, 'server.basePath') && !has(settings, 'server.rewriteBasePath')) { log( @@ -177,4 +187,5 @@ export const coreDeprecationProvider: ConfigDeprecationProvider = ({ rewriteBasePathDeprecation, cspRulesDeprecation, mapManifestServiceUrlDeprecation, + xsrfDeprecation, ]; From 263689de749a05d2cc0f6cdfad9fb7f4cd027de0 Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 27 Feb 2020 11:31:39 +0100 Subject: [PATCH 04/11] update docs --- .../kibana-plugin-server.routeconfigoptions.md | 1 + ...ugin-server.routeconfigoptions.xsrfrequired.md | 15 +++++++++++++++ src/core/server/http/router/route.ts | 2 +- src/core/server/server.api.md | 1 + 4 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md diff --git a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md index 0929e15b6228b7..dd7fb51ea2946e 100644 --- a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md +++ b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md @@ -19,4 +19,5 @@ export interface RouteConfigOptions | [authRequired](./kibana-plugin-server.routeconfigoptions.authrequired.md) | boolean | A flag shows that authentication for a route: enabled when true disabled when falseEnabled by default. | | [body](./kibana-plugin-server.routeconfigoptions.body.md) | Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody | Additional body options [RouteConfigOptionsBody](./kibana-plugin-server.routeconfigoptionsbody.md). | | [tags](./kibana-plugin-server.routeconfigoptions.tags.md) | readonly string[] | Additional metadata tag strings to attach to the route. | +| [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md) | boolean | Defines xsrf protection requirements for a route: - true. Requires an incoming request to contain kbn-xsrf header. - false. Disables xsrf protection.Set to true by default | diff --git a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md new file mode 100644 index 00000000000000..b6195e6b83649c --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md @@ -0,0 +1,15 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [RouteConfigOptions](./kibana-plugin-server.routeconfigoptions.md) > [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md) + +## RouteConfigOptions.xsrfRequired property + +Defines xsrf protection requirements for a route: - true. Requires an incoming request to contain `kbn-xsrf` header. - false. Disables xsrf protection. + +Set to true by default + +Signature: + +```typescript +xsrfRequired?: boolean; +``` diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index 92991a978c39b3..a4f2df230b5352 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -109,7 +109,7 @@ export interface RouteConfigOptions { authRequired?: boolean; /** - * Defines xsrf protection for a route: + * Defines xsrf protection requirements for a route: * - true. Requires an incoming request to contain `kbn-xsrf` header. * - false. Disables xsrf protection. * diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 8f4feb7169651e..795c8eb69592c0 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1397,6 +1397,7 @@ export interface RouteConfigOptions { authRequired?: boolean; body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody; tags?: readonly string[]; + xsrfRequired?: boolean; } // @public From a8b9f0937d55de9bf584d82f7bdbb2ff8f87c301 Mon Sep 17 00:00:00 2001 From: restrry Date: Thu, 27 Feb 2020 15:20:13 +0100 Subject: [PATCH 05/11] do not fail on manual KibanaRequest creation --- src/core/server/http/router/request.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 7bb84369dc1273..614a4af9dfeb47 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -190,7 +190,8 @@ export class KibanaRequest< const options = ({ authRequired: request.route.settings.auth !== false, - xsrfRequired: (request.app as KibanaRouteState).xsrfRequired, + // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 + xsrfRequired: (request.app as KibanaRouteState)?.xsrfRequired || true, tags: request.route.settings.tags || [], body: ['get', 'options'].includes(method) ? undefined From 081527cbc1f36f853d72412189a9ab79f729c8ce Mon Sep 17 00:00:00 2001 From: restrry Date: Fri, 28 Feb 2020 10:23:02 +0100 Subject: [PATCH 06/11] address comments --- src/core/server/config/deprecation/core_deprecations.ts | 5 ++++- src/core/server/http/router/request.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/core/server/config/deprecation/core_deprecations.ts b/src/core/server/config/deprecation/core_deprecations.ts index 36f6c565d9259c..26b61fa37c6d2a 100644 --- a/src/core/server/config/deprecation/core_deprecations.ts +++ b/src/core/server/config/deprecation/core_deprecations.ts @@ -39,7 +39,10 @@ const dataPathDeprecation: ConfigDeprecation = (settings, fromPath, log) => { }; const xsrfDeprecation: ConfigDeprecation = (settings, fromPath, log) => { - if (has(settings, 'server.xsrf.whitelist')) { + if ( + has(settings, 'server.xsrf.whitelist') && + get(settings, 'server.xsrf.whitelist').length > 0 + ) { log( 'It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].' + 'It will be removed in 8.0 release. Instead, supply the "kbn-xsrf" header.' diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 614a4af9dfeb47..e96b799a0b8b64 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -191,7 +191,7 @@ export class KibanaRequest< const options = ({ authRequired: request.route.settings.auth !== false, // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 - xsrfRequired: (request.app as KibanaRouteState)?.xsrfRequired || true, + xsrfRequired: (request.app as KibanaRouteState)?.xsrfRequired ?? true, tags: request.route.settings.tags || [], body: ['get', 'options'].includes(method) ? undefined From 5e54a83903274c27d54084d7813d8aa5373ac83c Mon Sep 17 00:00:00 2001 From: restrry Date: Fri, 28 Feb 2020 16:35:49 +0100 Subject: [PATCH 07/11] update tests --- src/core/server/http/http_server.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index a9fc80c86d878e..e8635a3a808540 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -811,6 +811,7 @@ test('exposes route details of incoming request to a route handler', async () => path: '/', options: { authRequired: true, + xsrfRequired: true, tags: [], }, }); @@ -923,6 +924,7 @@ test('exposes route details of incoming request to a route handler (POST + paylo path: '/', options: { authRequired: true, + xsrfRequired: true, tags: [], body: { parse: true, // hapi populates the default From 016f6641f1af532b4908e88c6718c0f0b28b5489 Mon Sep 17 00:00:00 2001 From: restrry Date: Mon, 2 Mar 2020 13:34:37 +0100 Subject: [PATCH 08/11] address comments --- src/core/server/config/deprecation/core_deprecations.test.ts | 2 +- src/core/server/config/deprecation/core_deprecations.ts | 2 +- src/core/server/http/http_server.mocks.ts | 3 +-- .../server/http/integration_tests/lifecycle_handlers.test.ts | 2 +- src/core/server/http/router/request.ts | 2 +- src/core/server/http/router/route.ts | 2 +- 6 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/server/config/deprecation/core_deprecations.test.ts b/src/core/server/config/deprecation/core_deprecations.test.ts index c4a3c74e6c296f..a91e128f62d2dc 100644 --- a/src/core/server/config/deprecation/core_deprecations.test.ts +++ b/src/core/server/config/deprecation/core_deprecations.test.ts @@ -88,7 +88,7 @@ describe('core deprecations', () => { }); expect(messages).toMatchInlineSnapshot(` Array [ - "It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].It will be removed in 8.0 release. Instead, supply the \\"kbn-xsrf\\" header.", + "It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist]. It will be removed in 8.0 release. Instead, supply the \\"kbn-xsrf\\" header.", ] `); }); diff --git a/src/core/server/config/deprecation/core_deprecations.ts b/src/core/server/config/deprecation/core_deprecations.ts index 26b61fa37c6d2a..d91e55115d0b13 100644 --- a/src/core/server/config/deprecation/core_deprecations.ts +++ b/src/core/server/config/deprecation/core_deprecations.ts @@ -44,7 +44,7 @@ const xsrfDeprecation: ConfigDeprecation = (settings, fromPath, log) => { get(settings, 'server.xsrf.whitelist').length > 0 ) { log( - 'It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].' + + 'It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist]. ' + 'It will be removed in 8.0 release. Instead, supply the "kbn-xsrf" header.' ); } diff --git a/src/core/server/http/http_server.mocks.ts b/src/core/server/http/http_server.mocks.ts index 0413dc6c27986b..741c723ca93652 100644 --- a/src/core/server/http/http_server.mocks.ts +++ b/src/core/server/http/http_server.mocks.ts @@ -70,7 +70,6 @@ function createKibanaRequestMock

({ return KibanaRequest.from( createRawRequestMock({ - app: kibanaRouteState, headers, params, query, @@ -84,7 +83,7 @@ function createKibanaRequestMock

({ search: queryString ? `?${queryString}` : queryString, }, route: { - settings: { tags: routeTags, auth: routeAuthRequired }, + settings: { tags: routeTags, auth: routeAuthRequired, app: kibanaRouteState }, }, raw: { req: { socket }, diff --git a/src/core/server/http/integration_tests/lifecycle_handlers.test.ts b/src/core/server/http/integration_tests/lifecycle_handlers.test.ts index 89995f1f4cf14e..b5364c616f17cf 100644 --- a/src/core/server/http/integration_tests/lifecycle_handlers.test.ts +++ b/src/core/server/http/integration_tests/lifecycle_handlers.test.ts @@ -244,7 +244,7 @@ describe('core lifecycle handlers', () => { }); it('accepts requests on a route with disabled xsrf protection', async () => { - await getSupertest(method.toLowerCase(), whitelistedTestPath).expect(200, 'ok'); + await getSupertest(method.toLowerCase(), xsrfDisabledTestPath).expect(200, 'ok'); }); }); }); diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index e96b799a0b8b64..73c3b6a6f4065b 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -191,7 +191,7 @@ export class KibanaRequest< const options = ({ authRequired: request.route.settings.auth !== false, // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 - xsrfRequired: (request.app as KibanaRouteState)?.xsrfRequired ?? true, + xsrfRequired: (request.route.settings.app as KibanaRouteState)?.xsrfRequired ?? true, tags: request.route.settings.tags || [], body: ['get', 'options'].includes(method) ? undefined diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index a4f2df230b5352..ada374c752bfbb 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -110,7 +110,7 @@ export interface RouteConfigOptions { /** * Defines xsrf protection requirements for a route: - * - true. Requires an incoming request to contain `kbn-xsrf` header. + * - true. Requires an incoming POST/PUT/DELETE request to contain `kbn-xsrf` header. * - false. Disables xsrf protection. * * Set to true by default From 07d72488def3a24e4844d3dbc32ebce7d19ceab6 Mon Sep 17 00:00:00 2001 From: restrry Date: Tue, 3 Mar 2020 10:08:49 +0100 Subject: [PATCH 09/11] make xsrfRequired available only for destructive methods --- src/core/server/http/http_server.test.ts | 2 +- src/core/server/http/http_server.ts | 11 +++++++---- src/core/server/http/router/index.ts | 1 + src/core/server/http/router/request.ts | 4 ++-- src/core/server/http/router/route.ts | 10 ++++++++-- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index e8635a3a808540..27db79bb94d252 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -811,7 +811,7 @@ test('exposes route details of incoming request to a route handler', async () => path: '/', options: { authRequired: true, - xsrfRequired: true, + xsrfRequired: false, tags: [], }, }); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index b1dc2eacefdff9..cffdffab0d0cf7 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -27,7 +27,7 @@ import { adoptToHapiOnPostAuthFormat, OnPostAuthHandler } from './lifecycle/on_p import { adoptToHapiOnPreAuthFormat, OnPreAuthHandler } from './lifecycle/on_pre_auth'; import { adoptToHapiOnPreResponseFormat, OnPreResponseHandler } from './lifecycle/on_pre_response'; -import { IRouter, KibanaRouteState } from './router'; +import { IRouter, KibanaRouteState, isSafeMethod } from './router'; import { SessionStorageCookieOptions, createCookieSessionStorageFactory, @@ -147,10 +147,13 @@ export class HttpServer { for (const route of router.getRoutes()) { this.log.debug(`registering route handler for [${route.path}]`); // Hapi does not allow payload validation to be specified for 'head' or 'get' requests - const validate = ['head', 'get'].includes(route.method) ? undefined : { payload: true }; - const { authRequired = true, tags, body = {}, xsrfRequired = true } = route.options; + const validate = isSafeMethod(route.method) ? undefined : { payload: true }; + const { authRequired = true, tags, body = {} } = route.options; const { accepts: allow, maxBytes, output, parse } = body; - const kibanaRouteState: KibanaRouteState = { xsrfRequired }; + + const kibanaRouteState: KibanaRouteState = { + xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), + }; this.server.route({ handler: route.handler, diff --git a/src/core/server/http/router/index.ts b/src/core/server/http/router/index.ts index 06efce3e979c52..787d8c3127c473 100644 --- a/src/core/server/http/router/index.ts +++ b/src/core/server/http/router/index.ts @@ -30,6 +30,7 @@ export { ensureRawRequest, } from './request'; export { + isSafeMethod, RouteMethod, RouteConfig, RouteConfigOptions, diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 73c3b6a6f4065b..bb2db6367f701f 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -24,7 +24,7 @@ import { shareReplay, first, takeUntil } from 'rxjs/operators'; import { deepFreeze, RecursiveReadonly } from '../../../utils'; import { Headers } from './headers'; -import { RouteMethod, RouteConfigOptions, validBodyOutput } from './route'; +import { RouteMethod, RouteConfigOptions, validBodyOutput, isSafeMethod } from './route'; import { KibanaSocket, IKibanaSocket } from './socket'; import { RouteValidator, RouteValidatorFullConfig } from './validator'; @@ -193,7 +193,7 @@ export class KibanaRequest< // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 xsrfRequired: (request.route.settings.app as KibanaRouteState)?.xsrfRequired ?? true, tags: request.route.settings.tags || [], - body: ['get', 'options'].includes(method) + body: isSafeMethod(method) ? undefined : { parse, diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index ada374c752bfbb..8d1d7d30f593e7 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -19,11 +19,17 @@ import { RouteValidatorFullConfig } from './validator'; +export function isSafeMethod(method: RouteMethod): method is SafeRouteMethod { + return method === 'get' || method === 'options'; +} + +export type DestructiveRouteMethod = 'post' | 'put' | 'delete' | 'patch'; +export type SafeRouteMethod = 'get' | 'options'; /** * The set of common HTTP methods supported by Kibana routing. * @public */ -export type RouteMethod = 'get' | 'post' | 'put' | 'delete' | 'patch' | 'options'; +export type RouteMethod = SafeRouteMethod | DestructiveRouteMethod; /** * The set of valid body.output @@ -115,7 +121,7 @@ export interface RouteConfigOptions { * * Set to true by default */ - xsrfRequired?: boolean; + xsrfRequired?: Method extends 'get' ? never : boolean; /** * Additional metadata tag strings to attach to the route. From e7829d907f4938629cc7dc9c182c83b660cda8ab Mon Sep 17 00:00:00 2001 From: restrry Date: Tue, 3 Mar 2020 10:21:00 +0100 Subject: [PATCH 10/11] update docs --- .../kibana-plugin-server.destructiveroutemethod.md | 13 +++++++++++++ .../development/core/server/kibana-plugin-server.md | 2 ++ .../kibana-plugin-server.routeconfigoptions.md | 2 +- ...plugin-server.routeconfigoptions.xsrfrequired.md | 4 ++-- .../core/server/kibana-plugin-server.routemethod.md | 2 +- .../server/kibana-plugin-server.saferoutemethod.md | 13 +++++++++++++ src/core/server/http/index.ts | 2 ++ src/core/server/http/router/index.ts | 2 ++ src/core/server/http/router/route.ts | 10 ++++++++++ src/core/server/index.ts | 2 ++ src/core/server/server.api.md | 10 ++++++++-- 11 files changed, 56 insertions(+), 6 deletions(-) create mode 100644 docs/development/core/server/kibana-plugin-server.destructiveroutemethod.md create mode 100644 docs/development/core/server/kibana-plugin-server.saferoutemethod.md diff --git a/docs/development/core/server/kibana-plugin-server.destructiveroutemethod.md b/docs/development/core/server/kibana-plugin-server.destructiveroutemethod.md new file mode 100644 index 00000000000000..48b1e837f6db95 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.destructiveroutemethod.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [DestructiveRouteMethod](./kibana-plugin-server.destructiveroutemethod.md) + +## DestructiveRouteMethod type + +Set of HTTP methods changing the state of the server. + +Signature: + +```typescript +export declare type DestructiveRouteMethod = 'post' | 'put' | 'delete' | 'patch'; +``` diff --git a/docs/development/core/server/kibana-plugin-server.md b/docs/development/core/server/kibana-plugin-server.md index 15a1fd05062568..0de321e06d0ccd 100644 --- a/docs/development/core/server/kibana-plugin-server.md +++ b/docs/development/core/server/kibana-plugin-server.md @@ -183,6 +183,7 @@ The plugin integrates with the core system via lifecycle events: `setup` | [ConfigDeprecationLogger](./kibana-plugin-server.configdeprecationlogger.md) | Logger interface used when invoking a [ConfigDeprecation](./kibana-plugin-server.configdeprecation.md) | | [ConfigDeprecationProvider](./kibana-plugin-server.configdeprecationprovider.md) | A provider that should returns a list of [ConfigDeprecation](./kibana-plugin-server.configdeprecation.md).See [ConfigDeprecationFactory](./kibana-plugin-server.configdeprecationfactory.md) for more usage examples. | | [ConfigPath](./kibana-plugin-server.configpath.md) | | +| [DestructiveRouteMethod](./kibana-plugin-server.destructiveroutemethod.md) | Set of HTTP methods changing the state of the server. | | [ElasticsearchClientConfig](./kibana-plugin-server.elasticsearchclientconfig.md) | | | [GetAuthHeaders](./kibana-plugin-server.getauthheaders.md) | Get headers to authenticate a user against Elasticsearch. | | [GetAuthState](./kibana-plugin-server.getauthstate.md) | Gets authentication state for a request. Returned by auth interceptor. | @@ -227,6 +228,7 @@ The plugin integrates with the core system via lifecycle events: `setup` | [RouteValidationFunction](./kibana-plugin-server.routevalidationfunction.md) | The custom validation function if @kbn/config-schema is not a valid solution for your specific plugin requirements. | | [RouteValidationSpec](./kibana-plugin-server.routevalidationspec.md) | Allowed property validation options: either @kbn/config-schema validations or custom validation functionsSee [RouteValidationFunction](./kibana-plugin-server.routevalidationfunction.md) for custom validation. | | [RouteValidatorFullConfig](./kibana-plugin-server.routevalidatorfullconfig.md) | Route validations config and options merged into one object | +| [SafeRouteMethod](./kibana-plugin-server.saferoutemethod.md) | Set of HTTP methods not changing the state of the server. | | [SavedObjectAttribute](./kibana-plugin-server.savedobjectattribute.md) | Type definition for a Saved Object attribute value | | [SavedObjectAttributeSingle](./kibana-plugin-server.savedobjectattributesingle.md) | Don't use this type, it's simply a helper type for [SavedObjectAttribute](./kibana-plugin-server.savedobjectattribute.md) | | [SavedObjectMigrationFn](./kibana-plugin-server.savedobjectmigrationfn.md) | A migration function for a [saved object type](./kibana-plugin-server.savedobjectstype.md) used to migrate it to a given version | diff --git a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md index dd7fb51ea2946e..7fbab90cc2c8a6 100644 --- a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md +++ b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.md @@ -19,5 +19,5 @@ export interface RouteConfigOptions | [authRequired](./kibana-plugin-server.routeconfigoptions.authrequired.md) | boolean | A flag shows that authentication for a route: enabled when true disabled when falseEnabled by default. | | [body](./kibana-plugin-server.routeconfigoptions.body.md) | Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody | Additional body options [RouteConfigOptionsBody](./kibana-plugin-server.routeconfigoptionsbody.md). | | [tags](./kibana-plugin-server.routeconfigoptions.tags.md) | readonly string[] | Additional metadata tag strings to attach to the route. | -| [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md) | boolean | Defines xsrf protection requirements for a route: - true. Requires an incoming request to contain kbn-xsrf header. - false. Disables xsrf protection.Set to true by default | +| [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md) | Method extends 'get' ? never : boolean | Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain kbn-xsrf header. - false. Disables xsrf protection.Set to true by default | diff --git a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md index b6195e6b83649c..801a0c3dc299b9 100644 --- a/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md +++ b/docs/development/core/server/kibana-plugin-server.routeconfigoptions.xsrfrequired.md @@ -4,12 +4,12 @@ ## RouteConfigOptions.xsrfRequired property -Defines xsrf protection requirements for a route: - true. Requires an incoming request to contain `kbn-xsrf` header. - false. Disables xsrf protection. +Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain `kbn-xsrf` header. - false. Disables xsrf protection. Set to true by default Signature: ```typescript -xsrfRequired?: boolean; +xsrfRequired?: Method extends 'get' ? never : boolean; ``` diff --git a/docs/development/core/server/kibana-plugin-server.routemethod.md b/docs/development/core/server/kibana-plugin-server.routemethod.md index 939ae94b85691b..ed0d8e9af4b194 100644 --- a/docs/development/core/server/kibana-plugin-server.routemethod.md +++ b/docs/development/core/server/kibana-plugin-server.routemethod.md @@ -9,5 +9,5 @@ The set of common HTTP methods supported by Kibana routing. Signature: ```typescript -export declare type RouteMethod = 'get' | 'post' | 'put' | 'delete' | 'patch' | 'options'; +export declare type RouteMethod = SafeRouteMethod | DestructiveRouteMethod; ``` diff --git a/docs/development/core/server/kibana-plugin-server.saferoutemethod.md b/docs/development/core/server/kibana-plugin-server.saferoutemethod.md new file mode 100644 index 00000000000000..432aa4c6e70147 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-server.saferoutemethod.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-server](./kibana-plugin-server.md) > [SafeRouteMethod](./kibana-plugin-server.saferoutemethod.md) + +## SafeRouteMethod type + +Set of HTTP methods not changing the state of the server. + +Signature: + +```typescript +export declare type SafeRouteMethod = 'get' | 'options'; +``` diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts index d31afe1670e419..8f4c02680f8a30 100644 --- a/src/core/server/http/index.ts +++ b/src/core/server/http/index.ts @@ -58,6 +58,8 @@ export { RouteValidationError, RouteValidatorFullConfig, RouteValidationResultFactory, + DestructiveRouteMethod, + SafeRouteMethod, } from './router'; export { BasePathProxyServer } from './base_path_proxy_server'; export { OnPreAuthHandler, OnPreAuthToolkit } from './lifecycle/on_pre_auth'; diff --git a/src/core/server/http/router/index.ts b/src/core/server/http/router/index.ts index 787d8c3127c473..d254f391ca5e41 100644 --- a/src/core/server/http/router/index.ts +++ b/src/core/server/http/router/index.ts @@ -30,12 +30,14 @@ export { ensureRawRequest, } from './request'; export { + DestructiveRouteMethod, isSafeMethod, RouteMethod, RouteConfig, RouteConfigOptions, RouteContentType, RouteConfigOptionsBody, + SafeRouteMethod, validBodyOutput, } from './route'; export { HapiResponseAdapter } from './response_adapter'; diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index 8d1d7d30f593e7..d1458ef4ad0632 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -23,8 +23,18 @@ export function isSafeMethod(method: RouteMethod): method is SafeRouteMethod { return method === 'get' || method === 'options'; } +/** + * Set of HTTP methods changing the state of the server. + * @public + */ export type DestructiveRouteMethod = 'post' | 'put' | 'delete' | 'patch'; + +/** + * Set of HTTP methods not changing the state of the server. + * @public + */ export type SafeRouteMethod = 'get' | 'options'; + /** * The set of common HTTP methods supported by Kibana routing. * @public diff --git a/src/core/server/index.ts b/src/core/server/index.ts index e45d4f28edcc37..d5f6d4cfd1219b 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -159,6 +159,8 @@ export { SessionStorageCookieOptions, SessionCookieValidationResult, SessionStorageFactory, + DestructiveRouteMethod, + SafeRouteMethod, } from './http'; export { RenderingServiceSetup, IRenderOptions } from './rendering'; export { Logger, LoggerFactory, LogMeta, LogRecord, LogLevel } from './logging'; diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 03f75b1b3c8987..96821e09e699f6 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -685,6 +685,9 @@ export interface DeprecationSettings { message: string; } +// @public +export type DestructiveRouteMethod = 'post' | 'put' | 'delete' | 'patch'; + // @public export interface DiscoveredPlugin { readonly configPath: ConfigPath; @@ -1397,7 +1400,7 @@ export interface RouteConfigOptions { authRequired?: boolean; body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody; tags?: readonly string[]; - xsrfRequired?: boolean; + xsrfRequired?: Method extends 'get' ? never : boolean; } // @public @@ -1412,7 +1415,7 @@ export interface RouteConfigOptionsBody { export type RouteContentType = 'application/json' | 'application/*+json' | 'application/octet-stream' | 'application/x-www-form-urlencoded' | 'multipart/form-data' | 'text/*'; // @public -export type RouteMethod = 'get' | 'post' | 'put' | 'delete' | 'patch' | 'options'; +export type RouteMethod = SafeRouteMethod | DestructiveRouteMethod; // @public export type RouteRegistrar = (route: RouteConfig, handler: RequestHandler) => void; @@ -1465,6 +1468,9 @@ export interface RouteValidatorOptions { }; } +// @public +export type SafeRouteMethod = 'get' | 'options'; + // @public (undocumented) export interface SavedObject { attributes: T; From a5c12a8638f5ce54d7a698f5873bdeba347256db Mon Sep 17 00:00:00 2001 From: restrry Date: Tue, 3 Mar 2020 13:42:36 +0100 Subject: [PATCH 11/11] another isSafeMethod usage --- src/core/server/http/lifecycle_handlers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/server/http/lifecycle_handlers.ts b/src/core/server/http/lifecycle_handlers.ts index 3d7a565dac64fd..7ef7e863260391 100644 --- a/src/core/server/http/lifecycle_handlers.ts +++ b/src/core/server/http/lifecycle_handlers.ts @@ -20,6 +20,7 @@ import { OnPostAuthHandler } from './lifecycle/on_post_auth'; import { OnPreResponseHandler } from './lifecycle/on_pre_response'; import { HttpConfig } from './http_config'; +import { isSafeMethod } from './router'; import { Env } from '../config'; import { LifecycleRegistrar } from './http_server'; @@ -39,11 +40,10 @@ export const createXsrfPostAuthHandler = (config: HttpConfig): OnPostAuthHandler return toolkit.next(); } - const isSafeMethod = request.route.method === 'get' || request.route.method === 'head'; const hasVersionHeader = VERSION_HEADER in request.headers; const hasXsrfHeader = XSRF_HEADER in request.headers; - if (!isSafeMethod && !hasVersionHeader && !hasXsrfHeader) { + if (!isSafeMethod(request.route.method) && !hasVersionHeader && !hasXsrfHeader) { return response.badRequest({ body: `Request must contain a ${XSRF_HEADER} header.` }); }