enable page plugin contributions for public routes and enhance GTM plugin#156
enable page plugin contributions for public routes and enhance GTM plugin#156ryota-yamada-jp wants to merge 17 commits into
Conversation
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hey! #119 fixed the issue with hooks for anonymous visitors, so this will need some reworking. |
|
@ascorbic Okay! I will fix this. Please wait a moment. |
|
@ascorbic I did fix it! |
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
PR template validation failedPlease fix the following issues by editing your PR description:
See CONTRIBUTING.md for the full contribution policy. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
The PR description claims two changes: (1) middleware fixes to enable page:fragments/page:metadata for public routes, and (2) GTM plugin enhancements. The actual diff contains only a new @emdash-cms/plugin-google-tag-manager package. The middleware narrative is stale—the fixes described were already merged to main in a prior release (see packages/core/CHANGELOG.md line 713 and the current middleware.ts). Reviewers should ignore the description and evaluate only the new plugin.
The plugin is a reasonable addition, but the implementation introduces stored XSS vulnerabilities and has several convention gaps that need fixing before merge:
- Unescaped user input in
page:fragments. The hook interpolates KV values (gtmScriptUrl,gtmNoScriptUrl,dataLayerName,containerId) directly into JavaScript string literals and HTML attributes without escaping or validation. A single quote in a URL breaks the JS string; a double quote breaks the HTML attribute. The corerenderFragmenthelper does not protect against this—forinline-scriptit only escapes</, and forhtmlit returns the markup verbatim. - No input validation on the admin form. Arbitrary strings are persisted to KV. URLs should be validated as
https://and the container ID should match theGTM-XXXXXXXformat. - Deprecated capability. The plugin uses
"page:inject", which is in the deprecation window. New plugins should use"hooks.page-fragments:register". - Missing changeset. A new published package needs a changeset to trigger a release.
- No tests. Other plugins in
packages/plugins/have unit/integration tests; this package has none. - Missing approved Discussion. Per
AGENTS.md, feature PRs require a prior approved Discussion.
Sandboxed plugins currently have no Lingui integration path (none of the existing plugins localize Block Kit strings), so I’m not flagging the hardcoded English as a dedicated finding—it’s a systemic gap.
Once the XSS/validation issues are fixed, the capability is updated, tests are added, and a changeset is included, this plugin is good to merge.
| code: `(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start': | ||
| new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0], | ||
| j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src= | ||
| '${gtmScriptUrl}?id='+i+dl;f.parentNode.insertBefore(j,f); | ||
| })(window,document,'script','${dataLayerName}','${containerId}');`, |
There was a problem hiding this comment.
[needs fixing] The page:fragments hook interpolates user-controlled KV values (gtmScriptUrl, dataLayerName, containerId) into single-quoted JavaScript string literals without escaping. A single quote or backslash in any of these values breaks the string literal and allows arbitrary code execution on every public page. For example, a gtmScriptUrl of https://evil.com/x';alert(1);var x=' would execute alert(1). The core renderFragment helper only escapes </ sequences in inline scripts; it does not escape single quotes. The plugin must sanitize before interpolation—at minimum by escaping \ and ', or by segmenting the values safely, and ideally by validating against expected formats.
| { | ||
| kind: "html", | ||
| placement: "body:start", | ||
| html: `<noscript><iframe src="${gtmNoScriptUrl}?id=${containerId}" height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>`, |
There was a problem hiding this comment.
[needs fixing] The html fragment interpolates gtmNoScriptUrl and containerId directly into an <iframe src> attribute without HTML-escaping or URL-encoding. A double quote in either value breaks out of the src attribute and injects arbitrary HTML. An ampersand in containerId injects extra query parameters. The renderFragment helper returns html contributions verbatim, so the plugin must escape attribute metacharacters (" → ", & → &) and use encodeURIComponent for the query string.
| if (interaction.type === "form_submit" && interaction.action_id === "save_gtm") { | ||
| const values = interaction.values || {}; | ||
| await ctx.kv.set("settings:gtmContainerId", values.gtm_container_id || ""); | ||
| await ctx.kv.set("settings:gtmDataLayerName", values.gtm_data_layer_name || "dataLayer"); | ||
| await ctx.kv.set( | ||
| "settings:gtmScriptUrl", | ||
| values.gtm_script_url || "https://www.googletagmanager.com/gtm.js", | ||
| ); | ||
| await ctx.kv.set( | ||
| "settings:gtmNoScriptUrl", | ||
| values.gtm_noscript_url || "https://www.googletagmanager.com/ns.html", | ||
| ); |
There was a problem hiding this comment.
[needs fixing] The admin form submission handler stores arbitrary strings from interaction.values directly into KV without validation. gtm_script_url and gtm_noscript_url should be validated as HTTPS URLs (or at least valid URLs), and gtm_container_id should match the expected GTM-XXXXXXX format. Without validation, invalid or malicious input can break every public page or be exploited as stored XSS via the page:fragments hook.
| format: "standard", | ||
| entrypoint: "@emdash-cms/plugin-google-tag-manager/sandbox", | ||
| options: {}, | ||
| capabilities: ["page:inject"], |
There was a problem hiding this comment.
[needs fixing] The plugin declares the deprecated capability "page:inject". This name exists only in the deprecation window and is normalized to "hooks.page-fragments:register" at runtime. New plugins should use the current canonical capability name to avoid future breakage.
| capabilities: ["page:inject"], | |
| capabilities: ["hooks.page-fragments:register"], |
| ], | ||
| "author": "Your Name", | ||
| "license": "MIT", | ||
| "dependencies": {}, |
There was a problem hiding this comment.
[suggestion] The author field contains the placeholder value "Your Name". Update it to the actual author or remove the field.
| "dependencies": {}, | |
| "author": "Ryota Yamada", |
What does this PR do?
This PR enables the Google Tag Manager (GTM) plugin to function correctly on public pages and adds the missing advanced configuration options for GTM.
1. Enable Page Plugin Contributions for Public Routes
Previously, the Astro middleware (
onRequestinmiddleware.ts) bypassedEmDashRuntimeinitialization for unauthenticated public requests as a performance optimization. This inadvertently prevented trusted plugins from injecting scripts via thepage:fragmentsandpage:metadatahooks on public pages, effectively breaking plugins like Google Tag Manager that need to run for all visitors.getRuntime(config)is properly initialized.collectPageMetadataandcollectPageFragmentsmethods onlocals.emdashso Astro components likeEmDashBodyStartcan correctly render the injected HTML.middleware.test.tsto ensure runtime initialization and method binding work as expected even without an active session.2. GTM Plugin Enhancements
dataLayer), GTM Script URL (defaults tohttps://www.googletagmanager.com/gtm.js), and GTM NoScript URL (defaults tohttps://www.googletagmanager.com/ns.html) to support Server-Side GTM (SGTM) setups.Type of change
Checklist
pnpm typecheckpassespnpm --silent lint:json | jq '.diagnostics | length'returns 0pnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output