add visual configurator (WP-1003)

[vsolovei-smartling]

No description provided.

[PavelLoparev]

Did we reflect this in some "meta" file in connector? does it affect minimum WP version we support from now?

[vsolovei-smartling]

Yes, https://github.com/Smartling/wordpress-localization-plugin/blob/master/smartling-connector.php#L22

[PavelLoparev]

Please add some screenshots or screencasts - how does it works and look like?

[PavelLoparev]

Code Review

Reviewed against production-tier standards. Jira: WP-1003.

---

🔴 Critical

1. No capability check on AJAX endpoints (VisualConfiguratorPage.php — all 4 handlers)

All handlers verify only the nonce (check_ajax_referer) but do not call current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_PROFILE_CAP). Any logged-in user who obtains a valid nonce (e.g. a subscriber visiting an admin page that embeds the nonce) can read, write, and delete rules. Add the capability check at the top of each ajax* method.

---

2. replacerId not validated against known allowlist (VisualConfiguratorPage.php:readRulePayload)

sanitize_text_field is the only guard. Arbitrary strings - including crafted pipe-separated values - can be persisted to storage and will later be passed to getReplacer() / parseReplacer() at download time with no server-side rejection. Fix: after stripping the |hint suffix, check the base ID against array_keys($this->replacerFactory->getListForUi()).

---

3. Unescaped output in AdminPage.php (lines 72-79)

admin_url() and __() are echoed raw via <?=. Wrap with esc_url() and esc_html__():

<a class="button button-primary" href="<?= esc_url(admin_url('admin.php?page=' . VisualConfiguratorPage::SLUG))?>">
    <?= esc_html__('Open Visual Configurator')?>

---

🟡 Warning

4. rulesByPath collision when two meta keys share the same JSON path (visual-configurator.js, lines ~1102-1107)

rulesByPath[r.propertyPath || r.metaKey] = r;  // keyed by path alone - overwrites when paths collide
rulesByPath[key] = r;

If rules exist for meta_key_a|$.items[*].id and meta_key_b|$.items[*].id, the second silently overwrites the first in the short-key slot. Remove the first write entirely; use only the composite metaKey|propertyPath key throughout, and update all lookup sites to match.

---

5. updateItem called without existence check (VisualConfiguratorPage.php:ajaxSaveRule)

When a non-empty id is submitted, updateItem($id, $data) is called regardless of whether the ID exists. If missing, behaviour depends on CustomizationManagerAbstract internals (silent create or silent no-op). Add a listItems() existence check and return 404 if absent.

---

6. Malformed rule entries silently swallowed with no log (JsonFieldRulesManager.php:listItems)

Corrupt stored entries are caught and skipped with no log output. In production this means an admin could save rules, observe they disappear, and have no diagnostic path. Add at minimum a warning log before continue.

---

7. loadData() called on every submission (ExternalContentJsonRules.php)

loadData() is called at the top of getContentFields, getRelatedContent, setContentFields, and removeUntranslatableFieldsForUpload. In a bulk download of N posts this triggers N storage reads for the same static rule set. Cache the loaded state per request (lazy-load once, guard with a flag).

---

8. No input length limit on propertyPath (VisualConfiguratorPage.php:readRulePayload)

An unbounded JSONPath expression passed to JsonObject::get() / getJsonObjects() could cause excessive recursion or ReDoS-class behaviour depending on the JsonPath library implementation. Add a max-length guard (~512 chars) in readRulePayload.

---

9. getJsonObjects return value guard is fragile (ExternalContentJsonRules.php:applyTranslateRule and applyRelatedRule)

Both methods guard === false but not null or other non-iterable scalars. Add || $objects === null before the array wrap.

---

10. confirm() for destructive delete action (visual-configurator.js:handleDeleteRule)

confirm() is blocked in iframe-embedded environments and returns false silently. Replace with a modal using the already-imported Modal from wp.components.

---

🔵 Suggestion

11. readMetaJson decodes JSON only to validate, then discards the result

json_decode is called and thrown away; callers re-parse the string via new JsonObject(...). Return the decoded value directly to avoid double-parsing.

12. replacerFactory->getListForUi() called twice per page load (VisualConfiguratorPage.php:enqueue + pageHandler)

Once for wp_localize_script and once for the data-replacer-options attribute on the root div. The JS reads only from window.smartlingVisualConfigurator.replacerOptions. The data- attribute is redundant - remove it.

13. getContentFields silently skips REPLACER_COPY fields

A user who configures a copy rule via the UI will see it listed but it will never be uploaded for translation, with no feedback. Either handle copy fields during upload or surface a note in the UI.

---

🟣 Questions

14. REST authentication for loadContent (visual-configurator.js)

restNonce is localized and passed as X-WP-Nonce header in the fetch() call to smartling-connector/v2. Confirm this is the correct nonce action expected by that REST endpoint.

15. removeUntranslatableFieldsForUpload strips ALL ruled meta keys regardless of rule type

A copy-type rule on _elementor_data would strip that key from the upload source array (preventing Elementor's own handler from seeing it, since json.rules runs last in the chain). Is this intentional?

16. WP options autoload for CUSTOM_JSON_FIELD_RULES

If CustomizationManagerAbstract stores this key with autoload enabled, large rule sets will bloat the options table on every WP page load. Confirm autoload is disabled.

---

Cross-cutting

• Authorization gap: All four AJAX endpoints rely solely on check_ajax_referer. Combine with current_user_can() (see item 1).
• Unminified JS shipped to production: visual-configurator.js is raw ES2015 IIFE. Run it through @wordpress/scripts build for minification and proper dependency verification.
• Handler registration cost: content.json.rules calls loadData() inside getSupportLevel(), meaning storage is read for every content type check even when no rules are configured. Guard with a listItems() !== [] short-circuit that defers I/O.

---

Ready to merge: No

Items 1-3 must be resolved before merge (capability bypass, replacer injection, XSS). Items 4-5 are correctness bugs worth fixing in the same PR.

[vsolovei-smartling]

Please add some screenshots or screencasts - how does it works and look like?

Demoed

[vsolovei-smartling]

Code Review

Reviewed against production-tier standards. Jira: WP-1003.

🔴 Critical

1. No capability check on AJAX endpoints (VisualConfiguratorPage.php — all 4 handlers)

All handlers verify only the nonce (check_ajax_referer) but do not call current_user_can(SmartlingUserCapabilities::SMARTLING_CAPABILITY_PROFILE_CAP). Any logged-in user who obtains a valid nonce (e.g. a subscriber visiting an admin page that embeds the nonce) can read, write, and delete rules. Add the capability check at the top of each ajax* method.

Created https://smartling.atlassian.net/browse/WP-1007

2. replacerId not validated against known allowlist (VisualConfiguratorPage.php:readRulePayload)

sanitize_text_field is the only guard. Arbitrary strings - including crafted pipe-separated values - can be persisted to storage and will later be passed to getReplacer() / parseReplacer() at download time with no server-side rejection. Fix: after stripping the |hint suffix, check the base ID against array_keys($this->replacerFactory->getListForUi()).

No issue, both getReplacer and parseReplacer deal with unknown replacers properly

3. Unescaped output in AdminPage.php (lines 72-79)

admin_url() and __() are echoed raw via <?=. Wrap with esc_url() and esc_html__():

<a class="button button-primary" href="<?= esc_url(admin_url('admin.php?page=' . VisualConfiguratorPage::SLUG))?>">
    <?= esc_html__('Open Visual Configurator')?>

Done.

🟡 Warning

4. rulesByPath collision when two meta keys share the same JSON path (visual-configurator.js, lines ~1102-1107)

rulesByPath[r.propertyPath || r.metaKey] = r;  // keyed by path alone - overwrites when paths collide
rulesByPath[key] = r;

If rules exist for meta_key_a|$.items[*].id and meta_key_b|$.items[*].id, the second silently overwrites the first in the short-key slot. Remove the first write entirely; use only the composite metaKey|propertyPath key throughout, and update all lookup sites to match.

Done.

5. updateItem called without existence check (VisualConfiguratorPage.php:ajaxSaveRule)

When a non-empty id is submitted, updateItem($id, $data) is called regardless of whether the ID exists. If missing, behaviour depends on CustomizationManagerAbstract internals (silent create or silent no-op). Add a listItems() existence check and return 404 if absent.

Done.

6. Malformed rule entries silently swallowed with no log (JsonFieldRulesManager.php:listItems)

Corrupt stored entries are caught and skipped with no log output. In production this means an admin could save rules, observe they disappear, and have no diagnostic path. Add at minimum a warning log before continue.

Done.

7. loadData() called on every submission (ExternalContentJsonRules.php)

loadData() is called at the top of getContentFields, getRelatedContent, setContentFields, and removeUntranslatableFieldsForUpload. In a bulk download of N posts this triggers N storage reads for the same static rule set. Cache the loaded state per request (lazy-load once, guard with a flag).

No issue, retrieving from db takes a fraction of the time required to download the translated file.

8. No input length limit on propertyPath (VisualConfiguratorPage.php:readRulePayload)

An unbounded JSONPath expression passed to JsonObject::get() / getJsonObjects() could cause excessive recursion or ReDoS-class behaviour depending on the JsonPath library implementation. Add a max-length guard (~512 chars) in readRulePayload.

Done.

9. getJsonObjects return value guard is fragile (ExternalContentJsonRules.php:applyTranslateRule and applyRelatedRule)

Both methods guard === false but not null or other non-iterable scalars. Add || $objects === null before the array wrap.

Done.

10. confirm() for destructive delete action (visual-configurator.js:handleDeleteRule)

confirm() is blocked in iframe-embedded environments and returns false silently. Replace with a modal using the already-imported Modal from wp.components.

Done.

🔵 Suggestion

11. readMetaJson decodes JSON only to validate, then discards the result

json_decode is called and thrown away; callers re-parse the string via new JsonObject(...). Return the decoded value directly to avoid double-parsing.

Done.

12. replacerFactory->getListForUi() called twice per page load (VisualConfiguratorPage.php:enqueue + pageHandler)

Once for wp_localize_script and once for the data-replacer-options attribute on the root div. The JS reads only from window.smartlingVisualConfigurator.replacerOptions. The data- attribute is redundant - remove it.

Done.

13. getContentFields silently skips REPLACER_COPY fields

A user who configures a copy rule via the UI will see it listed but it will never be uploaded for translation, with no feedback. Either handle copy fields during upload or surface a note in the UI.

That's what copy does.