Skip to content

Commit

Permalink
Harden console functions (elastic#171367)
Browse files Browse the repository at this point in the history
## Summary

This PR overrides console functions only in production, in order to
sanitize input parameters for any potential calls made to the global
console from Kibana's dependencies.

This initial implementation overrides the `debug`, `error`, `info`,
`log`, `trace`, and `warn` functions, and only sanitizes string inputs.
Future updates may expand this to handle other types, or strings nested
in objects.

The unmodified console methods are now exposed internally in Kibana as
`unsafeConsole`. Where needed for formatting (log appenders, core
logger), calls to the global console have been replaced by
`unsafeConsole`. This PR also adds a new es linting rule to disallow
calls to `unsafeConsole` unless `eslint-disable-next-line
@kbn/eslint/no_unsafe_console` is used.

### Testing
Not sure how we could test this. The overrides are only enabled when
running in a true production environment (e.g. docker) by checking
`process.env.NODE_ENV`.

I was able to manually test by adding additional console output denoting
when the console functions were being overriden or not.

Closes elastic/kibana-team#664
Closes elastic#176340

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
2 people authored and CoenWarmer committed Feb 15, 2024
1 parent 758f1a4 commit 6d4f784
Show file tree
Hide file tree
Showing 34 changed files with 379 additions and 47 deletions.
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ packages/kbn-search-index-documents @elastic/enterprise-search-frontend
packages/kbn-search-response-warnings @elastic/kibana-data-discovery
x-pack/plugins/searchprofiler @elastic/platform-deployment-management
x-pack/test/security_api_integration/packages/helpers @elastic/kibana-security
packages/kbn-security-hardening @elastic/kibana-security
x-pack/plugins/security @elastic/kibana-security
x-pack/packages/security/plugin_types_common @elastic/kibana-security
x-pack/packages/security/plugin_types_public @elastic/kibana-security
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,7 @@
"@kbn/search-index-documents": "link:packages/kbn-search-index-documents",
"@kbn/search-response-warnings": "link:packages/kbn-search-response-warnings",
"@kbn/searchprofiler-plugin": "link:x-pack/plugins/searchprofiler",
"@kbn/security-hardening": "link:packages/kbn-security-hardening",
"@kbn/security-plugin": "link:x-pack/plugins/security",
"@kbn/security-plugin-types-common": "link:x-pack/packages/security/plugin_types_common",
"@kbn/security-plugin-types-public": "link:x-pack/packages/security/plugin_types_public",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@
*/

import type { LogRecord } from '@kbn/logging';
import { unsafeConsole } from '@kbn/security-hardening';
import { createLogger } from './logger';

describe('createLogger', () => {
// Calling `.mockImplementation` on all of them to avoid jest logging the console usage
const logErrorSpy = jest.spyOn(console, 'error').mockImplementation();
const logWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
const logInfoSpy = jest.spyOn(console, 'info').mockImplementation();
const logDebugSpy = jest.spyOn(console, 'debug').mockImplementation();
const logTraceSpy = jest.spyOn(console, 'trace').mockImplementation();
const logLogSpy = jest.spyOn(console, 'log').mockImplementation();
const logErrorSpy = jest.spyOn(unsafeConsole, 'error').mockImplementation();
const logWarnSpy = jest.spyOn(unsafeConsole, 'warn').mockImplementation();
const logInfoSpy = jest.spyOn(unsafeConsole, 'info').mockImplementation();
const logDebugSpy = jest.spyOn(unsafeConsole, 'debug').mockImplementation();
const logTraceSpy = jest.spyOn(unsafeConsole, 'trace').mockImplementation();
const logLogSpy = jest.spyOn(unsafeConsole, 'log').mockImplementation();

beforeEach(() => {
jest.clearAllMocks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import type { Logger } from '@kbn/logging';
import { unsafeConsole } from '@kbn/security-hardening';

/**
* Create custom logger until we have a proper logging solution: https://github.com/elastic/kibana/issues/33796
Expand All @@ -16,20 +17,20 @@ export function createLogger(isDev: boolean): Logger {
// TODO: Replace with a core logger once we implement it in https://github.com/elastic/kibana/issues/33796
// For now, it logs only in dev mode
const logger: Logger = {
// eslint-disable-next-line no-console
fatal: (...args) => (isDev ? console.error(...args) : void 0),
// eslint-disable-next-line no-console
error: (...args) => (isDev ? console.error(...args) : void 0),
// eslint-disable-next-line no-console
warn: (...args) => (isDev ? console.warn(...args) : void 0),
// eslint-disable-next-line no-console
info: (...args) => (isDev ? console.info(...args) : void 0),
// eslint-disable-next-line no-console
debug: (...args) => (isDev ? console.debug(...args) : void 0),
// eslint-disable-next-line no-console
trace: (...args) => (isDev ? console.trace(...args) : void 0),
// eslint-disable-next-line no-console
log: (...args) => (isDev ? console.log(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
fatal: (...args) => (isDev ? unsafeConsole.error(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
error: (...args) => (isDev ? unsafeConsole.error(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
warn: (...args) => (isDev ? unsafeConsole.warn(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
info: (...args) => (isDev ? unsafeConsole.info(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
debug: (...args) => (isDev ? unsafeConsole.debug(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
trace: (...args) => (isDev ? unsafeConsole.trace(...args) : void 0),
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
log: (...args) => (isDev ? unsafeConsole.log(...args) : void 0),
isLevelEnabled: () => true,
get: () => logger,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
"@kbn/core-injected-metadata-browser-internal",
"@kbn/core-analytics-browser",
"@kbn/core-base-browser-mocks",
"@kbn/core-injected-metadata-browser-mocks"
"@kbn/core-injected-metadata-browser-mocks",
"@kbn/security-hardening"
],
"exclude": ["target/**/*"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
*/

import { LogRecord, LogLevel } from '@kbn/logging';
import { unsafeConsole } from '@kbn/security-hardening';
import { ConsoleAppender } from './console_appender';

test('`append()` correctly formats records and pushes them to console.', () => {
jest.spyOn(global.console, 'log').mockImplementation(() => {
jest.spyOn(unsafeConsole, 'log').mockImplementation(() => {
// noop
});

Expand Down Expand Up @@ -47,10 +48,7 @@ test('`append()` correctly formats records and pushes them to console.', () => {

for (const record of records) {
appender.append(record);
// eslint-disable-next-line no-console
expect(console.log).toHaveBeenCalledWith(`mock-${JSON.stringify(record)}`);
expect(unsafeConsole.log).toHaveBeenCalledWith(`mock-${JSON.stringify(record)}`);
}

// eslint-disable-next-line no-console
expect(console.log).toHaveBeenCalledTimes(records.length);
expect(unsafeConsole.log).toHaveBeenCalledTimes(records.length);
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import type { Layout, LogRecord, DisposableAppender } from '@kbn/logging';
import { unsafeConsole } from '@kbn/security-hardening';

/**
*
Expand All @@ -25,8 +26,8 @@ export class ConsoleAppender implements DisposableAppender {
* @param record `LogRecord` instance to be logged.
*/
public append(record: LogRecord) {
// eslint-disable-next-line no-console
console.log(this.layout.format(record));
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
unsafeConsole.log(this.layout.format(record));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* Side Public License, v 1.
*/

import { unsafeConsole } from '@kbn/security-hardening';
import { BrowserLoggingSystem } from './logging_system';

describe('', () => {
Expand All @@ -15,7 +16,7 @@ describe('', () => {
let loggingSystem: BrowserLoggingSystem;

beforeEach(() => {
mockConsoleLog = jest.spyOn(global.console, 'log').mockReturnValue(undefined);
mockConsoleLog = jest.spyOn(unsafeConsole, 'log').mockReturnValue(undefined);
jest.spyOn<any, any>(global, 'Date').mockImplementation(() => timestamp);
loggingSystem = new BrowserLoggingSystem({ logLevel: 'warn' });
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
],
"kbn_references": [
"@kbn/logging",
"@kbn/core-logging-common-internal"
"@kbn/core-logging-common-internal",
"@kbn/security-hardening"
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ jest.mock('../../layouts/layouts', () => {
});

import { LogRecord, LogLevel } from '@kbn/logging';
import { unsafeConsole } from '@kbn/security-hardening';
import { ConsoleAppender } from './console_appender';

test('`configSchema` creates correct schema.', () => {
Expand All @@ -37,7 +38,7 @@ test('`configSchema` creates correct schema.', () => {
});

test('`append()` correctly formats records and pushes them to console.', () => {
jest.spyOn(global.console, 'log').mockImplementation(() => {
jest.spyOn(unsafeConsole, 'log').mockImplementation(() => {
// noop
});

Expand Down Expand Up @@ -74,10 +75,7 @@ test('`append()` correctly formats records and pushes them to console.', () => {

for (const record of records) {
appender.append(record);
// eslint-disable-next-line no-console
expect(console.log).toHaveBeenCalledWith(`mock-${JSON.stringify(record)}`);
expect(unsafeConsole.log).toHaveBeenCalledWith(`mock-${JSON.stringify(record)}`);
}

// eslint-disable-next-line no-console
expect(console.log).toHaveBeenCalledTimes(records.length);
expect(unsafeConsole.log).toHaveBeenCalledTimes(records.length);
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import { schema } from '@kbn/config-schema';
import type { Layout, LogRecord, DisposableAppender } from '@kbn/logging';
import { unsafeConsole } from '@kbn/security-hardening';
import { Layouts } from '../../layouts/layouts';

const { literal, object } = schema;
Expand All @@ -34,8 +35,8 @@ export class ConsoleAppender implements DisposableAppender {
* @param record `LogRecord` instance to be logged.
*/
public append(record: LogRecord) {
// eslint-disable-next-line no-console
console.log(this.layout.format(record));
// eslint-disable-next-line @kbn/eslint/no_unsafe_console
unsafeConsole.log(this.layout.format(record));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ const mockCreateWriteStream = createWriteStream as unknown as jest.Mock<typeof c

import { LoggingSystem, config } from '..';
import { EcsVersion } from '@kbn/ecs';
import { unsafeConsole } from '@kbn/security-hardening';

let system: LoggingSystem;
beforeEach(() => {
mockConsoleLog = jest.spyOn(global.console, 'log').mockReturnValue(undefined);
mockConsoleLog = jest.spyOn(unsafeConsole, 'log').mockReturnValue(undefined);
jest.spyOn<any, any>(global, 'Date').mockImplementation(() => timestamp);
jest.spyOn(process, 'uptime').mockReturnValue(10);
system = new LoggingSystem();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"@kbn/utility-types-jest",
"@kbn/utility-types",
"@kbn/ecs",
"@kbn/security-hardening",
],
"exclude": [
"target/**/*",
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-eslint-config/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ module.exports = {
'@kbn/eslint/no_trailing_import_slash': 'error',
'@kbn/eslint/no_constructor_args_in_property_initializers': 'error',
'@kbn/eslint/no_this_in_property_initializers': 'error',
'@kbn/eslint/no_unsafe_console': 'error',
'@kbn/imports/no_unresolvable_imports': 'error',
'@kbn/imports/uniform_imports': 'error',
'@kbn/imports/no_unused_imports': 'error',
Expand Down
6 changes: 5 additions & 1 deletion packages/kbn-eslint-plugin-eslint/README.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,8 @@ module.exports = {
}
]
}
```
```

## no_unsafe_console

Disables the usage of kbn-security-hardening/console/unsafeConsole.
1 change: 1 addition & 0 deletions packages/kbn-eslint-plugin-eslint/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ module.exports = {
no_trailing_import_slash: require('./rules/no_trailing_import_slash'),
no_constructor_args_in_property_initializers: require('./rules/no_constructor_args_in_property_initializers'),
no_this_in_property_initializers: require('./rules/no_this_in_property_initializers'),
no_unsafe_console: require('./rules/no_unsafe_console'),
},
};
71 changes: 71 additions & 0 deletions packages/kbn-eslint-plugin-eslint/rules/no_unsafe_console.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

const tsEstree = require('@typescript-eslint/typescript-estree');
const esTypes = tsEstree.AST_NODE_TYPES;

/** @typedef {import("eslint").Rule.RuleModule} Rule */
/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.Node} Node */
/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.CallExpression} CallExpression */
/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.CallExpression} VariableDeclarator */

const ERROR_MSG = 'Unexpected unsafeConsole statement.';

/**
* @param {CallExpression} node
*/
const isUnsafeConsoleCall = (node) => {
return (
node.callee.type === esTypes.MemberExpression &&
node.callee.property.type === esTypes.Identifier &&
node.callee.object.name === 'unsafeConsole' &&
node.callee.property.name
);
};

/**
* @param {VariableDeclarator} node
*/
const isUnsafeConsoleObjectPatternDeclarator = (node) => {
return (
node.id.type === esTypes.ObjectPattern &&
node.init &&
node.init.type === esTypes.Identifier &&
node.init.name === 'unsafeConsole'
);
};

/** @type {Rule} */
module.exports = {
meta: {
fixable: 'code',
schema: [],
},
create: (context) => ({
CallExpression(_) {
const node = /** @type {CallExpression} */ (_);

if (isUnsafeConsoleCall(node)) {
context.report({
message: ERROR_MSG,
loc: node.callee.loc,
});
}
},
VariableDeclarator(_) {
const node = /** @type {VariableDeclarator} */ (_);

if (isUnsafeConsoleObjectPatternDeclarator(node)) {
context.report({
message: ERROR_MSG,
loc: node.init.loc,
});
}
},
}),
};
Loading

0 comments on commit 6d4f784

Please sign in to comment.