From 74080004e8156ad1f302f9c3b5ad809c6b3b8742 Mon Sep 17 00:00:00 2001
From: Fringg <egor.andrianovv@gmail.com>
Date: Mon, 23 Mar 2026 12:05:55 +0300
Subject: [PATCH] fix: media upload security hardening from 6-agent review

- uploadCount: always decrement in finally (prevent permanent uploading state)
- AbortSignal: pass to actual HTTP request (cancel network, not just UI)
- Concurrent uploads: use Set<AbortController> instead of single ref
- DOMPurify hooks: scope inside sanitizeHtml() to avoid global pollution
- Video src: restrict to HTTPS only (was allowing HTTP)
- GIF: remove from accept attributes (backend rejects GIF)
- Ref mutation: move handleMediaUploadRef update to useEffect
---
 src/api/news.ts               |   4 +-
 src/pages/AdminNewsCreate.tsx |  28 ++++---
 src/pages/NewsArticle.tsx     | 133 ++++++++++++++++++----------------
 3 files changed, 88 insertions(+), 77 deletions(-)

diff --git a/src/api/news.ts b/src/api/news.ts
index bcad9d3..996d5c0 100644
--- a/src/api/news.ts
+++ b/src/api/news.ts
@@ -58,13 +58,13 @@ export const newsApi = {
     return response.data;
   },
 
-  uploadMedia: async (file: File): Promise<NewsMediaUploadResponse> => {
+  uploadMedia: async (file: File, signal?: AbortSignal): Promise<NewsMediaUploadResponse> => {
     const formData = new FormData();
     formData.append('file', file);
     const response = await apiClient.post<NewsMediaUploadResponse>(
       '/cabinet/admin/news/media/upload',
       formData,
-      { headers: { 'Content-Type': 'multipart/form-data' } },
+      { headers: { 'Content-Type': 'multipart/form-data' }, signal },
     );
     return response.data;
   },
diff --git a/src/pages/AdminNewsCreate.tsx b/src/pages/AdminNewsCreate.tsx
index e1cafdc..c5578d1 100644
--- a/src/pages/AdminNewsCreate.tsx
+++ b/src/pages/AdminNewsCreate.tsx
@@ -215,7 +215,7 @@ export default function AdminNewsCreate() {
   const isUploading = uploadCount > 0;
   const [isDragging, setIsDragging] = useState(false);
   const [isFeaturedImageUploading, setIsFeaturedImageUploading] = useState(false);
-  const uploadAbortRef = useRef<AbortController | null>(null);
+  const activeUploadsRef = useRef(new Set<AbortController>());
 
   // Ref to hold the media upload handler — allows editorProps.handlePaste to
   // reference it without a circular dependency with useEditor.
@@ -301,14 +301,13 @@ export default function AdminNewsCreate() {
         return;
       }
 
-      uploadAbortRef.current?.abort();
       const controller = new AbortController();
-      uploadAbortRef.current = controller;
+      activeUploadsRef.current.add(controller);
 
       setUploadCount((c) => c + 1);
 
       try {
-        const result = await newsApi.uploadMedia(file);
+        const result = await newsApi.uploadMedia(file, controller.signal);
         if (controller.signal.aborted) return;
 
         if (!isSafeUrl(result.url)) {
@@ -335,19 +334,26 @@ export default function AdminNewsCreate() {
         haptic.error();
         setSaveError(t('news.admin.uploadError'));
       } finally {
-        if (!controller.signal.aborted) setUploadCount((c) => c - 1);
+        activeUploadsRef.current.delete(controller);
+        setUploadCount((c) => c - 1);
       }
     },
     [editor, haptic, t],
   );
 
   // Keep the ref in sync so editorProps handlers can access the latest callback
-  handleMediaUploadRef.current = handleMediaUpload;
-
-  // Cancel in-flight uploads on unmount to prevent state updates on destroyed editor
   useEffect(() => {
+    handleMediaUploadRef.current = handleMediaUpload;
+  }, [handleMediaUpload]);
+
+  // Cancel all in-flight uploads on unmount to prevent state updates on destroyed editor
+  useEffect(() => {
+    const uploads = activeUploadsRef.current;
     return () => {
-      uploadAbortRef.current?.abort();
+      for (const controller of uploads) {
+        controller.abort();
+      }
+      uploads.clear();
     };
   }, []);
 
@@ -689,7 +695,7 @@ export default function AdminNewsCreate() {
           <input
             ref={featuredImageInputRef}
             type="file"
-            accept="image/jpeg,image/png,image/webp,image/gif"
+            accept="image/jpeg,image/png,image/webp"
             onChange={handleFeaturedImageUpload}
             className="hidden"
             aria-hidden="true"
@@ -894,7 +900,7 @@ export default function AdminNewsCreate() {
           <input
             ref={mediaInputRef}
             type="file"
-            accept="image/jpeg,image/png,image/webp,image/gif,video/mp4,video/webm"
+            accept="image/jpeg,image/png,image/webp,video/mp4,video/webm"
             onChange={handleFileInputChange}
             className="hidden"
             aria-hidden="true"
diff --git a/src/pages/NewsArticle.tsx b/src/pages/NewsArticle.tsx
index 1098fc9..25d3e3b 100644
--- a/src/pages/NewsArticle.tsx
+++ b/src/pages/NewsArticle.tsx
@@ -78,51 +78,13 @@ function isAllowedIframeSrc(src: string): boolean {
   }
 }
 
-// Hook: strip iframes with disallowed src before DOMPurify finalizes the DOM.
-// Using 'afterSanitizeAttributes' is more reliable than 'uponSanitizeElement'
-// because the node is fully formed at this point.
-DOMPurify.addHook('afterSanitizeAttributes', (node) => {
-  if (node.tagName === 'IFRAME') {
-    const src = node.getAttribute('src') ?? '';
-    if (!isAllowedIframeSrc(src)) {
-      node.remove();
-      return;
-    }
-    // Force sandbox attribute on surviving iframes for defense-in-depth.
-    // allow-scripts + allow-same-origin are needed for YouTube/Vimeo embeds.
-    node.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-presentation');
-    // Strip any allow/allowfullscreen values we did not explicitly set
-    if (node.hasAttribute('allow')) {
-      // Only permit safe feature policies for video embeds
-      node.setAttribute('allow', 'autoplay; encrypted-media; picture-in-picture');
-    }
-  }
-});
-
-// Hook: validate <video> src — only allow http/https URLs
-DOMPurify.addHook('afterSanitizeAttributes', (node) => {
-  if (node.tagName === 'VIDEO') {
-    const src = node.getAttribute('src') ?? '';
-    try {
-      const url = new URL(src);
-      if (url.protocol !== 'https:' && url.protocol !== 'http:') {
-        node.remove();
-        return;
-      }
-    } catch {
-      node.remove();
-      return;
-    }
-    // Force safe defaults
-    node.setAttribute('controls', '');
-    node.setAttribute('preload', 'metadata');
-  }
-});
-
 /**
  * Strict allowlist of tags and attributes for article content.
  * Using ALLOWED_TAGS / ALLOWED_ATTR instead of ADD_TAGS / ADD_ATTR
  * ensures nothing from the permissive defaults leaks through.
+ *
+ * IMPORTANT: <source> is intentionally NOT in ALLOWED_TAGS — this ensures
+ * <video> elements can only load from their own src attribute (validated by hook).
  */
 const SANITIZE_CONFIG = {
   ALLOWED_TAGS: [
@@ -201,32 +163,75 @@ const SANITIZE_CONFIG = {
   ADD_ATTR: ['target'],
 };
 
-// Hook: force rel="noopener noreferrer" on all <a> tags to prevent tabnabbing
-DOMPurify.addHook('afterSanitizeAttributes', (node) => {
-  if (node.tagName === 'A') {
-    node.setAttribute('target', '_blank');
-    node.setAttribute('rel', 'noopener noreferrer');
-  }
-});
-
 /**
- * Restrict inline styles to only text-align (used by TipTap).
- * Strip any other CSS property to prevent CSS injection / exfiltration.
+ * Sanitize HTML with scoped DOMPurify hooks.
+ *
+ * Hooks are registered/removed inside this function to avoid polluting the
+ * global DOMPurify instance (other pages use DOMPurify with their own config).
  */
-DOMPurify.addHook('afterSanitizeAttributes', (node) => {
-  if (node.hasAttribute('style')) {
-    const style = node.getAttribute('style') ?? '';
-    const match = style.match(/text-align\s*:\s*(left|center|right|justify)/i);
-    if (match) {
-      node.setAttribute('style', `text-align: ${match[1]}`);
-    } else {
-      node.removeAttribute('style');
-    }
-  }
-});
-
 function sanitizeHtml(html: string): string {
-  return DOMPurify.sanitize(html, SANITIZE_CONFIG);
+  DOMPurify.removeAllHooks();
+
+  // Hook: strip iframes with disallowed src
+  DOMPurify.addHook('afterSanitizeAttributes', (node) => {
+    if (node.tagName === 'IFRAME') {
+      const src = node.getAttribute('src') ?? '';
+      if (!isAllowedIframeSrc(src)) {
+        node.remove();
+        return;
+      }
+      // Force sandbox — allow-scripts + allow-same-origin needed for YouTube/Vimeo
+      // (cross-origin, so sandbox escape via frameElement is not possible)
+      node.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-presentation');
+      if (node.hasAttribute('allow')) {
+        node.setAttribute('allow', 'autoplay; encrypted-media; picture-in-picture');
+      }
+    }
+  });
+
+  // Hook: validate <video> src — HTTPS only
+  DOMPurify.addHook('afterSanitizeAttributes', (node) => {
+    if (node.tagName === 'VIDEO') {
+      const src = node.getAttribute('src') ?? '';
+      try {
+        const url = new URL(src);
+        if (url.protocol !== 'https:') {
+          node.remove();
+          return;
+        }
+      } catch {
+        node.remove();
+        return;
+      }
+      node.setAttribute('controls', '');
+      node.setAttribute('preload', 'metadata');
+    }
+  });
+
+  // Hook: force safe link attributes
+  DOMPurify.addHook('afterSanitizeAttributes', (node) => {
+    if (node.tagName === 'A') {
+      node.setAttribute('target', '_blank');
+      node.setAttribute('rel', 'noopener noreferrer');
+    }
+  });
+
+  // Hook: restrict inline styles to text-align only (used by TipTap)
+  DOMPurify.addHook('afterSanitizeAttributes', (node) => {
+    if (node.hasAttribute('style')) {
+      const style = node.getAttribute('style') ?? '';
+      const match = style.match(/text-align\s*:\s*(left|center|right|justify)/i);
+      if (match) {
+        node.setAttribute('style', `text-align: ${match[1]}`);
+      } else {
+        node.removeAttribute('style');
+      }
+    }
+  });
+
+  const result = DOMPurify.sanitize(html, SANITIZE_CONFIG);
+  DOMPurify.removeAllHooks();
+  return result;
 }
 
 /**