Skip to content

Commit 061b9f8

Browse files
mneudertmneudert
authored
Append upstream API exception messages in ApiCallTool (#53)
1 parent 21e74b3 commit 061b9f8

5 files changed

Lines changed: 382 additions & 100 deletions

File tree

‎Services/Api/ApiCallQueryService.php‎

Lines changed: 47 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,15 @@ private function normalizeValue(mixed $value, string $context): mixed
115115

116116
private function buildFailureMessage(CoreApiRequestException $e): string
117117
{
118-
$detail = $this->extractSafeFailureDetail($e);
118+
$detail = $this->extractFailureDetail($e);
119119
if ($detail === null) {
120120
return self::GENERIC_FAILURE_MESSAGE;
121121
}
122122

123123
return self::DETAILED_FAILURE_PREFIX . $detail;
124124
}
125125

126-
private function extractSafeFailureDetail(CoreApiRequestException $e): ?string
126+
private function extractFailureDetail(CoreApiRequestException $e): ?string
127127
{
128128
$previous = $e->getPrevious();
129129
if (!$previous instanceof \Throwable) {
@@ -140,73 +140,74 @@ private function extractSafeFailureDetail(CoreApiRequestException $e): ?string
140140
return null;
141141
}
142142

143-
if (!$this->isSafeFailureDetail($message)) {
143+
if ($this->shouldSuppressFailureDetail($message)) {
144144
return null;
145145
}
146146

147147
return $message . '.';
148148
}
149149

150-
private function isSafeFailureDetail(string $message): bool
150+
private function shouldSuppressFailureDetail(string $message): bool
151151
{
152-
if (strlen($message) > 160) {
153-
return false;
154-
}
155-
156152
$normalized = strtolower($message);
153+
157154
$unsafeFragments = [
155+
// Secret/session-token wording. Access-denial phrasing is routed
156+
// via NoAccessLikeErrorDetector before this denylist runs.
157+
'token_auth',
158+
'bearer ',
159+
'session token',
160+
'session id',
161+
'phpsessid',
162+
'force_api_session',
163+
164+
// SQL internals. Verb+clause SQL is handled by the regex below.
158165
'sqlstate',
159-
'select ',
160-
'insert ',
161-
'update ',
162-
'delete ',
163166
'create table',
164167
'drop table',
165168
'alter table',
166-
'exception',
167-
'stack trace',
168-
' in /',
169-
' at /',
170-
'/var/www/',
171-
'\\',
172-
'<',
173-
'>',
174-
'token_auth',
175-
'bearer ',
176-
'session',
177-
'permission denied',
178-
'call to ',
179-
'uncaught ',
169+
170+
// PHP runtime crash wording. "Uncaught"/"Stack trace" live in
171+
// getTraceAsString(), not getMessage(), so they are not listed.
172+
'call to undefined ',
173+
174+
// Internal invariant/class-name failures.
175+
'sanity check:',
176+
'unknown datatable type',
177+
'unexpected datatable type',
178+
179+
// Filesystem path leakage.
180+
"wasn't found in ",
181+
182+
// SegmentEditor wraps nested parser/validator exception text verbatim.
183+
'the specified segment is invalid:',
180184
];
181185

182186
foreach ($unsafeFragments as $fragment) {
183187
if (str_contains($normalized, $fragment)) {
184-
return false;
188+
return true;
185189
}
186190
}
187191

188-
if (preg_match('/#\d+/', $message) === 1) {
189-
return false;
192+
// Raw SQL leakage: verb + matching clause keyword. The clause context
193+
// avoids suppressing prose like "please select a value" or
194+
// "failed to delete user".
195+
if (
196+
preg_match(
197+
'/\bselect\b.{0,200}\bfrom\b|\binsert\s+into\b|\bupdate\b.{0,200}\bset\b|\bdelete\s+from\b/s',
198+
$normalized,
199+
) === 1
200+
) {
201+
return true;
190202
}
191203

192-
$safeSignals = [
193-
'parameter',
194-
'missing',
195-
'invalid',
196-
'must be',
197-
'required',
198-
'expected',
199-
'unknown value',
200-
'not supported',
201-
'out of range',
202-
];
203-
204-
foreach ($safeSignals as $signal) {
205-
if (str_contains($normalized, $signal)) {
206-
return true;
207-
}
204+
// Namespaced class-like tokens ("Piwik\DataTable\Map"). Require two
205+
// PascalCase segments so stray escapes like "\n" or "\d" do not match.
206+
if (preg_match('/\b[A-Z][A-Za-z0-9_]+(?:\\\\[A-Z][A-Za-z0-9_]*){1,}/', $message) === 1) {
207+
return true;
208208
}
209209

210-
return false;
210+
// Absolute filesystem paths.
211+
return preg_match('~(?:^|[\s(])/(?:[^/\s]+/)+[^/\s)]+~', $message) === 1;
211212
}
212213
}

‎Support/Errors/NoAccessLikeErrorDetector.php‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,27 @@ public static function isDetected(\Throwable $e): bool
2424
return false;
2525
}
2626

27-
return str_contains($message, 'no access')
27+
if (
28+
str_contains($message, 'no access')
2829
|| str_contains($message, 'checkuserhasviewaccess')
29-
|| str_contains($message, 'view access');
30+
|| str_contains($message, 'view access')
31+
|| str_contains($message, 'access denied')
32+
|| str_contains($message, 'permission denied')
33+
) {
34+
return true;
35+
}
36+
37+
if (preg_match('/\bnot authorized\b/', $message) === 1) {
38+
return true;
39+
}
40+
41+
// "unauthorized" needs access context so validation wording like
42+
// "unauthorized character in input" does not get routed to the
43+
// no-access path. Allow bare "Unauthorized" replies (end-of-string
44+
// or trailing punctuation) and access-context nouns.
45+
return preg_match(
46+
'/\bunauthorized(?:\s+(?:access|request|user|client|api|to)\b|\s*(?=[.!?,:;)]|$))/',
47+
$message,
48+
) === 1;
3049
}
3150
}

‎tests/Integration/McpTools/ApiCallTest.php‎

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ public function testFullModeRejectsBlockedProxyLikeMethodBySplitSelector(): void
280280
);
281281
}
282282

283-
public function testFullModeAttemptsMutatingMethodCall(): void
283+
public function testFullModeReturnsDetailedValidationErrorForAddUser(): void
284284
{
285285
McpTestHelper::setRawApiAccessMode('full');
286286

@@ -295,17 +295,33 @@ public function testFullModeAttemptsMutatingMethodCall(): void
295295
);
296296

297297
McpTestHelper::assertToolError($result);
298-
$content = $result->content[0] ?? null;
299-
self::assertInstanceOf(\Matomo\Dependencies\McpServer\Mcp\Schema\Content\TextContent::class, $content);
300-
$errorText = $content->text;
301-
self::assertIsString($errorText);
302-
self::assertTrue(
303-
$errorText === 'Matomo API request failed.'
304-
|| str_starts_with($errorText, 'Matomo API request failed: '),
298+
$this->assertIsDetailedMissingParameterError($result);
299+
}
300+
301+
public function testFullModeSuppressesNestedSegmentValidationError(): void
302+
{
303+
McpTestHelper::setRawApiAccessMode('full');
304+
305+
$server = McpTestHelper::buildServer();
306+
$sessionId = McpTestHelper::initializeSession($server);
307+
McpTestHelper::callToolAndAssertError(
308+
$server,
309+
$sessionId,
310+
ApiCallFull::TOOL_NAME,
311+
[
312+
'method' => 'SegmentEditor.add',
313+
'parameters' => [
314+
'name' => 'Broken Segment',
315+
'definition' => 'invalidSegmentDefinition==value',
316+
'idSite' => $this->idSite,
317+
],
318+
],
319+
'Matomo API request failed.',
320+
__METHOD__,
305321
);
306322
}
307323

308-
public function testCreateModeAttemptsCreateMethodCall(): void
324+
public function testCreateModeReturnsDetailedValidationErrorForAddUser(): void
309325
{
310326
McpTestHelper::setRawApiAccessMode('create');
311327

@@ -320,11 +336,10 @@ public function testCreateModeAttemptsCreateMethodCall(): void
320336
);
321337

322338
McpTestHelper::assertToolError($result);
323-
$content = $result->content[0] ?? null;
324-
self::assertInstanceOf(\Matomo\Dependencies\McpServer\Mcp\Schema\Content\TextContent::class, $content);
339+
$this->assertIsDetailedMissingParameterError($result);
325340
}
326341

327-
public function testDeleteModeAttemptsDeleteMethodCall(): void
342+
public function testDeleteModeReturnsDetailedValidationErrorForDeleteSite(): void
328343
{
329344
McpTestHelper::setRawApiAccessMode('delete');
330345

@@ -339,17 +354,10 @@ public function testDeleteModeAttemptsDeleteMethodCall(): void
339354
);
340355

341356
McpTestHelper::assertToolError($result);
342-
$content = $result->content[0] ?? null;
343-
self::assertInstanceOf(\Matomo\Dependencies\McpServer\Mcp\Schema\Content\TextContent::class, $content);
344-
$errorText = $content->text;
345-
self::assertIsString($errorText);
346-
self::assertTrue(
347-
$errorText === 'Matomo API request failed.'
348-
|| str_starts_with($errorText, 'Matomo API request failed: '),
349-
);
357+
$this->assertIsDetailedMissingParameterError($result);
350358
}
351359

352-
public function testUpdateModeAttemptsUpdateMethodCall(): void
360+
public function testUpdateModeReturnsDetailedValidationErrorForUpdateUser(): void
353361
{
354362
McpTestHelper::setRawApiAccessMode('update');
355363

@@ -364,14 +372,7 @@ public function testUpdateModeAttemptsUpdateMethodCall(): void
364372
);
365373

366374
McpTestHelper::assertToolError($result);
367-
$content = $result->content[0] ?? null;
368-
self::assertInstanceOf(\Matomo\Dependencies\McpServer\Mcp\Schema\Content\TextContent::class, $content);
369-
$errorText = $content->text;
370-
self::assertIsString($errorText);
371-
self::assertTrue(
372-
$errorText === 'Matomo API request failed.'
373-
|| str_starts_with($errorText, 'Matomo API request failed: '),
374-
);
375+
$this->assertIsDetailedMissingParameterError($result);
375376
}
376377

377378
public function testRejectsReservedParameterKeys(): void
@@ -530,4 +531,14 @@ private function listToolNamesForCurrentConfig(): array
530531

531532
return array_values(array_map(static fn($tool) => $tool->name, $result->tools));
532533
}
534+
535+
private function assertIsDetailedMissingParameterError(
536+
\Matomo\Dependencies\McpServer\Mcp\Schema\Result\CallToolResult $result,
537+
): void {
538+
$content = $result->content[0] ?? null;
539+
self::assertInstanceOf(\Matomo\Dependencies\McpServer\Mcp\Schema\Content\TextContent::class, $content);
540+
self::assertIsString($content->text);
541+
self::assertStringStartsWith('Matomo API request failed: ', $content->text);
542+
self::assertStringContainsString('General_PleaseSpecifyValue', $content->text);
543+
}
533544
}

0 commit comments

Comments
 (0)