Files
dotnet__skills/.github/workflows/codeowners-folder-validation.yml
Abhitej John bb974f7360 Security Update (#331)
* Security Update

Identified and remediated 15 vulnerabilities across the skill-validator
runtime, dashboard, and CI/CD pipelines.

Changes:
- Runtime (AgentRunner.cs): path traversal prevention, deny-by-default
  permissions, env scrubbing, MCP server hardening
- Regex (AssertionEvaluator.cs): ReDoS timeout
- Logging (SkillDiscovery.cs): exception logging
- Dashboard (dashboard.html, dashboard.js): SRI hash, XSS escaping
- CI/CD (5 workflow files): SHA pinning, persist-credentials: false
- Tests (RunnerTests.cs): 45+ new security test cases

MCP Server Defense-in-Depth (5 layers):
1. Command allowlist: only dotnet, node, npx, python, python3, uvx
2. Path rejection: no /, \, or .. in command names
3. Args validation: per-runtime dangerous flag blocklist
4. Env sanitization: strips PATH, LD_PRELOAD, NODE_OPTIONS,
   DOTNET_STARTUP_HOOKS, etc.
5. Cwd dropped: custom working directories not passed to SDK

Subprocess Isolation:
- Sensitive env vars + prefix patterns scrubbed from child processes
- 120-second timeout with Kill(entireProcessTree: true)
- Deny-by-default permission model

Supply Chain:
- All GitHub Action references pinned to full commit SHAs
- All checkout steps set persist-credentials: false
- Chart.js CDN pinned to v4.4.7 with SRI integrity hash

* Update .github/workflows/skill-validator.yml

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update eng/skill-validator/src/Services/AgentRunner.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update eng/skill-validator/src/Services/AgentRunner.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update tests to use fullCommandText field name

Update ExtractsCommandProperty and PrefersPathOverCommand tests to use
the fullCommandText property instead of command, matching the updated
permission request schema in AgentRunner.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add 'dnx' to allowed MCP commands

* Address PR review feedback: security hardening fixes

- CheckPermission: normalize paths with TrimEndingDirectorySeparator to
  prevent double-separator false denials when dirs have trailing slash
- SanitizeMcpArgs: reject prefix forms (-econsole.log) and --flag=value
  variants that bypass exact-match blocklist checks
- Setup file path traversal: normalize canonicalWorkDir and
  canonicalSkillPath to avoid double-separator in StartsWith checks
- IsAllowedMcpCommand: use GetFileName instead of
  GetFileNameWithoutExtension to block dotnet.bat/node.cmd; only strip
  .exe on Windows

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* prune

* fix

* Fix concurrency setting

* Add binlog generation and upload to skill-validator workflow

Produce binary logs for build, test, and pack steps using /bl: flag.
Upload binlogs as artifacts (with if: always()) for build diagnostics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Improve CheckPermission diagnostics and fix AOT build error

Log allowed directories alongside denied paths to help diagnose
permission mismatches. Replace JsonSerializer.Serialize with manual
string building to fix IL3050 AOT compilation error.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Extract fileName from permission requests

Some permission requests use 'fileName' instead of 'path'. Handle
this property to avoid false denials for file-level operations.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* nownar nu1903 temporarily

* Log config dir in verbose mode alongside work dir

Include skilled/baseline label for consistency with the work dir log.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Add skilled/baseline label to permission denial logs

Helps identify which run type triggered the denial when debugging.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix empty runLabel brackets and add labels to judge permission checks

Conditionally format the label suffix to avoid empty () when no
runLabel is supplied. Pass 'judge' and 'pairwise-judge' labels from
the respective judge call sites.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Allow pairwise judge to read from skilled work directory

The pairwise judge compares baseline and skilled runs in one session,
but previously only allowed reads from the baseline work directory.
Add additionalAllowedDirs parameter to CheckPermission and pass the
skilled work directory through PairwiseJudgeOptions.SkilledWorkDir.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix tests to match allow-by-default for unrecognized permission requests

CheckPermission was changed to allow requests with no extractable
path/command (allow-by-default), but four tests still expected the
old deny-by-default behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Remove NoWarn

* Pin discover job checkout action to full commit SHA

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Dan Moseley <danmose@microsoft.com>
2026-03-12 18:00:55 +00:00

259 lines
10 KiB
YAML

name: codeowners-folder-validation
on:
pull_request:
push:
branches: [main]
paths:
- 'plugins/**'
- 'tests/**'
- '.github/CODEOWNERS'
- '.github/workflows/codeowners-folder-validation.yml'
workflow_dispatch:
permissions:
contents: read
issues: write
jobs:
validate-codeowners:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
with:
persist-credentials: false
- name: Find missing CODEOWNERS folder entries
id: audit
shell: pwsh
run: |
$codeownersPath = '.github/CODEOWNERS'
if (-not (Test-Path $codeownersPath)) {
Write-Error "Missing $codeownersPath"
exit 1
}
# Collect CODEOWNERS path tokens that represent concrete directory scopes.
# Wildcard tokens are ignored for this validation to keep checks folder-specific.
$codeownerEntries = New-Object 'System.Collections.Generic.List[PSCustomObject]'
$ownedPaths = New-Object 'System.Collections.Generic.List[string]'
$lines = Get-Content -Path $codeownersPath
foreach ($line in $lines) {
$trimmed = $line.Trim()
if ([string]::IsNullOrWhiteSpace($trimmed) -or $trimmed.StartsWith('#')) {
continue
}
$tokens = $trimmed -split '\s+'
$pathToken = $tokens[0]
if ($pathToken -match '[*?\[]') {
continue
}
if (-not $pathToken.StartsWith('/')) {
$pathToken = "/$pathToken"
}
if (-not $pathToken.EndsWith('/')) {
$pathToken = "$pathToken/"
}
$owners = @($tokens | Select-Object -Skip 1 | Where-Object { $_ -match '^@' })
$codeownerEntries.Add([PSCustomObject]@{
Path = $pathToken
Owners = $owners
})
$ownedPaths.Add($pathToken)
}
$expectedPaths = New-Object 'System.Collections.Generic.HashSet[string]' ([System.StringComparer]::OrdinalIgnoreCase)
if (Test-Path 'plugins') {
$pluginDirs = Get-ChildItem -Path 'plugins' -Directory
foreach ($pluginDir in $pluginDirs) {
$skillsRoot = Join-Path $pluginDir.FullName 'skills'
if (-not (Test-Path $skillsRoot)) {
continue
}
$skillDirs = Get-ChildItem -Path $skillsRoot -Directory
foreach ($skillDir in $skillDirs) {
[void]$expectedPaths.Add("/plugins/$($pluginDir.Name)/skills/$($skillDir.Name)/")
}
}
}
if (Test-Path 'tests') {
$testPluginDirs = Get-ChildItem -Path 'tests' -Directory
foreach ($testPluginDir in $testPluginDirs) {
$testSkillDirs = Get-ChildItem -Path $testPluginDir.FullName -Directory
foreach ($testSkillDir in $testSkillDirs) {
[void]$expectedPaths.Add("/tests/$($testPluginDir.Name)/$($testSkillDir.Name)/")
}
}
}
function Test-IsCovered([string]$ExpectedPath, [System.Collections.Generic.List[string]]$Scopes) {
foreach ($scope in $Scopes) {
if ($ExpectedPath.StartsWith($scope, [System.StringComparison]::OrdinalIgnoreCase)) {
return $true
}
}
return $false
}
# Return the last matching CODEOWNERS entry (last-match-wins semantics).
function Get-EffectiveEntry([string]$ExpectedPath, [System.Collections.Generic.List[PSCustomObject]]$Entries) {
$lastMatch = $null
foreach ($entry in $Entries) {
if ($ExpectedPath.StartsWith($entry.Path, [System.StringComparison]::OrdinalIgnoreCase)) {
$lastMatch = $entry
}
}
return $lastMatch
}
# Require at least one team or at least two individuals.
function Test-SufficientOwners([string[]]$Owners) {
$teams = @($Owners | Where-Object { $_ -match '/' })
if ($teams.Count -ge 1) { return $true }
$individuals = @($Owners | Where-Object { $_ -notmatch '/' })
if ($individuals.Count -ge 2) { return $true }
return $false
}
# --- Check 1: missing CODEOWNERS entries ---
$missing = @(
$expectedPaths |
Where-Object { -not (Test-IsCovered $_ $ownedPaths) } |
Sort-Object
)
# --- Check 2: insufficient owners (only for covered paths) ---
$insufficientOwners = @(
$expectedPaths |
Where-Object { Test-IsCovered $_ $ownedPaths } |
ForEach-Object {
$entry = Get-EffectiveEntry $_ $codeownerEntries
if ($entry -and -not (Test-SufficientOwners $entry.Owners)) {
[PSCustomObject]@{
path = $_
owners = $entry.Owners -join ' '
}
}
} |
Sort-Object -Property path
)
# --- Outputs ---
$missingJson = ConvertTo-Json -InputObject @($missing) -Compress
if (-not $missingJson) { $missingJson = '[]' }
$insufficientJson = ConvertTo-Json -InputObject @($insufficientOwners) -Compress -Depth 3
if (-not $insufficientJson) { $insufficientJson = '[]' }
$hasMissing = $missing.Count -gt 0
$hasInsufficient = $insufficientOwners.Count -gt 0
if ($hasMissing) {
Write-Host "Missing CODEOWNERS entries:"
$missing | ForEach-Object { Write-Host " - $_" }
}
if ($hasInsufficient) {
Write-Host "Insufficient owners (need 2+ individuals or 1+ team):"
$insufficientOwners | ForEach-Object { Write-Host " - $($_.path) (current: $($_.owners))" }
}
if (-not $hasMissing -and -not $hasInsufficient) {
Write-Host 'All CODEOWNERS entries are present and have sufficient owners.'
}
"has_missing=$($hasMissing.ToString().ToLower())" >> $env:GITHUB_OUTPUT
"missing_json=$missingJson" >> $env:GITHUB_OUTPUT
"has_insufficient_owners=$($hasInsufficient.ToString().ToLower())" >> $env:GITHUB_OUTPUT
"insufficient_owners_json=$insufficientJson" >> $env:GITHUB_OUTPUT
- name: Create or update issue for validation failures
if: (steps.audit.outputs.has_missing == 'true' || steps.audit.outputs.has_insufficient_owners == 'true') && github.event_name != 'pull_request'
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8
env:
MISSING_JSON: ${{ steps.audit.outputs.missing_json }}
INSUFFICIENT_OWNERS_JSON: ${{ steps.audit.outputs.insufficient_owners_json }}
with:
script: |
const missing = JSON.parse(process.env.MISSING_JSON || '[]');
const insufficientOwners = JSON.parse(process.env.INSUFFICIENT_OWNERS_JSON || '[]');
if ((!Array.isArray(missing) || missing.length === 0) &&
(!Array.isArray(insufficientOwners) || insufficientOwners.length === 0)) {
core.info('No issues to report.');
return;
}
const title = 'CODEOWNERS validation failures for skill/test folders';
const marker = '<!-- codeowners-folder-validation -->';
const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
const bodyParts = [
marker,
'Issues discovered by workflow `codeowners-folder-validation`.',
''
];
if (missing.length > 0) {
bodyParts.push('## Missing CODEOWNERS entries', '');
bodyParts.push(...missing.map((p) => `- \`${p}\``));
bodyParts.push('');
}
if (insufficientOwners.length > 0) {
bodyParts.push('## Insufficient owners', '');
bodyParts.push('Each skill/test folder must have at least **2 individual owners** or **1 team**.', '');
bodyParts.push(...insufficientOwners.map((e) => `- \`${e.path}\` — current: ${e.owners}`));
bodyParts.push('');
}
bodyParts.push(`Run: ${runUrl}`);
const body = bodyParts.join('\n');
const { data: issues } = await github.rest.issues.listForRepo({
owner: context.repo.owner,
repo: context.repo.repo,
state: 'open',
per_page: 100
});
const existing = issues.find((i) => i.title === title);
if (existing) {
await github.rest.issues.update({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: existing.number,
body
});
core.info(`Updated issue #${existing.number}`);
} else {
const created = await github.rest.issues.create({
owner: context.repo.owner,
repo: context.repo.repo,
title,
body
});
core.info(`Created issue #${created.data.number}`);
}
- name: Fail when CODEOWNERS validation fails
if: steps.audit.outputs.has_missing == 'true' || steps.audit.outputs.has_insufficient_owners == 'true'
shell: pwsh
env:
MISSING_JSON: ${{ steps.audit.outputs.missing_json }}
INSUFFICIENT_OWNERS_JSON: ${{ steps.audit.outputs.insufficient_owners_json }}
run: |
if ($env:MISSING_JSON -ne '[]') {
Write-Host "Missing entries: $env:MISSING_JSON"
}
if ($env:INSUFFICIENT_OWNERS_JSON -ne '[]') {
Write-Host "Insufficient owners: $env:INSUFFICIENT_OWNERS_JSON"
}
Write-Error 'CODEOWNERS validation failed. Each skill/test folder needs a CODEOWNERS entry with 2+ individuals or 1+ team.'
exit 1