App Security: scan multiple directories with --include-dir - #8739
Conversation
1e00e54 to
681002d
Compare
681002d to
a22217c
Compare
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/api/app-management.d.ts@@ -7,7 +7,6 @@ export declare const appManagementAppLogsUrl: (organizationId: string, cursor?:
status?: string;
source?: string;
}) => Promise<string>;
-export declare const appManagementChannelSpecExportUrl: (organizationId: string, appId: string) => Promise<string>;
export interface RequestOptions {
requestMode: RequestModeInput;
}
|
cfadbee to
ff94de3
Compare
App Security can scan code outside the app directory, such as a backend or a shared library. Each scan directory follows its own repositories' rules, and paths are relative to the app directory, so they may start with ../.
Gathered files are stored relative to the app directory, and no relative path reaches another drive or a network share, so those files were silently left unscanned.
ff94de3 to
1adfc17
Compare
|
| Command | Flag |
|---|---|
app:security:check |
--ignore |
jplhomer
left a comment
There was a problem hiding this comment.
LGTM - we'll work on cleaning up the deterministic checks to support this.
dmerand
left a comment
There was a problem hiding this comment.
Overall change works + LGTM. Per usual, I have a couple of agent suggestions to consider, at your discretion.
| ].filter((candidate, index, all) => all.findIndex(({directory}) => directory === candidate.directory) === index) | ||
|
|
||
| return { | ||
| scanDirectories: requested.filter( |
There was a problem hiding this comment.
Suggestion: a requested directory can be silently unscanned when an enclosing repository prunes an ancestor.
Example from the source: the app repository ignores vendor/, vendor/sdk is its own repository, and the user passes --include-dir vendor/sdk/src. Merging drops src because it sits inside the app. The outer walk prunes vendor/ by ignore rules. The ignored-warning probe asks the repository that owns vendor/sdk, which reports src as not ignored. So nothing from src is gathered, and no warning or coverage gap says why.
This is a static reading, not yet reproduced at runtime.
If outer-wins is intended, consider warning when a retained walk cannot reach a requested directory because an enclosing repository prunes an ancestor — the current probe only asks the requested directory's own parent. --include-dir vendor/sdk works today, so this can stay a suggestion.
| paths: [...new Set(absolutePaths.map((path) => normalizeCliPath(relativePath(appDirectory, path))))].sort(), | ||
| ignoredScanDirectories: scanDirectories.filter((_directory, index) => gathered[index]?.ignored), | ||
| ignoredScanDirectories, | ||
| listingStatus: gathered[0]?.listingStatus ?? (rules.gitFiltering ? 'tracked-only' : 'git-ignore-off'), |
There was a problem hiding this comment.
Suggestion: the first directory's listing status explains every secret finding, including files from other directories.
listingStatus comes from gathered[0], but with --include-dir the scan directories can have different statuses. Two cases from the source:
- The app directory is
listedand a second repository's listing fails. An ignored.envgathered from the second directory is then explained as inside a nested git repository, when the real reason was the failed listing. - The first directory is
failed, so every ignored file anywhere in the scan is explained as a listing failure.
Consider carrying the walk or repository status of the file's own scan directory to scanCommittedSecrets, instead of one global status. This affects explanations and evidence, not detection — no missed-finding case is established.
Static reading only; not reproduced at runtime.
| export async function gitStatusFor(appRoot: string, file: string): Promise<GitFileStatus> { | ||
| const run = async (args: string[]) => runGit(appRoot, args) | ||
| /** Asks the file's own repository, which may differ from the app's: a scan directory can be another repository. */ | ||
| export async function gitStatusFor(file: Pick<SourceFile, 'path' | 'absolutePath'>): Promise<GitFileStatus> { |
There was a problem hiding this comment.
Suggestion: the remediation for a file in another repository assumes the app repository.
For a tracked ../backend/.env, this branch suggests git rm --cached ../backend/.env and adding that path to .gitignore. Git cannot remove a path outside the running repository's work tree, and the app's ignore rules cannot protect a sibling repository's file.
The Git probes now correctly run in the file's own repository (dirname(file.absolutePath)), but the evidence strings still read as app-root commands. Consider rendering the remediation and evidence with the owning repository's directory and the filename relative to it, while keeping the finding location app-relative.
Presentation-only: no execution runs these commands. Static reading; not reproduced at runtime.
WHY are these changes introduced?
App code often lives outside the directory holding the TOML, such as a backend, a shared library or another repository, and couldn't be scanned.
WHAT is this pull request doing?
Add
--include-dirtocheck. The app directory and each--include-dirare merged by real path (duplicates and nested directories are dropped), each is walked under its own repositories' rules, and paths are reported relative to the app directory, so they may start with../. The secret scan asks each file's own repository for its Git status, andrecordaccepts../paths.How to manually test your changes?
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add