From c2eb0f630d7959e7c21b220130a11fa956149e6b Mon Sep 17 00:00:00 2001 From: Laura Yu <60276246+lauraway@users.noreply.github.com> Date: Mon, 13 Feb 2023 17:29:55 -0800 Subject: [PATCH 1/2] Skip validating `secrets` and `vars` when user does not have permission (#108) --- .../src/completion/descriptionDictionary.ts | 1 + .../src/context-providers/secrets.ts | 101 +++++++++--------- .../src/context-providers/variables.ts | 97 ++++++++--------- .../expression-validation/error-dictionary.ts | 10 +- .../src/validate.expressions.test.ts | 56 +++++++++- 5 files changed, 163 insertions(+), 102 deletions(-) diff --git a/actions-expressions/src/completion/descriptionDictionary.ts b/actions-expressions/src/completion/descriptionDictionary.ts index 0fe6892..2aaa3dd 100644 --- a/actions-expressions/src/completion/descriptionDictionary.ts +++ b/actions-expressions/src/completion/descriptionDictionary.ts @@ -9,6 +9,7 @@ export function isDescriptionDictionary(x: ExpressionData): x is DescriptionDict export class DescriptionDictionary extends Dictionary { private readonly descriptions = new Map(); + public complete: boolean = true; constructor(...pairs: DescriptionPair[]) { super(); diff --git a/actions-languageserver/src/context-providers/secrets.ts b/actions-languageserver/src/context-providers/secrets.ts index 61c6ca0..51372ec 100644 --- a/actions-languageserver/src/context-providers/secrets.ts +++ b/actions-languageserver/src/context-providers/secrets.ts @@ -29,51 +29,55 @@ export async function getSecrets( } } - const secrets = await getRemoteSecrets(octokit, cache, repo, environmentName); - - // Build combined map of secrets - const secretsMap = new Map< - string, - { - key: string; - value: data.StringData; - description?: string; - } - >(); - - secrets.orgSecrets.forEach(secret => - secretsMap.set(secret.value.toLowerCase(), { - key: secret.value, - value: new data.StringData("***"), - description: "Organization secret" - }) - ); - - // Override org secrets with repo secrets - secrets.repoSecrets.forEach(secret => - secretsMap.set(secret.value.toLowerCase(), { - key: secret.value, - value: new data.StringData("***"), - description: "Repository secret" - }) - ); - - // Override repo secrets with environment secrets (if defined) - secrets.environmentSecrets.forEach(secret => - secretsMap.set(secret.value.toLowerCase(), { - key: secret.value, - value: new data.StringData("***"), - description: `Secret for environment \`${environmentName}\`` - }) - ); - const secretsContext = defaultContext || new DescriptionDictionary(); + try { + const secrets = await getRemoteSecrets(octokit, cache, repo, environmentName); - // Sort secrets by key and add to context - Array.from(secretsMap.values()) - .sort((a, b) => a.key.localeCompare(b.key)) - .forEach(secret => secretsContext?.add(secret.key, secret.value, secret.description)); + // Build combined map of secrets + const secretsMap = new Map< + string, + { + key: string; + value: data.StringData; + description?: string; + } + >(); + secrets.orgSecrets.forEach(secret => + secretsMap.set(secret.value.toLowerCase(), { + key: secret.value, + value: new data.StringData("***"), + description: "Organization secret" + }) + ); + + // Override org secrets with repo secrets + secrets.repoSecrets.forEach(secret => + secretsMap.set(secret.value.toLowerCase(), { + key: secret.value, + value: new data.StringData("***"), + description: "Repository secret" + }) + ); + + // Override repo secrets with environment secrets (if defined) + secrets.environmentSecrets.forEach(secret => + secretsMap.set(secret.value.toLowerCase(), { + key: secret.value, + value: new data.StringData("***"), + description: `Secret for environment \`${environmentName}\`` + }) + ); + + // Sort secrets by key and add to context + Array.from(secretsMap.values()) + .sort((a, b) => a.key.localeCompare(b.key)) + .forEach(secret => secretsContext?.add(secret.key, secret.value, secret.description)); + } catch (e: any) { + if (e.status === 403 || e.status === 404) { + secretsContext.complete = false; + } + } return secretsContext; } @@ -88,9 +92,7 @@ async function getRemoteSecrets( orgSecrets: StringData[]; }> { return { - repoSecrets: await cache.get(`${repo.owner}/${repo.name}/secrets`, undefined, () => - fetchSecrets(octokit, repo.owner, repo.name) - ), + repoSecrets: await fetchSecrets(octokit, repo.owner, repo.name), environmentSecrets: (environmentName && (await cache.get(`${repo.owner}/${repo.name}/secrets/environment/${environmentName}`, undefined, () => @@ -114,9 +116,8 @@ async function fetchSecrets(octokit: Octokit, owner: string, name: string): Prom ); } catch (e) { console.log("Failure to retrieve secrets: ", e); + throw e; } - - return []; } async function fetchEnvironmentSecrets( @@ -136,9 +137,8 @@ async function fetchEnvironmentSecrets( ); } catch (e) { console.log("Failure to retrieve environment secrets: ", e); + throw e; } - - return []; } async function fetchOrganizationSecrets(octokit: Octokit, repo: RepositoryContext): Promise { @@ -155,7 +155,6 @@ async function fetchOrganizationSecrets(octokit: Octokit, repo: RepositoryContex return secrets.map(secret => new StringData(secret.name)); } catch (e) { console.log("Failure to retrieve organization secrets: ", e); + throw e; } - - return []; } diff --git a/actions-languageserver/src/context-providers/variables.ts b/actions-languageserver/src/context-providers/variables.ts index 52babbd..f78c650 100644 --- a/actions-languageserver/src/context-providers/variables.ts +++ b/actions-languageserver/src/context-providers/variables.ts @@ -30,51 +30,55 @@ export async function getVariables( } } - const variables = await getRemoteVariables(octokit, cache, repo, environmentName); - - // Build combined map of variables - const variablesMap = new Map< - string, - { - key: string; - value: data.StringData; - description?: string; - } - >(); - - variables.organizationVariables.forEach(variable => - variablesMap.set(variable.key.toLowerCase(), { - key: variable.key, - value: new data.StringData(variable.value.coerceString()), - description: `${variable.value.coerceString()} - Organization variable` - }) - ); - - // Override org variables with repo variables - variables.repoVariables.forEach(variable => - variablesMap.set(variable.key.toLowerCase(), { - key: variable.key, - value: new data.StringData(variable.value.coerceString()), - description: `${variable.value.coerceString()} - Repository variable` - }) - ); - - // Override repo variables with environment veriables (if defined) - variables.environmentVariables.forEach(variable => - variablesMap.set(variable.key.toLowerCase(), { - key: variable.key, - value: new data.StringData(variable.value.coerceString()), - description: `${variable.value.coerceString()} - Variable for environment \`${environmentName}\`` - }) - ); - const variablesContext = defaultContext || new DescriptionDictionary(); + try { + const variables = await getRemoteVariables(octokit, cache, repo, environmentName); - // Sort variables by key and add to context - Array.from(variablesMap.values()) - .sort((a, b) => a.key.localeCompare(b.key)) - .forEach(variable => variablesContext?.add(variable.key, variable.value, variable.description)); + // Build combined map of variables + const variablesMap = new Map< + string, + { + key: string; + value: data.StringData; + description?: string; + } + >(); + variables.organizationVariables.forEach(variable => + variablesMap.set(variable.key.toLowerCase(), { + key: variable.key, + value: new data.StringData(variable.value.coerceString()), + description: `${variable.value.coerceString()} - Organization variable` + }) + ); + + // Override org variables with repo variables + variables.repoVariables.forEach(variable => + variablesMap.set(variable.key.toLowerCase(), { + key: variable.key, + value: new data.StringData(variable.value.coerceString()), + description: `${variable.value.coerceString()} - Repository variable` + }) + ); + + // Override repo variables with environment veriables (if defined) + variables.environmentVariables.forEach(variable => + variablesMap.set(variable.key.toLowerCase(), { + key: variable.key, + value: new data.StringData(variable.value.coerceString()), + description: `${variable.value.coerceString()} - Variable for environment \`${environmentName}\`` + }) + ); + + // Sort variables by key and add to context + Array.from(variablesMap.values()) + .sort((a, b) => a.key.localeCompare(b.key)) + .forEach(variable => variablesContext?.add(variable.key, variable.value, variable.description)); + } catch (e: any) { + if (e.status === 403 || e.status === 404) { + variablesContext.complete = false; + } + } return variablesContext; } @@ -121,9 +125,8 @@ async function fetchVariables(octokit: Octokit, owner: string, name: string): Pr ); } catch (e) { console.log("Failure to retrieve variables: ", e); + throw e; } - - return []; } async function fetchEnvironmentVariables( @@ -146,9 +149,8 @@ async function fetchEnvironmentVariables( ); } catch (e) { console.log("Failure to retrieve environment variables: ", e); + throw e; } - - return []; } async function fetchOrganizationVariables(octokit: Octokit, repo: RepositoryContext): Promise { @@ -170,7 +172,6 @@ async function fetchOrganizationVariables(octokit: Octokit, repo: RepositoryCont }); } catch (e) { console.log("Failure to retrieve organization variables: ", e); + throw e; } - - return []; } diff --git a/actions-languageservice/src/expression-validation/error-dictionary.ts b/actions-languageservice/src/expression-validation/error-dictionary.ts index df96c42..afe4917 100644 --- a/actions-languageservice/src/expression-validation/error-dictionary.ts +++ b/actions-languageservice/src/expression-validation/error-dictionary.ts @@ -1,4 +1,4 @@ -import {data} from "@github/actions-expressions"; +import {data, isDescriptionDictionary} from "@github/actions-expressions"; import {isDictionary} from "@github/actions-expressions/data/dictionary"; import {ExpressionData, Pair} from "@github/actions-expressions/data/expressiondata"; @@ -12,6 +12,7 @@ export class ErrorDictionary extends data.Dictionary { constructor(...pairs: Pair[]) { super(...pairs); } + public complete: boolean = true; get(key: string): ExpressionData | undefined { const value = super.get(key); @@ -19,12 +20,17 @@ export class ErrorDictionary extends data.Dictionary { return value; } - throw new AccessError(`Invalid context access: ${key}`, key); + if (this.complete) { + throw new AccessError(`Invalid context access: ${key}`, key); + } } } export function wrapDictionary(d: data.Dictionary): ErrorDictionary { const e = new ErrorDictionary(); + if (isDescriptionDictionary(d)) { + e.complete = d.complete; + } for (const {key, value} of d.pairs()) { if (isDictionary(value)) { diff --git a/actions-languageservice/src/validate.expressions.test.ts b/actions-languageservice/src/validate.expressions.test.ts index df2b245..3a5f54c 100644 --- a/actions-languageservice/src/validate.expressions.test.ts +++ b/actions-languageservice/src/validate.expressions.test.ts @@ -1,8 +1,10 @@ +import {data, DescriptionDictionary} from "@github/actions-expressions/."; import {DiagnosticSeverity} from "vscode-languageserver-types"; +import {ContextProviderConfig} from "./context-providers/config"; import {registerLogger} from "./log"; import {createDocument} from "./test-utils/document"; import {TestLogger} from "./test-utils/logger"; -import {validate} from "./validate"; +import {validate, ValidationConfig} from "./validate"; registerLogger(new TestLogger()); @@ -39,6 +41,58 @@ jobs: ]); }); + it("partial skip access invalid context on incomplete", async () => { + const contextProviderConfig: ContextProviderConfig = { + getContext: async (context: string) => { + switch (context) { + case "secrets": + const dict = new DescriptionDictionary(); + dict.complete = false; + return dict; + } + + return undefined; + } + }; + + const validationConfig: ValidationConfig = { + contextProviderConfig: contextProviderConfig + }; + + const result = await validate( + createDocument( + "wf.yaml", + `on: push +run-name: name-\${{ github.does-not-exist }} +env: + secret: \${{ secrets.secret-not-exist }} +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo` + ), + validationConfig + ); + + expect(result).toEqual([ + { + message: "Context access might be invalid: does-not-exist", + range: { + end: { + character: 43, + line: 1 + }, + start: { + character: 15, + line: 1 + } + }, + severity: DiagnosticSeverity.Warning + } + ]); + }); + it("access invalid nested context field", async () => { const result = await validate( createDocument( From e5e52aecf3b2ce7f6914dc0cc7f226d0058bfd74 Mon Sep 17 00:00:00 2001 From: github-actions Date: Tue, 14 Feb 2023 01:30:33 +0000 Subject: [PATCH 2/2] v0.1.138 --- actions-expressions/package.json | 2 +- actions-languageserver/package.json | 6 ++--- actions-languageservice/package.json | 6 ++--- actions-workflow-parser/package.json | 4 ++-- browser-playground/package.json | 4 ++-- lerna.json | 2 +- package-lock.json | 34 ++++++++++++++-------------- 7 files changed, 29 insertions(+), 29 deletions(-) diff --git a/actions-expressions/package.json b/actions-expressions/package.json index 643e2d5..a42a88c 100755 --- a/actions-expressions/package.json +++ b/actions-expressions/package.json @@ -1,6 +1,6 @@ { "name": "@github/actions-expressions", - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "type": "module", "source": "./src/index.ts", diff --git a/actions-languageserver/package.json b/actions-languageserver/package.json index ba927af..e4aaf32 100644 --- a/actions-languageserver/package.json +++ b/actions-languageserver/package.json @@ -1,6 +1,6 @@ { "name": "@github/actions-languageserver", - "version": "0.1.137", + "version": "0.1.138", "description": "Language server for GitHub Actions", "license": "MIT", "type": "module", @@ -38,8 +38,8 @@ "watch": "tsc --build tsconfig.build.json --watch" }, "dependencies": { - "@github/actions-languageservice": "^0.1.137", - "@github/actions-workflow-parser": "^0.1.137", + "@github/actions-languageservice": "^0.1.138", + "@github/actions-workflow-parser": "^0.1.138", "@octokit/rest": "^19.0.7", "vscode-languageserver": "^8.0.2", "vscode-languageserver-textdocument": "^1.0.7", diff --git a/actions-languageservice/package.json b/actions-languageservice/package.json index b38ea88..3e92b9f 100644 --- a/actions-languageservice/package.json +++ b/actions-languageservice/package.json @@ -1,6 +1,6 @@ { "name": "@github/actions-languageservice", - "version": "0.1.137", + "version": "0.1.138", "description": "Language service for GitHub Actions", "license": "MIT", "type": "module", @@ -38,8 +38,8 @@ "watch": "tsc --build tsconfig.build.json --watch" }, "dependencies": { - "@github/actions-expressions": "^0.1.137", - "@github/actions-workflow-parser": "^0.1.137", + "@github/actions-expressions": "^0.1.138", + "@github/actions-workflow-parser": "^0.1.138", "vscode-languageserver-textdocument": "^1.0.7", "vscode-languageserver-types": "^3.17.2", "yaml": "^2.1.1" diff --git a/actions-workflow-parser/package.json b/actions-workflow-parser/package.json index 1be7cbe..910367a 100644 --- a/actions-workflow-parser/package.json +++ b/actions-workflow-parser/package.json @@ -1,6 +1,6 @@ { "name": "@github/actions-workflow-parser", - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "type": "module", "source": "./src/index.ts", @@ -40,7 +40,7 @@ "watch": "tsc --build tsconfig.build.json --watch" }, "dependencies": { - "@github/actions-expressions": "^0.1.137", + "@github/actions-expressions": "^0.1.138", "cronstrue": "^2.21.0", "yaml": "^2.0.0-8" }, diff --git a/browser-playground/package.json b/browser-playground/package.json index 7a08d9e..3ea9c36 100644 --- a/browser-playground/package.json +++ b/browser-playground/package.json @@ -1,12 +1,12 @@ { "name": "browser-playground", - "version": "0.1.137", + "version": "0.1.138", "description": "", "private": true, "main": "index.js", "type": "module", "dependencies": { - "@github/actions-languageserver": "^0.1.137", + "@github/actions-languageserver": "^0.1.138", "monaco-editor-webpack-plugin": "^7.0.1", "monaco-editor-workers": "^0.34.2", "monaco-languageclient": "^4.0.3", diff --git a/lerna.json b/lerna.json index ed12df8..863c978 100644 --- a/lerna.json +++ b/lerna.json @@ -1,5 +1,5 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", "useWorkspaces": true, - "version": "0.1.137" + "version": "0.1.138" } diff --git a/package-lock.json b/package-lock.json index e804b53..3df4db8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,7 +18,7 @@ }, "actions-expressions": { "name": "@github/actions-expressions", - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "devDependencies": { "@types/jest": "^29.0.3", @@ -34,11 +34,11 @@ }, "actions-languageserver": { "name": "@github/actions-languageserver", - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "dependencies": { - "@github/actions-languageservice": "^0.1.137", - "@github/actions-workflow-parser": "^0.1.137", + "@github/actions-languageservice": "^0.1.138", + "@github/actions-workflow-parser": "^0.1.138", "@octokit/rest": "^19.0.7", "vscode-languageserver": "^8.0.2", "vscode-languageserver-textdocument": "^1.0.7", @@ -59,11 +59,11 @@ }, "actions-languageservice": { "name": "@github/actions-languageservice", - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "dependencies": { - "@github/actions-expressions": "^0.1.137", - "@github/actions-workflow-parser": "^0.1.137", + "@github/actions-expressions": "^0.1.138", + "@github/actions-workflow-parser": "^0.1.138", "vscode-languageserver-textdocument": "^1.0.7", "vscode-languageserver-types": "^3.17.2", "yaml": "^2.1.1" @@ -82,10 +82,10 @@ }, "actions-workflow-parser": { "name": "@github/actions-workflow-parser", - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "dependencies": { - "@github/actions-expressions": "^0.1.137", + "@github/actions-expressions": "^0.1.138", "cronstrue": "^2.21.0", "yaml": "^2.0.0-8" }, @@ -105,10 +105,10 @@ } }, "browser-playground": { - "version": "0.1.137", + "version": "0.1.138", "license": "MIT", "dependencies": { - "@github/actions-languageserver": "^0.1.137", + "@github/actions-languageserver": "^0.1.138", "monaco-editor-webpack-plugin": "^7.0.1", "monaco-editor-workers": "^0.34.2", "monaco-languageclient": "^4.0.3", @@ -14570,8 +14570,8 @@ "@github/actions-languageserver": { "version": "file:actions-languageserver", "requires": { - "@github/actions-languageservice": "^0.1.137", - "@github/actions-workflow-parser": "^0.1.137", + "@github/actions-languageservice": "^0.1.138", + "@github/actions-workflow-parser": "^0.1.138", "@octokit/rest": "^19.0.7", "@types/jest": "^29.0.3", "fetch-mock": "^9.11.0", @@ -14588,8 +14588,8 @@ "@github/actions-languageservice": { "version": "file:actions-languageservice", "requires": { - "@github/actions-expressions": "^0.1.137", - "@github/actions-workflow-parser": "^0.1.137", + "@github/actions-expressions": "^0.1.138", + "@github/actions-workflow-parser": "^0.1.138", "@types/jest": "^29.0.3", "jest": "^29.0.3", "prettier": "^2.8.3", @@ -14604,7 +14604,7 @@ "@github/actions-workflow-parser": { "version": "file:actions-workflow-parser", "requires": { - "@github/actions-expressions": "^0.1.137", + "@github/actions-expressions": "^0.1.138", "@types/jest": "^29.0.3", "@typescript-eslint/eslint-plugin": "^5.40.0", "@typescript-eslint/parser": "^5.40.0", @@ -17562,7 +17562,7 @@ "browser-playground": { "version": "file:browser-playground", "requires": { - "@github/actions-languageserver": "^0.1.137", + "@github/actions-languageserver": "^0.1.138", "css-loader": "^6.7.2", "monaco-editor-webpack-plugin": "^7.0.1", "monaco-editor-workers": "^0.34.2",