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

Allow disabling xsrf protection per an endpoint #58717

Merged
merged 15 commits into from
Mar 3, 2020
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ export interface RouteConfigOptions<Method extends RouteMethod>
| [authRequired](./kibana-plugin-server.routeconfigoptions.authrequired.md) | <code>boolean</code> | A flag shows that authentication for a route: <code>enabled</code> when true <code>disabled</code> when false<!-- -->Enabled by default. |
| [body](./kibana-plugin-server.routeconfigoptions.body.md) | <code>Method extends 'get' &#124; 'options' ? undefined : RouteConfigOptionsBody</code> | Additional body options [RouteConfigOptionsBody](./kibana-plugin-server.routeconfigoptionsbody.md)<!-- -->. |
| [tags](./kibana-plugin-server.routeconfigoptions.tags.md) | <code>readonly string[]</code> | Additional metadata tag strings to attach to the route. |
| [xsrfRequired](./kibana-plugin-server.routeconfigoptions.xsrfrequired.md) | <code>boolean</code> | Defines xsrf protection requirements for a route: - true. Requires an incoming request to contain <code>kbn-xsrf</code> header. - false. Disables xsrf protection.<!-- -->Set to true by default |

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-server](./kibana-plugin-server.md) &gt; [RouteConfigOptions](./kibana-plugin-server.routeconfigoptions.md) &gt; [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

<b>Signature:</b>

```typescript
xsrfRequired?: boolean;
```
13 changes: 13 additions & 0 deletions src/core/server/config/deprecation/core_deprecations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
11 changes: 11 additions & 0 deletions src/core/server/config/deprecation/core_deprecations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
mshustov marked this conversation as resolved.
Show resolved Hide resolved
'It is not recommended to disable xsrf protections for API endpoints via [server.xsrf.whitelist].' +
mshustov marked this conversation as resolved.
Show resolved Hide resolved
'It will be removed in 8.0 release. Instead, supply the "kbn-xsrf" header.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshdover what is the appropriate place to track v8 breaking changes after #40768 was closed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this doc: docs/migration/migrate_8_0.asciidoc

);
}
return settings;
};

const rewriteBasePathDeprecation: ConfigDeprecation = (settings, fromPath, log) => {
if (has(settings, 'server.basePath') && !has(settings, 'server.rewriteBasePath')) {
log(
Expand Down Expand Up @@ -177,4 +187,5 @@ export const coreDeprecationProvider: ConfigDeprecationProvider = ({
rewriteBasePathDeprecation,
cspRulesDeprecation,
mapManifestServiceUrlDeprecation,
xsrfDeprecation,
];
5 changes: 5 additions & 0 deletions src/core/server/http/http_server.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -43,6 +44,7 @@ interface RequestFixtureOptions<P = any, Q = any, B = any> {
method?: RouteMethod;
socket?: Socket;
routeTags?: string[];
kibanaRouteState?: KibanaRouteState;
validation?: {
params?: RouteValidationSpec<P>;
query?: RouteValidationSpec<Q>;
Expand All @@ -60,11 +62,13 @@ function createKibanaRequestMock<P = any, Q = any, B = any>({
socket = new Socket(),
routeTags,
validation = {},
kibanaRouteState = { xsrfRequired: true },
}: RequestFixtureOptions<P, Q, B> = {}) {
const queryString = stringify(query, { sort: false });

return KibanaRequest.from<P, Q, B>(
createRawRequestMock({
app: kibanaRouteState,
headers,
params,
query,
Expand Down Expand Up @@ -105,6 +109,7 @@ function createRawRequestMock(customization: DeepPartial<Request> = {}) {
return merge(
{},
{
app: { xsrfRequired: true } as any,
headers: {},
path: '/',
route: { settings: {} },
Expand Down
7 changes: 5 additions & 2 deletions src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -148,15 +148,18 @@ 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,
path: route.path,
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: can't this be inlined to remove the local var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but I want to make sure that kibanaRouteState implements KibanaRouteState correctly.
This declaration doesn't show an error for unknown properties, for example:

app: { xsrfRequired } as 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
Expand Down
11 changes: 11 additions & 0 deletions src/core/server/http/integration_tests/lifecycle_handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -188,6 +189,12 @@ describe('core lifecycle handlers', () => {
return res.ok({ body: 'ok' });
}
);
((router as any)[method.toLowerCase()] as RouteRegistrar<any>)<any, any, any>(
{ path: xsrfDisabledTestPath, validate: false, options: { xsrfRequired: false } },
(context, req, res) => {
return res.ok({ body: 'ok' });
}
);
});

await server.start();
Expand Down Expand Up @@ -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');
});
});
});
});
Expand Down
29 changes: 27 additions & 2 deletions src/core/server/http/lifecycle_handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,22 @@ 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>): HttpConfig => partial as HttpConfig;

const forgeRequest = ({
headers = {},
path = '/',
method = 'get',
kibanaRouteState,
}: Partial<{
headers: Record<string, string>;
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', () => {
Expand Down Expand Up @@ -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');
});
});
});

Expand Down
6 changes: 5 additions & 1 deletion src/core/server/http/lifecycle_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that we added server.xsrf.disableProtection in spalger#6 for testing purposes solely. I'm wondering if we want to collect all test-only API in the one document to have a better understanding of API surface in case of future refactoring. (in addition to --migrationgs.skip, /internal/saved_objects/_migrate etc)

whitelist.includes(request.route.path) ||
request.route.options.xsrfRequired === false
) {
return toolkit.next();
}

Expand Down
1 change: 1 addition & 0 deletions src/core/server/http/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export {
KibanaRequestEvents,
KibanaRequestRoute,
KibanaRequestRouteOptions,
KibanaRouteState,
isRealRequest,
LegacyRequest,
ensureRawRequest,
Expand Down
10 changes: 9 additions & 1 deletion src/core/server/http/router/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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
Expand Down Expand Up @@ -184,6 +190,8 @@ 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,
mshustov marked this conversation as resolved.
Show resolved Hide resolved
tags: request.route.settings.tags || [],
body: ['get', 'options'].includes(method)
? undefined
Expand Down
9 changes: 9 additions & 0 deletions src/core/server/http/router/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,15 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
*/
authRequired?: boolean;

/**
* Defines xsrf protection requirements for a route:
* - true. Requires an incoming request to contain `kbn-xsrf` header.
mshustov marked this conversation as resolved.
Show resolved Hide resolved
* - false. Disables xsrf protection.
*
* Set to true by default
*/
xsrfRequired?: boolean;

/**
* Additional metadata tag strings to attach to the route.
*/
Expand Down
1 change: 1 addition & 0 deletions src/core/server/server.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,7 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
authRequired?: boolean;
body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody;
tags?: readonly string[];
xsrfRequired?: boolean;
}

// @public
Expand Down