Skip to content

Commit

Permalink
Clean up uses of deprecated API's in node core (#51431)
Browse files Browse the repository at this point in the history
Ensure no deprecated Node.js core API's are used in Kibana. This is
achieved by throwing an error in either development mode or in CI if one
of the deprecated API's is called, and as such, new PR's should no
longer be able to be merged if they use deprecated API's.

Some of these API's (like the `Buffer` constructor`) is a security risk.
  • Loading branch information
watson authored Dec 4, 2019
1 parent 686afd7 commit 43b97d8
Show file tree
Hide file tree
Showing 33 changed files with 638 additions and 207 deletions.
5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
"typespec": "typings-tester --config x-pack/legacy/plugins/canvas/public/lib/aeroelastic/tsconfig.json x-pack/legacy/plugins/canvas/public/lib/aeroelastic/__fixtures__/typescript/typespec_tests.ts",
"checkLicenses": "node scripts/check_licenses --dev",
"build": "node scripts/build --all-platforms",
"start": "node --trace-warnings --trace-deprecation scripts/kibana --dev ",
"start": "node --trace-warnings --throw-deprecation scripts/kibana --dev",
"debug": "node --nolazy --inspect scripts/kibana --dev",
"debug-break": "node --nolazy --inspect-brk scripts/kibana --dev",
"karma": "karma start",
Expand Down Expand Up @@ -406,7 +406,6 @@
"gulp-sourcemaps": "2.6.5",
"has-ansi": "^3.0.0",
"iedriver": "^3.14.1",
"image-diff": "1.6.3",
"intl-messageformat-parser": "^1.4.0",
"is-path-inside": "^2.1.0",
"istanbul-instrumenter-loader": "3.0.1",
Expand Down Expand Up @@ -434,7 +433,7 @@
"node-sass": "^4.9.4",
"normalize-path": "^3.0.0",
"nyc": "^14.1.1",
"pixelmatch": "4.0.2",
"pixelmatch": "^5.1.0",
"pkg-up": "^2.0.0",
"pngjs": "^3.4.0",
"postcss": "^7.0.5",
Expand Down
17 changes: 11 additions & 6 deletions packages/kbn-babel-code-parser/src/strategies.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { canRequire } from './can_require';
import { dependenciesVisitorsGenerator } from './visitors';
import { dirname, isAbsolute, resolve } from 'path';
import { builtinModules } from 'module';

export function _calculateTopLevelDependency(inputDep, outputDep = '') {
// The path separator will be always the forward slash
Expand Down Expand Up @@ -48,14 +49,18 @@ export function _calculateTopLevelDependency(inputDep, outputDep = '') {
return _calculateTopLevelDependency(depSplitPaths.join(pathSeparator), outputDep);
}

export async function dependenciesParseStrategy(cwd, parseSingleFile, mainEntry, wasParsed, results) {
// Retrieve native nodeJS modules
const natives = process.binding('natives');

export async function dependenciesParseStrategy(
cwd,
parseSingleFile,
mainEntry,
wasParsed,
results
) {
// Get dependencies from a single file and filter
// out node native modules from the result
const dependencies = (await parseSingleFile(mainEntry, dependenciesVisitorsGenerator))
.filter(dep => !natives[dep]);
const dependencies = (await parseSingleFile(mainEntry, dependenciesVisitorsGenerator)).filter(
dep => !builtinModules.includes(dep)
);

// Return the list of all the new entries found into
// the current mainEntry that we could use to look for
Expand Down
12 changes: 1 addition & 11 deletions packages/kbn-plugin-helpers/tasks/build/rewrite_package_json.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,9 @@ module.exports = function rewritePackage(buildSource, buildVersion, kibanaVersio
delete pkg.scripts;
delete pkg.devDependencies;

file.contents = toBuffer(JSON.stringify(pkg, null, 2));
file.contents = Buffer.from(JSON.stringify(pkg, null, 2));
}

return file;
});
};

function toBuffer(string) {
if (typeof Buffer.from === 'function') {
return Buffer.from(string, 'utf8');
} else {
// this was deprecated in node v5 in favor
// of Buffer.from(string, encoding)
return new Buffer(string, 'utf8');
}
}
2 changes: 1 addition & 1 deletion src/core/server/http/integration_tests/router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ describe('Response factory', () => {
const router = createRouter('/');

router.get({ path: '/', validate: false }, (context, req, res) => {
const buffer = new Buffer('abc');
const buffer = Buffer.from('abc');

return res.ok({
body: buffer,
Expand Down
2 changes: 2 additions & 0 deletions src/dev/ci_setup/setup_env.sh
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ cacheDir="$HOME/.kibana"
RED='\033[0;31m'
C_RESET='\033[0m' # Reset color

export NODE_OPTIONS="$NODE_OPTIONS --throw-deprecation"

###
### Since the Jenkins logging output collector doesn't look like a TTY
### Node/Chalk and other color libs disable their color output. But Jenkins
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { ProxyConfigCollection } from '../proxy_config_collection';

describe('ProxyConfigCollection', function () {
beforeEach(function () {
sinon.stub(fs, 'readFileSync').callsFake(() => new Buffer(0));
sinon.stub(fs, 'readFileSync').callsFake(() => Buffer.alloc(0));
});

afterEach(function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const indexPatternsService = {
make: async () => ({
fieldsFetcher: {
fetch: noop,
fetchForWildcard: noop,
},
}),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('telemetry_usage_collector', () => {
// empty
writeFileSync(tempFiles.empty, '');
// 1 byte too big
writeFileSync(tempFiles.too_big, new Buffer(MAX_FILE_SIZE + 1));
writeFileSync(tempFiles.too_big, Buffer.alloc(MAX_FILE_SIZE + 1));
// write-only file
writeFileSync(tempFiles.unreadable, 'valid: true', { mode: 0o222 });
// valid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,16 +95,16 @@ describe('yaxis.js', () => {

it('throws an error if currency is not three letter code', () => {
invoke(fn, [seriesList, 1, null, null, null, null, null, 'currency:abcde']).catch(e => {
expect(e).to.be.an(Error);
expect(e).to.be.an.instanceof(Error);
});
invoke(fn, [seriesList, 1, null, null, null, null, null, 'currency:12']).catch(e => {
expect(e).to.be.an(Error);
expect(e).to.be.an.instanceof(Error);
});
invoke(fn, [seriesList, 1, null, null, null, null, null, 'currency:$#']).catch(e => {
expect(e).to.be.an(Error);
expect(e).to.be.an.instanceof(Error);
});
invoke(fn, [seriesList, 1, null, null, null, null, null, 'currency:ab']).catch(e => {
expect(e).to.be.an(Error);
expect(e).to.be.an.instanceof(Error);
});
});

Expand Down
38 changes: 21 additions & 17 deletions src/legacy/server/sass/build.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { PUBLIC_PATH_PLACEHOLDER } from '../../../optimize/public_path_placehold

const renderSass = promisify(sass.render);
const writeFile = promisify(fs.writeFile);
const exists = promisify(fs.exists);
const access = promisify(fs.access);
const copyFile = promisify(fs.copyFile);
const mkdirAsync = promisify(fs.mkdir);

Expand Down Expand Up @@ -145,23 +145,27 @@ export class Build {
];

// verify that asset sources exist and import is valid before writing anything
await Promise.all(urlAssets.map(async (asset) => {
if (!await exists(asset.path)) {
throw this._makeError(
'Invalid url() in css output',
`url("${asset.requestUrl}") resolves to "${asset.path}", which does not exist.\n` +
` Make sure that the request is relative to "${asset.root}"`
);
}
await Promise.all(
urlAssets.map(async asset => {
try {
await access(asset.path);
} catch (e) {
throw this._makeError(
'Invalid url() in css output',
`url("${asset.requestUrl}") resolves to "${asset.path}", which does not exist.\n` +
` Make sure that the request is relative to "${asset.root}"`
);
}

if (!isPathInside(asset.path, asset.boundry)) {
throw this._makeError(
'Invalid url() in css output',
`url("${asset.requestUrl}") resolves to "${asset.path}"\n` +
` which is outside of "${asset.boundry}"`
);
}
}));
if (!isPathInside(asset.path, asset.boundry)) {
throw this._makeError(
'Invalid url() in css output',
`url("${asset.requestUrl}") resolves to "${asset.path}"\n` +
` which is outside of "${asset.boundry}"`
);
}
})
);

// write css
await mkdirAsync(this.targetDir, { recursive: true });
Expand Down
50 changes: 50 additions & 0 deletions src/legacy/ui/public/chrome/__mocks__/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,53 @@ const chrome = {

// eslint-disable-next-line import/no-default-export
export default chrome;

// Copied from `src/legacy/ui/public/chrome/chrome.js`
import _ from 'lodash';
import angular from 'angular';
import { metadata } from '../../metadata';

const internals = _.defaults(
_.cloneDeep(metadata),
{
basePath: '',
rootController: null,
rootTemplate: null,
showAppsLink: null,
xsrfToken: null,
devMode: true,
brand: null,
nav: [],
applicationClasses: []
}
);

const waitForBootstrap = new Promise(resolve => {
chrome.bootstrap = function (targetDomElement) {
// import chrome nav controls and hacks now so that they are executed after
// everything else, can safely import the chrome, and interact with services
// and such setup by all other modules
require('uiExports/chromeNavControls');
require('uiExports/hacks');

// sets attribute on body for stylesheet sandboxing
document.body.setAttribute('id', `${internals.app.id}-app`);

chrome.setupAngular();
targetDomElement.setAttribute('kbn-chrome', 'true');
targetDomElement.setAttribute('ng-class', '{ \'hidden-chrome\': !chrome.getVisible() }');
targetDomElement.className = 'app-wrapper';
angular.bootstrap(targetDomElement, ['kibana']);
resolve(targetDomElement);
};
});

chrome.dangerouslyGetActiveInjector = () => {
return waitForBootstrap.then((targetDomElement) => {
const $injector = angular.element(targetDomElement).injector();
if (!$injector) {
return Promise.reject('targetDomElement had no angular context after bootstrapping');
}
return $injector;
});
};
4 changes: 2 additions & 2 deletions src/legacy/utils/deep_clone_with_buffers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe('deepCloneWithBuffers()', () => {
});

it('copies buffers but keeps them buffers', () => {
const input = new Buffer('i am a teapot', 'utf8');
const input = Buffer.from('i am a teapot', 'utf8');
const output = deepCloneWithBuffers(input);

expect(Buffer.isBuffer(input)).toBe(true);
Expand All @@ -65,7 +65,7 @@ describe('deepCloneWithBuffers()', () => {
const input = {
a: {
b: {
c: new Buffer('i am a teapot', 'utf8'),
c: Buffer.from('i am a teapot', 'utf8'),
},
},
};
Expand Down
2 changes: 1 addition & 1 deletion src/legacy/utils/deep_clone_with_buffers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { cloneDeep } from 'lodash';
// type of the customizer function doesn't expect that.
function cloneBuffersCustomizer(val: unknown): any {
if (Buffer.isBuffer(val)) {
return new Buffer(val);
return Buffer.from(val);
}
}

Expand Down
11 changes: 6 additions & 5 deletions src/optimize/dynamic_dll_plugin/dll_compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import del from 'del';

const readFileAsync = promisify(fs.readFile);
const mkdirAsync = promisify(fs.mkdir);
const existsAsync = promisify(fs.exists);
const accessAsync = promisify(fs.access);
const writeFileAsync = promisify(fs.writeFile);

export class DllCompiler {
Expand Down Expand Up @@ -127,13 +127,14 @@ export class DllCompiler {
}

async ensurePathExists(filePath) {
const exists = await existsAsync(filePath);

if (!exists) {
try {
await accessAsync(filePath);
} catch (e) {
await mkdirAsync(path.dirname(filePath), { recursive: true });
return false;
}

return exists;
return true;
}

async ensureOutputPathExists() {
Expand Down
Loading

0 comments on commit 43b97d8

Please sign in to comment.