Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions api/auth.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import secrets
from typing import Optional

from fastapi import Depends, HTTPException, Request
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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",
Expand Down
119 changes: 9 additions & 110 deletions api/credentials_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
# =============================================================================
Expand Down Expand Up @@ -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()
Expand All @@ -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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: OpenAI custom model discovery still has the DNS-rebinding gap this change closes for other user-configured URLs. The openai path later uses credential base_url for discovery_url without a request-time validate_url(), so revalidating that path before httpx.get would keep Discover Models consistent with ollama, openai_compatible, and azure.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/credentials_service.py, line 461:

<comment>OpenAI custom model discovery still has the DNS-rebinding gap this change closes for other user-configured URLs. The `openai` path later uses credential `base_url` for `discovery_url` without a request-time `validate_url()`, so revalidating that path before `httpx.get` would keep Discover Models consistent with `ollama`, `openai_compatible`, and `azure`.</comment>

<file context>
@@ -562,6 +457,8 @@ def models_endpoint(url: str) -> str:
             return []
         try:
+            # Re-validate at request time (see ollama branch above).
+            validate_url(base_url, "openai_compatible")
             headers = {}
             if api_key:
</file context>

headers = {}
if api_key:
headers["Authorization"] = f"Bearer {api_key}"
Expand All @@ -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:
Expand Down
22 changes: 20 additions & 2 deletions api/routers/chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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:
Expand Down
16 changes: 14 additions & 2 deletions api/routers/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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 = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: When the batch insight fetch fails, all source insights are silently dropped from the context response instead of falling back to per-source queries. The except block sets insights_by_source = {}, which causes insights=[] to be passed to source.get_context(). Since Source.get_context() checks insights is not None (and [] is not None is True), it uses that empty list directly and never calls self.get_insights() as a fallback. The result is a context response with zero insights, which is worse than the error the batching was meant to avoid. Consider setting insights_by_source = None and conditionally passing insights only when the batch succeeded.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/routers/context.py, line 88:

<comment>When the batch insight fetch fails, all source insights are silently dropped from the context response instead of falling back to per-source queries. The `except` block sets `insights_by_source = {}`, which causes `insights=[]` to be passed to `source.get_context()`. Since `Source.get_context()` checks `insights is not None` (and `[] is not None` is True), it uses that empty list directly and never calls `self.get_insights()` as a fallback. The result is a context response with zero insights, which is worse than the error the batching was meant to avoid. Consider setting `insights_by_source = None` and conditionally passing insights only when the batch succeeded.</comment>

<file context>
@@ -77,9 +77,21 @@ async def get_notebook_context(notebook_id: str, context_request: ContextRequest
+                # 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:
</file context>

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:
Expand Down
4 changes: 3 additions & 1 deletion api/routers/insights.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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:
Expand Down
30 changes: 20 additions & 10 deletions api/routers/sources.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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}")
Expand All @@ -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),
Expand Down Expand Up @@ -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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Link source ingestion still permits SSRF to loopback/private network targets because validate_url() is the credential-endpoint validator and intentionally allows localhost and RFC1918 addresses. A source-specific URL guard should reject loopback/private/internal ranges before the URL is queued or fetched.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/routers/sources.py, line 370:

<comment>Link source ingestion still permits SSRF to loopback/private network targets because `validate_url()` is the credential-endpoint validator and intentionally allows localhost and RFC1918 addresses. A source-specific URL guard should reject loopback/private/internal ranges before the URL is queued or fetched.</comment>

<file context>
@@ -360,6 +364,12 @@ 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")
+            except ValueError as e:
+                raise HTTPException(status_code=400, detail=str(e))
</file context>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Creating a link source can stall other API requests while DNS resolution runs because validate_url() calls blocking socket.getaddrinfo() inside the async route. Offloading this validation to the thread pool would match the surrounding non-blocking endpoint design.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/routers/sources.py, line 370:

<comment>Creating a link source can stall other API requests while DNS resolution runs because `validate_url()` calls blocking `socket.getaddrinfo()` inside the async route. Offloading this validation to the thread pool would match the surrounding non-blocking endpoint design.</comment>

<file context>
@@ -360,6 +364,12 @@ 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")
+            except ValueError as e:
+                raise HTTPException(status_code=400, detail=str(e))
</file context>
Suggested change
validate_url(source_data.url, "source")
await asyncio.to_thread(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)
Expand Down
2 changes: 1 addition & 1 deletion docs/3-USER-GUIDE/api-configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion docs/5-CONFIGURATION/security.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading