feat(NET-16): Track public API surface; rename two SDK client methods - #40
Open
cozminu wants to merge 58 commits into
Open
feat(NET-16): Track public API surface; rename two SDK client methods#40cozminu wants to merge 58 commits into
cozminu wants to merge 58 commits into
Conversation
Adds the missing <param> tags for the Uri parameters introduced by the thread-safe base URL refactor (resourceServerUrl/incomingPaymentUrl/ outgoingPaymentUrl) in the Incoming/Outgoing Payment partials, clearing the CS1573 warnings from the documentation build. Also hardens the CI BaseUrl guard: scans all of OpenPayments.Sdk/ (not just Clients/) while excluding generated *.g.cs files, tolerates whitespace around the '=' in "BaseUrl =", and fails loudly instead of silently passing if its scan target ever disappears or is renamed.
Authenticated clients now use a signed named HttpClient carrying SigningHttpMessageHandler, and a separate unsigned client for wallet-address and public incoming-payment reads. Removes the two sync-over-async .Result signing hooks. Closes #17
…ed pipeline test coverage) - Replace the unresolvable <see cref="IHttpClientFactory"/> doc-comment reference in SigningHttpMessageHandler with <c>IHttpClientFactory</c>, since OpenPayments.Sdk.HttpSignatureUtils has no reference to Microsoft.Extensions.Http. This was the only warning in a clean Release build. - Add DI-level tests proving UseOpenPayments wires the "signed" named HttpClient (openpayments-signed) with the signing handler and the "unsigned" named HttpClient (openpayments) without it, so a swapped registration would now fail a test instead of silently shipping.
TryReadError was documented as "Never throws" but could throw ArgumentException when error.code or error.description were non-scalar JSON types (objects, arrays, numbers, etc.). Newtonsoft's explicit (string?) cast only catches JsonException, leaving ArgumentException uncaught. Refactored to extract string conversion logic into TryGetStringValue, which: - Checks token.Type == JTokenType.String before attempting conversion - Returns null for non-string tokens - Catches and swallows ArgumentException as a defensive measure Added test cases covering non-scalar error fields: nested objects, arrays, numbers, and booleans. All 90 tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Transport-level failures (DNS, connection, TLS, timeout) still surface as HttpRequestException/TaskCanceledException from HttpClient before any response exists — the README and XML doc previously implied OpenPaymentsApiException covered all failures.
The generated-methods check already caught legacy ApiException usage in OpenPayments.Sdk/Generated, but Clients/ was only scanned for EnsureSuccessStatusCode, not ApiException — a regression that catches and rethrows a generated ApiException inside ResourceClientBase or AuthClientBase would pass CI undetected.
TryGetStringValue's catch (ArgumentException) could never fire — the only cast is already guarded by a JTokenType.String check — and its comment mischaracterized non-scalar tokens as landing elsewhere. ReadBodyAsync's Content is null guard is dead on modern .NET (Content is never null) and was inconsistent with Helpers.ExtractHeaders, which dereferences response.Content with no such guard.
System.Net must sort first per dotnet_sort_system_directives_first, matching the sibling AuthenticatedClient_Tests.cs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds internal ContentDigestVerifier.MatchesBodyAsync, wired into HttpSignatureValidator.ValidateSignatureAsync right after the existing SignatureInputValidator guard. Closes a signature-bypass gap: the validator previously read the Content-Digest header but never compared it to the actual body, so a request whose body was swapped while replaying the original content headers produced a byte-identical signature base string and validated successfully.
…ader-source mismatch and unsigned-body bypass The whole-branch review of the 7-task signature-roundtrip fix (issue #19) found two independently-shipped, individually-reviewed changes that combined into exploitable gaps: 1. SignatureBaseBuilder.GetHeaderValue (what the Ed25519 signature actually commits to) resolves a covered header from request headers before content headers, but ContentDigestVerifier read Content-Digest from content headers only. Since Content-Digest isn't a recognized .NET content header type, an attacker could set the original digest on a request header (satisfying the signature) and a digest for a swapped body on the content header (satisfying the verifier), forging an arbitrary body under a valid signature. Fixed by making ContentDigestVerifier read the header the same way the signed base string does (GetHeaderValue is now internal instead of private). 2. HttpRequestSigner only covers content-digest/-length/-type when the body is non-empty at signing time, but validation never enforced the converse: a signature over a bodyless request (covering only @method/@target-uri) validated successfully against any body an attacker attached afterward, since nothing in the signature committed to it. Fixed by rejecting validation when the request carries a non-empty body that content-digest doesn't cover. Also fixed: a request missing the Signature header (with Signature-Input present) threw NullReferenceException instead of returning false, via a null-forgiving operator masking a null value all the way to TryParseSignature's first dereference. Four regression tests added; RED/GREEN verified for each finding against the unmodified code before applying the fixes. Full suite: 87/87 passing, 0 warnings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g sections, and cover signed/unsigned pipeline on the resolved client Tightens the AddOpenPaymentsCore ClientUrl guard to reject non-absolute URIs (configuration binding parses "wallet.example" as a valid relative Uri rather than failing), adds a section.Exists() guard so a missing/misspelled IConfiguration section no longer silently registers nothing, switches Assert.Single(services.Where(...)) to the predicate overload to clear xUnit2031 warnings, and cleans up temp key-file directories via IDisposable so test runs stop leaving Ed25519 PEM files scattered in the system temp dir. Also adds a test that resolves IAuthenticatedClient from a real ServiceProvider and proves its signed pipeline attaches Signature/Signature-Input headers while its unsigned pipeline does not. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… missing-section message Reverts the parameter type from IConfigurationSection back to IConfiguration as per the plan review. Changes the error message to no longer reference section.Path (which only exists on IConfigurationSection). Updated the test assertion to match the path-free message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move the inline BaseUrl and error-handling guard checks out of build.yaml into executable scripts under .github/workflows/scripts/ for readability.
…ted/ invariants Fixes gaps found in final whole-branch review: release.yaml regenerated DTOs with an unpinned NSwag before packing (defeating the drift check's purpose), build.yaml's path filters excluded the Makefile/spec submodule/ workflow files most likely to cause real drift, and the drift check itself only compared regeneration against committed output rather than asserting the DTO-only invariants directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wire Microsoft.CodeAnalysis.PublicApiAnalyzers 5.6.0 into OpenPayments.Sdk and OpenPayments.Sdk.HttpSignatureUtils, harvest their PublicAPI.Shipped.txt baselines straight from analyzer output (60 + 822 entries), and add the promote-api Make target that folds Unshipped.txt into Shipped.txt via Task 1's script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Completes a single payment. Aligns the auth layer with the resource layer, which already used the singular name. BREAKING.
Lists many. Aligns the resource layer with the auth layer, which already used the plural name. BREAKING.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…seline sort order, promote-script path matching Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cozminu
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Microsoft.CodeAnalysis.PublicApiAnalyzersto both shipping packages (OpenPayments.Sdk,OpenPayments.Sdk.HttpSignatureUtils) with committedPublicAPI.Shipped.txt/PublicAPI.Unshipped.txtbaselines, so an undeclared public API change now fails the build. Closes Track the public API surface and correct method names #22.IAuthenticatedClient.CompleteIncomingPaymentsAsync→CompleteIncomingPaymentAsync(completes one payment) andIResourceClientBase/ResourceClientBase.ListOutgoingPaymentAsync→ListOutgoingPaymentsAsync(lists many), so the auth and resource client layers agree on singular/plural naming.scripts/promote-public-api.shrelease step (make promote-api),CHANGELOG.md, and a README "Releasing" section documenting the process.See
docs/adr/0001-track-the-public-api-surface.md(local, git-ignored by repo convention) for the full design rationale behind the baseline-seeding and rule-scoping decisions.Test plan
dotnet build OpenPayments.sln -c Release --no-incremental— 0 warnings, 0 errorsdotnet test OpenPayments.sln -c Release— 227/227 passingscripts/promote-public-api.test.sh— 5/5 fixture checks passinggrep -rnacross the repo returns nothing outside the*REMOVED*baseline entry)