Skip to content

fix: constrain analytics app field characters on event type writes#28895

Open
pedroccastro wants to merge 3 commits intomainfrom
fix/normalize-analytics-field-chars
Open

fix: constrain analytics app field characters on event type writes#28895
pedroccastro wants to merge 3 commits intomainfrom
fix/normalize-analytics-field-chars

Conversation

@pedroccastro
Copy link
Copy Markdown
Contributor

What does this PR do?

Adds a character allowlist for analytics app fields (trackingId, trackingEvent, SITE_ID, etc.) that are interpolated into inline script templates rendered by BookingPageTagManager. Applied on event type create, update, and duplicate so that stored values consistently match the expected format.

Changes

  • New packages/app-store/_utils/sanitize-analytics-value.ts helper with sanitizeAnalyticsApps(metadata) — strips any character outside [a-zA-Z0-9\-._/:] from template fields of known analytics apps (ga4, gtm, metapixel, fathom, plausible, posthog, umami, matomo, databuddy, insihts, twipla)

  • Wraps metadata with sanitizeAnalyticsApps in: create.handler.ts , update.handler.ts , duplicate.handler.ts

  • Unit test coverage for valid values, empty strings, null metadata, non-analytics apps, and odd inputs

Context

Analytics app fields are substituted into script templates via parseValue. This normalizes stored values to the character set the templates expect, avoiding mismatched or ambiguous substitutions across integrations. The allowlist was derived from the shapes of each provider's script.content and zod.ts.

How should this be tested?

TZ=UTC yarn vitest run packages/app-store/_utils/sanitize-analytics-value.test.ts

Manual

  1. Configure an analytics app (e.g., GA4) on an event type with a normal tracking ID (G-ABC123) → saves and renders as before
  2. Duplicate that event type → new event keeps the same value
  3. Edit the event type → value remains unchanged on update

Mandatory Tasks

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have updated the developer docs in /docs if this PR makes changes that would require a documentation change.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e57be5e0-9b2f-40d5-988c-9130546ce3c9

📥 Commits

Reviewing files that changed from the base of the PR and between a17f28e and f954afa.

📒 Files selected for processing (5)
  • packages/app-store/_utils/sanitize-analytics-value.test.ts
  • packages/app-store/_utils/sanitize-analytics-value.ts
  • packages/trpc/server/routers/viewer/eventTypes/heavy/create.handler.ts
  • packages/trpc/server/routers/viewer/eventTypes/heavy/duplicate.handler.ts
  • packages/trpc/server/routers/viewer/eventTypes/heavy/update.handler.ts

📝 Walkthrough

Walkthrough

This pull request introduces a new utility function sanitizeAnalyticsApps in the app-store package that sanitizes analytics metadata values. The function strips potentially unsafe characters from analytics configuration fields while preserving known-good tracking identifiers for supported providers like GA4, GTM, and Plausible. The sanitization logic is integrated into three event type handlers—create, duplicate, and update—to ensure metadata is sanitized before being persisted to the database. A comprehensive test suite validates the sanitizer's behavior across various scenarios including XSS payload removal, null handling, and non-analytics app preservation.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: constraining analytics app field characters on event type writes, which matches the core purpose of adding the sanitizeAnalyticsApps helper.
Description check ✅ Passed The description is well-detailed and directly related to the changeset, explaining what the PR does, which files are changed, how to test it, and the context behind the change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/normalize-analytics-field-chars

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pedroccastro pedroccastro requested a review from a team April 15, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants