Skip to content

Allow static theme asset script URLs in app doctor - #8502

Open
francisbeaudoin wants to merge 1 commit into
Shopify:mainfrom
francisbeaudoin:fix-static-asset-script-src
Open

Allow static theme asset script URLs in app doctor#8502
francisbeaudoin wants to merge 1 commit into
Shopify:mainfrom
francisbeaudoin:fix-static-asset-script-src

Conversation

@francisbeaudoin

@francisbeaudoin francisbeaudoin commented Sep 8, 2026

Copy link
Copy Markdown

What changed

  • Allow the UNSAFE_INNERHTML Liquid check to skip literal theme asset script URLs like {{ 'chat.js' | asset_url }}.
  • Keep dynamic script asset URLs flagged, including {{ block.settings.script | asset_url }}.
  • Add a changeset for the App Doctor false-positive fix.

Testing

  • pnpm vitest packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts --run
  • pnpm eslint packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts

@francisbeaudoin
francisbeaudoin requested a review from a team as a code owner September 8, 2026 22:13
Copilot AI lite review requested due to automatic review settings September 8, 2026 22:13

Copilot AI 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.

🟢 Approval recommended

The behavior change is narrowly scoped, includes targeted test coverage, and preserves detection for dynamic script URLs.

Pull request overview

Updates the App Doctor Liquid security scanning to avoid flagging <script src> tags that load a static theme asset URL (a quoted literal piped to asset_url), reducing false positives while still flagging dynamic/merchant-controlled script URLs.

Changes:

  • Add a isStaticScriptAssetUrl guard to skip UNSAFE_INNERHTML findings for <script src="{{ 'literal.js' | asset_url }}">.
  • Extend Liquid AST analysis tests to cover the static-asset allowance and keep dynamic asset_url cases flagged.
  • Add a patch changeset documenting the false-positive fix.
File summaries
File Description
packages/app/src/cli/services/app-doctor-engine/rules/liquid-rules.ts Skips UNSAFE_INNERHTML for literal asset_url script src outputs to prevent false positives.
packages/app/src/cli/services/app-doctor-engine/tests/rule-analysis.test.ts Adds coverage ensuring static asset_url script tags are allowed while dynamic ones remain flagged.
.changeset/quiet-assets-repair.md Records the user-facing App Doctor false-positive fix as a patch release.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants