Skip to content

Conversation

@colton-demetriou
Copy link
Contributor

@colton-demetriou colton-demetriou commented Dec 29, 2025

Added unit tests, but unable to manually test as optimizelocation domains are only configured for production. Will need to smoke test this change along with the change to YSS to ensure nothing breaks in non-prod envs, and then test with an optimizelocation url in production.

@colton-demetriou colton-demetriou added the create-dev-release Triggers dev release workflow label Dec 29, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 29, 2025

commit: 103dd2b

@colton-demetriou colton-demetriou marked this pull request as ready for review December 30, 2025 19:21
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

Walkthrough

The changes add a new utility function isOriginAllowed that validates message origins by checking exact matches against a TARGET_ORIGINS set or matching the *.optimizelocation.com pattern (including the root domain). This function replaces direct origin membership checks in the message listener and updates dependency tracking in the hook. A comprehensive test suite validates behavior across exact matches, subdomain patterns, URL parsing, and edge cases for invalid inputs.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature: adding support for *.optimizelocation.com wildcard domains in the useMessage hook.
Description check ✅ Passed The description is related to the changeset, explaining the feature addition, testing approach, and manual testing limitations due to production-only domain configuration.
✨ Finishing touches
  • 📝 Generate docstrings

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
packages/visual-editor/src/internal/hooks/useMessage.ts (1)

219-219: Parameter targetOrigins is now ignored.

The targetOrigins parameter (line 201) is no longer used after this change. This breaks the contract of useListenAndRespondMessage and its public callers, which still accept and pass this parameter but now silently ignore it.

If the intent is to always validate against TARGET_ORIGINS + *.optimizelocation.com, the targetOrigins parameter should be removed from:

  • Line 201: useListenAndRespondMessage signature
  • Line 94: useSendMessageToIFrame signature
  • Line 129: useSendMessageToParent signature
  • Line 164: useReceiveMessage signature

This would be a breaking API change but makes the behavior explicit.

Note: This relates to the past review comment "TARGET_ORIGINS is a const so just use that instead of passing it in" — the parameter should be removed to complete the refactoring.

🧹 Nitpick comments (1)
packages/visual-editor/src/internal/hooks/useMessage.test.ts (1)

1-80: Comprehensive test coverage with room for minor enhancements.

The test suite thoroughly covers the core functionality of isOriginAllowed:

  • All seven TARGET_ORIGINS entries
  • HTTP and HTTPS protocols for *.optimizelocation.com
  • Various subdomains and the root domain
  • Invalid URLs and edge cases
  • Combined exact and pattern-based matching
Optional: Additional edge cases to strengthen test coverage

Consider adding tests for:

  1. Multi-level subdomains (explicitly):
+    it("should return true for multi-level subdomains", () => {
+      expect(isOriginAllowed("https://a.b.c.optimizelocation.com")).toBe(true);
+    });
  1. Case variations (verify URL constructor normalization):
+    it("should handle case-insensitive hostnames", () => {
+      expect(isOriginAllowed("https://XYZ.OptimizeLocation.COM")).toBe(true);
+    });
  1. Origins with explicit ports:
+    it("should handle origins with ports", () => {
+      expect(isOriginAllowed("https://xyz.optimizelocation.com:8080")).toBe(true);
+      expect(isOriginAllowed("http://localhost:3000")).toBe(false);
+    });
  1. Domain confusion attacks:
+    it("should reject similar but invalid domains", () => {
+      expect(isOriginAllowed("https://optimizelocation.com.evil.com")).toBe(false);
+      expect(isOriginAllowed("https://fakeoptimizelocation.com")).toBe(false);
+    });

These additions would provide defense-in-depth assurance but are not critical given the current implementation.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b3f86e3 and 103dd2b.

📒 Files selected for processing (2)
  • packages/visual-editor/src/internal/hooks/useMessage.test.ts
  • packages/visual-editor/src/internal/hooks/useMessage.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/internal/hooks/useMessage.test.ts (1)
packages/visual-editor/src/internal/hooks/useMessage.ts (1)
  • isOriginAllowed (47-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: call_unit_test / unit_tests (22.x)
  • GitHub Check: call_unit_test / unit_tests (24.x)
  • GitHub Check: call_unit_test / unit_tests (20.x)
  • GitHub Check: create-dev-release
  • GitHub Check: semgrep/ci
🔇 Additional comments (1)
packages/visual-editor/src/internal/hooks/useMessage.ts (1)

231-231: Dependency array is correct for current implementation.

Removing targetOrigins from the dependency array is correct since it's no longer referenced in onWatchEventHandler. However, this is part of the incomplete refactoring noted above—if targetOrigins were properly removed from the function signature, this would be fully consistent.

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

Labels

create-dev-release Triggers dev release workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants