clients/php: apply audit findings — token redaction + uploadStream + tests (1cff9b8 → next)
HIGH:
- H1: __debugInfo() redacts token on Client + AppToken; #[\SensitiveParameter]
on Client constructor's $token param so PHP scrubs it from stack traces.
MEDIUM:
- M1: uploadStream(StreamInterface, filename, ttl) overload so callers
handling form uploads have a non-path entry point. README warning above
the API table on uploadFile path-trust.
- M2: RunRequest now rejects empty-string model/system in the constructor
(callers should pass null/omit rather than '' to use defaults).
- M3: new MalformedResponseException extends ForgeException for
"transport succeeded, body unparseable as expected JSON object". Decoupled
from ApiException so callers can distinguish "server told me no" from
"server replied 200 with garbage". README + ApiException docstring updated.
- M4: non-UTF-8 / malformed JSON now flows through M3's new exception.
- M5: ApiException error-message extraction falls back to json_encode
(capped at 200 chars) when the error field is an object/array, so
callers don't get empty messages on {"error":{"code":...,"msg":...}}.
LOW:
- L2: revokeToken now requires server response ok === true, raises
MalformedResponseException on missing/false ok rather than silently
returning true.
- L5: README WordPress snippet uses bare Client (matches the use line above).
- L7: 29 new tests — token redaction (3), uploadStream (2), empty
model/system (2), MalformedResponseException across 7 scenarios incl.
non-UTF-8, ApiException object-error formatting + 200-char cap, revoke
ok=true requirement + ok=false + empty-name, RunRequest timeout bounds
(3) + non-string/empty files entries (2), uploadFile unreadable-path
+ 4xx + 5xx, healthz 500, Authorization header asserted on every
endpoint.
README polish: TLS verify=false caveat under "Custom HTTP client".
Audit memo: memory/clawdforge-audits/php-1cff9b8.md
This commit is contained in:
parent
e9d5e0ea16
commit
7745c5eb5c
7 changed files with 649 additions and 22 deletions
|
|
@ -9,6 +9,7 @@ use Clawdforge\Client;
|
|||
use Clawdforge\Exception\ApiException;
|
||||
use Clawdforge\Exception\AuthException;
|
||||
use Clawdforge\Exception\ForgeException;
|
||||
use Clawdforge\Exception\MalformedResponseException;
|
||||
use Clawdforge\Exception\TransportException;
|
||||
use Clawdforge\FileToken;
|
||||
use Clawdforge\RunRequest;
|
||||
|
|
@ -20,6 +21,7 @@ use GuzzleHttp\HandlerStack;
|
|||
use GuzzleHttp\Middleware;
|
||||
use GuzzleHttp\Psr7\Request;
|
||||
use GuzzleHttp\Psr7\Response;
|
||||
use GuzzleHttp\Psr7\Utils;
|
||||
use InvalidArgumentException;
|
||||
use PHPUnit\Framework\TestCase;
|
||||
|
||||
|
|
@ -337,4 +339,448 @@ final class ClientTest extends TestCase
|
|||
$this->expectException(InvalidArgumentException::class);
|
||||
new Client(self::BASE_URL, '');
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------
|
||||
// Audit-driven tests (1cff9b8 → next)
|
||||
// ---------------------------------------------------------------------
|
||||
|
||||
// H1 — token redaction --------------------------------------------------
|
||||
|
||||
public function testClientDebugInfoRedactsToken(): void
|
||||
{
|
||||
$client = new Client(self::BASE_URL, self::TOKEN);
|
||||
|
||||
// print_r honours __debugInfo() — this is what most framework error
|
||||
// reflectors (Whoops, Symfony VarDumper, Laravel Ignition, etc.) hit.
|
||||
$printed = print_r($client, true);
|
||||
self::assertStringNotContainsString(self::TOKEN, $printed);
|
||||
self::assertStringContainsString('***redacted***', $printed);
|
||||
|
||||
// var_dump() likewise routes through __debugInfo().
|
||||
ob_start();
|
||||
var_dump($client);
|
||||
$dumped = (string) ob_get_clean();
|
||||
self::assertStringNotContainsString(self::TOKEN, $dumped);
|
||||
self::assertStringContainsString('redacted', $dumped);
|
||||
|
||||
// The __debugInfo() return value itself must not include the bearer.
|
||||
$info = $client->__debugInfo();
|
||||
self::assertSame('***redacted***', $info['token']);
|
||||
self::assertSame(self::BASE_URL, $info['baseUrl']);
|
||||
}
|
||||
|
||||
public function testAppTokenDebugInfoRedactsToken(): void
|
||||
{
|
||||
$plaintext = 'cf_brandnew_super_secret_xxxxxx';
|
||||
$tok = new AppToken(name: 'cauldron', token: $plaintext);
|
||||
|
||||
$printed = print_r($tok, true);
|
||||
self::assertStringNotContainsString($plaintext, $printed);
|
||||
self::assertStringContainsString('***redacted***', $printed);
|
||||
|
||||
ob_start();
|
||||
var_dump($tok);
|
||||
$dumped = (string) ob_get_clean();
|
||||
self::assertStringNotContainsString($plaintext, $dumped);
|
||||
|
||||
// List-row tokens (no plaintext) should remain null in the debug
|
||||
// view — no false redaction marker.
|
||||
$listed = new AppToken(name: 'cauldron', token: null);
|
||||
$printedListed = print_r($listed, true);
|
||||
self::assertStringNotContainsString('redacted', $printedListed);
|
||||
}
|
||||
|
||||
public function testCreateTokenResponseRedactsPlaintextOnDebug(): void
|
||||
{
|
||||
$plaintext = 'cf_brandnew_xxx';
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], (string) json_encode([
|
||||
'name' => 'cauldron',
|
||||
'token' => $plaintext,
|
||||
'ip_cidrs' => [],
|
||||
])),
|
||||
]);
|
||||
|
||||
$tok = $client->createToken('cauldron');
|
||||
// The plaintext is still accessible on the property — that's the contract.
|
||||
self::assertSame($plaintext, $tok->token);
|
||||
// ...but it must not bleed into reflective output that goes through __debugInfo().
|
||||
$printed = print_r($tok, true);
|
||||
self::assertStringNotContainsString($plaintext, $printed);
|
||||
}
|
||||
|
||||
// M1 — uploadStream overload --------------------------------------------
|
||||
|
||||
public function testUploadStreamFromPsr7(): void
|
||||
{
|
||||
$history = [];
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], (string) json_encode([
|
||||
'file_token' => 'ff_stream',
|
||||
'ttl_secs' => 600,
|
||||
'size' => 5,
|
||||
])),
|
||||
], $history);
|
||||
|
||||
$stream = Utils::streamFor('hello');
|
||||
|
||||
$ft = $client->uploadStream($stream, 'greeting.txt', 600);
|
||||
|
||||
self::assertSame('ff_stream', $ft->fileToken);
|
||||
self::assertSame(600, $ft->ttlSecs);
|
||||
|
||||
/** @var \Psr\Http\Message\RequestInterface $req */
|
||||
$req = $history[0]['request'];
|
||||
self::assertSame('POST', $req->getMethod());
|
||||
self::assertSame('/files', $req->getUri()->getPath());
|
||||
self::assertSame('Bearer ' . self::TOKEN, $req->getHeaderLine('Authorization'));
|
||||
$body = (string) $req->getBody();
|
||||
self::assertStringContainsString('name="file"', $body);
|
||||
self::assertStringContainsString('filename="greeting.txt"', $body);
|
||||
self::assertStringContainsString('hello', $body);
|
||||
}
|
||||
|
||||
public function testUploadStreamRejectsEmptyFilename(): void
|
||||
{
|
||||
$client = $this->makeClient([]); // no requests should fly
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$client->uploadStream(Utils::streamFor('x'), '', 60);
|
||||
}
|
||||
|
||||
// M2 — empty model/system rejection -------------------------------------
|
||||
|
||||
public function testRunRequestRejectsEmptyModel(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
new RunRequest(prompt: 'hi', model: '');
|
||||
}
|
||||
|
||||
public function testRunRequestRejectsEmptySystem(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
new RunRequest(prompt: 'hi', system: '');
|
||||
}
|
||||
|
||||
// M3/M4 — MalformedResponseException -------------------------------------
|
||||
|
||||
public function testMalformedResponseExceptionOnRun(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, ['Content-Type' => 'text/html'], '<html>not json</html>'),
|
||||
]);
|
||||
|
||||
try {
|
||||
$client->run(new RunRequest(prompt: 'hi'));
|
||||
self::fail('expected MalformedResponseException');
|
||||
} catch (MalformedResponseException $e) {
|
||||
self::assertSame(200, $e->statusCode);
|
||||
self::assertSame('<html>not json</html>', $e->body);
|
||||
self::assertInstanceOf(ForgeException::class, $e);
|
||||
self::assertNotInstanceOf(ApiException::class, $e);
|
||||
}
|
||||
}
|
||||
|
||||
public function testMalformedResponseExceptionOnHealthz(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], 'definitely not json'),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->healthz();
|
||||
}
|
||||
|
||||
public function testMalformedResponseExceptionOnFiles(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], 'still not json'),
|
||||
]);
|
||||
|
||||
$tmp = tempnam(sys_get_temp_dir(), 'cf_');
|
||||
self::assertNotFalse($tmp);
|
||||
file_put_contents($tmp, 'x');
|
||||
try {
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->uploadFile($tmp);
|
||||
} finally {
|
||||
@unlink($tmp);
|
||||
}
|
||||
}
|
||||
|
||||
public function testMalformedResponseExceptionOnListTokensNonObject(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], (string) json_encode(['unexpected' => 'shape'])),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->listTokens();
|
||||
}
|
||||
|
||||
public function testMalformedResponseExceptionOnListTokensGarbageBody(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], '<<<not json>>>'),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->listTokens();
|
||||
}
|
||||
|
||||
public function testMalformedResponseExceptionOnCreateToken(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], 'garbage'),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->createToken('cauldron');
|
||||
}
|
||||
|
||||
public function testNonUtf8BodyDegradesToMalformedException(): void
|
||||
{
|
||||
// A latin-1 / non-UTF-8 byte sequence is invalid JSON and json_decode
|
||||
// returns null — the SDK must surface this as MalformedResponseException
|
||||
// rather than slipping a null through.
|
||||
$body = "\xff\xfe\xfd\xfc not utf8";
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], $body),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->healthz();
|
||||
}
|
||||
|
||||
// M5 — object error fields surface a JSON-encoded snippet ---------------
|
||||
|
||||
public function testApiExceptionMessageWithObjectError(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(500, [], (string) json_encode([
|
||||
'error' => ['code' => 'internal', 'msg' => 'bad'],
|
||||
])),
|
||||
]);
|
||||
|
||||
try {
|
||||
$client->healthz();
|
||||
self::fail('expected ApiException');
|
||||
} catch (ApiException $e) {
|
||||
self::assertSame(500, $e->statusCode);
|
||||
$msg = $e->getMessage();
|
||||
self::assertNotEmpty($msg);
|
||||
// Should round-trip the JSON of the object so callers see the
|
||||
// structured error rather than an empty trailing message.
|
||||
self::assertStringContainsString('internal', $msg);
|
||||
self::assertStringContainsString('bad', $msg);
|
||||
}
|
||||
}
|
||||
|
||||
public function testApiExceptionMessageObjectErrorCappedAt200Chars(): void
|
||||
{
|
||||
$longMsg = str_repeat('A', 500);
|
||||
$client = $this->makeClient([
|
||||
new Response(500, [], (string) json_encode([
|
||||
'error' => ['code' => 'overflow', 'msg' => $longMsg],
|
||||
])),
|
||||
]);
|
||||
|
||||
try {
|
||||
$client->healthz();
|
||||
self::fail('expected ApiException');
|
||||
} catch (ApiException $e) {
|
||||
$msg = $e->getMessage();
|
||||
// Prefix is "500 <reason>: " and then the snippet — snippet
|
||||
// itself must be capped at 200 chars.
|
||||
self::assertLessThanOrEqual(
|
||||
300,
|
||||
strlen($msg),
|
||||
'message snippet should be capped near 200 chars'
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// L2 — revokeToken requires ok === true ---------------------------------
|
||||
|
||||
public function testRevokeTokenRequiresOkTrue(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], (string) json_encode([])),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->revokeToken('cauldron');
|
||||
}
|
||||
|
||||
public function testRevokeTokenRejectsOkFalse(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], (string) json_encode(['ok' => false])),
|
||||
]);
|
||||
|
||||
$this->expectException(MalformedResponseException::class);
|
||||
$client->revokeToken('cauldron');
|
||||
}
|
||||
|
||||
public function testRevokeTokenRejectsEmptyName(): void
|
||||
{
|
||||
$client = $this->makeClient([]);
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$client->revokeToken('');
|
||||
}
|
||||
|
||||
// L7 — coverage gaps -----------------------------------------------------
|
||||
|
||||
public function testRunRequestRejectsTimeoutBelowFloor(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
new RunRequest(prompt: 'hi', timeoutSecs: 4);
|
||||
}
|
||||
|
||||
public function testRunRequestRejectsTimeoutAboveCeiling(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
new RunRequest(prompt: 'hi', timeoutSecs: 601);
|
||||
}
|
||||
|
||||
public function testRunRequestAcceptsTimeoutAtBounds(): void
|
||||
{
|
||||
$low = new RunRequest(prompt: 'hi', timeoutSecs: 5);
|
||||
$high = new RunRequest(prompt: 'hi', timeoutSecs: 600);
|
||||
self::assertSame(5, $low->timeoutSecs);
|
||||
self::assertSame(600, $high->timeoutSecs);
|
||||
}
|
||||
|
||||
public function testRunRequestRejectsNonStringFilesEntry(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
// @phpstan-ignore-next-line — deliberately pushing a bad type
|
||||
new RunRequest(prompt: 'hi', files: ['ff_ok', 123]);
|
||||
}
|
||||
|
||||
public function testRunRequestRejectsEmptyStringFilesEntry(): void
|
||||
{
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
new RunRequest(prompt: 'hi', files: ['ff_ok', '']);
|
||||
}
|
||||
|
||||
public function testUploadFileRejectsUnreadablePath(): void
|
||||
{
|
||||
$client = $this->makeClient([]);
|
||||
$this->expectException(InvalidArgumentException::class);
|
||||
$client->uploadFile('/no/such/file/anywhere/' . uniqid('cf_', true));
|
||||
}
|
||||
|
||||
public function testUploadFile4xxRaisesApiException(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(413, [], (string) json_encode(['detail' => 'file too large'])),
|
||||
]);
|
||||
|
||||
$tmp = tempnam(sys_get_temp_dir(), 'cf_');
|
||||
self::assertNotFalse($tmp);
|
||||
file_put_contents($tmp, 'x');
|
||||
try {
|
||||
try {
|
||||
$client->uploadFile($tmp);
|
||||
self::fail('expected ApiException');
|
||||
} catch (ApiException $e) {
|
||||
self::assertSame(413, $e->statusCode);
|
||||
self::assertStringContainsString('file too large', $e->getMessage());
|
||||
}
|
||||
} finally {
|
||||
@unlink($tmp);
|
||||
}
|
||||
}
|
||||
|
||||
public function testUploadFile5xxRaisesApiException(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(503, [], (string) json_encode(['error' => 'storage unavailable'])),
|
||||
]);
|
||||
|
||||
$tmp = tempnam(sys_get_temp_dir(), 'cf_');
|
||||
self::assertNotFalse($tmp);
|
||||
file_put_contents($tmp, 'x');
|
||||
try {
|
||||
try {
|
||||
$client->uploadFile($tmp);
|
||||
self::fail('expected ApiException');
|
||||
} catch (ApiException $e) {
|
||||
self::assertSame(503, $e->statusCode);
|
||||
self::assertNotInstanceOf(MalformedResponseException::class, $e);
|
||||
}
|
||||
} finally {
|
||||
@unlink($tmp);
|
||||
}
|
||||
}
|
||||
|
||||
public function testHealthz500RaisesApiException(): void
|
||||
{
|
||||
$client = $this->makeClient([
|
||||
new Response(500, [], (string) json_encode(['error' => 'boom'])),
|
||||
]);
|
||||
|
||||
try {
|
||||
$client->healthz();
|
||||
self::fail('expected ApiException');
|
||||
} catch (ApiException $e) {
|
||||
self::assertSame(500, $e->statusCode);
|
||||
self::assertNotInstanceOf(AuthException::class, $e);
|
||||
self::assertStringContainsString('boom', $e->getMessage());
|
||||
}
|
||||
}
|
||||
|
||||
public function testAuthorizationHeaderSentOnEveryEndpoint(): void
|
||||
{
|
||||
$history = [];
|
||||
$client = $this->makeClient([
|
||||
new Response(200, [], (string) json_encode([
|
||||
'ok' => true, 'claude_present' => true, 'claude_version' => '1',
|
||||
])),
|
||||
new Response(200, [], (string) json_encode([
|
||||
'ok' => true, 'result' => 'x', 'duration_ms' => 1, 'stop_reason' => 'end_turn',
|
||||
])),
|
||||
new Response(200, [], (string) json_encode([
|
||||
'file_token' => 'ff_x', 'ttl_secs' => 60, 'size' => 1,
|
||||
])),
|
||||
new Response(200, [], (string) json_encode([
|
||||
'name' => 'a', 'token' => 'cf_a', 'ip_cidrs' => [],
|
||||
])),
|
||||
new Response(200, [], (string) json_encode([
|
||||
'tokens' => [],
|
||||
])),
|
||||
new Response(200, [], (string) json_encode(['ok' => true])),
|
||||
], $history);
|
||||
|
||||
$client->healthz();
|
||||
$client->run(new RunRequest(prompt: 'hi'));
|
||||
|
||||
$tmp = tempnam(sys_get_temp_dir(), 'cf_');
|
||||
self::assertNotFalse($tmp);
|
||||
file_put_contents($tmp, 'x');
|
||||
try {
|
||||
$client->uploadFile($tmp);
|
||||
} finally {
|
||||
@unlink($tmp);
|
||||
}
|
||||
$client->createToken('a');
|
||||
$client->listTokens();
|
||||
$client->revokeToken('a');
|
||||
|
||||
self::assertCount(6, $history);
|
||||
foreach ($history as $i => $entry) {
|
||||
/** @var \Psr\Http\Message\RequestInterface $req */
|
||||
$req = $entry['request'];
|
||||
self::assertSame(
|
||||
'Bearer ' . self::TOKEN,
|
||||
$req->getHeaderLine('Authorization'),
|
||||
"Authorization missing/wrong on request #{$i} ({$req->getMethod()} {$req->getUri()->getPath()})"
|
||||
);
|
||||
self::assertSame(
|
||||
'application/json',
|
||||
$req->getHeaderLine('Accept'),
|
||||
"Accept missing on request #{$i}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue