Conversation
Require a known credential format or a non-placeholder value before scoring COMMITTED_SECRET, skip template env files and public client IDs, and stop rendering unverified git status as confirmed-tracked. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate detection gaps and contradictory template guidance remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes App Doctor secret-scanning false positives while preserving detection of valid credentials.
Changes:
- Requires stronger evidence for committed-secret findings.
- Adds template, placeholder, public-key, and git-status handling.
- Updates guidance, tests, rule version, and patch changeset.
File summaries
| File | Summary |
|---|---|
packages/app/src/cli/services/app-doctor-engine/tests/secret-safety.test.ts |
Adds regression coverage for refined secret detection. |
packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts |
Verifies deterministic rule versioning. |
packages/app/src/cli/services/app-doctor-engine/scanners/index.ts |
Bumps the deterministic rule version. |
packages/app/src/cli/services/app-doctor-engine/rules/secret-rules.ts |
Implements evidence-based classification; moderate issues remain for multi-suffix templates and quoted JSON keys. |
packages/app/src/cli/services/app-doctor-engine/checks/embedded.ts |
Updates embedded guidance; template guidance needs correction. |
packages/app/src/cli/services/app-doctor-engine/checks/COMMITTED_SECRET.md |
Updates check documentation; template guidance needs correction. |
.changeset/app-doctor-secret-false-positives.md |
Documents the patch release. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Match multi-suffix env templates, extract quoted JSON secret keys, and clarify that template files still report known credential formats. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Avoid ReDoS on (?:\.[^/]+)* when suffixes contain extra dots. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
| // `SHOPIFY_API_KEY` / `api_key` are client IDs and public; they are not secret names. | ||
| // Optional quotes cover JSON/YAML keys (`"password": "…"`) as well as env assignments. | ||
| const SECRET_ASSIGNMENT_PATTERN = | ||
| /(?:api[_-]?key|api[_-]?secret|access[_-]?token|secret[_-]?key|private[_-]?key|password|SHOPIFY_API_KEY|SHOPIFY_API_SECRET)\s*[:=]\s*(?:"[^"\r\n]+"|'[^'\r\n]+'|[^\s#'"\r\n][^#\r\n]*)/i | ||
| /["']?(?:api[_-]?secret|access[_-]?token|secret[_-]?key|private[_-]?key|password|SHOPIFY_API_SECRET)["']?\s*[:=]\s*(?:"([^"\r\n]*)"|'([^'\r\n]*)'|([^\s#'"\r\n][^#\r\n]*))/gi |
There was a problem hiding this comment.
when it comes to leaked credentials, do we care about shopify credentials specifically? or any credentials? because i think we can look for the token prefix patterns maybe? (like shpat, shpss, etc.)
question: ae we aiming to detect leaked Shopify credentials specifically, or credentials more broadly that could compromise the app and merchant data?
because i'm wondering if the deterministic check should focus on recognized secret formats, like how we use prefixes for our secrets (e.g., shpat_, shpss sort of thing). that wouldn't account for legacy secrets that don't have prefixes admittedly, though i'm not sure how common that is. wrt non-shopify secrets, we could also search for things with well known formats like stripe secret keys (e.g., sk_live_ but not sk_test_), github tokens (e.g., ghp_, github_pat_) , etc.
i think this change reduces the false positives which is good, but i think it's worth being explicit about whether we care about non-shopify api keys being detected. i'm not sure but maybe there are things we could flag for agentic review to be verified? just spitballing
There was a problem hiding this comment.
when it comes to leaked credentials, do we care about shopify credentials specifically? or any credentials?
I think our product aim is to try to catch common versioned secrets, even non-Shopify. I'll double-check this assumption!
wrt non-shopify secrets, we could also search for things with well known formats like stripe secret keys (e.g., sk_live_ but not sk_test_), github tokens (e.g., ghp_, github_pat_) , etc.
We support some of these common ones (Google, Stripe, AWS, Slack) today - see test cases in packages/app/src/cli/services/app-doctor-engine/tests/secret-safety.test.ts.
Unquoted 32-hex Shopify secrets now match the quoted form. Assignment scans no longer consume the next line or treat HAS_PASSWORD as a secret. Co-authored-by: AI (Pi/Grok 4.6) <noreply@pi.dev>
WHY are these changes introduced?
Fixes https://github.com/shop/issues-develop/issues/23957
Early-access partners were flooded with
COMMITTED_SECRETfindings for placeholders,.env.exampletemplates, public client IDs, and Stripe publishable keys. A name match plus a non-empty value was enough to score -50 high, which fails the static-tier bar of provable facts.WHAT is this pull request doing?
Require two pieces of evidence before scoring an env/secret file: a known credential format, or a secret-named assignment whose value is not a placeholder. Template env files (
.env.example,.sample,.template,.dist) only fire on a known token format. DropSHOPIFY_API_KEY/api_keyand Stripepk_from secret detection. Keep fail-closed for real secrets when git status is unknown, but stop rendering that case as confirmed-tracked.How to manually test your changes?
From an app with a committed
.env.exampleof placeholders and a tracked.envthat contains a realshpss_/shpat_token:Confirm the template is not reported and the real token still is.
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add