vrr/open-notebook
2
0
Fork 0
mirror of https://github.com/lfnovo/open-notebook.git synced 2026-10-19 09:13:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: make validate_url() async so DNS resolution doesn't block the event loop (#1011)

Browse source 
* fix: sanitize raw HTML in the note/transformation markdown preview

MarkdownEditor's live preview renders through @uiw/react-markdown-
preview, which parses literal HTML in the markdown source into real
elements (its `raw` default) - including a live <iframe>. Notes can
hold AI-generated content that echoes an indirect prompt injection
from an ingested document, so this was reachable without the user
writing any HTML themselves.

Add rehype-sanitize (default schema) ahead of rehype-katex in the
preview pipeline. Ordering matters: sanitizing after katex strips
katex's own generated markup (not in the default allowlist), while
sanitizing before it only touches the raw-HTML-derived tree and
leaves not-yet-rendered math nodes alone. Verified against the real
preview component that this strips <iframe>/<script>/<style>/
javascript: URLs while fully preserving math, syntax highlighting,
and GFM tables/task-lists.

* fix: make validate_url() async so DNS resolution doesn't block the event loop

socket.getaddrinfo() in validate_url()'s hostname-resolution branch ran
synchronously on every call - and this is called on the hot path of model
provisioning, potentially once per chat message/transformation. A slow or
hanging DNS lookup stalled every other concurrent request, not just the
one that triggered it.

Run the resolution via asyncio.to_thread() instead, and thread `await`
through every call site: credentials_service.py's discover functions,
routers/credentials.py's create/update, routers/sources.py's source-URL
ingestion, and connection_tester.py's three provider test functions.
This commit is contained in:
Pico 2026-07-10 09:43:36 -04:00 • committed by GitHub
parent 832b814e7b
commit 58ce1dd5d0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 200 additions and 65 deletions
Download patch file Download diff file Expand all files Collapse all files

2
api/CLAUDE.md
View file

@ -178,7 +178,7 @@ The Credential Management system enables users to configure AI provider credenti
**Security Features**:
- NEVER returns actual API key values (only metadata)
- URL validation (SSRF protection) on all URL fields via `_validate_url()`
- URL validation (SSRF protection) on all URL fields via `validate_url()` (`open_notebook/utils/url_validation.py`) - async, since hostname resolution runs `socket.getaddrinfo()` via `asyncio.to_thread` rather than blocking the event loop
- Allows private IPs and localhost for self-hosted services (Ollama, LM Studio)
- Requires `OPEN_NOTEBOOK_ENCRYPTION_KEY` to be set for storing credentials

6
api/credentials_service.py
View file

@ -435,7 +435,7 @@ async def discover_with_config(provider: str, config: dict) -> List[dict]:
# 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")
await 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()
@ -458,7 +458,7 @@ async def discover_with_config(provider: str, config: dict) -> List[dict]:
return []
try:
# Re-validate at request time (see ollama branch above).
validate_url(base_url, "openai_compatible")
await validate_url(base_url, "openai_compatible")
headers = {}
if api_key:
headers["Authorization"] = f"Bearer {api_key}"
@ -486,7 +486,7 @@ async def discover_with_config(provider: str, config: dict) -> List[dict]:
return []
try:
# Re-validate at request time (see ollama branch above).
validate_url(endpoint, "azure")
await 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:

4
api/routers/credentials.py
View file

@ -152,7 +152,7 @@ async def create_credential(request: CreateCredentialRequest):
]:
if url_field:
try:
validate_url(url_field, request.provider)
await validate_url(url_field, request.provider)
except ValueError as e:
raise _handle_value_error(e)
@ -209,7 +209,7 @@ async def update_credential(credential_id: str, request: UpdateCredentialRequest
]:
if url_field:
try:
validate_url(url_field, "update")
await validate_url(url_field, "update")
except ValueError as e:
raise _handle_value_error(e)

2
api/routers/sources.py
View file

@ -367,7 +367,7 @@ async def create_source(
# 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")
await validate_url(source_data.url, "source")
except ValueError as e:
raise HTTPException(status_code=400, detail=str(e))
content_state["url"] = source_data.url

30
frontend/package-lock.json generated
View file

@ -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",

1
frontend/package.json
View file

@ -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",

74
frontend/src/components/ui/markdown-editor.test.tsx Normal file
View file

@ -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 <iframe>/<script>/<style>/javascript: URL - while still
// preserving math, syntax highlighting, and GFM, which are real features.
// These tests exercise the actual PREVIEW_OPTIONS MarkdownEditor uses, via
// the same underlying preview component, rather than a hand-rolled pipeline.
function renderPreview(source: string) {
return render(
<MarkdownPreview
source={source}
remarkPlugins={PREVIEW_OPTIONS.remarkPlugins}
rehypePlugins={PREVIEW_OPTIONS.rehypePlugins}
/>
)
}
describe('MarkdownEditor preview sanitization', () => {
it('strips a raw <iframe> to a live, embeddable element', () => {
const { container } = renderPreview('before <iframe src="https://evil.example"></iframe> after')
expect(container.querySelector('iframe')).toBeNull()
expect(container.innerHTML).not.toContain('evil.example')
})
it('strips raw <script> content', () => {
const { container } = renderPreview('before <script>window.__pwned = true</script> after')
expect(container.innerHTML).not.toContain('__pwned')
})
it('strips a raw <style> element (CSS-based UI redress)', () => {
// The style tag itself must not survive as a live element - its text
// content becoming inert prose (rather than a stylesheet) is fine.
const { container } = renderPreview('before <style>body{display:none}</style> 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('<img src="x" onerror="window.__pwned = true">')
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')
})
})

14
frontend/src/components/ui/markdown-editor.tsx
View file

@ -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 <iframe>, not just
// inert text. rehypeSanitize (default schema) strips that down to safe
// HTML. It must run *before* rehypeKatex: katex's own generated markup
// (katex-html spans, MathML) isn't in the default sanitize schema and gets
// stripped if sanitize runs after it - order here is load-bearing, verified
// against the actual rendered output for math/code/GFM before changing it.
export const PREVIEW_OPTIONS = {
remarkPlugins: [remarkMath],
rehypePlugins: [rehypeKatex],
rehypePlugins: [rehypeSanitize, rehypeKatex],
}
export interface MarkdownEditorProps {

8
frontend/vitest.config.ts
View file

@ -4,9 +4,17 @@ import path from 'path'
export default defineConfig({
plugins: [react()],
// jsdom doesn't paint, so tests never need real CSS/style computation - and
// without this, importing a component that bundles its own .css (e.g.
// @uiw/react-markdown-preview) fails to resolve this project's Tailwind v4
// postcss.config.mjs (string-form plugin list) outside of Next.js's own
// build pipeline. `test.css: false` alone doesn't prevent Vite from trying
// to resolve the postcss config; this bypasses it outright.
css: { postcss: { plugins: [] } },
test: {
environment: 'jsdom',
globals: true,
css: false,
setupFiles: ['./src/test/setup.ts'],
alias: {
'@': path.resolve(__dirname, './src')

6
open_notebook/ai/connection_tester.py
View file

@ -66,7 +66,7 @@ async def _test_azure_connection(
# Re-validate at request time: the endpoint may have been saved
# against a hostname that only later resolved to an internal
# address (DNS rebinding), so a save-time check alone isn't enough.
validate_url(test_endpoint, "azure")
await validate_url(test_endpoint, "azure")
async with httpx.AsyncClient(timeout=10.0) as client:
response = await client.get(
f"{test_endpoint}/openai/models?api-version={test_api_version}",
@ -106,7 +106,7 @@ async def _test_ollama_connection(base_url: str) -> Tuple[bool, str]:
"""Test Ollama server connectivity."""
try:
# Re-validate at request time (see _test_azure_connection for why).
validate_url(base_url, "ollama")
await validate_url(base_url, "ollama")
async with httpx.AsyncClient(timeout=10.0) as client:
# Try /api/tags endpoint (standard Ollama)
response = await client.get(f"{base_url}/api/tags")
@ -145,7 +145,7 @@ async def _test_openai_compatible_connection(base_url: str, api_key: Optional[st
"""Test OpenAI-compatible server connectivity."""
try:
# Re-validate at request time (see _test_azure_connection for why).
validate_url(base_url, "openai_compatible")
await validate_url(base_url, "openai_compatible")
headers = {}
if api_key:
headers["Authorization"] = f"Bearer {api_key}"

6
open_notebook/ai/models.py
View file

@ -28,7 +28,7 @@ _URL_CONFIG_KEYS = (
)
def _revalidate_config_urls(config: dict, provider: str) -> None:
async def _revalidate_config_urls(config: dict, provider: str) -> None:
"""
Re-validate a credential's URL fields immediately before they're used for
a real request.
@ -44,7 +44,7 @@ def _revalidate_config_urls(config: dict, provider: str) -> None:
value = config.get(key)
if value:
try:
validate_url(value, provider)
await validate_url(value, provider)
except ValueError as e:
raise ConfigurationError(str(e)) from e
@ -156,7 +156,7 @@ class ModelManager:
credential = await model.get_credential_obj()
if credential:
config = credential.to_esperanto_config()
_revalidate_config_urls(config, model.provider)
await _revalidate_config_urls(config, model.provider)
logger.debug(
f"Using credential '{credential.name}' for model {model.name}"
)

11
open_notebook/utils/url_validation.py
View file

@ -8,6 +8,7 @@ provider-configured URL immediately before it is actually used - closing most
of the DNS-rebinding TOCTOU window left by validating only once, at save time.
"""
import asyncio
import ipaddress
import socket
from urllib.parse import urlparse
@ -18,7 +19,7 @@ from urllib.parse import urlparse
_AWS_IMDS_V6_ADDRESS = ipaddress.ip_address("fd00:ec2::254")
def validate_url(url: str, provider: str) -> None:
async def validate_url(url: str, provider: str) -> None:
"""
Validate URL format for API endpoints.
@ -68,8 +69,12 @@ def validate_url(url: str, provider: str) -> None:
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)
# Resolve hostname to IP address. This is a blocking call -
# run it off the event loop so a slow/hanging DNS lookup
# doesn't stall every other concurrent request (this is
# called on the hot path of model provisioning, potentially
# once per chat message/transformation).
resolved_ips = await asyncio.to_thread(socket.getaddrinfo, hostname, None)
for family, _, _, _, sockaddr in resolved_ips:
ip_addr = sockaddr[0]
try:

101
tests/test_url_validation.py
View file

@ -9,122 +9,129 @@ It only blocks:
Localhost and private IPs are ALLOWED because this is a self-hosted application
where users commonly run local services (Ollama, LM Studio, etc.).
validate_url() is async (the hostname-resolution branch runs
socket.getaddrinfo() via asyncio.to_thread so it doesn't block the event
loop - see open_notebook/utils/url_validation.py), so every test here is
async too.
"""
import pytest
from api.credentials_service import validate_url
pytestmark = pytest.mark.asyncio
class TestUrlValidation:
"""Test suite for URL validation to prevent SSRF attacks."""
def test_valid_https_url(self):
async def test_valid_https_url(self):
"""Valid HTTPS URLs should pass."""
validate_url("https://api.openai.com", "openai")
validate_url("https://example.com/api", "anthropic")
await validate_url("https://api.openai.com", "openai")
await validate_url("https://example.com/api", "anthropic")
# Should not raise
def test_valid_http_url(self):
async def test_valid_http_url(self):
"""Valid HTTP URLs should pass."""
validate_url("http://example.com", "openai")
await validate_url("http://example.com", "openai")
# Should not raise
def test_invalid_scheme(self):
async def test_invalid_scheme(self):
"""URLs with invalid schemes should be rejected."""
with pytest.raises(ValueError, match="Invalid URL scheme"):
validate_url("ftp://example.com", "openai")
await validate_url("ftp://example.com", "openai")
with pytest.raises(ValueError, match="Invalid URL scheme"):
validate_url("file:///etc/passwd", "openai")
await validate_url("file:///etc/passwd", "openai")
def test_localhost_allowed_for_self_hosted(self):
async def test_localhost_allowed_for_self_hosted(self):
"""Localhost should be allowed for self-hosted services."""
# This is a self-hosted app, localhost is valid for local services
validate_url("http://localhost:8000", "openai")
validate_url("http://127.0.0.1:8000", "azure")
await validate_url("http://localhost:8000", "openai")
await validate_url("http://127.0.0.1:8000", "azure")
# Should not raise
def test_localhost_allowed_for_ollama(self):
async def test_localhost_allowed_for_ollama(self):
"""Localhost should be allowed for Ollama provider."""
validate_url("http://localhost:11434", "ollama")
validate_url("http://127.0.0.1:11434", "ollama")
await validate_url("http://localhost:11434", "ollama")
await validate_url("http://127.0.0.1:11434", "ollama")
# Should not raise
def test_private_ip_allowed_for_self_hosted(self):
async def test_private_ip_allowed_for_self_hosted(self):
"""Private IP addresses should be allowed for self-hosted scenarios."""
# This is a self-hosted app, private IPs are valid for internal services
validate_url("http://10.0.0.1", "openai")
validate_url("http://172.16.0.1:8080", "anthropic")
validate_url("http://192.168.1.1", "azure")
await validate_url("http://10.0.0.1", "openai")
await validate_url("http://172.16.0.1:8080", "anthropic")
await validate_url("http://192.168.1.1", "azure")
# Should not raise
def test_private_ip_allowed_for_ollama(self):
async def test_private_ip_allowed_for_ollama(self):
"""Private IP addresses should be allowed for Ollama provider."""
validate_url("http://192.168.1.100:11434", "ollama")
validate_url("http://10.0.0.50:11434", "ollama")
await validate_url("http://192.168.1.100:11434", "ollama")
await validate_url("http://10.0.0.50:11434", "ollama")
# Should not raise
def test_loopback_allowed_for_self_hosted(self):
async def test_loopback_allowed_for_self_hosted(self):
"""Loopback addresses should be allowed for self-hosted scenarios."""
validate_url("http://127.0.0.2", "openai")
await validate_url("http://127.0.0.2", "openai")
# Should not raise
def test_link_local_rejection(self):
async def test_link_local_rejection(self):
"""Link-local addresses should be rejected (cloud metadata protection)."""
with pytest.raises(ValueError, match="Link-local addresses"):
validate_url("http://169.254.169.254", "openai")
await validate_url("http://169.254.169.254", "openai")
# Also reject for ollama - link-local is never valid
with pytest.raises(ValueError, match="Link-local addresses"):
validate_url("http://169.254.169.254", "ollama")
await validate_url("http://169.254.169.254", "ollama")
def test_ipv6_localhost_allowed(self):
async def test_ipv6_localhost_allowed(self):
"""IPv6 localhost should be allowed for self-hosted scenarios."""
validate_url("http://[::1]:8000", "openai")
await validate_url("http://[::1]:8000", "openai")
# Should not raise
def test_empty_url(self):
async def test_empty_url(self):
"""Empty URLs should not raise (handled elsewhere)."""
validate_url("", "openai")
await validate_url("", "openai")
# None is handled by the function's early return check
# Should not raise
def test_invalid_url_format(self):
async def test_invalid_url_format(self):
"""Malformed URLs should be rejected."""
with pytest.raises(ValueError):
validate_url("not-a-url", "openai")
await validate_url("not-a-url", "openai")
def test_public_hostnames_allowed(self):
async def test_public_hostnames_allowed(self):
"""Public hostnames should be allowed."""
validate_url("https://api.openai.com/v1", "openai")
validate_url("https://api.anthropic.com", "anthropic")
validate_url("https://generativelanguage.googleapis.com", "google")
validate_url("https://api.groq.com", "groq")
await validate_url("https://api.openai.com/v1", "openai")
await validate_url("https://api.anthropic.com", "anthropic")
await validate_url("https://generativelanguage.googleapis.com", "google")
await validate_url("https://api.groq.com", "groq")
# Should not raise
def test_azure_specific_urls(self):
async def test_azure_specific_urls(self):
"""Azure OpenAI endpoints should be validated."""
validate_url(
await validate_url(
"https://my-resource.openai.azure.com", "azure"
)
# Localhost is allowed for self-hosted
validate_url("http://localhost:8000", "azure")
await validate_url("http://localhost:8000", "azure")
# Should not raise
def test_openai_compatible_urls(self):
async def test_openai_compatible_urls(self):
"""OpenAI-compatible provider URLs should be validated."""
validate_url("https://api.together.xyz", "openai_compatible")
await validate_url("https://api.together.xyz", "openai_compatible")
# Private IPs are allowed for self-hosted
validate_url("http://192.168.1.1:8080", "openai_compatible")
await validate_url("http://192.168.1.1:8080", "openai_compatible")
# Should not raise
def test_ipv4_mapped_ipv6_link_local_rejected(self):
async def test_ipv4_mapped_ipv6_link_local_rejected(self):
"""IPv4-mapped IPv6 addresses pointing to link-local should be rejected."""
with pytest.raises(ValueError, match="Link-local addresses"):
validate_url("http://[::ffff:169.254.169.254]", "openai")
await validate_url("http://[::ffff:169.254.169.254]", "openai")
def test_ipv4_mapped_ipv6_private_allowed(self):
async def test_ipv4_mapped_ipv6_private_allowed(self):
"""IPv4-mapped IPv6 addresses pointing to private IPs should be allowed."""
validate_url("http://[::ffff:192.168.1.1]", "openai")
await validate_url("http://[::ffff:192.168.1.1]", "openai")
# Should not raise - private IPs allowed for self-hosted