chore: remove dead auth helper and fix stale default-password docs - #1026
Merged
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 26 files
Confidence score: 2/5
open_notebook/ai/models.pystill allows DNS-rebinding rechecks to be skipped on unlinked model provisioning paths (DB→env fallback and podcast profile resolution), so stale or attacker-influenced host resolution can slip through with security impact — apply the same recheck logic across all model resolution/provisioning branches before merging.- In
api/routers/sources.py, concurrent uploads can collide because filename allocation is check-then-create after the threadpool change, which can overwrite or mis-associate uploaded content under load — switch to atomic exclusive creation (O_EXCL) or add retry-on-collision inside_write...before merge. api/credentials_service.pynow rejects valid hosts containingmetadata(for examplemetadata.company.com), creating a user-facing regression where legitimate OpenAI-compatible endpoints fail validation — narrow thevalidate_url()rule so it blocks true metadata-service targets without denying normal hostnames.open_notebook/ai/connection_tester.pyandapi/routers/sources.pyrun synchronous DNS resolution from async request paths, so slow or attacker-controlled hostnames can block the FastAPI event loop and degrade unrelated traffic — move URL validation/DNS work toasyncio.to_thread(or equivalent) and keep request handlers non-blocking.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="api/credentials_service.py">
<violation number="1" location="api/credentials_service.py:21">
P2: Credential URL validation now rejects otherwise valid hostnames containing `metadata` (for example internal OpenAI-compatible endpoints like `metadata.company.com`). The imported shared `validate_url()` treats the word `metadata` in the `ipaddress` parse error as a security rejection, so this should be tightened in the helper before routing credential validation through it.</violation>
</file>
<file name="open_notebook/ai/models.py">
<violation number="1" location="open_notebook/ai/models.py:159">
P1: DNS-rebinding rechecks are still bypassed for model provisioning paths that do not hit this new linked-credential branch: unlinked models using DB→env fallback and podcast profile model resolution can still use stale credential URLs. Consider centralizing the revalidation in the shared credential/key-provider resolver so every DB-sourced URL is checked immediately before use.</violation>
</file>
<file name="open_notebook/ai/connection_tester.py">
<violation number="1" location="open_notebook/ai/connection_tester.py:69">
P2: Testing a credential with a slow or attacker-controlled hostname can block the FastAPI event loop because validate_url() performs synchronous DNS resolution inside these async handlers. Consider offloading validate_url() with asyncio.to_thread() or using an async resolver so connection tests do not stall unrelated requests.</violation>
</file>
<file name="open_notebook/graphs/prompt.py">
<violation number="1" location="open_notebook/graphs/prompt.py:25">
P2: The new prompt workflow safety rule is easy to miss because the referenced security docs still imply sandboxing is the complete mitigation. Since this change intentionally treats caller-supplied prompts as data rather than template source, the security guidance should also say not to pass user-controlled prompt text to `Prompter(template_text=...)`.</violation>
</file>
<file name="api/routers/sources.py">
<violation number="1" location="api/routers/sources.py:124">
P2: Concurrent uploads with the same filename can collide after this threadpool change because filename selection is check-then-create, not atomic. Using exclusive file creation (O_EXCL) or retry-on-collision inside `_write_uploaded_file` would preserve unique-name guarantees under parallel requests.</violation>
<violation number="2" location="api/routers/sources.py:370">
P2: Link-source creation now performs blocking DNS resolution on the request event loop, so slow/timeout DNS can stall unrelated requests. Running URL validation in `asyncio.to_thread` keeps this endpoint non-blocking.</violation>
</file>
<file name="frontend/src/components/ui/markdown-editor.test.tsx">
<violation number="1" location="frontend/src/components/ui/markdown-editor.test.tsx:26">
P3: This test name is misleading and describes the opposite of what the test actually checks. The name "strips a raw <iframe> to a live, embeddable element" reads as though the iframe IS transformed into a live element, but the assertion confirms it is stripped entirely (`expect(container.querySelector('iframe')).toBeNull()`). The phrase "to a live, embeddable element" should be something like "entirely" or "so it is not rendered as a live element" — the current wording will confuse future readers scanning test output.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| credential = await model.get_credential_obj() | ||
| if credential: | ||
| config = credential.to_esperanto_config() | ||
| _revalidate_config_urls(config, model.provider) |
Contributor
There was a problem hiding this comment.
P1: DNS-rebinding rechecks are still bypassed for model provisioning paths that do not hit this new linked-credential branch: unlinked models using DB→env fallback and podcast profile model resolution can still use stale credential URLs. Consider centralizing the revalidation in the shared credential/key-provider resolver so every DB-sourced URL is checked immediately before use.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At open_notebook/ai/models.py, line 159:
<comment>DNS-rebinding rechecks are still bypassed for model provisioning paths that do not hit this new linked-credential branch: unlinked models using DB→env fallback and podcast profile model resolution can still use stale credential URLs. Consider centralizing the revalidation in the shared credential/key-provider resolver so every DB-sourced URL is checked immediately before use.</comment>
<file context>
@@ -123,6 +156,7 @@ async def get_model(self, model_id: str, **kwargs) -> Optional[ModelType]:
credential = await model.get_credential_obj()
if credential:
config = credential.to_esperanto_config()
+ _revalidate_config_urls(config, model.provider)
logger.debug(
f"Using credential '{credential.name}' for model {model.name}"
</file context>
| 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 |
Contributor
There was a problem hiding this comment.
P2: Credential URL validation now rejects otherwise valid hostnames containing metadata (for example internal OpenAI-compatible endpoints like metadata.company.com). The imported shared validate_url() treats the word metadata in the ipaddress parse error as a security rejection, so this should be tightened in the helper before routing credential validation through it.
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 21:
<comment>Credential URL validation now rejects otherwise valid hostnames containing `metadata` (for example internal OpenAI-compatible endpoints like `metadata.company.com`). The imported shared `validate_url()` treats the word `metadata` in the `ipaddress` parse error as a security rejection, so this should be tightened in the helper before routing credential validation through it.</comment>
<file context>
@@ -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
# =============================================================================
</file context>
| # 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") |
Contributor
There was a problem hiding this comment.
P2: Testing a credential with a slow or attacker-controlled hostname can block the FastAPI event loop because validate_url() performs synchronous DNS resolution inside these async handlers. Consider offloading validate_url() with asyncio.to_thread() or using an async resolver so connection tests do not stall unrelated requests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At open_notebook/ai/connection_tester.py, line 69:
<comment>Testing a credential with a slow or attacker-controlled hostname can block the FastAPI event loop because validate_url() performs synchronous DNS resolution inside these async handlers. Consider offloading validate_url() with asyncio.to_thread() or using an async resolver so connection tests do not stall unrelated requests.</comment>
<file context>
@@ -61,6 +63,10 @@ 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")
async with httpx.AsyncClient(timeout=10.0) as client:
response = await client.get(
</file context>
| # state["prompt"] is caller-supplied free text. Never compile it as Jinja | ||
| # template *source* (Prompter(template_text=...)) - pass it as a plain | ||
| # render variable into a fixed, developer-authored template instead. | ||
| # See docs/7-DEVELOPMENT/security.md (GHSA-f35w-wx37-26q7). |
Contributor
There was a problem hiding this comment.
P2: The new prompt workflow safety rule is easy to miss because the referenced security docs still imply sandboxing is the complete mitigation. Since this change intentionally treats caller-supplied prompts as data rather than template source, the security guidance should also say not to pass user-controlled prompt text to Prompter(template_text=...).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At open_notebook/graphs/prompt.py, line 25:
<comment>The new prompt workflow safety rule is easy to miss because the referenced security docs still imply sandboxing is the complete mitigation. Since this change intentionally treats caller-supplied prompts as data rather than template source, the security guidance should also say not to pass user-controlled prompt text to `Prompter(template_text=...)`.</comment>
<file context>
@@ -19,8 +19,12 @@ class PatternChainState(TypedDict):
+ # state["prompt"] is caller-supplied free text. Never compile it as Jinja
+ # template *source* (Prompter(template_text=...)) - pass it as a plain
+ # render variable into a fixed, developer-authored template instead.
+ # See docs/7-DEVELOPMENT/security.md (GHSA-f35w-wx37-26q7).
system_prompt = Prompter(
- template_text=state["prompt"], parser=state.get("parser")
</file context>
| # 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") |
Contributor
There was a problem hiding this comment.
P2: Link-source creation now performs blocking DNS resolution on the request event loop, so slow/timeout DNS can stall unrelated requests. Running URL validation in asyncio.to_thread keeps this endpoint non-blocking.
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 creation now performs blocking DNS resolution on the request event loop, so slow/timeout DNS can stall unrelated requests. Running URL validation in `asyncio.to_thread` keeps this endpoint non-blocking.</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") |
| raise ValueError("No filename provided") | ||
|
|
||
| content = await upload_file.read() | ||
| return await asyncio.to_thread(_write_uploaded_file, upload_file.filename, content) |
Contributor
There was a problem hiding this comment.
P2: Concurrent uploads with the same filename can collide after this threadpool change because filename selection is check-then-create, not atomic. Using exclusive file creation (O_EXCL) or retry-on-collision inside _write_uploaded_file would preserve unique-name guarantees under parallel requests.
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 124:
<comment>Concurrent uploads with the same filename can collide after this threadpool change because filename selection is check-then-create, not atomic. Using exclusive file creation (O_EXCL) or retry-on-collision inside `_write_uploaded_file` would preserve unique-name guarantees under parallel requests.</comment>
<file context>
@@ -120,6 +115,15 @@ async def save_uploaded_file(upload_file: UploadFile) -> str:
+ raise ValueError("No filename provided")
+
+ content = await upload_file.read()
+ return await asyncio.to_thread(_write_uploaded_file, upload_file.filename, content)
+
+
</file context>
| } | ||
|
|
||
| describe('MarkdownEditor preview sanitization', () => { | ||
| it('strips a raw <iframe> to a live, embeddable element', () => { |
Contributor
There was a problem hiding this comment.
P3: This test name is misleading and describes the opposite of what the test actually checks. The name "strips a raw <iframe> to a live, embeddable element" reads as though the iframe IS transformed into a live element, but the assertion confirms it is stripped entirely (expect(container.querySelector('iframe')).toBeNull()). The phrase "to a live, embeddable element" should be something like "entirely" or "so it is not rendered as a live element" — the current wording will confuse future readers scanning test output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/ui/markdown-editor.test.tsx, line 26:
<comment>This test name is misleading and describes the opposite of what the test actually checks. The name "strips a raw <iframe> to a live, embeddable element" reads as though the iframe IS transformed into a live element, but the assertion confirms it is stripped entirely (`expect(container.querySelector('iframe')).toBeNull()`). The phrase "to a live, embeddable element" should be something like "entirely" or "so it is not rendered as a live element" — the current wording will confuse future readers scanning test output.</comment>
<file context>
@@ -0,0 +1,74 @@
+}
+
+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()
</file context>
Suggested change
| it('strips a raw <iframe> to a live, embeddable element', () => { | |
| it('does not render a raw <iframe> as a live element', () => { |
check_api_password() (an unused HTTPBearer-based dependency, superseded
by PasswordAuthMiddleware) and its now-unused imports are dead code -
nothing calls it. Removed.
Docs across api/CLAUDE.md, docs/3-USER-GUIDE/api-configuration.md,
docs/5-CONFIGURATION/security.md, docs/7-DEVELOPMENT/security.md, and
docs/SECURITY_REVIEW.md still described a hardcoded default password
("open-notebook-change-me") that PasswordAuthMiddleware doesn't actually
have - if OPEN_NOTEBOOK_PASSWORD is unset, auth is fully disabled
instead. Updated to match actual behavior.
Owner
|
Verified: check_api_password is gone with no remaining references anywhere, the leftover |
lfnovo
force-pushed
the
fix/auth-docs-cleanup
branch
from
July 10, 2026 18:47
4cf4799 to
66917ae
Compare
lfnovo
added a commit
that referenced
this pull request
Jul 11, 2026
Catch the Unreleased section up with everything merged since v1.10.0 that wasn't yet recorded: - Security: the July hardening batch (#1002-#1007, #1012-#1015, #1017, #1021, #1024, #1025) - Added: sources table sorting (#895), EasyPanel template + guide (#189), CI coverage measurement (#942) - Fixed: SSE streaming through the Next.js proxy (#770), insight/job batching and event-loop fixes (#1008, #1009, #1011, #1018), provider allowlist 422 (#1016), silent embed-queue failures (#1019), deprecated gemini model cleanup appended to the #970 entry - Changed: default-password docs correction (#1026)
This was referenced Sep 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
`check_api_password()` (an unused HTTPBearer-based dependency, superseded by `PasswordAuthMiddleware`) and its now-unused imports are dead code - nothing calls it. Removed.
Docs across `api/CLAUDE.md`, `docs/3-USER-GUIDE/api-configuration.md`, `docs/5-CONFIGURATION/security.md`, `docs/7-DEVELOPMENT/security.md`, and `docs/SECURITY_REVIEW.md` still described a hardcoded default password (`open-notebook-change-me`) that `PasswordAuthMiddleware` doesn't actually have - if `OPEN_NOTEBOOK_PASSWORD` is unset, auth is fully disabled instead. Updated to match actual behavior.
Docs-only + dead-code cleanup, not a new fix. Stacked on #1002-#1009 (unmerged). Full test suite passes (222/222) on this branch.