feat(IDE-1701): settings page auth flow — bridge persist and forward apiUrl#453
Conversation
…rotocol version to 25 [IDE-1701] Replace __ideLogin__/__ideLogout__ COM methods with a generic __ideExecuteCommand__ bridge that dispatches any LS command with callback support via window.__ideCallbacks__. Bump ProtocolVersion to 25.
…use [IDE-1701] Move BuildClientScript() and DispatchAsync() into a standalone ExecuteCommandBridge class. HtmlSettingsScriptingBridge delegates dispatch and HtmlSettingsWindow uses BuildClientScript() for injection, enabling any future WebBrowser panel (e.g. tree view) to reuse the same bridge.
… flag - Remove persist flag from OnHasAuthenticated — always saves endpoint + token - Keep Save(options, false) to avoid DidChangeConfigurationAsync loop - Keep isNewLogin guard — only triggers HandleAuthenticationSuccess on first login - Keep UpdateAuthToken(token, apiUrl) — settings page webview always updated - Add bridge persist in HtmlSettingsScriptingBridge.__ideExecuteCommand__: when snyk.login called with 3+ args, save authMethod/endpoint/ignoreUnknownCA to options properties directly (no Save() call → no SettingsChanged event → no DidChangeConfigurationAsync to LS) - Remove OnHasAuthenticated_NoPersist test; rename persist-prefixed tests - Add HtmlSettingsScriptingBridgeTest with 6 tests for login args persist
✅ 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. |
…ebview Pass apiUrl alongside token when calling window.setAuthToken via InvokeSetAuthToken so the settings page can update both the token and apiUrl fields after auth.
- Add callbackId XSS allowlist guard (regex ^(__cb_\d+)?$) in ExecuteCommandBridge.IsValidCallbackId, used in DispatchAsync and InvokeCommandCallback - Fix JS string escaping order in InvokeSetAuthToken: escape backslash before single-quote - Add volatile keyword to HtmlSettingsWindow singleton instance for cross-thread visibility - Clear stored token when auth method changes in ParseAndSaveConfigAsync to prevent stale token from one method being used with another - Add tests for all four fixes
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.
This comment has been minimized.
This comment has been minimized.
The LS HTML page checks window.__ideExecuteCommand__ during its own initialization scripts. Injecting only in LoadCompleted (after the page has already parsed and run its scripts) means the auth button is never wired up, causing it to do nothing when clicked. Now inject bridge functions directly into the HTML string before NavigateToString so they are defined before any LS page scripts run. The LoadCompleted injection is kept as a secondary safety net.
f63c7b5 to
af40ad6
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.
This comment has been minimized.
This comment has been minimized.
| var script = BuildIdeBridgeScript(); | ||
| var scriptTag = $"<script type=\"text/javascript\">{script}</script>"; | ||
|
|
||
| var headIndex = html.IndexOf("<head>", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Tiny nitpick: "" should probably be extracted to a variable as we have two places (here and line 193) where we rely on the string's exact contents.
There was a problem hiding this comment.
Not strictly necessary in this case given the close proximity of the two use cases, but I think it's good practice.
| doc.GetElementsByTagName("head")[0].AppendChild(scriptElement); | ||
| }); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Any reason for wrapping the run in a try/catch instead of the other way around?
| } | ||
|
|
||
| var escapedToken = token.Replace("'", "\\'").Replace("\"", "\\\""); | ||
| var escapedToken = token.Replace("\\", "\\\\").Replace("'", "\\'"); |
There was a problem hiding this comment.
Same comment as the PRs for the other IDEs; might be worth extracting the escape logic into a utility function.
| [InlineData("__cb_abc")] | ||
| [InlineData(";drop table--")] | ||
| [InlineData("__cb_1; alert(1)")] | ||
| [InlineData("__cb_")] |
There was a problem hiding this comment.
Might also be worth adding a line for "_cb_8" (i.e, with only a single leading underscore)
There was a problem hiding this comment.
Also, a blank but non-empty string: " ".
| } | ||
|
|
||
| [Fact] | ||
| public void IdeExecuteCommand_SnykLogin_SavesOAuthMethod() |
There was a problem hiding this comment.
Please add a test for the default (e.g, checking that we set oauth when an incorrect, blank, or empty string is passed)
- Extract <head> tag string to constant in InjectBridgeScriptIntoHtml - Move callbackId validation outside ThreadHelper.Run in InvokeCommandCallback - Extract JS string escaping into ExecuteCommandBridge.EscapeForJsString utility - Add _cb_8 and whitespace test cases for IsValidCallbackId - Add test for default OAuth fallback on invalid auth method string
PR Reviewer Guide 🔍
|
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 HTML settings window so the settings page can update both fields after auth.Bridge persist writes to option properties directly — no
Save()call meansSettingsChangednever fires,DidChangeConfigurationAsyncis not triggered.Changes
HtmlSettingsScriptingBridge.cs— bridge persistIn
__ideExecuteCommand__, before dispatchingsnyk.loginto the LS, whenargs.Length >= 3:Options.AuthenticationMethod(C# switch:"oauth"→OAuth,"pat"→Pat,"token"→Token)Options.CustomEndpointfromargs[1]Options.IgnoreUnknownCAfromargs[2]Save()call — properties written in-memory only, noSettingsChangedevent, noDidChangeConfigurationAsyncSnykLanguageClientCustomTarget.cs—OnHasAuthenticatedCustomEndpoint(when non-empty) andApiTokenSave(serviceProvider.Options, false)—falsesuppressesSettingsChanged→ noDidChangeConfigurationAsyncloopHtmlSettingsWindow.Instance?.UpdateAuthToken(token, apiUrl)— always updates webview so settings page shows both fields immediatelyisNewLoginguard (string.IsNullOrEmpty(oldToken) && !string.IsNullOrEmpty(token)) gatesHandleAuthenticationSuccess+ scan — token refreshes skip scanningHtmlSettingsWindow.xaml.csUpdateAuthToken(string token, string apiUrl = null)passesapiUrlthrough towindow.setAuthToken(escapedToken, escapedApiUrl).Tests
SnykLanguageClientCustomTargetTests.cs: new tests for new login (verifies endpoint + token saved,Save(_, false),HandleAuthenticationSuccesscalled), token refresh (quiet save, noHandleAuthenticationSuccess), and always-updates-webview casesHtmlSettingsScriptingBridgeTest.cs(new): 7 tests — auth method mapping (oauth/pat/token), endpoint, insecure, no-op on <3 args, no-op on non-login commandTest plan
DidChangeConfigurationAsyncfired when bridge saves auth params