os.download: Fix balenaOS version format validation #1017

pdcastro · 2020-11-08T22:03:33Z

We've found that malformed balenaOS versions are not rejected as intended when passed as arguments to the balena os download CLI command. See balena-io/balena-cli#2084 (comment)

Change-type: patch

pdcastro · 2020-11-08T22:11:07Z

lib/models/os.ts

@@ -30,7 +30,7 @@ import type {
 import { InjectedDependenciesParam, InjectedOptionsParam } from '..';
 import { getAuthDependentMemoize } from '../util/cache';

-const BALENAOS_VERSION_REGEX = /v?\d+\.\d+\.\d+(\.rev\d+)?((\-|\+).+)?/;
+const BALENAOS_VERSION_REGEX = /^\d+\.\d+\.\d+(\+rev\d+)?(\.(dev|prod))?$/;


Please double check this regex change:

(\.rev\d+)? in the old regex seemed wrong to me, because we write balenaOS versions like 2.60.1+rev1 rather than 2.60.1.rev1.

((\-|\+).+)? in the old regex might match prerelease versions as defined in the semver spec, and would match any strange thing like 2.60.1-foo. A question is whether this regex should verify any possible semver version as per semver spec, versus validating balenaOS versions as we know them. In this PR I have opted for the latter. If we wanted to match any possible semver spec, then the regex should not include the rev\d+ part, which applies to balenaOS versions but not to generic semver versions.

Note I've removed the leading v? because this regex is used after v removal (see code where the regex is used). If the leading v? was not removed, then a malformed version like vv2.60.1 would be accepted.

Note I've added ^ at the beginning and $ at the end, to be stricter about the balenaOS version format.

@pdcastro The issue is that we did have malformed semver os releases and that's why we originally added this here.

Imo we should ideally try to drop this semver regex and use balena-semver for validation instead.
Even if balena-semver is looser, I expect that the API would anyway error if the version is wrong.

Even if balena-semver is looser, I expect that the API would anyway error if the version is wrong.

@thgreasi, on this point, my investigation didn't go far enough to provide an explanation, but actually the following CLI commands download an image (don't ask me what image, but an image is downloaded!), despite the versions being nonsense:

$ balena os download raspberrypi3 -o test.img --version 2.60.1+rev1-nodev $ balena os download raspberrypi3 -o test.img --version 2.60.1+rev1-foo $ balena os download raspberrypi3 -o test.img --version 2.60.1+rev9751

It's true though that semver validation (this PR) would not completely solve this problem, but the regex changes that I proposed would at least catch things like -dev instead of .dev. Indeed, this was the motivation for this PR, originating from this other CLI PR by @codewithcheese: balena-io/balena-cli/pull/2084

@pdcastro it would be interesting to check the os-version in the downloaded images.
It seems that the reason is that balena-image-manager is doing a version range check (which ends up using semver.maxSatisfying) which as a result.

See: https://github.com/balena-io-modules/balena-image-manager/blob/2a4e905f7b125396303b69bff37bc901038c25d5/lib/manager.js#L92
See: https://github.com/balena-io-modules/balena-image-manager/blob/2a4e905f7b125396303b69bff37bc901038c25d5/lib/utils.js#L64-L65

Here are the respective test cases of the SDK method:
See:

balena-sdk/tests/integration/models/os.spec.ts

Lines 172 to 182 in a3ade4c

it('should return an exact match, if it exists, when given a specific version', () =>

// Concern here is that semver says .dev is equivalent to .prod, but

// we want provide an exact version and use _exactly_ that version.

expect(_getMaxSatisfyingVersion('2.0.1+rev2.dev', osVersions)).to.equal(

'2.0.1+rev2.dev',

));

it('should return an equivalent result, if no exact result exists, when given a specific version', () =>

expect(_getMaxSatisfyingVersion('2.0.1+rev2', osVersions)).to.equal(

'2.0.1+rev2.prod',

));

This behavior complicates the issue 🤔
The thing is that if you have a typo, you suddenly get the latest version matching just the major.minor.patch of what you provided, since anything after those doesn't count when sorting.

Just thinking out loud:
It might make sense to just clarify to the user which os version is getting downloaded if the final version doesn't match what they provided.
An alternative could be to only do range matching when only the major.minor.patch parts are provided, but this would stop 2.40.99+rev1 for successfully downloading an image, since the .prod is missing.

I think it makes sense to find all invalid os versions and confirm whether we still want to support them.
PS: The Classic app type requires version >=2.0.3

It might make sense to just clarify to the user which os version is getting downloaded if the final version doesn't match what they provided.

Having to use etcher-sdk to extract a specific file (/etc/os-release) from a specific partition from a .img image file, just because of the risk that the user specified a non-existent, even invalid version like 2.60.1.foo, sounds... Doable but a bit disheartening. :-)

An alternative could be to only do range matching when only the major.minor.patch parts are provided

I like this suggestion! If a user adds a specific suffix like +rev2, then surely they don't mean a version range. 🤔 This also means that prefixes like ~ or ^ would not be compatible with a +rev suffix. On the other hand, .dev or .prod are orthogonal to all this. .dev and .prod are compatible with version ranges, and ^2.60.1.dev actually makes some sense (unlike ^2.60.1+rev2). This kind of validation would be beyond generic semver -- these are balenaOS specific validations, with specific meanings attached to specific suffixes.

but this would stop 2.40.99+rev1 for successfully downloading an image, since the .prod is missing.

The SDK could automatically append .prod if neither .dev not .prod is provided. This PR could do it.

I think it makes sense to find all invalid os versions and confirm whether we still want to support them.
PS: The Classic app type requires version >=2.0.3

How does one go about finding all available versions?
And: What about a SDK option to disable version validation? So that, by default, the malformed versions would raise an error, but CLI users would be given the option of adding --novalidation to disable validation.

Having to use etcher-sdk to extract a specific file (/etc/os-release) from a specific partition from a .img image file, just because of the risk that the user specified a non-existent, even invalid version like 2.60.1.foo, sounds... Doable but a bit disheartening. :-)

I actually meant this by calling the respective method from the balena-image-manager (for sure not parsing the stream).
Update: I actually see that we already do print the version that end up being downloaded:
See: https://github.com/balena-io/balena-cli/blob/fa4f91e08d891b2f99f7e76aecb0ff7018d16ff4/lib/utils/cloud.ts#L146

^2.60.1.dev

I wouldn't expect this to work as any .dev after 2.60.1. Never seen any such semantics. Do we intend to add support for that? Imo it sounds like adding an extra variant filtering option to balena-image-manager would be better than extending balena-semver to allow such "filtered" range matching specifically for .dev & .prod (since that would diverge it even more from plain semver).

How does one go about finding all available versions?

@nazrhom / @sradevski can probably help you on that

And: What about a SDK option to disable version validation? So that, by default, the malformed versions would raise an error, but CLI users would be given the option of adding --novalidation to disable validation.

Maybe we should drop this regex and just use balena-semver and always parse/validate the provided string.
Alternatively, instead of a --novalidation, I would prefer a strict/exact argument that just throws a NotFoundError when a version with the exact string provided isn't available, than trying to validate the semver.
Essentially this would fill the "gap" in the npm semver where there is no range symbol targeting the build part of a semver.

^2.60.1.dev

I wouldn't expect this to work as any .dev after 2.60.1. Never seen any such semantics. Do we intend to add support for that?

Wow, this is an important comment. For whatever reason, I assumed that appending .dev to the OS version in a call to the SDK method os.download(deviceType, [version]) was the way to request the download of a development image, as opposed to a production image. @codewithcheese even added this note to the output of balena help os download in a recent CLI pull request (balena-io/balena-cli#2084).

$ balena help os download ... To download a development image append `.dev` to the version or select from the interactive menu.

Question: what is the correct way of selecting a development image in a SDK call to os.download(deviceType, [version])?

It should actually hint the users to provide the exact version string (including the .dev build part of the semver) as printed by balena os versions <device_type>.

Question: what is the correct way of selecting a development image in a SDK call to os.download(deviceType, [version])?

Yes, that's he one to use, which actually is the only download related method provided.

Oh your question was about "how" not whether that's the one to use.
For the "how" part atm you have to pass the exact os version string eg: os.download('raspberry-pi', '2.56.0+rev2.dev').
Update: Actually if you are passing an argument other than latest to os.download, then it has to be the full version string otherwise it erros.

pdcastro · 2020-11-08T22:12:10Z

lib/models/os.ts

@@ -159,7 +159,7 @@ const getOsModel = function (
 		}
 		const vNormalized = v[0] === 'v' ? v.substring(1) : v;
 		if (!BALENAOS_VERSION_REGEX.test(vNormalized)) {
-			throw new Error(`Invalid semver version: ${v}`);
+			throw new Error(`Invalid balenaOS version format: ${v}`);


This change goes back to a point I raised in another comment, whether we mean to validate any generic semver version or specifically balenaOS versions. This change aligns with the latter.

pdcastro · 2020-11-08T22:18:24Z

tests/integration/models/os.spec.ts

+				for (const version of versions) {
+					await expect(
+						balena.models.os.download('raspberry-pi', version),
+					).to.be.rejectedWith(`No such version for the device type`);


This test works, but performs rather poorly: 5 seconds on my laptop. What I really wanted to test is the normalizeVersion function in lib/os/models/os.ts, but it is not exported. If I had access to that function, the test would run in milliseconds.
Any suggestions on how I could test normalizeVersion directly? I thought of using rewire, but I am not sure if it would work because of the peculiar SDK arrangement of defining functions inside functions and returning selected internal functions.

@pdcastro Looks like normalizeVersion can be safely be moved to the module scope outside of getOsModel

Looks like normalizeVersion can be safely be moved to the module scope outside of getOsModel

This might work in combination with rewire (assuming we don't want to export the function). 👍
Would however there be any side effects in relation to the "webpacking" and in-browser SDK functionality? Hmm os.download is not meant to work in browsers anyway...

@pdcastro you can move it to ~/utils/device-os-versions.ts.

os.download: Fix balenaOS version format validation

b139f13

Change-type: patch

pdcastro requested review from thgreasi and codewithcheese November 8, 2020 22:03

pdcastro commented Nov 8, 2020

View reviewed changes

pdcastro mentioned this pull request Nov 8, 2020

Modify os download help to mention -dev suffix balena-io/balena-cli#2084

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os.download: Fix balenaOS version format validation #1017

os.download: Fix balenaOS version format validation #1017

pdcastro commented Nov 8, 2020

pdcastro Nov 8, 2020 •

edited

Loading

thgreasi Nov 9, 2020

pdcastro Nov 9, 2020

thgreasi Nov 9, 2020

thgreasi Nov 9, 2020

pdcastro Nov 9, 2020

thgreasi Nov 16, 2020

pdcastro Nov 16, 2020

thgreasi Nov 16, 2020

thgreasi Nov 16, 2020 •

edited

Loading

pdcastro Nov 8, 2020

pdcastro Nov 8, 2020

codewithcheese Nov 9, 2020 •

edited

Loading

pdcastro Nov 9, 2020

thgreasi Nov 9, 2020

	it('should return an exact match, if it exists, when given a specific version', () =>
	// Concern here is that semver says .dev is equivalent to .prod, but
	// we want provide an exact version and use _exactly_ that version.
	expect(_getMaxSatisfyingVersion('2.0.1+rev2.dev', osVersions)).to.equal(
	'2.0.1+rev2.dev',
	));

	it('should return an equivalent result, if no exact result exists, when given a specific version', () =>
	expect(_getMaxSatisfyingVersion('2.0.1+rev2', osVersions)).to.equal(
	'2.0.1+rev2.prod',
	));

os.download: Fix balenaOS version format validation #1017

Are you sure you want to change the base?

os.download: Fix balenaOS version format validation #1017

Conversation

pdcastro commented Nov 8, 2020

pdcastro Nov 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thgreasi Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codewithcheese Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdcastro Nov 8, 2020 •

edited

Loading

thgreasi Nov 16, 2020 •

edited

Loading

codewithcheese Nov 9, 2020 •

edited

Loading