feat: add version release management and build listing API endpoints#152
feat: add version release management and build listing API endpoints#152
Conversation
Add 4 new authenticated endpoints to /api/v1:
- GET /plugins/{slug}/builds — list builds (last 50)
- POST /plugins/{slug}/versions/{ver}/release — release or GPG-sign-release
- POST /plugins/{slug}/versions/{ver}/unrelease — revert to pre-release
- DELETE /plugins/{slug}/versions/{ver} — remove a version
Supporting changes:
- Add State property to BuildData (also exposed on existing GET build endpoint)
- Add byte[] overload to GPGKeyService for JSON API signature verification
- Extract ManifestHelper utility from PluginController private methods
- Create ReleaseVersionRequest model for the release endpoint
|
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main changes: adding version release management and build listing API endpoints, which are the primary features added across the changeset. |
| Description check | ✅ Passed | The description comprehensively explains the new endpoints, data model changes, shared utility extraction, and provides a test plan aligned with the implementation changes. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
api-update
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@PluginBuilder/Controllers/ApiController.cs`:
- Around line 478-503: The DELETE and the build state update in RemoveVersion
are non-atomic and can leave inconsistent state; open a DB transaction on the
connection returned by connectionFactory.Open() and perform both the state
change and the DELETE inside that transaction (or at minimum call
buildService.UpdateBuild first then DELETE), ensuring you use the same
connection/transaction so both succeed or both roll back; locate RemoveVersion,
the conn usage from connectionFactory.Open(), the DELETE against the versions
table, and the call to buildService.UpdateBuild (and the FullBuildId
construction) and move both operations into a single transaction scope (or
reorder to update the build state first) and commit only after both succeed.
- Around line 337-377: The Git commit truncation uses buildInfo?.GitCommit?[..8]
which will throw if GitCommit exists but is shorter than 8 chars; update
ListBuilds (and the other Build endpoint) to safely take up to 8 chars by
checking length first, e.g. set Commit = buildInfo?.GitCommit != null &&
buildInfo.GitCommit.Length >= 8 ? buildInfo.GitCommit[..8] :
buildInfo?.GitCommit (or use Substring(0, Math.Min(8,
buildInfo.GitCommit.Length))); modify the BuildData population in ListBuilds
(and the equivalent code in the Build method) accordingly.
- Around line 379-454: The ReleaseVersion method fetches pluginBuild but doesn't
validate the build's state before allowing release and also selects p.identifier
unnecessarily; after obtaining pluginBuild (the (buildId, identifier) tuple),
query the builds table for the build's state using pluginBuild.Value.buildId and
ensure it's in an allowed releasable state (e.g., "uploaded") before
proceeding—if not, add a model error (or return a validation result) similar to
other validation paths and stop the release; remove p.identifier from the
initial SELECT to avoid unused data; apply the same build-state guard to the
other entry point (PluginController.Release) that performs releases.
In `@PluginBuilder/Services/GPGKeyService.cs`:
- Around line 108-170: The PgpUtilities.GetDecoderStream result (decodedStream)
in VerifyDetachedSignature is IDisposable but not disposed; wrap its creation in
a using (e.g., using var decodedStream = PgpUtilities.GetDecoderStream(new
MemoryStream(signatureBytes))) so the PgpObjectFactory(sigFact) initialized from
decodedStream is created after and used within that using scope and ensure the
decodedStream stays alive for the lifetime of sigFact usage (do not dispose
earlier than the loop), so replace the standalone decodedStream variable with a
using-bound variable that scopes the entire signature parsing/verification
logic.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@PluginBuilder/Services/GPGKeyService.cs`: - Around line 108-170: The PgpUtilities.GetDecoderStream result (decodedStream) in VerifyDetachedSignature is IDisposable but not disposed; wrap its creation in a using (e.g., using var decodedStream = PgpUtilities.GetDecoderStream(new MemoryStream(signatureBytes))) so the PgpObjectFactory(sigFact) initialized from decodedStream is created after and used within that using scope and ensure the decodedStream stays alive for the lifetime of sigFact usage (do not dispose earlier than the loop), so replace the standalone decodedStream variable with a using-bound variable that scopes the entire signature parsing/verification logic.PluginBuilder/Services/GPGKeyService.cs (1)
108-170: Minor:decodedStreamfromPgpUtilities.GetDecoderStreamis not disposed.Line 119 creates a
decodedStreamthat isIDisposablebut is never disposed. Consider wrapping it withusing. This is low-risk since the underlyingMemoryStreamholds no unmanaged resources, but it's good hygiene.♻️ Suggested fix
- var decodedStream = PgpUtilities.GetDecoderStream(new MemoryStream(signatureBytes)); + using var decodedStream = PgpUtilities.GetDecoderStream(new MemoryStream(signatureBytes));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PluginBuilder/Services/GPGKeyService.cs` around lines 108 - 170, The PgpUtilities.GetDecoderStream result (decodedStream) in VerifyDetachedSignature is IDisposable but not disposed; wrap its creation in a using (e.g., using var decodedStream = PgpUtilities.GetDecoderStream(new MemoryStream(signatureBytes))) so the PgpObjectFactory(sigFact) initialized from decodedStream is created after and used within that using scope and ensure the decodedStream stays alive for the lifetime of sigFact usage (do not dispose earlier than the loop), so replace the standalone decodedStream variable with a using-bound variable that scopes the entire signature parsing/verification logic.
| [HttpPost("plugins/{pluginSlug}/versions/{version}/release")] | ||
| public async Task<IActionResult> ReleaseVersion( | ||
| [ModelBinder(typeof(PluginSlugModelBinder))] | ||
| PluginSlug pluginSlug, | ||
| [ModelBinder(typeof(PluginVersionModelBinder))] | ||
| PluginVersion version, | ||
| [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] | ||
| ReleaseVersionRequest? request = null) | ||
| { | ||
| await using var conn = await connectionFactory.Open(); | ||
|
|
||
| var pluginBuild = await conn.QueryFirstOrDefaultAsync<(long buildId, string identifier)?>( | ||
| "SELECT v.build_id, p.identifier FROM versions v " + | ||
| "JOIN plugins p ON v.plugin_slug = p.slug " + | ||
| "WHERE v.plugin_slug=@pluginSlug AND v.ver=@version", | ||
| new { pluginSlug = pluginSlug.ToString(), version = version.VersionParts }); | ||
|
|
||
| if (pluginBuild is null) | ||
| return NotFound(); | ||
|
|
||
| var hasSignature = !string.IsNullOrEmpty(request?.Signature); | ||
|
|
||
| if (hasSignature) | ||
| { | ||
| var manifest_info = await conn.QueryFirstOrDefaultAsync<string>( | ||
| "SELECT manifest_info FROM builds b " + | ||
| "WHERE b.plugin_slug=@pluginSlug AND b.id=@buildId LIMIT 1", | ||
| new { pluginSlug = pluginSlug.ToString(), pluginBuild.Value.buildId }); | ||
|
|
||
| var niceManifest = ManifestHelper.NiceJson(manifest_info); | ||
| var manifestHash = ManifestHelper.GetManifestHash(niceManifest, true); | ||
| if (string.IsNullOrEmpty(manifestHash)) | ||
| { | ||
| ModelState.AddModelError("", "Manifest information for plugin not available"); | ||
| return ValidationErrorResult(ModelState); | ||
| } | ||
|
|
||
| byte[] signatureBytes; | ||
| try | ||
| { | ||
| signatureBytes = Convert.FromBase64String(request!.Signature!); | ||
| } | ||
| catch (FormatException) | ||
| { | ||
| ModelState.AddModelError(nameof(request.Signature), "Signature must be valid base64"); | ||
| return ValidationErrorResult(ModelState); | ||
| } | ||
|
|
||
| var userId = userManager.GetUserId(User)!; | ||
| var signatureVerification = await gpgKeyService.VerifyDetachedSignature( | ||
| pluginSlug.ToString(), userId, | ||
| Encoding.UTF8.GetBytes(manifestHash), signatureBytes); | ||
|
|
||
| if (!signatureVerification.valid) | ||
| { | ||
| ModelState.AddModelError(nameof(request.Signature), signatureVerification.message); | ||
| return ValidationErrorResult(ModelState); | ||
| } | ||
|
|
||
| await conn.UpdateVersionReleaseStatus(pluginSlug, "sign_release", version, signatureVerification.proof); | ||
| } | ||
| else | ||
| { | ||
| var pluginSettings = await conn.GetSettings(pluginSlug); | ||
| if (pluginSettings?.RequireGPGSignatureForRelease == true) | ||
| { | ||
| ModelState.AddModelError("", "A verified GPG signature is required to release this version"); | ||
| return ValidationErrorResult(ModelState); | ||
| } | ||
|
|
||
| await conn.UpdateVersionReleaseStatus(pluginSlug, "release", version); | ||
| } | ||
|
|
||
| await outputCacheStore.EvictByTagAsync(CacheTags.Plugins, CancellationToken.None); | ||
| return Ok(new { version = version.ToString(), released = true }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for BuildStates and build state definitions
rg -n "BuildStates|build.*state" --type=cs -i | head -40
echo "--- Searching for UpdateVersionReleaseStatus ---"
rg -n "UpdateVersionReleaseStatus" --type=cs -A10 -B2Repository: btcpayserver/btcpayserver-plugin-builder
Length of output: 12614
🏁 Script executed:
#!/bin/bash
# Check the full BuildStates enum
cat PluginBuilder/BuildStates.cs
echo "--- Checking if p.identifier is used in ReleaseVersion ---"
# Look at ApiController.cs around the ReleaseVersion method
sed -n '379,454p' PluginBuilder/Controllers/ApiController.cs | grep -i identifier
echo "--- Checking PluginController.Release method ---"
rg -n "public.*Release" PluginBuilder/Controllers/PluginController.cs -A5 | head -50Repository: btcpayserver/btcpayserver-plugin-builder
Length of output: 2115
🏁 Script executed:
#!/bin/bash
# Get the full PluginController.Release method
sed -n '387,455p' PluginBuilder/Controllers/PluginController.cs
echo "--- Check if identifier is used anywhere in ApiController.ReleaseVersion ---"
sed -n '379,454p' PluginBuilder/Controllers/ApiController.cs | grep -i "identifier"Repository: btcpayserver/btcpayserver-plugin-builder
Length of output: 4362
ReleaseVersion: no build-state validation before releasing.
The endpoint checks that the version row exists but doesn't verify the associated build is in a releasable state (e.g., "uploaded"). A build in states like "removed", "failed", "running", "queued", or "uploading" could be released through this endpoint. The same vulnerability exists in PluginController.Release. Since the API is programmatic and likely to be scripted, this gap is particularly important to close.
Add a build state check after fetching pluginBuild:
🛡️ Suggested guard
+ var buildState = await conn.QueryFirstOrDefaultAsync<string>(
+ "SELECT state FROM builds WHERE plugin_slug=@pluginSlug AND id=@buildId",
+ new { pluginSlug = pluginSlug.ToString(), buildId = pluginBuild.Value.buildId });
+
+ if (buildState is not "uploaded")
+ return BadRequest(new { error = $"Build is in '{buildState}' state and cannot be released" });
+
var hasSignature = !string.IsNullOrEmpty(request?.Signature);Also, line 391 fetches p.identifier but it's never used in the method body—drop it from the SELECT to keep the query lean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PluginBuilder/Controllers/ApiController.cs` around lines 379 - 454, The
ReleaseVersion method fetches pluginBuild but doesn't validate the build's state
before allowing release and also selects p.identifier unnecessarily; after
obtaining pluginBuild (the (buildId, identifier) tuple), query the builds table
for the build's state using pluginBuild.Value.buildId and ensure it's in an
allowed releasable state (e.g., "uploaded") before proceeding—if not, add a
model error (or return a validation result) similar to other validation paths
and stop the release; remove p.identifier from the initial SELECT to avoid
unused data; apply the same build-state guard to the other entry point
(PluginController.Release) that performs releases.
- Safely slice GitCommit to avoid ArgumentOutOfRangeException on short hashes - Validate build is in 'uploaded' state before allowing release - Replace unused p.identifier with b.state in ReleaseVersion query - Wrap RemoveVersion DELETE + state update in a transaction for atomicity - Dispose decodedStream in GPGKeyService signature verification
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
PluginBuilder/Controllers/ApiController.cs (2)
379-403: UseBuildStates.Uploadedconstant instead of the"uploaded"string literal.Line 399 uses a bare string while
RemoveVersionat line 502 usesBuildStates.Removed. Using the constant prevents silent breakage if the state value is ever renamed.♻️ Proposed fix
- if (pluginBuild.Value.state is not "uploaded") + if (pluginBuild.Value.state is not BuildStates.Uploaded) { ModelState.AddModelError("", $"Build is in '{pluginBuild.Value.state}' state and cannot be released");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PluginBuilder/Controllers/ApiController.cs` around lines 379 - 403, Replace the hard-coded state check in ReleaseVersion (the comparison against "uploaded" on pluginBuild.Value.state) with the BuildStates.Uploaded constant; update the condition that currently reads pluginBuild.Value.state is not "uploaded" to use BuildStates.Uploaded so it matches the pattern used by RemoveVersion (BuildStates.Removed) and avoids fragile string literals.
390-420: Optional: merge the two DB roundtrips into one query.Lines 390–394 fetch
(buildId, state), and lines 409–412 issue a second query formanifest_infoonly when a signature is present. A single query returning all three columns on the first hit removes one network roundtrip on the signed-release path and simplifies the control flow.♻️ Proposed refactor
- var pluginBuild = await conn.QueryFirstOrDefaultAsync<(long buildId, string state)?>( - "SELECT v.build_id, b.state FROM versions v " + - "JOIN builds b ON v.plugin_slug = b.plugin_slug AND v.build_id = b.id " + - "WHERE v.plugin_slug=@pluginSlug AND v.ver=@version", - new { pluginSlug = pluginSlug.ToString(), version = version.VersionParts }); + var pluginBuild = await conn.QueryFirstOrDefaultAsync<(long buildId, string state, string? manifest_info)?>( + "SELECT v.build_id, b.state, b.manifest_info FROM versions v " + + "JOIN builds b ON v.plugin_slug = b.plugin_slug AND v.build_id = b.id " + + "WHERE v.plugin_slug=@pluginSlug AND v.ver=@version", + new { pluginSlug = pluginSlug.ToString(), version = version.VersionParts }); if (pluginBuild is null) return NotFound(); if (pluginBuild.Value.state is not BuildStates.Uploaded) { ... } var hasSignature = !string.IsNullOrEmpty(request?.Signature); if (hasSignature) { - var manifest_info = await conn.QueryFirstOrDefaultAsync<string>( - "SELECT manifest_info FROM builds b " + - "WHERE b.plugin_slug=@pluginSlug AND b.id=@buildId LIMIT 1", - new { pluginSlug = pluginSlug.ToString(), pluginBuild.Value.buildId }); - - var niceManifest = ManifestHelper.NiceJson(manifest_info); + var niceManifest = ManifestHelper.NiceJson(pluginBuild.Value.manifest_info);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PluginBuilder/Controllers/ApiController.cs` around lines 390 - 420, The initial DB fetch for pluginBuild only selects (build_id, state) causing a second roundtrip to get manifest_info when hasSignature is true; change the first conn.QueryFirstOrDefaultAsync call (the one producing pluginBuild) to also select manifest_info (e.g., return a tuple or a small DTO with buildId, state, manifest_info), then use that returned manifest_info for the signed path instead of issuing the second query; update subsequent references (pluginBuild.Value.buildId, pluginBuild.Value.state, manifest_info usage, and the hasSignature/ManifestHelper.NiceJson/GetManifestHash logic) to read from the combined result and remove the second conn.QueryFirstOrDefaultAsync call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PluginBuilder/Services/GPGKeyService.cs`:
- Around line 108-113: In VerifyDetachedSignature add a null/empty guard for the
rawSignedBytes parameter similar to the existing signatureBytes check: before
any use (prior to calling signature.Update(rawSignedBytes)) validate
rawSignedBytes is not null and Length > 0 and return a clear
SignatureProofResponse(false, "Signed data is required") if it fails; update the
method VerifyDetachedSignature and any related early-return logic so the null
case is rejected explicitly rather than allowing
signature.Update(rawSignedBytes) to throw.
---
Duplicate comments:
In `@PluginBuilder/Controllers/ApiController.cs`:
- Around line 337-377: No changes required: the ListBuilds method correctly
handles nullable manifest_info/build_info, safely parses BuildInfo and
PluginManifest, and truncates Commit consistent with the Build endpoint; leave
ListBuilds and the BuildData population (including Commit truncation logic using
buildInfo?.GitCommit and gc[..8]) as-is.
---
Nitpick comments:
In `@PluginBuilder/Controllers/ApiController.cs`:
- Around line 379-403: Replace the hard-coded state check in ReleaseVersion (the
comparison against "uploaded" on pluginBuild.Value.state) with the
BuildStates.Uploaded constant; update the condition that currently reads
pluginBuild.Value.state is not "uploaded" to use BuildStates.Uploaded so it
matches the pattern used by RemoveVersion (BuildStates.Removed) and avoids
fragile string literals.
- Around line 390-420: The initial DB fetch for pluginBuild only selects
(build_id, state) causing a second roundtrip to get manifest_info when
hasSignature is true; change the first conn.QueryFirstOrDefaultAsync call (the
one producing pluginBuild) to also select manifest_info (e.g., return a tuple or
a small DTO with buildId, state, manifest_info), then use that returned
manifest_info for the signed path instead of issuing the second query; update
subsequent references (pluginBuild.Value.buildId, pluginBuild.Value.state,
manifest_info usage, and the
hasSignature/ManifestHelper.NiceJson/GetManifestHash logic) to read from the
combined result and remove the second conn.QueryFirstOrDefaultAsync call.
| public async Task<SignatureProofResponse> VerifyDetachedSignature(string pluginslug, string userId, byte[] rawSignedBytes, byte[] signatureBytes) | ||
| { | ||
| try | ||
| { | ||
| if (signatureFile is not { Length: > 0 }) | ||
| return new SignatureProofResponse(false, "Please upload a valid GPG signature file (.asc or .sig)"); | ||
| if (signatureBytes is not { Length: > 0 }) | ||
| return new SignatureProofResponse(false, "Signature data is required"); |
There was a problem hiding this comment.
Add a null/empty guard for rawSignedBytes.
signatureBytes is guarded at line 112, but rawSignedBytes is never validated. If a null array reaches line 152 (signature.Update(rawSignedBytes)), BouncyCastle will throw a NullReferenceException caught only by the outer catch, producing the opaque message "Verification failed: Object reference not set…" rather than a clear error.
🛡️ Proposed guard
if (signatureBytes is not { Length: > 0 })
return new SignatureProofResponse(false, "Signature data is required");
+
+ if (rawSignedBytes is not { Length: > 0 })
+ return new SignatureProofResponse(false, "Signed data is required");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async Task<SignatureProofResponse> VerifyDetachedSignature(string pluginslug, string userId, byte[] rawSignedBytes, byte[] signatureBytes) | |
| { | |
| try | |
| { | |
| if (signatureFile is not { Length: > 0 }) | |
| return new SignatureProofResponse(false, "Please upload a valid GPG signature file (.asc or .sig)"); | |
| if (signatureBytes is not { Length: > 0 }) | |
| return new SignatureProofResponse(false, "Signature data is required"); | |
| public async Task<SignatureProofResponse> VerifyDetachedSignature(string pluginslug, string userId, byte[] rawSignedBytes, byte[] signatureBytes) | |
| { | |
| try | |
| { | |
| if (signatureBytes is not { Length: > 0 }) | |
| return new SignatureProofResponse(false, "Signature data is required"); | |
| if (rawSignedBytes is not { Length: > 0 }) | |
| return new SignatureProofResponse(false, "Signed data is required"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PluginBuilder/Services/GPGKeyService.cs` around lines 108 - 113, In
VerifyDetachedSignature add a null/empty guard for the rawSignedBytes parameter
similar to the existing signatureBytes check: before any use (prior to calling
signature.Update(rawSignedBytes)) validate rawSignedBytes is not null and Length
> 0 and return a clear SignatureProofResponse(false, "Signed data is required")
if it fails; update the method VerifyDetachedSignature and any related
early-return logic so the null case is rejected explicitly rather than allowing
signature.Update(rawSignedBytes) to throw.
thgO-O
left a comment
There was a problem hiding this comment.
Thanks for the PR @Kukks!
I have a few points before merge:
The version lifecycle logic (release, sign_release, unrelease, remove) is now implemented in both ApiController and PluginController.
This is already diverging (API checks build.state == uploaded, UI flow does not), so behavior is inconsistent depending on entry point.
In API RemoveVersion, we update the build directly via DB (conn.UpdateBuild(...)) instead of using buildService.UpdateBuild(...) like the UI path. That likely skips the BuildChanged event.
Suggestion: move these lifecycle operations into a single shared service and call it from both controllers. This will remove duplication and keep rules consistent.
Would be great if you could add tests for the new API endpoints also.
Summary
statefield to build data responsesNew Endpoints (all require HTTP Basic Auth + plugin ownership)
GET/api/v1/plugins/{slug}/buildsPOST/api/v1/plugins/{slug}/versions/{ver}/release{"signature":"<base64>"})POST/api/v1/plugins/{slug}/versions/{ver}/unreleaseDELETE/api/v1/plugins/{slug}/versions/{ver}Other Changes
BuildDatanow includesstatefield (also returned by existingGET /builds/{buildId})GPGKeyServicegains abyte[]overload so the JSON API can accept base64 signatures instead of file uploadsManifestHelperextracted fromPluginControllerprivate methods to shared utilityReleaseVersionRequestmodel: whensignatureis provided, performs GPG-signed release; when absent, plain release (blocked if plugin requires GPG)Test plan
dotnet buildpasses with 0 errors, 0 warningsGET /api/v1/plugins/{slug}/buildsreturns build list with state fieldPOST .../versions/{ver}/releasewith empty body releases a versionPOST .../versions/{ver}/releasewith{"signature":"..."}performs signed releasePOST .../versions/{ver}/releaseblocked whenRequireGPGSignatureForReleaseis true and no signaturePOST .../versions/{ver}/unreleasereverts to pre-release and clears signature proofDELETE .../versions/{ver}removes version and sets build state to removedGET /api/v1/plugins/{slug}/builds/{buildId}now includesstatePluginControllerrelease/build views still work correctly afterManifestHelperextraction