Repository navigation
fix!: address security and code review findings - #3
Merged
Merged
Conversation
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.
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.
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
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.
Summary
Fixes from a full-codebase security review and code review.
Security
x-bookstack-urlmust be onBOOKSTACK_ALLOWED_BASE_URLSand come withx-bookstack-token. The configured token is never sent to a host the caller picks, and the BookStack client no longer follows redirects.file_pathuploads needBOOKSTACK_UPLOAD_ROOTunder every transport, stdio included.mainand never overwrites a published tag.Correctness
section_matchedcontextis an exact slice that can be used asold_string. Formatting-only edits now verify, and a failed re-read after a write returnsverified: nullinstead of throwing.isErrorresults with their hints. Resource templates are listed viaresources/templates/list. GET/DELETE on/messageanswer 405.network_error/timeout_error. Keep-alive agents are shared.NODE_ENV/DEBUGconfig and dead code are removed, healthchecks honourSERVER_PORT, and the docs are corrected.Breaking changes
x-bookstack-urlrequiresBOOKSTACK_ALLOWED_BASE_URLSandx-bookstack-token.file_pathrequiresBOOKSTACK_UPLOAD_ROOT.isErrorresults instead of JSON-RPC errors.resources/templates/list.Testing
ci.ymlandimage.yml.