ExternalVendorCode/balena-cli
1
0
Fork 0
mirror of https://github.com/balena-io/balena-cli.git synced 2024-12-19 05:37:51 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge pull request #1735 from balena-io/fix-instance-of

Browse Source 
Unpin balena-sdk and review 'instanceof' usage
This commit is contained in:
Paulo Castro 2020-04-20 14:15:19 +01:00 committed by GitHub
commit 1319e0642b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 107 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
CONTRIBUTING.md
View File

@ -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'`

7
lib/app-oclif.ts
View File

@ -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;

31
lib/errors.ts
View File

@ -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;
}

3
lib/utils/device/live.ts
View File

@ -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}`);

4
lib/utils/patterns.ts
View File

@ -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`);

31
npm-shrinkwrap.json generated
View File

@ -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",

5
package.json
View File

@ -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",
@ -170,10 +169,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",