feat(IDE-1652): migrate HTML settings page onto FolderConfigSettings + ExecuteCommandBridge (3/3) by nick-y-snyk · Pull Request #396 · snyk/snyk-eclipse-plugin

nick-y-snyk · 2026-05-18T12:53:06Z

Summary

Third and final new PR in the feat/IDE-1652 split. Migrates the HTML settings page to the new store introduced in PR #395. After this lands, all legacy POJOs and native UI shells are dead code with no callers — PR #393 deletes them.

HTMLSettingsPreferencePage — rewired to read/write settings via FolderConfigSettings and LsSettingsRegistry instead of the legacy Settings / IdeConfigData path; bridges auth and settings updates through ExecuteCommandBridge
settings-fallback.html — synced from snyk-ls shared IDE resources with updated JS wiring for new settings bridge commands
HTMLSettingsPreferencePageTest — updated to cover new wiring

Dead code after this PR (deleted by PR #393)

Settings, FolderConfig, FolderConfigsParam, FolderConfigs, IdeConfigData, NativeProjectPropertyPage, PreferencesPage, TokenFieldEditor, LsRuntimeEnvironment env-var path, Preferences native getters.

PR sequence

PR	Scope
1/3 — PR #394	Foundation: registry, protocol POJOs
2/3 — PR #395	Wiring: inbound handler, outbound updater, folder settings store
This PR (3/3)	HTML settings page migration
PR #393	All deletions: legacy POJOs + native UI

Test plan

./mvnw test -pl plugin,tests passes
./mvnw pmd:check -pl plugin — zero violations
Smoke: Eclipse Snyk preferences open (HTML page), auth flow completes, settings persist across restart

…and ExecuteCommandBridge (3/3) - HTMLSettingsPreferencePage: rewired to read/write settings via FolderConfigSettings and LsSettingsRegistry instead of the legacy Settings / IdeConfigData path; bridges auth and settings updates through ExecuteCommandBridge - settings-fallback.html: sync from snyk-ls shared_ide_resources with updated JS wiring for new settings bridge commands - HTMLSettingsPreferencePageTest: updated to cover new wiring After this PR all legacy POJOs (Settings, FolderConfig, FolderConfigs, FolderConfigsParam, IdeConfigData) and native UI shells (NativeProjectPropertyPage, PreferencesPage, TokenFieldEditor) are dead code with no callers. PR #393 deletes them.

snyk-io · 2026-05-18T12:53:19Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

…cture Deletes all code made dead by the new LsSettingsRegistry / FolderConfigSettings / LspConfigurationParam architecture landed in PRs #394, #395, #396: Native settings UI (no callers after HTML-only migration): - PreferencesPage, TokenFieldEditor, NativeProjectPropertyPage - SnykPreferencePageFactoryTest, ProjectPropertyPageFactoryTest Legacy LS protocol POJOs (replaced by LspConfigurationParam / ConfigSetting / LspFolderConfig / FolderConfigSettings): - Settings, FolderConfig, FolderConfigsParam, FolderConfigs, IdeConfigData - FolderConfigTest

acke · 2026-05-20T11:33:10Z

+          prefs.markExplicitlyChangedNoFlush(entry.prefKey);
+        } else {
+          prefs.store(entry.prefKey, nodeToString(n));
+          prefs.markExplicitlyChangedNoFlush(entry.prefKey);


Won't fix in this PR. The fallback HTML is owned by snyk-ls/shared_ide_resources/ — the fix (send only diff, not all fields) belongs there. Tracked in IDE-2072.

acke · 2026-05-20T11:34:30Z

+      var fields = folderNode.fields();
+      while (fields.hasNext()) {
+        var field = fields.next();
+        String key = field.getKey();


Won't fix here — if the LS changes the folder key contract (e.g. folderPath → folder_path), the plugin will need updating regardless. Eclipse has no LS integration tests currently, so the failure mode is a silent regression caught only by manual testing. Raising a separate ticket to add LS integration test coverage is the right long-term fix.

acke · 2026-05-20T11:40:09Z

+        cli_path: get('cli_path').value,
+        automatic_download: get('automatic_download').checked,
+        binary_base_url: get('binary_base_url').value || get('binary_base_url').placeholder,
+        cli_release_channel: get('cli_release_channel').value === 'custom' ? (function() { var v = (get('cli_release_channel_custom').value || '').trim(); if (v && v.charAt(0) !== 'v') { v = 'v' + v; } return v || 'stable'; })() : get('cli_release_channel').value,


settings-fallback.html is owned by snyk-ls/shared_ide_resources/. This behavior is the LS's concern.

acke · 2026-05-20T11:44:27Z

+          <option value="preview" {{CHANNEL_PREVIEW_SELECTED}}>Preview</option>
+          <option value="custom" {{CHANNEL_CUSTOM_SELECTED}}>Specify version…</option>
+        </select>
+        <input type="text" id="cli_release_channel_custom" name="cli_release_channel_custom" class="{{CLI_RELEASE_CHANNEL_CUSTOM_HIDDEN}}" placeholder="e.g. v1.1292.0" value="{{CLI_RELEASE_CHANNEL_CUSTOM_VALUE}}" maxlength="20" pattern="^v?(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)$">


Won't fix here — settings-fallback.html is synced from snyk-ls/shared_ide_resources/. The placeholder substitution ({{CHANNEL_CUSTOM_SELECTED}} etc.) belongs in the LS repo. Eclipse ships the file as-is.

acke · 2026-05-20T11:55:29Z

+          continue;
+        }
+        if (LsFolderSettingsKeys.SCAN_COMMAND_CONFIG.equals(key)) {
+          Map<String, ScanCommandConfig> scanCommandMap = objectMapper.convertValue(


Fixed — added isObject() guard before convertValue and wrapped in try/catch IllegalArgumentException to isolate malformed scan_command_config to the single folder without aborting the rest of the save.

acke · 2026-05-20T11:55:29Z

-      <label for="cliPath"><span data-toggle="tooltip">CLI Path<span class="tooltip-popup">Path to Snyk CLI executable</span></span></label>
-      <input type="text" id="cliPath" name="cliPath" value="{{CLI_PATH}}" placeholder="/path/to/snyk-cli">
+      <label for="cli_path"><span data-toggle="tooltip">CLI Path<span class="tooltip-popup">Path to Snyk CLI executable</span></span></label>
+      <input type="text" id="cli_path" name="cli_path" value="{{CLI_PATH}}" placeholder="/path/to/snyk-cli">


Fixed — added htmlAttr() helper that escapes &, ", < and applied it to {{CLI_PATH}} and {{CLI_BASE_DOWNLOAD_URL}} substitutions in loadFallbackHtml().

acke · 2026-05-20T11:55:29Z

+      }
+    }
+
+    function validateCliVersion() {


Won't fix here — settings-fallback.html is owned by snyk-ls/shared_ide_resources/. JS validation logic belongs there.

acke · 2026-05-20T11:55:30Z

🟠 High — Posted as a PR-level comment because the affected file (IdeConfigData.java) is not in this PR's diff.

[plugin/src/main/java/io/snyk/eclipse/plugin/preferences/IdeConfigData.java] PR description states "After this lands, all legacy POJOs are dead code with no callers — PR #393 deletes them." Verified locally: after this diff is applied, grep -r IdeConfigData plugin/src/main/java/ returns only IdeConfigData.java itself — HTMLSettingsPreferencePage is the only consumer and this PR removes every reference.

Leaving the POJO in the tree (with its @JsonIgnoreProperties annotations and nested records IssueViewOptions, FilterSeverity, ScanCommandConfigData, FolderConfigData) is a maintenance trap — future devs reading the file will mistake it for the active wire contract and may add fields here, expecting them to be parsed. Cross-PR dead-code coupling (delete-in-#393 / use-from-#395-#396) is brittle if PR ordering changes.

Fix:

Prefer to delete plugin/src/main/java/io/snyk/eclipse/plugin/preferences/IdeConfigData.java in this PR — it's a fully self-contained POJO with no remaining callers once this diff lands, so the deletion is independent of PR #393's broader cleanup.

If you must defer to PR #393, add a top-of-file marker so the next reader understands the state:

// @deprecated Slated for deletion in PR #393. No remaining callers after PR #396
//             (HTMLSettingsPreferencePage migrated to JsonNode-driven parsing).
//             Do not add new fields.

— AI review comment

Converting to non-blocking review — findings remain visible inline and in the review body, but should not block merge.

…cture Deletes all code made dead by the new LsSettingsRegistry / FolderConfigSettings / LspConfigurationParam architecture landed in PRs #394, #395, #396: Native settings UI (no callers after HTML-only migration): - PreferencesPage, TokenFieldEditor, NativeProjectPropertyPage - SnykPreferencePageFactoryTest, ProjectPropertyPageFactoryTest Legacy LS protocol POJOs (replaced by LspConfigurationParam / ConfigSetting / LspFolderConfig / FolderConfigSettings): - Settings, FolderConfig, FolderConfigsParam, FolderConfigs, IdeConfigData - FolderConfigTest

…-migration

…ception Non-object nodes and null boolean fields throw IllegalArgumentException which the outer JsonProcessingException catch misses, silently aborting the whole save. Guard with isObject() check and isolate the failure to the single folder.

Pref values substituted into HTML value= attributes with no escaping — a stored value containing "><script> would render in the privileged SWT Browser. Added htmlAttr() helper (escapes & , ", <) and applied to CLI_PATH and CLI_BASE_DOWNLOAD_URL substitutions.

nick-y-snyk · 2026-05-21T11:51:20Z

Re: #396 (comment)

Already deleted in the follow-up PR #393 — no action needed here.

snyk-pr-review-bot · 2026-05-21T11:53:16Z

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Data clearing blocked 🟡 [minor] In `collectData()`, the `binary_base_url` field uses a logical OR with the placeholder: `get('binary_base_url').value \|\| get('binary_base_url').placeholder`. This makes it impossible for a user to explicitly clear this setting via the fallback form to revert to a global default, as an empty input will always send the placeholder string ('https://downloads.snyk.io') instead of a null or empty value that the backend could use to trigger `prefs.clearExplicitlyChangedNoFlush`. binary_base_url: get('binary_base_url').value \|\| get('binary_base_url').placeholder,
📚 Repository Context Analyzed This review considered 32 relevant code sections from 12 files (average relevance: 0.91) plugin/src/main/java/io/snyk/eclipse/plugin/preferences/PreferencesPage.java (2 segments, lines 101-200) plugin/src/main/java/io/snyk/eclipse/plugin/preferences/SnykPreferencePageFactory.java (lines 1-24) plugin/src/main/java/io/snyk/eclipse/plugin/properties/NativeProjectPropertyPage.java (3 segments, lines 1-100) plugin/src/main/java/io/snyk/eclipse/plugin/html/StaticPageHtmlProvider.java (lines 101-187) plugin/src/main/java/io/snyk/eclipse/plugin/views/snyktoolview/BrowserHandler.java (lines 1-100) plugin/src/main/java/io/snyk/languageserver/protocolextension/SnykExtendedLanguageClient.java (lines 1-100) plugin/src/main/java/io/snyk/eclipse/plugin/html/ExecuteCommandBridge.java (lines 1-100) ... and 5 more file(s)

acke

Re-review (review-orchestrator)

Verdict: APPROVE

Verification of prior comments

#	File:Line	Status
1	`HTMLSettingsPreferencePage.java:198` fallback HTML diff-only save	⏸️ Declined — owned by `snyk-ls/shared_ide_resources/` (IDE-2072)
2	`HTMLSettingsPreferencePage.java:244` LS folder-key test gap	⏸️ Declined — separate ticket
3	`settings-fallback.html:376` fallback UX	⏸️ Declined — owned upstream
4	`settings-fallback.html:345` placeholder	⏸️ Declined — owned upstream
5	`HTMLSettingsPreferencePage.java:250` scan_command_config	✅ Fixed (`ac6e1ef`) — `isObject()` guard + try/catch
6	`HTMLSettingsPreferencePage.java:159-160` XSS in loadFallbackHtml	✅ Fixed (`9df85ea`) — `htmlAttr()` helper. See M1 — escape set incomplete
7	`settings-fallback.html:461` JS validation	⏸️ Declined — owned upstream
8	PR-level: `IdeConfigData.java` orphan	⏸️ Deferred to #393 — verified deleted there ✓

New pass findings (4)

🟡 Medium HTMLSettingsPreferencePage.java:278-281 — htmlAttr escapes &/"/< only; safe for double-quoted attribute context but brittle if reused
🟡 Medium HTMLSettingsPreferencePage.java:312-326 — performOk races between browser.evaluate and runAsync(updateConfiguration) if the JS save handler is async
🔵 Low HTMLSettingsPreferencePage.java:270-272 — processFolderConfig silently flattens unknown folder field types to text
🔵 Low HTMLSettingsPreferencePage.java:155-169 — loadFallbackHtml re-reads the template per page load (trivial cost)

Important cross-PR note: this PR's outbound flow goes through the form parser (not persistGlobalSettings), so the allowlist concern raised on #395 does NOT affect it.

See inline comments for full block-formatted findings.

Manifest: .claude/reviews/396/TOOLS_USED.md
— AI review comment

acke · 2026-05-21T15:47:38Z

+  private static String htmlAttr(String v) {
+    if (v == null) return "";
+    return v.replace("&", "&amp;").replace("\"", "&quot;").replace("<", "&lt;");
  }


🟡 Medium

[HTMLSettingsPreferencePage.java:278-281] htmlAttr escapes &, ", < but not > or '. For attribute values inside double-quoted HTML this is technically sufficient, but it's a brittle invariant — if a future template uses single-quoted attributes or embeds the value in textContent/URL/JS context, the helper underescapes.

Fix:
Either inline-document the constraint:

/** Escapes for double-quoted HTML attribute context only. Do not use in JS/URL contexts. */ private static String htmlAttr(String v) { ... }

Or switch to org.apache.commons.text.StringEscapeUtils.escapeHtml4(v) and rename to htmlEscape.

— AI review comment

acke · 2026-05-21T15:47:40Z

+        } else if (node.isBoolean()) {
+          config = config.withSetting(key, node.booleanValue(), true);
+        } else {
+          config = config.withSetting(key, node.asText(), true);


🔵 Low

[HTMLSettingsPreferencePage.java:270-272] processFolderConfig stores unknown folder field types as raw text. If a future LS adds a structured folder field and the registry hasn't been updated, the value gets flattened to a JSON string. Symptom would be confusing downstream type errors rather than a clear "unknown key" log.

Fix:
Add an else warn log:

} else { SnykLogger.logInfo("processFolderConfig: storing unknown field type as text for key=" + key + " jsonType=" + node.getNodeType()); config = config.withSetting(key, node.asText(), true); }

— AI review comment

acke · 2026-05-21T15:47:42Z

-              prefs.getPref(Preferences.CLI_BASE_URL, "https://downloads.snyk.io"))
-          .replace("{{CLI_PATH}}", prefs.getCliPath())
+              htmlAttr(prefs.getPref(Preferences.CLI_BASE_URL, "https://downloads.snyk.io")))
+          .replace("{{CLI_PATH}}", htmlAttr(prefs.getCliPath()))


🔵 Low

[HTMLSettingsPreferencePage.java:155-169] loadFallbackHtml reads the template once per page load. If the user opens settings repeatedly during a session, getResourceAsStream reopens the jar each time and re-substitutes. Trivial cost, but caching the template + lazy-substituting the dynamic placeholders would mirror typical Eclipse patterns. Skip if you've measured no perf concern.

— AI review comment

acke · 2026-05-21T15:50:22Z

🟡 Medium

[HTMLSettingsPreferencePage.java:312-326] (Posted as PR-level comment because target line is outside this PR's diff hunks — performOk was untouched.)

performOk calls browser.evaluate("…getAndSaveIdeConfig()…") and then fires lc.updateConfiguration() via runAsync. Safe only if getAndSaveIdeConfig invokes __saveIdeConfig__ synchronously — if the JS uses any async primitive (await fetch, Promise.then, setTimeout), browser.evaluate returns before parseAndSaveConfig runs, and the outbound didChangeConfiguration ships with stale prefs.

The risk is non-obvious because the LS-served HTML lives outside this repo and could change.

Fix:
Have __saveIdeConfig__ return a sentinel (e.g. "OK"), capture it via browser.evaluate, and only call updateConfiguration if the save acknowledged. Or move updateConfiguration inside parseAndSaveConfig (last step) so it can never race.

— AI review comment

acke

APPROVED

nick-y-snyk requested a review from a team as a code owner May 18, 2026 12:53

This comment has been minimized.

Sign in to view

nick-y-snyk mentioned this pull request May 18, 2026

refactor(IDE-1652): remove native settings UI and legacy FolderConfig infrastructure #393

Open

2 tasks

acke previously requested changes May 20, 2026

View reviewed changes

acke reviewed May 20, 2026

View reviewed changes

Merge branch 'feat/IDE-1652-pr-b-wiring' into feat/IDE-1652-pr-c-html…

1148cd9

…-migration

This comment has been minimized.

Sign in to view

nick-y-snyk added 2 commits May 21, 2026 13:46

nick-y-snyk requested a review from acke May 21, 2026 11:52

acke mentioned this pull request May 21, 2026

feat(IDE-1652): add inbound $/snyk.configuration handler and outbound updater (2/3) #395

Open

3 tasks

acke reviewed May 21, 2026

View reviewed changes

acke approved these changes May 21, 2026

View reviewed changes

Conversation

nick-y-snyk commented May 18, 2026

Summary

Dead code after this PR (deleted by PR #393)

PR sequence

Test plan

Uh oh!

snyk-io Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

This comment has been minimized.

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke May 20, 2026 • edited by nick-y-snyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acke commented May 20, 2026

Uh oh!

This comment has been minimized.

nick-y-snyk commented May 21, 2026

Uh oh!

snyk-pr-review-bot Bot commented May 21, 2026

PR Reviewer Guide 🔍

Uh oh!

acke left a comment

Choose a reason for hiding this comment

Re-review (review-orchestrator)

Verification of prior comments

New pass findings (4)

Uh oh!

acke May 21, 2026

Choose a reason for hiding this comment

Uh oh!

acke May 21, 2026

Choose a reason for hiding this comment

Uh oh!

acke May 21, 2026

Choose a reason for hiding this comment

Uh oh!

acke commented May 21, 2026

Uh oh!

acke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snyk-io Bot commented May 18, 2026 •

edited

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading

acke May 20, 2026 •

edited by nick-y-snyk

Loading