feat(IDE-1701): settings page auth flow — bridge persist and forward apiUrl#366
Merged
nick-y-snyk merged 12 commits intomainfrom Mar 27, 2026
Merged
Conversation
…rotocol version to 25 [IDE-1701] Replace __ideLogin__/__ideLogout__ BrowserFunctions with a generic __ideExecuteCommand__ bridge that dispatches any LS command with callback support. Bump REQUIRED_LS_PROTOCOL_VERSION to 25.
…use [IDE-1701] Move the __ideExecuteCommandBridge__ BrowserFunction and JS wrapper injection into a standalone ExecuteCommandBridge class. HTMLSettingsPreferencePage delegates to ExecuteCommandBridge.install(browser), enabling any future SWT Browser panel (e.g. tree view) to reuse the same bridge.
… flag - Remove persist field from HasAuthenticatedParam — always saves token and apiUrl - Restore original hasAuthenticated flow: always stores endpoint + token, calls configurationUpdater.configurationChanged(), triggers scan when conditions met - Keep notifyAuthTokenChanged(token, apiUrl) with apiUrl param — settings page webview always shows current token after auth - Add bridge persist in ExecuteCommandBridge.registerBridgeFunction: when snyk.login called with 3+ args, save authMethod/endpoint/insecure to Preferences directly (no configurationChanged() → no didChangeConfiguration to LS) - Remove setPersist() calls and persist-related tests from SnykExtendedLanguageClientTest - Add saveLoginArgs() tests to ExecuteCommandBridgeTest covering auth method mapping, endpoint, and insecure saving
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
… page browser Pass apiUrl alongside token when calling window.setAuthToken in the browser so the settings page can update both the token and apiUrl fields after auth.
| prefs.store(Preferences.AUTHENTICATION_METHOD, authMethod); | ||
| prefs.store(Preferences.ENDPOINT_KEY, endpoint); | ||
| prefs.store(Preferences.INSECURE_KEY, insecure); | ||
| } catch (Exception e) { |
Check warning
Code scanning / PMD
Avoid catching Exception in try-catch block Warning
| List<Object> args; | ||
| try { | ||
| args = Arrays.asList(OBJECT_MAPPER.readValue(argsJson, Object[].class)); | ||
| } catch (Exception e) { |
Check warning
Code scanning / PMD
Avoid catching Exception in try-catch block Warning
| browser.evaluate(script); | ||
| } | ||
| }); | ||
| } catch (Exception e) { |
Check warning
Code scanning / PMD
Avoid catching Exception in try-catch block Warning
This comment has been minimized.
This comment has been minimized.
- Add singleton + notifyAuthTokenChanged to native PreferencesPage so the SWT token field is updated live when hasAuthenticated fires, not just the HTML webview - Reorder hasAuthenticated() to update UIs before persisting to storage, avoiding race conditions where a settings-changed event re-reads stale values - Escape token and apiUrl before JS interpolation in notifyAuthTokenChanged to guard against single-quote injection - Fix dispose() identity check (== instead of .equals()) in both settings pages
This comment has been minimized.
This comment has been minimized.
…to compatibility - Add public updateToken() method to TokenFieldEditor to expose setStringValue() from outside the class (setStringValue is protected in StringFieldEditor) - Fix test failures on Java 21 by adding -XX:+EnableDynamicAgentLoading to tycho-surefire-plugin argLine, required for mockito-inline 4.5.1 to do byte-buddy instrumentation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ion in ExecuteCommandBridge
440f31d to
fb0dcc8
Compare
This comment has been minimized.
This comment has been minimized.
Prevents XSS-to-arbitrary-command escalation by rejecting any command not prefixed with "snyk." before it reaches the Language Server.
andrewrobinsonhodges-snyk
approved these changes
Mar 26, 2026
| if (instance != null | ||
| && instance.browser != null | ||
| && !instance.browser.isDisposed()) { | ||
| String safeToken = token.replace("\\", "\\\\").replace("'", "\\'"); |
Contributor
There was a problem hiding this comment.
We could consider pulling the escaping logc out into a utility function
| } | ||
|
|
||
| @Test | ||
| void saveLoginArgs_savesOauthAuthMethod() { |
Contributor
There was a problem hiding this comment.
Add test for default (this is implicitly testing it; better to be explicit)
Pull escapeForJsString() into ExecuteCommandBridge and replace inline escaping in both ExecuteCommandBridge and HTMLSettingsPreferencePage. Add explicit test for default (oauth) auth method fallback.
PR Reviewer Guide 🔍
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & Why
The settings page drives authentication via
snyk.loginwith[authMethod, endpoint, insecure]args. Before forwarding to the LS, the IDE must persist these values locally so they survive a page close. Additionally,$/snyk.hasAuthenticatednow carriesapiUrlwhich is forwarded to the settings browser so the settings page can update both fields after auth.Bridge persist calls
Preferences.store()only — noconfigurationChanged()means nodidChangeConfigurationloop.Changes
ExecuteCommandBridge.java(refactored to shared class, with new persist logic)saveLoginArgs(List<Object> args)(package-private for testability) called beforeCommandHandler.getInstance().executeCommand(...)when"snyk.login".equals(command) && args.size() >= 3:args.get(0):"pat"→AUTH_PERSONAL_ACCESS_TOKEN,"token"→AUTH_API_TOKEN, default →AUTH_OAUTH2prefs.store(Preferences.AUTHENTICATION_METHOD, authMethod)prefs.store(Preferences.ENDPOINT_KEY, endpoint)prefs.store(Preferences.INSECURE_KEY, String.valueOf(insecure))configurationChanged()call → nodidChangeConfigurationsent to LSSnykExtendedLanguageClient.java—hasAuthenticatedCalls
HTMLSettingsPreferencePage.notifyAuthTokenChanged(newToken, param.getApiUrl())when token changes — passesapiUrlalongside token so the settings browser updates both fields.HTMLSettingsPreferencePage.javanotifyAuthTokenChanged(String token, String apiUrl)— passesapiUrltowindow.setAuthToken(token, apiUrl)in the browser evaluate call.Tests
ExecuteCommandBridgeTest.java: 5 newsaveLoginArgstests (oauth/pat/token mapping, endpoint stored, insecure stored as string"true"); class now extendsLsBaseTestforprefsfixtureTest plan
didChangeConfigurationfired when bridge saves auth paramsmvn test