feat: new secscan action - #155
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new secscan GitHub Action (Node/TypeScript) intended to run Canonical’s secscan-client against a provided snap, collect scanner outputs, and upload results as an artifact, alongside introducing a pnpm-based TypeScript tooling setup for the repo.
Changes:
- Introduce the
secscanaction implementation (inputs parsing + scan execution + artifact upload) and a Vitest test suite. - Add monorepo JS tooling: pnpm workspace/lockfile, root TypeScript config, and ESLint flat config.
- Update
.gitignorefor pnpm and (implicitly) for committing built action artifacts.
Reviewed changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tsconfig.json |
Adds TypeScript compiler configuration for the workspace. |
secscan/src/index.ts |
Implements the action logic (input handling, scan orchestration, artifact upload). |
secscan/tests/index.test.ts |
Adds unit tests for parsing/derivation and scanner exit-code handling. |
secscan/action.yaml |
Declares the secscan GitHub Action interface (inputs/outputs/runtime). |
secscan/package.json |
Defines package scripts/dependencies for building/testing the action. |
package.json |
Adds root-level pnpm scripts and dev tool dependencies. |
eslint.config.js |
Adds ESLint flat config with TypeScript support. |
pnpm-workspace.yaml |
Configures pnpm workspace layout. |
pnpm-lock.yaml |
Adds pnpm lockfile for reproducible installs. |
.gitignore |
Updates ignore rules for pnpm (and changes handling of dist/). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8c83b7a to
a1cb5f9
Compare
a1cb5f9 to
9ce60e4
Compare
ef61dd3 to
540f7aa
Compare
540f7aa to
bd2db59
Compare
There was a problem hiding this comment.
The transpiled, flattened version of the code must be present on main for actions to work. It's possible to set up a CI pipeline that generates and commits it for you, but for the time being it's a lot simpler to just make sure this is committed. A workflow will soon come to make sure that the committed version is up-to-date with the source.
| export function getSnapInfoField(output: string, field: string): string { | ||
| return ( | ||
| output.match(new RegExp(`^${field}:\\s+(.+)$`, "m"))?.[1]?.trim() ?? "" | ||
| ); | ||
| } |
There was a problem hiding this comment.
This regex pulls out the value in the name: or version: field from snap info <snap_file>
| } | ||
|
|
||
| export async function runScanner( | ||
| scanner: (typeof SUPPORTED_SCANNERS)[number], |
There was a problem hiding this comment.
This type-narrows scanner from "any string" to "only the strings contained by SUPPORTED_SCANNERS".
| tokensDir: string, | ||
| resultsDir: string, | ||
| ): Promise<void> { | ||
| const tokenFile = join(tokensDir, `${scanner}-token.txt`); |
There was a problem hiding this comment.
The secscan client writes session tokens to this file so subsequent commands can reference an in-progress or completed scan. It is no longer needed once the scan results are collected.
| const reportExt = SCANNER_OUTPUT_FORMAT[scanner]; | ||
| const reportFile = join(scannerResultsDir, `${scanner}_report.${reportExt}`); | ||
| await writeFile(reportFile, capturedOutput); | ||
| await resultPromise; |
There was a problem hiding this comment.
This is probably me gilding the lily a bit, but I await resultPromise at the very end here just because its result is irrelevant to this method, so I didn't want to await in the middle of actual calculations
| if (import.meta.url === `file://${process.argv[1]}`) { | ||
| run(); | ||
| } |
There was a problem hiding this comment.
This is the TS equivalent to if __name__ == "__main__":, used to avoid invoking run() when importing the module for tests.
| banner: { | ||
| js: "import { createRequire as __require__ } from 'module'; globalThis.require = __require__(import.meta.url);", | ||
| }, |
There was a problem hiding this comment.
This prepends the string to dist/index.js for each built action. The string fixes an issue that's frankly really convoluted and circular and exemplifies the Node ecosystem. TL;DR, there's multiple "kinds" of Node scripts and modern TypeScript is not the same "kind" as the flattened JavaScript we need to output for GitHub Actions, and this is mostly fine but it caused some confusion around the require function and this is the workaround that the internet recommends.
| "build": "pnpm -r run build", | ||
| "test": "pnpm -r run test", | ||
| "lint": "eslint . && tsc --noEmit" |
There was a problem hiding this comment.
Each of these can be run at the root of the project as pnpm <script>. pnpm -r <action> will find all subdirectories that contain a package.json file and run pnpm <action>. You can also run it for a specific action without cding with, for example, pnpm lint -F secscan.
Actions here are overridden by actions of the same name in subdirectories.
| } | ||
| }, | ||
| "type": "module", | ||
| "devDependencies": { |
There was a problem hiding this comment.
These dependencies are inherited by subdirectories and are common dependencies other actions may want. It's the compiler, the testing framework, and the linter.
There was a problem hiding this comment.
This says "everything with a package.json is part of this workspace, and esbuild is the only command that's allowed to be executed during builds" (we love not having ACE vulnerabilities in our build systems)
Creates a new action to automate secscan. The intended use-case is for this to run on new tags, or as a monthly workflow against
latest/stable. You can see it in action here: canonical/snapcraft#6303CRAFT-5208