diff --git a/clients/csharp/README.md b/clients/csharp/README.md index 4526d49..9201c2d 100644 --- a/clients/csharp/README.md +++ b/clients/csharp/README.md @@ -163,7 +163,12 @@ await client.RunAsync(new RunRequest { ### `UploadStreamAsync(Stream, string fileName, int ttlSecs = 0, CancellationToken)` Same as `UploadFileAsync` but takes any `Stream`, useful for in-memory -blobs or piped data. The stream is not closed by the SDK. +blobs or piped data. The stream **is disposed** by the SDK once the +request completes — this matches the standard `HttpClient` / +`MultipartFormDataContent` / `StreamContent` chain (they all call +`Dispose()` through to the underlying stream). If the caller needs to +retain ownership of the stream, wrap it in a non-disposing adapter +before passing it in. ### Admin (require admin bootstrap token) @@ -282,3 +287,17 @@ dotnet run --project examples/Basic if present — the server ignores it. - File uploads stream via `StreamContent(FileStream)` — the file body is not buffered into memory. +- **Transport security:** the SDK does not enforce HTTPS — `BaseUrl` may + be plain `http://` (clawdforge is a LAN-only service in our + deployment). For any non-localhost / non-LAN target, use HTTPS or + tunnel the connection through WireGuard / a VPN. Bearer tokens travel + in `Authorization` headers; cleartext over an untrusted hop is a token + leak. +- **Runtime:** .NET `8.0.10` or later is recommended. Two BCL CVEs + ([CVE-2024-30105][cve1], [CVE-2024-43485][cve2]) cover JSON DoS paths + the SDK does not currently exercise (`DeserializeAsyncEnumerable` and + `JsonExtensionData`), but pinning to a patched runtime is cheap + belt-and-suspenders insurance. + +[cve1]: https://github.com/dotnet/announcements/issues/322 +[cve2]: https://github.com/dotnet/announcements/issues/325 diff --git a/clients/csharp/examples/Basic/Program.cs b/clients/csharp/examples/Basic/Program.cs index f2dfd4d..b576d6c 100644 --- a/clients/csharp/examples/Basic/Program.cs +++ b/clients/csharp/examples/Basic/Program.cs @@ -20,7 +20,7 @@ using var client = new ForgeClient(new ForgeOptions Token = token, }); -// 60s ceiling for the whole demo via cancellation. +// 120s ceiling for the whole demo via cancellation. using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(120)); var ct = cts.Token; diff --git a/clients/csharp/src/Clawdforge/ForgeClient.cs b/clients/csharp/src/Clawdforge/ForgeClient.cs index a2c4971..ff6ca28 100644 --- a/clients/csharp/src/Clawdforge/ForgeClient.cs +++ b/clients/csharp/src/Clawdforge/ForgeClient.cs @@ -2,7 +2,6 @@ using System.Net; using System.Net.Http.Headers; using System.Net.Http.Json; using System.Text.Json; -using System.Text.Json.Serialization; using Clawdforge.Exceptions; using Clawdforge.Models; @@ -28,19 +27,25 @@ namespace Clawdforge; /// public sealed class ForgeClient : IDisposable { - private static readonly JsonSerializerOptions JsonOpts = new() + // Shared default JSON options live in Models/JsonDefaults.cs to avoid + // duplicate config drift between the wire client and RunResult helpers. + private static readonly JsonSerializerOptions JsonOpts = JsonDefaults.Options; + + // Defensive cap on JSON-document parsing depth used by the error-body + // summarizer. Belt-and-suspenders alongside the 8 MiB body cap. + private static readonly JsonDocumentOptions ErrorBodyDocOpts = new() { - PropertyNameCaseInsensitive = true, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + MaxDepth = 32, }; private readonly HttpClient _http; private readonly bool _ownsHttpClient; private readonly Uri _baseUri; private readonly string? _token; + private readonly string _baseUrlString; /// The base URL the client targets (no trailing slash). - public string BaseUrl => _baseUri.GetLeftPart(UriPartial.Authority) + _baseUri.AbsolutePath.TrimEnd('/'); + public string BaseUrl => _baseUrlString; /// /// Construct a standalone backed by an @@ -68,6 +73,7 @@ public sealed class ForgeClient : IDisposable } _baseUri = new Uri(options.BaseUrl.TrimEnd('/') + "/", UriKind.Absolute); + _baseUrlString = _baseUri.GetLeftPart(UriPartial.Authority) + _baseUri.AbsolutePath.TrimEnd('/'); _token = options.Token; if (httpClient is null) @@ -107,7 +113,7 @@ public sealed class ForgeClient : IDisposable public async Task RunAsync(RunRequest request, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(request); - if (string.IsNullOrEmpty(request.Prompt)) + if (string.IsNullOrWhiteSpace(request.Prompt)) { throw new ArgumentException("RunRequest.Prompt is required", nameof(request)); } @@ -133,7 +139,7 @@ public sealed class ForgeClient : IDisposable int ttlSecs = 0, CancellationToken cancellationToken = default) { - if (string.IsNullOrEmpty(path)) + if (string.IsNullOrWhiteSpace(path)) { throw new ArgumentException("path is required", nameof(path)); } @@ -155,7 +161,16 @@ public sealed class ForgeClient : IDisposable /// in-memory blobs or data piped from another source. The stream is read /// without buffering its contents into the SDK. /// - /// Stream of file bytes; not closed by the SDK. + /// + /// Stream of file bytes. The stream IS disposed by the SDK when the + /// request completes (this matches the standard + /// / + /// / + /// contract — they all chain + /// through to the underlying stream). + /// If the caller needs to retain ownership of , + /// wrap it in a non-disposing adapter before passing it in. + /// /// Filename to advertise on the wire. /// /// Server clamps to 60..86400. Pass 0 to use the server @@ -169,7 +184,7 @@ public sealed class ForgeClient : IDisposable CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(content); - if (string.IsNullOrEmpty(fileName)) + if (string.IsNullOrWhiteSpace(fileName)) { throw new ArgumentException("fileName is required", nameof(fileName)); } @@ -201,7 +216,7 @@ public sealed class ForgeClient : IDisposable CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(request); - if (string.IsNullOrEmpty(request.Name)) + if (string.IsNullOrWhiteSpace(request.Name)) { throw new ArgumentException("CreateTokenRequest.Name is required", nameof(request)); } @@ -228,7 +243,7 @@ public sealed class ForgeClient : IDisposable /// public async Task RevokeTokenAsync(string name, CancellationToken cancellationToken = default) { - if (string.IsNullOrEmpty(name)) + if (string.IsNullOrWhiteSpace(name)) { throw new ArgumentException("name is required", nameof(name)); } @@ -353,7 +368,10 @@ public sealed class ForgeClient : IDisposable if (string.IsNullOrWhiteSpace(body)) return null; try { - using var doc = JsonDocument.Parse(body); + // Cap parse depth to keep a hostile error body from being a DoS + // vector via JSON nesting. 32 is well above any real API shape + // we ever expect from clawdforge. + using var doc = JsonDocument.Parse(body, ErrorBodyDocOpts); if (doc.RootElement.ValueKind == JsonValueKind.Object) { foreach (var key in new[] { "error", "detail", "message" }) @@ -366,7 +384,7 @@ public sealed class ForgeClient : IDisposable } } } - catch (JsonException) { /* not JSON */ } + catch (JsonException) { /* not JSON / too deep */ } return null; } diff --git a/clients/csharp/src/Clawdforge/Models/JsonDefaults.cs b/clients/csharp/src/Clawdforge/Models/JsonDefaults.cs new file mode 100644 index 0000000..75d760d --- /dev/null +++ b/clients/csharp/src/Clawdforge/Models/JsonDefaults.cs @@ -0,0 +1,30 @@ +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace Clawdforge.Models; + +/// +/// Shared used by the wire client and +/// the helpers. +/// +/// +/// +/// Centralized so that ForgeClient.JsonOpts and +/// RunResult.AsJson<T>() can't drift. Keep all defaults here. +/// +/// +/// MaxDepth = 32 caps deserialization recursion. The default is 64 +/// and the result payload is arbitrary upstream JSON, so we trim +/// the ceiling defensively to keep a hostile or malformed reply from +/// stack-walking the runtime. +/// +/// +internal static class JsonDefaults +{ + internal static readonly JsonSerializerOptions Options = new() + { + PropertyNameCaseInsensitive = true, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + MaxDepth = 32, + }; +} diff --git a/clients/csharp/src/Clawdforge/Models/RunResult.cs b/clients/csharp/src/Clawdforge/Models/RunResult.cs index ce8c60f..f88c451 100644 --- a/clients/csharp/src/Clawdforge/Models/RunResult.cs +++ b/clients/csharp/src/Clawdforge/Models/RunResult.cs @@ -46,12 +46,23 @@ public sealed class RunResult /// Deserialize as using the /// supplied options (or a sensible default). /// + /// + /// Returns default when 's + /// is + /// (e.g. RunResult was + /// constructed without a server payload). For all other kinds the call + /// is forwarded to JsonElement.Deserialize<T>. + /// /// /// Thrown if doesn't deserialize into /// . /// public T? AsJson(JsonSerializerOptions? options = null) { + if (Result.ValueKind == JsonValueKind.Undefined) + { + return default; + } return Result.Deserialize(options ?? JsonDefaults.Options); } @@ -72,11 +83,3 @@ public sealed class RunResult } } -internal static class JsonDefaults -{ - internal static readonly JsonSerializerOptions Options = new() - { - PropertyNameCaseInsensitive = true, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, - }; -} diff --git a/clients/csharp/tests/Clawdforge.Tests/ForgeClientTests.cs b/clients/csharp/tests/Clawdforge.Tests/ForgeClientTests.cs index 0ee19c0..c69a468 100644 --- a/clients/csharp/tests/Clawdforge.Tests/ForgeClientTests.cs +++ b/clients/csharp/tests/Clawdforge.Tests/ForgeClientTests.cs @@ -338,6 +338,29 @@ public class ForgeClientTests client.RunAsync(new RunRequest { Prompt = "" })); } + [Fact] + public async Task RunRequest_PromptWithOnlyWhitespace_Rejected() + { + // Audit L4: required-string args use IsNullOrWhiteSpace consistently — + // a prompt of " \t\n " is not a meaningful request. + using var client = new ForgeClient( + new ForgeOptions { BaseUrl = "http://forge.test", Token = "cf_test" }); + + await Assert.ThrowsAsync(() => + client.RunAsync(new RunRequest { Prompt = " \t\n " })); + } + + [Fact] + public async Task CreateToken_NameWithOnlyWhitespace_Rejected() + { + // Audit L4: required-string args use IsNullOrWhiteSpace consistently. + using var client = new ForgeClient( + new ForgeOptions { BaseUrl = "http://forge.test", Token = "admin-token" }); + + await Assert.ThrowsAsync(() => + client.CreateTokenAsync(new CreateTokenRequest { Name = " " })); + } + [Fact] public void BaseUrl_IsTrimmedAtConstruction() { @@ -347,6 +370,111 @@ public class ForgeClientTests Assert.Equal("http://forge.test", client.BaseUrl); } + [Fact] + public void BaseUrl_Cached_ReusesString() + { + // Audit nit: BaseUrl getter shouldn't rebuild the string on every + // access. Identity check confirms a single cached instance. + using var client = new ForgeClient( + new ForgeOptions { BaseUrl = "http://forge.test/", Token = "cf_test" }); + + var first = client.BaseUrl; + var second = client.BaseUrl; + Assert.Same(first, second); + } + + [Fact] + public async Task JsonOpts_MaxDepth_RejectsDeeplyNested() + { + // Audit M1: ForgeClient JsonOpts must cap deserialization depth so + // the server's `result` field can't stack-walk the runtime via + // pathological nesting. We synthesize a 200-deep object and verify + // the SDK surfaces a transport exception (JsonException-wrapped), + // NOT a stack-overflow / process termination. + var deepResult = new string('[', 200) + new string(']', 200); + var body = "{\"ok\":true,\"result\":" + deepResult + + ",\"duration_ms\":1,\"stop_reason\":\"end_turn\"}"; + + var handler = new MockHandler((req, ct) => new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(body, Encoding.UTF8, "application/json"), + }); + + using var http = new HttpClient(handler); + using var client = new ForgeClient( + new ForgeOptions { BaseUrl = "http://forge.test", Token = "cf_test" }, + http); + + var ex = await Assert.ThrowsAsync(() => + client.RunAsync(new RunRequest { Prompt = "x" })); + Assert.IsType(ex.InnerException); + } + + [Fact] + public async Task SummarizeBody_DeeplyNestedHandled() + { + // Audit M2: SummarizeBody parses the error body to extract a + // `detail`/`error`/`message` string. A hostile error body with + // pathological nesting must not crash the summarize path — the + // JsonDocumentOptions.MaxDepth=32 cap means JsonDocument.Parse + // throws JsonException, which SummarizeBody swallows. Verify the + // 401 still surfaces as ForgeAuthException with body intact. + var deeplyNestedBody = "{\"detail\":" + new string('[', 200) + new string(']', 200) + "}"; + + var handler = new MockHandler((req, ct) => new HttpResponseMessage(HttpStatusCode.Unauthorized) + { + Content = new StringContent(deeplyNestedBody, Encoding.UTF8, "application/json"), + }); + + using var http = new HttpClient(handler); + using var client = new ForgeClient( + new ForgeOptions { BaseUrl = "http://forge.test", Token = "cf_test" }, + http); + + var ex = await Assert.ThrowsAsync(() => + client.RunAsync(new RunRequest { Prompt = "x" })); + Assert.Equal(401, ex.StatusCode); + // Raw body is preserved verbatim; only the summary parse is gated. + Assert.Contains("detail", ex.Body); + } + + [Fact] + public async Task UploadStreamAsync_DisposesCallerStream() + { + // Audit M3: documented contract is that the caller's stream IS + // disposed by the SDK once the request completes (matches + // HttpClient/MultipartFormDataContent/StreamContent convention). + var handler = new MockHandler((req, ct) => + JsonResponse(new { file_token = "ff_x", ttl_secs = 3600, size = 5 })); + + using var http = new HttpClient(handler); + using var client = new ForgeClient( + new ForgeOptions { BaseUrl = "http://forge.test", Token = "cf_test" }, + http); + + var observed = new DisposeObservingStream(new MemoryStream(Encoding.UTF8.GetBytes("hello"))); + var ft = await client.UploadStreamAsync(observed, "hello.txt", ttlSecs: 3600); + + Assert.Equal("ff_x", ft.Token); + Assert.True(observed.WasDisposed, + "UploadStreamAsync should dispose the caller's stream — see README contract."); + } + + [Fact] + public void AsJson_OnUndefinedResult_DefaultReturned() + { + // Audit L2: RunResult.AsJson() previously threw InvalidOperationException + // when Result.ValueKind == Undefined (default JsonElement). Guarded + // to return default(T) instead. + var rr = new RunResult { Ok = true, DurationMs = 0 }; + Assert.Equal(JsonValueKind.Undefined, rr.Result.ValueKind); + + // Reference type: returns null. + Assert.Null(rr.AsJson()); + // Value type: returns default(int) = 0. + Assert.Equal(0, rr.AsJson()); + } + // ---- helpers ----------------------------------------------------------- private static HttpResponseMessage JsonResponse(object payload, HttpStatusCode status = HttpStatusCode.OK) @@ -365,6 +493,38 @@ public class ForgeClientTests public string Food { get; init; } = string.Empty; } + /// + /// Wraps an inner stream and flips a flag the first time + /// runs. Used by the + /// UploadStreamAsync disposal-contract test. + /// + private sealed class DisposeObservingStream : Stream + { + private readonly Stream _inner; + public bool WasDisposed { get; private set; } + + public DisposeObservingStream(Stream inner) { _inner = inner; } + + public override bool CanRead => _inner.CanRead; + public override bool CanSeek => _inner.CanSeek; + public override bool CanWrite => _inner.CanWrite; + public override long Length => _inner.Length; + public override long Position { get => _inner.Position; set => _inner.Position = value; } + + public override void Flush() => _inner.Flush(); + public override int Read(byte[] buffer, int offset, int count) => _inner.Read(buffer, offset, count); + public override long Seek(long offset, SeekOrigin origin) => _inner.Seek(offset, origin); + public override void SetLength(long value) => _inner.SetLength(value); + public override void Write(byte[] buffer, int offset, int count) => _inner.Write(buffer, offset, count); + + protected override void Dispose(bool disposing) + { + WasDisposed = true; + if (disposing) _inner.Dispose(); + base.Dispose(disposing); + } + } + private sealed class MockHandler : HttpMessageHandler { private readonly Func> _handler;