Skip to content

Exclude gitignored files from App Security discovery - #8691

Merged
jek merged 7 commits into
mainfrom
app-security/exclude-gitignore
Sep 30, 2026
Merged

jek merged 7 commits into
mainfrom
app-security/exclude-gitignore

Conversation

@jek

@jek jek commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

shopify app security check scanned files that git ignores, such as local .env files and generated output. That produced findings, including committed-secret findings, for files that never reach the repository.

WHAT is this pull request doing?

Discovery now skips paths that git ignores. It asks git once for the app's untracked, ignored paths (git ls-files --others --ignored --exclude-standard --directory), so nested .gitignore files, negations, .git/info/exclude and global excludes all apply. Tracked files that match .gitignore are still scanned.

To support this, discovery walks the app root once with a single matcher, instead of running a separate glob per file type. The matcher combines the default exclusions with git's ignored paths and prunes excluded folders during the walk. #8702 builds on it to add --ignore, whose patterns override both, for example !build/ to scan a folder that's excluded by default.

Changes that follow from the new walker:

  • Dot-folders are now scanned; previously none were. .github/ and .vscode/ are checked for secrets. Generated dot-folders (.next/, .nuxt/, .yarn/, .cache/, …) are default exclusions, which --ignore can override in Add --ignore to app security check and instructions #8702. .shopify/ is excluded as a whole, so it stays unscanned; before, only .shopify/app-security/ was listed, which made no difference because dot-folders were skipped anyway.
  • Dependabot and Renovate config follows the same exclusions, so an ignored config file doesn't count as dependency automation.
  • The committed-secret check no longer checks ignore status itself, because discovery does. When a file git ignores is still scanned, the finding says why. The check's version goes from 2 to 3.
  • Exclusions aren't recorded in the trace or submission, matching the existing default exclusions.

Edge cases

Much of the diff is tests for these, found during development and review:

  • An enclosing repository ignores the app folder: git's rules aren't applied. That repository doesn't own the app, and applying its rules would empty the scan and report the app clean. The check uses git check-ignore --no-index, so a force-tracked file inside the app doesn't hide it.
  • Whitelist-style .gitignore (*, then !src/): the repository's top level and folders included again aren't treated as ignored.
  • Nested git repository inside the app: scanned when the app's repository doesn't ignore it, because the app repository's rules don't reach inside it. When the app's repository ignores it, directly or through a parent folder, it's excluded like any ignored folder.
  • No repository, git missing, or git refuses the repository (dubious ownership, corrupt config): no git exclusions apply. A .git marker check tells "no repository" apart from "git failed" without parsing git's localized error messages.
  • App folder inside .git: treated as not in a repository.
  • Git worktrees: .git is excluded both as a folder and as the file that worktrees use.
  • Filenames with gitignore-significant characters ([id].ts, #hash.ts): git's ignored paths are matched literally.
  • Selected app configuration file: loaded and scanned for secrets even when git ignores it (shopify.app.staging.toml) or a default exclusion matches it (*.test.* matches shopify.app.test.toml).
  • Symlinked folders: listed but not traversed.
  • Nested apps: still skipped, even when their configuration file is excluded.
  • Unignored node_modules/: git's listing skips the default folders, so it stays fast.

How to manually test your changes?

In a git-tracked app:

  1. Add .env to .gitignore, and put a Shopify token-shaped value (shpat_ followed by 32 hex characters) in .env.
  2. pnpm shopify app security check --path /path/to/app: no committed-secret finding for .env.
  3. git -C /path/to/app add -f .env, then rerun: the finding appears, because tracked files are scanned.
  4. Put the same value in .github/workflows/deploy.yml and rerun: it's reported.

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

@jek
jek marked this pull request as ready for review September 28, 2026 22:14
@jek
jek requested a review from a team as a code owner September 28, 2026 22:14
@github-actions github-actions Bot added the Area: @shopify/app @shopify/app package issues label Sep 28, 2026
@jek
jek requested a review from jplhomer September 28, 2026 22:14

Copy link
Copy Markdown
Contributor

Sorry to ask, but why is a change to exclude gitignored files, a +2000 line change?
I'm sure this can be simplified?

@jek
jek added this pull request to stack #8703 September 29, 2026 20:26
@jek

jek commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Sorry to ask, but why is a change to exclude gitignored files, a +2000 line change? I'm sure this can be simplified?

Fair question! The PR description didn't do a good job cataloging the knock-on effects this had and the edge cases closed. Updated that description & trimmed a lot of comment fat as well.

jek added 2 commits September 29, 2026 15:37
App Security's deterministic checks scanned files that git ignores, such
as local .env files and generated output, and reported findings for files
that never reach the repository.

Discovery now walks the app root once and skips a path when either of two
exclusion phases matches it:

- Default .gitignore-style patterns for dependencies, build output,
  caches, test and fixture trees, and CLI-generated folders.
- The untracked, ignored paths git reports, so nested .gitignore files,
  negations, .git/info/exclude and global excludes all apply. Tracked
  files that match .gitignore are still scanned.

No git exclusions apply when the app isn't in a git repository, when git
fails, or when an enclosing repository ignores the app folder or one of
its ancestors. Git's listing skips the default directories, so git doesn't
traverse trees the walker never enters.

The walker prunes excluded folders, stops at nested apps, and walks
dot-folders and dotfiles, so .github/ and .vscode/ are now scanned for
secrets. Loading the app configuration isn't subject to exclusions.
Dependabot and Renovate configuration is, so an ignored configuration file
no longer counts as dependency automation.

The committed-secret check no longer skips untracked, ignored files on its
own, because discovery decides what is scanned. When git ignores a file
that was still scanned, the finding says why: an enclosing repository
ignores the app, the file is inside a nested repository, or git couldn't
list ignored files.
@jek
jek force-pushed the app-security/exclude-gitignore branch from 78a0d46 to 1b46bd1 Compare September 29, 2026 22:39

@dmerand dmerand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes + description updates. I have a few LLM comments below but overall it looks good.

Comment thread packages/app/src/cli/services/app-security-engine/scanners/index.ts
Comment thread .changeset/app-security-gitignore.md Outdated
Comment thread packages/app/src/cli/services/app-security-engine/scanners/index.ts Outdated
jek added 4 commits September 30, 2026 11:52
…de it

The selected app configuration was loaded and analyzed by config checks
even when gitignored, but its secret scan depended on discovery, so path
rules silently dropped it. A token-shaped value in a gitignored
shopify.app.staging.toml went unreported, even though deploys send its
values to Shopify. A default exclusion had the same effect: `*.test.*`
matches shopify.app.test.toml.

The selected configuration is an explicit input, so it now joins the
secret-scan inventory whenever it loads.
App Security stays out of public release notes until it ships (#8698).
The check now reports ignored files that discovery still scanned, with the
reason, instead of skipping them, and it scans the selected app
configuration even when path rules exclude it. Findings and the execution
record carry the new version.
A nested repository is excluded like any ignored folder when the app's
repository ignores it, directly or through a parent folder. Finding
repositories below ignored parents would mean walking the ignored trees
that pruning skips; --ignore from #8702 can opt one back in.
@github-actions github-actions Bot added no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. and removed Area: @shopify/app @shopify/app package issues labels Sep 30, 2026
Discovery joins paths with cli-kit, which uses / on every platform, so
node:path join produced a mismatched expectation on Windows.
@jek
jek added this pull request to the merge queue Sep 30, 2026
Merged via the queue into main with commit 8b15d8c Sep 30, 2026
30 checks passed
@jek
jek deleted the app-security/exclude-gitignore branch September 30, 2026 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants