Skip to content

Commit

Permalink
Apply more reviewer comments.
Browse files Browse the repository at this point in the history 
- Use case-sensitive checks for user-defined objects.
  We've already normalized the case of the registry field, so can use === to compare.

- The reg file can be ascii-encoded.

Any non-ascii characters should appear only inside SZ strings and be hex-encoded.
All the names in the schema can be represented in 7-bit ASCII characters so there's
no need to support other encodings.

Signed-off-by: Eric Promislow <[email protected]>
  • Loading branch information
@ericpromislow
ericpromislow committed May 12, 2023
1 parent f22127b commit cf06fdc
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 40 deletions.
21 changes: 10 additions & 11 deletions pkg/rancher-desktop/main/__tests__/deploymentProfiles.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,12 @@ async function clearRegistry() {
}

async function installInRegistry(regFileContents: string) {
const BOM = ''; // \uFEFF';

await fs.promises.writeFile(regFilePath, BOM + regFileContents, { encoding: 'latin1' });
await fs.promises.writeFile(regFilePath, regFileContents, { encoding: 'ascii' });
try {
await spawnFile('reg', ['IMPORT', regFilePath]);
} catch (ex: any) {
expect(ex).toMatchObject({});
// Use expect to display the error message
expect(ex).toBeNull();
throw ex;
}
}
Expand Down Expand Up @@ -256,8 +255,8 @@ describeWindows('windows deployment profiles', () => {
it('loads nothing', async() => {
const profile = await readDeploymentProfiles(REGISTRY_PATH_PROFILE);

expect(profile.defaults).toMatchObject({});
expect(profile.locked).toMatchObject({});
expect(profile.defaults).toEqual({});
expect(profile.locked).toEqual({});
});
});

Expand All @@ -268,8 +267,8 @@ describeWindows('windows deployment profiles', () => {
await installInRegistry(lockedUserRegFile);
const profile = await readDeploymentProfiles(REGISTRY_PATH_PROFILE);

expect(profile.defaults).toMatchObject(defaultUserProfile);
expect(profile.locked).toMatchObject(lockedUserProfile);
expect(profile.defaults).toEqual(defaultUserProfile);
expect(profile.locked).toEqual(lockedUserProfile);
});
});

Expand All @@ -278,8 +277,8 @@ describeWindows('windows deployment profiles', () => {
await installInRegistry(arrayFromSingleStringDefaultsUserRegFile);
const profile = await readDeploymentProfiles(REGISTRY_PATH_PROFILE);

expect(profile.defaults).toMatchObject({
containerEngine: { allowedImages: { patterns: ['hokey smoke!'] } },
expect(profile.defaults).toEqual({
containerEngine: { allowedImages: { patterns: ['hokey smoke!'] }, name: 'moby' },
});
});
});
Expand All @@ -298,7 +297,7 @@ describeWindows('windows deployment profiles', () => {
await installInRegistry(incorrectDefaultsUserRegFile);
const profile = await readDeploymentProfiles(REGISTRY_PATH_PROFILE);

expect(profile.defaults).toMatchObject(limitedUserProfile);
expect(profile.defaults).toEqual(limitedUserProfile);
// Remember that sub-objects are processed before values
expect(consoleMock).toHaveBeenNthCalledWith(1,
expect.stringMatching(/Expecting registry entry .*?application.adminAccess to be a boolean, but it's a registry object/),
Expand Down
44 changes: 15 additions & 29 deletions pkg/rancher-desktop/main/deploymentProfiles.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import os from 'os';
import { join } from 'path';
import stream from 'stream';

import _ from 'lodash';
import * as nativeReg from 'native-reg';

import * as settings from '@pkg/config/settings';
Expand Down Expand Up @@ -197,12 +198,8 @@ class Win32DeploymentReader {
console.error('Error reading deployment profile: ', err);
} finally {
nativeReg.closeKey(registryKey);
if (defaultsKey) {
nativeReg.closeKey(defaultsKey);
}
if (lockedKey) {
nativeReg.closeKey(lockedKey);
}
nativeReg.closeKey(defaultsKey);
nativeReg.closeKey(lockedKey);
}
// If we found something in the HKLM Defaults or Locked registry hive, don't look at the user's
// Alternatively, if the keys work, we could break, even if both hives are empty.
Expand Down Expand Up @@ -237,17 +234,17 @@ class Win32DeploymentReader {
// Drop the initial 'defaults' or 'locked' field
const pathPartsWithoutHiveType = pathParts.slice(1);

for (const originalKey of nativeReg.enumKeyNames(regKey)) {
const schemaKey = fixProfileKeyCase(originalKey, schemaKeys);
for (const registryKey of nativeReg.enumKeyNames(regKey)) {
const schemaKey = fixProfileKeyCase(registryKey, schemaKeys);
// "fixed case" means mapping existing keys in the registry (which typically supports case-insensitive lookups)
// to the actual case in the schema.

if (schemaKey === null) {
unknownKeys.push(originalKey);
} else if (haveUserDefinedObject(pathPartsWithoutHiveType.concat(schemaKey), isEquivalentIgnoreCase)) {
userDefinedObjectKeys.push({ schemaKey, registryKey: originalKey });
unknownKeys.push(registryKey);
} else if (haveUserDefinedObject(pathPartsWithoutHiveType.concat(schemaKey))) {
userDefinedObjectKeys.push({ schemaKey, registryKey });
} else {
commonKeys.push({ schemaKey, registryKey: originalKey });
commonKeys.push({ schemaKey, registryKey });
}
}
if (unknownKeys.length) {
Expand Down Expand Up @@ -382,9 +379,9 @@ class Win32DeploymentReader {
switch (rawValue.type) {
case nativeReg.ValueType.SZ:
if (isUserDefinedObject || (typeof schemaVal) === 'string') {
return nativeReg.parseValue(rawValue) as string;
return nativeReg.parseString(rawValue);
} else if (expectingArray) {
return [nativeReg.parseValue(rawValue) as string];
return [nativeReg.parseString(rawValue)];
} else {
console.error(`Expecting registry entry ${ fullPath } to be a ${ typeof schemaVal }, but it's a ${ valueTypeNames[rawValue.type] }, value: ${ parsedValueForErrorMessage }`);
}
Expand All @@ -404,7 +401,7 @@ class Win32DeploymentReader {
break;
case nativeReg.ValueType.MULTI_SZ:
if (expectingArray) {
return nativeReg.parseValue(rawValue) as string [];
return nativeReg.parseMultiString(rawValue);
} else if (typeof schemaVal === 'string') {
console.error(`Expecting registry entry ${ fullPath } to be a single string, but it's an array of strings, value: ${ parsedValueForErrorMessage }`);
} else {
Expand Down Expand Up @@ -446,7 +443,7 @@ function validateDeploymentProfile(profile: any, schema: any, parentPathParts: s
console.log(`Deployment Profile ignoring '${ parentPathParts.join('.') }.${ key }': got an array, expecting type ${ typeof schema[key] }.`);
delete profile[key];
}
} else if (haveUserDefinedObject(parentPathParts.concat(key), isEquivalentRespectCase)) {
} else if (haveUserDefinedObject(parentPathParts.concat(key))) {
// Keep this part of the profile
} else {
// Finally recurse and compare the schema sub-object with the specified sub-object
Expand All @@ -457,10 +454,6 @@ function validateDeploymentProfile(profile: any, schema: any, parentPathParts: s
return profile;
}

function isEquivalentRespectCase(a: string, b: string): boolean {
return a === b;
}

const caseInsensitiveComparator = new Intl.Collator('en', { sensitivity: 'base' });

function isEquivalentIgnoreCase(a: string, b: string): boolean {
Expand All @@ -485,15 +478,8 @@ const userDefinedKeys = [
* @param pathParts - On Windows, the parts of the registry path below KEY\Software\Rancher Desktop\Profile\{defaults|locked|}
* The first field is always either 'defaults' or 'locked' and can be ignored
* On other platforms its the path-parts up to but not including the root (which is unnamed anyway).
* @param equivFunc - comparison function used to ignore or respect case.
* @returns boolean
*/
function haveUserDefinedObject(pathParts: string[], equivFunc: (a: string, b: string) => boolean): boolean {
for (const userDefinedKey of userDefinedKeys.filter(userDefinedKey => userDefinedKey.length === pathParts.length)) {
if (pathParts.every((p, i) => equivFunc(p, userDefinedKey[i]))) {
return true;
}
}

return false;
function haveUserDefinedObject(pathParts: string[]): boolean {
return userDefinedKeys.some(userDefinedKey => _.isEqual(userDefinedKey, pathParts));
}

0 comments on commit cf06fdc

Please sign in to comment.