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] 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(