From a8b0573699ab1dedb07f7d2a44a4ac62983c16b1 Mon Sep 17 00:00:00 2001 From: Paulo Castro Date: Sat, 18 Apr 2020 02:24:17 +0100 Subject: [PATCH 1/3] Unpin balena-sdk (bump balena-sdk to v12.33.0) Change-type: patch --- npm-shrinkwrap.json | 31 +++++++++++++++++++------------ package.json | 4 ++-- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index c813b09e..2d97e8ac 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -2080,12 +2080,19 @@ } }, "balena-errors": { - "version": "4.2.1", - "resolved": "https://registry.npmjs.org/balena-errors/-/balena-errors-4.2.1.tgz", - "integrity": "sha512-8V92f8C5xckM8uIl4OPPgR2IgjOKSYaSIz927dNMou9Ht9YP8SHm7e4m7njijibL5mpUho2RPXoUOg+6L6V/iA==", + "version": "4.3.2", + "resolved": "https://registry.npmjs.org/balena-errors/-/balena-errors-4.3.2.tgz", + "integrity": "sha512-daa+3jHqvoha8zIIIX4jvxdus1LWhH+2Y5IJOzOuIzxmfLEq51EXeszMbcHrbSLlGlJpfaJDG7dfBmFXtabL6Q==", "requires": { - "tslib": "^1.7.1", + "tslib": "^1.11.1", "typed-error": "^3.0.0" + }, + "dependencies": { + "tslib": { + "version": "1.11.1", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-1.11.1.tgz", + "integrity": "sha512-aZW88SY8kQbU7gpV19lN24LtXh/yD4ZZg6qieAJDDg+YBsJcSmLGK9QpnUjAKVG/xefmvJGd1WUmfpT/g6AJGA==" + } } }, "balena-hup-action-utils": { @@ -2210,11 +2217,11 @@ } }, "balena-register-device": { - "version": "6.0.1", - "resolved": "https://registry.npmjs.org/balena-register-device/-/balena-register-device-6.0.1.tgz", - "integrity": "sha512-2u2iFlbMP0nieTyitt9KW+LDDcZvLT6h5NbTHQsaepYcP8zIQOh9+EiIquCwHbDyVIjbSMbhoVWFU5VmNanCiA==", + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/balena-register-device/-/balena-register-device-6.1.0.tgz", + "integrity": "sha512-iPCm8HiZUG2xwNLVECaHx8g589jnqBDtCpLJ0ow7828xd0Spk9nF/GncRqiqmeo5OUZPnXW7prLkaRwu4s2Otg==", "requires": { - "bluebird": "^3.0.0", + "bluebird": "^3.7.2", "randomstring": "^1.1.5" } }, @@ -2242,9 +2249,9 @@ } }, "balena-sdk": { - "version": "12.30.0", - "resolved": "https://registry.npmjs.org/balena-sdk/-/balena-sdk-12.30.0.tgz", - "integrity": "sha512-V7t2vGamLCkWRt1ZMEnFuuEw/4zUC9cEAlh2v+VM9rWMgnebIb2KVm8pXen/BQb44sYyiIVS8S8VXhIyPwrRzg==", + "version": "12.33.0", + "resolved": "https://registry.npmjs.org/balena-sdk/-/balena-sdk-12.33.0.tgz", + "integrity": "sha512-riqcJeA8SYMf20bgt1lhgw/eWiWXVwivGHgBer54/dxDIo48RIwys98LvkisIO2sTco1AmTL4Q0rePsMGjEYwQ==", "requires": { "@types/bluebird": "^3.5.30", "@types/lodash": "^4.14.149", @@ -2253,7 +2260,7 @@ "abortcontroller-polyfill": "^1.4.0", "balena-auth": "^3.0.1", "balena-device-status": "^3.2.1", - "balena-errors": "^4.2.1", + "balena-errors": "^4.3.0", "balena-hup-action-utils": "~4.0.0", "balena-pine": "^10.1.1", "balena-register-device": "^6.0.1", diff --git a/package.json b/package.json index be16fac5..e945a59e 100644 --- a/package.json +++ b/package.json @@ -170,10 +170,10 @@ "balena-config-json": "^2.1.1", "balena-device-init": "^5.0.2", "balena-device-status": "^3.2.1", - "balena-errors": "^4.2.1", + "balena-errors": "^4.3.0", "balena-image-manager": "^6.1.2", "balena-preload": "^8.4.0", - "balena-sdk": "12.30.0", + "balena-sdk": "^12.33.0", "balena-semver": "^2.2.0", "balena-settings-client": "^4.0.4", "balena-sync": "^10.2.0", From 655534469a796679c130ea481c38dfe6954b7115 Mon Sep 17 00:00:00 2001 From: Paulo Castro Date: Sat, 18 Apr 2020 02:31:13 +0100 Subject: [PATCH 2/3] Review 'instanceof' usage with classes of external packages Change-type: patch --- lib/app-oclif.ts | 7 ++++--- lib/errors.ts | 31 +++++++++++++++++++++++++++++++ lib/utils/device/live.ts | 3 ++- lib/utils/patterns.ts | 4 ++-- package.json | 1 - 5 files changed, 39 insertions(+), 7 deletions(-) diff --git a/lib/app-oclif.ts b/lib/app-oclif.ts index 0414c838..8156ce8f 100644 --- a/lib/app-oclif.ts +++ b/lib/app-oclif.ts @@ -16,7 +16,6 @@ */ import { Main } from '@oclif/command'; -import { ExitError } from '@oclif/errors'; import { trackPromise } from './hooks/prerun/track'; @@ -43,9 +42,11 @@ export async function run(command: string[], options: AppOptions) { return require('@oclif/command/flush'); } }, - (error: Error) => { + error => { // oclif sometimes exits with ExitError code 0 (not an error) - if (error instanceof ExitError && error.oclif.exit === 0) { + // (Avoid `error instanceof ExitError` here for the reasons explained + // in the CONTRIBUTING.md file regarding the `instanceof` operator.) + if (error.oclif?.exit === 0) { return; } else { throw error; diff --git a/lib/errors.ts b/lib/errors.ts index 383273bf..ddd2f21b 100644 --- a/lib/errors.ts +++ b/lib/errors.ts @@ -25,6 +25,37 @@ export class NotLoggedInError extends ExpectedError {} export class InsufficientPrivilegesError extends ExpectedError {} +/** + * instanceOf is a more reliable implemention of the plain `instanceof` + * typescript operator, for use with TypedError errors when the error + * classes may be defined in external packages/dependencies. + * Sample usage: + * instanceOf(err, BalenaApplicationNotFound) + * + * A plain Typescript `instanceof` test may fail if `npm install` results + * in multiple instances of a package, for example multiple versions of + * `balena-errors`: + * $ find node_modules -type d -name balena-errors + * node_modules/balena-errors + * node_modules/balena-sdk/node_modules/balena-errors + * + * In these cases, `instanceof` produces a false negative when comparing objects + * and classes of the different package versions, but the `err.name` test still + * succeeds. + * + * @param err Error object, for example in a `catch(err)` block + * @param klass TypedError subclass, e.g. BalenaApplicationNotFound. The type + * is annotated as 'any' for the same reason of multiple package installations + * mentioned above. + */ +export function instanceOf(err: any, klass: any): boolean { + if (err instanceof klass) { + return true; + } + const name: string | undefined = err.name || err.constructor?.name; + return name != null && name === klass.name; +} + function hasCode(error: any): error is Error & { code: string } { return error.code != null; } diff --git a/lib/utils/device/live.ts b/lib/utils/device/live.ts index 4dc81834..d32d0cf7 100644 --- a/lib/utils/device/live.ts +++ b/lib/utils/device/live.ts @@ -7,6 +7,7 @@ import * as path from 'path'; import { Composition } from 'resin-compose-parse'; import { BuildTask } from 'resin-multibuild'; +import { instanceOf } from '../../errors'; import Logger = require('../logger'); import DeviceAPI, { DeviceInfo, Status } from './api'; @@ -285,7 +286,7 @@ export class LivepushManager { this.logger.logError( `An error occured whilst trying to perform a livepush: `, ); - if (e instanceof ContainerNotRunningError) { + if (instanceOf(e, ContainerNotRunningError)) { this.logger.logError(' Livepush container not running'); } else { this.logger.logError(` ${e.message}`); diff --git a/lib/utils/patterns.ts b/lib/utils/patterns.ts index 27c29abd..4e128951 100644 --- a/lib/utils/patterns.ts +++ b/lib/utils/patterns.ts @@ -20,7 +20,7 @@ import { stripIndent } from 'common-tags'; import _ = require('lodash'); import _form = require('resin-cli-form'); -import { NotLoggedInError } from '../errors'; +import { instanceOf, NotLoggedInError } from '../errors'; import { getBalenaSdk, getChalk, getVisuals } from './lazy'; import messages = require('./messages'); import validation = require('./validation'); @@ -405,7 +405,7 @@ export async function getOnlineTargetUuid( })), }); } catch (err) { - if (!(err instanceof BalenaApplicationNotFound)) { + if (!instanceOf(err, BalenaApplicationNotFound)) { throw err; } logger.logDebug(`Application not found`); diff --git a/package.json b/package.json index e945a59e..9f028f23 100644 --- a/package.json +++ b/package.json @@ -159,7 +159,6 @@ }, "dependencies": { "@oclif/command": "^1.5.19", - "@oclif/errors": "^1.2.2", "@resin.io/valid-email": "^0.1.0", "@sentry/node": "^5.13.2", "@types/update-notifier": "^4.1.0", From e3b6db25d8be293f3b697dae6043d7655d63ca4d Mon Sep 17 00:00:00 2001 From: Paulo Castro Date: Sat, 18 Apr 2020 02:31:58 +0100 Subject: [PATCH 3/3] Review CONTRIBUTING.md and add 'instanceof' usage advice Change-type: patch --- CONTRIBUTING.md | 65 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 18 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ad717e2b..561c7e61 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -182,22 +182,51 @@ One thing that most CLI bugs have in common is the absence of test cases exercis code, so writing some test code is a great idea. Having said that, there are also some common gotchas to bear in mind: -* Forward slashes _vs._ backslashes in file paths. Most developers are aware that they should use - Node.js functions like - [path.join](https://nodejs.org/docs/latest-v12.x/api/path.html#path_path_join_paths), which will - automatically use backslashes on Windows and forward slashes on Linux and macOS. Where people get - caught is actually when handling paths in tar streams, which are sent to the Docker daemon and to - balenaCloud. Tar streams use forward slashes regardless of whether the CLI runs on Linux or - Windows, and for those paths the ideal is to use `path.posix.join` instead of `path.join`. Or, - simply hardcode those forward slashes! +* Forward slashes ('/') _vs._ backslashes ('\') in file paths. The Node.js + [path.sep](https://nodejs.org/docs/latest-v12.x/api/path.html#path_path_sep) variable stores a + platform-specific path separator character: the backslash on Windows and the forward slash on + Linux and macOS. The + [path.join](https://nodejs.org/docs/latest-v12.x/api/path.html#path_path_join_paths) function + builds paths using such platform-specific path separator. However: + * Note that Windows (kernel, cmd.exe, PowerShell, many applications) accepts ***both*** forward + slashes and backslashes as path separators (including mixing them in a path string), so code + like `mypath.split(path.sep)` may fail on Windows if `mypath` contains forward slashes. The + [path.parse](https://nodejs.org/docs/latest-v12.x/api/path.html#path_path_parse_path) function + understands both forward slashes and backslashes on Windows, and the + [path.normalize](https://nodejs.org/docs/latest-v12.x/api/path.html#path_path_normalize_path) + function will _replace_ forward slashes with backslashes. + * In [tar](https://en.wikipedia.org/wiki/Tar_(computing)#File_format) streams sent to the Docker + daemon and to balenaCloud, the forward slash is the only acceptable path separator, regardless + of the OS where the CLI is running. Therefore, `path.sep` and `path.join` should never be used + when handling paths in tar streams! `path.posix.join` may be used instead of `path.join`. -* When executing external commands, for example 'ssh', developers often rely on the shell and write - something like `spawn('command "arg1" "arg2"', { shell: true })`. Besides the usual security - concerns, another problem is to get argument escaping right (single quote, double quote, - backslashes...) because of the differences between the Windows 'cmd.exe' shell and the unix - '/bin/sh'. Most of the time, it turns out that it is possible to avoid relying on the shell - altogether. The [which](https://www.npmjs.com/package/which) package can be used to get the full - path of a command, resolving `'ssh'` to, say, `'C:\WINDOWS\System32\OpenSSH\ssh.EXE'`, and then - the command can be executed directly with `spawn(fullPath, argArray, { shell: false})`. It's a - rare combination of more secure and more interoperable with less development effort (as it avoids - time-consuming cross-platform trial and error with argument escaping). +* Avoid using the system shell to execute external commands, for example: + `child_process.exec('ssh "arg1" "arg2"');` + `child_process.spawn('ssh "arg1" "arg2"', { shell: true });` + Besides the usual security concerns of unsanitized strings, another problem is to get argument + escaping right because of the differences between the Windows 'cmd.exe' shell and the Unix + '/bin/sh'. For example, 'cmd.exe' doesn't recognize single quotes like '/bin/sh', and uses the + caret (^) instead of the backslash as the escape character. Bug territory! Most of the time, + it is possible to avoid relying on the shell altogether by providing a Javascript array of + arguments: + `spawn('ssh', ['arg1', 'arg2'], { shell: false});` + To allow for logging and debugging, the [which](https://www.npmjs.com/package/which) package may + be used to get the full path of a command before executing it, without relying on any shell: + `const fullPath = await which('ssh');` + `console.log(fullPath); # 'C:\WINDOWS\System32\OpenSSH\ssh.EXE'` + `spawn(fullPath, ['arg1', 'arg2'], { shell: false });` + +* Avoid the `instanceof` operator when testing against classes/types from external packages + (including base classes), because `npm install` may result in multiple versions of the same + package being installed (to satisfy declared dependencies) and a false negative may result when + comparing an object instance from one package version with a class of another package version + (even if the implementations are identical in both packages). For example, once we fixed a bug + where the test: + `error instanceof BalenaApplicationNotFound` + changed from true to false because `npm install` added an additional copy of the `balena-errors` + package to satisfy a minor `balena-sdk` version update: + `$ find node_modules -name balena-errors` + `node_modules/balena-errors` + `node_modules/balena-sdk/node_modules/balena-errors` + In the case of subclasses of `TypedError`, a string comparison may be used instead: + `error.name === 'BalenaApplicationNotFound'`