Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions Classes/Controller/AjaxController.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
* Provides backend API endpoints for chat and completion requests,
* routing them through the centralized LlmServiceManager.
*
* JSON responses carry raw data; HTML escaping is the frontend's responsibility.
* JSON responses carry raw data — JSON encoding prevents XSS by design.
* Content sanitization for DOM insertion is the frontend's responsibility.
*/
#[AsController]
final readonly class AjaxController
Expand Down Expand Up @@ -140,9 +141,9 @@ public function chatAction(ServerRequestInterface $request): ResponseInterface

return $this->jsonResponseWithRateLimitHeaders([
'success' => true,
'content' => htmlspecialchars($response->content ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'model' => htmlspecialchars($response->model ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'finishReason' => htmlspecialchars($response->finishReason ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'content' => $response->content ?? '',
'model' => $response->model ?? '',
'finishReason' => $response->finishReason ?? '',
], $rateLimitResult);
} catch (ProviderException $e) {
$this->logger->error('Chat provider error', ['exception' => $e->getMessage()]);
Expand Down Expand Up @@ -313,13 +314,13 @@ public function streamAction(ServerRequestInterface $request): ResponseInterface
$generator = $this->llmServiceManager->streamChatWithConfiguration($messages, $configuration);

foreach ($generator as $chunk) {
$chunks[] = 'data: ' . json_encode(['content' => htmlspecialchars($chunk, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')], JSON_THROW_ON_ERROR) . "\n\n";
$chunks[] = 'data: ' . json_encode(['content' => $chunk], JSON_THROW_ON_ERROR) . "\n\n";
}

// Add final "done" event
$chunks[] = 'data: ' . json_encode([
'done' => true,
'model' => htmlspecialchars($configuration->getModelId(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'model' => $configuration->getModelId(),
], JSON_THROW_ON_ERROR) . "\n\n";

$body = implode('', $chunks);
Expand Down Expand Up @@ -408,9 +409,9 @@ public function getTasksAction(ServerRequestInterface $request): ResponseInterfa

$list[] = [
'uid' => $task->getUid(),
'identifier' => htmlspecialchars($task->getIdentifier(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'name' => htmlspecialchars($task->getName(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'description' => htmlspecialchars($task->getDescription(), ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
'identifier' => $task->getIdentifier(),
'name' => $task->getName(),
'description' => $task->getDescription(),
];
}

Expand Down
13 changes: 7 additions & 6 deletions Classes/Domain/DTO/CompleteResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
/**
* Response DTO for completion AJAX endpoint.
*
* All LLM output fields are HTML-escaped server-side to prevent XSS.
* The frontend decodes entities where raw HTML is needed (e.g. CKEditor insertion).
* JSON responses carry raw data — JSON encoding prevents XSS by design.
* Content sanitization for DOM insertion is the frontend's responsibility.
*
* @internal
*/
Expand All @@ -35,15 +35,16 @@ private function __construct(
/**
* Create a successful response from nr-llm CompletionResponse.
*
* All string fields are HTML-escaped to prevent XSS when rendered in the browser.
* Returns raw content — JSON encoding prevents XSS by design.
* Frontend sanitizes content before DOM insertion.
*/
public static function success(CompletionResponse $response): self
{
return new self(
success: true,
content: htmlspecialchars($response->content ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
model: htmlspecialchars($response->model ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
finishReason: htmlspecialchars($response->finishReason ?? '', ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8'),
content: $response->content ?? '',
model: $response->model ?? '',
finishReason: $response->finishReason ?? '',
usage: UsageData::fromUsageStatistics($response->usage),
error: null,
retryAfter: null,
Expand Down
24 changes: 2 additions & 22 deletions Resources/Public/JavaScript/Ckeditor/CowriterDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,15 +251,12 @@ export class CowriterDialog {
);

if (result.success && result.content) {
// Server returns HTML-escaped content for XSS safety;
// decode entities to recover the original HTML for CKEditor.
const decoded = this._decodeEntities(result.content);
preview.replaceChildren();
const safeBody = this._sanitizeHtml(decoded);
const safeBody = this._sanitizeHtml(result.content);
while (safeBody.firstChild) {
preview.appendChild(safeBody.firstChild);
}
resultContent = decoded;
resultContent = result.content;
state = 'result';

if (result.model) {
Expand Down Expand Up @@ -541,23 +538,6 @@ export class CowriterDialog {
return row;
}

/**
* Decode HTML entities back to their original characters.
*
* The server HTML-escapes all LLM output for XSS safety.
* This reverses the escaping so the content can be used as HTML in CKEditor.
* Uses a textarea element which safely decodes entities without executing scripts.
*
* @param {string} escaped
* @returns {string} The decoded HTML string
* @private
*/
_decodeEntities(escaped) {
const textarea = document.createElement('textarea');
textarea.innerHTML = escaped;
return textarea.value;
}

/**
* Parse HTML string safely via DOMParser and remove dangerous elements/attributes.
*
Expand Down
41 changes: 18 additions & 23 deletions Tests/E2E/CowriterWorkflowTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ public function completeWorkflowWithLongContent(): void

#[Test]
#[DataProvider('xssPayloadProvider')]
public function completeWorkflowEscapesXssFromLlm(string $maliciousContent, string $description): void
public function completeWorkflowReturnsRawLlmContent(string $maliciousContent, string $description): void
{
// Arrange: LLM returns malicious content (simulating prompt injection attack)
$llmResponse = $this->createOpenAiResponse(
Expand All @@ -183,15 +183,12 @@ public function completeWorkflowEscapesXssFromLlm(string $maliciousContent, stri
$request = $this->createJsonRequest(['prompt' => 'Improve this text']);
$result = $stack['controller']->completeAction($request);

// Assert: XSS content is escaped through the entire stack
// Assert: Raw content returned in JSON (JSON encoding is inherently XSS-safe;
// frontend sanitizes via DOMParser before DOM insertion)
$data = json_decode((string) $result->getBody(), true, 512, JSON_THROW_ON_ERROR);
self::assertIsArray($data);
self::assertTrue($data['success']);

// Verify HTML entities are used, not raw tags
self::assertStringNotContainsString('<', $data['content'], "Raw < found in: {$description}");
self::assertStringNotContainsString('>', $data['content'], "Raw > found in: {$description}");
self::assertStringContainsString('&lt;', $data['content'], "Missing escaped < in: {$description}");
self::assertSame($maliciousContent, $data['content'], "Content mismatch for: {$description}");
}

/**
Expand Down Expand Up @@ -226,9 +223,9 @@ public static function xssPayloadProvider(): iterable
}

#[Test]
public function completeWorkflowEscapesXssInModelName(): void
public function completeWorkflowReturnsRawModelName(): void
{
// Arrange: LLM returns XSS in model field (edge case)
// Arrange: LLM returns special chars in model field
$llmResponse = new CompletionResponse(
content: 'Normal response',
model: '<script>alert("xss")</script>',
Expand All @@ -246,11 +243,10 @@ public function completeWorkflowEscapesXssInModelName(): void
$request = $this->createJsonRequest(['prompt' => 'Test']);
$result = $stack['controller']->completeAction($request);

// Assert: Model field is also escaped
// Assert: Raw content in JSON (JSON encoding is inherently safe)
$data = json_decode((string) $result->getBody(), true, 512, JSON_THROW_ON_ERROR);
self::assertIsArray($data);
self::assertStringNotContainsString('<script>', $data['model']);
self::assertStringContainsString('&lt;script&gt;', $data['model']);
self::assertSame('<script>alert("xss")</script>', $data['model']);
}

// =========================================================================
Expand Down Expand Up @@ -813,7 +809,7 @@ public function streamWorkflowReturnsChunkedSseResponse(): void
}

#[Test]
public function streamWorkflowEscapesXssInChunks(): void
public function streamWorkflowReturnsRawContentInChunks(): void
{
$stack = $this->createStreamingStack(['<script>alert(1)</script>']);
$config = $this->createLlmConfiguration();
Expand All @@ -825,17 +821,16 @@ public function streamWorkflowEscapesXssInChunks(): void
$events = $this->parseSseEvents((string) $result->getBody());
self::assertCount(2, $events); // 1 content + 1 done

// XSS content must be escaped
self::assertStringNotContainsString('<script>', $events[0]['content']);
self::assertStringContainsString('&lt;script&gt;', $events[0]['content']);
// Raw content in JSON-encoded SSE data (JSON encoding is inherently safe)
self::assertSame('<script>alert(1)</script>', $events[0]['content']);
}

#[Test]
public function streamWorkflowEscapesXssInModelName(): void
public function streamWorkflowReturnsRawModelName(): void
{
$stack = $this->createStreamingStack(['Hello']);

// Create config with XSS in model name
// Create config with special chars in model name
$config = self::createStub(\Netresearch\NrLlm\Domain\Model\LlmConfiguration::class);
$config->method('getIdentifier')->willReturn('test');
$config->method('getName')->willReturn('Test');
Expand All @@ -849,8 +844,7 @@ public function streamWorkflowEscapesXssInModelName(): void
$events = $this->parseSseEvents((string) $result->getBody());
$doneEvent = end($events);
self::assertTrue($doneEvent['done']);
self::assertStringNotContainsString('<img', $doneEvent['model']);
self::assertStringContainsString('&lt;img', $doneEvent['model']);
self::assertSame('<img onerror=alert(1)>', $doneEvent['model']);
}

#[Test]
Expand Down Expand Up @@ -1226,7 +1220,7 @@ public function executeTaskWorkflowWithAdHocRules(): void
}

#[Test]
public function executeTaskWorkflowEscapesXssFromLlm(): void
public function executeTaskWorkflowReturnsRawLlmContent(): void
{
$llmResponse = $this->createOpenAiResponse(
content: '<script>document.cookie</script>Malicious response',
Expand All @@ -1249,11 +1243,12 @@ public function executeTaskWorkflowEscapesXssFromLlm(): void
]);
$result = $stack['controller']->executeTaskAction($request);

// Raw content in JSON (JSON encoding is inherently safe;
// frontend sanitizes via DOMParser before DOM insertion)
$data = json_decode((string) $result->getBody(), true, 512, JSON_THROW_ON_ERROR);
self::assertIsArray($data);
self::assertTrue($data['success']);
self::assertStringNotContainsString('<script>', $data['content']);
self::assertStringContainsString('&lt;script&gt;', $data['content']);
self::assertSame('<script>document.cookie</script>Malicious response', $data['content']);
}

#[Test]
Expand Down
53 changes: 26 additions & 27 deletions Tests/Unit/Controller/AjaxControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ public function chatActionHandlesUnexpectedException(): void
}

#[Test]
public function chatActionEscapesHtmlInResponse(): void
public function chatActionReturnsRawContentInResponse(): void
{
$messages = [['role' => 'user', 'content' => 'Hello']];
$config = $this->createConfigurationMock();
Expand All @@ -325,9 +325,9 @@ public function chatActionEscapesHtmlInResponse(): void

$data = $this->decodeJsonResponse($response);
$this->assertTrue($data['success']);
$this->assertSame('&lt;p&gt;Hello &lt;strong&gt;world&lt;/strong&gt;&lt;/p&gt;', $data['content']);
$this->assertSame('model&#039;s-name', $data['model']);
$this->assertSame('it&#039;s done', $data['finishReason']);
$this->assertSame('<p>Hello <strong>world</strong></p>', $data['content']);
$this->assertSame("model's-name", $data['model']);
$this->assertSame("it's done", $data['finishReason']);
}

// ===========================================
Expand Down Expand Up @@ -416,7 +416,7 @@ public function completeActionReturnsErrorWhenNoConfigurationAvailable(): void
}

#[Test]
public function completeActionEscapesHtmlInContent(): void
public function completeActionReturnsRawHtmlContent(): void
{
$config = $this->createConfigurationMock();
$this->configRepositoryMock
Expand All @@ -432,7 +432,7 @@ public function completeActionEscapesHtmlInContent(): void
$response = $this->subject->completeAction($request);

$data = $this->decodeJsonResponse($response);
$this->assertSame('&lt;p&gt;Improved &lt;strong&gt;text&lt;/strong&gt;&lt;/p&gt;', $data['content']);
$this->assertSame('<p>Improved <strong>text</strong></p>', $data['content']);
}

#[Test]
Expand Down Expand Up @@ -796,7 +796,7 @@ public function streamActionReturnsSseResponseWhenStreamingSupported(): void
}

#[Test]
public function streamActionEscapesHtmlInSseChunks(): void
public function streamActionReturnsRawHtmlInSseChunks(): void
{
$config = $this->createConfigurationMock();
$this->configRepositoryMock->method('findDefault')->willReturn($config);
Expand All @@ -813,14 +813,14 @@ public function streamActionEscapesHtmlInSseChunks(): void
$response->getBody()->rewind();
$body = $response->getBody()->getContents();

// HTML is escaped in SSE data
// Raw content in SSE data (JSON encoding handles safety)
$events = array_filter(explode("\n\n", $body), static fn (string $s): bool => $s !== '');
$firstEvent = json_decode(substr(reset($events), 6), true);
$this->assertSame('&lt;p&gt;Hello&lt;/p&gt;', $firstEvent['content']);
$this->assertSame('<p>Hello</p>', $firstEvent['content']);
}

#[Test]
public function streamActionEscapesSpecialCharactersInSseChunks(): void
public function streamActionPreservesSpecialCharactersInSseChunks(): void
{
$config = $this->createConfigurationMock();
$this->configRepositoryMock->method('findDefault')->willReturn($config);
Expand All @@ -837,8 +837,10 @@ public function streamActionEscapesSpecialCharactersInSseChunks(): void
$response->getBody()->rewind();
$body = $response->getBody()->getContents();

// Single quotes are HTML-escaped
$this->assertStringContainsString('It&#039;s a test', $body);
// Raw content preserved in JSON-encoded SSE data
$events = array_filter(explode("\n\n", $body), static fn (string $s): bool => $s !== '');
$firstEvent = json_decode(substr(reset($events), 6), true);
$this->assertSame("It's a test", $firstEvent['content']);
}

#[Test]
Expand Down Expand Up @@ -918,8 +920,8 @@ public function streamActionSseDoneEventIncludesModelName(): void
$events = array_filter(explode("\n\n", $body), static fn (string $s): bool => $s !== '');
$lastEvent = json_decode(substr(end($events), 6), true);
$this->assertTrue($lastEvent['done']);
// Model is HTML-escaped
$this->assertSame('test-model&#039;s', $lastEvent['model']);
// Model is returned raw — JSON encoding prevents XSS
$this->assertSame("test-model's", $lastEvent['model']);
}

#[Test]
Expand Down Expand Up @@ -1488,7 +1490,7 @@ public function getTasksActionFiltersInactiveTasks(): void
}

#[Test]
public function getTasksActionEscapesTaskFields(): void
public function getTasksActionReturnsRawTaskFields(): void
{
$task = $this->createTaskMock(1, 'fix<test>', 'Fix Grammar & Spelling', 'Desc with "quotes"', true);

Expand All @@ -1499,18 +1501,15 @@ public function getTasksActionEscapesTaskFields(): void
$response = $this->subject->getTasksAction($this->createMock(ServerRequestInterface::class));

$data = $this->decodeJsonResponse($response);
// HTML-escaped to prevent XSS
self::assertSame('fix&lt;test&gt;', $data['tasks'][0]['identifier']);
self::assertSame('Fix Grammar &amp; Spelling', $data['tasks'][0]['name']);
self::assertSame('Desc with &quot;quotes&quot;', $data['tasks'][0]['description']);
// Raw content — JSON encoding prevents XSS by design
self::assertSame('fix<test>', $data['tasks'][0]['identifier']);
self::assertSame('Fix Grammar & Spelling', $data['tasks'][0]['name']);
self::assertSame('Desc with "quotes"', $data['tasks'][0]['description']);
}

#[Test]
public function getTasksActionEscapesSingleQuotesInTaskFields(): void
public function getTasksActionPreservesSingleQuotesInTaskFields(): void
{
// Kills BitwiseOr mutants #6 and #7:
// ENT_QUOTES | ENT_SUBSTITUTE → ENT_QUOTES & ENT_SUBSTITUTE
// With ENT_QUOTES & ENT_SUBSTITUTE (= 0), single quotes are NOT escaped
$task = $this->createTaskMock(1, "it's", "Task's Name", "It's a description", true);

$this->taskRepositoryMock
Expand All @@ -1522,10 +1521,10 @@ public function getTasksActionEscapesSingleQuotesInTaskFields(): void
$data = $this->decodeJsonResponse($response);
self::assertTrue($data['success']);
self::assertCount(1, $data['tasks']);
// Single quotes MUST be escaped to &#039; — proves ENT_QUOTES is in effect
self::assertSame('it&#039;s', $data['tasks'][0]['identifier']);
self::assertSame('Task&#039;s Name', $data['tasks'][0]['name']);
self::assertSame('It&#039;s a description', $data['tasks'][0]['description']);
// Raw content — JSON encoding prevents XSS by design
self::assertSame("it's", $data['tasks'][0]['identifier']);
self::assertSame("Task's Name", $data['tasks'][0]['name']);
self::assertSame("It's a description", $data['tasks'][0]['description']);
}

#[Test]
Expand Down
Loading
Loading