From 2d29ecfc625e04ec6f3b631ff3f11fe323f80ef0 Mon Sep 17 00:00:00 2001 From: Josh Spicer Date: Mon, 21 Nov 2022 23:50:47 +0000 Subject: [PATCH 1/6] validate input of getRef() --- .../containerCollectionsOCI.ts | 33 +++++++++++++++++-- .../containerFeaturesConfiguration.ts | 5 ++- .../containerFeaturesOCI.ts | 10 ++++-- .../containerTemplatesOCI.ts | 3 ++ .../publishCommandImpl.ts | 12 ++++--- src/spec-node/featuresCLI/info.ts | 8 +++++ src/spec-node/featuresCLI/publish.ts | 15 +++++++-- src/spec-node/templatesCLI/publish.ts | 5 +++ src/spec-node/utils.ts | 3 ++ 9 files changed, 79 insertions(+), 15 deletions(-) diff --git a/src/spec-configuration/containerCollectionsOCI.ts b/src/spec-configuration/containerCollectionsOCI.ts index 7f95785d0..a6e98d7bd 100644 --- a/src/spec-configuration/containerCollectionsOCI.ts +++ b/src/spec-configuration/containerCollectionsOCI.ts @@ -11,6 +11,18 @@ export const DEVCONTAINER_MANIFEST_MEDIATYPE = 'application/vnd.devcontainers'; export const DEVCONTAINER_TAR_LAYER_MEDIATYPE = 'application/vnd.devcontainers.layer.v1+tar'; export const DEVCONTAINER_COLLECTION_LAYER_MEDIATYPE = 'application/vnd.devcontainers.collection.layer.v1+json'; +// Following Spec: https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests +// Alternative Spec: https://docs.docker.com/registry/spec/api/#overview +// +// Entire path ('namespace' in spec terminology) for the given repository +// (eg: devcontainers/features/go) +const regexForPath = new RegExp('[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*', ''); + + +// MUST be either (a) the digest of the manifest or (b) a tag +// MUST be at most 128 characters in length and MUST match the following regular expression: +const regexForReference = new RegExp('[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}', ''); // + export type HEADERS = { 'authorization'?: string; 'user-agent': string; 'content-type'?: string; 'accept'?: string }; // ghcr.io/devcontainers/features/go:1.0.0 @@ -58,13 +70,17 @@ interface OCITagList { tags: string[]; } -export function getRef(output: Log, resourceAndVersion: string): OCIRef { +// https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests +// Attempts to parse the given string into an OCIRef +export function getRef(output: Log, resourceAndVersion: string): OCIRef | undefined { + // Normalize input by downcasing entire string + resourceAndVersion = resourceAndVersion.toLowerCase(); // ex: ghcr.io/codspace/features/ruby:1 // ex: ghcr.io/codspace/templates/ruby:1 const splitOnColon = resourceAndVersion.split(':'); const resource = splitOnColon[0]; - const version = splitOnColon[1] ? splitOnColon[1] : 'latest'; + const version = splitOnColon[1] ? splitOnColon[1] : 'latest'; // TODO: Support parsing out manifest digest (...@sha256:...) const splitOnSlash = resource.split('/'); @@ -83,6 +99,17 @@ export function getRef(output: Log, resourceAndVersion: string): OCIRef { output.write(`registry: ${registry}`, LogLevel.Trace); output.write(`path: ${path}`, LogLevel.Trace); + // Validate result + if (!regexForPath.test(path)) { + output.write(`Parsed path (${path}) for input '${resourceAndVersion}' failed validation.`, LogLevel.Error); + return undefined; + } + + if (!regexForReference.test(version)) { + output.write(`Parsed version (${version}) for input '${resourceAndVersion}' failed validation.`, LogLevel.Error); + return undefined; + } + return { id, version, @@ -229,7 +256,7 @@ export async function getPublishedVersions(ref: OCIRef, output: Log, sorted: boo let authToken = await fetchRegistryAuthToken(output, ref.registry, ref.path, process.env, 'pull'); if (!authToken) { - output.write(`(!) ERR: Failed to publish ${collectionType}: ${ref.resource}`, LogLevel.Error); + output.write(`(!) ERR: Failed to get published versions for ${collectionType}: ${ref.resource}`, LogLevel.Error); return undefined; } diff --git a/src/spec-configuration/containerFeaturesConfiguration.ts b/src/spec-configuration/containerFeaturesConfiguration.ts index 873b87027..906ec6d52 100644 --- a/src/spec-configuration/containerFeaturesConfiguration.ts +++ b/src/spec-configuration/containerFeaturesConfiguration.ts @@ -12,7 +12,7 @@ import { mkdirpLocal, readLocalFile, rmLocal, writeLocalFile, cpDirectoryLocal, import { Log, LogLevel } from '../spec-utils/log'; import { request } from '../spec-utils/httpRequest'; import { computeFeatureInstallationOrder } from './containerFeaturesOrder'; -import { fetchOCIFeature, getOCIFeatureSet, fetchOCIFeatureManifestIfExistsFromUserIdentifier } from './containerFeaturesOCI'; +import { fetchOCIFeature, tryGetOCIFeatureSet, fetchOCIFeatureManifestIfExistsFromUserIdentifier } from './containerFeaturesOCI'; import { OCIManifest, OCIRef } from './containerCollectionsOCI'; // v1 @@ -769,8 +769,7 @@ export async function processFeatureIdentifier(output: Log, configPath: string, // (6) Oci Identifier if (type === 'oci' && manifest) { - let newFeaturesSet: FeatureSet = getOCIFeatureSet(output, userFeature.id, userFeature.options, manifest, originalUserFeatureId); - return newFeaturesSet; + return tryGetOCIFeatureSet(output, userFeature.id, userFeature.options, manifest, originalUserFeatureId); } output.write(`Github feature.`); diff --git a/src/spec-configuration/containerFeaturesOCI.ts b/src/spec-configuration/containerFeaturesOCI.ts index f6117b280..91bc1c5f9 100644 --- a/src/spec-configuration/containerFeaturesOCI.ts +++ b/src/spec-configuration/containerFeaturesOCI.ts @@ -2,9 +2,12 @@ import { Log, LogLevel } from '../spec-utils/log'; import { Feature, FeatureSet } from './containerFeaturesConfiguration'; import { fetchOCIManifestIfExists, getBlob, getRef, OCIManifest } from './containerCollectionsOCI'; -export function getOCIFeatureSet(output: Log, identifier: string, options: boolean | string | Record, manifest: OCIManifest, originalUserFeatureId: string): FeatureSet { - +export function tryGetOCIFeatureSet(output: Log, identifier: string, options: boolean | string | Record, manifest: OCIManifest, originalUserFeatureId: string): FeatureSet | undefined { const featureRef = getRef(output, identifier); + if (!featureRef) { + output.write(`Unable to parse '${identifier}'`, LogLevel.Error); + return undefined; + } const feat: Feature = { id: featureRef.id, @@ -30,6 +33,9 @@ export function getOCIFeatureSet(output: Log, identifier: string, options: boole export async function fetchOCIFeatureManifestIfExistsFromUserIdentifier(output: Log, env: NodeJS.ProcessEnv, identifier: string, manifestDigest?: string, authToken?: string): Promise { const featureRef = getRef(output, identifier); + if (!featureRef) { + return undefined; + } return await fetchOCIManifestIfExists(output, env, featureRef, manifestDigest, authToken); } diff --git a/src/spec-configuration/containerTemplatesOCI.ts b/src/spec-configuration/containerTemplatesOCI.ts index 84a332400..98a9f93c9 100644 --- a/src/spec-configuration/containerTemplatesOCI.ts +++ b/src/spec-configuration/containerTemplatesOCI.ts @@ -122,6 +122,9 @@ export async function fetchTemplate(output: Log, selectedTemplate: SelectedTempl async function fetchOCITemplateManifestIfExistsFromUserIdentifier(output: Log, env: NodeJS.ProcessEnv, identifier: string, manifestDigest?: string, authToken?: string): Promise { const templateRef = getRef(output, identifier); + if (!templateRef) { + return undefined; + } return await fetchOCIManifestIfExists(output, env, templateRef, manifestDigest, authToken); } diff --git a/src/spec-node/collectionCommonUtils/publishCommandImpl.ts b/src/spec-node/collectionCommonUtils/publishCommandImpl.ts index e5d3bbf61..2976f837f 100644 --- a/src/spec-node/collectionCommonUtils/publishCommandImpl.ts +++ b/src/spec-node/collectionCommonUtils/publishCommandImpl.ts @@ -44,7 +44,7 @@ export async function doPublishCommand(version: string, ociRef: OCIRef, outputDi const publishedVersions = await getPublishedVersions(ociRef, output); if (!publishedVersions) { - process.exit(1); + return false; } const semanticVersions: string[] | undefined = getSermanticVersions(version, publishedVersions, output); @@ -53,11 +53,12 @@ export async function doPublishCommand(version: string, ociRef: OCIRef, outputDi output.write(`Publishing versions: ${semanticVersions.toString()}...`, LogLevel.Info); const pathToTgz = path.join(outputDir, getArchiveName(ociRef.id, collectionType)); if (! await pushOCIFeatureOrTemplate(output, ociRef, pathToTgz, semanticVersions, collectionType)) { - output.write(`(!) ERR: Failed to publish ${collectionType}: ${ociRef.resource}`, LogLevel.Error); - process.exit(1); + output.write(`(!) ERR: Failed to publish ${collectionType}: '${ociRef.resource}'`, LogLevel.Error); + return false; } - output.write(`Published ${collectionType}: ${ociRef.id}...`, LogLevel.Info); } + output.write(`Published ${collectionType}: ${ociRef.id}...`, LogLevel.Info); + return true; } export async function doPublishMetadata(collectionRef: OCICollectionRef, outputDir: string, output: Log, collectionType: string) { @@ -67,7 +68,8 @@ export async function doPublishMetadata(collectionRef: OCICollectionRef, outputD const pathToCollectionFile = path.join(outputDir, OCICollectionFileName); if (! await pushCollectionMetadata(output, collectionRef, pathToCollectionFile, collectionType)) { output.write(`(!) ERR: Failed to publish collection metadata: ${OCICollectionFileName}`, LogLevel.Error); - process.exit(1); + return false; } output.write('Published collection metadata...', LogLevel.Info); + return true; } diff --git a/src/spec-node/featuresCLI/info.ts b/src/spec-node/featuresCLI/info.ts index 23a911e4c..49681e2db 100644 --- a/src/spec-node/featuresCLI/info.ts +++ b/src/spec-node/featuresCLI/info.ts @@ -40,6 +40,14 @@ async function featuresInfo({ }, pkg, new Date(), disposables, true); const featureOciRef = getRef(output, featureId); + if (!featureOciRef) { + if (outputFormat === 'json') { + output.raw(JSON.stringify({}), LogLevel.Info); + } else { + output.raw(`Failed to parse Feature identifier '${featureId}'\n`, LogLevel.Error); + } + process.exit(1); + } const publishedVersions = await getPublishedVersions(featureOciRef, output, true); if (!publishedVersions || publishedVersions.length === 0) { diff --git a/src/spec-node/featuresCLI/publish.ts b/src/spec-node/featuresCLI/publish.ts index 249f9d6c8..34027f133 100644 --- a/src/spec-node/featuresCLI/publish.ts +++ b/src/spec-node/featuresCLI/publish.ts @@ -77,7 +77,15 @@ async function featuresPublish({ const resource = `${registry}/${namespace}/${f.id}`; const featureRef = getRef(output, resource); - await doPublishCommand(f.version, featureRef, outputDir, output, collectionType); + if (!featureRef) { + output.write(`(!) Could not parse provided Feature identifier: '${resource}'`, LogLevel.Error); + process.exit(1); + } + + if (! await doPublishCommand(f.version, featureRef, outputDir, output, collectionType)) { + output.write(`(!) ERR: Failed to publish '${resource}'`, LogLevel.Error); + process.exit(1); + } } const featureCollectionRef: OCICollectionRef = { @@ -86,7 +94,10 @@ async function featuresPublish({ version: 'latest' }; - await doPublishMetadata(featureCollectionRef, outputDir, output, collectionType); + if (! await doPublishMetadata(featureCollectionRef, outputDir, output, collectionType)) { + output.write(`(!) ERR: Failed to publish '${featureCollectionRef.registry}/${featureCollectionRef.path}'`, LogLevel.Error); + process.exit(1); + } // Cleanup await rmLocal(outputDir, { recursive: true, force: true }); diff --git a/src/spec-node/templatesCLI/publish.ts b/src/spec-node/templatesCLI/publish.ts index e876f1fb8..94bd90788 100644 --- a/src/spec-node/templatesCLI/publish.ts +++ b/src/spec-node/templatesCLI/publish.ts @@ -78,6 +78,11 @@ async function templatesPublish({ const resource = `${registry}/${namespace}/${t.id}`; const templateRef = getRef(output, resource); + if (!templateRef) { + output.write(`(!) Could not parse provided Template identifier: '${resource}'`, LogLevel.Error); + process.exit(1); + } + await doPublishCommand(t.version, templateRef, outputDir, output, collectionType); } diff --git a/src/spec-node/utils.ts b/src/spec-node/utils.ts index aa81a09b8..d0814884b 100644 --- a/src/spec-node/utils.ts +++ b/src/spec-node/utils.ts @@ -215,6 +215,9 @@ export async function inspectDockerImage(params: DockerResolverParameters | Dock export async function inspectImageInRegistry(output: Log, name: string, authToken?: string): Promise { const resourceAndVersion = qualifyImageName(name); const ref = getRef(output, resourceAndVersion); + if (!ref) { + throw new Error(`Could not parse image name '${name}'`); + } const auth = authToken ?? await fetchRegistryAuthToken(output, ref.registry, ref.path, process.env, 'pull'); const registryServer = ref.registry === 'docker.io' ? 'registry-1.docker.io' : ref.registry; From 57ce44ccc41e2a1b60ae9144c7f241eab83a5b49 Mon Sep 17 00:00:00 2001 From: Josh Spicer Date: Tue, 22 Nov 2022 18:59:57 +0000 Subject: [PATCH 2/6] update tests using getRef() --- src/test/container-features/containerFeaturesOCI.test.ts | 9 +++++++++ .../container-features/containerFeaturesOCIPush.test.ts | 3 +++ src/test/container-features/featuresCLICommands.test.ts | 3 +++ 3 files changed, 15 insertions(+) diff --git a/src/test/container-features/containerFeaturesOCI.test.ts b/src/test/container-features/containerFeaturesOCI.test.ts index f47bac425..4469a596a 100644 --- a/src/test/container-features/containerFeaturesOCI.test.ts +++ b/src/test/container-features/containerFeaturesOCI.test.ts @@ -7,6 +7,9 @@ export const output = makeLog(createPlainLog(text => process.stdout.write(text), describe('Test OCI Pull', () => { it('Parse OCI identifier', async () => { const feat = getRef(output, 'ghcr.io/codspace/features/ruby:1'); + if (!feat) { + assert.fail('featureRef should not be undefined'); + } output.write(`feat: ${JSON.stringify(feat)}`); assert.equal(feat.id, 'ruby'); @@ -20,6 +23,9 @@ describe('Test OCI Pull', () => { it('Get a manifest by tag', async () => { const featureRef = getRef(output, 'ghcr.io/codspace/features/ruby:1.0.13'); + if (!featureRef) { + assert.fail('featureRef should not be undefined'); + } const manifest = await getManifest(output, process.env, 'https://ghcr.io/v2/codspace/features/ruby/manifests/1.0.13', featureRef); assert.isNotNull(manifest); assert.exists(manifest); @@ -42,6 +48,9 @@ describe('Test OCI Pull', () => { it('Download a feature', async () => { const featureRef = getRef(output, 'ghcr.io/codspace/features/ruby:1.0.13'); + if (!featureRef) { + assert.fail('featureRef should not be undefined'); + } const blobResult = await getBlob(output, process.env, 'https://ghcr.io/v2/codspace/features/ruby/blobs/sha256:8f59630bd1ba6d9e78b485233a0280530b3d0a44338f472206090412ffbd3efb', '/tmp', '/tmp/featureTest', featureRef); assert.isDefined(blobResult); assert.isArray(blobResult?.files); diff --git a/src/test/container-features/containerFeaturesOCIPush.test.ts b/src/test/container-features/containerFeaturesOCIPush.test.ts index 32ebe28ec..2ccfdec7e 100644 --- a/src/test/container-features/containerFeaturesOCIPush.test.ts +++ b/src/test/container-features/containerFeaturesOCIPush.test.ts @@ -46,6 +46,9 @@ describe('Test OCI Push', () => { it('Can check whether a blob exists', async () => { const ociFeatureRef = getRef(output, 'ghcr.io/codspace/features/go:1'); + if (!ociFeatureRef) { + assert.fail('getRef() for the Feature should not be undefined'); + } const { registry, resource } = ociFeatureRef; const sessionAuth = await fetchRegistryAuthToken(output, registry, resource, process.env, 'pull'); if (!sessionAuth) { diff --git a/src/test/container-features/featuresCLICommands.test.ts b/src/test/container-features/featuresCLICommands.test.ts index f265280fc..c25cc85e7 100644 --- a/src/test/container-features/featuresCLICommands.test.ts +++ b/src/test/container-features/featuresCLICommands.test.ts @@ -478,6 +478,9 @@ describe('test function getPublishedVersions', async () => { it('should list published versions', async () => { const resource = 'ghcr.io/devcontainers/features/node'; const featureRef = getRef(output, resource); + if (!featureRef) { + assert.fail('featureRef should not be undefined'); + } const versionsList = await getPublishedVersions(featureRef, output) ?? []; assert.includeMembers(versionsList, ['1', '1.0', '1.0.0', 'latest']); }); From f6fab1acc5d5efce5820625d43bc774a761a4a8d Mon Sep 17 00:00:00 2001 From: Josh Spicer Date: Tue, 22 Nov 2022 19:16:28 +0000 Subject: [PATCH 3/6] add unit tests --- .../containerFeaturesOCI.test.ts | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/test/container-features/containerFeaturesOCI.test.ts b/src/test/container-features/containerFeaturesOCI.test.ts index 4469a596a..79fa37004 100644 --- a/src/test/container-features/containerFeaturesOCI.test.ts +++ b/src/test/container-features/containerFeaturesOCI.test.ts @@ -4,6 +4,50 @@ import { createPlainLog, LogLevel, makeLog } from '../../spec-utils/log'; export const output = makeLog(createPlainLog(text => process.stdout.write(text), () => LogLevel.Trace)); +describe('getRef()', async function () { + this.timeout('120s'); + + it('valid getRef() with a tag', async () => { + const feat = getRef(output, 'ghcr.io/devcontainers/templates/docker-from-docker:latest'); + if (!feat) { + assert.fail('featureRef should not be undefined'); + } + assert.ok(feat); + assert.equal(feat.id, 'docker-from-docker'); + assert.equal(feat.namespace, 'devcontainers/templates'); + assert.equal(feat.owner, 'devcontainers'); + assert.equal(feat.registry, 'ghcr.io'); + assert.equal(feat.resource, 'ghcr.io/devcontainers/templates/docker-from-docker'); + assert.equal(feat.version, 'latest'); + assert.equal(feat.path, 'devcontainers/templates/docker-from-docker'); + }); + + it('valid getRef() without a version tag', async () => { + const feat = getRef(output, 'ghcr.io/devcontainers/templates/docker-from-docker'); + if (!feat) { + assert.fail('featureRef should not be undefined'); + } + assert.ok(feat); + assert.equal(feat.id, 'docker-from-docker'); + assert.equal(feat.namespace, 'devcontainers/templates'); + assert.equal(feat.owner, 'devcontainers'); + assert.equal(feat.registry, 'ghcr.io'); + assert.equal(feat.resource, 'ghcr.io/devcontainers/templates/docker-from-docker'); + assert.equal(feat.path, 'devcontainers/templates/docker-from-docker'); + assert.isUndefined(feat.version); + }); + + it('invalid getRef() with duplicate version tags', async () => { + const feat = getRef(output, 'ghcr.io/devcontainers/templates/docker-from-docker:latest:latest'); + assert.isUndefined(feat); + }); + + it('invalid getRef() with invalid character', async () => { + const feat = getRef(output, 'ghcr.io/devco%ntainers/templates/docker-from-docker:latest'); + assert.isUndefined(feat); + }); +}); + describe('Test OCI Pull', () => { it('Parse OCI identifier', async () => { const feat = getRef(output, 'ghcr.io/codspace/features/ruby:1'); From 58070e32653733d8ef3b7be3cd52c958dbe20a5b Mon Sep 17 00:00:00 2001 From: Josh Spicer Date: Tue, 22 Nov 2022 20:42:43 +0000 Subject: [PATCH 4/6] Refactor getRef() --- .../containerCollectionsOCI.ts | 54 +++++++++++-------- .../containerFeaturesOCI.test.ts | 2 +- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/spec-configuration/containerCollectionsOCI.ts b/src/spec-configuration/containerCollectionsOCI.ts index a6e98d7bd..843ea581a 100644 --- a/src/spec-configuration/containerCollectionsOCI.ts +++ b/src/spec-configuration/containerCollectionsOCI.ts @@ -11,18 +11,6 @@ export const DEVCONTAINER_MANIFEST_MEDIATYPE = 'application/vnd.devcontainers'; export const DEVCONTAINER_TAR_LAYER_MEDIATYPE = 'application/vnd.devcontainers.layer.v1+tar'; export const DEVCONTAINER_COLLECTION_LAYER_MEDIATYPE = 'application/vnd.devcontainers.collection.layer.v1+json'; -// Following Spec: https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests -// Alternative Spec: https://docs.docker.com/registry/spec/api/#overview -// -// Entire path ('namespace' in spec terminology) for the given repository -// (eg: devcontainers/features/go) -const regexForPath = new RegExp('[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*', ''); - - -// MUST be either (a) the digest of the manifest or (b) a tag -// MUST be at most 128 characters in length and MUST match the following regular expression: -const regexForReference = new RegExp('[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}', ''); // - export type HEADERS = { 'authorization'?: string; 'user-agent': string; 'content-type'?: string; 'accept'?: string }; // ghcr.io/devcontainers/features/go:1.0.0 @@ -70,17 +58,36 @@ interface OCITagList { tags: string[]; } +// Following Spec: https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests +// Alternative Spec: https://docs.docker.com/registry/spec/api/#overview +// +// Entire path ('namespace' in spec terminology) for the given repository +// (eg: devcontainers/features/go) +const regexForPath = /[a-z0-9]+([._-][a-z0-9]+)*(\/[a-z0-9]+([._-][a-z0-9]+)*)*/; +// MUST be either (a) the digest of the manifest or (b) a tag +// MUST be at most 128 characters in length and MUST match the following regular expression: +const regexForReference = /[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}/; + // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests // Attempts to parse the given string into an OCIRef -export function getRef(output: Log, resourceAndVersion: string): OCIRef | undefined { +export function getRef(output: Log, input: string): OCIRef | undefined { // Normalize input by downcasing entire string - resourceAndVersion = resourceAndVersion.toLowerCase(); + input = input.toLowerCase(); // ex: ghcr.io/codspace/features/ruby:1 // ex: ghcr.io/codspace/templates/ruby:1 - const splitOnColon = resourceAndVersion.split(':'); - const resource = splitOnColon[0]; - const version = splitOnColon[1] ? splitOnColon[1] : 'latest'; // TODO: Support parsing out manifest digest (...@sha256:...) + const indexOfLastColon = input.lastIndexOf(':'); + + let resource = ''; + let version = ''; // TODO: Support parsing out manifest digest (...@sha256:...) + if (indexOfLastColon === -1) { + // No colon, assume latest. + resource = input; + version = 'latest'; + } else { + resource = input.substring(0, indexOfLastColon); + version = input.substring(indexOfLastColon + 1); + } const splitOnSlash = resource.split('/'); @@ -99,14 +106,17 @@ export function getRef(output: Log, resourceAndVersion: string): OCIRef | undefi output.write(`registry: ${registry}`, LogLevel.Trace); output.write(`path: ${path}`, LogLevel.Trace); - // Validate result - if (!regexForPath.test(path)) { - output.write(`Parsed path (${path}) for input '${resourceAndVersion}' failed validation.`, LogLevel.Error); + // Validate results of parse. + + const regexForPathResult = regexForPath.exec(path); + if (!regexForPathResult || regexForPathResult[0] !== path) { + output.write(`Parsed path '${path}') for input '${input}' failed validation.`, LogLevel.Error); return undefined; } - if (!regexForReference.test(version)) { - output.write(`Parsed version (${version}) for input '${resourceAndVersion}' failed validation.`, LogLevel.Error); + const regexForReferenceResult = regexForReference.exec(version); + if (!regexForReferenceResult || regexForReferenceResult[0] !== version) { + output.write(`Parsed version '${version}' for input '${input}' failed validation.`, LogLevel.Error); return undefined; } diff --git a/src/test/container-features/containerFeaturesOCI.test.ts b/src/test/container-features/containerFeaturesOCI.test.ts index 79fa37004..d7131203b 100644 --- a/src/test/container-features/containerFeaturesOCI.test.ts +++ b/src/test/container-features/containerFeaturesOCI.test.ts @@ -34,7 +34,7 @@ describe('getRef()', async function () { assert.equal(feat.registry, 'ghcr.io'); assert.equal(feat.resource, 'ghcr.io/devcontainers/templates/docker-from-docker'); assert.equal(feat.path, 'devcontainers/templates/docker-from-docker'); - assert.isUndefined(feat.version); + assert.equal(feat.version, 'latest'); // Defaults to 'latest' if not version supplied. }); it('invalid getRef() with duplicate version tags', async () => { From 5be0b46db241cc96bb965f312914491a64325244 Mon Sep 17 00:00:00 2001 From: Josh Spicer Date: Tue, 22 Nov 2022 22:02:09 +0000 Subject: [PATCH 5/6] add more test cases --- .../containerCollectionsOCI.ts | 14 ++-- .../containerFeaturesOCI.test.ts | 68 ++++++++++++++++++- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/src/spec-configuration/containerCollectionsOCI.ts b/src/spec-configuration/containerCollectionsOCI.ts index 843ea581a..0f3ce6bff 100644 --- a/src/spec-configuration/containerCollectionsOCI.ts +++ b/src/spec-configuration/containerCollectionsOCI.ts @@ -74,14 +74,16 @@ export function getRef(output: Log, input: string): OCIRef | undefined { // Normalize input by downcasing entire string input = input.toLowerCase(); - // ex: ghcr.io/codspace/features/ruby:1 - // ex: ghcr.io/codspace/templates/ruby:1 const indexOfLastColon = input.lastIndexOf(':'); let resource = ''; let version = ''; // TODO: Support parsing out manifest digest (...@sha256:...) - if (indexOfLastColon === -1) { - // No colon, assume latest. + + // 'If' condition is true in the following cases: + // 1. The final colon is before the first slash (a port) : eg: ghcr.io:8081/codspace/features/ruby + // 2. There is no version : eg: ghcr.io/codspace/features/ruby + // In both cases, assume 'latest' tag. + if (indexOfLastColon === -1 || indexOfLastColon < input.indexOf('/')) { resource = input; version = 'latest'; } else { @@ -102,7 +104,7 @@ export function getRef(output: Log, input: string): OCIRef | undefined { output.write(`id: ${id}`, LogLevel.Trace); output.write(`version: ${version}`, LogLevel.Trace); output.write(`owner: ${owner}`, LogLevel.Trace); - output.write(`namespace: ${namespace}`, LogLevel.Trace); + output.write(`namespace: ${namespace}`, LogLevel.Trace); // TODO: We assume 'namespace' includes at least one slash (eg: 'devcontainers/features') output.write(`registry: ${registry}`, LogLevel.Trace); output.write(`path: ${path}`, LogLevel.Trace); @@ -110,7 +112,7 @@ export function getRef(output: Log, input: string): OCIRef | undefined { const regexForPathResult = regexForPath.exec(path); if (!regexForPathResult || regexForPathResult[0] !== path) { - output.write(`Parsed path '${path}') for input '${input}' failed validation.`, LogLevel.Error); + output.write(`Parsed path '${path}' for input '${input}' failed validation.`, LogLevel.Error); return undefined; } diff --git a/src/test/container-features/containerFeaturesOCI.test.ts b/src/test/container-features/containerFeaturesOCI.test.ts index d7131203b..fafcdfb8d 100644 --- a/src/test/container-features/containerFeaturesOCI.test.ts +++ b/src/test/container-features/containerFeaturesOCI.test.ts @@ -37,15 +37,81 @@ describe('getRef()', async function () { assert.equal(feat.version, 'latest'); // Defaults to 'latest' if not version supplied. }); + it('valid getRef() automatically downcases', async () => { + const feat = getRef(output, 'ghcr.io/DeVContainERS/templates/Docker-FROM-Docker'); + if (!feat) { + assert.fail('featureRef should not be undefined'); + } + assert.ok(feat); + assert.equal(feat.id, 'docker-from-docker'); + assert.equal(feat.namespace, 'devcontainers/templates'); + assert.equal(feat.owner, 'devcontainers'); + assert.equal(feat.registry, 'ghcr.io'); + assert.equal(feat.resource, 'ghcr.io/devcontainers/templates/docker-from-docker'); + assert.equal(feat.path, 'devcontainers/templates/docker-from-docker'); + assert.equal(feat.version, 'latest'); // Defaults to 'latest' if not version supplied. + }); + + it('valid getRef() with a registry that contains a port.', async () => { + const feat = getRef(output, 'docker.io:8001/devcontainers/templates/docker-from-docker:latest'); + if (!feat) { + assert.fail('featureRef should not be undefined'); + } + assert.ok(feat); + assert.equal(feat.id, 'docker-from-docker'); + assert.equal(feat.namespace, 'devcontainers/templates'); + assert.equal(feat.owner, 'devcontainers'); + assert.equal(feat.registry, 'docker.io:8001'); + assert.equal(feat.resource, 'docker.io:8001/devcontainers/templates/docker-from-docker'); + assert.equal(feat.path, 'devcontainers/templates/docker-from-docker'); + assert.equal(feat.version, 'latest'); // Defaults to 'latest' if not version supplied. + }); + + it('valid getRef() really short path and no version', async () => { + const feat = getRef(output, 'docker.io:8001/a/b/c'); + if (!feat) { + assert.fail('featureRef should not be undefined'); + } + assert.ok(feat); + assert.equal(feat.id, 'c'); + assert.equal(feat.namespace, 'a/b'); + assert.equal(feat.owner, 'a'); + assert.equal(feat.registry, 'docker.io:8001'); + assert.equal(feat.resource, 'docker.io:8001/a/b/c'); + assert.equal(feat.path, 'a/b/c'); + assert.equal(feat.version, 'latest'); // Defaults to 'latest' if not version supplied. + }); + it('invalid getRef() with duplicate version tags', async () => { const feat = getRef(output, 'ghcr.io/devcontainers/templates/docker-from-docker:latest:latest'); assert.isUndefined(feat); }); - it('invalid getRef() with invalid character', async () => { + it('invalid getRef() with invalid character in namespace', async () => { const feat = getRef(output, 'ghcr.io/devco%ntainers/templates/docker-from-docker:latest'); assert.isUndefined(feat); }); + + it('invalid getRef() with invalid character in feature name', async () => { + const feat = getRef(output, 'ghcr.io/devcontainers/templates/docker-from@docker:latest'); + assert.isUndefined(feat); + }); + + it('invalid getRef() with missing path with version tag', async () => { + const feat = getRef(output, 'ghcr.io/:latest'); + assert.isUndefined(feat); + }); + + it('invalid getRef() with missing path without version tag', async () => { + const feat = getRef(output, 'ghcr.io'); + assert.isUndefined(feat); + }); + + it('invalid getRef() multiple slashes in sequence', async () => { + const feat = getRef(output, 'ghcr.io/devcontainers//templates/docker-from-docker:latest'); + assert.isUndefined(feat); + }); + }); describe('Test OCI Pull', () => { From d55df58487d704f8ef9c0b083205d7f1b0acf905 Mon Sep 17 00:00:00 2001 From: Josh Spicer Date: Wed, 23 Nov 2022 15:48:01 +0000 Subject: [PATCH 6/6] dregex match entire string --- src/spec-configuration/containerCollectionsOCI.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/spec-configuration/containerCollectionsOCI.ts b/src/spec-configuration/containerCollectionsOCI.ts index 0f3ce6bff..9a23f0e8a 100644 --- a/src/spec-configuration/containerCollectionsOCI.ts +++ b/src/spec-configuration/containerCollectionsOCI.ts @@ -63,10 +63,10 @@ interface OCITagList { // // Entire path ('namespace' in spec terminology) for the given repository // (eg: devcontainers/features/go) -const regexForPath = /[a-z0-9]+([._-][a-z0-9]+)*(\/[a-z0-9]+([._-][a-z0-9]+)*)*/; +const regexForPath = /^[a-z0-9]+([._-][a-z0-9]+)*(\/[a-z0-9]+([._-][a-z0-9]+)*)*$/; // MUST be either (a) the digest of the manifest or (b) a tag // MUST be at most 128 characters in length and MUST match the following regular expression: -const regexForReference = /[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}/; +const regexForReference = /^[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}$/; // https://github.com/opencontainers/distribution-spec/blob/main/spec.md#pulling-manifests // Attempts to parse the given string into an OCIRef @@ -110,14 +110,12 @@ export function getRef(output: Log, input: string): OCIRef | undefined { // Validate results of parse. - const regexForPathResult = regexForPath.exec(path); - if (!regexForPathResult || regexForPathResult[0] !== path) { + if (!regexForPath.exec(path)) { output.write(`Parsed path '${path}' for input '${input}' failed validation.`, LogLevel.Error); return undefined; } - const regexForReferenceResult = regexForReference.exec(version); - if (!regexForReferenceResult || regexForReferenceResult[0] !== version) { + if (!regexForReference.test(version)) { output.write(`Parsed version '${version}' for input '${input}' failed validation.`, LogLevel.Error); return undefined; }