Merge pull request #94 from netresearch/fix/review-fixes

fix: remove redundant htmlspecialchars from JSON API responses

Author: CybotTM

Classes/Controller/AjaxController.php

@@ -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.
+ * Returns raw data in JSON responses — no server-side HTML escaping.
+ * The frontend sanitizes content via DOMParser before DOM insertion.
  */
 #[AsController]
 final readonly class AjaxController
@@ -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()]);
@@ -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);
@@ -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(),
             ];
         }
 

Classes/Domain/DTO/CompleteResponse.php

@@ -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).
+ * Returns raw data in JSON responses — no server-side HTML escaping.
+ * The frontend sanitizes content via DOMParser before DOM insertion.
  *
  * @internal
  */
@@ -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 without server-side HTML escaping.
+     * The frontend sanitizes content via DOMParser 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,

Resources/Public/JavaScript/Ckeditor/CowriterDialog.js

@@ -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) {
@@ -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.
      *

Tests/E2E/CowriterWorkflowTest.php

@@ -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(
@@ -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}");
     }
 
     /**
@@ -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>',
@@ -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']);
     }
 
     // =========================================================================
@@ -360,12 +356,12 @@ public function chatWorkflowEscapesXssInAllFields(): void
         ]);
         $result = $stack['controller']->chatAction($request);
 
-        // Assert: All fields escaped
+        // Assert: Raw content returned — frontend sanitizes before DOM insertion
         $data = json_decode((string) $result->getBody(), true, 512, JSON_THROW_ON_ERROR);
         self::assertIsArray($data);
-        self::assertStringNotContainsString('<', $data['content']);
-        self::assertStringNotContainsString('<', $data['model']);
-        self::assertStringNotContainsString('<', $data['finishReason']);
+        self::assertSame('<script>steal()</script>', $data['content']);
+        self::assertSame('<img onerror=alert(1)>', $data['model']);
+        self::assertSame('<svg onload=hack()>', $data['finishReason']);
     }
 
     // =========================================================================
@@ -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();
@@ -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');
@@ -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]
@@ -999,7 +993,7 @@ public function getTasksWorkflowReturnsActiveTasks(): void
     }
 
     #[Test]
-    public function getTasksWorkflowEscapesXss(): void
+    public function getTasksWorkflowReturnsRawContent(): void
     {
         $stack = $this->createCompleteStack([]);
 
@@ -1020,9 +1014,10 @@ public function getTasksWorkflowEscapesXss(): void
         self::assertIsArray($data);
         self::assertTrue($data['success']);
         self::assertCount(1, $data['tasks']);
-        self::assertStringNotContainsString('<script>', $data['tasks'][0]['identifier']);
-        self::assertStringNotContainsString('<img', $data['tasks'][0]['name']);
-        self::assertStringNotContainsString('<svg', $data['tasks'][0]['description']);
+        // Raw content — JSON encoding is the transport safety, frontend sanitizes
+        self::assertSame('<script>alert(1)</script>', $data['tasks'][0]['identifier']);
+        self::assertSame('<img onerror=hack>', $data['tasks'][0]['name']);
+        self::assertSame('<svg onload=steal()>', $data['tasks'][0]['description']);
     }
 
     #[Test]
@@ -1226,7 +1221,7 @@ public function executeTaskWorkflowWithAdHocRules(): void
     }
 
     #[Test]
-    public function executeTaskWorkflowEscapesXssFromLlm(): void
+    public function executeTaskWorkflowReturnsRawLlmContent(): void
     {
         $llmResponse = $this->createOpenAiResponse(
             content: '<script>document.cookie</script>Malicious response',
@@ -1249,11 +1244,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]

Tests/Integration/Controller/AjaxControllerIntegrationTest.php

@@ -168,9 +168,9 @@ public function completeFlowWithModelPrefix(): void
 
     #[Test]
     #[DataProvider('xssPayloadProvider')]
-    public function completeActionEscapesXssPayloadsInContent(string $payload, string $description): void
+    public function completeActionReturnsRawContentForFrontendSanitization(string $payload, string $description): void
     {
-        // Arrange: LLM returns malicious content
+        // Arrange: LLM returns potentially dangerous content
         $config = $this->createLlmConfiguration();
         $this->configRepoMock->method('findDefault')->willReturn($config);
 
@@ -181,14 +181,10 @@ public function completeActionEscapesXssPayloadsInContent(string $payload, strin
         $request = $this->createJsonRequest(['prompt' => 'Test prompt']);
         $result  = $this->subject->completeAction($request);
 
-        // Assert: XSS payload is HTML-escaped (angle brackets become entities)
+        // Assert: Raw content returned — JSON encoding is the transport safety,
+        // frontend sanitizes via DOMParser before DOM insertion
         $data = $this->assertSuccessfulJsonResponse($result);
-        // Check that < and > are escaped to &lt; and &gt;
-        self::assertStringNotContainsString('<', $data['content'], "Unescaped < in: {$description}");
-        self::assertStringNotContainsString('>', $data['content'], "Unescaped > in: {$description}");
-        // Verify the escaped versions are present
-        self::assertStringContainsString('&lt;', $data['content'], "Missing escaped < in: {$description}");
-        self::assertStringContainsString('&gt;', $data['content'], "Missing escaped > in: {$description}");
+        self::assertSame($payload, $data['content'], "Content should be raw for: {$description}");
     }
 
     /**
@@ -291,13 +287,13 @@ public function chatFlowWithMultipleMessages(): void
     }
 
     #[Test]
-    public function chatActionEscapesXssInAllFields(): void
+    public function chatActionReturnsRawContentInAllFields(): void
     {
         // Arrange: Setup configuration
         $config = $this->createLlmConfiguration();
         $this->configRepoMock->method('findDefault')->willReturn($config);
 
-        // Arrange: Response with XSS in all fields
+        // Arrange: Response with HTML in all fields
         $response = new CompletionResponse(
             content: '<script>alert(1)</script>',
             model: '<img onerror=alert(2)>',
@@ -313,12 +309,12 @@ public function chatActionEscapesXssInAllFields(): void
         ]);
         $result = $this->subject->chatAction($request);
 
-        // Assert: All fields are escaped (< and > become &lt; and &gt;)
+        // Assert: Raw content returned — frontend sanitizes before DOM insertion
         $data = json_decode((string) $result->getBody(), true, 512, JSON_THROW_ON_ERROR);
         self::assertIsArray($data);
-        self::assertStringNotContainsString('<', $data['content']);
-        self::assertStringNotContainsString('<', $data['model']);
-        self::assertStringNotContainsString('<', $data['finishReason']);
+        self::assertSame('<script>alert(1)</script>', $data['content']);
+        self::assertSame('<img onerror=alert(2)>', $data['model']);
+        self::assertSame('<svg onload=alert(3)>', $data['finishReason']);
     }
 
     // =========================================================================