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
This commit is contained in:
Fringg
2026-03-23 12:05:55 +03:00
parent 723591e5c3
commit 74080004e8
3 changed files with 88 additions and 77 deletions

View File

@@ -58,13 +58,13 @@ export const newsApi = {
return response.data; return response.data;
}, },
uploadMedia: async (file: File): Promise<NewsMediaUploadResponse> => { uploadMedia: async (file: File, signal?: AbortSignal): Promise<NewsMediaUploadResponse> => {
const formData = new FormData(); const formData = new FormData();
formData.append('file', file); formData.append('file', file);
const response = await apiClient.post<NewsMediaUploadResponse>( const response = await apiClient.post<NewsMediaUploadResponse>(
'/cabinet/admin/news/media/upload', '/cabinet/admin/news/media/upload',
formData, formData,
{ headers: { 'Content-Type': 'multipart/form-data' } }, { headers: { 'Content-Type': 'multipart/form-data' }, signal },
); );
return response.data; return response.data;
}, },

View File

@@ -215,7 +215,7 @@ export default function AdminNewsCreate() {
const isUploading = uploadCount > 0; const isUploading = uploadCount > 0;
const [isDragging, setIsDragging] = useState(false); const [isDragging, setIsDragging] = useState(false);
const [isFeaturedImageUploading, setIsFeaturedImageUploading] = 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 // Ref to hold the media upload handler — allows editorProps.handlePaste to
// reference it without a circular dependency with useEditor. // reference it without a circular dependency with useEditor.
@@ -301,14 +301,13 @@ export default function AdminNewsCreate() {
return; return;
} }
uploadAbortRef.current?.abort();
const controller = new AbortController(); const controller = new AbortController();
uploadAbortRef.current = controller; activeUploadsRef.current.add(controller);
setUploadCount((c) => c + 1); setUploadCount((c) => c + 1);
try { try {
const result = await newsApi.uploadMedia(file); const result = await newsApi.uploadMedia(file, controller.signal);
if (controller.signal.aborted) return; if (controller.signal.aborted) return;
if (!isSafeUrl(result.url)) { if (!isSafeUrl(result.url)) {
@@ -335,19 +334,26 @@ export default function AdminNewsCreate() {
haptic.error(); haptic.error();
setSaveError(t('news.admin.uploadError')); setSaveError(t('news.admin.uploadError'));
} finally { } finally {
if (!controller.signal.aborted) setUploadCount((c) => c - 1); activeUploadsRef.current.delete(controller);
setUploadCount((c) => c - 1);
} }
}, },
[editor, haptic, t], [editor, haptic, t],
); );
// Keep the ref in sync so editorProps handlers can access the latest callback // 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(() => { 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 () => { return () => {
uploadAbortRef.current?.abort(); for (const controller of uploads) {
controller.abort();
}
uploads.clear();
}; };
}, []); }, []);
@@ -689,7 +695,7 @@ export default function AdminNewsCreate() {
<input <input
ref={featuredImageInputRef} ref={featuredImageInputRef}
type="file" type="file"
accept="image/jpeg,image/png,image/webp,image/gif" accept="image/jpeg,image/png,image/webp"
onChange={handleFeaturedImageUpload} onChange={handleFeaturedImageUpload}
className="hidden" className="hidden"
aria-hidden="true" aria-hidden="true"
@@ -894,7 +900,7 @@ export default function AdminNewsCreate() {
<input <input
ref={mediaInputRef} ref={mediaInputRef}
type="file" 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} onChange={handleFileInputChange}
className="hidden" className="hidden"
aria-hidden="true" aria-hidden="true"

View File

@@ -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. * Strict allowlist of tags and attributes for article content.
* Using ALLOWED_TAGS / ALLOWED_ATTR instead of ADD_TAGS / ADD_ATTR * Using ALLOWED_TAGS / ALLOWED_ATTR instead of ADD_TAGS / ADD_ATTR
* ensures nothing from the permissive defaults leaks through. * 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 = { const SANITIZE_CONFIG = {
ALLOWED_TAGS: [ ALLOWED_TAGS: [
@@ -201,32 +163,75 @@ const SANITIZE_CONFIG = {
ADD_ATTR: ['target'], 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). * Sanitize HTML with scoped DOMPurify hooks.
* Strip any other CSS property to prevent CSS injection / exfiltration. *
* 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 { 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;
} }
/** /**