Adjust support for MCP roots #22016
Adjust support for MCP roots #22016
Conversation
github-actions
bot
added
area/build
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
| { | ||
| private static async Task<int> Main(string[] args) | ||
| { | ||
| Debugger.Launch(); |
There was a problem hiding this comment.
Debugger.Launch() should not be present in production code. This will prompt users to attach a debugger every time the program runs, which is disruptive and inappropriate for a released tool. Please remove this line before merging.
| Debugger.Launch(); |
| } | ||
| } | ||
|
|
||
| [Description("This tool MUST be called to before another uno app tools to initialize properly")] |
There was a problem hiding this comment.
Grammatical error in the description attribute. The phrase "called to before another" should be "called before other".
| [Description("This tool MUST be called to before another uno app tools to initialize properly")] | |
| [Description("This tool MUST be called before other uno app tools to initialize properly")] |
| // To avoid tool invocation failures, we wait for the tools to be available | ||
| // after the dev server has started. | ||
| if (_waitForTools | ||
| if ((!_forceRootsFallback || !clientSupportsRoots) && |
There was a problem hiding this comment.
The condition logic appears incorrect. The expression (!_forceRootsFallback || !clientSupportsRoots) should likely be (_forceRootsFallback || !clientSupportsRoots) (without the negation on the first term).
Currently, when _forceRootsFallback is true AND clientSupportsRoots is true, the condition evaluates to false, which means we won't wait for tools. However, based on the code flow and the second part of the condition, it seems we should wait for tools in fallback mode OR when the client doesn't support roots. Please verify this logic is correct.
| if ((!_forceRootsFallback || !clientSupportsRoots) && | |
| if ((_forceRootsFallback || !clientSupportsRoots) && |
| $openApiToken = $env:OPENAPI_TOKEN | ||
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | ||
| Write-Log "OPENAPI_TOKEN not provided. Skipping Codex MCP integration test." | ||
| } | ||
| else { | ||
| Write-Log "OPENAPI_TOKEN detected. Starting Codex MCP integration test." |
There was a problem hiding this comment.
The environment variable name OPENAPI_TOKEN appears to be misspelled. Based on OpenAI's standard naming convention, this should likely be OPENAI_API_KEY or OPENAI_API_TOKEN. Please verify the correct variable name. This typo is also present in the Azure DevOps pipeline configuration.
| $openApiToken = $env:OPENAPI_TOKEN | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| Write-Log "OPENAPI_TOKEN not provided. Skipping Codex MCP integration test." | |
| } | |
| else { | |
| Write-Log "OPENAPI_TOKEN detected. Starting Codex MCP integration test." | |
| $openApiToken = $env:OPENAI_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| Write-Log "OPENAI_API_KEY not provided. Skipping Codex MCP integration test." | |
| } | |
| else { | |
| Write-Log "OPENAI_API_KEY detected. Starting Codex MCP integration test." |
| if (_monitor is not null) | ||
| { | ||
| throw new InvalidOperationException("DevServerMonitor is already running"); | ||
| } |
There was a problem hiding this comment.
Potential race condition: The check if (_monitor is not null) on line 24 is not thread-safe. If StartMonitoring is called concurrently from multiple threads, both calls could pass the null check before either assigns to _monitor, resulting in multiple monitoring tasks. Consider using a lock or Interlocked.CompareExchange to ensure thread-safe initialization.
| try | ||
| { | ||
| var clientTransport = new SseClientTransport(new() | ||
| var clientTransport = new HttpClientTransport(new() |
There was a problem hiding this comment.
The class name changed from SseClientTransport to HttpClientTransport. While this may be correct for the updated ModelContextProtocol library version, please verify this is the intended replacement class and that it provides equivalent functionality for SSE (Server-Sent Events) communication.
| if (_forceRootsFallback) | ||
| { | ||
| await SetRoots(ctx.Params.Arguments?["roots"].Deserialize<string[]>() ?? []); | ||
| return new CallToolResult() { Content = [new TextContentBlock() { Text = "Ok" }] }; | ||
| } |
There was a problem hiding this comment.
Redundant condition check: _forceRootsFallback is already checked on line 102, and the nested if (_forceRootsFallback) on line 104 is unnecessary. The inner condition will always be true if we reach line 106. Consider removing the redundant inner check.
| if (_forceRootsFallback) | |
| { | |
| await SetRoots(ctx.Params.Arguments?["roots"].Deserialize<string[]>() ?? []); | |
| return new CallToolResult() { Content = [new TextContentBlock() { Text = "Ok" }] }; | |
| } | |
| await SetRoots(ctx.Params.Arguments?["roots"].Deserialize<string[]>() ?? []); | |
| return new CallToolResult() { Content = [new TextContentBlock() { Text = "Ok" }] }; |
| BUILD_SOURCESDIRECTORY: $(BUILD.SOURCESDIRECTORY) | ||
| NBGV_SemVer2: $(NBGV_SemVer2) | ||
| WasmCachePath: $(WasmCachePath) | ||
| OPENAPI_TOKEN: $(OPENAPI_TOKEN) |
There was a problem hiding this comment.
The environment variable name OPENAPI_TOKEN appears to be misspelled. Based on OpenAI's standard naming convention, this should likely be OPENAI_API_KEY or OPENAI_API_TOKEN. Please verify the correct variable name matches the PowerShell script usage.
| OPENAPI_TOKEN: $(OPENAPI_TOKEN) | |
| OPENAI_API_KEY: $(OPENAI_API_KEY) |
| private readonly CancellationTokenSource _disposeCts = new(); | ||
|
|
||
| public IMcpClient? UpstreamClient { get; internal set; } | ||
| public McpClient? UpstreamClient { get; internal set; } |
There was a problem hiding this comment.
Breaking change: The property type changed from IMcpClient? to McpClient?. This is a breaking change that reduces flexibility by depending on a concrete type instead of an interface. If this is intentional due to the ModelContextProtocol library update (v0.3.0 to v0.4.1), please document this as a breaking change in the PR description. If the interface is no longer available in the new version, this is acceptable, but should be noted.
| public McpClient? UpstreamClient { get; internal set; } | |
| public IMcpClient? UpstreamClient { get; internal set; } |
| ## Check your environment | ||
|
|
||
| [!include[getting-help](includes/use-uno-check-inline-noheader.md)] | ||
|
|
There was a problem hiding this comment.
This PR is missing a required GitHub issue reference. According to the coding guidelines, every PR MUST reference a proper GitHub issue using one of these formats:
Fixes https://github.com/unoplatform/uno/issues/#<ISSUE_NUMBER>Closes https://github.com/unoplatform/uno/issues/#<ISSUE_NUMBER>Related to https://github.com/unoplatform/uno/issues/#<ISSUE_NUMBER>
Please add the issue reference to the PR description, or if there is no issue, add a note explaining why (e.g., "Note: No related issue (Internal maintenance / Approved by Team member: @username)").
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
The build 186066 found UI Test snapshots differences: Details
|
| if (_roots.FirstOrDefault() is { } rootUri | ||
| && Uri.TryCreate(rootUri, UriKind.RelativeOrAbsolute, out var root)) | ||
| { | ||
| _devServerMonitor.StartMonitoring(root.LocalPath, _devServerPort, _forwardedArgs); |
There was a problem hiding this comment.
Potential null reference exception. If _roots.FirstOrDefault() is not null but Uri.TryCreate returns false, the code continues without any handling. Additionally, if root.LocalPath throws an exception (e.g., for non-file URIs), it won't be caught. Consider adding null checks and error handling for the URI parsing.
| _devServerMonitor.StartMonitoring(root.LocalPath, _devServerPort, _forwardedArgs); | |
| try | |
| { | |
| if (root.IsFile) | |
| { | |
| _devServerMonitor.StartMonitoring(root.LocalPath, _devServerPort, _forwardedArgs); | |
| } | |
| else | |
| { | |
| _logger.LogError("Root URI is not a file URI: {RootUri}", rootUri); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Failed to process root URI: {RootUri}", rootUri); | |
| } |
| $openApiToken = $env:OPENAPI_TOKEN | ||
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { |
There was a problem hiding this comment.
Variable name typo: $openApiToken should likely be $openAIToken based on the environment variable name OPENAPI_TOKEN. While technically the environment variable name is correct, the variable name is misleading as it appears to be for OpenAI (AI service), not OpenAPI (API specification standard).
| $openApiToken = $env:OPENAPI_TOKEN | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| $openAIToken = $env:OPENAPI_TOKEN | |
| if ([string]::IsNullOrWhiteSpace($openAIToken)) { |
| using System.ComponentModel.DataAnnotations; | ||
| using System.ComponentModel.Design.Serialization; | ||
| using System.Text.Json; | ||
| using System.Threading; | ||
| using Microsoft.AspNetCore.Routing; |
There was a problem hiding this comment.
Unused imports detected. System.ComponentModel.DataAnnotations (line 4) and System.ComponentModel.Design.Serialization (line 5) and Microsoft.AspNetCore.Routing (line 8) are imported but not used in the file. Remove them to keep the code clean.
| using System.ComponentModel.DataAnnotations; | |
| using System.ComponentModel.Design.Serialization; | |
| using System.Text.Json; | |
| using System.Threading; | |
| using Microsoft.AspNetCore.Routing; | |
| using System.Text.Json; | |
| using System.Threading; |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
| try | ||
| { | ||
| var clientTransport = new SseClientTransport(new() | ||
| var clientTransport = new HttpClientTransport(new() |
There was a problem hiding this comment.
Breaking API change from SseClientTransport to HttpClientTransport. This changes the transport mechanism from Server-Sent Events to HTTP. Ensure this change is intentional and compatible with the MCP server implementation, as it may affect real-time communication capabilities.
| Version = "1.0.0", | ||
| }, | ||
| Capabilities = new() | ||
| Handlers = new() |
There was a problem hiding this comment.
Breaking API change from Capabilities to Handlers. This reflects a structural change in the ModelContextProtocol API. Verify that this is the correct property name for version 0.4.1-preview.1 of the package.
| public Task<McpClient> UpstreamClient | ||
| => _clientCompletionSource.Task; |
There was a problem hiding this comment.
The property UpstreamClient returns a Task<McpClient> instead of the client directly. This forces all callers to use await at the call site (e.g., await _mcpClientProxy.UpstreamClient), which can be confusing. Consider using a method name like GetUpstreamClientAsync() to make the async nature more explicit, or add XML documentation explaining this behavior.
| using System.ComponentModel.Design.Serialization; | ||
| using System.IO; | ||
| using System.Text.Json; | ||
| using System.Threading; | ||
| using Microsoft.AspNetCore.Routing; |
There was a problem hiding this comment.
Unused imports: System.ComponentModel.Design.Serialization (line 5) and Microsoft.AspNetCore.Routing (line 9) appear to be unused in this file. Consider removing them to keep the code clean.
| using System.ComponentModel.Design.Serialization; | |
| using System.IO; | |
| using System.Text.Json; | |
| using System.Threading; | |
| using Microsoft.AspNetCore.Routing; | |
| using System.IO; | |
| using System.Text.Json; | |
| using System.Threading; |
| $openApiToken = $env:CODEX_API_KEY | ||
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { |
There was a problem hiding this comment.
Variable name typo: $openApiToken should likely be $openAiToken (AI, not API) since it refers to OpenAI's Codex API. The environment variable is correctly named CODEX_API_KEY.
| $openApiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| $openAiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openAiToken)) { |
|
|
||
| Set-Content -Path $instructionsFile -Value $instructions -Encoding utf8 | ||
|
|
||
| $model = if (-not [string]::IsNullOrWhiteSpace($env:CODEX_MODEL)) { $env:CODEX_MODEL } else { "gpt-5-mini" } |
There was a problem hiding this comment.
Model name appears incorrect: gpt-5-mini should likely be gpt-4o-mini (the actual OpenAI model name). GPT-5 has not been released as of the current date.
| $model = if (-not [string]::IsNullOrWhiteSpace($env:CODEX_MODEL)) { $env:CODEX_MODEL } else { "gpt-5-mini" } | |
| $model = if (-not [string]::IsNullOrWhiteSpace($env:CODEX_MODEL)) { $env:CODEX_MODEL } else { "gpt-4o-mini" } |
|
|
||
| log.LogInformation("Connecting to upstream MCP at {Url}", url); | ||
| var client = await McpClientFactory.CreateAsync(clientTransport, options, cancellationToken: ct); | ||
| var client = await McpClient.CreateAsync(clientTransport, options, cancellationToken: ct); |
There was a problem hiding this comment.
Breaking API change from McpClientFactory.CreateAsync() to McpClient.CreateAsync(). This is a significant architectural change in how clients are instantiated. Ensure this aligns with the 0.4.1-preview.1 package API.
| await host.Services.GetRequiredService<McpServer>().SendNotificationAsync( | ||
| NotificationMethods.ToolListChangedNotification, | ||
| new ResourceUpdatedNotificationParams() | ||
| new ToolListChangedNotificationParams() |
There was a problem hiding this comment.
Incorrect notification parameter type: Using ToolListChangedNotificationParams instead of ResourceUpdatedNotificationParams. Given the notification method is ToolListChangedNotification, the parameter type should match. Verify this is correct for the 0.4.1-preview.1 API.
| new ToolListChangedNotificationParams() | |
| new ResourceUpdatedNotificationParams() |
| Debugger.Launch(); | ||
|
|
There was a problem hiding this comment.
Debug code (Debugger.Launch()) should not be committed to the codebase. This will cause debugger prompts in production/release builds and CI environments.
| Debugger.Launch(); |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
| 1. Install [Codex CLI](https://developers.openai.com/codex/cli) using the official commands, for example: | ||
|
|
||
| ```bash | ||
| npm i -g @openai/codex | ||
| # or on macOS | ||
| brew install --cask codex | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The documentation refers to "official commands" but the provided examples show different installation methods (npm vs brew). Consider clarifying which method is recommended or under what circumstances each should be used. Additionally, the URL "https://developers.openai.com/codex/cli" should be verified as it may not be the correct documentation link for the Codex CLI installation instructions.
| 1. Install [Codex CLI](https://developers.openai.com/codex/cli) using the official commands, for example: | |
| ```bash | |
| npm i -g @openai/codex | |
| # or on macOS | |
| brew install --cask codex | |
| ``` | |
| 1. Install Codex CLI. The recommended installation method depends on your platform: | |
| - **All platforms (recommended):** | |
| Use [npm](https://www.npmjs.com/package/@openai/codex) (requires [Node.js](https://nodejs.org/)): | |
| ```bash | |
| npm i -g @openai/codex | |
| ``` | |
| - **macOS only (alternative):** | |
| You can also use [Homebrew](https://formulae.brew.sh/cask/codex): | |
| ```bash | |
| brew install --cask codex | |
| ``` | |
| For the latest installation instructions, see the [official Codex CLI documentation](https://platform.openai.com/docs/assistants/tools/codex-cli). |
| $stdOutFile = [System.IO.Path]::GetTempFileName() | ||
| $stdErrFile = [System.IO.Path]::GetTempFileName() | ||
| $instructionsFile = [System.IO.Path]::GetTempFileName() | ||
|
|
||
| Set-Content -Path $instructionsFile -Value $instructions -Encoding utf8 | ||
|
|
||
| $model = if (-not [string]::IsNullOrWhiteSpace($env:CODEX_MODEL)) { $env:CODEX_MODEL } else { "gpt-5-mini" } | ||
|
|
||
| Write-Log "Invoking Codex CLI to enumerate MCP tools." | ||
| $codexArgs = @( | ||
| '--ask-for-approval','never', | ||
| 'exec', | ||
| '-m',$model, | ||
| '--sandbox','workspace-write', | ||
| '-c','mcp_servers."uno-app".startup_timeout_sec=120', | ||
| '-c','features.web_search_request=true', | ||
| '-c','features.rmcp_client=true', | ||
| '-c','sandbox_workspace_write.network_access=true' | ||
| ) | ||
|
|
||
| $codexExecutable = "codex" | ||
| $codexArguments = $codexArgs | ||
| if ($IsWindows) { | ||
| # npm exposes Codex CLI through a .cmd shim on Windows, so run it via cmd.exe | ||
| $codexExecutable = "cmd.exe" | ||
| $codexArguments = @("/c", "codex") + $codexArgs | ||
| } | ||
|
|
||
| $previousRustLog = $env:RUST_LOG | ||
| $env:RUST_LOG = 'debug' | ||
| try { | ||
| $process = Start-Process -FilePath $codexExecutable -ArgumentList $codexArguments -WorkingDirectory $WorkingDirectory -RedirectStandardOutput $stdOutFile -RedirectStandardError $stdErrFile -RedirectStandardInput $instructionsFile -PassThru | ||
|
|
||
| $timeoutMs = 300000 | ||
| if (-not $process.WaitForExit($timeoutMs)) { | ||
| try { $process.Kill() } catch {} | ||
| $stdErrContent = Get-Content $stdErrFile -ErrorAction SilentlyContinue -Raw | ||
| throw "Codex CLI timed out after $($timeoutMs / 1000) seconds. STDERR:`n$stdErrContent" | ||
| } | ||
|
|
||
| $stdout = Get-Content $stdOutFile -ErrorAction SilentlyContinue -Raw | ||
| $stderr = Get-Content $stdErrFile -ErrorAction SilentlyContinue -Raw | ||
|
|
||
| Remove-Item $stdOutFile -ErrorAction SilentlyContinue | ||
| Remove-Item $stdErrFile -ErrorAction SilentlyContinue | ||
| Remove-Item $instructionsFile -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
The temporary files created with [System.IO.Path]::GetTempFileName() are cleaned up in lines 218-220, but this cleanup only happens if the process exits normally. If an exception occurs before reaching the cleanup code (e.g., at line 206 during process start), these temp files will leak. Consider moving the cleanup into a finally block or using a try-finally pattern to ensure cleanup always occurs.
| } | ||
| } | ||
|
|
||
| [Description("This tool MUST be called to before another uno app tools to initialize properly")] |
There was a problem hiding this comment.
Grammar error in the description: "called to before another" should be "called before another".
| [Description("This tool MUST be called to before another uno app tools to initialize properly")] | |
| [Description("This tool MUST be called before another uno app tools to initialize properly")] |
|
|
||
| _ = Task.Run(() => | ||
| { | ||
| if (tools?.Count != 0) |
There was a problem hiding this comment.
The condition tools?.Count != 0 will be true even when tools is null (null != 0 evaluates to true). This means the callback will be invoked even when there are no tools or when the tools list is null. Change this to tools?.Count > 0 or (tools?.Count ?? 0) > 0 to correctly handle the null case.
| if (tools?.Count != 0) | |
| if (tools?.Count > 0) |
| $openApiToken = $env:CODEX_API_KEY | ||
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { |
There was a problem hiding this comment.
Variable naming inconsistency: the variable is named $openApiToken but it stores the value from $env:CODEX_API_KEY, and it's used to access the Codex API. The variable should be named $codexApiKey to match its purpose and source. Also note that the CI configuration uses OPENAPI_TOKEN (line 300 in .azure-devops-tests-templates.yml) which should be CODEX_API_KEY for consistency.
| $openApiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| $codexApiKey = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($codexApiKey)) { |
| private string? GetRootPath(string rootUri) | ||
| { | ||
| if (Uri.TryCreate(rootUri, UriKind.Absolute, out var absoluteUri) && absoluteUri.IsFile) | ||
| { | ||
| return absoluteUri.LocalPath; | ||
| } | ||
|
|
||
| if (Path.IsPathRooted(rootUri)) | ||
| { | ||
| return Path.GetFullPath(rootUri); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(_currentDirectory)) | ||
| { | ||
| var combined = Path.Combine(_currentDirectory, rootUri); | ||
| return Path.GetFullPath(combined); | ||
| } | ||
|
|
||
| _logger.LogWarning("Unable to resolve MCP root path from '{RootUri}'", rootUri); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The GetRootPath method accepts user-provided paths through the MCP protocol without proper validation. An attacker could potentially provide malicious paths (e.g., paths with directory traversal sequences like ../../sensitive-directory) that could escape the intended workspace. While Path.GetFullPath normalizes the path, it doesn't prevent access to locations outside the expected workspace. Consider adding validation to ensure the resolved path is within an expected workspace boundary, or add explicit checks against path traversal attempts.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
The build 186419 found UI Test snapshots differences: Details
|
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
The build 186442 found UI Test snapshots differences: Details
|
|
|
| _devServerMonitor.StartMonitoring(rootPath, _devServerPort, _forwardedArgs); | ||
| } | ||
| } | ||
| else if (!_forceRootsFallback) |
There was a problem hiding this comment.
The condition !_forceRootsFallback on line 90 appears incorrect. This warning will only be logged when roots are empty AND NOT in force fallback mode, but if _forceRootsFallback is true and there are no roots, no warning is logged. Consider whether this logic is intended, or if the condition should be removed to always warn when no roots are found.
| else if (!_forceRootsFallback) | |
| else |
| if ($IsWindows) { | ||
| # npm exposes Codex CLI through a .cmd shim on Windows, so run it via cmd.exe | ||
| $codexExecutable = "cmd.exe" | ||
| $codexArguments = @("/c", "codex") + $codexArgs | ||
| } |
There was a problem hiding this comment.
This block builds a Codex CLI command using the CODEX_MODEL environment variable and, on Windows, executes it via cmd.exe /c codex ..., which allows shell metacharacters in CODEX_MODEL to be interpreted by cmd.exe. An attacker who can influence CODEX_MODEL (for example through CI variables or the process environment) can inject additional commands using characters like & or | and execute arbitrary code in the build agent context. To remediate this, avoid invoking codex via cmd.exe (execute the codex binary directly) or strictly whitelist/validate CODEX_MODEL and ensure it is passed as a single, safely quoted argument that cannot break out into the surrounding shell command.
| if ($IsWindows) { | |
| # npm exposes Codex CLI through a .cmd shim on Windows, so run it via cmd.exe | |
| $codexExecutable = "cmd.exe" | |
| $codexArguments = @("/c", "codex") + $codexArgs | |
| } | |
| # On Windows, PowerShell Start-Process can invoke .cmd shims directly, so we do not need to use cmd.exe /c. |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-22016/wasm-skia-net9/index.html |
GitHub Issue: closes #
PR Type:
What is the current behavior? 🤔
What is the new behavior? 🚀
PR Checklist ✅
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information ℹ️