Skip to content

Commit 9f13351

Browse files
KSemenenkoKSemenenko
committed
Harden catalog sync and installer path handling
1 parent 19add93 commit 9f13351

12 files changed

Lines changed: 434 additions & 24 deletions

‎cli/ManagedCode.DotnetSkills/Runtime/AgentCatalogPackage.cs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,19 @@ public static AgentCatalogPackage LoadFromDirectory(DirectoryInfo rootDirectory,
3535
return new AgentCatalogPackage(rootDirectory, agents, sourceLabel);
3636
}
3737

38+
public static AgentCatalogPackage LoadFromManifest(DirectoryInfo catalogRoot, AgentManifest manifest, string sourceLabel)
39+
{
40+
return new AgentCatalogPackage(catalogRoot, manifest.Agents, sourceLabel);
41+
}
42+
3843
public DirectoryInfo ResolveAgentSource(string agentName)
3944
{
4045
var agent = Agents.FirstOrDefault(candidate => string.Equals(candidate.Name, agentName, StringComparison.OrdinalIgnoreCase))
4146
?? throw new InvalidOperationException($"Agent metadata is missing for {agentName} in {SourceLabel}");
42-
var directory = new DirectoryInfo(Path.Combine(CatalogRoot.FullName, agent.Path.Replace('/', Path.DirectorySeparatorChar)));
47+
var directory = PathSafety.ResolveDirectoryWithinRoot(
48+
CatalogRoot,
49+
agent.Path,
50+
$"Agent payload path for {agentName} in {SourceLabel}");
4351
if (!directory.Exists)
4452
{
4553
throw new InvalidOperationException($"Agent payload is missing for {agentName} in {SourceLabel}");

‎cli/ManagedCode.DotnetSkills/Runtime/AgentInstaller.cs‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public AgentInstallSummary Install(IReadOnlyList<AgentEntry> agents, AgentInstal
3636
{
3737
var sourceDirectory = catalog.ResolveAgentSource(agent.Name);
3838
var fileName = $"{agent.Name}{layout.FileExtension}";
39-
var destinationFile = new FileInfo(Path.Combine(layout.PrimaryRoot.FullName, fileName));
39+
var destinationFile = ResolveInstalledAgentFile(layout, agent, fileName);
4040

4141
if (destinationFile.Exists && !force)
4242
{
@@ -92,7 +92,7 @@ public AgentRemoveSummary Remove(IReadOnlyList<AgentEntry> agents, AgentInstallL
9292
foreach (var agent in agents)
9393
{
9494
var fileName = $"{agent.Name}{layout.FileExtension}";
95-
var destinationFile = new FileInfo(Path.Combine(layout.PrimaryRoot.FullName, fileName));
95+
var destinationFile = ResolveInstalledAgentFile(layout, agent, fileName);
9696

9797
if (!destinationFile.Exists)
9898
{
@@ -110,7 +110,7 @@ public AgentRemoveSummary Remove(IReadOnlyList<AgentEntry> agents, AgentInstallL
110110
public bool IsInstalled(AgentEntry agent, AgentInstallLayout layout)
111111
{
112112
var fileName = $"{agent.Name}{layout.FileExtension}";
113-
return File.Exists(Path.Combine(layout.PrimaryRoot.FullName, fileName));
113+
return ResolveInstalledAgentFile(layout, agent, fileName).Exists;
114114
}
115115

116116
public IReadOnlyList<InstalledAgentRecord> GetInstalledAgents(AgentInstallLayout layout)
@@ -140,6 +140,14 @@ private static bool TryResolveAgent(
140140
return false;
141141
}
142142

143+
private static FileInfo ResolveInstalledAgentFile(AgentInstallLayout layout, AgentEntry agent, string fileName)
144+
{
145+
return PathSafety.ResolveFileWithinRoot(
146+
layout.PrimaryRoot,
147+
fileName,
148+
$"Installed agent path for {agent.Name}");
149+
}
150+
143151
private static void WriteMarkdownAgent(FileInfo destinationFile, DirectoryInfo sourceDirectory)
144152
{
145153
var agentFile = new FileInfo(Path.Combine(sourceDirectory.FullName, "AGENT.md"));

‎cli/ManagedCode.DotnetSkills/Runtime/GitHubCatalogReleaseClient.cs‎

Lines changed: 62 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ internal sealed class GitHubCatalogReleaseClient
1313
private const string CatalogTagPrefix = "catalog-v";
1414
private const string CatalogManifestAssetName = "dotnet-skills-manifest.json";
1515
private const string CatalogPayloadAssetName = "dotnet-skills-catalog.zip";
16+
private const int ReleasesPerPage = 50;
1617

1718
private static readonly JsonSerializerOptions JsonOptions = new()
1819
{
@@ -70,6 +71,7 @@ public async Task<SkillCatalogPackage> SyncAsync(string? catalogVersion, bool fo
7071
cacheRoot.Create();
7172

7273
var tempDirectory = new DirectoryInfo(Path.Combine(cacheRoot.FullName, $".tmp-{Guid.NewGuid():N}"));
74+
DirectoryInfo? stagedReleaseDirectory = new DirectoryInfo(Path.Combine(cacheRoot.FullName, $".stage-{Guid.NewGuid():N}"));
7375
tempDirectory.Create();
7476

7577
try
@@ -85,15 +87,13 @@ public async Task<SkillCatalogPackage> SyncAsync(string? catalogVersion, bool fo
8587

8688
ZipFile.ExtractToDirectory(archivePath, tempDirectory.FullName, overwriteFiles: true);
8789

88-
if (releaseDirectory.Exists)
89-
{
90-
releaseDirectory.Delete(recursive: true);
91-
}
92-
9390
var extractedCatalogDirectory = ResolveExtractedCatalogDirectory(tempDirectory);
9491

95-
releaseDirectory.Create();
96-
SkillInstaller.CopyDirectory(extractedCatalogDirectory, new DirectoryInfo(Path.Combine(releaseDirectory.FullName, "catalog")));
92+
stagedReleaseDirectory.Create();
93+
SkillInstaller.CopyDirectory(extractedCatalogDirectory, new DirectoryInfo(Path.Combine(stagedReleaseDirectory.FullName, "catalog")));
94+
SkillCatalogPackage.LoadFromDirectory(stagedReleaseDirectory, $"GitHub release {release.TagName}", release.Version);
95+
ReplaceReleaseDirectory(stagedReleaseDirectory, releaseDirectory);
96+
stagedReleaseDirectory = null;
9797

9898
return SkillCatalogPackage.LoadFromDirectory(releaseDirectory, $"GitHub release {release.TagName}", release.Version);
9999
}
@@ -103,6 +103,11 @@ public async Task<SkillCatalogPackage> SyncAsync(string? catalogVersion, bool fo
103103
{
104104
tempDirectory.Delete(recursive: true);
105105
}
106+
107+
if (stagedReleaseDirectory?.Exists == true)
108+
{
109+
stagedReleaseDirectory.Delete(recursive: true);
110+
}
106111
}
107112
}
108113

@@ -177,11 +182,26 @@ internal static DirectoryInfo ResolveExtractedCatalogDirectory(DirectoryInfo ext
177182

178183
private async Task<IReadOnlyList<GitHubRelease>> GetReleasesAsync(CancellationToken cancellationToken)
179184
{
180-
using var response = await httpClient.GetAsync($"https://api.github.com/repos/{Owner}/{Repository}/releases?per_page=50", cancellationToken);
181-
response.EnsureSuccessStatusCode();
185+
var releases = new List<GitHubRelease>();
182186

183-
await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken);
184-
return await JsonSerializer.DeserializeAsync<List<GitHubRelease>>(stream, JsonOptions, cancellationToken) ?? [];
187+
for (var page = 1; ; page++)
188+
{
189+
using var response = await httpClient.GetAsync(
190+
$"https://api.github.com/repos/{Owner}/{Repository}/releases?per_page={ReleasesPerPage}&page={page}",
191+
cancellationToken);
192+
response.EnsureSuccessStatusCode();
193+
194+
await using var stream = await response.Content.ReadAsStreamAsync(cancellationToken);
195+
var releasePage = await JsonSerializer.DeserializeAsync<List<GitHubRelease>>(stream, JsonOptions, cancellationToken) ?? [];
196+
releases.AddRange(releasePage);
197+
198+
if (releasePage.Count < ReleasesPerPage)
199+
{
200+
break;
201+
}
202+
}
203+
204+
return releases;
185205
}
186206

187207
private async Task<GitHubRelease> GetReleaseByTagAsync(string tag, CancellationToken cancellationToken)
@@ -196,7 +216,7 @@ private async Task<GitHubRelease> GetReleaseByTagAsync(string tag, CancellationT
196216

197217
private async Task<Stream> DownloadAssetAsync(string downloadUrl, CancellationToken cancellationToken)
198218
{
199-
var request = new HttpRequestMessage(HttpMethod.Get, downloadUrl);
219+
using var request = new HttpRequestMessage(HttpMethod.Get, downloadUrl);
200220
using var response = await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken);
201221
response.EnsureSuccessStatusCode();
202222

@@ -215,6 +235,36 @@ private static HttpClient CreateHttpClient()
215235
return httpClient;
216236
}
217237

238+
private static void ReplaceReleaseDirectory(DirectoryInfo stagedReleaseDirectory, DirectoryInfo releaseDirectory)
239+
{
240+
DirectoryInfo? backupDirectory = null;
241+
242+
try
243+
{
244+
if (releaseDirectory.Exists)
245+
{
246+
backupDirectory = new DirectoryInfo(Path.Combine(releaseDirectory.Parent!.FullName, $".backup-{Guid.NewGuid():N}"));
247+
releaseDirectory.MoveTo(backupDirectory.FullName);
248+
}
249+
250+
stagedReleaseDirectory.MoveTo(releaseDirectory.FullName);
251+
252+
if (backupDirectory?.Exists == true)
253+
{
254+
backupDirectory.Delete(recursive: true);
255+
}
256+
}
257+
catch
258+
{
259+
if (!releaseDirectory.Exists && backupDirectory?.Exists == true)
260+
{
261+
backupDirectory.MoveTo(releaseDirectory.FullName);
262+
}
263+
264+
throw;
265+
}
266+
}
267+
218268
private static NuGetVersion? TryParseCatalogVersion(string tagName)
219269
{
220270
if (!tagName.StartsWith(CatalogTagPrefix, StringComparison.Ordinal))

‎cli/ManagedCode.DotnetSkills/Runtime/PathSafety.cs‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
namespace ManagedCode.DotnetSkills.Runtime;
2+
3+
internal static class PathSafety
4+
{
5+
public static DirectoryInfo ResolveDirectoryWithinRoot(DirectoryInfo root, string relativePath, string description)
6+
{
7+
return new DirectoryInfo(ResolvePathWithinRoot(root, relativePath, description));
8+
}
9+
10+
public static FileInfo ResolveFileWithinRoot(DirectoryInfo root, string relativePath, string description)
11+
{
12+
return new FileInfo(ResolvePathWithinRoot(root, relativePath, description));
13+
}
14+
15+
internal static string ResolvePathWithinRoot(DirectoryInfo root, string relativePath, string description)
16+
{
17+
if (string.IsNullOrWhiteSpace(relativePath))
18+
{
19+
throw new InvalidOperationException($"{description} must not use an empty path.");
20+
}
21+
22+
var normalizedRelativePath = relativePath
23+
.Replace('\\', Path.DirectorySeparatorChar)
24+
.Replace('/', Path.DirectorySeparatorChar);
25+
var rootPath = EnsureTrailingSeparator(Path.GetFullPath(root.FullName));
26+
var candidatePath = Path.GetFullPath(Path.Combine(root.FullName, normalizedRelativePath));
27+
28+
if (!candidatePath.StartsWith(rootPath, PathComparison)
29+
|| string.Equals(candidatePath, rootPath.TrimEnd(Path.DirectorySeparatorChar), PathComparison))
30+
{
31+
throw new InvalidOperationException($"{description} must stay within {root.FullName}.");
32+
}
33+
34+
return candidatePath;
35+
}
36+
37+
private static StringComparison PathComparison =>
38+
OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
39+
40+
private static string EnsureTrailingSeparator(string path)
41+
{
42+
return path.EndsWith(Path.DirectorySeparatorChar)
43+
? path
44+
: path + Path.DirectorySeparatorChar;
45+
}
46+
}

‎cli/ManagedCode.DotnetSkills/Runtime/ProjectSkillRecommender.cs‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,16 @@ public static ProjectInventory Load(IEnumerable<FileInfo> projectFiles)
306306

307307
foreach (var projectFile in projectFiles)
308308
{
309-
var document = XDocument.Load(projectFile.FullName, LoadOptions.None);
309+
XDocument document;
310+
try
311+
{
312+
document = XDocument.Load(projectFile.FullName, LoadOptions.None);
313+
}
314+
catch (Exception exception) when (exception is IOException or UnauthorizedAccessException or System.Xml.XmlException)
315+
{
316+
continue;
317+
}
318+
310319
var projectElement = document.Root;
311320
if (projectElement is null)
312321
{

‎cli/ManagedCode.DotnetSkills/Runtime/SkillCatalogPackage.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,10 @@ public DirectoryInfo ResolveSkillSource(string skillName)
5555
{
5656
var skill = Skills.FirstOrDefault(candidate => string.Equals(candidate.Name, skillName, StringComparison.OrdinalIgnoreCase))
5757
?? throw new InvalidOperationException($"Skill metadata is missing for {skillName} in {SourceLabel}");
58-
var directory = new DirectoryInfo(Path.Combine(CatalogRoot.FullName, skill.Path.Replace('/', Path.DirectorySeparatorChar)));
58+
var directory = PathSafety.ResolveDirectoryWithinRoot(
59+
CatalogRoot,
60+
skill.Path,
61+
$"Skill payload path for {skillName} in {SourceLabel}");
5962
if (!directory.Exists)
6063
{
6164
throw new InvalidOperationException($"Skill payload is missing for {skillName} in {SourceLabel}");

‎cli/ManagedCode.DotnetSkills/Runtime/SkillInstaller.cs‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public SkillInstallSummary Install(IReadOnlyList<SkillEntry> skills, SkillInstal
7676
foreach (var skill in skills)
7777
{
7878
var sourceDirectory = catalog.ResolveSkillSource(skill.Name);
79-
var destinationDirectory = new DirectoryInfo(Path.Combine(layout.PrimaryRoot.FullName, skill.Name));
79+
var destinationDirectory = ResolveInstalledSkillDirectory(layout, skill);
8080

8181
if (destinationDirectory.Exists)
8282
{
@@ -103,7 +103,7 @@ public SkillRemoveSummary Remove(IReadOnlyList<SkillEntry> skills, SkillInstallL
103103

104104
foreach (var skill in skills)
105105
{
106-
var destinationDirectory = new DirectoryInfo(Path.Combine(layout.PrimaryRoot.FullName, skill.Name));
106+
var destinationDirectory = ResolveInstalledSkillDirectory(layout, skill);
107107
if (!destinationDirectory.Exists)
108108
{
109109
missingSkills.Add(skill.Name);
@@ -119,7 +119,7 @@ public SkillRemoveSummary Remove(IReadOnlyList<SkillEntry> skills, SkillInstallL
119119

120120
public bool IsInstalled(SkillEntry skill, SkillInstallLayout layout)
121121
{
122-
return Directory.Exists(Path.Combine(layout.PrimaryRoot.FullName, skill.Name));
122+
return ResolveInstalledSkillDirectory(layout, skill).Exists;
123123
}
124124

125125
public IReadOnlyList<InstalledSkillRecord> GetInstalledSkills(SkillInstallLayout layout)
@@ -179,10 +179,19 @@ private static string NormalizePackageKey(string value)
179179
return new string(value.Where(char.IsLetterOrDigit).ToArray()).ToLowerInvariant();
180180
}
181181

182+
private static DirectoryInfo ResolveInstalledSkillDirectory(SkillInstallLayout layout, SkillEntry skill)
183+
{
184+
return PathSafety.ResolveDirectoryWithinRoot(
185+
layout.PrimaryRoot,
186+
skill.Name,
187+
$"Installed skill path for {skill.Name}");
188+
}
189+
182190
private static string ReadInstalledVersion(SkillEntry skill, SkillInstallLayout layout)
183191
{
184-
return ReadManifestValue(Path.Combine(layout.PrimaryRoot.FullName, skill.Name, "manifest.json"), "version")
185-
?? ReadFrontMatterValue(Path.Combine(layout.PrimaryRoot.FullName, skill.Name, "SKILL.md"), "version")
192+
var skillDirectory = ResolveInstalledSkillDirectory(layout, skill);
193+
return ReadManifestValue(Path.Combine(skillDirectory.FullName, "manifest.json"), "version")
194+
?? ReadFrontMatterValue(Path.Combine(skillDirectory.FullName, "SKILL.md"), "version")
186195
?? "unknown";
187196
}
188197

‎tests/ManagedCode.DotnetSkills.Tests/AgentInstallerTests.cs‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,4 +69,45 @@ public void Install_MarkdownAgentFiles_CopiesAgentMarkdown()
6969
Assert.Contains("name: dotnet-router", contents, StringComparison.Ordinal);
7070
Assert.Contains("# .NET Router", contents, StringComparison.Ordinal);
7171
}
72+
73+
[Fact]
74+
public void Install_RejectsAgentNameThatEscapesLayoutRoot()
75+
{
76+
using var tempDirectory = new TemporaryDirectory();
77+
var sourceDirectory = Directory.CreateDirectory(Path.Combine(tempDirectory.Path, "catalog", "Frameworks", "Aspire", "agents", "dotnet-safe-agent"));
78+
File.WriteAllText(
79+
Path.Combine(sourceDirectory.FullName, "AGENT.md"),
80+
"""
81+
---
82+
name: dotnet-safe-agent
83+
description: "safe"
84+
---
85+
86+
# Safe agent
87+
""");
88+
89+
var manifest = new AgentManifest
90+
{
91+
Agents =
92+
[
93+
new AgentEntry
94+
{
95+
Name = "../escape",
96+
Title = "Escape",
97+
Description = "Escape test",
98+
Path = "catalog/Frameworks/Aspire/agents/dotnet-safe-agent"
99+
},
100+
],
101+
};
102+
103+
var catalog = AgentCatalogPackage.LoadFromManifest(new DirectoryInfo(tempDirectory.Path), manifest, "test payload");
104+
var installer = new AgentInstaller(catalog);
105+
using var installDirectory = new TemporaryDirectory();
106+
var layout = new AgentInstallLayout(AgentPlatform.Claude, InstallScope.Project, AgentInstallMode.MarkdownAgentFiles, new DirectoryInfo(installDirectory.Path), false);
107+
108+
var exception = Assert.Throws<InvalidOperationException>(() => installer.Install(catalog.Agents, layout, force: false));
109+
110+
Assert.Contains("must stay within", exception.Message, StringComparison.Ordinal);
111+
Assert.False(File.Exists(Path.Combine(tempDirectory.Path, "escape.md")));
112+
}
72113
}

0 commit comments

Comments
 (0)