From 671f92dbc64d13d74e1ad5ad14909c8ee65ab064 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Thu, 5 Feb 2026 23:05:56 +0000 Subject: [PATCH 1/2] Add validation for empty container image Related PR: - https://github.com/actions/runner/pull/4220 Relaxing schema non-empty-string for container/service image and moving to custom validation. This matches current production behavior which allows empty string at runtime, but not parse time. --- .../src/model/converter/container.test.ts | 314 ++++++++++++++++++ .../src/model/converter/container.ts | 175 ++++++---- workflow-parser/src/model/converter/job.ts | 4 +- workflow-parser/src/workflow-v1.0.json | 4 +- workflow-parser/testdata/skipped-tests.txt | 1 + 5 files changed, 437 insertions(+), 61 deletions(-) create mode 100644 workflow-parser/src/model/converter/container.test.ts diff --git a/workflow-parser/src/model/converter/container.test.ts b/workflow-parser/src/model/converter/container.test.ts new file mode 100644 index 00000000..a57f3794 --- /dev/null +++ b/workflow-parser/src/model/converter/container.test.ts @@ -0,0 +1,314 @@ +/* eslint-disable @typescript-eslint/no-non-null-assertion */ +import {nullTrace} from "../../test-utils/null-trace.js"; +import {parseWorkflow} from "../../workflows/workflow-parser.js"; +import {convertWorkflowTemplate, ErrorPolicy} from "../convert.js"; + +async function getErrors(content: string): Promise { + const result = parseWorkflow({name: "wf.yaml", content}, nullTrace); + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + return (template.errors ?? []).map((e: {Message: string}) => e.Message); +} + +function expectNoContainerErrors(errors: string[]): void { + const containerErrors = errors.filter(e => e.includes("Container image")); + expect(containerErrors).toHaveLength(0); +} + +function expectContainerError(errors: string[], count = 1): void { + const containerErrors = errors.filter(e => e.includes("Container image cannot be empty")); + expect(containerErrors).toHaveLength(count); +} + +describe("container image validation", () => { + describe("shorthand form", () => { + it("container: '' is silent for job container", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: '' + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container: valid-image passes", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: ubuntu:16.04 + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + }); + + describe("mapping form", () => { + it("container image: '' errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: '' + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container image: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container: {} (empty object, missing image) errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: {} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container image: null errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("empty image with expression in other field still errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: '' + options: \${{ matrix.opts }} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + }); + + describe("services shorthand", () => { + it("services svc: '' errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: '' + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("services svc: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + }); + + describe("services mapping", () => { + it("services svc image: '' errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: + image: '' + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("services svc image: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: + image: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("services svc: {} (empty object) errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: {} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("empty image with expression sibling service still errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc1: + image: '' + svc2: \${{ matrix.svc }} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + }); + + describe("expression safety", () => { + it("container: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: \${{ matrix.container }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container image: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: \${{ matrix.image }} + options: --privileged + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container with expression key skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + \${{ vars.KEY }}: ubuntu + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: \${{ fromJSON(inputs.services) }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services with expression alias key skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + \${{ matrix.alias }}: postgres + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services container with expression key skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + db: + \${{ vars.KEY }}: postgres + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container with all expression fields skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: \${{ matrix.image }} + options: \${{ matrix.options }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services svc: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + db: \${{ matrix.db }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services image: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + db: + image: \${{ matrix.db_image }} + options: --health-cmd pg_isready + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + }); +}); diff --git a/workflow-parser/src/model/converter/container.ts b/workflow-parser/src/model/converter/container.ts index 4c4bf79e..8ef9b273 100644 --- a/workflow-parser/src/model/converter/container.ts +++ b/workflow-parser/src/model/converter/container.ts @@ -3,103 +3,164 @@ import {MappingToken, SequenceToken, StringToken, TemplateToken} from "../../tem import {isString} from "../../templates/tokens/type-guards.js"; import {Container, Credential} from "../workflow-template.js"; -export function convertToJobContainer(context: TemplateContext, container: TemplateToken): Container | undefined { +const DOCKER_URI_PREFIX = "docker://"; + +function isEmptyImage(value: string): boolean { + const trimmed = value.startsWith(DOCKER_URI_PREFIX) ? value.substring(DOCKER_URI_PREFIX.length) : value; + return trimmed.length === 0; +} + +export function convertToJobContainer( + context: TemplateContext, + container: TemplateToken, + isServiceContainer = false +): Container | undefined { + if (container.isExpression) { + return; + } + + // Shorthand form + if (isString(container)) { + const image = container.assertString("container item"); + if (!image || image.value.length === 0) { + if (isServiceContainer) { + context.error(container, "Container image cannot be empty"); + } + return; + } + + if (isEmptyImage(image.value)) { + context.error(container, "Container image cannot be empty"); + return; + } + + return {image}; + } + + // Mapping form + const mapping = container.assertMapping("container item"); + if (!mapping) { + return; + } + let image: StringToken | undefined; let env: MappingToken | undefined; let ports: SequenceToken | undefined; let volumes: SequenceToken | undefined; let options: StringToken | undefined; + let credentials: Credential | undefined; + let hasExpressionKey = false; + let hasExpression = false; - // Skip validation for expressions for now to match - // behavior of the other parsers - for (const [, token] of TemplateToken.traverse(container)) { - if (token.isExpression) { - return; + for (const item of mapping) { + if (item.key.isExpression) { + hasExpressionKey = true; + continue; } - } - if (isString(container)) { - // Workflow uses shorthand syntax `container: image-name` - image = container.assertString("container item"); - return {image: image}; - } + const key = item.key.assertString("container item key"); - const mapping = container.assertMapping("container item"); - if (mapping) - for (const item of mapping) { - const key = item.key.assertString("container item key"); - const value = item.value; - - switch (key.value) { - case "image": - image = value.assertString("container image"); - break; - case "credentials": - convertToJobCredentials(context, value); - break; - case "env": - env = value.assertMapping("container env"); - for (const envItem of env) { - envItem.key.assertString("container env value"); - } - break; - case "ports": - ports = value.assertSequence("container ports"); - for (const port of ports) { - port.assertString("container port"); - } - break; - case "volumes": - volumes = value.assertSequence("container volumes"); - for (const volume of volumes) { - volume.assertString("container volume"); - } - break; - case "options": - options = value.assertString("container options"); + switch (key.value) { + case "image": + if (item.value.isExpression) { + hasExpression = true; break; - default: - context.error(key, `Unexpected container item key: ${key.value}`); - } + } + image = item.value.assertString("container image"); + break; + case "credentials": + if (!item.value.isExpression) { + credentials = convertCredentials(context, item.value); + } + break; + case "env": + if (!item.value.isExpression) { + env = item.value.assertMapping("container env"); + } + break; + case "ports": + if (!item.value.isExpression) { + ports = item.value.assertSequence("container ports"); + } + break; + case "volumes": + if (!item.value.isExpression) { + volumes = item.value.assertSequence("container volumes"); + } + break; + case "options": + if (!item.value.isExpression) { + options = item.value.assertString("container options"); + } + break; + default: + context.error(key, `Unexpected container item key: ${key.value}`); } + } - if (!image) { + // Validate image + if (image) { + if (isEmptyImage(image.value)) { + context.error(image, "Container image cannot be empty"); + return; + } + return {image, credentials, env, ports, volumes, options}; + } + + // No image key — skip error if expression keys could provide one + if (!hasExpressionKey && !hasExpression) { context.error(container, "Container image cannot be empty"); - } else { - return {image, env, ports, volumes, options}; } } export function convertToJobServices(context: TemplateContext, services: TemplateToken): Container[] | undefined { - const serviceList: Container[] = []; + if (services.isExpression) { + return; + } + const serviceList: Container[] = []; const mapping = services.assertMapping("services"); + for (const service of mapping) { + if (service.key.isExpression) { + continue; + } + service.key.assertString("service key"); - const container = convertToJobContainer(context, service.value); + const container = convertToJobContainer(context, service.value, true); if (container) { serviceList.push(container); } } + return serviceList; } -function convertToJobCredentials(context: TemplateContext, value: TemplateToken): Credential | undefined { +function convertCredentials(context: TemplateContext, value: TemplateToken): Credential | undefined { const mapping = value.assertMapping("credentials"); + if (!mapping) { + return; + } let username: StringToken | undefined; let password: StringToken | undefined; for (const item of mapping) { + if (item.key.isExpression) { + continue; + } + const key = item.key.assertString("credentials item"); - const value = item.value; + if (item.value.isExpression) { + continue; + } switch (key.value) { case "username": - username = value.assertString("credentials username"); + username = item.value.assertString("credentials username"); break; case "password": - password = value.assertString("credentials password"); + password = item.value.assertString("credentials password"); break; default: context.error(key, `credentials key ${key.value}`); diff --git a/workflow-parser/src/model/converter/job.ts b/workflow-parser/src/model/converter/job.ts index 514de15a..dda4a634 100644 --- a/workflow-parser/src/model/converter/job.ts +++ b/workflow-parser/src/model/converter/job.ts @@ -50,7 +50,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: break; case "container": - convertToJobContainer(context, item.value); + handleTemplateTokenErrors(item.value, context, undefined, () => convertToJobContainer(context, item.value)); container = item.value; break; @@ -103,7 +103,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: break; case "services": - convertToJobServices(context, item.value); + handleTemplateTokenErrors(item.value, context, undefined, () => convertToJobServices(context, item.value)); services = item.value; break; diff --git a/workflow-parser/src/workflow-v1.0.json b/workflow-parser/src/workflow-v1.0.json index 07a6d6dd..6d21be4f 100644 --- a/workflow-parser/src/workflow-v1.0.json +++ b/workflow-parser/src/workflow-v1.0.json @@ -2345,7 +2345,7 @@ "mapping": { "properties": { "image": { - "type": "non-empty-string", + "type": "string", "description": "Use `jobs..container.image` to define the Docker image to use as the container to run the action. The value can be the Docker Hub image or a registry name." }, "options": { @@ -2390,7 +2390,7 @@ "matrix" ], "one-of": [ - "non-empty-string", + "string", "container-mapping" ] }, diff --git a/workflow-parser/testdata/skipped-tests.txt b/workflow-parser/testdata/skipped-tests.txt index 9f0ac875..44f4fae2 100644 --- a/workflow-parser/testdata/skipped-tests.txt +++ b/workflow-parser/testdata/skipped-tests.txt @@ -91,3 +91,4 @@ yaml-schema-sequence.yml yaml-schema-str-flow-styles.yml yaml-schema-string.yml yaml-schema-timestamp.yml +job-container-invalid.yml From c85997ad0da37199ba5c7e09ec1019aae260a8a3 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Fri, 13 Feb 2026 21:02:30 +0000 Subject: [PATCH 2/2] Gate container image validation behind feature flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add containerImageValidation experimental feature flag that gates the new container image validation behind an opt-in toggle. When the flag is off (default), the legacy converter logic is used. When enabled, the improved validation with expression handling runs. The legacy code is duplicated to keep code paths fully isolated and make the eventual cleanup diff minimal — just delete the legacy functions and the flag guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- expressions/src/features.ts | 13 +- workflow-parser/src/model/convert.ts | 18 ++- .../src/model/converter/container.test.ts | 4 + .../src/model/converter/container.ts | 120 ++++++++++++++++++ 4 files changed, 152 insertions(+), 3 deletions(-) diff --git a/expressions/src/features.ts b/expressions/src/features.ts index b0a07709..4bc90b36 100644 --- a/expressions/src/features.ts +++ b/expressions/src/features.ts @@ -28,6 +28,13 @@ export interface ExperimentalFeatures { * @default false */ blockScalarChompingWarning?: boolean; + + /** + * Enable improved container image validation that handles + * expressions gracefully and validates empty/docker:// images. + * @default false + */ + containerImageValidation?: boolean; } /** @@ -39,7 +46,11 @@ export type ExperimentalFeatureKey = Exclude; * All known experimental feature keys. * This list must be kept in sync with the ExperimentalFeatures interface. */ -const allFeatureKeys: ExperimentalFeatureKey[] = ["missingInputsQuickfix", "blockScalarChompingWarning"]; +const allFeatureKeys: ExperimentalFeatureKey[] = [ + "missingInputsQuickfix", + "blockScalarChompingWarning", + "containerImageValidation" +]; export class FeatureFlags { private readonly features: ExperimentalFeatures; diff --git a/workflow-parser/src/model/convert.ts b/workflow-parser/src/model/convert.ts index afc14d45..7dbc946d 100644 --- a/workflow-parser/src/model/convert.ts +++ b/workflow-parser/src/model/convert.ts @@ -1,3 +1,4 @@ +import {FeatureFlags} from "@actions/expressions"; import {TemplateContext} from "../templates/template-context.js"; import {TemplateToken, TemplateTokenError} from "../templates/tokens/template-token.js"; import {FileProvider} from "../workflows/file-provider.js"; @@ -37,9 +38,15 @@ export type WorkflowTemplateConverterOptions = { * By default, conversion will be skipped if there are errors in the {@link TemplateContext}. */ errorPolicy?: ErrorPolicy; + + /** + * Feature flags for experimental features. + * When not provided, all experimental features are disabled. + */ + featureFlags?: FeatureFlags; }; -const defaultOptions: Required = { +const defaultOptions: Omit, "featureFlags"> = { maxReusableWorkflowDepth: 4, fetchReusableWorkflowDepth: 0, errorPolicy: ErrorPolicy.ReturnErrorsOnly @@ -54,6 +61,11 @@ export async function convertWorkflowTemplate( const result = {} as WorkflowTemplate; const opts = getOptionsWithDefaults(options); + // Store feature flags in context for converter functions + if (options.featureFlags) { + context.state["featureFlags"] = options.featureFlags; + } + if (context.errors.getErrors().length > 0 && opts.errorPolicy === ErrorPolicy.ReturnErrorsOnly) { result.errors = context.errors.getErrors().map(x => ({ Message: x.message @@ -132,7 +144,9 @@ export async function convertWorkflowTemplate( return result; } -function getOptionsWithDefaults(options: WorkflowTemplateConverterOptions): Required { +function getOptionsWithDefaults( + options: WorkflowTemplateConverterOptions +): Omit, "featureFlags"> { return { maxReusableWorkflowDepth: options.maxReusableWorkflowDepth !== undefined diff --git a/workflow-parser/src/model/converter/container.test.ts b/workflow-parser/src/model/converter/container.test.ts index a57f3794..47c435a8 100644 --- a/workflow-parser/src/model/converter/container.test.ts +++ b/workflow-parser/src/model/converter/container.test.ts @@ -3,8 +3,12 @@ import {nullTrace} from "../../test-utils/null-trace.js"; import {parseWorkflow} from "../../workflows/workflow-parser.js"; import {convertWorkflowTemplate, ErrorPolicy} from "../convert.js"; +// Minimal FeatureFlags-compatible object for tests +const featureFlags = {isEnabled: (f: string) => f === "containerImageValidation"}; + async function getErrors(content: string): Promise { const result = parseWorkflow({name: "wf.yaml", content}, nullTrace); + result.context.state["featureFlags"] = featureFlags; const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { errorPolicy: ErrorPolicy.TryConversion }); diff --git a/workflow-parser/src/model/converter/container.ts b/workflow-parser/src/model/converter/container.ts index 8ef9b273..0a1895d7 100644 --- a/workflow-parser/src/model/converter/container.ts +++ b/workflow-parser/src/model/converter/container.ts @@ -1,8 +1,13 @@ +import {FeatureFlags} from "@actions/expressions"; import {TemplateContext} from "../../templates/template-context.js"; import {MappingToken, SequenceToken, StringToken, TemplateToken} from "../../templates/tokens/index.js"; import {isString} from "../../templates/tokens/type-guards.js"; import {Container, Credential} from "../workflow-template.js"; +function getFeatureFlags(context: TemplateContext): FeatureFlags | undefined { + return context.state["featureFlags"] as FeatureFlags | undefined; +} + const DOCKER_URI_PREFIX = "docker://"; function isEmptyImage(value: string): boolean { @@ -15,6 +20,11 @@ export function convertToJobContainer( container: TemplateToken, isServiceContainer = false ): Container | undefined { + // Feature flag guard — use legacy implementation when flag is off + if (!getFeatureFlags(context)?.isEnabled("containerImageValidation")) { + return convertToJobContainerLegacy(context, container); + } + if (container.isExpression) { return; } @@ -114,6 +124,11 @@ export function convertToJobContainer( } export function convertToJobServices(context: TemplateContext, services: TemplateToken): Container[] | undefined { + // Feature flag guard — use legacy implementation when flag is off + if (!getFeatureFlags(context)?.isEnabled("containerImageValidation")) { + return convertToJobServicesLegacy(context, services); + } + if (services.isExpression) { return; } @@ -169,3 +184,108 @@ function convertCredentials(context: TemplateContext, value: TemplateToken): Cre return {username, password}; } + +// ===== Legacy implementations (remove when containerImageValidation graduates) ===== + +function convertToJobContainerLegacy(context: TemplateContext, container: TemplateToken): Container | undefined { + let image: StringToken | undefined; + let env: MappingToken | undefined; + let ports: SequenceToken | undefined; + let volumes: SequenceToken | undefined; + let options: StringToken | undefined; + + for (const [, token] of TemplateToken.traverse(container)) { + if (token.isExpression) { + return; + } + } + + if (isString(container)) { + image = container.assertString("container item"); + return {image: image}; + } + + const mapping = container.assertMapping("container item"); + if (mapping) + for (const item of mapping) { + const key = item.key.assertString("container item key"); + const value = item.value; + + switch (key.value) { + case "image": + image = value.assertString("container image"); + break; + case "credentials": + convertToJobCredentialsLegacy(context, value); + break; + case "env": + env = value.assertMapping("container env"); + for (const envItem of env) { + envItem.key.assertString("container env value"); + } + break; + case "ports": + ports = value.assertSequence("container ports"); + for (const port of ports) { + port.assertString("container port"); + } + break; + case "volumes": + volumes = value.assertSequence("container volumes"); + for (const volume of volumes) { + volume.assertString("container volume"); + } + break; + case "options": + options = value.assertString("container options"); + break; + default: + context.error(key, `Unexpected container item key: ${key.value}`); + } + } + + if (!image) { + context.error(container, "Container image cannot be empty"); + } else { + return {image, env, ports, volumes, options}; + } +} + +function convertToJobServicesLegacy(context: TemplateContext, services: TemplateToken): Container[] | undefined { + const serviceList: Container[] = []; + + const mapping = services.assertMapping("services"); + for (const service of mapping) { + service.key.assertString("service key"); + const container = convertToJobContainerLegacy(context, service.value); + if (container) { + serviceList.push(container); + } + } + return serviceList; +} + +function convertToJobCredentialsLegacy(context: TemplateContext, value: TemplateToken): Credential | undefined { + const mapping = value.assertMapping("credentials"); + + let username: StringToken | undefined; + let password: StringToken | undefined; + + for (const item of mapping) { + const key = item.key.assertString("credentials item"); + const value = item.value; + + switch (key.value) { + case "username": + username = value.assertString("credentials username"); + break; + case "password": + password = value.assertString("credentials password"); + break; + default: + context.error(key, `credentials key ${key.value}`); + } + } + + return {username, password}; +}