Skip to content

fix!: address security and code review findings - #3

Merged
gusocegodk merged 2 commits into
mainfrom
fix/review-findings
Oct 6, 2026
Merged

gusocegodk merged 2 commits into
mainfrom
fix/review-findings

Conversation

@gusocegodk

Copy link
Copy Markdown

Summary

Fixes from a full-codebase security review and code review.

Security

  • x-bookstack-url must be on BOOKSTACK_ALLOWED_BASE_URLS and come with x-bookstack-token. The configured token is never sent to a host the caller picks, and the BookStack client no longer follows redirects.
  • file_path uploads need BOOKSTACK_UPLOAD_ROOT under every transport, stdio included.
  • The page outline, HTML-to-text and anchor search now run in linear time, closing a ReDoS.
  • Errors returned to callers carry no stack traces or server paths.
  • axios is bumped to 1.20.0.
  • CI actions are pinned by SHA with read-only permissions.
  • The image workflow builds only commits on main and never overwrites a published tag.
  • The dev BookStack binds to loopback.

Correctness

  • Page sections:
    • skip fenced code
    • end at the next heading of the same or higher level
    • match exactly before falling back to a unique substring
    • report section_matched
  • grep context is an exact slice that can be used as old_string. Formatting-only edits now verify, and a failed re-read after a write returns verified: null instead of throwing.
  • Tool failures are returned as isError results with their hints. Resource templates are listed via resources/templates/list. GET/DELETE on /message answer 405.
  • An occupied port, or stdio without a token, exits 1 with a clear message.
  • Connection failures on safe methods are retried and reported as network_error/timeout_error. Keep-alive agents are shared.
  • Published schemas now match runtime validation. The NODE_ENV/DEBUG config and dead code are removed, healthchecks honour SERVER_PORT, and the docs are corrected.

Breaking changes

  • x-bookstack-url requires BOOKSTACK_ALLOWED_BASE_URLS and x-bookstack-token.
  • stdio file_path requires BOOKSTACK_UPLOAD_ROOT.
  • Tool errors are isError results instead of JSON-RPC errors.
  • Templated resources moved to resources/templates/list.

Testing

  • 472 pass, 0 fail, with about 90 new regression tests. Typecheck and lint are clean.
  • The image smoke suite passes 6/6, and actionlint is clean on ci.yml and image.yml.

Security:
- x-bookstack-url must be on BOOKSTACK_ALLOWED_BASE_URLS and come with
  x-bookstack-token; the configured token never goes to a caller's host,
  and the BookStack client no longer follows redirects.
- file_path uploads need BOOKSTACK_UPLOAD_ROOT under every transport.
- Linear-time page outline, html-to-text and anchor search (ReDoS).
- Caller-facing errors carry no stack traces or server paths.
- axios 1.20.0; CI pins actions by SHA with read-only permissions; the
  image workflow builds only commits on main and never overwrites a
  published tag; the dev BookStack binds to loopback.

Correctness:
- Page sections skip fenced code, end at the next same-or-higher heading,
  match exactly before a unique substring, and report section_matched.
- grep context is an exact slice usable as old_string; formatting-only
  edits verify; a failed re-read after a write returns verified: null.
- Tool failures are isError results with their hints; resource templates
  are listed via resources/templates/list; GET/DELETE /message answer 405;
  an occupied port and stdio without a token exit 1 with a clear message.
- Connection failures on safe methods are retried and reported as
  network_error/timeout_error; keep-alive agents are shared.
- Published schemas match runtime validation; NODE_ENV/DEBUG config and
  dead code removed; healthchecks honour SERVER_PORT; docs corrected.

BREAKING CHANGE: x-bookstack-url requires BOOKSTACK_ALLOWED_BASE_URLS and
x-bookstack-token; stdio file_path needs BOOKSTACK_UPLOAD_ROOT; tool errors
are isError results instead of JSON-RPC errors; templated resources move
to resources/templates/list.
@gusocegodk gusocegodk self-assigned this Oct 6, 2026
The stub answered unimplemented routes without reading the body. With the
shared keep-alive agent the next request reused that connection, and Bun
parsed the leftover bytes as a malformed request (an empty 400), which
made the upload contract tests flaky in CI.
@gusocegodk
gusocegodk merged commit 716c1de into main Oct 6, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant