diff --git a/api/auth.py b/api/auth.py index 481fce48dd..e0f70b1908 100644 --- a/api/auth.py +++ b/api/auth.py @@ -1,3 +1,4 @@ +import secrets from typing import Optional from fastapi import Depends, HTTPException, Request @@ -67,8 +68,10 @@ async def dispatch( headers={"WWW-Authenticate": "Bearer"}, ) - # Check password - if credentials != self.password: + # Check password (constant-time to avoid a timing side-channel) + if not secrets.compare_digest( + credentials.encode("utf-8"), self.password.encode("utf-8") + ): return JSONResponse( status_code=401, content={"detail": "Invalid password"}, @@ -108,8 +111,10 @@ def check_api_password( headers={"WWW-Authenticate": "Bearer"}, ) - # Check password - if credentials.credentials != password: + # Check password (constant-time to avoid a timing side-channel) + if not secrets.compare_digest( + credentials.credentials.encode("utf-8"), password.encode("utf-8") + ): raise HTTPException( status_code=401, detail="Invalid password", diff --git a/api/credentials_service.py b/api/credentials_service.py index 6661d63433..207e1f2ddd 100644 --- a/api/credentials_service.py +++ b/api/credentials_service.py @@ -7,11 +7,8 @@ All functions raise ValueError for business errors (router converts to HTTPException). """ -import ipaddress import os -import socket from typing import Dict, List, Optional -from urllib.parse import urlparse import httpx from loguru import logger @@ -21,6 +18,7 @@ from open_notebook.ai.model_discovery import classify_model_type from open_notebook.domain.credential import Credential from open_notebook.utils.encryption import get_secret_from_env +from open_notebook.utils.url_validation import validate_url # ============================================================================= # Constants @@ -84,113 +82,6 @@ } -# ============================================================================= -# URL Validation (SSRF protection) -# ============================================================================= - - -def validate_url(url: str, provider: str) -> None: - """ - Validate URL format for API endpoints. - - This is a self-hosted application, so we allow: - - Private IPs (10.x, 172.16-31.x, 192.168.x) for self-hosted services - - Localhost for local services (Ollama, LM Studio, etc.) - - We only block: - - Invalid schemes (must be http or https) - - Malformed URLs - - Link-local addresses (169.254.x.x) - used for cloud metadata endpoints - - Hostnames that resolve to link-local addresses - - Args: - url: The URL to validate - provider: The provider name (for logging/context) - - Raises: - ValueError: If the URL is invalid - """ - if not url or not url.strip(): - return # Empty URLs handled elsewhere - - try: - parsed = urlparse(url.strip()) - - # Validate scheme - only http/https allowed - if parsed.scheme not in ("http", "https"): - raise ValueError( - f"Invalid URL scheme: '{parsed.scheme}'. Only http and https are allowed." - ) - - # Extract hostname - hostname = parsed.hostname - if not hostname: - raise ValueError("Invalid URL: hostname could not be determined.") - - # Try to parse as IP address to check for dangerous addresses - try: - ip = ipaddress.ip_address(hostname) - - # Block link-local addresses (169.254.x.x) - used for cloud metadata - # These are dangerous as they can expose cloud instance credentials - if ip.is_link_local: - raise ValueError( - "Link-local addresses (169.254.x.x) are not allowed for security reasons. " - "These addresses are used for cloud metadata endpoints." - ) - - # Block IPv4-mapped IPv6 addresses pointing to link-local - # e.g. ::ffff:169.254.169.254 bypasses IPv6 is_link_local check - if hasattr(ip, "ipv4_mapped") and ip.ipv4_mapped and ip.ipv4_mapped.is_link_local: - raise ValueError( - "Link-local addresses (169.254.x.x) are not allowed for security reasons. " - "These addresses are used for cloud metadata endpoints." - ) - - except ValueError as ve: - # Re-raise our own ValueErrors - if "Link-local" in str(ve) or "Invalid URL" in str(ve): - raise - # Not an IP address, it's a hostname - need to resolve and check - try: - # Resolve hostname to IP address - resolved_ips = socket.getaddrinfo(hostname, None) - for family, _, _, _, sockaddr in resolved_ips: - ip_addr = sockaddr[0] - try: - parsed_ip = ipaddress.ip_address(ip_addr) - if parsed_ip.is_link_local: - raise ValueError( - f"Hostname '{hostname}' resolves to a link-local address (169.254.x.x) which is not allowed for security reasons. " - "These addresses are used for cloud metadata endpoints." - ) - # Block IPv4-mapped IPv6 addresses pointing to link-local - if ( - hasattr(parsed_ip, "ipv4_mapped") - and parsed_ip.ipv4_mapped - and parsed_ip.ipv4_mapped.is_link_local - ): - raise ValueError( - f"Hostname '{hostname}' resolves to a link-local address (169.254.x.x) which is not allowed for security reasons. " - "These addresses are used for cloud metadata endpoints." - ) - except ValueError as inner_ve: - if "link-local" in str(inner_ve).lower() or "Link-local" in str(inner_ve): - raise - # Skip non-IP addresses (e.g., IPv6 zones) - continue - except socket.gaierror: - # Could not resolve hostname - allow it since the URL may be - # valid in the deployment environment (e.g., Azure endpoints, - # internal DNS names). We only block link-local addresses. - pass - - except ValueError: - raise - except Exception: - raise ValueError("Invalid URL format. Check server logs for details.") - - # ============================================================================= # Helpers # ============================================================================= @@ -541,6 +432,10 @@ def models_endpoint(url: str) -> str: if provider == "ollama": ollama_url = base_url or "http://localhost:11434" try: + # Re-validate at request time: the base_url may have been saved + # against a hostname that only later resolved to an internal + # address (DNS rebinding). + validate_url(ollama_url, "ollama") async with httpx.AsyncClient() as client: response = await client.get(f"{ollama_url}/api/tags", timeout=10.0) response.raise_for_status() @@ -562,6 +457,8 @@ def models_endpoint(url: str) -> str: if not base_url: return [] try: + # Re-validate at request time (see ollama branch above). + validate_url(base_url, "openai_compatible") headers = {} if api_key: headers["Authorization"] = f"Bearer {api_key}" @@ -588,6 +485,8 @@ def models_endpoint(url: str) -> str: if not endpoint or not api_key: return [] try: + # Re-validate at request time (see ollama branch above). + validate_url(endpoint, "azure") url = f"{endpoint.rstrip('/')}/openai/models?api-version={api_version}" headers = {"api-key": api_key} async with httpx.AsyncClient() as client: diff --git a/api/routers/chat.py b/api/routers/chat.py index 103e0aa28f..459c8d6141 100644 --- a/api/routers/chat.py +++ b/api/routers/chat.py @@ -8,7 +8,13 @@ from pydantic import BaseModel, Field from open_notebook.database.repository import ensure_record_id, repo_query -from open_notebook.domain.notebook import ChatSession, Note, Notebook, Source +from open_notebook.domain.notebook import ( + ChatSession, + Note, + Notebook, + Source, + SourceInsight, +) from open_notebook.exceptions import ( NotFoundError, ) @@ -490,9 +496,21 @@ async def build_context(request: BuildContextRequest): else: # Default behavior - include all sources and notes with short context sources = await notebook.get_sources() + try: + insights_by_source = await SourceInsight.get_for_sources( + [source.id for source in sources if source.id] + ) + except Exception as e: + # Match the per-source fallback below: a hiccup fetching + # insights shouldn't fail the whole context request. + logger.warning(f"Error batch-fetching source insights: {str(e)}") + insights_by_source = {} for source in sources: try: - source_context = await source.get_context(context_size="short") + source_context = await source.get_context( + context_size="short", + insights=insights_by_source.get(source.id or "", []), + ) context_data["sources"].append(source_context) total_content += str(source_context) except Exception as e: diff --git a/api/routers/context.py b/api/routers/context.py index 92dd723b2b..ce4d74a10b 100644 --- a/api/routers/context.py +++ b/api/routers/context.py @@ -2,7 +2,7 @@ from loguru import logger from api.models import ContextRequest, ContextResponse -from open_notebook.domain.notebook import Note, Notebook, Source +from open_notebook.domain.notebook import Note, Notebook, Source, SourceInsight from open_notebook.exceptions import InvalidInputError from open_notebook.utils import token_count @@ -77,9 +77,21 @@ async def get_notebook_context(notebook_id: str, context_request: ContextRequest else: # Default behavior - include all sources and notes with short context sources = await notebook.get_sources() + try: + insights_by_source = await SourceInsight.get_for_sources( + [source.id for source in sources if source.id] + ) + except Exception as e: + # Match the per-source fallback below: a hiccup fetching + # insights shouldn't fail the whole context request. + logger.warning(f"Error batch-fetching source insights: {str(e)}") + insights_by_source = {} for source in sources: try: - source_context = await source.get_context(context_size="short") + source_context = await source.get_context( + context_size="short", + insights=insights_by_source.get(source.id or "", []), + ) context_data["source"].append(source_context) total_content += str(source_context) except Exception as e: diff --git a/api/routers/insights.py b/api/routers/insights.py index b9e2c7145f..1518a02b01 100644 --- a/api/routers/insights.py +++ b/api/routers/insights.py @@ -3,7 +3,7 @@ from api.models import NoteResponse, SaveAsNoteRequest, SourceInsightResponse from open_notebook.domain.notebook import SourceInsight -from open_notebook.exceptions import InvalidInputError +from open_notebook.exceptions import InvalidInputError, NotFoundError router = APIRouter() @@ -73,6 +73,8 @@ async def save_insight_as_note(insight_id: str, request: SaveAsNoteRequest): ) except HTTPException: raise + except NotFoundError: + raise HTTPException(status_code=404, detail="Notebook not found") except InvalidInputError as e: raise HTTPException(status_code=400, detail=str(e)) except Exception as e: diff --git a/api/routers/sources.py b/api/routers/sources.py index 8dd3f0ec37..1e47562912 100644 --- a/api/routers/sources.py +++ b/api/routers/sources.py @@ -17,6 +17,7 @@ from surreal_commands import execute_command_sync, submit_command from api.command_service import CommandService +from api.credentials_service import validate_url from api.models import ( AssetModel, CreateSourceInsightRequest, @@ -96,18 +97,12 @@ def generate_unique_filename(original_filename: str, upload_folder: str) -> str: counter += 1 -async def save_uploaded_file(upload_file: UploadFile) -> str: - """Save uploaded file to uploads folder and return file path.""" - if not upload_file.filename: - raise ValueError("No filename provided") - - # Generate unique filename - file_path = generate_unique_filename(upload_file.filename, UPLOADS_FOLDER) - +def _write_uploaded_file(filename: str, content: bytes) -> str: + """Sync filesystem work for save_uploaded_file() - run via asyncio.to_thread + so a large upload doesn't block the event loop for other requests.""" + file_path = generate_unique_filename(filename, UPLOADS_FOLDER) try: - # Save file with open(file_path, "wb") as f: - content = await upload_file.read() f.write(content) logger.info(f"Saved uploaded file to: {file_path}") @@ -120,6 +115,15 @@ async def save_uploaded_file(upload_file: UploadFile) -> str: raise +async def save_uploaded_file(upload_file: UploadFile) -> str: + """Save uploaded file to uploads folder and return file path.""" + if not upload_file.filename: + raise ValueError("No filename provided") + + content = await upload_file.read() + return await asyncio.to_thread(_write_uploaded_file, upload_file.filename, content) + + def parse_source_form_data( type: str = Form(...), notebook_id: Optional[str] = Form(None), @@ -360,6 +364,12 @@ async def create_source( raise HTTPException( status_code=400, detail="URL is required for link type" ) + # Block SSRF to internal/metadata addresses before the server ever + # fetches this URL (same guard used for provider-credential URLs). + try: + validate_url(source_data.url, "source") + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) content_state["url"] = source_data.url elif source_data.type == "upload": # Use uploaded file path or provided file_path (backward compatibility) diff --git a/docs/3-USER-GUIDE/api-configuration.md b/docs/3-USER-GUIDE/api-configuration.md index 940e0eff35..29cd018c80 100644 --- a/docs/3-USER-GUIDE/api-configuration.md +++ b/docs/3-USER-GUIDE/api-configuration.md @@ -31,7 +31,7 @@ environment: - OPEN_NOTEBOOK_ENCRYPTION_KEY=my-secret-passphrase ``` -Any string works as a key — it will be securely derived via SHA-256 internally. +Any string works as a key — it will be securely derived via salted PBKDF2-HMAC-SHA256 (600k iterations) internally. > **Warning**: If you change or lose the encryption key, **all stored credentials become unreadable**. Back up your encryption key securely and separately from your database backups. diff --git a/docs/5-CONFIGURATION/security.md b/docs/5-CONFIGURATION/security.md index f31796340f..1a826ba027 100644 --- a/docs/5-CONFIGURATION/security.md +++ b/docs/5-CONFIGURATION/security.md @@ -24,7 +24,7 @@ Set the encryption key to any secret string: OPEN_NOTEBOOK_ENCRYPTION_KEY=my-secret-passphrase ``` -Any string works — it will be securely derived via SHA-256 internally. Use a strong passphrase for production deployments. +Any string works — it will be securely derived via salted PBKDF2-HMAC-SHA256 (600k iterations) internally. Use a strong passphrase for production deployments. ### Default Credentials diff --git a/frontend/package-lock.json b/frontend/package-lock.json index c8d0074d1e..ecfbec1afc 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -48,6 +48,7 @@ "react-markdown": "^10.1.0", "react-syntax-highlighter": "^16.1.1", "rehype-katex": "^7.0.1", + "rehype-sanitize": "^6.0.0", "remark-gfm": "^4.0.1", "remark-math": "^6.0.0", "sonner": "^2.0.6", @@ -7483,6 +7484,21 @@ "url": "https://opencollective.com/unified" } }, + "node_modules/hast-util-sanitize": { + "version": "5.0.2", + "resolved": "https://registry.npmjs.org/hast-util-sanitize/-/hast-util-sanitize-5.0.2.tgz", + "integrity": "sha512-3yTWghByc50aGS7JlGhk61SPenfE/p1oaFeNwkOOyrscaOkMGrcW9+Cy/QAIOBpZxP1yqDIzFMR0+Np0i0+usg==", + "license": "MIT", + "dependencies": { + "@types/hast": "^3.0.0", + "@ungap/structured-clone": "^1.0.0", + "unist-util-position": "^5.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + }, "node_modules/hast-util-select": { "version": "6.0.4", "resolved": "https://registry.npmjs.org/hast-util-select/-/hast-util-select-6.0.4.tgz", @@ -10929,6 +10945,20 @@ "url": "https://jaywcjlove.github.io/#/sponsor" } }, + "node_modules/rehype-sanitize": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/rehype-sanitize/-/rehype-sanitize-6.0.0.tgz", + "integrity": "sha512-CsnhKNsyI8Tub6L4sm5ZFsme4puGfc6pYylvXo1AeqaGbjOYyzNv3qZPwvs0oMJ39eryyeOdmxwUIo94IpEhqg==", + "license": "MIT", + "dependencies": { + "@types/hast": "^3.0.0", + "hast-util-sanitize": "^5.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + }, "node_modules/rehype-slug": { "version": "6.0.0", "resolved": "https://registry.npmjs.org/rehype-slug/-/rehype-slug-6.0.0.tgz", diff --git a/frontend/package.json b/frontend/package.json index 87d7105b4e..4ebe8b0971 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -53,6 +53,7 @@ "react-markdown": "^10.1.0", "react-syntax-highlighter": "^16.1.1", "rehype-katex": "^7.0.1", + "rehype-sanitize": "^6.0.0", "remark-gfm": "^4.0.1", "remark-math": "^6.0.0", "sonner": "^2.0.6", diff --git a/frontend/src/components/ui/markdown-editor.test.tsx b/frontend/src/components/ui/markdown-editor.test.tsx new file mode 100644 index 0000000000..4dabeabb00 --- /dev/null +++ b/frontend/src/components/ui/markdown-editor.test.tsx @@ -0,0 +1,74 @@ +import { describe, it, expect } from 'vitest' +import { render } from '@testing-library/react' +import MarkdownPreview from '@uiw/react-markdown-preview' + +import { PREVIEW_OPTIONS } from './markdown-editor' + +// MarkdownEditor's live preview renders through @uiw/react-markdown-preview, +// which parses raw HTML in the markdown source into real elements (its `raw` +// default). Notes can hold AI-generated content that echoes an indirect +// prompt injection, so anything rendered here must not let that raw HTML +// become a live after') + expect(container.querySelector('iframe')).toBeNull() + expect(container.innerHTML).not.toContain('evil.example') + }) + + it('strips raw after') + expect(container.innerHTML).not.toContain('__pwned') + }) + + it('strips a raw after') + expect(container.querySelector('style')).toBeNull() + }) + + it('strips javascript: URLs from links', () => { + const { container } = renderPreview('[click me](javascript:alert(1))') + const link = container.querySelector('a') + expect(link?.getAttribute('href') ?? '').not.toContain('javascript:') + }) + + it('does not execute inline event-handler attributes on parsed raw elements', () => { + const { container } = renderPreview('') + expect(container.innerHTML).not.toContain('onerror') + }) + + it('still renders KaTeX math with its classes and MathML intact', () => { + const { container } = renderPreview('Inline math $x^2 + y^2 = z^2$') + expect(container.querySelector('.katex')).not.toBeNull() + expect(container.querySelector('.katex-mathml math')).not.toBeNull() + }) + + it('still syntax-highlights fenced code blocks', () => { + const { container } = renderPreview('```python\ndef hello():\n return 42\n```') + expect(container.querySelectorAll('span[class*="token"]').length).toBeGreaterThan(0) + }) + + it('still renders GFM tables, task lists, and safe links', () => { + const { container } = renderPreview( + '| a | b |\n|---|---|\n| 1 | 2 |\n\n- [x] done\n- [ ] todo\n\n[a link](https://example.com)' + ) + expect(container.querySelector('table')).not.toBeNull() + expect(container.querySelectorAll('input[type="checkbox"]').length).toBe(2) + expect(container.querySelector('a')?.getAttribute('href')).toBe('https://example.com') + }) +}) diff --git a/frontend/src/components/ui/markdown-editor.tsx b/frontend/src/components/ui/markdown-editor.tsx index 4d2582837a..0836036dca 100644 --- a/frontend/src/components/ui/markdown-editor.tsx +++ b/frontend/src/components/ui/markdown-editor.tsx @@ -4,6 +4,7 @@ import dynamic from 'next/dynamic' import { forwardRef } from 'react' import remarkMath from 'remark-math' import rehypeKatex from 'rehype-katex' +import rehypeSanitize from 'rehype-sanitize' const MDEditor = dynamic( () => import('@uiw/react-md-editor').then((mod) => mod.default), @@ -14,9 +15,18 @@ const MDEditor = dynamic( // concatenates these with its defaults (gfm, prism, raw), so syntax // highlighting and GFM are preserved. KaTeX CSS is loaded globally in // app/layout.tsx. -const PREVIEW_OPTIONS = { +// +// The library's own `raw` default lets literal HTML in the markdown source +// (e.g. pasted content, or an AI-generated note echoing an indirect prompt +// injection) render as live elements - notably a real