Skip to content

Commit f4e367b

Browse files
security(workspace): swap Arguments-string for ArgumentList — canonical CodeQL-recognised pattern (#47)
The previous QuoteArg-on-Arguments-string shape (commit ff1e192) landed alert #47 in the next CodeQL scan: the analyser treats `ProcessStartInfo.Arguments = …` as a single sink that can carry shell metacharacters regardless of how the value was sanitised upstream. `ArgumentList.Add` is the canonical fix per CodeQL's csharp-CommandLineInjection.qll — each entry goes into argv as a discrete element, so the OS process loader never tokenises through any shell parser. No quoting, no escaping, no concatenation concerns. // Before: psi.Arguments = QuoteArg(resolved); // After: psi.ArgumentList.Add(resolved); Functional behaviour identical (Explorer / Finder / xdg-open receives the resolved path as its argv[1]), but the data flow CodeQL traces from user input now terminates at a recognised sanitiser barrier instead of pretending Arguments-string-quoting is enough. The QuoteArg helper is removed — no caller left. Defense-in-depth still in place: 1. SanitiseWorkspaceId allow-lists alnum+_- at the boundary 2. StartsWith(userRoot, Ordinal) path-traversal guard 3. SafeResolvedPathPattern.IsMatch sink-adjacent allow-list 4. UseShellExecute=false + ArgumentList.Add — argv-level handoff #46's dismiss stays — it referenced the pre-refactor shape. #47 should drop on the next CodeQL scan now that the canonical pattern is in place.
1 parent 66f66cb commit f4e367b

1 file changed

Lines changed: 13 additions & 16 deletions

File tree

src/Kuestenlogik.Bowire/Endpoints/BowireWorkspaceEndpoints.cs

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -279,30 +279,27 @@ private static void LaunchPlatformFileManager(string path)
279279
fileName = "xdg-open";
280280
}
281281

282+
// Use `ArgumentList.Add` instead of the string `Arguments`
283+
// property — each entry goes into argv as a discrete element,
284+
// so the OS process loader never tokenises the resolved path
285+
// through any shell parser. This is the canonical CodeQL-
286+
// recognised pattern (csharp-CommandLineInjection.qll) for
287+
// closing the taint flow from user input → Process.Start;
288+
// the previous QuoteArg-on-Arguments-string shape still flagged
289+
// because CodeQL treats the Arguments setter as a single-line
290+
// sink that can carry shell metacharacters regardless of the
291+
// sanitiser applied to the value. ArgumentList sidesteps that
292+
// entirely: no string concatenation, no quoting concerns, no
293+
// shell tokenisation.
282294
var psi = new ProcessStartInfo
283295
{
284296
FileName = fileName,
285-
Arguments = QuoteArg(resolved),
286297
UseShellExecute = false,
287298
};
299+
psi.ArgumentList.Add(resolved);
288300
Process.Start(psi);
289301
}
290302

291-
/// <summary>
292-
/// Defensive argument quoting — wraps the value in <c>"..."</c>
293-
/// and escapes embedded <c>"</c> as <c>\"</c>. CodeQL's
294-
/// csharp-CommandLineInjection.qll recognises this idiom as a
295-
/// sanitiser barrier when applied to a
296-
/// <see cref="ProcessStartInfo.Arguments"/> value, so it closes
297-
/// the taint flow from user-input → <c>Process.Start</c>.
298-
/// Combined with the earlier path-traversal + allow-list guards
299-
/// this means a hostile workspaceId would have failed three
300-
/// successive checks before it could influence the spawned
301-
/// process's command line.
302-
/// </summary>
303-
internal static string QuoteArg(string value) =>
304-
"\"" + value.Replace("\"", "\\\"", StringComparison.Ordinal) + "\"";
305-
306303
// #58 — Workspace file schema. Property declaration order doubles
307304
// as the on-disk JSON key order; keep version first so a stale
308305
// reader sees the format hint before anything else, then the

0 commit comments

Comments
 (0)