Repository navigation
App Security: gather files with Git ignore rules, --exclude and --no-git-ignore - #8738
Conversation
github-actions
Bot
added
the
no-changelog
jek
changed the title
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/cli.d.ts@@ -35,7 +35,6 @@ export declare function runCreateCLI(options: RunCLIOptions, launchCLI?: (option
export declare const globalFlags: {
'no-color': import("@oclif/core/interfaces").BooleanFlag<boolean>;
verbose: import("@oclif/core/interfaces").BooleanFlag<boolean>;
- 'no-input': import("@oclif/core/interfaces").BooleanFlag<boolean>;
};
export declare const jsonFlag: {
json: import("@oclif/core/interfaces").BooleanFlag<boolean>;
|
5187bd9 to
74700bd
Compare
74700bd to
e991491
Compare
e991491 to
4856f12
Compare
…it-ignore Replace --ignore and the default exclude list. Each file follows its own repository's ignore rules, --exclude takes globs relative to the working directory, and --no-git-ignore turns Git filtering off. A scan directory that its repository ignores gathers only tracked files, with a warning. All reads go through one reader configured per scan.
4856f12 to
63f31b9
Compare
|
| Command | Flag |
|---|---|
app:security:check |
--ignore |
jplhomer
left a comment
jplhomer
left a comment
There was a problem hiding this comment.
Only potential side effect that may bite is having all the tests/fixtures included in the run (they were ignored before with a custom list).
| const parent = dirname(directory) | ||
| if (parent === directory) return false | ||
| const result = await runGit(parent, ['check-ignore', '--no-index', '-q', '--', basename(directory)]) | ||
| return result?.exitCode === 0 |
There was a problem hiding this comment.
It may be worth treating a missing Git binary as a listing failure instead of trusting the exit code.
I smoke-tested the compiled CLI with Git unavailable from PATH. check exits 0, warns that the app directory is ignored by Git, gathers only the selected TOML, and reports files_scanned: 0 with no gaps and an executed COMMITTED_SECRET check with no findings. A synthetic token in an untracked .env goes unreported. The PR base reports that finding; --no-git-ignore also reports it.
The cause: when the Git spawn fails, captureOutputWithExitCode returns exit code 0 with empty output. This function then reports the directory as ignored, and listTrackedFiles returns the empty list instead of undefined, so the tracked-only branch gathers nothing and the failed listing status is never reached.
| /** What gathering reports besides the result, for the caller to show. It isn't part of the stored findings. */ | ||
| export interface ScanOutput extends ScanResult { | ||
| /** Absolute paths of the scan directories that their repository ignores, so only the files Git tracks in them were scanned. */ | ||
| ignoredScanDirectories: string[] |
There was a problem hiding this comment.
Did you consider surfacing ignored scan directories in the JSON output and stored coverage?
scan_directories shows the requested roots, but nothing in the machine-readable output says that a root was ignored by Git and therefore gathered tracked-only. The terminal report warns about it, yet a JSON consumer — or anyone reading deterministic-findings.json later — sees a scan that looks fully walked.
3 participants
WHY are these changes introduced?
Gathering combined a hidden default exclude list,
--ignoreoverrides and a skip of directories holding another TOML, so it was hard to predict which files were scanned.WHAT is this pull request doing?
Replace
--ignoreand the default list with Git's own rules: each file follows its own repository's ignore rules, tracked files are always scanned, and a scan directory that its repository ignores gathers only its tracked files, with a warning.--excludetakes globs relative to the working directory, and--no-git-ignoreturns Git filtering off. The selected TOML is always scanned. All reads go through one reader configured per scan.With
--no-git-ignore, an untracked secret file that Git ignores is no longer reported as COMMITTED_SECRET.How to manually test your changes?
shopify app security check --exclude 'dist/**' shopify app security check --no-git-ignore --exclude node_modulesChecklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add