Sulkta-Coop/clawdforge
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

clients/csharp: apply audit findings — JSON depth caps + stream lifecycle (09aca58 → new)

Browse source 
MEDIUM:
- M1: JsonSerializerOptions.MaxDepth = 32 on the consolidated
  JsonDefaults.Options (referenced from both ForgeClient and
  RunResult.AsJson<T>) so the result payload's arbitrary upstream JSON
  cannot stack-walk the runtime.
- M2: JsonDocumentOptions.MaxDepth = 32 in SummarizeBody for parsing
  error-body summaries — defensive belt alongside the existing 8 MiB
  body cap.
- M3: UploadStreamAsync doc updated to match reality — the input stream
  IS disposed when the request completes (matches HttpClient /
  MultipartFormDataContent / StreamContent convention). Old doc was
  incorrect; chose doc-update over a non-disposing wrapper to stay
  closest to standard .NET stream semantics.

LOW:
- L2: RunResult.AsJson<T>() now guards JsonValueKind.Undefined and
  returns default(T) instead of throwing InvalidOperationException
  (e.g. when RunResult is constructed without a server payload).
- L4: IsNullOrWhiteSpace consistent across RunRequest.Prompt,
  CreateTokenRequest.Name, RevokeTokenAsync.name, UploadFileAsync.path,
  UploadStreamAsync.fileName (was IsNullOrEmpty letting space through).

Nit polish:
- BaseUrl cached in ctor instead of rebuilt per access.
- JsonDefaults moved to its own file (Models/JsonDefaults.cs) and is
  now the single source of truth for serializer options across the
  client.
- examples/Basic/Program.cs comment fixed: '60s' → '120s' to match
  TimeSpan.FromSeconds(120).

README:
- HTTPS / WireGuard recommendation in the Notes section — SDK does not
  enforce HTTPS, callers off-LAN should tunnel.
- .NET 8.0.10+ runtime recommendation with cref to CVE-2024-30105 and
  CVE-2024-43485 (SDK does not exercise the affected code paths;
  belt-and-suspenders).
- UploadStream section reflects the corrected disposal contract.

Tests (12 → 19, all passing):
- JsonOpts_MaxDepth_RejectsDeeplyNested — 200-deep result rejected via
  ForgeTransportException wrapping JsonException, no stack overflow.
- SummarizeBody_DeeplyNestedHandled — 200-deep error body still
  produces ForgeAuthException with raw body intact; summary parse
  fails closed without crashing.
- UploadStreamAsync_DisposesCallerStream — DisposeObservingStream
  helper verifies the contract change.
- AsJson_OnUndefinedResult_DefaultReturned — reference + value type.
- RunRequest_PromptWithOnlyWhitespace_Rejected.
- CreateToken_NameWithOnlyWhitespace_Rejected.
- BaseUrl_Cached_ReusesString — Assert.Same identity check.

Build: dotnet build -c Release -m:1 clean (0 warnings, 0 errors).
Tests: dotnet test -c Release -m:1 → 19 passed, 0 failed.
Pack:  dotnet pack -c Release -o dist -m:1 clean.
Vulns: dotnet list package --vulnerable --include-transitive → 0.

Audit: memory/clawdforge-audits/csharp-09aca58.md
This commit is contained in:
Kayos 2026-04-28 23:22:48 -07:00
parent 9866e97977
commit a507ed2a00
6 changed files with 253 additions and 23 deletions
Download patch file Download diff file Expand all files Collapse all files

21
clients/csharp/README.md
View file

@ -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

2
clients/csharp/examples/Basic/Program.cs
View file

@ -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;

44
clients/csharp/src/Clawdforge/ForgeClient.cs
View file

@ -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;
/// </remarks>
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;
/// <summary>The base URL the client targets (no trailing slash).</summary>
public string BaseUrl => _baseUri.GetLeftPart(UriPartial.Authority) + _baseUri.AbsolutePath.TrimEnd('/');
public string BaseUrl => _baseUrlString;
/// <summary>
/// Construct a standalone <see cref="ForgeClient"/> 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<RunResult> 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.
/// </summary>
/// <param name="content">Stream of file bytes; not closed by the SDK.</param>
/// <param name="content">
/// Stream of file bytes. The stream IS disposed by the SDK when the
/// request completes (this matches the standard
/// <see cref="HttpClient"/> /
/// <see cref="MultipartFormDataContent"/> /
/// <see cref="StreamContent"/> contract — they all chain
/// <see cref="IDisposable.Dispose"/> through to the underlying stream).
/// If the caller needs to retain ownership of <paramref name="content"/>,
/// wrap it in a non-disposing adapter before passing it in.
/// </param>
/// <param name="fileName">Filename to advertise on the wire.</param>
/// <param name="ttlSecs">
/// Server clamps to <c>60..86400</c>. Pass <c>0</c> 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
/// </summary>
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;
}

30
clients/csharp/src/Clawdforge/Models/JsonDefaults.cs Normal file
View file

@ -0,0 +1,30 @@
using System.Text.Json;
using System.Text.Json.Serialization;
namespace Clawdforge.Models;
/// <summary>
/// Shared <see cref="JsonSerializerOptions"/> used by the wire client and
/// the <see cref="RunResult"/> helpers.
/// </summary>
/// <remarks>
/// <para>
/// Centralized so that <c>ForgeClient.JsonOpts</c> and
/// <c>RunResult.AsJson&lt;T&gt;()</c> can't drift. Keep all defaults here.
/// </para>
/// <para>
/// <c>MaxDepth = 32</c> caps deserialization recursion. The default is 64
/// and the <c>result</c> payload is arbitrary upstream JSON, so we trim
/// the ceiling defensively to keep a hostile or malformed reply from
/// stack-walking the runtime.
/// </para>
/// </remarks>
internal static class JsonDefaults
{
internal static readonly JsonSerializerOptions Options = new()
{
PropertyNameCaseInsensitive = true,
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
MaxDepth = 32,
};
}

19
clients/csharp/src/Clawdforge/Models/RunResult.cs
View file

@ -46,12 +46,23 @@ public sealed class RunResult
/// Deserialize <see cref="Result"/> as <typeparamref name="T"/> using the
/// supplied options (or a sensible default).
/// </summary>
/// <remarks>
/// Returns <c>default</c> when <see cref="Result"/>'s
/// <see cref="JsonElement.ValueKind"/> is
/// <see cref="JsonValueKind.Undefined"/> (e.g. <c>RunResult</c> was
/// constructed without a server payload). For all other kinds the call
/// is forwarded to <c>JsonElement.Deserialize&lt;T&gt;</c>.
/// </remarks>
/// <exception cref="JsonException">
/// Thrown if <see cref="Result"/> doesn't deserialize into
/// <typeparamref name="T"/>.
/// </exception>
public T? AsJson<T>(JsonSerializerOptions? options = null)
{
if (Result.ValueKind == JsonValueKind.Undefined)
{
return default;
}
return Result.Deserialize<T>(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,
};
}

160
clients/csharp/tests/Clawdforge.Tests/ForgeClientTests.cs
View file

@ -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<ArgumentException>(() =>
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<ArgumentException>(() =>
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<ForgeTransportException>(() =>
client.RunAsync(new RunRequest { Prompt = "x" }));
Assert.IsType<JsonException>(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<ForgeAuthException>(() =>
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<T>() 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<Ingredient>());
// Value type: returns default(int) = 0.
Assert.Equal(0, rr.AsJson<int>());
}
// ---- 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;
}
/// <summary>
/// Wraps an inner stream and flips a flag the first time
/// <see cref="Stream.Dispose(bool)"/> runs. Used by the
/// UploadStreamAsync disposal-contract test.
/// </summary>
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<HttpRequestMessage, CancellationToken, Task<HttpResponseMessage>> _handler;