SemianiakaVY/w4c-workflows-api
1
0
Fork 0
Code Issues Pull requests Actions Packages Projects Releases Wiki Activity

security fix

Browse source
This commit is contained in:
Vitali sharp8n 2026-09-12 21:45:33 +03:00
parent e2265c7856
commit 3bad99fad3
6 changed files with 135 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

53
Controllers/WorkflowFilesController.cs
View file

@ -114,13 +114,35 @@ public class WorkflowFilesController : ControllerBase
if (request == null || string.IsNullOrWhiteSpace(request.RepoName))
return BadRequest(new { error = "repoName is required" });
var repoName = WorkflowRepoStore.NormalizeName(request.RepoName);
// A workflow repo is always owned by the caller's own Forgejo account, so
// only a single safe name segment is accepted (no owner prefix / traversal).
if (!WorkflowRepoStore.TryNormalizeName(request.RepoName, out var repoName))
return BadRequest(new { error = "Invalid repository name. Use a single repository name (letters, digits, '.', '_', '-')." });
var login = MaybeForgejoLogin;
// Materialize the selected repo BEFORE persisting the switch. An
// unreachable/missing repo must fail loudly instead of being saved as the
// tenant's selection (which would strand the workflows module on a dead repo).
if (_sourceFactory.Forgejo != null && !string.IsNullOrWhiteSpace(login))
{
try
{
await _sourceFactory.Forgejo.EnsureCloneAsync(TenantId, login, repoName, ct);
await _sourceFactory.Forgejo.PullAsync(TenantId, login, repoName, ct);
}
catch (Exception ex)
{
_logger.LogWarning(ex, "Workflow repo switch for tenant {TenantId} to {Repo} failed", TenantId, repoName);
return BadRequest(new { error = $"Repository '{repoName}' could not be opened: {ex.Message}" });
}
}
await _repoStore.SetNameAsync(TenantId, repoName, ct);
// Re-sync workflows for the newly selected repo so the list reflects it.
var syncResult = await _sync.SyncAsync(TenantId, repoName, ct);
var login = MaybeForgejoLogin;
string? fullName = null, cloneDir = null;
if (_sourceFactory.Forgejo != null && !string.IsNullOrWhiteSpace(login))
{
@ -155,9 +177,8 @@ public class WorkflowFilesController : ControllerBase
? sourceDir
: Path.Combine(sourceDir, path.Replace('/', Path.DirectorySeparatorChar));
var fullSourceDir = Path.GetFullPath(sourceDir);
var fullDir = Path.GetFullPath(dir);
if (!fullDir.StartsWith(fullSourceDir, StringComparison.Ordinal))
if (!IsWithin(sourceDir, fullDir))
return BadRequest(new { error = "Invalid path" });
if (!Directory.Exists(fullDir))
@ -259,8 +280,7 @@ public class WorkflowFilesController : ControllerBase
{
if (string.IsNullOrWhiteSpace(raw)) continue;
var full = Path.GetFullPath(Path.Combine(sourceDir, raw.Replace('/', Path.DirectorySeparatorChar)));
if (!full.StartsWith(fullSourceDir, StringComparison.Ordinal) ||
full == fullSourceDir)
if (full == fullSourceDir || !IsWithin(sourceDir, full))
continue;
if (Directory.Exists(full))
@ -334,7 +354,7 @@ public class WorkflowFilesController : ControllerBase
: request!.Message;
var (success, output) = await _sourceFactory.Forgejo.CommitAndPushAsync(
TenantId, login, message, HttpContext.RequestAborted);
TenantId, login, message, CurrentRepo, HttpContext.RequestAborted);
_logger.LogInformation("Commit/push for login {Login}: {Success} ({Output})",
login, success, output?.Trim() ?? "");
@ -353,7 +373,7 @@ public class WorkflowFilesController : ControllerBase
if (string.IsNullOrWhiteSpace(login))
return StatusCode(503, new { error = "Forgejo login not resolved for this tenant." });
var ok = await _sourceFactory.Forgejo.PullAsync(TenantId, login, HttpContext.RequestAborted);
var ok = await _sourceFactory.Forgejo.PullAsync(TenantId, login, CurrentRepo, HttpContext.RequestAborted);
return Ok(new { success = ok });
}
@ -362,8 +382,7 @@ public class WorkflowFilesController : ControllerBase
{
var sourceDir = SourceDir;
var full = Path.GetFullPath(Path.Combine(sourceDir, path.Replace('/', Path.DirectorySeparatorChar)));
var fullSourceDir = Path.GetFullPath(sourceDir);
if (!full.StartsWith(fullSourceDir, StringComparison.Ordinal))
if (!IsWithin(sourceDir, full))
{
status = 400;
return full;
@ -372,6 +391,20 @@ public class WorkflowFilesController : ControllerBase
return full;
}
/// <summary>
/// True when <paramref name="candidate"/> is the root directory or a path
/// inside it. Uses a trailing separator so a sibling directory whose name
/// merely shares the root's prefix (e.g. ".../workflows-evil") can't escape.
/// </summary>
private static bool IsWithin(string root, string candidate)
{
var fullRoot = Path.GetFullPath(root).TrimEnd(Path.DirectorySeparatorChar);
var fullCandidate = Path.GetFullPath(candidate);
if (string.Equals(fullCandidate, fullRoot, StringComparison.Ordinal))
return true;
return fullCandidate.StartsWith(fullRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal);
}
private string? RunGit(string workDir, params string[] args)
{
try

18
Services/ForgejoWorkflowRepoService.cs
View file

@ -131,7 +131,7 @@ public sealed class ForgejoWorkflowRepoService
/// if missing (idempotent). Returns the repo full name (owner/repo).
/// Throws on hard failures.
/// </summary>
public async Task<string> EnsureRepoAsync(string login, CancellationToken ct = default)
public async Task<string> EnsureRepoAsync(string login, string? repoName = null, CancellationToken ct = default)
{
if (!IsConfigured)
throw new InvalidOperationException(
@ -139,7 +139,7 @@ public sealed class ForgejoWorkflowRepoService
"Set Forgejo:AdminToken and Forgejo:Owner (or Forgejo:WorkflowRepoOwner).");
var owner = login.ToLowerInvariant();
var name = Sanitize(WorkflowRepoName);
var name = Sanitize(repoName ?? WorkflowRepoName);
var fullName = $"{owner}/{name}";
try
@ -211,8 +211,10 @@ public sealed class ForgejoWorkflowRepoService
return tenantDir;
}
// Ensure the Forgejo repo exists.
var fullName = await EnsureRepoAsync(login, ct);
// Ensure the Forgejo repo exists. The selected repo name must be honored
// here; otherwise switching to a non-default repo would clone the default
// repo ("workflows") into the directory named after the selected repo.
var fullName = await EnsureRepoAsync(login, repoName, ct);
var cloneUrl = $"{_forgejoBase}/{fullName}.git";
Directory.CreateDirectory(tenantDir);
@ -297,9 +299,9 @@ public sealed class ForgejoWorkflowRepoService
/// <summary>
/// Pulls the latest changes from the remote. Returns true on success.
/// </summary>
public async Task<bool> PullAsync(string tenantId, string login, CancellationToken ct = default)
public async Task<bool> PullAsync(string tenantId, string login, string? repoName = null, CancellationToken ct = default)
{
var tenantDir = TenantCloneDir(tenantId, login);
var tenantDir = TenantCloneDir(tenantId, login, repoName);
if (!Directory.Exists(Path.Combine(tenantDir, ".git")))
{
_logger.LogWarning("No local clone for login {Login}, cannot pull", login);
@ -331,9 +333,9 @@ public sealed class ForgejoWorkflowRepoService
/// edits via the source code API to persist changes to Forgejo.
/// </summary>
public async Task<(bool Success, string Output)> CommitAndPushAsync(
string tenantId, string login, string message, CancellationToken ct = default)
string tenantId, string login, string message, string? repoName = null, CancellationToken ct = default)
{
var tenantDir = TenantCloneDir(tenantId, login);
var tenantDir = TenantCloneDir(tenantId, login, repoName);
if (!Directory.Exists(Path.Combine(tenantDir, ".git")))
return (false, "No local clone");

4
Services/PerTenantWorkflowSource.cs
View file

@ -130,8 +130,8 @@ public sealed class WorkflowSourceFactory
// Ensure the Forgejo repo exists and is cloned locally.
var cloneDir = await _forgejo.EnsureCloneAsync(tenantId, login, repoName, ct);
// Pull latest changes before reading.
await _forgejo.PullAsync(tenantId, login, ct);
// Pull latest changes before reading (from the SELECTED repo, not the default).
await _forgejo.PullAsync(tenantId, login, repoName, ct);
var logger = _loggers.CreateLogger<PerTenantWorkflowSource>();
return new PerTenantWorkflowSource(tenantId, cloneDir, logger);

27
Services/WorkflowRepoStore.cs
View file

@ -73,4 +73,31 @@ public sealed class WorkflowRepoStore
var clean = (repoName ?? string.Empty).Trim().Trim('/');
return string.IsNullOrWhiteSpace(clean) ? "workflows" : clean;
}
/// <summary>
/// Validates a workflow repo basename. The repo is always owned by the
/// caller's own Forgejo account, so only a single safe name segment is
/// accepted — no owner prefix, path separators or traversal. Returns false
/// for anything else.
/// </summary>
public static bool TryNormalizeName(string? repoName, out string normalized)
{
normalized = string.Empty;
var clean = (repoName ?? string.Empty).Trim();
if (clean.Length == 0 || clean.Length > 120)
return false;
foreach (var ch in clean)
{
if (!(char.IsLetterOrDigit(ch) || ch is '.' or '_' or '-'))
return false;
}
// A name made only of dots ("..", ".") is not a repo.
if (clean.Trim('.').Length == 0)
return false;
normalized = clean;
return true;
}
}

21
Services/WorkflowSyncService.cs
View file

@ -49,17 +49,20 @@ public class WorkflowSyncService
public async Task<SyncResult> SyncAsync(string tenantId, string repoName, CancellationToken ct)
{
// In Forgejo-backed mode, pull the latest changes before reading files.
if (_sourceFactory?.IsForgejoBacked == true && _sourceFactory.Forgejo != null)
// Resolve the source for the SELECTED repo. The request-scoped `_source`
// was built from whatever repo was current when the request started, which
// is the wrong repo immediately after a repository switch. In Forgejo-backed
// mode, creating the source ensures the per-user clone exists and is up to
// date (clone + pull), so a switch always reads the newly selected repo.
var source = _source;
if (_sourceFactory?.IsForgejoBacked == true)
{
_logger.LogDebug("Pulling latest workflow files from Forgejo for tenant {TenantId}", tenantId);
var login = await _sourceFactory.ResolveForgejoLoginAsync(tenantId, ct);
if (!string.IsNullOrEmpty(login))
await _sourceFactory.Forgejo.PullAsync(tenantId, login, ct);
_logger.LogDebug("Resolving workflow source for tenant {TenantId}, repo {Repo}", tenantId, repoName);
source = await _sourceFactory.CreateAsync(tenantId, repoName, ct);
}
var state = _source.GetState();
var files = await _source.ListAsync(ct);
var state = source.GetState();
var files = await source.ListAsync(ct);
var result = new SyncResult { HeadSha = state.HeadSha, Dirty = state.Dirty };
@ -85,7 +88,7 @@ public class WorkflowSyncService
string content;
try
{
content = await _source.ReadAsync(file.Path, ct);
content = await source.ReadAsync(file.Path, ct);
}
catch (Exception ex)
{

41
w4c-workflows-api.Tests/WorkflowRepoStoreTests.cs Normal file
View file

@ -0,0 +1,41 @@
using w4c_workflows.Services;
using Xunit;
namespace w4c_workflows.Tests;
public class WorkflowRepoStoreTests
{
[Theory]
[InlineData("workflows")]
[InlineData("wiz4apps")]
[InlineData("my.repo_1-test")]
public void Accepts_single_safe_repo_names(string input)
{
Assert.True(WorkflowRepoStore.TryNormalizeName(input, out var normalized));
Assert.Equal(input, normalized);
}
[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData(" ")]
[InlineData(".")]
[InlineData("..")]
[InlineData("other-owner/repo")]
[InlineData("../escape")]
[InlineData("a\\b")]
[InlineData("repo name")]
[InlineData("repo\nname")]
public void Rejects_traversal_owner_prefix_and_unsafe_names(string? input)
{
Assert.False(WorkflowRepoStore.TryNormalizeName(input, out var normalized));
Assert.Equal(string.Empty, normalized);
}
[Fact]
public void Rejects_names_longer_than_120_chars()
{
Assert.False(WorkflowRepoStore.TryNormalizeName(new string('a', 121), out _));
Assert.True(WorkflowRepoStore.TryNormalizeName(new string('a', 120), out _));
}
}