|
| 1 | +# Security Guidelines |
| 2 | + |
| 3 | +This document outlines security practices for Open Notebook development. It is informed by real vulnerabilities discovered through coordinated disclosure with [CERT-EU](https://cert.europa.eu) and should be treated as mandatory reading for all contributors. |
| 4 | + |
| 5 | +## Reporting Vulnerabilities |
| 6 | + |
| 7 | +If you discover a security vulnerability, **do not open a public GitHub issue**. Instead: |
| 8 | + |
| 9 | +1. Use [GitHub Security Advisories](https://github.com/lfnovo/open-notebook/security/advisories/new) to report privately |
| 10 | +2. Or email the maintainers directly |
| 11 | + |
| 12 | +We follow coordinated vulnerability disclosure and will work with you on a fix before any public announcement. |
| 13 | + |
| 14 | +--- |
| 15 | + |
| 16 | +## Database Queries (SurrealQL Injection) |
| 17 | + |
| 18 | +**Rule: Never interpolate user input into SurrealQL queries via f-strings.** |
| 19 | + |
| 20 | +SurrealQL injection is the equivalent of SQL injection. User-controlled values must be passed as parameterized bind variables using `$variable` syntax. |
| 21 | + |
| 22 | +### Parameterized queries (safe) |
| 23 | + |
| 24 | +```python |
| 25 | +# Good: parameterized query |
| 26 | +result = await repo_query( |
| 27 | + "SELECT * FROM source WHERE id = $id", |
| 28 | + {"id": ensure_record_id(source_id)} |
| 29 | +) |
| 30 | +``` |
| 31 | + |
| 32 | +### F-string interpolation (vulnerable) |
| 33 | + |
| 34 | +```python |
| 35 | +# Bad: user input in f-string |
| 36 | +result = await repo_query(f"SELECT * FROM source WHERE id = {source_id}") |
| 37 | +``` |
| 38 | + |
| 39 | +### ORDER BY and other clauses that can't be parameterized |
| 40 | + |
| 41 | +`ORDER BY`, `LIMIT`, and similar clauses typically cannot accept bind variables in SurrealDB. Use **allowlist validation** instead: |
| 42 | + |
| 43 | +```python |
| 44 | +# Good: validate against allowlist, then interpolate |
| 45 | +allowed_fields = {"name", "created", "updated"} |
| 46 | +allowed_directions = {"asc", "desc"} |
| 47 | + |
| 48 | +parts = order_by.strip().lower().split() |
| 49 | +if parts[0] not in allowed_fields: |
| 50 | + raise HTTPException(status_code=400, detail="Invalid sort field") |
| 51 | +if len(parts) > 1 and parts[1] not in allowed_directions: |
| 52 | + raise HTTPException(status_code=400, detail="Invalid sort direction") |
| 53 | + |
| 54 | +query = f"SELECT * FROM notebook ORDER BY {validated_order_by}" |
| 55 | +``` |
| 56 | + |
| 57 | +See `api/routers/sources.py` for the reference implementation of sort parameter validation. |
| 58 | + |
| 59 | +### Checklist |
| 60 | + |
| 61 | +- [ ] All user-provided values use `$variable` binding |
| 62 | +- [ ] Any f-string in a query only contains validated/hardcoded values |
| 63 | +- [ ] `ORDER BY`, `LIMIT`, etc. use allowlist validation |
| 64 | +- [ ] Database values used in subsequent queries are also parameterized (prevents second-order injection) |
| 65 | + |
| 66 | +--- |
| 67 | + |
| 68 | +## Template Rendering (Server-Side Template Injection) |
| 69 | + |
| 70 | +**Rule: Always use `SandboxedEnvironment` when rendering Jinja2 templates that contain user-provided content.** |
| 71 | + |
| 72 | +The [ai-prompter](https://github.com/lfnovo/ai-prompter) library (>= 0.4.0) uses `SandboxedEnvironment` by default, which blocks access to dangerous Python attributes like `__globals__`, `__subclasses__`, and `__init__`. |
| 73 | + |
| 74 | +### What SandboxedEnvironment prevents |
| 75 | + |
| 76 | +```jinja2 |
| 77 | +{# These are blocked and raise SecurityError #} |
| 78 | +{{ cycler.__init__.__globals__.os.popen('id').read() }} |
| 79 | +{{ ''.__class__.__mro__[1].__subclasses__() }} |
| 80 | +``` |
| 81 | + |
| 82 | +### Guidelines |
| 83 | + |
| 84 | +- Never downgrade ai-prompter below 0.4.0 |
| 85 | +- If using Jinja2 directly (outside ai-prompter), always use `jinja2.sandbox.SandboxedEnvironment` |
| 86 | +- Never pass user-provided strings to `jinja2.Environment` or `jinja2.Template` directly |
| 87 | + |
| 88 | +--- |
| 89 | + |
| 90 | +## File Handling (Path Traversal and Local File Inclusion) |
| 91 | + |
| 92 | +### File uploads |
| 93 | + |
| 94 | +**Rule: Always sanitize filenames and validate resolved paths.** |
| 95 | + |
| 96 | +```python |
| 97 | +import os |
| 98 | +from pathlib import Path |
| 99 | + |
| 100 | +# 1. Strip directory components |
| 101 | +safe_filename = os.path.basename(original_filename) |
| 102 | + |
| 103 | +# 2. Validate resolved path stays within target directory |
| 104 | +resolved = (Path(upload_folder) / safe_filename).resolve() |
| 105 | +if not str(resolved).startswith(str(Path(upload_folder).resolve()) + os.sep): |
| 106 | + raise ValueError("Path traversal detected") |
| 107 | +``` |
| 108 | + |
| 109 | +Key points: |
| 110 | + |
| 111 | +- Use `os.path.basename()` to strip directory components from user-provided filenames |
| 112 | +- Use `Path.resolve()` to resolve symlinks and `..` components |
| 113 | +- Use `startswith()` with a **trailing `os.sep`** to prevent sibling directory bypass (e.g., `/uploads_evil/` matching `/uploads`) |
| 114 | + |
| 115 | +### File path inputs |
| 116 | + |
| 117 | +**Rule: Validate that any user-provided file path is within the expected directory.** |
| 118 | + |
| 119 | +```python |
| 120 | +uploads_resolved = Path(UPLOADS_FOLDER).resolve() |
| 121 | +file_resolved = Path(user_provided_path).resolve() |
| 122 | +if not str(file_resolved).startswith(str(uploads_resolved) + os.sep): |
| 123 | + raise HTTPException(status_code=400, detail="Invalid file path") |
| 124 | +``` |
| 125 | + |
| 126 | +Never pass user-provided file paths directly to file reading or content extraction functions without validation. |
| 127 | + |
| 128 | +### Checklist |
| 129 | + |
| 130 | +- [ ] Filenames from uploads are sanitized with `os.path.basename()` |
| 131 | +- [ ] Resolved paths are validated with `startswith(directory + os.sep)` |
| 132 | +- [ ] User-provided `file_path` values are validated before use |
| 133 | +- [ ] No directory creation from user input (`mkdir` with traversal paths) |
| 134 | + |
| 135 | +--- |
| 136 | + |
| 137 | +## Authentication and CORS |
| 138 | + |
| 139 | +### Authentication |
| 140 | + |
| 141 | +Open Notebook currently uses simple password-based middleware (`PasswordAuthMiddleware`). This is suitable for single-user self-hosted deployments but should be hardened for production: |
| 142 | + |
| 143 | +- Change the default password (`OPEN_NOTEBOOK_PASSWORD`) |
| 144 | +- Change the default encryption key (`OPEN_NOTEBOOK_ENCRYPTION_KEY`) |
| 145 | +- Consider deploying behind a reverse proxy with proper authentication (OAuth, OIDC) |
| 146 | + |
| 147 | +### CORS |
| 148 | + |
| 149 | +The default CORS configuration allows all origins (`allow_origins=["*"]`). This is tracked for improvement in [#730](https://github.com/lfnovo/open-notebook/issues/730). For production deployments, restrict origins to only the frontend URL. |
| 150 | + |
| 151 | +--- |
| 152 | + |
| 153 | +## Secrets Management |
| 154 | + |
| 155 | +### Encryption key |
| 156 | + |
| 157 | +`OPEN_NOTEBOOK_ENCRYPTION_KEY` is used to encrypt API keys stored in SurrealDB. In production: |
| 158 | + |
| 159 | +- Set a strong, unique key (do not use the default) |
| 160 | +- Use Docker secrets via `OPEN_NOTEBOOK_ENCRYPTION_KEY_FILE` when possible |
| 161 | +- Never log or expose this value |
| 162 | + |
| 163 | +### Environment variables |
| 164 | + |
| 165 | +- Sensitive values (API keys, passwords, encryption keys) should never appear in logs |
| 166 | +- Use `loguru` with caution — avoid logging full request bodies or environment dumps |
| 167 | +- The Docker container runs as root by default; consider running as a non-root user |
| 168 | + |
| 169 | +--- |
| 170 | + |
| 171 | +## Code Review Security Checklist |
| 172 | + |
| 173 | +When reviewing PRs, check for: |
| 174 | + |
| 175 | +1. **Query injection**: Any f-string containing user input in a SurrealQL query |
| 176 | +2. **Template injection**: User-provided strings passed to Jinja2 without sandboxing |
| 177 | +3. **Path traversal**: User-provided filenames or paths used without sanitization |
| 178 | +4. **Information disclosure**: Error messages that expose internal paths, stack traces, or configuration |
| 179 | +5. **SSRF**: User-provided URLs passed to server-side HTTP requests without validation |
| 180 | +6. **Secrets in logs**: Sensitive values logged at any level |
| 181 | + |
| 182 | +--- |
| 183 | + |
| 184 | +## Past Vulnerabilities |
| 185 | + |
| 186 | +These vulnerabilities were reported by CERT-EU and are documented here as learning examples: |
| 187 | + |
| 188 | +| Version | Vulnerability | Severity | Advisory | |
| 189 | +|---------|--------------|----------|----------| |
| 190 | +| <= 1.8.2 | SurrealDB injection via `order_by` parameter | High (8.7) | [GHSA-5wj9-f8q5-8f9c](https://github.com/lfnovo/open-notebook/security/advisories/GHSA-5wj9-f8q5-8f9c) | |
| 191 | +| <= 1.8.3 | RCE via Jinja2 SSTI in transformations | Critical (9.2) | [GHSA-f35w-wx37-26q7](https://github.com/lfnovo/open-notebook/security/advisories/GHSA-f35w-wx37-26q7) | |
| 192 | +| <= 1.8.3 | Arbitrary file write via path traversal | High (7.0) | [GHSA-x4q2-89g5-594v](https://github.com/lfnovo/open-notebook/security/advisories/GHSA-x4q2-89g5-594v) | |
| 193 | +| <= 1.8.3 | Arbitrary file read via LFI | High (8.2) | [GHSA-842v-h4cj-r646](https://github.com/lfnovo/open-notebook/security/advisories/GHSA-842v-h4cj-r646) | |
0 commit comments