From caff5ce7b63ddf374d830de869074b4878385772 Mon Sep 17 00:00:00 2001 From: Josh Gross Date: Tue, 13 Dec 2022 14:58:14 -0500 Subject: [PATCH 1/2] Validate action inputs --- actions-languageserver/src/connection.ts | 40 ++- actions-languageserver/src/value-providers.ts | 4 +- .../src/value-providers/action-inputs.ts | 63 +++-- .../src/action.test.ts | 2 +- .../src/action.ts | 10 + .../src/validate-action.ts | 82 ++++++ .../src/validate.actions.test.ts | 259 ++++++++++++++++++ actions-languageservice/src/validate.ts | 38 ++- 8 files changed, 441 insertions(+), 57 deletions(-) rename actions-languageserver/src/utils/action-reference.test.ts => actions-languageservice/src/action.test.ts (99%) rename actions-languageserver/src/utils/action-reference.ts => actions-languageservice/src/action.ts (77%) create mode 100644 actions-languageservice/src/validate-action.ts create mode 100644 actions-languageservice/src/validate.actions.test.ts diff --git a/actions-languageserver/src/connection.ts b/actions-languageserver/src/connection.ts index 627cd90..c610947 100644 --- a/actions-languageserver/src/connection.ts +++ b/actions-languageserver/src/connection.ts @@ -1,6 +1,7 @@ -import { hover } from "@github/actions-languageservice/hover"; -import { registerLogger, setLogLevel } from "@github/actions-languageservice/log"; -import { validate } from "@github/actions-languageservice/validate"; +import {hover} from "@github/actions-languageservice/hover"; +import {registerLogger, setLogLevel} from "@github/actions-languageservice/log"; +import {validate, ValidationConfig} from "@github/actions-languageservice/validate"; +import {Octokit} from "@octokit/rest"; import { CompletionItem, Connection, @@ -12,12 +13,13 @@ import { TextDocuments, TextDocumentSyncKind } from "vscode-languageserver"; -import { TextDocument } from "vscode-languageserver-textdocument"; -import { contextProviders } from "./context-providers"; -import { InitializationOptions, RepositoryContext } from "./initializationOptions"; -import { onCompletion } from "./on-completion"; -import { TTLCache } from "./utils/cache"; -import { valueProviders } from "./value-providers"; +import {TextDocument} from "vscode-languageserver-textdocument"; +import {contextProviders} from "./context-providers"; +import {InitializationOptions, RepositoryContext} from "./initializationOptions"; +import {onCompletion} from "./on-completion"; +import {TTLCache} from "./utils/cache"; +import {valueProviders} from "./value-providers"; +import {getActionInputs} from "./value-providers/action-inputs"; export function initConnection(connection: Connection) { const documents: TextDocuments = new TextDocuments(TextDocument); @@ -82,11 +84,21 @@ export function initConnection(connection: Connection) { async function validateTextDocument(textDocument: TextDocument): Promise { const repoContext = repos.find(repo => textDocument.uri.startsWith(repo.workspaceUri)); - const result = await validate( - textDocument, - valueProviders(sessionToken, repoContext, cache), - contextProviders(sessionToken, repoContext, cache) - ); + + const config: ValidationConfig = { + valueProviderConfig: valueProviders(sessionToken, repoContext, cache), + contextProviderConfig: contextProviders(sessionToken, repoContext, cache), + getActionInputs: async action => { + if (sessionToken) { + const octokit = new Octokit({ + auth: sessionToken + }); + return await getActionInputs(octokit, cache, action); + } + return undefined; + } + }; + const result = await validate(textDocument, config); connection.sendDiagnostics({uri: textDocument.uri, diagnostics: result}); } diff --git a/actions-languageserver/src/value-providers.ts b/actions-languageserver/src/value-providers.ts index 3ac95d5..d5e0eba 100644 --- a/actions-languageserver/src/value-providers.ts +++ b/actions-languageserver/src/value-providers.ts @@ -4,7 +4,7 @@ import {ValueProviderKind} from "@github/actions-languageservice/value-providers import {Octokit} from "@octokit/rest"; import {RepositoryContext} from "./initializationOptions"; import {TTLCache} from "./utils/cache"; -import {getActionInputs} from "./value-providers/action-inputs"; +import {getActionInputValues} from "./value-providers/action-inputs"; import {getEnvironments} from "./value-providers/job-environment"; import {getRunnerLabels} from "./value-providers/runs-on"; @@ -36,7 +36,7 @@ export function valueProviders( }, "step-with": { kind: ValueProviderKind.AllowedValues, - get: (context: WorkflowContext) => getActionInputs(octokit, cache, context) + get: (context: WorkflowContext) => getActionInputValues(octokit, cache, context) } }; } diff --git a/actions-languageserver/src/value-providers/action-inputs.ts b/actions-languageserver/src/value-providers/action-inputs.ts index 4d0af91..61ff70a 100644 --- a/actions-languageserver/src/value-providers/action-inputs.ts +++ b/actions-languageserver/src/value-providers/action-inputs.ts @@ -1,12 +1,33 @@ +import { + actionIdentifier, + ActionInputs, + ActionReference, + parseActionReference +} from "@github/actions-languageservice/action"; import {WorkflowContext} from "@github/actions-languageservice/context/workflow-context"; import {Value} from "@github/actions-languageservice/value-providers/config"; import {isActionStep} from "@github/actions-workflow-parser/model/type-guards"; import {Octokit, RestEndpointMethodTypes} from "@octokit/rest"; import {parse} from "yaml"; -import {actionIdentifier, ActionReference, parseActionReference} from "../utils/action-reference"; import {TTLCache} from "../utils/cache"; -export async function getActionInputs(client: Octokit, cache: TTLCache, context: WorkflowContext): Promise { +export async function getActionInputs( + client: Octokit, + cache: TTLCache, + action: ActionReference +): Promise { + const inputs = await cache.get(`${actionIdentifier(action)}/action-inputs`, undefined, () => + fetchActionInputs(client, action) + ); + + return inputs; +} + +export async function getActionInputValues( + client: Octokit, + cache: TTLCache, + context: WorkflowContext +): Promise { if (!context.step || !isActionStep(context.step)) { return []; } @@ -15,18 +36,23 @@ export async function getActionInputs(client: Octokit, cache: TTLCache, context: if (!action) { return []; } + const inputs = await getActionInputs(client, cache, action); + if (!inputs) { + return []; + } - const inputs = await cache.get(`${actionIdentifier(action)}/action-inputs`, undefined, () => - fetchActionInputs(client, action) - ); - - return inputs; + return Object.entries(inputs).map(([inputName, input]) => { + return { + label: inputName, + description: input.description + }; + }); } -async function fetchActionInputs(client: Octokit, action: ActionReference): Promise { +async function fetchActionInputs(client: Octokit, action: ActionReference): Promise { const metadata = await getActionMetadata(client, action); if (!metadata) { - return []; + return undefined; } return parseActionMetadata(metadata); @@ -71,7 +97,7 @@ async function getActionMetadata(client: Octokit, action: ActionReference): Prom } type ActionMetadata = { - inputs?: Record; + inputs?: ActionInputs; }; // https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#inputs @@ -83,20 +109,7 @@ type ActionInput = { }; // https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions -async function parseActionMetadata(content: string): Promise { - const inputs = new Array(); - +async function parseActionMetadata(content: string): Promise { const metadata: ActionMetadata = parse(content); - if (metadata.inputs === undefined) { - return inputs; - } - - for (const [name, input] of Object.entries(metadata.inputs)) { - inputs.push({ - label: name, - description: input.description - }); - } - - return inputs; + return metadata.inputs ?? {}; } diff --git a/actions-languageserver/src/utils/action-reference.test.ts b/actions-languageservice/src/action.test.ts similarity index 99% rename from actions-languageserver/src/utils/action-reference.test.ts rename to actions-languageservice/src/action.test.ts index eff9ef7..f06de9b 100644 --- a/actions-languageserver/src/utils/action-reference.test.ts +++ b/actions-languageservice/src/action.test.ts @@ -1,4 +1,4 @@ -import {actionIdentifier, parseActionReference as parse} from "./action-reference"; +import {actionIdentifier, parseActionReference as parse} from "./action"; describe("parseActionReference", () => { it("basic action", () => { diff --git a/actions-languageserver/src/utils/action-reference.ts b/actions-languageservice/src/action.ts similarity index 77% rename from actions-languageserver/src/utils/action-reference.ts rename to actions-languageservice/src/action.ts index 408984b..2f92b5d 100644 --- a/actions-languageserver/src/utils/action-reference.ts +++ b/actions-languageservice/src/action.ts @@ -1,3 +1,13 @@ +// https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#inputs +export type ActionInput = { + description: string; + required?: boolean; + default?: string; + deprecationMessage?: string; +}; + +export type ActionInputs = Record; + export type ActionReference = { owner: string; name: string; diff --git a/actions-languageservice/src/validate-action.ts b/actions-languageservice/src/validate-action.ts new file mode 100644 index 0000000..dbd404a --- /dev/null +++ b/actions-languageservice/src/validate-action.ts @@ -0,0 +1,82 @@ +import {isMapping} from "@github/actions-workflow-parser"; +import {isActionStep} from "@github/actions-workflow-parser/model/type-guards"; +import {Step} from "@github/actions-workflow-parser/model/workflow-template"; +import {ScalarToken} from "@github/actions-workflow-parser/templates/tokens/scalar-token"; +import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/template-token"; +import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types"; +import {parseActionReference} from "./action"; +import {mapRange} from "./utils/range"; +import {ValidationConfig} from "./validate"; + +export async function validateAction( + diagnostics: Diagnostic[], + stepToken: TemplateToken, + step: Step | undefined, + config: ValidationConfig | undefined +): Promise { + if (!isMapping(stepToken) || !step || !isActionStep(step) || !config?.getActionInputs) { + return; + } + + const action = parseActionReference(step.uses.value); + if (!action) { + return; + } + + const actionInputs = await config.getActionInputs(action); + if (actionInputs === undefined) { + return; + } + + let withKey: ScalarToken | undefined; + let withToken: TemplateToken | undefined; + for (const {key, value} of stepToken) { + if (key.toString() === "with") { + withKey = key; + withToken = value; + break; + } + } + + const stepInputs = new Map(); + if (withToken && isMapping(withToken)) { + for (const {key} of withToken) { + stepInputs.set(key.toString(), key); + } + } + + for (const [input, inputToken] of stepInputs) { + if (!actionInputs[input]) { + diagnostics.push({ + severity: DiagnosticSeverity.Error, + range: mapRange(inputToken.range), + message: `Invalid action input '${input}'` + }); + } + + const deprecationMessage = actionInputs[input]?.deprecationMessage; + if (deprecationMessage) { + diagnostics.push({ + severity: DiagnosticSeverity.Warning, + range: mapRange(inputToken.range), + message: deprecationMessage + }); + } + } + + const missingRequiredInputs = Object.entries(actionInputs).filter( + ([inputName, input]) => input.required && !stepInputs.has(inputName) + ); + + if (missingRequiredInputs.length > 0) { + const message = + missingRequiredInputs.length === 1 + ? `Missing required input \`${missingRequiredInputs[0][0]}\`` + : `Missing required inputs: ${missingRequiredInputs.map(input => `\`${input[0]}\``).join(", ")}`; + diagnostics.push({ + severity: DiagnosticSeverity.Error, + range: mapRange((withKey || stepToken).range), // Highlight the whole step if we don't have a with key + message: message + }); + } +} diff --git a/actions-languageservice/src/validate.actions.test.ts b/actions-languageservice/src/validate.actions.test.ts new file mode 100644 index 0000000..9b499bf --- /dev/null +++ b/actions-languageservice/src/validate.actions.test.ts @@ -0,0 +1,259 @@ +import {Diagnostic, DiagnosticSeverity} from "vscode-languageserver-types"; +import {ActionInput, ActionReference} from "./action"; +import {registerLogger} from "./log"; +import {createDocument} from "./test-utils/document"; +import {TestLogger} from "./test-utils/logger"; +import {validate, ValidationConfig} from "./validate"; +import {ValueProviderKind} from "./value-providers/config"; + +registerLogger(new TestLogger()); + +const validationConfig: ValidationConfig = { + getActionInputs: async (ref: ActionReference) => { + let inputs: Record = {}; + switch (ref.owner + "/" + ref.name + "@" + ref.ref) { + case "actions/checkout@v3": + inputs = { + repository: { + description: "Repository name with owner", + default: "${{ github.repository }}" + } + }; + break; + case "actions/setup-node@v1": + inputs = { + version: { + description: "Deprecated. Use node-version instead. Will not be supported after October 1, 2019", + deprecationMessage: + "The version property will not be supported after October 1, 2019. Use node-version instead" + } + }; + break; + case "actions/cache@v1": + inputs = { + path: { + description: "A directory to store and save the cache", + required: true + }, + key: { + description: "An explicit key for restoring and saving the cache", + required: true + }, + "restore-keys": { + description: "An ordered list of keys to use for restoring the cache if no cache hit occurred for key", + required: false + } + }; + break; + } + return inputs; + } +}; +describe("validate action steps", () => { + it("valid action reference", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 +`; + const result = await validate(createDocument("wf.yaml", input), validationConfig); + + expect(result).toEqual([]); + }); + + it("invalid input", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + notanoption: true +`; + const result = await validate(createDocument("wf.yaml", input), validationConfig); + + expect(result).toEqual([ + { + message: "Invalid action input 'notanoption'", + range: { + end: { + character: 19, + line: 8 + }, + start: { + character: 8, + line: 8 + } + }, + severity: DiagnosticSeverity.Error + } + ]); + }); + + it("deprecated input", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/setup-node@v1 + with: + version: 10 +`; + const result = await validate(createDocument("wf.yaml", input), validationConfig); + + expect(result).toEqual([ + { + message: "The version property will not be supported after October 1, 2019. Use node-version instead", + range: { + end: { + character: 15, + line: 8 + }, + start: { + character: 8, + line: 8 + } + }, + severity: DiagnosticSeverity.Warning + } + ]); + }); + + it("missing required input", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/cache@v1 + with: + key: \${{ runner.os }}-node-\${{ hashFiles('**/package-lock.json') }} +`; + const result = await validate(createDocument("wf.yaml", input), validationConfig); + + expect(result).toEqual([ + { + message: "Missing required input `path`", + range: { + end: { + character: 10, + line: 7 + }, + start: { + character: 6, + line: 7 + } + }, + severity: DiagnosticSeverity.Error + } + ]); + }); + + it("multiple missing required inputs", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/cache@v1 + with: + restore-keys: \${{ runner.os }}-node- +`; + const result = await validate(createDocument("wf.yaml", input), validationConfig); + + expect(result).toEqual([ + { + message: "Missing required inputs: `path`, `key`", + range: { + end: { + character: 10, + line: 7 + }, + start: { + character: 6, + line: 7 + } + }, + severity: DiagnosticSeverity.Error + } + ]); + }); + + it("missing required inputs without a `with` key", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/cache@v1 +`; + const result = await validate(createDocument("wf.yaml", input), validationConfig); + + expect(result).toEqual([ + { + message: "Missing required inputs: `path`, `key`", + range: { + end: { + character: 0, + line: 7 + }, + start: { + character: 6, + line: 6 + } + }, + severity: DiagnosticSeverity.Error + } + ]); + }); + + it("skips extra validation from custom value provider", async () => { + const input = ` +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + with: + notanoption: true +`; + const config = validationConfig; + config.valueProviderConfig = { + "step-with": { + kind: ValueProviderKind.AllowedValues, + get: async () => { + return [{label: "repository", description: "Repository name with owner."}]; + } + } + }; + const result = await validate(createDocument("wf.yaml", input), config); + + expect(result).toEqual([ + { + message: "Invalid action input 'notanoption'", + range: { + end: { + character: 19, + line: 8 + }, + start: { + character: 8, + line: 8 + } + }, + severity: DiagnosticSeverity.Error + } + ]); + }); +}); diff --git a/actions-languageservice/src/validate.ts b/actions-languageservice/src/validate.ts index d913a26..8d2a0b4 100644 --- a/actions-languageservice/src/validate.ts +++ b/actions-languageservice/src/validate.ts @@ -16,6 +16,7 @@ import {TemplateToken} from "@github/actions-workflow-parser/templates/tokens/te import {File} from "@github/actions-workflow-parser/workflows/file"; import {TextDocument} from "vscode-languageserver-textdocument"; import {Diagnostic, DiagnosticSeverity, URI} from "vscode-languageserver-types"; +import {ActionInputs, ActionReference} from "./action"; import {ContextProviderConfig} from "./context-providers/config"; import {getContext, Mode} from "./context-providers/default"; @@ -26,9 +27,16 @@ import {nullTrace} from "./nulltrace"; import {getAllowedContext} from "./utils/allowed-context"; import {findToken} from "./utils/find-token"; import {mapRange} from "./utils/range"; +import {validateAction} from "./validate-action"; import {ValueProviderConfig, ValueProviderKind} from "./value-providers/config"; import {defaultValueProviders} from "./value-providers/default"; +export type ValidationConfig = { + valueProviderConfig?: ValueProviderConfig; + contextProviderConfig?: ContextProviderConfig; + getActionInputs?(action: ActionReference): Promise; +}; + /** * Validates a workflow file * @@ -37,9 +45,8 @@ import {defaultValueProviders} from "./value-providers/default"; */ export async function validate( textDocument: TextDocument, + config?: ValidationConfig // TODO: Support multiple files, context for API calls - valueProviderConfig?: ValueProviderConfig, - contextProviderConfig?: ContextProviderConfig ): Promise { const file: File = { name: textDocument.uri, @@ -55,14 +62,7 @@ export async function validate( const template = convertWorkflowTemplate(result.context, result.value, ErrorPolicy.TryConversion); // Validate expressions and value providers - await additionalValidations( - diagnostics, - textDocument.uri, - template, - result.value, - valueProviderConfig, - contextProviderConfig - ); + await additionalValidations(diagnostics, textDocument.uri, template, result.value, config); } // For now map parser errors directly to diagnostics @@ -86,8 +86,7 @@ async function additionalValidations( documentUri: URI, template: WorkflowTemplate, root: TemplateToken, - valueProviderConfig: ValueProviderConfig | undefined, - contextProviderConfig: ContextProviderConfig | undefined + config?: ValidationConfig ) { for (const [parent, token, key] of TemplateToken.traverse(root)) { // If the token is a value in a pair, use the key definition for validation @@ -103,18 +102,27 @@ async function additionalValidations( diagnostics, token, allowedContext, - contextProviderConfig, + config?.contextProviderConfig, getProviderContext(documentUri, template, root, token) ); } + if (token.definition?.key === "regular-step") { + const context = getProviderContext(documentUri, template, root, token); + await validateAction(diagnostics, token, context.step, config); + } + // Allowed values coming from the schema have already been validated. Only check if // a value provider is defined for a token and if it is, validate the values match. - if (valueProviderConfig && token.range && validationDefinition) { + if (config?.valueProviderConfig && token.range && validationDefinition) { const defKey = validationDefinition.key; + if (defKey === "step-with") { + // Action inputs should be validated already in validateAction + continue; + } // Try a custom value provider first - let valueProvider = valueProviderConfig[defKey]; + let valueProvider = config.valueProviderConfig[defKey]; if (!valueProvider) { // fall back to default valueProvider = defaultValueProviders[defKey]; From de7e919d884906dfdf2cb95d59cb79d3c40f954e Mon Sep 17 00:00:00 2001 From: Josh Gross Date: Tue, 13 Dec 2022 15:14:57 -0500 Subject: [PATCH 2/2] Fix validation config in tests --- actions-languageservice/src/validate.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actions-languageservice/src/validate.test.ts b/actions-languageservice/src/validate.test.ts index 44beb3a..eaaa38e 100644 --- a/actions-languageservice/src/validate.test.ts +++ b/actions-languageservice/src/validate.test.ts @@ -70,7 +70,7 @@ jobs: steps: - run: echo` ), - defaultValueProviders + {valueProviderConfig: defaultValueProviders} ); expect(result.length).toBe(1); @@ -103,7 +103,7 @@ jobs: steps: - run: echo` ), - defaultValueProviders + {valueProviderConfig: defaultValueProviders} ); expect(result.length).toBe(1); @@ -139,7 +139,7 @@ jobs: steps: - run: echo` ), - defaultValueProviders + {valueProviderConfig: defaultValueProviders} ); expect(result[0]).toEqual({ @@ -168,7 +168,7 @@ jobs: runs-on: - ubuntu-latest` ), - defaultValueProviders + {valueProviderConfig: defaultValueProviders} ); expect(result.length).toBe(1);