fix: address PR review feedback for executeCommand bridge

nick-y-snyk · nick-y-snyk · commit 5764fec9d206 · 2026-03-30T11:56:13.000+02:00
- Extract &lt;head&gt; 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
diff --git a/Snyk.VisualStudio.Extension.2022/Settings/HtmlSettingsWindow.xaml.cs b/Snyk.VisualStudio.Extension.2022/Settings/HtmlSettingsWindow.xaml.cs
@@ -187,10 +187,11 @@ protected virtual string InjectBridgeScriptIntoHtml(string html)
             var script = BuildIdeBridgeScript();
             var scriptTag = $"<script type=\"text/javascript\">{script}</script>";
 
-            var headIndex = html.IndexOf("<head>", StringComparison.OrdinalIgnoreCase);
+            const string headTag = "<head>";
+            var headIndex = html.IndexOf(headTag, StringComparison.OrdinalIgnoreCase);
             if (headIndex >= 0)
             {
-                return html.Insert(headIndex + "<head>".Length, scriptTag);
+                return html.Insert(headIndex + headTag.Length, scriptTag);
             }
 
             // Fallback: prepend if no <head> found
@@ -291,6 +292,12 @@ public void UpdateAuthToken(string token, string apiUrl = null)
 
         private void InvokeCommandCallback(string callbackId, string resultJson)
         {
+            if (!ExecuteCommandBridge.IsValidCallbackId(callbackId))
+            {
+                Logger.Warning("Rejected callbackId with unexpected format: {CallbackId}", callbackId);
+                return;
+            }
+
             try
             {
                 ThreadHelper.JoinableTaskFactory.Run(async () =>
@@ -300,13 +307,7 @@ private void InvokeCommandCallback(string callbackId, string resultJson)
                     dynamic doc = SettingsBrowser.Document;
                     if (doc == null) return;
 
-                    if (!ExecuteCommandBridge.IsValidCallbackId(callbackId))
-                    {
-                        Logger.Warning("Rejected callbackId with unexpected format: {CallbackId}", callbackId);
-                        return;
-                    }
-
-                    var escapedCallbackId = callbackId.Replace("\\", "\\\\").Replace("'", "\\'");
+                    var escapedCallbackId = ExecuteCommandBridge.EscapeForJsString(callbackId);
                     var script = $"if(window.__ideCallbacks__&&window.__ideCallbacks__['{escapedCallbackId}']){{" +
                                  $"var cb=window.__ideCallbacks__['{escapedCallbackId}'];" +
                                  $"delete window.__ideCallbacks__['{escapedCallbackId}'];" +
@@ -336,8 +337,8 @@ private void InvokeSetAuthToken(string token, string apiUrl)
                     return;
                 }
 
-                var escapedToken = token.Replace("\\", "\\\\").Replace("'", "\\'");
-                var escapedApiUrl = (apiUrl ?? string.Empty).Replace("\\", "\\\\").Replace("'", "\\'");
+                var escapedToken = ExecuteCommandBridge.EscapeForJsString(token);
+                var escapedApiUrl = ExecuteCommandBridge.EscapeForJsString(apiUrl ?? string.Empty);
                 var script = $@"
                     (function() {{
                         if (typeof window.setAuthToken === 'function') {{
diff --git a/Snyk.VisualStudio.Extension.2022/UI/Html/ExecuteCommandBridge.cs b/Snyk.VisualStudio.Extension.2022/UI/Html/ExecuteCommandBridge.cs
@@ -30,6 +30,13 @@ public static class ExecuteCommandBridge
         // Allowlist regex for callbackIds produced by BuildClientScript: "" or "__cb_<digits>"
         private static readonly Regex CallbackIdPattern = new Regex(@"^(__cb_\d+)?$", RegexOptions.Compiled);
 
+        /// <summary>
+        /// Escapes a string for safe embedding inside a single-quoted JavaScript string literal.
+        /// Handles backslashes first, then single quotes, to avoid double-escaping.
+        /// </summary>
+        public static string EscapeForJsString(string value) =>
+            (value ?? string.Empty).Replace("\\", "\\\\").Replace("'", "\\'");
+
         /// <summary>
         /// Returns true if <paramref name="callbackId"/> matches the expected format produced by
         /// <see cref="BuildClientScript"/>. Used as an XSS guard before injecting the id back into JS.
diff --git a/Snyk.VisualStudio.Extension.Tests/UI/Html/ExecuteCommandBridgeTest.cs b/Snyk.VisualStudio.Extension.Tests/UI/Html/ExecuteCommandBridgeTest.cs
@@ -62,6 +62,8 @@ public void IsValidCallbackId_AcceptsValidFormats(string callbackId, bool expect
         [InlineData(";drop table--")]
         [InlineData("__cb_1; alert(1)")]
         [InlineData("__cb_")]
+        [InlineData("_cb_8")]
+        [InlineData(" ")]
         public void IsValidCallbackId_RejectsInvalidFormats(string callbackId)
         {
             Assert.False(ExecuteCommandBridge.IsValidCallbackId(callbackId));
diff --git a/Snyk.VisualStudio.Extension.Tests/UI/Html/HtmlSettingsScriptingBridgeTest.cs b/Snyk.VisualStudio.Extension.Tests/UI/Html/HtmlSettingsScriptingBridgeTest.cs
@@ -64,6 +64,19 @@ public void IdeExecuteCommand_SnykLogin_SavesTokenMethod()
             optionsMock.VerifySet(o => o.AuthenticationMethod = AuthenticationType.Token);
         }
 
+        [Theory]
+        [InlineData("unknown")]
+        [InlineData("")]
+        [InlineData(" ")]
+        public void IdeExecuteCommand_SnykLogin_DefaultsToOAuthForInvalidMethod(string authMethod)
+        {
+            var args = JsonConvert.SerializeObject(new object[] { authMethod, "https://api.snyk.io", false });
+
+            bridge.__ideExecuteCommand__("snyk.login", args, "");
+
+            optionsMock.VerifySet(o => o.AuthenticationMethod = AuthenticationType.OAuth);
+        }
+
         [Fact]
         public void IdeExecuteCommand_SnykLogin_SavesEndpoint()
         {

Original file line number	Diff line number	Diff line change
`@@ -62,6 +62,8 @@ public void IsValidCallbackId_AcceptsValidFormats(string callbackId, bool expect`
`62`	`62`	`[InlineData(";drop table--")]`
`63`	`63`	`[InlineData("__cb_1; alert(1)")]`
`64`	`64`	`[InlineData("__cb_")]`
	`65`	`+ [InlineData("_cb_8")]`
	`66`	`+ [InlineData(" ")]`
`65`	`67`	`public void IsValidCallbackId_RejectsInvalidFormats(string callbackId)`
`66`	`68`	`{`
`67`	`69`	`Assert.False(ExecuteCommandBridge.IsValidCallbackId(callbackId));`