btcpayserver/btcpayserver-plugin-builder
btcpayserver/btcpayserver-plugin-builder
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Address Hermes review: test ctor, abuse controls, race, info leak

Browse Source 
Blocking fixes:
  * Test constructor: drop stale httpClientFactory arg so
    PluginBuilder.Tests builds (ctor is (IConfiguration, ILogger)).
  * Anonymous abuse controls: replace PublicApiRateLimit on /submit with
    a dedicated Policies.BtcMapsSubmitRateLimit policy (5 requests /
    24h per IP) so the shared GitHub PAT and OSM bearer can't be used
    as an open-write proxy at the default public-API rate.
  * GitHub race / idempotency: before opening a PR, search open PRs
    in the target repo for an embedded normalized-URL marker and skip
    with Skipped="duplicate-open-pr" when one is already in flight.
    Replace the per-second branch name with a Guid-derived suffix so
    two simultaneous submissions with the same slug can't collide on
    branch creation.
  * Info disclosure: upstream failures no longer echo ex.Message to
    callers. Response carries a correlationId; full detail stays in
    LogError.

Non-blocking follow-ups from the review:
  * Narrow both catch blocks with `when (ex is not OperationCanceledException)`
    so request disconnects bubble as cancellations instead of being
    misreported as 502 upstream failures.
  * Decouple the OSM changeset-close call from the request cancellation
    token: use an independent 10s timeout so client disconnects don't
    leave changesets open.
This commit is contained in:
r1ckstardev 2026-04-22 18:38:55 +00:00
parent 1c7e04a37f
commit c1ce1a4e12
5 changed files with 49 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
PluginBuilder.Tests/BtcMapsServiceTests.cs
View File

@ -11,15 +11,9 @@ public class BtcMapsServiceTests
{
private static BtcMapsService MakeService() =>
new BtcMapsService(
httpClientFactory: new DummyHttpClientFactory(),
configuration: new ConfigurationBuilder().Build(),
logger: NullLogger<BtcMapsService>.Instance);
private sealed class DummyHttpClientFactory : IHttpClientFactory
{
public HttpClient CreateClient(string name) => new HttpClient();
}
[Fact]
public void Validate_RequiresAtLeastOneAction_NotEnforcedHere()
{

18
PluginBuilder/Controllers/BtcMapsController.cs
View File

@ -18,7 +18,7 @@ public sealed class BtcMapsController(
public IActionResult Ping() => Ok(new { ok = true, service = "btcmaps", version = "v1" });
[HttpPost("submit")]
[EnableRateLimiting(Policies.PublicApiRateLimit)]
[EnableRateLimiting(Policies.BtcMapsSubmitRateLimit)]
public async Task<IActionResult> Submit(
[FromBody] BtcMapsSubmitRequest? request,
CancellationToken cancellationToken)
@ -33,6 +33,7 @@ public sealed class BtcMapsController(
if (errors.Count > 0)
return BadRequest(new { errors });
var correlationId = Guid.NewGuid().ToString("N");
var response = new BtcMapsSubmitResponse();
if (request.SubmitToDirectory)
@ -41,13 +42,14 @@ public sealed class BtcMapsController(
{
response.Directory = await btcMapsService.SubmitToDirectoryAsync(request, cancellationToken);
}
catch (Exception ex)
catch (Exception ex) when (ex is not OperationCanceledException)
{
logger.LogError(ex, "BTCMaps directory submission failed for {Name} ({Url})", request.Name, request.Url);
logger.LogError(ex, "BTCMaps directory submission failed (correlationId={CorrelationId}) for {Name} ({Url})",
correlationId, request.Name, request.Url);
return StatusCode(StatusCodes.Status502BadGateway, new
{
error = "directory-upstream-failed",
detail = ex.Message
correlationId
});
}
}
@ -58,14 +60,14 @@ public sealed class BtcMapsController(
{
response.Osm = await btcMapsService.TagOnOsmAsync(request, cancellationToken);
}
catch (Exception ex)
catch (Exception ex) when (ex is not OperationCanceledException)
{
logger.LogError(ex, "BTCMaps OSM tagging failed for {Name} node {NodeType}/{NodeId}",
request.Name, request.OsmNodeType, request.OsmNodeId);
logger.LogError(ex, "BTCMaps OSM tagging failed (correlationId={CorrelationId}) for {Name} node {NodeType}/{NodeId}",
correlationId, request.Name, request.OsmNodeType, request.OsmNodeId);
return StatusCode(StatusCodes.Status502BadGateway, new
{
error = "osm-upstream-failed",
detail = ex.Message,
correlationId,
partial = response
});
}

1
PluginBuilder/Policies.cs
View File

@ -4,4 +4,5 @@ public class Policies
{
public const string OwnPlugin = "OwnPlugin";
public const string PublicApiRateLimit = "PublicApiRateLimit";
public const string BtcMapsSubmitRateLimit = "BtcMapsSubmitRateLimit";
}

11
PluginBuilder/Program.cs
View File

@ -263,6 +263,17 @@ public class Program
QueueLimit = 0
});
});
options.AddPolicy(Policies.BtcMapsSubmitRateLimit, httpContext =>
{
var clientIp = httpContext.Connection.RemoteIpAddress?.ToString() ?? "unknown";
return RateLimitPartition.GetFixedWindowLimiter(clientIp, _ => new FixedWindowRateLimiterOptions
{
PermitLimit = 5,
Window = TimeSpan.FromHours(24),
QueueProcessingOrder = QueueProcessingOrder.OldestFirst,
QueueLimit = 0
});
});
});
services.AddOutputCache(options =>

31
PluginBuilder/Services/BtcMapsService.cs
View File

@ -159,6 +159,22 @@ public sealed class BtcMapsService
}
}
var marker = BuildUrlMarker(normalizedUrl);
var openPrSearch = await GetJsonAsync(
client,
$"search/issues?q={Uri.EscapeDataString($"repo:{repo} is:pr is:open in:body \"{marker}\"")}",
cancellationToken);
if (openPrSearch.TryGetProperty("total_count", out var totalCount) && totalCount.GetInt32() > 0)
{
var firstItem = openPrSearch.GetProperty("items")[0];
return new BtcMapsDirectoryResult
{
Skipped = "duplicate-open-pr",
PrUrl = firstItem.TryGetProperty("html_url", out var h) ? h.GetString() : null,
PrNumber = firstItem.TryGetProperty("number", out var n) ? n.GetInt32() : null
};
}
var newEntry = BuildMerchantEntry(request);
var updated = merchants
.Select(e => (JsonElement?)e)
@ -177,7 +193,8 @@ public sealed class BtcMapsService
var baseSha = branchRef.GetProperty("object").GetProperty("sha").GetString()
?? throw new InvalidOperationException("base sha missing");
var branchName = $"btcmaps/{Slugify(request.Name!)}-{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}";
var branchSuffix = Guid.NewGuid().ToString("N")[..8];
var branchName = $"btcmaps/{Slugify(request.Name!)}-{branchSuffix}";
await PostJsonAsync(client, $"repos/{repo}/git/refs",
new { @ref = $"refs/heads/{branchName}", sha = baseSha }, cancellationToken);
@ -190,7 +207,7 @@ public sealed class BtcMapsService
branch = branchName
}, cancellationToken);
var prBody = BuildPrBody(request);
var prBody = BuildPrBody(request, marker);
var prResponse = await PostJsonAsync(client, $"repos/{repo}/pulls",
new
{
@ -267,10 +284,11 @@ public sealed class BtcMapsService
}
finally
{
using var closeCts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
try
{
await client.PutAsync($"changeset/{changesetId}/close",
new StringContent(string.Empty), cancellationToken);
new StringContent(string.Empty), closeCts.Token);
}
catch (Exception ex)
{
@ -317,7 +335,7 @@ public sealed class BtcMapsService
return JsonDocument.Parse(ms).RootElement.Clone();
}
private static string BuildPrBody(BtcMapsSubmitRequest request)
private static string BuildPrBody(BtcMapsSubmitRequest request, string urlMarker)
{
var sb = new StringBuilder();
sb.AppendLine("Automated submission from the BTCPay Server plugin-builder `/apis/btcmaps/v1/submit` endpoint.");
@ -333,9 +351,14 @@ public sealed class BtcMapsService
sb.AppendLine(request.Description);
sb.AppendLine();
sb.AppendLine("_Please review before merge — this PR was opened programmatically by a BTCMap-plugin merchant submission, not by a maintainer._");
sb.AppendLine();
sb.AppendLine($"<!-- {urlMarker} -->");
return sb.ToString();
}
private static string BuildUrlMarker(string normalizedUrl) =>
$"btcmaps-submit:url={normalizedUrl}";
public static string NormalizeUrl(string url) =>
url.Trim().TrimEnd('/').ToLowerInvariant();