From 3bad99fad3b575b87f1fd459806072e1c8ea594e Mon Sep 17 00:00:00 2001 From: Vitali sharp8n Date: Sat, 12 Sep 2026 21:45:33 +0300 Subject: [PATCH] security fix --- Controllers/WorkflowFilesController.cs | 53 +++++++++++++++---- Services/ForgejoWorkflowRepoService.cs | 18 ++++--- Services/PerTenantWorkflowSource.cs | 4 +- Services/WorkflowRepoStore.cs | 27 ++++++++++ Services/WorkflowSyncService.cs | 21 ++++---- .../WorkflowRepoStoreTests.cs | 41 ++++++++++++++ 6 files changed, 135 insertions(+), 29 deletions(-) create mode 100644 w4c-workflows-api.Tests/WorkflowRepoStoreTests.cs diff --git a/Controllers/WorkflowFilesController.cs b/Controllers/WorkflowFilesController.cs index 61e10eb..2089c34 100644 --- a/Controllers/WorkflowFilesController.cs +++ b/Controllers/WorkflowFilesController.cs @@ -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; } + /// + /// True when 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. + /// + 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 diff --git a/Services/ForgejoWorkflowRepoService.cs b/Services/ForgejoWorkflowRepoService.cs index 0d731de..3a8dd35 100644 --- a/Services/ForgejoWorkflowRepoService.cs +++ b/Services/ForgejoWorkflowRepoService.cs @@ -131,7 +131,7 @@ public sealed class ForgejoWorkflowRepoService /// if missing (idempotent). Returns the repo full name (owner/repo). /// Throws on hard failures. /// - public async Task EnsureRepoAsync(string login, CancellationToken ct = default) + public async Task 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 /// /// Pulls the latest changes from the remote. Returns true on success. /// - public async Task PullAsync(string tenantId, string login, CancellationToken ct = default) + public async Task 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. /// 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"); diff --git a/Services/PerTenantWorkflowSource.cs b/Services/PerTenantWorkflowSource.cs index a93ea49..30377cd 100644 --- a/Services/PerTenantWorkflowSource.cs +++ b/Services/PerTenantWorkflowSource.cs @@ -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(); return new PerTenantWorkflowSource(tenantId, cloneDir, logger); diff --git a/Services/WorkflowRepoStore.cs b/Services/WorkflowRepoStore.cs index 6e83689..c72b416 100644 --- a/Services/WorkflowRepoStore.cs +++ b/Services/WorkflowRepoStore.cs @@ -73,4 +73,31 @@ public sealed class WorkflowRepoStore var clean = (repoName ?? string.Empty).Trim().Trim('/'); return string.IsNullOrWhiteSpace(clean) ? "workflows" : clean; } + + /// + /// 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. + /// + 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; + } } diff --git a/Services/WorkflowSyncService.cs b/Services/WorkflowSyncService.cs index 9a5593d..f5cc238 100644 --- a/Services/WorkflowSyncService.cs +++ b/Services/WorkflowSyncService.cs @@ -49,17 +49,20 @@ public class WorkflowSyncService public async Task 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) { diff --git a/w4c-workflows-api.Tests/WorkflowRepoStoreTests.cs b/w4c-workflows-api.Tests/WorkflowRepoStoreTests.cs new file mode 100644 index 0000000..1d2a19f --- /dev/null +++ b/w4c-workflows-api.Tests/WorkflowRepoStoreTests.cs @@ -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 _)); + } +}