diff --git a/languageservice/src/context-providers/default.ts b/languageservice/src/context-providers/default.ts index f7fa83e..0086efe 100644 --- a/languageservice/src/context-providers/default.ts +++ b/languageservice/src/context-providers/default.ts @@ -198,9 +198,13 @@ function getDefaultActionContext( case "runner": return getRunnerContext(); - case "env": - // Actions can access env but we don't have runtime values - return new DescriptionDictionary(); + case "env": { + // Actions can access env but we don't know what env vars the calling workflow defines + // Mark as incomplete to avoid false positive "Context access might be invalid" warnings + const envContext = new DescriptionDictionary(); + envContext.complete = false; + return envContext; + } case "job": { // https://docs.github.com/en/actions/learn-github-actions/contexts#job-context @@ -218,9 +222,13 @@ function getDefaultActionContext( case "strategy": return getStrategyContext(); - case "matrix": - // Actions can access matrix context at runtime - return new DescriptionDictionary(); + case "matrix": { + // Actions can access matrix context at runtime but we don't know the calling workflow's matrix + // Mark as incomplete to avoid false positive "Context access might be invalid" warnings + const matrixContext = new DescriptionDictionary(); + matrixContext.complete = false; + return matrixContext; + } } return undefined; diff --git a/languageservice/src/utils/validate-if.ts b/languageservice/src/utils/validate-if.ts new file mode 100644 index 0000000..66cec83 --- /dev/null +++ b/languageservice/src/utils/validate-if.ts @@ -0,0 +1,65 @@ +/** + * Shared validation utilities for `if` condition literal text detection. + * Used by both workflow and action validation. + */ + +import {data} from "@actions/expressions"; +import {Expr, FunctionCall, Literal, Logical} from "@actions/expressions/ast"; + +/** + * Checks if a format function contains literal text in its format string. + * This indicates user confusion about how expressions work. + * + * Example: format('push == {0}', github.event_name) + * The literal text "push == " will always evaluate to truthy. + * + * @param expr The expression to check + * @returns true if the expression is a format() call with literal text + */ +export function hasFormatWithLiteralText(expr: Expr): boolean { + // If this is a logical AND expression (from ensureStatusFunction wrapping) + // check the right side for the format call + if (expr instanceof Logical && expr.operator.lexeme === "&&" && expr.args.length === 2) { + return hasFormatWithLiteralText(expr.args[1]); + } + + if (!(expr instanceof FunctionCall)) { + return false; + } + + // Check if this is a format function + if (expr.functionName.lexeme.toLowerCase() !== "format") { + return false; + } + + // Check if the first argument is a string literal + if (expr.args.length < 1) { + return false; + } + + const firstArg = expr.args[0]; + if (!(firstArg instanceof Literal) || firstArg.literal.kind !== data.Kind.String) { + return false; + } + + // Get the format string and trim whitespace + const formatString = firstArg.literal.coerceString(); + const trimmed = formatString.trim(); + + // Check if there's literal text (non-replacement tokens) after trimming + let inToken = false; + for (let i = 0; i < trimmed.length; i++) { + if (!inToken && trimmed[i] === "{") { + inToken = true; + } else if (inToken && trimmed[i] === "}") { + inToken = false; + } else if (inToken && trimmed[i] >= "0" && trimmed[i] <= "9") { + // OK - this is a replacement token like {0}, {1}, etc. + } else { + // Found literal text + return true; + } + } + + return false; +} diff --git a/languageservice/src/utils/validate-uses.ts b/languageservice/src/utils/validate-uses.ts new file mode 100644 index 0000000..4a6828b --- /dev/null +++ b/languageservice/src/utils/validate-uses.ts @@ -0,0 +1,118 @@ +/** + * Shared validation utilities for step `uses` field format. + * Used by both workflow and action validation. + */ + +import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types"; +import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token"; +import {mapRange} from "./range.js"; + +// Matches a short SHA (7-8 hex characters) that looks like it should be a full SHA +const SHORT_SHA_PATTERN = /^[0-9a-f]{7,8}$/i; +const SHORT_SHA_DOCS_URL = + "https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions"; + +/** + * Checks if a ref looks like a short SHA and adds a warning if so. + * Returns true if a warning was added. + */ +export function warnIfShortSha(diagnostics: Diagnostic[], token: StringToken, ref: string): boolean { + if (SHORT_SHA_PATTERN.test(ref)) { + diagnostics.push({ + message: `The provided ref '${ref}' may be a shortened commit SHA. If so, please use the full 40-character commit SHA instead, as short SHAs are not supported.`, + severity: DiagnosticSeverity.Warning, + range: mapRange(token.range), + code: "short-sha-ref", + codeDescription: { + href: SHORT_SHA_DOCS_URL + } + }); + return true; + } + return false; +} + +/** + * Validates the format of a step's `uses` field. + * + * Valid formats: + * - docker://image:tag + * - ./local/path + * - .\local\path (Windows) + * - {owner}/{repo}@{ref} + * - {owner}/{repo}/{path}@{ref} + */ +export function validateStepUsesFormat(diagnostics: Diagnostic[], token: StringToken): void { + const uses = token.value; + + // Empty uses value + if (!uses) { + diagnostics.push({ + message: "'uses' value in action cannot be blank", + severity: DiagnosticSeverity.Error, + range: mapRange(token.range), + code: "invalid-uses-format" + }); + return; + } + + // Docker image reference - always valid format + if (uses.startsWith("docker://")) { + return; + } + + // Local action path - always valid format + if (uses.startsWith("./") || uses.startsWith(".\\")) { + return; + } + + // Remote action: must be {owner}/{repo}[/path]@{ref} + const atSegments = uses.split("@"); + + // Must have exactly one @ + if (atSegments.length !== 2) { + addStepUsesFormatError(diagnostics, token); + return; + } + + const [repoPath, gitRef] = atSegments; + + // Ref cannot be empty + if (!gitRef) { + addStepUsesFormatError(diagnostics, token); + return; + } + + // Split by / or \ to get path segments + const pathSegments = repoPath.split(/[\\/]/); + + // Must have at least owner and repo (both non-empty) + if (pathSegments.length < 2 || !pathSegments[0] || !pathSegments[1]) { + addStepUsesFormatError(diagnostics, token); + return; + } + + // Check if this is a reusable workflow reference (should be at job level, not step) + // Path would be like: owner/repo/.github/workflows/file.yml + if (pathSegments.length >= 4 && pathSegments[2] === ".github" && pathSegments[3] === "workflows") { + diagnostics.push({ + message: "Reusable workflows should be referenced at the top-level `jobs..uses` key, not within steps", + severity: DiagnosticSeverity.Error, + range: mapRange(token.range), + code: "invalid-uses-format" + }); + return; + } + + // Warn if ref looks like a short SHA + warnIfShortSha(diagnostics, token, gitRef); +} + +function addStepUsesFormatError(diagnostics: Diagnostic[], token: StringToken): void { + diagnostics.push({ + message: `Expected format {owner}/{repo}[/path]@{ref}. Actual '${token.value}'`, + severity: DiagnosticSeverity.Error, + range: mapRange(token.range), + code: "invalid-uses-format" + }); +} diff --git a/languageservice/src/validate-action.test.ts b/languageservice/src/validate-action.test.ts index 495fc0b..887fb12 100644 --- a/languageservice/src/validate-action.test.ts +++ b/languageservice/src/validate-action.test.ts @@ -527,4 +527,488 @@ runs: expect(diagnostics.some(d => d.message.includes("is not valid for"))).toBe(false); }); }); + + describe("composite step uses format validation", () => { + it("validates valid uses format with version", async () => { + const doc = createActionDocument(` +name: My Action +description: Uses another action +runs: + using: composite + steps: + - uses: actions/checkout@v4 +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "invalid-uses-format")).toBe(false); + }); + + it("validates docker:// uses format", async () => { + const doc = createActionDocument(` +name: My Action +description: Uses docker image +runs: + using: composite + steps: + - uses: docker://alpine:3.14 +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "invalid-uses-format")).toBe(false); + }); + + it("validates local ./ uses format", async () => { + const doc = createActionDocument(` +name: My Action +description: Uses local action +runs: + using: composite + steps: + - uses: ./local-action +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "invalid-uses-format")).toBe(false); + }); + + it("errors on missing @ref", async () => { + const doc = createActionDocument(` +name: My Action +description: Missing version +runs: + using: composite + steps: + - uses: actions/checkout +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "invalid-uses-format")).toBe(true); + expect(diagnostics.some(d => d.message.includes("Expected format"))).toBe(true); + }); + + it("errors on invalid format", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid format +runs: + using: composite + steps: + - uses: invalid-format +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "invalid-uses-format")).toBe(true); + }); + + it("warns on short SHA", async () => { + const doc = createActionDocument(` +name: My Action +description: Short SHA +runs: + using: composite + steps: + - uses: actions/checkout@a1b2c3d +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "short-sha-ref")).toBe(true); + expect(diagnostics.some(d => d.message.includes("shortened commit SHA"))).toBe(true); + }); + + it("allows full SHA", async () => { + const doc = createActionDocument(` +name: My Action +description: Full SHA +runs: + using: composite + steps: + - uses: actions/checkout@a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2 +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "short-sha-ref")).toBe(false); + }); + + it("errors on reusable workflow in step uses", async () => { + const doc = createActionDocument(` +name: My Action +description: Wrong workflow reference +runs: + using: composite + steps: + - uses: owner/repo/.github/workflows/build.yml@main +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.message.includes("Reusable workflows should be referenced"))).toBe(true); + }); + }); + + describe("composite step if literal text validation", () => { + it("errors when literal text mixed with embedded expression", async () => { + const doc = createActionDocument(` +name: My Action +description: Literal text in if +runs: + using: composite + steps: + - if: push == \${{ github.event_name }} + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(true); + expect(diagnostics.some(d => d.message.includes("literal text outside replacement tokens"))).toBe(true); + }); + + it("allows valid expression in if", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid if expression +runs: + using: composite + steps: + - if: \${{ github.event_name == 'push' }} + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("allows if without expression markers (auto-wrapped)", async () => { + const doc = createActionDocument(` +name: My Action +description: If without markers +runs: + using: composite + steps: + - if: github.event_name == 'push' + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("allows success() function", async () => { + const doc = createActionDocument(` +name: My Action +description: Success function +runs: + using: composite + steps: + - if: success() + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("errors on format with literal text in if", async () => { + const doc = createActionDocument(` +name: My Action +description: Format with literal text +runs: + using: composite + steps: + - if: \${{ format('event is {0}', github.event_name) }} + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(true); + }); + + it("allows format with only replacement tokens", async () => { + const doc = createActionDocument(` +name: My Action +description: Format with only tokens +runs: + using: composite + steps: + - if: \${{ format('{0}', github.event_name) }} + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("validates if in uses-step", async () => { + const doc = createActionDocument(` +name: My Action +description: If in uses step +runs: + using: composite + steps: + - if: push == \${{ github.event_name }} + uses: actions/checkout@v4 +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(true); + }); + }); + + describe("pre-if and post-if validation", () => { + it("errors on explicit expression with literal text in pre-if for node action", async () => { + const doc = createActionDocument(` +name: My Action +description: Literal text in pre-if +runs: + using: node20 + main: index.js + pre: setup.js + pre-if: push == \${{ github.event_name }} +`); + const diagnostics = await validate(doc); + // Explicit ${{ }} syntax is not allowed for pre-if, so we get that error + expect(diagnostics.some(d => d.code === "explicit-expression-not-allowed")).toBe(true); + }); + + it("errors on explicit expression with literal text in post-if for node action", async () => { + const doc = createActionDocument(` +name: My Action +description: Literal text in post-if +runs: + using: node20 + main: index.js + post: cleanup.js + post-if: event == \${{ github.event_name }} +`); + const diagnostics = await validate(doc); + // Explicit ${{ }} syntax is not allowed for post-if, so we get that error + expect(diagnostics.some(d => d.code === "explicit-expression-not-allowed")).toBe(true); + }); + + it("errors on explicit expression with literal text in pre-if for docker action", async () => { + const doc = createActionDocument(` +name: My Action +description: Literal text in pre-if +runs: + using: docker + image: Dockerfile + pre-entrypoint: /setup.sh + pre-if: push == \${{ github.event_name }} +`); + const diagnostics = await validate(doc); + // Explicit ${{ }} syntax is not allowed for pre-if, so we get that error + expect(diagnostics.some(d => d.code === "explicit-expression-not-allowed")).toBe(true); + }); + + it("errors on explicit expression with literal text in post-if for docker action", async () => { + const doc = createActionDocument(` +name: My Action +description: Literal text in post-if +runs: + using: docker + image: Dockerfile + post-entrypoint: /cleanup.sh + post-if: event == \${{ github.event_name }} +`); + const diagnostics = await validate(doc); + // Explicit ${{ }} syntax is not allowed for post-if, so we get that error + expect(diagnostics.some(d => d.code === "explicit-expression-not-allowed")).toBe(true); + }); + + it("allows valid expression in pre-if for node action", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid pre-if +runs: + using: node20 + main: index.js + pre: setup.js + pre-if: success() +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("allows valid expression in post-if for node action", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid post-if +runs: + using: node20 + main: index.js + post: cleanup.js + post-if: always() +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("errors on explicit expression syntax in pre-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Explicit expression in pre-if +runs: + using: node20 + main: index.js + pre: setup.js + pre-if: \${{ runner.os == 'Windows' }} +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "explicit-expression-not-allowed")).toBe(true); + expect(diagnostics.some(d => d.message.includes("pre-if"))).toBe(true); + }); + + it("errors on explicit expression syntax in post-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Explicit expression in post-if +runs: + using: node20 + main: index.js + post: cleanup.js + post-if: \${{ always() }} +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "explicit-expression-not-allowed")).toBe(true); + expect(diagnostics.some(d => d.message.includes("post-if"))).toBe(true); + }); + + it("allows expression with failure() in post-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid post-if +runs: + using: node20 + main: index.js + post: cleanup.js + post-if: failure() +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + + it("allows expression with cancelled() in post-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid post-if +runs: + using: node20 + main: index.js + post: cleanup.js + post-if: cancelled() +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "expression-literal-text-in-condition")).toBe(false); + }); + }); + + describe("format string validation", () => { + it("errors on format() with too few arguments in composite step if", async () => { + const doc = createActionDocument(` +name: My Action +description: Format mismatch +runs: + using: composite + steps: + - if: format('{0} {1}', 'only-one') + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(true); + }); + + it("errors on invalid format string in composite step if", async () => { + const doc = createActionDocument(` +name: My Action +description: Invalid format +runs: + using: composite + steps: + - if: format('{', 'arg') + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "invalid-format-string")).toBe(true); + }); + + it("errors on format() with too few arguments in pre-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Format mismatch in pre-if +runs: + using: node20 + main: index.js + pre: setup.js + pre-if: format('{0} {1}', 'only-one') +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(true); + }); + + it("errors on format() with too few arguments in post-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Format mismatch in post-if +runs: + using: node20 + main: index.js + post: cleanup.js + post-if: format('{0} {1} {2}', 'a', 'b') +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(true); + }); + + it("allows valid format() call in composite step if", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid format +runs: + using: composite + steps: + - if: format('{0} {1}', 'a', 'b') == 'a b' + run: echo hi + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(false); + expect(diagnostics.some(d => d.code === "invalid-format-string")).toBe(false); + }); + + it("allows valid format() call in pre-if", async () => { + const doc = createActionDocument(` +name: My Action +description: Valid format in pre-if +runs: + using: node20 + main: index.js + pre: setup.js + pre-if: format('{0}', runner.os) == 'Linux' +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(false); + expect(diagnostics.some(d => d.code === "invalid-format-string")).toBe(false); + }); + + it("errors on format() with too few arguments in run expression", async () => { + const doc = createActionDocument(` +name: My Action +description: Format mismatch in run +runs: + using: composite + steps: + - run: echo \${{ format('{0} {1}', 'only-one') }} + shell: bash +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(true); + }); + + it("errors on format() with too few arguments in input default", async () => { + const doc = createActionDocument(` +name: My Action +description: Format mismatch in input default +inputs: + greeting: + description: Greeting message + default: \${{ format('{0} {1}', 'hello') }} +runs: + using: node20 + main: index.js +`); + const diagnostics = await validate(doc); + expect(diagnostics.some(d => d.code === "format-arg-count-mismatch")).toBe(true); + }); + }); }); diff --git a/languageservice/src/validate-action.ts b/languageservice/src/validate-action.ts index 4d4b2a1..d06939c 100644 --- a/languageservice/src/validate-action.ts +++ b/languageservice/src/validate-action.ts @@ -2,20 +2,30 @@ * Validation for action.yml / action.yaml manifest files */ -import {isMapping} from "@actions/workflow-parser"; +import {Lexer, Parser} from "@actions/expressions"; +import {Expr} from "@actions/expressions/ast"; +import {isBasicExpression, isMapping, isString} from "@actions/workflow-parser"; import {isActionStep} from "@actions/workflow-parser/model/type-guards"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; +import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition"; +import {splitAllowedContext} from "@actions/workflow-parser/templates/allowed-context"; +import {BasicExpressionToken} from "@actions/workflow-parser/templates/tokens/basic-expression-token"; import {MappingToken} from "@actions/workflow-parser/templates/tokens/mapping-token"; import {SequenceToken} from "@actions/workflow-parser/templates/tokens/sequence-token"; +import {StringToken} from "@actions/workflow-parser/templates/tokens/string-token"; import {TemplateToken} from "@actions/workflow-parser/templates/tokens/template-token"; +import {TokenRange} from "@actions/workflow-parser/templates/tokens/token-range"; import {TemplateValidationError} from "@actions/workflow-parser/templates/template-validation-error"; import {File} from "@actions/workflow-parser/workflows/file"; import {TextDocument} from "vscode-languageserver-textdocument"; import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types"; import {error} from "./log.js"; import {mapRange} from "./utils/range.js"; +import {hasFormatWithLiteralText} from "./utils/validate-if.js"; +import {validateStepUsesFormat} from "./utils/validate-uses.js"; import {getOrConvertActionTemplate, getOrParseAction} from "./utils/workflow-cache.js"; import {validateActionReference} from "./validate-action-reference.js"; +import {validateFormatCalls} from "./validate-format-string.js"; import {ValidationConfig} from "./validate.js"; /** @@ -114,9 +124,23 @@ export async function validateAction(textDocument: TextDocument, config?: Valida if (isActionStep(step) && isMapping(stepToken)) { await validateActionReference(diagnostics, stepToken, step, config); } + + // Validate step tokens (uses format, if conditions) + if (isMapping(stepToken)) { + validateCompositeStepTokens(diagnostics, stepToken); + } } } } + + // Validate pre-if and post-if for node and docker actions + const runsMapping = findRunsMapping(result.value); + if (runsMapping) { + validateRunsIfConditions(diagnostics, runsMapping); + } + + // Validate format() calls in all expressions throughout the action + validateAllExpressions(diagnostics, result.value); } } catch (e) { error(`Unhandled error while validating action file: ${(e as Error).message}`); @@ -125,6 +149,148 @@ export async function validateAction(textDocument: TextDocument, config?: Valida return diagnostics; } +/** + * Validates tokens within a composite action step. + * Checks `uses` format and `if` literal text detection. + */ +function validateCompositeStepTokens(diagnostics: Diagnostic[], stepToken: MappingToken): void { + for (let i = 0; i < stepToken.count; i++) { + const {key, value} = stepToken.get(i); + const keyStr = isString(key) ? key.value.toLowerCase() : ""; + + // Validate `uses` field format + if (keyStr === "uses" && isString(value)) { + validateStepUsesFormat(diagnostics, value); + } + + // Validate `if` field for literal text outside expressions + if (keyStr === "if" && value.range) { + if (isString(value)) { + // Plain string if condition (no ${{ }} markers) + validateIfCondition(diagnostics, value); + } else if (isBasicExpression(value)) { + // Expression token - check for format() with literal text + // This happens when the parser converts "push == ${{ expr }}" to format('push == {0}', expr) + validateIfConditionExpression(diagnostics, value); + } + } + } +} + +/** + * Validates an `if` condition (StringToken). + * Checks for literal text outside expressions and validates format() calls. + */ +function validateIfCondition(diagnostics: Diagnostic[], token: StringToken): void { + const condition = token.value.trim(); + if (!condition) { + return; + } + + // Get allowed context for step-if from the token's definition + const allowedContext = token.definitionInfo?.allowedContext || []; + const {namedContexts, functions} = splitAllowedContext(allowedContext); + + // Ensure the condition has a status function, wrapping if needed + const finalCondition = ensureStatusFunction(condition, token.definitionInfo); + + // Create a BasicExpressionToken for validation + const expressionToken = new BasicExpressionToken( + token.file, + token.range, + finalCondition, + token.definitionInfo, + undefined, + token.source, + undefined, + token.blockScalarHeader + ); + + // Check for literal text in the expression (format with literal text) + try { + const l = new Lexer(expressionToken.expression); + const lr = l.lex(); + const p = new Parser(lr.tokens, namedContexts, functions); + const expr = p.parse(); + + if (hasFormatWithLiteralText(expr)) { + diagnostics.push({ + message: + "Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?", + range: mapRange(token.range), + severity: DiagnosticSeverity.Error, + code: "expression-literal-text-in-condition" + }); + } + + // Validate format() function calls + validateFormatCallsAndAddDiagnostics(diagnostics, expr, token.range); + } catch { + // Ignore parse errors here - they'll be caught by schema validation + } +} + +/** + * Validates an `if` condition (BasicExpressionToken). + * Checks for literal text outside expressions. + * Called when the parser has converted "push == ${{ expr }}" to format('push == {0}', expr). + * Note: format() validation is handled by validateAllExpressions for BasicExpressionTokens. + */ +function validateIfConditionExpression(diagnostics: Diagnostic[], token: BasicExpressionToken): void { + const allowedContext = token.definitionInfo?.allowedContext || []; + const {namedContexts, functions} = splitAllowedContext(allowedContext); + + try { + const l = new Lexer(token.expression); + const lr = l.lex(); + const p = new Parser(lr.tokens, namedContexts, functions); + const expr = p.parse(); + + if (hasFormatWithLiteralText(expr)) { + diagnostics.push({ + message: + "Conditional expression contains literal text outside replacement tokens. This will cause the expression to always evaluate to truthy. Did you mean to put the entire expression inside ${{ }}?", + range: mapRange(token.range), + severity: DiagnosticSeverity.Error, + code: "expression-literal-text-in-condition" + }); + } + // Note: format() validation is done by validateAllExpressions() for all BasicExpressionTokens + } catch { + // Ignore parse errors here - they'll be caught by schema validation + } +} + +/** + * Helper to validate format() function calls and add diagnostics. + */ +function validateFormatCallsAndAddDiagnostics( + diagnostics: Diagnostic[], + expr: Expr, + range: TokenRange | undefined +): void { + const formatErrors = validateFormatCalls(expr); + for (const formatError of formatErrors) { + if (formatError.type === "invalid-syntax") { + diagnostics.push({ + message: `Invalid format string: ${formatError.message}`, + range: mapRange(range), + severity: DiagnosticSeverity.Error, + code: "invalid-format-string" + }); + } else if (formatError.type === "arg-count-mismatch") { + diagnostics.push({ + message: `Format string references argument {${formatError.expected - 1}} but only ${ + formatError.provided + } argument(s) provided`, + range: mapRange(range), + severity: DiagnosticSeverity.Error, + code: "format-arg-count-mismatch" + }); + } + } +} + /** * Find the steps sequence token from the raw action template. * Traverses the token tree looking for the "composite-steps" definition. @@ -138,6 +304,77 @@ function findStepsSequence(root: TemplateToken): SequenceToken | undefined { return undefined; } +/** + * Find the runs mapping token from the raw action template. + */ +function findRunsMapping(root: TemplateToken): MappingToken | undefined { + if (root instanceof MappingToken) { + for (let i = 0; i < root.count; i++) { + const {key, value} = root.get(i); + if (key.toString().toLowerCase() === "runs" && value instanceof MappingToken) { + return value; + } + } + } + return undefined; +} + +/** + * Validates pre-if and post-if conditions at the runs level (for node and docker actions). + * Checks for literal text outside expressions that would always be truthy. + */ +function validateRunsIfConditions(diagnostics: Diagnostic[], runsMapping: MappingToken): void { + for (let i = 0; i < runsMapping.count; i++) { + const {key, value} = runsMapping.get(i); + const keyStr = key.toString().toLowerCase(); + + // Validate pre-if and post-if fields for literal text + if ((keyStr === "pre-if" || keyStr === "post-if") && value.range) { + if (isString(value)) { + // Plain string condition (no ${{ }} markers) + validateIfCondition(diagnostics, value); + } else if (isBasicExpression(value)) { + // The runner doesn't support explicit ${{ }} syntax for pre-if/post-if + // Only implicit expressions are allowed + diagnostics.push({ + message: `Explicit expression syntax \${{ }} is not supported for '${keyStr}'. Remove the \${{ }} markers and use the expression directly.`, + range: mapRange(value.range), + severity: DiagnosticSeverity.Error, + code: "explicit-expression-not-allowed" + }); + } + } + } +} + +/** + * Validates format() function calls in all expressions throughout the action template. + * This catches format string errors in any expression, not just if conditions. + */ +function validateAllExpressions(diagnostics: Diagnostic[], root: TemplateToken): void { + for (const [, token] of TemplateToken.traverse(root)) { + if (token instanceof BasicExpressionToken) { + // Process original expressions if available (for combined expressions like "${{ a }} text ${{ b }}") + // This ensures error ranges point to the correct original expression location + for (const expression of token.originalExpressions || [token]) { + const allowedContext = expression.definitionInfo?.allowedContext || []; + const {namedContexts, functions} = splitAllowedContext(allowedContext); + + try { + const l = new Lexer(expression.expression); + const lr = l.lex(); + const p = new Parser(lr.tokens, namedContexts, functions); + const expr = p.parse(); + + validateFormatCallsAndAddDiagnostics(diagnostics, expr, expression.range); + } catch { + // Ignore parse errors - they'll be caught by schema validation + } + } + } + } +} + /** * Validates that the keys under `runs:` are valid for the specified `using:` type. * Also filters out schema errors (in place) that this validation replaces with more specific messages. diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index bd5b937..43b253e 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -1,5 +1,5 @@ -import {FeatureFlags, Lexer, Parser, data} from "@actions/expressions"; -import {Expr, FunctionCall, Literal, Logical} from "@actions/expressions/ast"; +import {FeatureFlags, Lexer, Parser} from "@actions/expressions"; +import {Expr} from "@actions/expressions/ast"; import {TemplateParseResult, WorkflowTemplate, isBasicExpression, isMapping, isString} from "@actions/workflow-parser"; import {ErrorPolicy} from "@actions/workflow-parser/model/convert"; import {getCronDescription, hasCronIntervalLessThan5Minutes} from "@actions/workflow-parser/model/converter/cron"; @@ -24,6 +24,8 @@ import {error} from "./log.js"; import {isActionDocument} from "./utils/document-type.js"; import {findToken} from "./utils/find-token.js"; import {mapRange} from "./utils/range.js"; +import {hasFormatWithLiteralText} from "./utils/validate-if.js"; +import {validateStepUsesFormat, warnIfShortSha} from "./utils/validate-uses.js"; import {getOrConvertWorkflowTemplate, getOrParseWorkflow} from "./utils/workflow-cache.js"; import {validateActionReference} from "./validate-action-reference.js"; import {validateAction} from "./validate-action.js"; @@ -285,116 +287,6 @@ function validateCronExpression(diagnostics: Diagnostic[], token: StringToken): } } -// Matches a short SHA (7-8 hex characters) that looks like it should be a full SHA -const SHORT_SHA_PATTERN = /^[0-9a-f]{7,8}$/i; -const SHORT_SHA_DOCS_URL = - "https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions"; - -/** - * Checks if a ref looks like a short SHA and adds a warning if so. - * Returns true if a warning was added. - */ -function warnIfShortSha(diagnostics: Diagnostic[], token: StringToken, ref: string): boolean { - if (SHORT_SHA_PATTERN.test(ref)) { - diagnostics.push({ - message: `The provided ref '${ref}' may be a shortened commit SHA. If so, please use the full 40-character commit SHA instead, as short SHAs are not supported.`, - severity: DiagnosticSeverity.Warning, - range: mapRange(token.range), - code: "short-sha-ref", - codeDescription: { - href: SHORT_SHA_DOCS_URL - } - }); - return true; - } - return false; -} - -/** - * Validates the format of a step's `uses` field. - * - * Valid formats: - * - docker://image:tag - * - ./local/path - * - .\local\path (Windows) - * - {owner}/{repo}@{ref} - * - {owner}/{repo}/{path}@{ref} - */ -function validateStepUsesFormat(diagnostics: Diagnostic[], token: StringToken): void { - const uses = token.value; - - // Empty uses value - if (!uses) { - diagnostics.push({ - message: "`uses' value in action cannot be blank", - severity: DiagnosticSeverity.Error, - range: mapRange(token.range), - code: "invalid-uses-format" - }); - return; - } - - // Docker image reference - always valid format - if (uses.startsWith("docker://")) { - return; - } - - // Local action path - always valid format - if (uses.startsWith("./") || uses.startsWith(".\\")) { - return; - } - - // Remote action: must be {owner}/{repo}[/path]@{ref} - const atSegments = uses.split("@"); - - // Must have exactly one @ - if (atSegments.length !== 2) { - addStepUsesFormatError(diagnostics, token); - return; - } - - const [repoPath, gitRef] = atSegments; - - // Ref cannot be empty - if (!gitRef) { - addStepUsesFormatError(diagnostics, token); - return; - } - - // Split by / or \ to get path segments - const pathSegments = repoPath.split(/[\\/]/); - - // Must have at least owner and repo (both non-empty) - if (pathSegments.length < 2 || !pathSegments[0] || !pathSegments[1]) { - addStepUsesFormatError(diagnostics, token); - return; - } - - // Check if this is a reusable workflow reference (should be at job level, not step) - // Path would be like: owner/repo/.github/workflows/file.yml - if (pathSegments.length >= 4 && pathSegments[2] === ".github" && pathSegments[3] === "workflows") { - diagnostics.push({ - message: "Reusable workflows should be referenced at the top-level `jobs..uses` key, not within steps", - severity: DiagnosticSeverity.Error, - range: mapRange(token.range), - code: "invalid-uses-format" - }); - return; - } - - // Warn if ref looks like a short SHA - warnIfShortSha(diagnostics, token, gitRef); -} - -function addStepUsesFormatError(diagnostics: Diagnostic[], token: StringToken): void { - diagnostics.push({ - message: `Expected format {owner}/{repo}[/path]@{ref}. Actual '${token.value}'`, - severity: DiagnosticSeverity.Error, - range: mapRange(token.range), - code: "invalid-uses-format" - }); -} - /** * Validates the format of a job's `uses` field (reusable workflow reference). * @@ -639,64 +531,6 @@ function getProviderContext( return getWorkflowContext(documentUri, template, path); } -/** - * Checks if a format function contains literal text in its format string. - * This indicates user confusion about how expressions work. - * - * Example: format('push == {0}', github.event_name) - * The literal text "push == " will always evaluate to truthy. - * - * @param expr The expression to check - * @returns true if the expression is a format() call with literal text - */ -function hasFormatWithLiteralText(expr: Expr): boolean { - // If this is a logical AND expression (from ensureStatusFunction wrapping) - // check the right side for the format call - if (expr instanceof Logical && expr.operator.lexeme === "&&" && expr.args.length === 2) { - return hasFormatWithLiteralText(expr.args[1]); - } - - if (!(expr instanceof FunctionCall)) { - return false; - } - - // Check if this is a format function - if (expr.functionName.lexeme.toLowerCase() !== "format") { - return false; - } - - // Check if the first argument is a string literal - if (expr.args.length < 1) { - return false; - } - - const firstArg = expr.args[0]; - if (!(firstArg instanceof Literal) || firstArg.literal.kind !== data.Kind.String) { - return false; - } - - // Get the format string and trim whitespace - const formatString = firstArg.literal.coerceString(); - const trimmed = formatString.trim(); - - // Check if there's literal text (non-replacement tokens) after trimming - let inToken = false; - for (let i = 0; i < trimmed.length; i++) { - if (!inToken && trimmed[i] === "{") { - inToken = true; - } else if (inToken && trimmed[i] === "}") { - inToken = false; - } else if (inToken && trimmed[i] >= "0" && trimmed[i] <= "9") { - // OK - this is a replacement token like {0}, {1}, etc. - } else { - // Found literal text - return true; - } - } - - return false; -} - async function validateExpression( diagnostics: Diagnostic[], token: BasicExpressionToken, diff --git a/languageservice/src/validate.uses-format.test.ts b/languageservice/src/validate.uses-format.test.ts index 248dcce..3f09dc6 100644 --- a/languageservice/src/validate.uses-format.test.ts +++ b/languageservice/src/validate.uses-format.test.ts @@ -295,7 +295,7 @@ jobs: `; const result = await validate(createDocument("wf.yaml", input)); expect(result).toContainEqual({ - message: "`uses' value in action cannot be blank", + message: "'uses' value in action cannot be blank", severity: DiagnosticSeverity.Error, range: { start: {line: 5, character: 12}, diff --git a/workflow-parser/src/action-v1.0.json b/workflow-parser/src/action-v1.0.json index 65cc4d8..368daeb 100644 --- a/workflow-parser/src/action-v1.0.json +++ b/workflow-parser/src/action-v1.0.json @@ -137,6 +137,23 @@ ], "string": {} }, + "runs-if": { + "description": "Condition to control when this action's pre or post script runs.", + "context": [ + "runner", + "github", + "job", + "strategy", + "matrix", + "env", + "inputs", + "always(0,0)", + "success(0,0)", + "failure(0,0)", + "cancelled(0,0)" + ], + "string": {} + }, "runs": { "one-of": [ "container-runs", @@ -242,7 +259,7 @@ "description": "Allows you to run a script before the entrypoint action begins.\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspre-entrypoint)" }, "pre-if": { - "type": "non-empty-string", + "type": "runs-if", "description": "Allows you to define conditions for the pre: action execution. The pre: action will only run if the conditions in pre-if are met. If not set, then pre-if defaults to always().\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspre-if)" }, "post-entrypoint": { @@ -250,7 +267,7 @@ "description": "Allows you to run a cleanup script once the runs.entrypoint action has completed.\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspost-entrypoint)" }, "post-if": { - "type": "non-empty-string", + "type": "runs-if", "description": "Allows you to define conditions for the post: action execution. The post: action will only run if the conditions in post-if are met. If not set, then post-if defaults to always().\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspost-if)" } } @@ -275,7 +292,7 @@ "description": "Allows you to run a script at the start of a job, before the main: action begins. You can use pre: to run prerequisite setup scripts.\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspre)" }, "pre-if": { - "type": "non-empty-string", + "type": "runs-if", "description": "Allows you to define conditions for the pre: action execution. The pre: action will only run if the conditions in pre-if are met. If not set, then pre-if defaults to always().\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspre-if)" }, "post": { @@ -283,7 +300,7 @@ "description": "Allows you to run a script at the end of a job, once the main: action has completed. You can use post: to run cleanup scripts.\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspost)" }, "post-if": { - "type": "non-empty-string", + "type": "runs-if", "description": "Allows you to define conditions for the post: action execution. The post: action will only run if the conditions in post-if are met. If not set, then post-if defaults to always().\n\n[Documentation](https://docs.github.com/actions/creating-actions/metadata-syntax-for-github-actions#runspost-if)" } }