From 85b699ab9f0ba9012bf5add4fc89a1d5b77229e2 Mon Sep 17 00:00:00 2001 From: Nathan Panchout Date: Tue, 22 Apr 2025 10:59:17 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(frontend)=20enhance=20fil?= =?UTF-8?q?e=20download=20security?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added a safety check for URLs in the FileDownloadButton component. Now, before opening a URL, it verifies if the URL is safe using the isSafeUrl function. This prevents potentially unsafe URLs from being opened in a new tab --- CHANGELOG.md | 4 + .../BlockNoteToolBar/FileDownloadButton.tsx | 7 +- .../impress/src/utils/__tests__/url.test.tsx | 110 ++++++++++++++++++ src/frontend/apps/impress/src/utils/url.ts | 51 ++++++++ 4 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 src/frontend/apps/impress/src/utils/__tests__/url.test.tsx create mode 100644 src/frontend/apps/impress/src/utils/url.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 367271613..2d1aff7f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## [Unreleased] +## Fixed + +- 🔒(frontend) enhance file download security #889 + ## Added - 🚸(backend) make document search on title accent-insensitive #874 diff --git a/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteToolBar/FileDownloadButton.tsx b/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteToolBar/FileDownloadButton.tsx index 75dacd3d4..e0c318478 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteToolBar/FileDownloadButton.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteToolBar/FileDownloadButton.tsx @@ -15,6 +15,7 @@ import { useCallback, useMemo } from 'react'; import { RiDownload2Fill } from 'react-icons/ri'; import { downloadFile, exportResolveFileUrl } from '@/docs/doc-export'; +import { isSafeUrl } from '@/utils/url'; export const FileDownloadButton = ({ open, @@ -59,7 +60,11 @@ export const FileDownloadButton = ({ */ if (!url.includes(window.location.hostname) && !url.includes('base64')) { if (!editor.resolveFileUrl) { - window.open(url); + if (!isSafeUrl(url)) { + return; + } + + window.open(url, '_blank', 'noopener,noreferrer'); } else { void editor .resolveFileUrl(url) diff --git a/src/frontend/apps/impress/src/utils/__tests__/url.test.tsx b/src/frontend/apps/impress/src/utils/__tests__/url.test.tsx new file mode 100644 index 000000000..26e33fc7b --- /dev/null +++ b/src/frontend/apps/impress/src/utils/__tests__/url.test.tsx @@ -0,0 +1,110 @@ +import { isSafeUrl } from '@/utils/url'; + +describe('isSafeUrl', () => { + // XSS Attacks + const xssUrls = [ + "javascript:alert('xss')", + "data:text/html,", + "vbscript:msgbox('xss')", + "expression(alert('xss'))", + "https://example.com/\">", + "https://example.com/\">", + "javascript:/*--><svg/onload='+/\"/+/onmouseover=1/+/[*/[]/+alert(1)//'>", + ]; + + // Directory Traversal + const traversalUrls = [ + 'https://example.com/../../etc/passwd', + 'https://example.com/..%2F..%2Fetc%2Fpasswd', + 'https://example.com/..\\..\\Windows\\System32\\config\\SAM', + ]; + + // SQL Injection + const sqlInjectionUrls = [ + "https://example.com/' OR '1'='1", + 'https://example.com/; DROP TABLE users;', + "https://example.com/' OR 1=1 --", + ]; + + // Malicious Encodings + const encodingUrls = [ + "https://example.com/%3Cscript%3Ealert('xss')%3C/script%3E", + 'https://example.com/%00', + 'https://example.com/\\0', + 'https://example.com/file.php%00.jpg', + ]; + + // Unauthorized Protocols + const protocolUrls = [ + 'file:///etc/passwd', + 'ftp://attacker.com/malware.exe', + 'telnet://attacker.com', + ]; + + // Long URLs + const longUrls = ['https://example.com/' + 'a'.repeat(2001)]; + + // Safe URLs + const safeUrls = [ + 'https://example.com', + 'https://example.com/path/to/file', + 'https://example.com?param=value', + 'https://example.com#section', + ]; + + describe('should block XSS attacks', () => { + xssUrls.forEach((url) => { + it(`should block ${url}`, () => { + expect(isSafeUrl(url)).toBe(false); + }); + }); + }); + + describe('should block directory traversal', () => { + traversalUrls.forEach((url) => { + it(`should block ${url}`, () => { + expect(isSafeUrl(url)).toBe(false); + }); + }); + }); + + describe('should block SQL injection', () => { + sqlInjectionUrls.forEach((url) => { + it(`should block ${url}`, () => { + expect(isSafeUrl(url)).toBe(false); + }); + }); + }); + + describe('should block malicious encodings', () => { + encodingUrls.forEach((url) => { + it(`should block ${url}`, () => { + expect(isSafeUrl(url)).toBe(false); + }); + }); + }); + + describe('should block unauthorized protocols', () => { + protocolUrls.forEach((url) => { + it(`should block ${url}`, () => { + expect(isSafeUrl(url)).toBe(false); + }); + }); + }); + + describe('should block long URLs', () => { + longUrls.forEach((url) => { + it(`should block ${url}`, () => { + expect(isSafeUrl(url)).toBe(false); + }); + }); + }); + + describe('should allow safe URLs', () => { + safeUrls.forEach((url) => { + it(`should allow ${url}`, () => { + expect(isSafeUrl(url)).toBe(true); + }); + }); + }); +}); diff --git a/src/frontend/apps/impress/src/utils/url.ts b/src/frontend/apps/impress/src/utils/url.ts new file mode 100644 index 000000000..d880d72ca --- /dev/null +++ b/src/frontend/apps/impress/src/utils/url.ts @@ -0,0 +1,51 @@ +export function isSafeUrl(url: string): boolean { + try { + // Parse the URL with a base to support relative URLs + const parsed = new URL(url, window.location.origin); + + // List of allowed protocols + const allowedProtocols = ['http:', 'https:']; + + // Check protocol + if (!allowedProtocols.includes(parsed.protocol)) { + return false; + } + + // Check for dangerous characters in the pathname + const dangerousChars = ['<', '>', '"', "'", '(', ')', ';', '=', '{', '}']; + if (dangerousChars.some((char) => parsed.pathname.includes(char))) { + return false; + } + + // Check URL length (protection against buffer overflow attacks) + if (url.length > 2000) { + return false; + } + + // Check for malicious encodings + if (url.includes('%00') || url.includes('\\0')) { + return false; + } + + // Check for XSS injection attempts + const xssPatterns = [ + '<script', + 'javascript:', + 'data:', + 'vbscript:', + 'expression(', + ]; + if (xssPatterns.some((pattern) => url.toLowerCase().includes(pattern))) { + return false; + } + + // Check for directory traversal attempts + if (url.includes('..') || url.includes('../') || url.includes('..\\')) { + return false; + } + + return true; + } catch { + return false; + } +}