mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
bb974f7360
* 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>
259 lines
10 KiB
YAML
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
|
|
|