Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os.download: Fix balenaOS version format validation #1017

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

os.download: Fix balenaOS version format validation #1017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/models/os.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))?$/;
Copy link
Author

@pdcastro pdcastro Nov 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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',
));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^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])?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@thgreasi thgreasi Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const getOsModel = function (
deps: InjectedDependenciesParam,
Expand Down Expand Up @@ -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}`);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return vNormalized;
};
Expand Down
43 changes: 43 additions & 0 deletions tests/integration/models/os.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,49 @@ describe('OS model', function () {
const promise = balena.models.os.download('foo-bar-baz');
return expect(promise).to.be.rejectedWith('No such device type');
}));

describe('given well formed and malformed balenaOS versions', () => {
it('should reject malformed versions with an error', async () => {
const versions = [
'2.60.1+foo',
'2.60.1-foo',
'2.60.1.foo',
'2.60.1+rev',
'2.60.1+rev1+foo',
'2.60.1+rev1-foo',
'2.60.1+rev1.foo',
'2.60.1+rev1.dev.foo',
'2.60.1+rev1.prod.foo',
];
for (const version of [...versions]) {
versions.push(`v${version}`);
}
for (const version of versions) {
await expect(
balena.models.os.download('raspberry-pi', version),
).to.be.rejectedWith(`Invalid balenaOS version format: ${version}`);
}
});

it('should accept well formed versions', async () => {
const versions = [
'0.0.1',
'0.0.1.dev',
'0.0.1.prod',
'0.0.1+rev1',
'0.0.1+rev1.dev',
'0.0.1+rev1.prod',
];
for (const version of [...versions]) {
versions.push(`v${version}`);
}
for (const version of versions) {
await expect(
balena.models.os.download('raspberry-pi', version),
).to.be.rejectedWith(`No such version for the device type`);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@codewithcheese codewithcheese Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
});
});
});

describe('balena.models.os.isSupportedOsUpdate()', function () {
Expand Down