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 new file mode 100644 index 00000000..47c435a8 --- /dev/null +++ b/workflow-parser/src/model/converter/container.test.ts @@ -0,0 +1,318 @@ +/* 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"; + +// 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 + }); + 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..0a1895d7 100644 --- a/workflow-parser/src/model/converter/container.ts +++ b/workflow-parser/src/model/converter/container.ts @@ -1,17 +1,199 @@ +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"; -export function convertToJobContainer(context: TemplateContext, container: TemplateToken): Container | undefined { +function getFeatureFlags(context: TemplateContext): FeatureFlags | undefined { + return context.state["featureFlags"] as FeatureFlags | 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 { + // Feature flag guard — use legacy implementation when flag is off + if (!getFeatureFlags(context)?.isEnabled("containerImageValidation")) { + return convertToJobContainerLegacy(context, container); + } + + 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; + + for (const item of mapping) { + if (item.key.isExpression) { + hasExpressionKey = true; + continue; + } + + const key = item.key.assertString("container item key"); + + switch (key.value) { + case "image": + if (item.value.isExpression) { + hasExpression = true; + break; + } + 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}`); + } + } + + // 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"); + } +} + +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; + } + + 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, true); + if (container) { + serviceList.push(container); + } + } + + return serviceList; +} + +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"); + if (item.value.isExpression) { + continue; + } + + switch (key.value) { + case "username": + username = item.value.assertString("credentials username"); + break; + case "password": + password = item.value.assertString("credentials password"); + break; + default: + context.error(key, `credentials key ${key.value}`); + } + } + + 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; - // Skip validation for expressions for now to match - // behavior of the other parsers for (const [, token] of TemplateToken.traverse(container)) { if (token.isExpression) { return; @@ -19,7 +201,6 @@ export function convertToJobContainer(context: TemplateContext, container: Templ } if (isString(container)) { - // Workflow uses shorthand syntax `container: image-name` image = container.assertString("container item"); return {image: image}; } @@ -35,7 +216,7 @@ export function convertToJobContainer(context: TemplateContext, container: Templ image = value.assertString("container image"); break; case "credentials": - convertToJobCredentials(context, value); + convertToJobCredentialsLegacy(context, value); break; case "env": env = value.assertMapping("container env"); @@ -70,13 +251,13 @@ export function convertToJobContainer(context: TemplateContext, container: Templ } } -export function convertToJobServices(context: TemplateContext, services: TemplateToken): Container[] | undefined { +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 = convertToJobContainer(context, service.value); + const container = convertToJobContainerLegacy(context, service.value); if (container) { serviceList.push(container); } @@ -84,7 +265,7 @@ export function convertToJobServices(context: TemplateContext, services: Templat return serviceList; } -function convertToJobCredentials(context: TemplateContext, value: TemplateToken): Credential | undefined { +function convertToJobCredentialsLegacy(context: TemplateContext, value: TemplateToken): Credential | undefined { const mapping = value.assertMapping("credentials"); let username: StringToken | undefined; 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