Skip to content

security: fix CodeQL stack-trace-exposure and URL-sanitization findings - #192

Merged
rolandpg merged 1 commit into
masterfrom
security/codeql-stack-trace-fix
Jul 7, 2026
Merged

security: fix CodeQL stack-trace-exposure and URL-sanitization findings#192
rolandpg merged 1 commit into
masterfrom
security/codeql-stack-trace-fix

Conversation

@rolandpg

@rolandpg rolandpg commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Clears all 15 open CodeQL alerts (verified locally with CodeQL CLI 2.25.6, same version/default suite the repo uses — this session's GitHub App token doesn't have security_events read access to confirm against the live dashboard).

Related issue

Follow-up to manual triage of https://github.com/ThreatRecall/zettelforge/security/code-scanning

Changes

  • web/app.py — 11 py/stack-trace-exposure alerts: every 5xx handler returned raw str(e) to API callers, potentially leaking paths, SQL fragments, or config values. Added _error_response() / _log_and_get_error_id(): the real exception is still logged server-side via logger.exception, but the client now only sees a generic message plus an opaque error_id for correlating a support report with server logs. Swept all 12 sites with this pattern (including the per-item error in the bulk-ingest endpoint, which CodeQL's query didn't flag but has the identical shape).
  • src/zettelforge/config.pyWebConfig.host defaulted to "0.0.0.0" but is never actually read to bind a socket; the real bind address comes from web/app.py's --host CLI arg (already defaults to 127.0.0.1, gated behind an API key for wider binds) or an explicit --host in deployment tooling (e.g. the Docker image passes 0.0.0.0 directly). Flipped the dead default so the config editor UI doesn't advertise an all-interfaces bind that isn't actually wired up.
  • tests/test_basic.py — 4 py/incomplete-url-substring-sanitization false positives: CodeQL's heuristic flags domain/URL-shaped string literals used with in, even for exact list-membership assertions in test code. Rewrote as direct membership checks and a set-equality comparison — both are stronger assertions (they also catch unexpected extras) and no longer match the query's pattern.

Testing

  • Tests pass (pytest tests/ -v) — 701 passed (excluded: an unrelated network-blocked embedding-model download test, and test_osint_collectors.py's live-network tests, both environment issues in this sandbox unrelated to this change)
  • Linting passes (ruff check src/zettelforge/)
  • New tests added for new functionality — n/a, existing tests rewritten to fix false-positive alerts
  • No new external infrastructure dependencies without discussion

Verified via local CodeQL rebuild + rescan: 15 alerts → 0 (all three configured languages — Python, GitHub Actions, JavaScript/TypeScript — checked; the latter two had zero alerts before and after).

Note

While rescanning, CodeQL also flagged examples/athf_bridge.py:48 as unparseable (SyntaxError: '{' was never closed), which silently excludes that file from every scan, including the real GitHub Actions run. Pre-existing, unrelated to this change — flagging here rather than fixing, since it's out of scope for this PR.


Generated by Claude Code

web/app.py exposed raw exception text (`str(e)`) to API callers on every
5xx response across 12 handlers (py/stack-trace-exposure, 11 CodeQL
alerts) — paths, SQL fragments, and config values could leak through
error bodies. Add `_error_response()`/`_log_and_get_error_id()`: the
real exception is still logged server-side via `logger.exception`, but
callers now only see a generic message plus an opaque error_id for
correlating support reports with server logs.

WebConfig.host defaulted to "0.0.0.0" but is never actually read to
bind a socket — the real bind address comes from web/app.py's --host
CLI arg (already defaults to 127.0.0.1, gated behind an API key for
wider binds) or an explicit --host in deployment tooling. Flip the
dead default to match, so the config editor UI doesn't advertise an
all-interfaces bind that was never wired up.

tests/test_basic.py: CodeQL's py/incomplete-url-substring-sanitization
flagged 4 assertions using `any(lit in x for x in list)` /
`lit in list` on domain- and URL-shaped string literals — a false
positive since these are exact list-membership checks in test
assertions, not sanitization logic. Rewrote as direct membership and a
set-equality comparison, which are both stronger assertions (catch
missing/extra entries) and no longer match the query's heuristic.

Verified via local CodeQL CLI 2.25.6 (same version/query suite the
repo's default setup uses): 15 alerts -> 0. Full pytest suite passes
(701 passed; excluded failures are network/environment issues
unrelated to this change — embedding model download blocked, plus
test_osint_collectors.py's live network tests).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqYpmV8TABMzq27HBTLtNP
@rolandpg
rolandpg marked this pull request as ready for review July 7, 2026 20:21
@rolandpg
rolandpg merged commit 51c2044 into master Jul 7, 2026
16 of 17 checks passed
@rolandpg
rolandpg deleted the security/codeql-stack-trace-fix branch July 7, 2026 20:21

@devin-ai-integration devin-ai-integration Bot left a comment

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.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread src/zettelforge/config.py
# tooling (e.g. the Docker image's 0.0.0.0). Kept here for the config
# editor UI; default matches the safe CLI default rather than
# advertising an all-interfaces bind.
host: str = "127.0.0.1"

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.

🔍 Documentation still advertises 0.0.0.0 as the default for web.host

The reference documentation at docs/reference/configuration.md:386-394 still shows host: str = "0.0.0.0" in the code snippet and 0.0.0.0 in the defaults table, but the actual dataclass default was changed to 127.0.0.1 in this PR. The config editor UI tooltip descriptions at web/ui/js/views/configuration.js:77 and web/templates/config_editor.html:200 also describe this as "Bind host for the web interface" without noting it's cosmetic-only. This drift could confuse operators consulting the docs.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants