From a210350596931a6471e6c8e93b63806e40967dff Mon Sep 17 00:00:00 2001 From: gusocegodk Date: Tue, 6 Oct 2026 13:17:20 +0200 Subject: [PATCH 1/2] fix!: address security and code review findings 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. --- .env.example | 20 +- .github/workflows/ci.yml | 11 +- .github/workflows/image.yml | 25 ++ Dockerfile | 2 +- README.md | 56 +-- bun.lock | 4 +- docker-compose.yml | 4 +- docs/api-reference.md | 65 ++-- docs/examples-and-workflows.md | 20 +- docs/releasing.md | 12 +- docs/resources-guide.md | 14 +- docs/setup-guide.md | 60 ++-- docs/tools-overview.md | 44 ++- package.json | 2 +- src/api/client.ts | 133 +++---- src/config/manager.ts | 57 ++- src/server.ts | 203 ++++++++--- src/tools/attachments.ts | 10 +- src/tools/images.ts | 10 +- src/tools/pages.ts | 94 +++-- src/tools/server-info.ts | 7 +- src/utils/errors.ts | 136 +++++-- src/utils/logger.ts | 12 - src/utils/page-content.ts | 262 +++++++++++--- src/validation/validator.ts | 36 +- tests/integration/attachments.test.ts | 52 +-- tests/integration/audit.test.ts | 1 - tests/integration/books.test.ts | 1 - tests/integration/chapters.test.ts | 1 - tests/integration/images.test.ts | 38 +- tests/integration/pages.test.ts | 1 - tests/integration/permissions.test.ts | 1 - tests/integration/recyclebin.test.ts | 1 - tests/integration/resources.test.ts | 13 +- tests/integration/roles.test.ts | 1 - tests/integration/search.test.ts | 1 - tests/integration/shelves.test.ts | 1 - tests/integration/system.test.ts | 3 +- tests/integration/users.test.ts | 1 - tests/transport/docker-smoke.test.ts | 2 +- tests/transport/http.test.ts | 91 ++++- tests/transport/oauth.test.ts | 13 + tests/transport/stdio.test.ts | 79 ++++- tests/transport/tools.test.ts | 222 +++++++++++- tests/transport/upstream-overrides.test.ts | 213 +++++++++++ tests/unit/books.test.ts | 16 +- tests/unit/bookstack-credential.test.ts | 1 - tests/unit/client-redirect.test.ts | 98 ++++++ tests/unit/client-transport.test.ts | 392 +++++++++++++++++++++ tests/unit/config-manager.test.ts | 44 +++ tests/unit/error-handler.test.ts | 118 +++++++ tests/unit/healthcheck.test.ts | 78 ++++ tests/unit/list-params.test.ts | 1 - tests/unit/page-content.test.ts | 267 +++++++++++++- tests/unit/pages.test.ts | 168 ++++++++- tests/unit/rate-limit.test.ts | 1 - tests/unit/retry.test.ts | 1 - tests/unit/server-info.test.ts | 42 +++ tests/unit/upload-guard.test.ts | 120 +++++++ 59 files changed, 2863 insertions(+), 519 deletions(-) create mode 100644 tests/transport/upstream-overrides.test.ts create mode 100644 tests/unit/client-redirect.test.ts create mode 100644 tests/unit/client-transport.test.ts create mode 100644 tests/unit/config-manager.test.ts create mode 100644 tests/unit/error-handler.test.ts create mode 100644 tests/unit/healthcheck.test.ts create mode 100644 tests/unit/server-info.test.ts create mode 100644 tests/unit/upload-guard.test.ts diff --git a/.env.example b/.env.example index 11a35c5..df9ded8 100644 --- a/.env.example +++ b/.env.example @@ -52,13 +52,19 @@ BOOKSTACK_BASE_URL=http://localhost:8080/api BOOKSTACK_API_TOKEN=your-api-token-here BOOKSTACK_TIMEOUT=30000 +# Per-request BookStack URL (HTTP transport, token mode) +# Comma-separated BookStack API base URLs a caller may name in the x-bookstack-url header. +# Unset refuses that header with a 400. An allowed URL also requires x-bookstack-token: the +# BOOKSTACK_API_TOKEN above is only ever sent to BOOKSTACK_BASE_URL. +# BOOKSTACK_ALLOWED_BASE_URLS=https://books.example.com/api,https://staging-books.example.com/api + # Uploads (optional) # The image/attachment tools accept a `file_path` to a file on the server instead of -# inlined base64. Under the stdio transport the client shares this process's trust -# domain, so any readable path is allowed. Under HTTP the caller may be remote, so -# `file_path` is refused outright unless you opt in by setting BOOKSTACK_UPLOAD_ROOT: -# the path is then resolved with realpath() and must land inside this directory, -# which blocks `../` traversal and symlink escapes. +# inlined base64. Under every transport, stdio included, `file_path` is refused unless you +# opt in by setting BOOKSTACK_UPLOAD_ROOT: whoever steers the tool arguments - a remote +# caller, or text planted in a page an agent reads - could otherwise upload any file this +# process can read. The path is resolved with realpath() and must land inside this +# directory, which blocks `../` traversal and symlink escapes. # BOOKSTACK_UPLOAD_ROOT=/var/lib/bookstack-mcp/uploads # Server Configuration @@ -145,7 +151,3 @@ VALIDATION_STRICT_MODE=true # tests/transport/stdio.test.ts, which assert them against the real output streams. LOG_LEVEL=info LOG_FORMAT=pretty - -# Development -NODE_ENV=development -DEBUG=false diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 63d8073..9edfa44 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,9 @@ on: branches: [main] pull_request: +permissions: + contents: read + env: # Single source of truth for the Bun this repo is validated on. The Dockerfile's # BUN_IMAGE pin must name this same version: the docker job's smoke suite compares @@ -17,8 +20,8 @@ jobs: name: Typecheck, test, lint runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 - - uses: oven-sh/setup-bun@v2 + - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + - uses: oven-sh/setup-bun@735343b667d3e6f658f44d0eca948eb6282f2b76 # v2.0.2 with: bun-version: ${{ env.BUN_VERSION }} - name: Install dependencies @@ -47,8 +50,8 @@ jobs: # Independent of `quality` on purpose: the two report in parallel, so a lint failure # and a broken image surface on the same run instead of one hiding the other. steps: - - uses: actions/checkout@v5 - - uses: oven-sh/setup-bun@v2 + - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + - uses: oven-sh/setup-bun@735343b667d3e6f658f44d0eca948eb6282f2b76 # v2.0.2 with: bun-version: ${{ env.BUN_VERSION }} - name: Build image diff --git a/.github/workflows/image.yml b/.github/workflows/image.yml index df8ee6d..d35d80a 100644 --- a/.github/workflows/image.yml +++ b/.github/workflows/image.yml @@ -28,7 +28,17 @@ jobs: contents: read packages: write steps: + # Full history, so the tagged commit can be checked against origin/main. - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + with: + fetch-depth: 0 + + - name: Require the tagged commit to be on main + run: | + if ! git merge-base --is-ancestor HEAD refs/remotes/origin/main; then + echo "::error::The tagged commit $(git rev-parse HEAD) is not on main; only merged commits are released." >&2 + exit 1 + fi - name: Read the version from the tag id: version @@ -83,6 +93,21 @@ jobs: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} + # A published tag is immutable: re-releasing a version takes a new -r. + - name: Refuse to overwrite a published image + env: + VERSION: ${{ steps.version.outputs.version }} + run: | + REF="${IMAGE,,}:${VERSION}" + if OUT="$(docker buildx imagetools inspect "$REF" 2>&1)"; then + echo "::error::${REF} is already published; tag a new -r instead." >&2 + exit 1 + elif ! grep -q ': not found' <<<"$OUT"; then + echo "::error::Could not check whether ${REF} is published; refusing to push blind." >&2 + echo "$OUT" >&2 + exit 1 + fi + # Rebuilds from the builder cache above, so the pushed layers are the ones just smoked. - name: Push image uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0 diff --git a/Dockerfile b/Dockerfile index 46bf933..190e5c6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -50,6 +50,6 @@ USER bun # GET /health returns 200 when healthy, 503 otherwise. The image ships `bun` # but NOT `node`, so the probe is implemented with `bun -e`. HEALTHCHECK --interval=30s --timeout=10s --start-period=15s --retries=3 \ - CMD bun -e "const r=await fetch('http://localhost:3000/health').catch(()=>null); process.exit(r&&r.status===200?0:1)" + CMD bun -e "const r=await fetch('http://localhost:'+(process.env.SERVER_PORT||3000)+'/health').catch(()=>null); process.exit(r&&r.status===200?0:1)" CMD ["bun", "run", "src/server.ts"] diff --git a/README.md b/README.md index 42d51ef..679adad 100644 --- a/README.md +++ b/README.md @@ -1,10 +1,10 @@ # BookStack MCP Server -Connect BookStack to Claude and other AI assistants through the Model Context Protocol (MCP). This server exposes 59 tools and 11 resources covering the supported subset of the BookStack API — books, pages, chapters, shelves, search, users, roles, permissions, attachments, images, the recycle bin, the audit log and system info. +Connect BookStack to Claude and other AI assistants through the Model Context Protocol (MCP). This server exposes 59 tools, 5 resources and 6 resource templates covering the supported subset of the BookStack API — books, pages, chapters, shelves, search, users, roles, permissions, attachments, images, the recycle bin, the audit log and system info. This server supports two transport modes: **Streamable HTTP** (default) and **Stdio**. -- **Streamable HTTP (default)**: A stateless HTTP transport. Authentication parameters can be overridden per-request using HTTP headers (`x-bookstack-url` and `x-bookstack-token`). +- **Streamable HTTP (default)**: A stateless HTTP transport. The BookStack token can be overridden per request with `x-bookstack-token`, and the BookStack URL with `x-bookstack-url` when the operator allows it (see [per-request overrides](#per-request-overrides)). - **Stdio Mode**: Standard input/output for local integration (e.g., with Claude Desktop). Set `MCP_TRANSPORT=stdio` to enable. > ⚠️ **Looking for the HTTP endpoint?** The MCP endpoint is `POST /message` — not `/`. @@ -13,7 +13,7 @@ This server supports two transport modes: **Streamable HTTP** (default) and **St ## ✨ What You Get - **BookStack Integration** - Access your books, pages, chapters, and content -- **59 MCP Tools & 11 Resources** - CRUD, search and export across the supported endpoint families +- **59 MCP Tools, 5 Resources & 6 Resource Templates** - CRUD, search and export across the supported endpoint families - **Search & Export** - Find content and export in multiple formats - **User Management** - Handle users, roles, and permissions - **Production Ready** - Rate limiting, validation, error handling, and logging @@ -98,7 +98,9 @@ export MCP_TRANSPORT="http" | `BOOKSTACK_BASE_URL` | `http://localhost:8080/api` | Full URL to the BookStack API. Must be a valid URL and include the `/api` suffix. | | `BOOKSTACK_API_TOKEN` | _(none — required, unless `MCP_AUTH_MODE=oauth`)_ | **Outbound** BookStack API token as `token_id:token_secret`. This is the credential the server spends on every tool call. Startup fails if unset. | | `BOOKSTACK_TIMEOUT` | `30000` | BookStack request timeout in milliseconds. | -| `SERVER_PORT` | `3000` | Port the HTTP transport listens on. Ignored in stdio mode. | +| `BOOKSTACK_ALLOWED_BASE_URLS` | _(none)_ | Comma-separated BookStack API base URLs a caller may name in the `x-bookstack-url` header, compared canonically. Unset refuses that header. See [per-request overrides](#per-request-overrides). | +| `BOOKSTACK_UPLOAD_ROOT` | _(none)_ | Directory the image/attachment tools' `file_path` may read from. Unset refuses `file_path` under every transport, stdio included; the path must resolve inside this directory. Must be unset with `MCP_AUTH_MODE=oauth`. | +| `SERVER_PORT` | `3000` | Port the HTTP transport listens on; if it is taken, startup fails with exit code `1`. Ignored in stdio mode. | | `HTTP_BODY_LIMIT` | `73400320` (70 MiB) | Maximum accepted `POST /message` body, in bytes. Sized for the largest inline base64 upload the image/attachment tools advertise (50,000 KB). Express's own default is ~100 KB, which would reject real uploads with a `413`. Lower it if untrusted callers can reach the port. | | `SERVER_NAME` | `bookstack-mcp-server` | Server name reported over MCP and by `GET /`. | | `SERVER_VERSION` | the package's own version | Version reported over MCP `initialize`, by `GET /`, and by `bookstack_server_info`. Defaults to `package.json#version`; leave it unset unless you deliberately want a different value. | @@ -108,8 +110,6 @@ export MCP_TRANSPORT="http" | `VALIDATION_STRICT_MODE` | `true` | Reject invalid tool params at the boundary. Set to `false` to log a warning and forward them to BookStack instead. | | `LOG_LEVEL` | `info` | One of `error`, `warn`, `info`, `debug`. | | `LOG_FORMAT` | `pretty` | One of `json`, `pretty`. | -| `NODE_ENV` | `development` | One of `development`, `production`, `test`. | -| `DEBUG` | `false` | Set to `true` for debug output. | > 💡 Need detailed setup? See the complete [Setup Guide](docs/setup-guide.md) @@ -128,14 +128,15 @@ The transport is chosen at startup from `MCP_TRANSPORT`: ### HTTP endpoints When running in HTTP mode the server exposes three endpoints, plus the OAuth metadata -document in [OAuth mode](#per-user-oauth). Any other path returns a JSON `404` listing -the valid ones. +document in [OAuth mode](#per-user-oauth). `GET` and `DELETE` on `/message` answer `405` +with `Allow: POST`, before authentication: the server offers no SSE stream and no sessions. +Any other path returns a JSON `404` listing the valid ones. | Method & path | Purpose | Status codes | | --- | --- | --- | | `GET /` | Server info JSON (name, version, `status: "running"`, endpoint list) | `200` | | `GET /health` | Health check — verifies live connectivity to BookStack | `200` healthy, `503` unhealthy | -| `POST /message` | **The MCP endpoint.** Send JSON-RPC MCP messages here | `200`, `401` without a valid bearer token, `500` on error | +| `POST /message` | **The MCP endpoint.** Send JSON-RPC MCP messages here | `200`, `400` on a malformed body or refused override, `401` without a valid bearer token, `500` on error | `GET /` and `GET /health` are unauthenticated. **`POST /message` requires an inbound `Authorization: Bearer ` header** — it dispatches every tool, including @@ -178,17 +179,19 @@ with the failing check named: "checks": [ { "name": "bookstack_connection", "healthy": false, "message": "BookStack API connection" }, { "name": "tools_loaded", "healthy": true, "message": "59 tools loaded" }, - { "name": "resources_loaded", "healthy": true, "message": "11 resources loaded" } + { "name": "resources_loaded", "healthy": true, "message": "5 resources and 6 resource templates loaded" } ] } ``` -> ⚠️ A **missing** `BOOKSTACK_API_TOKEN` behaves differently: config validation rejects -> an empty token at startup, so the process exits with -> `Configuration validation failed: bookstack.apiToken: BookStack API token is required` -> before Express ever listens. There is no `/health` to call — `curl` gets a connection -> refused, and under Docker the container restart-loops. A `503` therefore always means -> the token is **present but not working**; a dead port means it is **absent**. +> ⚠️ A **missing** `BOOKSTACK_API_TOKEN` behaves differently: the transport refuses to +> start. In HTTP token mode the process prints +> `Failed to start HTTP transport: Configuration validation failed: bookstack.apiToken: BookStack API token is required - set BOOKSTACK_API_TOKEN environment variable` +> to stderr and exits `1` before Express ever listens. There is no `/health` to call — +> `curl` gets a connection refused, and under Docker the container restart-loops. A `503` +> therefore always means the token is **present but not working**; a dead port means it +> is **absent**. Under stdio the same reason is printed as +> `Failed to start stdio transport: …`, stdout stays empty and the process exits `1`. **Call the MCP endpoint** — an `initialize` handshake. Both the `Content-Type` and `Accept` headers are required by the Streamable HTTP transport, and @@ -214,9 +217,14 @@ curl -X POST http://localhost:3000/message \ }' ``` -Per-request credential overrides are supported on `POST /message` via the -`x-bookstack-url` and `x-bookstack-token` headers; both fall back to the -`BOOKSTACK_BASE_URL` / `BOOKSTACK_API_TOKEN` environment variables. OAuth mode refuses them. +#### Per-request overrides + +`POST /message` accepts two optional headers in token mode. OAuth mode refuses both. + +- `x-bookstack-token` alone spends the caller's own BookStack token at `BOOKSTACK_BASE_URL`. +- `x-bookstack-url` is refused with a `400` unless it names one of + `BOOKSTACK_ALLOWED_BASE_URLS`, and then `x-bookstack-token` is required too. A + caller-chosen URL is never sent the configured `BOOKSTACK_API_TOKEN`. ### Per-user OAuth @@ -328,9 +336,9 @@ only the fragment it wants changed: | Tool | What it does | |---|---| | `bookstack_pages_outline` | Heading structure with offsets and section sizes. No content transferred. | -| `bookstack_pages_read` with `grep` | Matching excerpts from the **stored** source — paste one straight into `old_string`. | +| `bookstack_pages_read` with `grep` | Matches from the **stored** source, each with an exact `context` slice (no ellipses) — paste it straight into `old_string`. | | `bookstack_pages_edit` | Literal find-and-replace. `old_string` must match exactly and be unique unless `replace_all` is set. | -| `bookstack_pages_append` | Insert at the end of the page, the end of a named section, or right after a section heading. | +| `bookstack_pages_append` | Insert at the end of the page, the end of a named section (after its subsections), or right after a section heading. Reports the heading used as `section_matched`. | ```jsonc // 1. What sections exist, and how big are they? @@ -357,8 +365,10 @@ only the fragment it wants changed: error carries the first few matches with context. A missing anchor reports the same text found with different whitespace, which is the usual near-miss. A result smaller than half the original is refused unless `allow_shrink` is set. After a write the page is re-read and the -change is looked for in normalised text — BookStack rewrites stored HTML on save (heading -anchors, injected `id` attributes), so a byte comparison would call every success a failure. +change is looked for in the raw stored source, then in normalised text — BookStack rewrites +stored HTML on save (heading anchors, injected `id` attributes), so a byte comparison alone +would call every success a failure. If that re-read fails, the result is `verified: null`: +the write landed, so do not retry it. Every write creates a BookStack revision, so an applied edit can be rolled back in the UI. No response from these tools contains page content. `expected_updated_at` detects a page changed before this server reads it; BookStack's page diff --git a/bun.lock b/bun.lock index d89ba8b..d7ab4cf 100644 --- a/bun.lock +++ b/bun.lock @@ -6,7 +6,7 @@ "name": "bookstack-mcp-server", "dependencies": { "@modelcontextprotocol/sdk": "1.29.0", - "axios": "1.18.1", + "axios": "1.20.0", "dotenv": "17.4.2", "express": "5.2.1", "jose": "6.2.3", @@ -128,7 +128,7 @@ "asynckit": ["asynckit@0.4.0", "", {}, "sha512-Oei9OH4tRh0YqU3GxhX79dM/mwVgvbZJaSNaRk+bshkj0S5cfHcgYakreBjrHwatXKbz+IoIdYLxrKim2MjW0Q=="], - "axios": ["axios@1.18.1", "", { "dependencies": { "follow-redirects": "^1.16.0", "form-data": "^4.0.5", "https-proxy-agent": "^5.0.1", "proxy-from-env": "^2.1.0" } }, "sha512-3nTvFlvpn9Zu/RkHUqtc7/+al4UpRW5az71ap5zccp6e8RAYEzhMTecX8Dz1wWDYrPpUoB1HAQEGEAEvUr7S9g=="], + "axios": ["axios@1.20.0", "", { "dependencies": { "follow-redirects": "^1.16.0", "form-data": "^4.0.6", "https-proxy-agent": "^5.0.1", "proxy-from-env": "^2.1.0" } }, "sha512-r8aOh8j9cGKpgQAqpzrUHnSIc6a59Y3Xf/cv8sy1DrHCkZHzQGEuoq1tARk6qSyDdtQGSDgpb9kFlruzPvrgwg=="], "body-parser": ["body-parser@2.3.0", "", { "dependencies": { "bytes": "^3.1.2", "content-type": "^2.0.0", "debug": "^4.4.3", "http-errors": "^2.0.1", "iconv-lite": "^0.7.2", "on-finished": "^2.4.1", "qs": "^6.15.2", "raw-body": "^3.0.2", "type-is": "^2.1.0" } }, "sha512-2cGmJupaNgg+QUwVLAucDuWuoMZ6EX9iHDRswZ5lsNYEmwPaRknMPCLZz07yTzVq/83p4o/wzbDZbBrTvGGTIw=="], diff --git a/docker-compose.yml b/docker-compose.yml index 40043d8..fcb8466 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -88,7 +88,7 @@ services: volumes: - bookstack_config:/config ports: - - "6875:80" + - "127.0.0.1:6875:80" mcp: build: . @@ -126,7 +126,7 @@ services: - "127.0.0.1:3000:3000" healthcheck: # Same bun-based probe as the Dockerfile (the image has bun, not node). - test: ["CMD", "bun", "-e", "const r=await fetch('http://localhost:3000/health').catch(()=>null); process.exit(r&&r.status===200?0:1)"] + test: ["CMD", "bun", "-e", "const r=await fetch('http://localhost:'+(process.env.SERVER_PORT||3000)+'/health').catch(()=>null); process.exit(r&&r.status===200?0:1)"] interval: 30s timeout: 10s retries: 3 diff --git a/docs/api-reference.md b/docs/api-reference.md index b2a3888..41150e2 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -29,7 +29,7 @@ The BookStack MCP Server provides comprehensive access to the BookStack knowledg ### Key Features -- **API Coverage**: 59 tools and 11 resources across 13 categories, covering the supported subset of the BookStack API (comments, imports, tag listings, image-gallery `data` endpoints and ZIP export are not exposed) +- **API Coverage**: 59 tools, 5 resources and 6 resource templates across 13 categories, covering the supported subset of the BookStack API (comments, imports, tag listings, image-gallery `data` endpoints and ZIP export are not exposed) - **Type Safety**: Full TypeScript interfaces for all operations - **Robust Error Handling**: Comprehensive error mapping and recovery guidance - **Rate Limiting**: Token bucket algorithm with configurable limits @@ -77,10 +77,6 @@ VALIDATION_STRICT_MODE=true # default: true (reject invalid params at the boun # Optional - Logging LOG_LEVEL=info # error | warn | info | debug LOG_FORMAT=pretty # pretty | json - -# Optional - Development -NODE_ENV=development # development | production | test -DEBUG=false ``` > The HTTP transport applies no CORS headers and no Helmet hardening. Put it @@ -124,10 +120,6 @@ interface Config { level: 'error' | 'warn' | 'info' | 'debug'; // Severity threshold format: 'json' | 'pretty'; // Output shape }; - development: { - nodeEnv: 'development' | 'production' | 'test'; - debug: boolean; - }; } ``` @@ -158,17 +150,21 @@ Comprehensive error mapping from HTTP status codes to MCP errors: | 422 | `validation_error` | Validation failed | Check required fields and constraints | | 429 | `rate_limit_error` | Rate limit exceeded | Wait and retry, or reduce request frequency | | 500+ | `server_error` | Server-side error | Check BookStack server status | +| — | `network_error` | BookStack could not be reached (connection refused, reset or dropped before a response) | Check BookStack is up and `BOOKSTACK_BASE_URL` is right | +| — | `timeout_error` | BookStack did not respond within `BOOKSTACK_TIMEOUT` ms | Check BookStack load, or raise `BOOKSTACK_TIMEOUT` | ### Retry Policy Transient failures are retried automatically: - **Retryable Status Codes**: 429, 500, 502, 503, 504 +- **Retryable connection failures**: connection refused, reset or dropped before a + response (`network_error`). Timeouts (`timeout_error`) are never retried. - **Max Attempts**: 4 (the initial request plus up to 3 retries) - **Which requests are replayed**: a `429` is retried for **any** method — BookStack rejects a throttled request before it executes, so replaying it is safe. The - `5xx` codes are only retried for **`GET`, `HEAD` and `OPTIONS`**, because a - `5xx` on a write may have partially applied. + `5xx` codes and connection failures are only retried for **`GET`, `HEAD` and + `OPTIONS`**, because a write may have partially applied. - **Backoff**: a server-directed wait wins when present — `Retry-After`, else `X-RateLimit-Reset`. Otherwise exponential from 500ms, doubling to an 8s cap, with up to 25% jitter. @@ -194,7 +190,7 @@ interface BooksListParams { offset?: number; // Number to skip (default: 0) sort?: 'name' | 'created_at' | 'updated_at'; // Sort field filter?: { - name?: string; // Partial name match + name?: string; // Exact name match created_by?: number; // Creator user ID }; } @@ -300,7 +296,7 @@ interface PagesListParams extends PaginationParams { filter?: { book_id?: number; // Filter by parent book chapter_id?: number; // Filter by parent chapter - name?: string; // Partial name match + name?: string; // Exact name match draft?: boolean; // Filter by draft status template?: boolean; // Filter by template status }; @@ -342,6 +338,10 @@ interface ReadPageOptions { length?: number; // Length of that window metadata_only?: boolean; // Metadata and total_chars only, no content } + +// A grep response lists matches: [{ offset, match, context, context_truncated_start, +// context_truncated_end }]. `context` is an exact slice of the stored source (no ellipses), +// so it can be used verbatim as an edit's old_string. ``` #### Update Page @@ -380,14 +380,16 @@ interface EditPageParams { // edits: [{ index, occurrences_replaced, context }], // written, verified, unverified_fragment_count, chars_stored // } +// verified is null, with a note, when the write succeeded but the page could not be +// re-read: do not retry the write, read the page to check it. ``` -An anchor that cannot be applied comes back as `InvalidParams` with actionable -detail rather than a bare failure: `found_with_different_whitespace` when the text -exists but the whitespace differs, `first_occurrences` when it matched more than -once, `chars_before`/`chars_after` when the shrink guard fired. A page that changed -before this server read it comes back as `InvalidRequest` with -`type: 'concurrent_modification'`. +An anchor that cannot be applied comes back as a tool error result (`isError: true`) +whose text carries actionable detail rather than a bare failure: +`found_with_different_whitespace` when the text exists but the whitespace differs, +`first_occurrences` when it matched more than once, `chars_before`/`chars_after` when +the shrink guard fired. A page that changed before this server read it comes back the +same way with `type: 'concurrent_modification'`. #### Append to Page ```typescript @@ -403,7 +405,11 @@ interface AppendPageParams { } ``` -An unknown `section` fails with `available_sections` listing the real heading names. +An exact heading match (trimmed, case-insensitive) wins; otherwise a substring is accepted +only if exactly one heading contains it. An unknown `section` fails with `available_sections` +listing the real heading names, an ambiguous one with `matching_sections`. A section ends at +the next heading of the same or a higher level, so `end` lands after its subsections. The +response, dry runs included, reports the heading used as `section_matched`. #### Outline Page ```typescript @@ -412,7 +418,7 @@ An unknown `section` fails with `available_sections` listing the real heading na // { // page_id, name, slug, book_id, chapter_id, updated_at, revision_count, // editor, field, total_chars, heading_count, -// headings: [{ level, text, offset, length }] // length = section size +// headings: [{ level, text, offset, length }] // length = section size, subsections included // } ``` @@ -438,7 +444,7 @@ Chapters organize pages within books. interface ChaptersListParams extends PaginationParams { filter?: { book_id?: number; // Filter by parent book - name?: string; // Partial name match + name?: string; // Exact name match created_by?: number; // Creator user ID }; } @@ -493,7 +499,7 @@ Bookshelves organize books into collections. // Tool: bookstack_shelves_list interface ShelvesListParams extends PaginationParams { filter?: { - name?: string; // Partial name match + name?: string; // Exact name match created_by?: number; // Creator user ID }; } @@ -541,7 +547,7 @@ User management operations (requires admin permissions). // Tool: bookstack_users_list interface UsersListParams extends PaginationParams { filter?: { - name?: string; // Partial name match + name?: string; // Exact name match email?: string; // Partial email match active?: boolean; // Filter by active status }; @@ -658,7 +664,7 @@ File attachments for pages. // Tool: bookstack_attachments_list interface AttachmentsListParams extends PaginationParams { filter?: { - name?: string; // Partial name match + name?: string; // Exact name match uploaded_to?: number; // Page ID filter extension?: string; // File extension filter }; @@ -723,7 +729,7 @@ Image gallery management. // Tool: bookstack_images_list interface ImageGalleryListParams extends PaginationParams { filter?: { - name?: string; // Partial name match + name?: string; // Exact name match type?: 'gallery' | 'drawio'; // Image type filter uploaded_to?: number; // Page association filter }; @@ -1083,7 +1089,7 @@ interface PaginationParams { } interface FilterParams { - name?: string; // Name filter (partial match) + name?: string; // Name filter (exact match) created_by?: number;// Creator filter // ... other entity-specific filters } @@ -1093,6 +1099,11 @@ interface FilterParams { ### MCP Error Codes +A failed tool call is not a JSON-RPC error: it returns a `tools/call` result with +`isError: true`, whose text gives the message and then the details as JSON (`type`, +`validation`, `status`, `details` and any recovery hints). The codes below apply to the +protocol errors that remain: an unknown tool name and a failed `resources/read`. + | Code | Type | Description | HTTP Status | |------|------|-------------|-------------| | `InvalidRequest` | `validation_error` | Invalid parameters | 400 | diff --git a/docs/examples-and-workflows.md b/docs/examples-and-workflows.md index f80edfb..094bf2c 100644 --- a/docs/examples-and-workflows.md +++ b/docs/examples-and-workflows.md @@ -62,7 +62,7 @@ curl http://localhost:3000/health "checks": [ { "name": "bookstack_connection", "healthy": true, "message": "BookStack API connection" }, { "name": "tools_loaded", "healthy": true, "message": "59 tools loaded" }, - { "name": "resources_loaded", "healthy": true, "message": "11 resources loaded" } + { "name": "resources_loaded", "healthy": true, "message": "5 resources and 6 resource templates loaded" } ] } ``` @@ -1172,7 +1172,7 @@ async function callTool(name: string, args: Record): Promise 'Content-Type': 'application/json', Accept: 'application/json, text/event-stream', Authorization: `Bearer ${MCP_AUTH_TOKEN}`, - // Optional per-request credential override; falls back to the server's env vars + // Optional: the caller's own BookStack token, spent at the server's BOOKSTACK_BASE_URL 'x-bookstack-token': process.env.BOOKSTACK_API_TOKEN ?? '', }, body: JSON.stringify({ @@ -1187,6 +1187,10 @@ async function callTool(name: string, args: Record): Promise if (body.error) { throw new Error(`${name} failed: ${body.error.message}`); } + // A tool failure is a result with isError; its text is the message plus JSON details. + if (body.result.isError) { + throw new Error(`${name} failed: ${body.result.content[0].text}`); + } // Tool results come back as MCP content parts; the payload is JSON in the text part. return JSON.parse(body.result.content[0].text) as T; } @@ -1226,10 +1230,11 @@ step (or a service container) before this script runs. #### Environment-Specific Configuration A single running server can address several BookStack instances: `POST /message` -accepts **per-request credential overrides** via the `x-bookstack-url` and -`x-bookstack-token` headers, each falling back to `BOOKSTACK_BASE_URL` / -`BOOKSTACK_API_TOKEN` when omitted. So "environments" are just header sets — you do not -need one server process per environment. +accepts **per-request overrides** via the `x-bookstack-url` and `x-bookstack-token` +headers. The server operator lists every instance's API base URL in +`BOOKSTACK_ALLOWED_BASE_URLS`, and each request that names one must also carry that +instance's own token; anything else is refused with a `400`. So "environments" are just +header sets — you do not need one server process per environment. ```typescript // config/environments.ts @@ -1294,6 +1299,7 @@ async function callTool(env: Env, name: string, args: Record const body = await res.json(); if (body.error) throw new Error(`${name} failed: ${body.error.message}`); + if (body.result.isError) throw new Error(`${name} failed: ${body.result.content[0].text}`); return JSON.parse(body.result.content[0].text) as T; } @@ -1302,7 +1308,7 @@ const syncEnvironments = async (sourceEnv: Env, targetEnv: Env) => { // accepts only book_id / chapter_id / name / created_by / draft / template. Under // the shipped default (VALIDATION_STRICT_MODE=true) an unknown key like `tag` is // REJECTED at the boundary — `bookstack_pages_list` with `filter: {tag: ...}` - // answers JSON-RPC -32602 `Validation failed` / `Unrecognized key: "tag"`. Only + // answers an isError result: `Validation failed` / `Unrecognized key: "tag"`. Only // with strict mode disabled are the params forwarded to BookStack unchanged, which // ignores the unrecognised filter and hands back EVERY page. Neither mode filters // by tag, so reach for search. diff --git a/docs/releasing.md b/docs/releasing.md index a9dcc22..95fa5fd 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -381,17 +381,23 @@ Images are versioned `-r`: `` is `package.json#versi Pushing a tag like `v2.1.0-r1` runs `.github/workflows/image.yml`, which: -1. refuses a tag whose `` is not `package.json#version`; +1. refuses a tag whose commit is not on `main`, or whose `` is not + `package.json#version`; 2. builds the image with `SERVER_VERSION=2.1.0-r1`, so `GET /` and MCP `initialize` report it; 3. runs the image smoke suite against it; -4. only then pushes `ghcr.io//bookstack-mcp-server:2.1.0-r1`, with provenance and +4. refuses a tag whose image already exists in GHCR, so a published image is never + overwritten; +5. only then pushes `ghcr.io//bookstack-mcp-server:2.1.0-r1`, with provenance and an SBOM. No `latest` or moving tags: deploy by digest. ```bash git tag v2.1.0-r1 && git push origin v2.1.0-r1 -gh workflow run image.yml --ref v2.1.0-r1 # rebuild an existing tag +gh workflow run image.yml --ref v2.1.0-r1 # build a tag that has no image yet ``` +The dispatch is for a tag whose run failed before pushing. To change a published image, +tag the next revision (`v2.1.0-r2`) instead. + Plain `vX.Y.Z` tags, including the ones release-please creates, build no image. To follow these tags with Renovate, use regex versioning such as `regex:^(?\d+)\.(?\d+)\.(?\d+)-r(?\d+)$`; its default diff --git a/docs/resources-guide.md b/docs/resources-guide.md index 93c2b62..e982211 100644 --- a/docs/resources-guide.md +++ b/docs/resources-guide.md @@ -22,7 +22,14 @@ This guide covers the **Resources** system in the BookStack MCP Server, which pr ## Available Resource Types -The BookStack MCP Server provides **12 resource types** across 6 categories: +The BookStack MCP Server provides **5 resources and 6 resource templates** across 6 categories: + +- `resources/list` returns the 5 concrete URIs (`bookstack://books`, `bookstack://pages`, + `bookstack://chapters`, `bookstack://shelves`, `bookstack://users`). +- `resources/templates/list` returns the 6 templates as `uriTemplate` entries + (`bookstack://books/{id}`, `bookstack://pages/{id}`, `bookstack://chapters/{id}`, + `bookstack://shelves/{id}`, `bookstack://users/{id}`, `bookstack://search/{query}`). + Fill in the placeholder and pass the concrete URI to `resources/read`. ### 1. Book Resources @@ -331,7 +338,7 @@ The BookStack MCP resources include enhanced schema definitions with: All resources include these standard properties: -- `uri`: Resource identifier pattern +- `uri`: Resource identifier, or the URI template (published as `uriTemplate` on `resources/templates/list`) - `name`: Human-readable name - `description`: Detailed description of the resource - `mimeType`: Content type (typically `application/json`) @@ -559,8 +566,9 @@ const overview = { Use resource metadata for dynamic applications: ```javascript -// Discover available resources +// Discover available resources; parameterised ones come from listResourceTemplates() const resources = await listResources(); +const templates = await listResourceTemplates(); // e.g. { uriTemplate: 'bookstack://books/{id}' } const bookResources = resources.filter(r => r.uri.includes('books')); // Build dynamic navigation diff --git a/docs/setup-guide.md b/docs/setup-guide.md index d0c0341..ceacaef 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -21,7 +21,7 @@ The BookStack MCP Server provides comprehensive access to BookStack's knowledge ### Key Features - **59 MCP Tools** across 13 categories, covering the supported subset of the BookStack API -- **11 Resources** for dynamic content retrieval +- **5 Resources and 6 Resource Templates** for dynamic content retrieval - **Rate Limiting** with configurable limits - **Comprehensive Validation** using Zod schemas - **Error Handling** with retry logic @@ -197,10 +197,6 @@ VALIDATION_STRICT_MODE=true # Logging LOG_LEVEL=info LOG_FORMAT=pretty - -# Development -NODE_ENV=development -DEBUG=false ``` > 💡 These are the variables the server actually reads. The full, authoritative @@ -369,8 +365,6 @@ For production deployments: ```env # Production Environment Variables -NODE_ENV=production -DEBUG=false LOG_LEVEL=warn LOG_FORMAT=json @@ -384,9 +378,12 @@ A single server process talks to exactly one BookStack instance. To cover several, run one server per instance with its own `BOOKSTACK_BASE_URL`, `BOOKSTACK_API_TOKEN`, and `SERVER_PORT`. -Alternatively, in HTTP mode a single server can serve multiple instances by -sending per-request credentials on `POST /message` with the `x-bookstack-url` -and `x-bookstack-token` headers, which override the environment defaults. +Alternatively, in HTTP mode a single server can serve multiple instances. List +their API base URLs in `BOOKSTACK_ALLOWED_BASE_URLS` (comma-separated), and send +`x-bookstack-url` together with that instance's own `x-bookstack-token` on +`POST /message`. A URL that is not listed, or one sent without a token, is refused +with a `400`: the configured `BOOKSTACK_API_TOKEN` is only ever sent to +`BOOKSTACK_BASE_URL`. ## 🛠️ Development Setup @@ -542,7 +539,7 @@ docker run -i --rm \ Use `-i`, not `-it` — allocating a TTY corrupts the JSON-RPC stream that MCP clients pipe over stdin/stdout. No port mapping is needed in stdio mode. -> ⚠️ The image's `HEALTHCHECK` probes `http://localhost:3000/health`, so a +> ⚠️ The image's `HEALTHCHECK` probes `http://localhost:$SERVER_PORT/health` (default `3000`), so a > container started with `MCP_TRANSPORT=stdio` will report `unhealthy` — nothing > is listening on a port in stdio mode. That is expected. @@ -620,8 +617,6 @@ has no stdin, so stdin hits EOF and the process exits. Enable debug logging: ```bash -# Set debug environment -export DEBUG=true export LOG_LEVEL=debug # Run with debug output @@ -630,8 +625,9 @@ bun run dev ### HTTP Endpoints -In HTTP mode the server exposes exactly three endpoints. Any other path returns -a JSON `404` listing the valid ones. +In HTTP mode the server exposes exactly three endpoints. `GET` and `DELETE` on +`/message` answer `405` with `Allow: POST`, before authentication: the server offers +no SSE stream and no sessions. Any other path returns a JSON `404` listing the valid ones. | Method & path | Purpose | Status codes | | --- | --- | --- | @@ -644,9 +640,10 @@ inbound `Authorization: Bearer ` header**, since it dispatches every too the HTTP transport refuses to start until a secret is configured, and its startup error names the exact variable to set. -`POST /message` also accepts per-request credential overrides via the -`x-bookstack-url` and `x-bookstack-token` headers, falling back to -`BOOKSTACK_BASE_URL` / `BOOKSTACK_API_TOKEN`. +`POST /message` also accepts per-request overrides. `x-bookstack-token` alone +spends the caller's own token at `BOOKSTACK_BASE_URL`. `x-bookstack-url` is refused +with a `400` unless it is listed in `BOOKSTACK_ALLOWED_BASE_URLS`, and then requires +`x-bookstack-token` as well. ```bash # MCP initialize handshake — Content-Type and Accept are required by the @@ -683,7 +680,7 @@ curl -i http://localhost:3000/health "checks": [ {"name": "bookstack_connection", "healthy": true, "message": "BookStack API connection"}, {"name": "tools_loaded", "healthy": true, "message": "59 tools loaded"}, - {"name": "resources_loaded", "healthy": true, "message": "11 resources loaded"} + {"name": "resources_loaded", "healthy": true, "message": "5 resources and 6 resource templates loaded"} ] } ``` @@ -693,17 +690,24 @@ check failed — most often `bookstack_connection`, caused by an **invalid** `BOOKSTACK_API_TOKEN` (present but wrong or revoked) or an unreachable `BOOKSTACK_BASE_URL`. -A **missing** `BOOKSTACK_API_TOKEN` never produces a `503`. The token is validated -at startup (`z.string().min(1)`), so an empty value fails config validation before -Express binds the port: +A **missing** `BOOKSTACK_API_TOKEN` never produces a `503`. The configuration itself +still loads, because OAuth mode runs without the token, but each transport refuses to +start without it: -``` -error: Configuration validation failed: bookstack.apiToken: BookStack API token is required - set BOOKSTACK_API_TOKEN environment variable -``` +- **HTTP (token mode)** fails before Express binds the port. Stderr shows the line + below, the process exits with code `1`, nothing listens on `SERVER_PORT`, and `curl` + gets a connection refused rather than a status code. + + ``` + Failed to start HTTP transport: Configuration validation failed: bookstack.apiToken: BookStack API token is required - set BOOKSTACK_API_TOKEN environment variable + ``` + +- **stdio** prints the same reason as `Failed to start stdio transport: …` to stderr, + writes nothing to stdout, and exits with code `1`, so the MCP client sees the server + exit during startup. -The process exits, nothing listens on `SERVER_PORT`, and `curl` gets a connection -refused rather than a status code. Distinguishing the two saves time: **`503` = bad -token, connection refused = no token.** +Distinguishing the two saves time: **`503` = bad token, connection refused = no +token.** ### Log Analysis diff --git a/docs/tools-overview.md b/docs/tools-overview.md index 4879e58..3eb2637 100644 --- a/docs/tools-overview.md +++ b/docs/tools-overview.md @@ -6,11 +6,11 @@ This document provides an overview of every tool implemented in the BookStack MC ## Executive Summary -The BookStack MCP Server provides **59 tools** (and **11 resources**) organized into **13 categories**, implementing the supported subset of the BookStack knowledge management API. Each tool follows consistent patterns for validation, error handling, and logging. +The BookStack MCP Server provides **59 tools** (plus **5 resources** and **6 resource templates**) organized into **13 categories**, implementing the supported subset of the BookStack knowledge management API. Each tool follows consistent patterns for validation, error handling, and logging. The categories below are the ones returned by `bookstack_tool_categories`, and the per-category counts add up to the 59 tools the server registers at boot (it logs -`Registered 59 tools` / `Registered 11 resources` on startup): +`Registered 59 tools` / `Registered 5 resources and 6 resource templates` on startup): | Section | Category | Tools | |---------|----------|-------| @@ -98,7 +98,7 @@ The guards, and why each exists: | `dry_run` | Applies the edits in memory and reports what would change. Nothing is sent to BookStack. | | `expected_updated_at` | Best-effort stale-page preflight against `updated_at`. It catches a page changed before the server reads it; BookStack has no atomic version condition, so it cannot prevent a later racing write. | | Shrink guard | A result smaller than half the original is refused unless `allow_shrink` is set, so an anchor that accidentally swallows most of the document cannot be applied. | -| Post-write verification | The page is re-read and the written fragments are looked for in normalised text — BookStack rewrites stored HTML on save (heading anchors, injected `id` attributes), so a byte comparison would report every success as a failure. A fragment that cannot be found comes back as `verified: false` rather than an error: the write did happen. | +| Post-write verification | The page is re-read and the written fragments are looked for in the raw stored source, then in normalised text — BookStack rewrites stored HTML on save (heading anchors, injected `id` attributes), so a byte comparison alone would report every success as a failure. A fragment that cannot be found comes back as `verified: false` rather than an error: the write did happen. If the re-read itself fails, the result is `verified: null` with a note, so a client does not retry a write that landed. | Every write creates a BookStack revision, so an applied edit can be rolled back in the UI. No response from these tools contains page content, which is what keeps it out of the model. @@ -357,12 +357,21 @@ bookstack_audit_log_list({ filter: { loggable_type: "page", loggable_id: 42 } }) ``` > ⚠️ `count` above 500 is **rejected, not clamped**. With `VALIDATION_STRICT_MODE` -> (default `true`) the schema's upper bound fails the call at the boundary: +> (default `true`) the schema's upper bound fails the call at the boundary. The result +> has `isError: true`, and its text content reads: > -> ```json -> { "code": -32602, "message": "MCP error -32602: Validation failed", -> "data": { "type": "validation_error", -> "validation": [{ "field": "count", "message": "Too big: expected number to be <=500" }] } } +> ``` +> Validation failed +> +> { +> "type": "validation_error", +> "validation": [ +> { +> "field": "count", +> "message": "Too big: expected number to be <=500" +> } +> ] +> } > ``` > > This applies to the audit log and recycle bin listings too. `bookstack_search` is @@ -427,16 +436,23 @@ bookstack_audit_log_list({ filter: { loggable_type: "page", loggable_id: 42 } }) - Recovery: Wait and retry with exponential backoff ### Error Response Format + +A tool that fails - invalid arguments, a BookStack error, a page edit that cannot be +applied - returns a normal `tools/call` result with `isError: true`, so the model can +read the reason and correct its call. The text holds the message, a blank line, then +the details as JSON: `type`, `validation` issues with their field paths, BookStack's +`status` and `details`, and recovery hints such as `found_with_different_whitespace`, +`available_sections` or `matched_sections`. + ```typescript { - error: { - code: string, - message: string, - details?: object - } + isError: true, + content: [{ type: 'text', text: string }] } ``` +An unknown tool name and a failed `resources/read` stay JSON-RPC errors. + ## Validation and Security ### Input Validation @@ -579,6 +595,6 @@ The modular architecture allows for easy extension: ## Conclusion -The BookStack MCP Server is a production-ready implementation providing LLMs with access to the supported subset of BookStack's API. With 59 tools across 13 categories, 11 resources, robust error handling, strict validation, and extensive documentation, it enables sophisticated knowledge management workflows while maintaining security and reliability. +The BookStack MCP Server is a production-ready implementation providing LLMs with access to the supported subset of BookStack's API. With 59 tools across 13 categories, 5 resources and 6 resource templates, robust error handling, strict validation, and extensive documentation, it enables sophisticated knowledge management workflows while maintaining security and reliability. The consistent patterns, extensive examples, and self-documenting capabilities make it easy for LLMs to understand and effectively utilize the full power of the BookStack platform through the MCP protocol. diff --git a/package.json b/package.json index 3de0c77..67143fa 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "1.29.0", - "axios": "1.18.1", + "axios": "1.20.0", "dotenv": "17.4.2", "express": "5.2.1", "jose": "6.2.3", diff --git a/src/api/client.ts b/src/api/client.ts index a74110c..0d6b0d1 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -1,6 +1,7 @@ import { Buffer } from 'node:buffer'; import { readFile, realpath } from 'node:fs/promises'; -import { Agent } from 'node:https'; +import { Agent as HttpAgent } from 'node:http'; +import { Agent as HttpsAgent } from 'node:https'; import { basename, extname, sep } from 'node:path'; import axios, { type AxiosError, @@ -105,10 +106,11 @@ const RETRY_JITTER_RATIO = 0.25; /** * Methods whose replay cannot create or duplicate state. * - * Used ONLY to gate 5xx retries. A 5xx means the request reached BookStack and may have - * partially applied - replaying a POST could duplicate a page - so 5xx is retried for - * safe verbs only. A 429 is different in kind: the throttle middleware rejects the - * request *before* the route runs, so nothing was executed and any verb may be replayed. + * Gates 5xx and connection-failure retries. A 5xx means the request reached BookStack and + * may have partially applied - replaying a POST could duplicate a page - and a dropped + * connection cannot say whether it did, so both are retried for safe verbs only. A 429 is + * different in kind: the throttle middleware rejects the request *before* the route runs, + * so nothing was executed and any verb may be replayed. */ const REPLAY_SAFE_METHODS: readonly string[] = ['GET', 'HEAD', 'OPTIONS']; @@ -161,6 +163,21 @@ export interface ExportContent extends ExportResult { byte_length: number; } +/** Socket idle timeout for the shared agents; request time limits stay on axios's `timeout`. */ +const AGENT_SOCKET_TIMEOUT_MS = 30_000; + +/** Keep-alive agents shared by every client, so sockets are pooled across requests. */ +const sharedHttpAgent = new HttpAgent({ + keepAlive: true, + maxFreeSockets: 10, + timeout: AGENT_SOCKET_TIMEOUT_MS, +}); +const sharedHttpsAgent = new HttpsAgent({ + keepAlive: true, + maxFreeSockets: 10, + timeout: AGENT_SOCKET_TIMEOUT_MS, +}); + /** Pause for `ms`, used between retry attempts. */ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -311,67 +328,58 @@ function isContainedIn(target: string, root: string): boolean { return target.startsWith(rootWithSep); } -/** Resolve a path to its real location, reporting a clear error if it is unreadable. */ -async function resolveRealPath(candidate: string, label: string): Promise { - try { - return await realpath(candidate); - } catch { - throw new Error(`${label} '${candidate}' does not exist or is not readable by the server.`); - } -} +/** `file_path` without an operator-chosen upload directory. */ +export const UPLOAD_ROOT_UNSET_MESSAGE = + "'file_path' is disabled because BOOKSTACK_UPLOAD_ROOT is not set. Ask the operator to set " + + 'it to a directory uploads may be read from, or send the file content inline as base64.'; + +/** BOOKSTACK_UPLOAD_ROOT is set but cannot be resolved; the value itself is not repeated. */ +export const UPLOAD_ROOT_UNUSABLE_MESSAGE = + "'file_path' is unavailable because BOOKSTACK_UPLOAD_ROOT is not a readable directory on " + + 'this server. Send the file content inline as base64 instead.'; + +/** One refusal for missing, unreadable and out-of-root paths alike, so it reveals nothing. */ +export const UPLOAD_PATH_REFUSED_MESSAGE = + "'file_path' must name a readable file inside BOOKSTACK_UPLOAD_ROOT. Send the file content " + + 'inline as base64 instead.'; /** * SECURITY GUARD for the `file_path` upload parameter. * * `file_path` asks the *server process* to read a local file and upload its bytes to - * BookStack. This server also exposes an HTTP transport (`POST /message`), where the - * caller is remote and untrusted. An unguarded `file_path` would therefore be an - * arbitrary-local-file-read and exfiltration primitive: a remote caller could pass - * `/etc/passwd` or `~/.ssh/id_rsa` and have the server upload it into BookStack. - * - * The rules, in order: + * BookStack. Unguarded, that is an arbitrary-local-file-read and exfiltration primitive for + * whoever steers the tool arguments: a remote HTTP caller, or - under stdio - text planted in + * a page that an agent reads and then acts on, aiming it at `~/.ssh/id_rsa`. * - * 1. stdio transport (`MCP_TRANSPORT=stdio`): the MCP client launched this process and - * shares its trust domain, so any path it could already read itself is allowed. - * 2. Any other transport (HTTP is the default): `file_path` is refused outright unless - * the operator explicitly opts in by setting `BOOKSTACK_UPLOAD_ROOT`. When set, the - * candidate is resolved with realpath() - which also expands symlinks - and must be - * contained within the likewise-resolved root. This rejects `../` traversal and - * symlink escapes, since containment is checked after resolution, not before. + * So under EVERY transport, stdio included, `file_path` is refused unless the operator opts + * in with `BOOKSTACK_UPLOAD_ROOT`. The candidate is then resolved with realpath() - which also + * expands symlinks - and must be contained within the likewise-resolved root. This rejects + * `../` traversal and symlink escapes, since containment is checked after resolution. * - * Refusal is always explicit; `file_path` is never silently ignored. + * Refusal is always explicit, and never names a resolved path or the root. */ export async function readGuardedUploadFile(filePath: string): Promise { - const transport = process.env.MCP_TRANSPORT ?? 'http'; - - // 1. Local stdio client: same trust domain as this process. - if (transport === 'stdio') { - const realTarget = await resolveRealPath(filePath, 'file_path'); - return readFile(realTarget); - } - - // 2. Remote-capable transport: require an explicit opt-in root. const uploadRoot = process.env.BOOKSTACK_UPLOAD_ROOT; if (!uploadRoot) { - throw new Error( - `'file_path' is refused under the '${transport}' transport because the caller may be remote, ` + - 'and reading arbitrary server-local files would leak them into BookStack. ' + - 'Set BOOKSTACK_UPLOAD_ROOT to a directory that uploads may be read from to enable it, ' + - 'or send the file content inline as base64 instead.' - ); + throw new Error(UPLOAD_ROOT_UNSET_MESSAGE); } - const realRoot = await resolveRealPath(uploadRoot, 'BOOKSTACK_UPLOAD_ROOT'); - const realTarget = await resolveRealPath(filePath, 'file_path'); - - if (!isContainedIn(realTarget, realRoot)) { - throw new Error( - `'file_path' resolves to '${realTarget}', which is outside BOOKSTACK_UPLOAD_ROOT ` + - `('${realRoot}'). Only files within that directory may be uploaded.` - ); + let realRoot: string; + try { + realRoot = await realpath(uploadRoot); + } catch { + throw new Error(UPLOAD_ROOT_UNUSABLE_MESSAGE); } - return readFile(realTarget); + try { + const realTarget = await realpath(filePath); + if (!isContainedIn(realTarget, realRoot)) { + throw new Error(UPLOAD_PATH_REFUSED_MESSAGE); + } + return await readFile(realTarget); + } catch { + throw new Error(UPLOAD_PATH_REFUSED_MESSAGE); + } } /** @@ -484,7 +492,7 @@ export class BookStackClient implements BookStackAPIClient { // value makes that class of drift unrepresentable rather than merely absent today. // // It also validates: `x-bookstack-url` is caller-supplied and never passes through the - // config schema (server.ts merges the header straight into a Partial), so this + // config schema (server.ts only checks it against BOOKSTACK_ALLOWED_BASE_URLS), so this // is where a junk or non-http(s) URL is refused - before axios is pointed at it. const baseUrl = canonicalBaseUrl(config.bookstack.baseUrl); @@ -504,19 +512,15 @@ export class BookStackClient implements BookStackAPIClient { burstLimit: config.rateLimit.burstLimit, }); - // Create HTTP agent for connection pooling - const httpsAgent = new Agent({ - keepAlive: true, - maxSockets: 10, - timeout: config.bookstack.timeout, - }); - // Initialize Axios client this.client = axios.create({ // The same canonical value the bucket above is keyed by - see the note there. baseURL: baseUrl, timeout: config.bookstack.timeout, - httpsAgent, + httpAgent: sharedHttpAgent, + httpsAgent: sharedHttpsAgent, + // A redirect would carry the credential to a host nobody configured. + maxRedirects: 0, headers: { Authorization: `${credential.scheme} ${credential.secret}`, 'Content-Type': 'application/json', @@ -599,9 +603,9 @@ export class BookStackClient implements BookStackAPIClient { * - Is replaying this verb safe? -> here, because only the caller knows the method. * * A 429 is always replayable: Laravel's throttle middleware rejects the request before - * the route executes, so no work was done and no state changed. A 5xx is only replayed - * for safe verbs, since the request did reach the application and a POST that timed out - * mid-write could otherwise be duplicated. + * the route executes, so no work was done and no state changed. A 5xx or a dropped + * connection is only replayed for safe verbs, since the request may have reached the + * application and a POST could otherwise be duplicated. Timeouts are never replayed. */ private isReplayable(error: unknown, method: string): boolean { if (!this.errorHandler.isRetryable(error)) { @@ -679,6 +683,7 @@ export class BookStackClient implements BookStackAPIClient { method, url: config.url, status: info.status, + connectionFailed: info.connectionFailed, attempt, nextAttempt: attempt + 1, maxAttempts: RETRY_MAX_ATTEMPTS, @@ -842,7 +847,7 @@ export class BookStackClient implements BookStackAPIClient { * Resolve a `file_path` field into binary content for `fileField`. * * Delegates the path check to readGuardedUploadFile(); see that function for the - * transport-dependent security rules. + * security rules. */ private async resolveLocalFile(fields: UploadParams, fileField: string): Promise { const filePath = fields[LOCAL_PATH_FIELD]; diff --git a/src/config/manager.ts b/src/config/manager.ts index 45d0e29..5c2bbb2 100644 --- a/src/config/manager.ts +++ b/src/config/manager.ts @@ -35,9 +35,9 @@ dotenvConfig({ quiet: true }); * the helper stays beside the identity it defines rather than moving somewhere neutral. * * This validates without transforming. Canonicalisation stays at the single point that - * decides identity (getSharedRateLimiter/BookStackClient), so what an operator configured is - * what getSummary() reports back to them, and `Partial` overrides - which never pass - * through this schema - cannot come to depend on the schema having rewritten a value. + * decides identity (getSharedRateLimiter/BookStackClient), so `Partial` overrides - + * which never pass through this schema - cannot come to depend on the schema having + * rewritten a value. */ const baseUrlCheck = (value: string, ctx: z.RefinementCtx): void => { try { @@ -88,10 +88,6 @@ export const ConfigSchema = z.object({ level: z.enum(['error', 'warn', 'info', 'debug']).default('info'), format: z.enum(['json', 'pretty']).default('pretty'), }), - development: z.object({ - nodeEnv: z.enum(['development', 'production', 'test']).default('development'), - debug: z.boolean().default(false), - }), }); export type Config = z.infer; @@ -182,6 +178,18 @@ export const OAuthConfigSchema = z export type OAuthConfig = z.infer; +/** baseUrlCheck() for one BOOKSTACK_ALLOWED_BASE_URLS entry, naming that setting. */ +const allowedBaseUrlCheck = (value: string, ctx: z.RefinementCtx): void => { + try { + canonicalBaseUrl(value); + } catch (error) { + ctx.addIssue({ + code: 'custom', + message: `BOOKSTACK_ALLOWED_BASE_URLS: ${(error as Error).message}`, + }); + } +}; + const AuthModeSchema = z .enum(['token', 'oauth'], { error: "MCP_AUTH_MODE must be 'token' or 'oauth'" }) .default('token'); @@ -214,6 +222,8 @@ export const HttpTransportConfigSchema = z.object({ */ authToken: z.string().min(1).optional(), oauth: OAuthConfigSchema.optional(), + /** BookStack base URLs `x-bookstack-url` may name (BOOKSTACK_ALLOWED_BASE_URLS); unset refuses the header. */ + allowedBaseUrls: z.array(z.string().superRefine(allowedBaseUrlCheck)).min(1).optional(), }); export type HttpTransportConfig = z.infer; @@ -272,6 +282,15 @@ function envString(value: string | undefined): string | undefined { return trimmed === undefined || trimmed === '' ? undefined : trimmed; } +/** Read a comma-separated environment variable; blank entries are dropped, and none means unset. */ +function envList(value: string | undefined): string[] | undefined { + const entries = (value ?? '') + .split(',') + .map((entry) => entry.trim()) + .filter((entry) => entry !== ''); + return entries.length > 0 ? entries : undefined; +} + /** Render Zod issues as `path: message` pairs for operator-facing errors. */ function formatZodIssues(error: z.ZodError): string[] { return error.issues.map((issue: z.core.$ZodIssue) => @@ -291,6 +310,7 @@ export function loadHttpTransportConfig(env: EnvSource = process.env): HttpTrans const http = HttpTransportConfigSchema.parse({ bodyLimitBytes: envNumber(env.HTTP_BODY_LIMIT), authToken: envString(env.MCP_AUTH_TOKEN), + allowedBaseUrls: envList(env.BOOKSTACK_ALLOWED_BASE_URLS), oauth: mode === 'oauth' ? { @@ -394,10 +414,6 @@ export class ConfigManager { level: process.env.LOG_LEVEL || 'info', format: process.env.LOG_FORMAT || 'pretty', }, - development: { - nodeEnv: process.env.NODE_ENV || 'development', - debug: process.env.DEBUG === 'true', - }, }; try { @@ -442,25 +458,6 @@ export class ConfigManager { this.config = this.loadConfig(); return this.config; } - - /** - * Get configuration summary for logging - */ - getSummary(): object { - const config = this.getConfig(); - return { - bookstack: { - baseUrl: config.bookstack.baseUrl, - hasApiToken: !!config.bookstack.apiToken, - timeout: config.bookstack.timeout, - }, - server: config.server, - rateLimit: config.rateLimit, - validation: config.validation, - logging: config.logging, - development: config.development, - }; - } } export default ConfigManager; diff --git a/src/server.ts b/src/server.ts index f8489a3..debb9a9 100644 --- a/src/server.ts +++ b/src/server.ts @@ -8,6 +8,7 @@ import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; import { CallToolRequestSchema, ListResourcesRequestSchema, + ListResourceTemplatesRequestSchema, ListToolsRequestSchema, ReadResourceRequestSchema, } from '@modelcontextprotocol/sdk/types.js'; @@ -82,6 +83,11 @@ function schemaPropertyNames(node: MCPSchemaNode, into: Set): void { } } +/** Whether a registered resource URI is an RFC 6570 template rather than a concrete URI. */ +function isResourceTemplate(uri: string): boolean { + return uri.includes('{'); +} + /** * Split the names a tool was called with into "ours" and "how many others". * @@ -123,7 +129,7 @@ function splitArgumentNames(tool: MCPTool, args: unknown): { known: string[]; un * shelves, users, roles, attachments, image gallery, search, recycle bin, content * permissions, the audit log and system info. Not every endpoint family is exposed - * comments, imports, tags, image-gallery `data` and ZIP export are not. - * - 11 resources for read-only content access + * - 5 resources and 6 resource templates for read-only content access * - Comprehensive error handling and validation * - Rate limiting and retry policies */ @@ -173,9 +179,11 @@ export class BookStackMCPServer { this.registerLogVocabulary(); this.setupHandlers(); + const resourceCounts = this.resourceCounts(); this.logger.info('BookStack MCP Server initialized', { tools: this.tools.size, - resources: this.resources.size, + resources: resourceCounts.resources, + resource_templates: resourceCounts.templates, // Not `{baseUrl}` - see the note at the same line in ./api/client.ts, and // describeBaseUrl() in ./utils/rateLimit. ...describeBaseUrl(config.bookstack.baseUrl), @@ -262,7 +270,14 @@ export class BookStackMCPServer { }); }); - this.logger.info(`Registered ${this.resources.size} resources`); + const { resources, templates } = this.resourceCounts(); + this.logger.info(`Registered ${resources} resources and ${templates} resource templates`); + } + + /** How many registered resources are concrete URIs, and how many are templates. */ + private resourceCounts(): { resources: number; templates: number } { + const templates = [...this.resources.keys()].filter(isResourceTemplate).length; + return { resources: this.resources.size - templates, templates }; } /** @@ -365,23 +380,41 @@ export class BookStackMCPServer { // stack. Handing it pre-stringified would be redacted down to a size and // lose the type/status that make the line useful. this.logger.error('Tool failed', { tool: tool.name, err: error }); - throw this.errorHandler.handleError(error); + // Execution and validation errors are results the model reads, per the MCP spec. + return this.errorHandler.toToolErrorResult(error); } }); - // List resources handler + // List resources handler: concrete URIs only, templates are listed separately this.server.setRequestHandler(ListResourcesRequestSchema, async () => { - const resources = Array.from(this.resources.values()).map((resource) => ({ - uri: resource.uri, - name: resource.name, - description: resource.description, - mimeType: resource.mimeType, - })); + const resources = Array.from(this.resources.values()) + .filter((resource) => !isResourceTemplate(resource.uri)) + .map((resource) => ({ + uri: resource.uri, + name: resource.name, + description: resource.description, + mimeType: resource.mimeType, + })); this.logger.debug(`Listed ${resources.length} resources`); return { resources }; }); + // List resource templates handler + this.server.setRequestHandler(ListResourceTemplatesRequestSchema, async () => { + const resourceTemplates = Array.from(this.resources.values()) + .filter((resource) => isResourceTemplate(resource.uri)) + .map((resource) => ({ + uriTemplate: resource.uri, + name: resource.name, + description: resource.description, + mimeType: resource.mimeType, + })); + + this.logger.debug(`Listed ${resourceTemplates.length} resource templates`); + return { resourceTemplates }; + }); + // Read resource handler this.server.setRequestHandler(ReadResourceRequestSchema, async (request) => { const { uri } = request.params; @@ -445,7 +478,7 @@ export class BookStackMCPServer { uri: string ): { resource: MCPResource; facts: Record } | undefined { for (const [template, resource] of this.resources.entries()) { - if (!template.includes('{')) { + if (!isResourceTemplate(template)) { if (template === uri) { return { resource, facts: {} }; } @@ -500,6 +533,7 @@ export class BookStackMCPServer { status: 'healthy' | 'unhealthy'; checks: Array<{ name: string; healthy: boolean; message?: string }>; }> { + const { resources, templates } = this.resourceCounts(); const checks = [ { name: 'bookstack_connection', @@ -514,7 +548,7 @@ export class BookStackMCPServer { { name: 'resources_loaded', healthy: this.resources.size > 0, - message: `${this.resources.size} resources loaded`, + message: `${resources} resources and ${templates} resource templates loaded`, }, ]; @@ -771,24 +805,82 @@ async function oauthReadiness( return { status: checks.every((check) => check.healthy) ? 'healthy' : 'unhealthy', checks }; } -/** - * Per-request credential selection in shared-secret mode. Reachable only by callers that - * cleared the bearer check; it is an outbound-request surface, since x-bookstack-url - * decides which host this server will talk to on the caller's behalf. - */ -function tokenModeOverrides(req: Request, config: Config): Partial { - const bookstackUrl = req.headers['x-bookstack-url'] as string; - const bookstackToken = req.headers['x-bookstack-token'] as string; - - return { - bookstack: { - baseUrl: bookstackUrl || config.bookstack.baseUrl, - apiToken: bookstackToken || config.bookstack.apiToken, - timeout: config.bookstack.timeout, - }, +/** x-bookstack-url without an operator allowlist. */ +export const UPSTREAM_URL_DISABLED_MESSAGE = + 'x-bookstack-url is not accepted: this server has no BOOKSTACK_ALLOWED_BASE_URLS configured.'; + +/** x-bookstack-url naming an upstream the operator did not list; the value is not repeated. */ +export const UPSTREAM_URL_NOT_ALLOWED_MESSAGE = + 'x-bookstack-url is not one of the BookStack base URLs in BOOKSTACK_ALLOWED_BASE_URLS.'; + +/** x-bookstack-url without the caller's own token. */ +export const UPSTREAM_URL_WITHOUT_TOKEN_MESSAGE = + 'x-bookstack-url requires x-bookstack-token: the configured BOOKSTACK_API_TOKEN is only ' + + 'ever sent to BOOKSTACK_BASE_URL.'; + +/** One override header as a string; blank counts as absent, as it always has. */ +function overrideHeader(req: Request, name: string): string | undefined { + const value = req.headers[name]; + return typeof value === 'string' && value !== '' ? value : undefined; +} + +/** `canonicalBaseUrl()`, or undefined for a value it refuses. */ +function canonicalOrUndefined(url: string): string | undefined { + try { + return canonicalBaseUrl(url); + } catch { + return undefined; + } +} + +/** Token-mode overrides: x-bookstack-url must be allowlisted and carry its own x-bookstack-token. */ +function tokenModeOverrides( + config: Config, + allowedBaseUrls: ReadonlySet | undefined +): RequestHandler { + return (req, res, next) => { + const url = overrideHeader(req, 'x-bookstack-url'); + const token = overrideHeader(req, 'x-bookstack-token'); + + let refusal: string | undefined; + if (url !== undefined) { + const canonical = canonicalOrUndefined(url); + if (!allowedBaseUrls) { + refusal = UPSTREAM_URL_DISABLED_MESSAGE; + } else if (canonical === undefined || !allowedBaseUrls.has(canonical)) { + refusal = UPSTREAM_URL_NOT_ALLOWED_MESSAGE; + } else if (token === undefined) { + refusal = UPSTREAM_URL_WITHOUT_TOKEN_MESSAGE; + } + } + if (refusal) { + res.status(400).json({ error: 'Bad Request', message: refusal }); + return; + } + + const overrides: Partial = { + bookstack: { + baseUrl: url ?? config.bookstack.baseUrl, + apiToken: token ?? config.bookstack.apiToken, + timeout: config.bookstack.timeout, + }, + }; + res.locals.bookstackOverrides = overrides; + next(); }; } +/** Streamable HTTP without SSE or sessions: GET and DELETE on the MCP endpoint are not offered. */ +const methodNotAllowed: RequestHandler = (req, res) => { + res.setHeader('Allow', 'POST'); + res.status(405).json({ + error: 'Method Not Allowed', + message: + `${req.method} /message is not supported: this server has no SSE stream or sessions. ` + + 'Send MCP messages with POST /message.', + }); +}; + /** Per-request BookStack overrides would let a caller send the user's BookStack token elsewhere. */ const refuseUpstreamOverrides: RequestHandler = (req, res, next) => { if ( @@ -850,6 +942,10 @@ export function createHttpApp(options: HttpAppOptions): express.Express { // process could never serve a single tool call, so it must not reach a listening socket // and then admit that one unauthenticated /health at a time. canonicalBaseUrl(config.bookstack.baseUrl); + // Same for a hand-built allowlist: an entry that cannot be canonicalised refuses the app. + const allowedBaseUrls = http.allowedBaseUrls?.length + ? new Set(http.allowedBaseUrls.map(canonicalBaseUrl)) + : undefined; const app = express(); let mcpServer: BookStackMCPServer | undefined; @@ -900,8 +996,8 @@ export function createHttpApp(options: HttpAppOptions): express.Express { * * Built from the app's own config rather than re-reading the singleton, so the readiness * probe reports on the BookStack this app was actually configured with. Cached across - * requests because constructing one registers all 59 tools and 11 resources - work an - * anonymous caller must not be able to trigger per request. + * requests because constructing one registers all 59 tools, 5 resources and 6 resource + * templates - work an anonymous caller must not be able to trigger per request. */ function healthServer(): BookStackMCPServer { if (!mcpServer) { @@ -1007,15 +1103,21 @@ export function createHttpApp(options: HttpAppOptions): express.Express { } }); + // Answered before authentication, in both auth modes. + app.get('/message', methodNotAllowed); + app.delete('/message', methodNotAllowed); + app.post( '/message', // Order is load-bearing. Authentication runs before express.json() below, so an // unauthenticated caller is turned away at the header - it can never make this // process buffer a body up to the ceiling, nor reach a BookStackMCPServer (and // therefore the operator's BookStack token). In OAuth mode the override refusal runs - // first, so a refused request never spends a token exchange. + // first, so a refused request never spends a token exchange; in token mode the override + // check runs after authentication and before the body is buffered. ...(oauth ? [refuseUpstreamOverrides] : []), authenticate, + ...(oauth ? [] : [tokenModeOverrides(config, allowedBaseUrls)]), express.json({ limit: http.bodyLimitBytes }), createBodyErrorHandler(http.bodyLimitBytes), // `req`/`res` are annotated because Express stops inferring handler parameter types @@ -1025,7 +1127,7 @@ export function createHttpApp(options: HttpAppOptions): express.Express { try { const server = oauth ? new BookStackMCPServer({ bookstack: config.bookstack }, bookstackCredential(res)) - : new BookStackMCPServer(tokenModeOverrides(req, config)); + : new BookStackMCPServer(res.locals.bookstackOverrides as Partial); // Omitting `sessionIdGenerator` selects the SDK's stateless mode, which is exactly // what passing it as `undefined` did; `exactOptionalPropertyTypes` forbids the // explicit-undefined form since the option is declared as `sessionIdGenerator?: () => string`. @@ -1076,7 +1178,14 @@ function startHttpServer(): void { const app = createHttpApp({ config, http: httpConfig }); const port = config.server.port; - app.listen(port, () => { + app.listen(port, (error) => { + if (error) { + const code = (error as NodeJS.ErrnoException).code; + console.error( + `Failed to start HTTP transport: cannot listen on port ${port} (${code ?? error.message})` + ); + process.exit(1); + } // Logger, never console.log: console.log writes to STDOUT. This path only runs // under the HTTP transport today, but a stray stdout write is exactly what // corrupted the stdio JSON-RPC stream once already (dotenv's banner), so the @@ -1089,24 +1198,30 @@ function startHttpServer(): void { }); } +/** Build and connect the stdio transport. Rejects if it is not safe to start. */ +async function startStdioServer(): Promise { + const server = new BookStackMCPServer(); + await server.connect(new StdioServerTransport()); + + console.error('BookStack MCP Server started and listening on stdio'); + + // Handle graceful shutdown + process.on('SIGINT', () => server.shutdown()); + process.on('SIGTERM', () => server.shutdown()); +} + // Start server if run directly if (import.meta.main) { const transport = process.env.MCP_TRANSPORT || 'http'; if (transport === 'stdio') { - const server = new BookStackMCPServer(); - const stdioTransport = new StdioServerTransport(); - - server.connect(stdioTransport).catch((error) => { - console.error('Failed to start server:', error); + // One line on stderr, never a stack: stdout is the protocol stream. + startStdioServer().catch((error: unknown) => { + console.error( + `Failed to start stdio transport: ${error instanceof Error ? error.message : String(error)}` + ); process.exit(1); }); - - console.error('BookStack MCP Server started and listening on stdio'); - - // Handle graceful shutdown - process.on('SIGINT', () => server.shutdown()); - process.on('SIGTERM', () => server.shutdown()); } else { try { startHttpServer(); diff --git a/src/tools/attachments.ts b/src/tools/attachments.ts index ad0960e..8b7fc1e 100644 --- a/src/tools/attachments.ts +++ b/src/tools/attachments.ts @@ -173,13 +173,15 @@ export class AttachmentTools { }, file: { type: 'string', + minLength: 1, description: 'Base64 encoded file content, at most 50000 KB. Mutually exclusive with file_path and link.', }, file_path: { type: 'string', + minLength: 1, description: - 'Path to a file on the server to upload instead of inlining base64. Mutually exclusive with file and link. Allowed when the server runs over the stdio transport; over HTTP it requires the operator to set BOOKSTACK_UPLOAD_ROOT and the path must resolve inside it.', + 'Path to a file on the server to upload instead of inlining base64. Mutually exclusive with file and link. Requires the operator to set BOOKSTACK_UPLOAD_ROOT, under every transport, and the path must resolve inside it.', }, link: { type: 'string', @@ -233,7 +235,7 @@ export class AttachmentTools { ], usage_patterns: [ 'Use attachments for non-image files or external links that should be associated with a page', - 'Prefer file_path over base64 for large files, where the transport allows it', + 'Prefer file_path over base64 for large files, where the operator has set BOOKSTACK_UPLOAD_ROOT', ], related_tools: ['bookstack_pages_read'], error_codes: [ @@ -350,13 +352,15 @@ export class AttachmentTools { }, file: { type: 'string', + minLength: 1, description: 'New Base64 encoded file content (Replaces existing file), at most 50000 KB. Mutually exclusive with file_path and link.', }, file_path: { type: 'string', + minLength: 1, description: - 'Path to a file on the server to upload instead of inlining base64. Mutually exclusive with file and link. Allowed when the server runs over the stdio transport; over HTTP it requires the operator to set BOOKSTACK_UPLOAD_ROOT and the path must resolve inside it.', + 'Path to a file on the server to upload instead of inlining base64. Mutually exclusive with file and link. Requires the operator to set BOOKSTACK_UPLOAD_ROOT, under every transport, and the path must resolve inside it.', }, link: { type: 'string', diff --git a/src/tools/images.ts b/src/tools/images.ts index c897f9d..37e1467 100644 --- a/src/tools/images.ts +++ b/src/tools/images.ts @@ -163,13 +163,15 @@ export class ImageTools { }, image: { type: 'string', + minLength: 1, description: 'Base64 encoded image content. Supply this or file_path, not both. Must be a JPEG, PNG, GIF, WebP or AVIF, and at most 50000 KB.', }, file_path: { type: 'string', + minLength: 1, description: - 'Path to a file on the server to upload instead of inlining base64. Allowed when the server runs over the stdio transport; over HTTP it requires the operator to set BOOKSTACK_UPLOAD_ROOT and the path must resolve inside it.', + 'Path to a file on the server to upload instead of inlining base64. Requires the operator to set BOOKSTACK_UPLOAD_ROOT, under every transport, and the path must resolve inside it.', }, type: { type: 'string', @@ -227,7 +229,7 @@ export class ImageTools { ], usage_patterns: [ 'Upload images first, then use the returned URL to embed them in page HTML/Markdown', - 'Prefer file_path over base64 for large images, where the transport allows it', + 'Prefer file_path over base64 for large images, where the operator has set BOOKSTACK_UPLOAD_ROOT', 'Prefer short names. The uploaded filename is derived from `name`, and BookStack stores the gallery URL in a 191-character column, so a long name silently truncates the returned `url` mid-name (dropping the extension); that URL then serves the HTML app page rather than the image. The upload itself still succeeds and `path` keeps the full name. How long is too long depends on the length of the instance base URL.', ], related_tools: ['bookstack_pages_update'], @@ -341,13 +343,15 @@ export class ImageTools { }, image: { type: 'string', + minLength: 1, description: 'New Base64 encoded image content (Replaces existing image). Supply this or file_path, not both. Should be the same file type as the original image, and at most 50000 KB.', }, file_path: { type: 'string', + minLength: 1, description: - 'Path to a file on the server to upload instead of inlining base64. Allowed when the server runs over the stdio transport; over HTTP it requires the operator to set BOOKSTACK_UPLOAD_ROOT and the path must resolve inside it.', + 'Path to a file on the server to upload instead of inlining base64. Requires the operator to set BOOKSTACK_UPLOAD_ROOT, under every transport, and the path must resolve inside it.', }, }, // At most one content source: unlike create, sending neither is valid here - that diff --git a/src/tools/pages.ts b/src/tools/pages.ts index 89c248b..d5f1d3e 100644 --- a/src/tools/pages.ts +++ b/src/tools/pages.ts @@ -35,6 +35,11 @@ import type { /** The whole `bookstack_pages_update` request: the page to update, plus the changes. */ type UpdatePageRequest = UpdatePageParams & IdRequest; +/** Collapse whitespace runs, which BookStack may reflow when it stores a page. */ +function collapseWhitespace(value: string): string { + return value.replace(/\s+/g, ' ').trim(); +} + /** What a post-write read must prove without requiring byte-identical HTML. */ interface WriteVerification { mustContain: string[]; @@ -384,7 +389,7 @@ export class PageTools { minLength: 1, maxLength: 1000, description: - 'Literal text to search for in the STORED page source. Returns matching excerpts with surrounding context instead of the whole page. Regex syntax is treated literally, so searching the stored source (not the rendered HTML) makes a returned excerpt usable verbatim as an `old_string` anchor.', + "Literal text to search for in the STORED page source. Returns matching excerpts instead of the whole page; regex syntax is treated literally. Each match's `context` is an exact slice of the stored source with no ellipses (`context_truncated_start`/`context_truncated_end` say where the page continues), so it can be pasted verbatim as an `old_string` anchor.", }, case_sensitive: { type: 'boolean', @@ -450,7 +455,7 @@ export class PageTools { usage_patterns: [ 'Use this to get the "before" state of content when performing updates', 'Useful for answering questions based on specific documentation', - 'On a large page, run `grep` first and pass the returned excerpt to bookstack_pages_edit as `old_string` - that avoids sending the page through the model twice', + "On a large page, run `grep` first and pass a match's `context` (or a unique part of it) to bookstack_pages_edit as `old_string` - that avoids sending the page through the model twice", 'A call with none of the narrowing options behaves exactly as before and returns the whole page', ], related_tools: [ @@ -734,7 +739,7 @@ export class PageTools { }, ], usage_patterns: [ - 'Locate the text first: bookstack_pages_read with `grep` returns excerpts you can paste straight into old_string', + 'Locate the text first: each bookstack_pages_read `grep` match carries an exact `context` you can paste straight into old_string', 'Run with dry_run: true first on anything non-trivial - it costs no write and proves the anchors resolve', 'Pass expected_updated_at from the read that produced your anchors to catch a page that was already stale when this server read it; BookStack cannot make this a race-free lock', 'BookStack keeps a revision per write, so an applied edit can be rolled back in the UI', @@ -822,7 +827,6 @@ export class PageTools { // edit landed, including markup-only fragments omitted by text normalisation. const normalized = normalizeForComparison(edit.new_string, source.writeField); if (normalized.length === 0) { - const collapseWhitespace = (value: string) => value.replace(/\s+/g, ' ').trim(); return collapseWhitespace(source.source).includes( collapseWhitespace(edit.new_string) ); @@ -843,7 +847,7 @@ export class PageTools { return { name: 'bookstack_pages_append', description: - 'Add content to a page without resending the existing content. Appends to the end of the page by default, or to the end of a named section, or directly after a section heading with `position: "start"`.\n\nSection names come from `bookstack_pages_outline`; matching is case-insensitive and falls back to a substring match. The response never contains the page content.', + 'Add content to a page without resending the existing content. Appends to the end of the page by default, or to the end of a named section, or directly after a section heading with `position: "start"`.\n\nSection names come from `bookstack_pages_outline`. An exact heading match (trimmed, case-insensitive) wins; otherwise a substring is accepted only if exactly one heading contains it. A section runs to the next heading of the same or a higher level, so "end" lands after its subsections. The response reports the heading used as `section_matched` and never contains the page content.', category: 'pages', inputSchema: { type: 'object', @@ -871,7 +875,7 @@ export class PageTools { type: 'string', minLength: 1, description: - 'Heading text of the section to insert into. Omit to target the whole page. If no heading matches, the error lists the available section names.', + 'Heading text of the section to insert into. Omit to target the whole page. If no heading matches, the error lists the available section names; if several match, it lists those.', }, separator: { type: 'string', @@ -905,23 +909,24 @@ export class PageTools { section: 'Change log', content: '

2026-08-17: retention extended.

', }, - expected_output: 'Summary naming the section that was appended to', + expected_output: 'Summary whose section_matched names the heading that was appended to', use_case: 'Maintaining a log section in a long document', }, ], usage_patterns: [ 'Call bookstack_pages_outline first to get exact section names and see which editor the page uses', 'Match the page format: HTML for a wysiwyg page, Markdown for a markdown page - mixing them produces visible markup', - 'Use position: "start" to introduce a section, "end" to add to it', + 'Use position: "start" to introduce a section, "end" to add to it (after its subsections)', + 'Check section_matched in the response, dry_run included, to confirm which heading was used', 'This never rewrites existing content, so it is the safe choice for adding to a page you have not read', ], related_tools: ['bookstack_pages_outline', 'bookstack_pages_edit', 'bookstack_pages_read'], error_codes: [ { code: 'INVALID_PARAMS', - description: 'The named section does not exist', + description: 'The named section does not exist, or matches more than one heading', recovery_suggestion: - 'The error details list the available section names; use one of those or omit `section` to append to the page', + 'The error details list the available or the matching section names; pass the full text of one of those, or omit `section` to append to the page', }, { code: 'INVALID_REQUEST', @@ -942,7 +947,7 @@ export class PageTools { this.assertNotStale(page, options.expected_updated_at); const source = selectSource(page); - const result = insertContent(source.source, options.content, { + const { result, heading } = insertContent(source.source, options.content, { position: options.position, ...(options.section !== undefined ? { section: options.section } : {}), ...(options.separator !== undefined ? { separator: options.separator } : {}), @@ -955,6 +960,7 @@ export class PageTools { field: source.writeField, position: options.position, section: options.section ?? null, + section_matched: heading?.text ?? null, chars_before: source.source.length, chars_after: result.length, delta: result.length - source.source.length, @@ -1013,7 +1019,7 @@ export class PageTools { ], usage_patterns: [ 'Run this before bookstack_pages_append to get exact section names', - 'The `length` of a heading is the size of its section, which shows where the content actually sits', + 'The `length` of a heading is the size of its section, subsections included, which shows where the content actually sits', 'Check `editor` before writing: it decides whether content must be HTML or Markdown', 'A page with no headings returns an empty array - that is not an error', ], @@ -1089,10 +1095,13 @@ export class PageTools { * * The re-read is not paranoia about the network - it is about BookStack rewriting what it * stores. On save it re-generates heading anchors and injects `id` attributes, so the bytes - * that come back are not the bytes that were sent. Verification therefore compares - * NORMALISED text (see containsNormalized), and a fragment that cannot be found is reported - * as `verified: false` rather than thrown: the write did happen, and the caller needs to - * know both facts. + * that come back are not always the bytes that were sent. Each fragment is therefore looked + * for in the raw stored source first and in NORMALISED text second (see + * normalizeForComparison). A fragment that cannot be found is reported as `verified: false` + * rather than thrown: the write did happen, and the caller needs to know both facts. + * + * Only a failed PUT throws. If the re-read fails after a successful PUT, the result is + * `verified: null`: throwing there would invite a retry that applies the change twice. */ private async writeAndVerify( page: PageWithContent, @@ -1100,29 +1109,48 @@ export class PageTools { result: string, verification: WriteVerification ): Promise> { - await this.client.updatePage(page.id, { [source.writeField]: result }); + const put = await this.client.updatePage(page.id, { [source.writeField]: result }); + + let written: PageWithContent; + try { + written = await this.client.getPage(page.id); + } catch (error) { + this.logger.warn('Page written but not re-read for verification', { + page_id: page.id, + err: error, + }); + return { + written: true, + verified: null, + note: 'The page was written but could not be re-read to verify it. Do not retry the write; read the page to check it.', + updated_at: put.updated_at, + revision_count: put.revision_count, + }; + } - const written = await this.client.getPage(page.id); const writtenSource = selectSource(written); + const field = writtenSource.writeField; + const storedRaw = collapseWhitespace(writtenSource.source); + const storedText = normalizeForComparison(writtenSource.source, field); + const sentRaw = collapseWhitespace(result); + const sentText = normalizeForComparison(result, field); + // A markup-only fragment (`
`) has empty text, so only the raw check can see it. + const forms = (fragment: string) => ({ + raw: collapseWhitespace(fragment), + text: normalizeForComparison(fragment, field), + }); + const missing = verification.mustContain.filter((fragment) => { - const normalized = normalizeForComparison(fragment, writtenSource.writeField); - if (normalized.length === 0) { - // HTML-to-text intentionally removes markup-only fragments (`
`, ``, etc.). - // They still need a structural post-write check, otherwise a stored fragment would be - // reported as missing forever; collapse formatting whitespace but require the literal - // markup to remain present. - const collapseWhitespace = (value: string) => value.replace(/\s+/g, ' ').trim(); - return !collapseWhitespace(writtenSource.source).includes(collapseWhitespace(fragment)); - } - return !containsNormalized(writtenSource.source, fragment, writtenSource.writeField); + const { raw, text } = forms(fragment); + return !storedRaw.includes(raw) && !(text.length > 0 && storedText.includes(text)); }); - // An empty replacement deletes its old anchor. An empty normalised anchor cannot be - // meaningfully searched for, so treat it as unverified rather than claiming success. + // Survived: stored in a form the sent result lacks. Whitespace-only anchors stay unverified. const stillPresent = verification.mustNotContain.filter((fragment) => { - const normalized = normalizeForComparison(fragment, writtenSource.writeField); + const { raw, text } = forms(fragment); return ( - normalized.length === 0 || - containsNormalized(writtenSource.source, fragment, writtenSource.writeField) + raw.length === 0 || + (storedRaw.includes(raw) && !sentRaw.includes(raw)) || + (text.length > 0 && storedText.includes(text) && !sentText.includes(text)) ); }); const unverifiedCount = missing.length + stillPresent.length; diff --git a/src/tools/server-info.ts b/src/tools/server-info.ts index 15136bd..267a133 100644 --- a/src/tools/server-info.ts +++ b/src/tools/server-info.ts @@ -162,7 +162,10 @@ export class ServerInfoTools { }, authentication: { required: true, - methods: ['API Token'], + methods: [ + 'BookStack API token', + 'OAuth access token (HTTP transport with MCP_AUTH_MODE=oauth)', + ], }, rate_limiting: { enabled: !!config.rateLimit, @@ -774,7 +777,7 @@ export class ServerInfoTools { tool_or_resource: 'bookstack_pages_read', parameters: { id: 12, grep: 'retention period', context: 300 }, description: - 'Returns only matching excerpts from the STORED source. Copy one verbatim as old_string - that is what makes the anchor match', + "Returns only matching excerpts from the STORED source. Copy a match's exact `context` verbatim as old_string - that is what makes the anchor match", }, { step: 3, diff --git a/src/utils/errors.ts b/src/utils/errors.ts index 419229d..d03a601 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -11,10 +11,58 @@ interface ErrorLike { isAxiosError?: boolean; name?: string; message?: string; - stack?: string; response?: { status?: number; headers?: unknown }; } +/** Axios timeout codes: ECONNABORTED by default, ETIMEDOUT with `clarifyTimeoutError`. */ +const TIMEOUT_ERROR_CODES: readonly string[] = ['ECONNABORTED', 'ETIMEDOUT']; + +/** Codes that mean the connection failed before any response arrived. */ +const CONNECTION_ERROR_CODES: readonly string[] = ['ECONNRESET', 'ECONNREFUSED', 'EPIPE']; + +/** Codes raised without a response that are not upstream failures. */ +const NON_TRANSPORT_ERROR_CODES: readonly string[] = [ + 'ERR_CANCELED', + 'ERR_BAD_OPTION', + 'ERR_BAD_OPTION_VALUE', + 'ERR_INVALID_URL', + 'ERR_NOT_SUPPORT', +]; + +/** Caller-facing message for a connection-level failure; names no host. */ +export const NETWORK_ERROR_MESSAGE = 'BookStack could not be reached'; + +/** Caller-facing message for a request that ran out of time; names no host. */ +function timeoutMessage(timeoutMs: unknown): string { + return typeof timeoutMs === 'number' && timeoutMs > 0 + ? `BookStack did not respond within ${timeoutMs} ms` + : 'BookStack did not respond in time'; +} + +/** Classify an axios failure with no response: `timeout`, `network` (sent but unanswered), or neither. */ +function transportFailure(error: AxiosError): 'timeout' | 'network' | undefined { + if (error.response !== undefined) { + return undefined; + } + const code = typeof error.code === 'string' ? error.code : ''; + if (TIMEOUT_ERROR_CODES.includes(code)) { + return 'timeout'; + } + if (NON_TRANSPORT_ERROR_CODES.includes(code)) { + return undefined; + } + if ( + CONNECTION_ERROR_CODES.includes(code) || + code.startsWith('ERR_SOCKET_') || + code === 'ERR_NETWORK' || + /socket hang up/i.test(error.message ?? '') || + error.request !== undefined + ) { + return 'network'; + } + return undefined; +} + /** * HTTP statuses that represent a *transient* upstream condition, i.e. one where an * identical request has a genuine chance of succeeding later. @@ -39,6 +87,8 @@ export interface RetryInfo { method?: string; retryAfterMs?: number; rateLimitRemaining?: number; + /** The connection failed before any response, so the request may never have arrived. */ + connectionFailed?: boolean; } /** Narrow an unknown value to an indexable object without asserting `any`. */ @@ -46,6 +96,28 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null; } +/** A failed tool call as MCP returns it: a result the model can read, not a protocol error. */ +export interface ToolErrorResult { + [key: string]: unknown; + isError: true; + content: Array<{ type: 'text'; text: string }>; +} + +/** The `MCP error : ` prefix McpError puts in front of its message. */ +const MCP_ERROR_PREFIX = /^MCP error -?\d+: /; + +/** McpError data as indented JSON, without stack frames or the upstream request path. */ +function toolErrorHints(data: unknown): string | undefined { + if (!isRecord(data)) { + return undefined; + } + const { url: _requestPath, ...hints } = data; + if (Object.keys(hints).length === 0) { + return undefined; + } + return JSON.stringify(hints, (key, value) => (key === 'stack' ? undefined : value), 2); +} + /** * Read a single header case-insensitively from an unknown header bag. * @@ -155,10 +227,16 @@ export class ErrorHandler { */ handleAxiosError(error: AxiosError): McpError { const status = error.response?.status; - const mapping = this.errorMappings[status as keyof typeof this.errorMappings] || { - type: 'unknown_error', - message: 'Unknown error occurred', - }; + const failure = transportFailure(error); + const mapping = + failure === 'timeout' + ? { type: 'timeout_error', message: timeoutMessage(error.config?.timeout) } + : failure === 'network' + ? { type: 'network_error', message: NETWORK_ERROR_MESSAGE } + : this.errorMappings[status as keyof typeof this.errorMappings] || { + type: 'unknown_error', + message: 'Unknown error occurred', + }; // The client's response interceptor converts every AxiosError into an McpError before // request() ever sees it, so the retry hints must be carried across on the McpError - @@ -237,13 +315,12 @@ export class ErrorHandler { }); } - // Handle generic errors + // Handle generic errors. No `stack` in `data`: it goes to the caller, the frames go to the log. const mcpError = new McpError( ErrorCode.InternalError, err.message || 'An unexpected error occurred', { type: 'internal_error', - stack: err.stack, } ); @@ -259,6 +336,17 @@ export class ErrorHandler { return mcpError; } + /** A tool failure as an `isError` result: the message, then the McpError data as JSON. */ + toToolErrorResult(error: unknown): ToolErrorResult { + const handled = this.handleError(error); + const message = handled.message.replace(MCP_ERROR_PREFIX, ''); + const hints = toolErrorHints(handled.data); + return { + isError: true, + content: [{ type: 'text', text: hints ? `${message}\n\n${hints}` : message }], + }; + } + /** * Map HTTP status codes to MCP error codes */ @@ -328,7 +416,7 @@ export class ErrorHandler { } const info: RetryInfo = {}; - const { status, method, retryAfterMs, rateLimitRemaining } = error.data; + const { status, method, retryAfterMs, rateLimitRemaining, type } = error.data; if (typeof status === 'number') { info.status = status; @@ -342,6 +430,9 @@ export class ErrorHandler { if (typeof rateLimitRemaining === 'number') { info.rateLimitRemaining = rateLimitRemaining; } + if (type === 'network_error') { + info.connectionFailed = true; + } return info; } @@ -362,6 +453,9 @@ export class ErrorHandler { if (method !== undefined) { info.method = method; } + if (transportFailure(axiosError) === 'network') { + info.connectionFailed = true; + } return info; } @@ -372,30 +466,14 @@ export class ErrorHandler { * This answers a narrow question: is the *upstream condition* transient? It says * nothing about whether replaying the request is safe - a 500 on a POST is transient * but may have partially applied. That call belongs to the caller, which knows the - * verb. + * verb. A dropped or refused connection is transient; a timeout is not retried. */ isRetryable(error: unknown): boolean { - const status = this.getRetryInfo(error).status; - return status !== undefined && RETRYABLE_STATUS_CODES.includes(status); - } - - /** - * Create a user-friendly error message - */ - getUserFriendlyMessage(error: unknown): string { - if (error instanceof McpError) { - return error.message; + const info = this.getRetryInfo(error); + if (info.connectionFailed) { + return true; } - - const err = error as ErrorLike; - - if (err.isAxiosError) { - const status = err.response?.status; - const mapping = this.errorMappings[status as keyof typeof this.errorMappings]; - return mapping?.message || 'An error occurred while communicating with BookStack'; - } - - return 'An unexpected error occurred'; + return info.status !== undefined && RETRYABLE_STATUS_CODES.includes(info.status); } } diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 5ddcb73..0a30c9f 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -762,11 +762,6 @@ export function redactLogMeta(meta: unknown): unknown { return redactValue(meta, 0, new WeakSet(), 'withhold'); } -/** As `redactLogMeta`, for bound child metadata, whose record shape must be preserved. */ -export function redactLogRecord(meta: Record): Record { - return redactRecord(meta, 0, new WeakSet()); -} - /** Build the Winston format matching `format`. */ function buildFormat(format: LogFormat): winston.Logform.Format { if (format === 'json') { @@ -873,13 +868,6 @@ export class Logger { error(message: string, meta?: unknown): void { this.write('error', message, meta); } - - /** Bound metadata is redacted too: it is rendered onto every line the child writes. */ - child(meta: Record): Logger { - const childLogger = new Logger(this.options); - childLogger.logger = this.logger.child(redactLogRecord(meta)); - return childLogger; - } } export default Logger; diff --git a/src/utils/page-content.ts b/src/utils/page-content.ts index 40a229b..c72b866 100644 --- a/src/utils/page-content.ts +++ b/src/utils/page-content.ts @@ -42,7 +42,11 @@ export interface Heading { export interface GrepMatch { offset: number; match: string; + /** Exact stored source around the match, usable verbatim as an `old_string` anchor. */ context: string; + /** Whether the page continues before / after `context`. */ + context_truncated_start: boolean; + context_truncated_end: boolean; } /** @@ -78,6 +82,8 @@ export class PageStaleError extends Error { const CONTEXT_RADIUS = 120; const MAX_DIAGNOSTIC_LENGTH = 300; +/** Longest anchor, in words, the whitespace-tolerant search runs for: its cost is page words x anchor words. */ +const MAX_TOLERANT_MATCH_WORDS = 256; /** Escape literal text for a regular expression that must retain literal semantics. */ function escapeRegExp(value: string): string { @@ -144,9 +150,12 @@ function findWhitespaceTolerantMatch(source: string, needle: string): string | n return null; } - const pattern = trimmed.split(/\s+/).map(escapeRegExp).join('\\s+'); + const words = trimmed.split(/\s+/); + if (words.length > MAX_TOLERANT_MATCH_WORDS) { + return null; + } - const match = new RegExp(pattern).exec(source); + const match = new RegExp(words.map(escapeRegExp).join('\\s+')).exec(source); if (!match) { return null; } @@ -242,10 +251,12 @@ export function applyEdits( /** * Strip tags and decode the handful of entities BookStack emits in headings. + * + * A tag is `<[^<>]*>`, not `<[^>]*>`: the latter is quadratic on a run of unmatched '<'. */ function htmlToText(html: string): string { return html - .replace(/<[^>]*>/g, '') + .replace(/<[^<>]*>/g, '') .replace(/ /g, ' ') .replace(/</g, '<') .replace(/>/g, '>') @@ -282,43 +293,200 @@ export function containsNormalized( return normalizeForComparison(haystack, writeField).includes(normalizedNeedle); } +/** What `^`, `$` and `.` treat as a line break in a multiline JavaScript regex. */ +const LINE_TERMINATOR = /[\n\r\u2028\u2029]/; + +function isBlank(char: string | undefined): boolean { + return char === ' ' || char === '\t'; +} + +/** Read one line as a markdown heading, in linear time; the equivalent regex backtracked quadratically. */ +function markdownHeading(line: string): { level: number; text: string } | undefined { + let level = 0; + while (line[level] === '#') { + level += 1; + } + if (level === 0 || level > 6) { + return undefined; + } + + let start = level; + while (isBlank(line[start])) { + start += 1; + } + if (start === level) { + return undefined; + } + if (start === line.length) { + // Blanks only: the lazy title takes the last blank, so a second one is needed. + return start - level > 1 ? { level, text: '' } : undefined; + } + + // Drop closing hashes, then the blanks before them, keeping at least one character. + let end = line.length; + while (end > start && line[end - 1] === '#') { + end -= 1; + } + while (end > start && isBlank(line[end - 1])) { + end -= 1; + } + return { level, text: line.slice(start, Math.max(end, start + 1)).trim() }; +} + +interface CodeFence { + char: string; + length: number; + rest: string; +} + +/** Read one line as a code fence: up to three spaces, then three or more ` or ~. */ +function codeFence(line: string): CodeFence | undefined { + let indent = 0; + while (indent < 4 && line[indent] === ' ') { + indent += 1; + } + const char = line[indent]; + if (indent > 3 || (char !== '`' && char !== '~')) { + return undefined; + } + + let end = indent; + while (line[end] === char) { + end += 1; + } + return end - indent >= 3 ? { char, length: end - indent, rest: line.slice(end) } : undefined; +} + +function markdownHeadings(source: string): Omit[] { + const headings: Omit[] = []; + let openFence: CodeFence | undefined; + let offset = 0; + for (const line of source.split(LINE_TERMINATOR)) { + const fence = codeFence(line); + if (openFence) { + // Closed only by the same character, at least as long, with nothing after it. + const closes = + fence?.char === openFence.char && + fence.length >= openFence.length && + fence.rest.trim() === ''; + if (closes) { + openFence = undefined; + } + } else if (fence && !(fence.char === '`' && fence.rest.includes('`'))) { + openFence = fence; + } else { + const heading = markdownHeading(line); + if (heading) { + headings.push({ ...heading, offset }); + } + } + offset += line.length + 1; + } + return headings; +} + +/** First match of global `pattern` at or after a never-decreasing `from`, so all calls scan `text` once. */ +function forwardSearch(text: string, pattern: RegExp): (from: number) => number { + let found: number | undefined; + return (from) => { + if (found !== undefined && (found === -1 || found >= from)) { + return found; + } + pattern.lastIndex = from; + found = pattern.exec(text)?.index ?? -1; + return found; + }; +} + +/** Every html heading in one forward pass; the equivalent regex was quadratic on unterminated tags. */ +function htmlHeadings(source: string): Omit[] { + const headings: Omit[] = []; + const opening = //g); + const nextClosing = [1, 2, 3, 4, 5, 6].map((level) => + forwardSearch(source, new RegExp(``, 'gi')) + ); + + for (let open = opening.exec(source); open; open = opening.exec(source)) { + const tagEnd = nextTagEnd(opening.lastIndex); + if (tagEnd === -1) { + // No later ``.length; + } + + return headings; +} + /** * Map the heading structure of a page, so a large page can be navigated * without loading its content. */ export function buildOutline(source: string, writeField: PageWriteField): Heading[] { - const headings: Omit[] = []; - - // `matchAll` rather than a `while ((m = re.exec(s)))` loop: the assignment-in-condition - // form is what the linter flags, and matchAll also advances past a zero-length match on its - // own, which that loop has to remember to do by hand. - if (writeField === 'markdown') { - for (const match of source.matchAll(/^(#{1,6})[ \t]+(.+?)[ \t]*#*$/gm)) { - headings.push({ - level: match[1].length, - text: match[2].trim(), - offset: match.index, - }); - } - } else { - for (const match of source.matchAll(/]*>([\s\S]*?)<\/h\1>/gi)) { - headings.push({ - level: Number(match[1]), - text: htmlToText(match[2]), - offset: match.index, - }); + const headings: Heading[] = ( + writeField === 'markdown' ? markdownHeadings(source) : htmlHeadings(source) + ).map((heading) => ({ ...heading, length: source.length - heading.offset })); + + // A section runs to the next heading of the same or a higher level, subsections included. + const open: Heading[] = []; + for (const heading of headings) { + while (open.length > 0 && (open.at(-1) as Heading).level >= heading.level) { + const closed = open.pop() as Heading; + closed.length = heading.offset - closed.offset; } + open.push(heading); } + return headings; +} - return headings.map((heading, i) => ({ - ...heading, - length: (i + 1 < headings.length ? headings[i + 1].offset : source.length) - heading.offset, - })); +/** The heading a section name addresses: exact (trimmed, case-insensitive) first, else the one containing it. */ +export function findSection(source: string, section: string, writeField: PageWriteField): Heading { + const headings = buildOutline(source, writeField); + const wanted = section.trim().toLowerCase(); + const exact = headings.filter((h) => h.text.trim().toLowerCase() === wanted); + const matches = + exact.length > 0 ? exact : headings.filter((h) => h.text.toLowerCase().includes(wanted)); + + if (matches.length === 0) { + throw new PageContentError(`Section not found: ${section}`, { + available_sections: headings.map((h) => h.text), + hint: 'Use bookstack_pages_outline to list the exact heading texts.', + }); + } + if (matches.length > 1) { + throw new PageContentError(`Section is ambiguous: ${matches.length} headings match`, { + matching_sections: matches.map((h) => h.text), + hint: 'Pass the full text of exactly one heading. Headings with identical text cannot be told apart; anchor a bookstack_pages_edit on the text instead.', + }); + } + return matches[0]; +} + +/** Whether cutting `text` at `index` would split a surrogate pair. */ +function splitsSurrogatePair(text: string, index: number): boolean { + const high = text.charCodeAt(index - 1); + const low = text.charCodeAt(index); + return high >= 0xd800 && high <= 0xdbff && low >= 0xdc00 && low <= 0xdfff; } /** * Search inside page content and return exact matches with surrounding * context, suitable for building an `old_string` anchor. + * + * `context` is an exact slice of the source with no ellipses; the flags say where it was cut. */ export function grepContent( source: string, @@ -344,10 +512,22 @@ export function grepContent( for (const match of source.matchAll(regex)) { total += 1; if (matches.length < maxMatches) { + const matchEnd = match.index + match[0].length; + let start = Math.max(0, match.index - contextChars); + let end = Math.min(source.length, matchEnd + contextChars); + // Half a surrogate pair cannot be sent back as an anchor. + if (start < match.index && splitsSurrogatePair(source, start)) { + start += 1; + } + if (end > matchEnd && splitsSurrogatePair(source, end)) { + end -= 1; + } matches.push({ offset: match.index, match: match[0], - context: contextAround(source, match.index, contextChars), + context: source.slice(start, end), + context_truncated_start: start > 0, + context_truncated_end: end < source.length, }); } } @@ -357,6 +537,8 @@ export function grepContent( /** * Insert content at the start or end of a page, or of a named section. + * + * Returns the new source and, when a section was named, the heading it resolved to. */ export function insertContent( source: string, @@ -367,26 +549,16 @@ export function insertContent( separator?: string; writeField: PageWriteField; } -): string { +): { result: string; heading: Heading | undefined } { const { position = 'end', section, writeField } = options; const separator = options.separator ?? (writeField === 'markdown' ? '\n\n' : '\n'); let rangeStart = 0; let rangeEnd = source.length; + let found: Heading | undefined; if (section) { - const headings = buildOutline(source, writeField); - const wanted = section.trim().toLowerCase(); - const found = - headings.find((h) => h.text.toLowerCase() === wanted) ?? - headings.find((h) => h.text.toLowerCase().includes(wanted)); - - if (!found) { - throw new PageContentError(`Section not found: ${section}`, { - available_sections: headings.map((h) => h.text), - hint: 'Use bookstack_pages_outline to list the exact heading texts.', - }); - } + found = findSection(source, section, writeField); // "start" means directly after the heading itself, not before it const headingBlock = source.slice(found.offset, found.offset + found.length); @@ -414,9 +586,11 @@ export function insertContent( } } - return insertAt > 0 - ? `${source.slice(0, insertAt)}${separator}${content}${source.slice(insertAt)}` - : `${content}${separator}${source.slice(insertAt)}`; + const result = + insertAt > 0 + ? `${source.slice(0, insertAt)}${separator}${content}${source.slice(insertAt)}` + : `${content}${separator}${source.slice(insertAt)}`; + return { result, heading: found }; } /** diff --git a/src/validation/validator.ts b/src/validation/validator.ts index 2d3d6e3..305b5d6 100644 --- a/src/validation/validator.ts +++ b/src/validation/validator.ts @@ -738,9 +738,9 @@ const ValidationSchemas = { .strictObject({ uploaded_to: entityId, name: z.string().min(1).max(255), - file: z.string().optional(), // base64 encoded + file: z.string().min(1).optional(), // base64 encoded file_path: z.string().min(1).optional(), // server-local path - link: z.string().url().optional(), + link: z.string().url().max(2000).optional(), }) .superRefine((data, ctx) => { const { sources, conflictMessage } = providedAttachmentSources(data); @@ -766,9 +766,9 @@ const ValidationSchemas = { id: entityId, uploaded_to: entityId.optional(), name: z.string().min(1).max(255).optional(), - file: z.string().optional(), // base64 encoded + file: z.string().min(1).optional(), // base64 encoded file_path: z.string().min(1).optional(), // server-local path - link: z.string().url().optional(), + link: z.string().url().max(2000).optional(), }) .superRefine((data, ctx) => { const { sources, conflictMessage } = providedAttachmentSources(data); @@ -807,7 +807,7 @@ const ValidationSchemas = { imageCreate: z .strictObject({ name: z.string().min(1).max(180).optional(), - image: z.string().optional(), // base64 encoded + image: z.string().min(1).optional(), // base64 encoded file_path: z.string().min(1).optional(), // server-local path type: z.enum(['gallery', 'drawio']).default('gallery'), uploaded_to: entityId, // required by BookStack @@ -823,7 +823,7 @@ const ValidationSchemas = { .strictObject({ id: entityId, name: z.string().min(1).max(180).optional(), - image: z.string().optional(), // base64 encoded + image: z.string().min(1).optional(), // base64 encoded file_path: z.string().min(1).optional(), // server-local path }) .refine((data) => !(data.image && data.file_path), { @@ -1110,30 +1110,6 @@ export class ValidationHandler { return params as T; } } - - /** - * Validate required fields are present - */ - validateRequired(params: Record, requiredFields: string[]): void { - if (!this.enabled) { - return; - } - - const missing = requiredFields.filter( - (field) => params[field] === undefined || params[field] === null - ); - - if (missing.length > 0) { - throw new Error(`Missing required fields: ${missing.join(', ')}`); - } - } - - /** - * Get available schemas - */ - getAvailableSchemas(): string[] { - return Object.keys(ValidationSchemas); - } } export default ValidationHandler; diff --git a/tests/integration/attachments.test.ts b/tests/integration/attachments.test.ts index de3657e..d2e3e4f 100644 --- a/tests/integration/attachments.test.ts +++ b/tests/integration/attachments.test.ts @@ -250,7 +250,6 @@ describe.skipIf(!runIntegration)('BookStack attachment tools (live)', () => { // which is what lets the rejection tests assert on a real failure. validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; const client = new BookStackClient(config, logger, new ErrorHandler(logger)); attachmentTools = new AttachmentTools( @@ -552,10 +551,10 @@ describe.skipIf(!runIntegration)('BookStack attachment tools (live)', () => { /** file_path over the multipart-update path (`POST` + `_method=PUT`). */ it( - 'replaces content from a file_path under the stdio transport', + 'replaces content from a file_path inside BOOKSTACK_UPLOAD_ROOT under the stdio transport', async () => { setGuardEnv('MCP_TRANSPORT', 'stdio'); - setGuardEnv('BOOKSTACK_UPLOAD_ROOT', undefined); + setGuardEnv('BOOKSTACK_UPLOAD_ROOT', uploadRoot); const name = `${uniqueName('itest-att-stdio')}.txt`; const original = payloadBytes('stdio-v1'); @@ -577,28 +576,31 @@ describe.skipIf(!runIntegration)('BookStack attachment tools (live)', () => { ); it( - 'refuses file_path over a remote-capable transport without BOOKSTACK_UPLOAD_ROOT', + 'refuses file_path without BOOKSTACK_UPLOAD_ROOT under stdio and http alike', async () => { - setGuardEnv('MCP_TRANSPORT', 'http'); setGuardEnv('BOOKSTACK_UPLOAD_ROOT', undefined); - const name = `${uniqueName('itest-att-refused')}.txt`; - const filePath = join(uploadRoot, name); - await writeFile(filePath, payloadBytes('refused')); - - await expect( - runTool('bookstack_attachments_create', { - uploaded_to: pageId, - name, - file_path: filePath, - }) - ).rejects.toThrow(/'file_path' is refused under the 'http' transport/); - - // Refusal is real, not cosmetic: nothing reached the page. - const listed = (await runTool('bookstack_attachments_list', { - filter: { uploaded_to: pageId, name }, - })) as ListResponse; - expect(listed.data).toEqual([]); + for (const transport of ['stdio', 'http']) { + setGuardEnv('MCP_TRANSPORT', transport); + + const name = `${uniqueName('itest-att-refused')}.txt`; + const filePath = join(uploadRoot, name); + await writeFile(filePath, payloadBytes('refused')); + + await expect( + runTool('bookstack_attachments_create', { + uploaded_to: pageId, + name, + file_path: filePath, + }) + ).rejects.toThrow(/BOOKSTACK_UPLOAD_ROOT is not set/); + + // Refusal is real, not cosmetic: nothing reached the page. + const listed = (await runTool('bookstack_attachments_list', { + filter: { uploaded_to: pageId, name }, + })) as ListResponse; + expect(listed.data).toEqual([]); + } }, TEST_TIMEOUT_MS ); @@ -624,7 +626,7 @@ describe.skipIf(!runIntegration)('BookStack attachment tools (live)', () => { name, file_path: traversal, }) - ).rejects.toThrow(/which is outside BOOKSTACK_UPLOAD_ROOT/); + ).rejects.toThrow(/must name a readable file inside BOOKSTACK_UPLOAD_ROOT/); const listed = (await runTool('bookstack_attachments_list', { filter: { uploaded_to: pageId, name }, @@ -713,8 +715,9 @@ describe.skipIf(!runIntegration)('BookStack attachment tools (live)', () => { it( `rejects create with ${sources.join(' + ')}, naming what collided`, async () => { - // stdio, so file_path is an allowed source and cannot be what fails. + // file_path sits inside BOOKSTACK_UPLOAD_ROOT, so it cannot be what fails. setGuardEnv('MCP_TRANSPORT', 'stdio'); + setGuardEnv('BOOKSTACK_UPLOAD_ROOT', uploadRoot); const name = `${uniqueName('itest-att-excl-create')}.txt`; const params = await sourceParams(sources, 'excl-create'); @@ -763,6 +766,7 @@ describe.skipIf(!runIntegration)('BookStack attachment tools (live)', () => { `rejects update with ${sources.join(' + ')}, leaving the stored file intact`, async () => { setGuardEnv('MCP_TRANSPORT', 'stdio'); + setGuardEnv('BOOKSTACK_UPLOAD_ROOT', uploadRoot); const name = `${uniqueName('itest-att-excl-update')}.txt`; const original = payloadBytes('excl-update-original'); diff --git a/tests/integration/audit.test.ts b/tests/integration/audit.test.ts index c115dca..acbdd1a 100644 --- a/tests/integration/audit.test.ts +++ b/tests/integration/audit.test.ts @@ -160,7 +160,6 @@ describe.skipIf(!runIntegration)('audit-log tool (live BookStack)', () => { rateLimit: { requestsPerMinute: 60, burstLimit: 10 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/books.test.ts b/tests/integration/books.test.ts index 1c3adb1..2e78546 100644 --- a/tests/integration/books.test.ts +++ b/tests/integration/books.test.ts @@ -305,7 +305,6 @@ describe.skipIf(!runIntegration)('bookstack_books_* tools (live BookStack)', () rateLimit: { requestsPerMinute: 120, burstLimit: 20 }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/chapters.test.ts b/tests/integration/chapters.test.ts index 64ee46f..260c310 100644 --- a/tests/integration/chapters.test.ts +++ b/tests/integration/chapters.test.ts @@ -296,7 +296,6 @@ describe.skipIf(!runIntegration)('bookstack_chapters_* tools (live BookStack)', rateLimit: { requestsPerMinute: 120, burstLimit: 20 }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/images.test.ts b/tests/integration/images.test.ts index 588298e..bbfa40f 100644 --- a/tests/integration/images.test.ts +++ b/tests/integration/images.test.ts @@ -261,7 +261,6 @@ describe.skipIf(!runIntegration)('BookStack image tools (live)', () => { // which is what lets the rejection tests assert on a real failure. validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; const client = new BookStackClient(config, logger, new ErrorHandler(logger)); imageTools = new ImageTools( @@ -575,7 +574,7 @@ describe.skipIf(!runIntegration)('BookStack image tools (live)', () => { 'creates an image from file_path with no name, taking the file basename', async () => { setGuardEnv('MCP_TRANSPORT', 'stdio'); - setGuardEnv('BOOKSTACK_UPLOAD_ROOT', undefined); + setGuardEnv('BOOKSTACK_UPLOAD_ROOT', uploadRoot); const basename = `${uniqueName('itest-img-unnamed-file')}.png`; const filePath = join(uploadRoot, basename); @@ -737,10 +736,10 @@ describe.skipIf(!runIntegration)('BookStack image tools (live)', () => { describe('file_path uploads', () => { it( - 'uploads a file from disk under the stdio transport', + 'uploads a file inside BOOKSTACK_UPLOAD_ROOT under the stdio transport', async () => { setGuardEnv('MCP_TRANSPORT', 'stdio'); - setGuardEnv('BOOKSTACK_UPLOAD_ROOT', undefined); + setGuardEnv('BOOKSTACK_UPLOAD_ROOT', uploadRoot); const name = `${uniqueName('itest-img-stdio')}.png`; const filePath = join(uploadRoot, name); @@ -763,24 +762,27 @@ describe.skipIf(!runIntegration)('BookStack image tools (live)', () => { ); it( - 'refuses file_path over a remote-capable transport without BOOKSTACK_UPLOAD_ROOT', + 'refuses file_path without BOOKSTACK_UPLOAD_ROOT under stdio and http alike', async () => { - setGuardEnv('MCP_TRANSPORT', 'http'); setGuardEnv('BOOKSTACK_UPLOAD_ROOT', undefined); - const name = `${uniqueName('itest-img-refused')}.png`; - const filePath = join(uploadRoot, name); - await writeFile(filePath, PNG_RED); + for (const transport of ['stdio', 'http']) { + setGuardEnv('MCP_TRANSPORT', transport); - await expect( - runTool('bookstack_images_create', { name, file_path: filePath, uploaded_to: pageId }) - ).rejects.toThrow(/'file_path' is refused under the 'http' transport/); + const name = `${uniqueName('itest-img-refused')}.png`; + const filePath = join(uploadRoot, name); + await writeFile(filePath, PNG_RED); - // Refusal is real, not cosmetic: nothing reached the gallery. - const listed = (await runTool('bookstack_images_list', { - filter: { name }, - })) as ListResponse; - expect(listed.data).toEqual([]); + await expect( + runTool('bookstack_images_create', { name, file_path: filePath, uploaded_to: pageId }) + ).rejects.toThrow(/BOOKSTACK_UPLOAD_ROOT is not set/); + + // Refusal is real, not cosmetic: nothing reached the gallery. + const listed = (await runTool('bookstack_images_list', { + filter: { name }, + })) as ListResponse; + expect(listed.data).toEqual([]); + } }, TEST_TIMEOUT_MS ); @@ -801,7 +803,7 @@ describe.skipIf(!runIntegration)('BookStack image tools (live)', () => { await expect( runTool('bookstack_images_create', { name, file_path: traversal, uploaded_to: pageId }) - ).rejects.toThrow(/which is outside BOOKSTACK_UPLOAD_ROOT/); + ).rejects.toThrow(/must name a readable file inside BOOKSTACK_UPLOAD_ROOT/); const listed = (await runTool('bookstack_images_list', { filter: { name }, diff --git a/tests/integration/pages.test.ts b/tests/integration/pages.test.ts index e622ac4..6746f81 100644 --- a/tests/integration/pages.test.ts +++ b/tests/integration/pages.test.ts @@ -289,7 +289,6 @@ describe.skipIf(!runIntegration)('bookstack_pages_* tools (live BookStack)', () rateLimit: { requestsPerMinute: 120, burstLimit: 20 }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/permissions.test.ts b/tests/integration/permissions.test.ts index cda580c..8d2e754 100644 --- a/tests/integration/permissions.test.ts +++ b/tests/integration/permissions.test.ts @@ -173,7 +173,6 @@ describe.skipIf(!runIntegration)('content-permission tools (live BookStack)', () rateLimit: { requestsPerMinute: 60, burstLimit: 10 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/recyclebin.test.ts b/tests/integration/recyclebin.test.ts index ad7187e..a6c5243 100644 --- a/tests/integration/recyclebin.test.ts +++ b/tests/integration/recyclebin.test.ts @@ -176,7 +176,6 @@ describe.skipIf(!runIntegration)('recycle-bin tools (live BookStack)', () => { rateLimit: { requestsPerMinute: 60, burstLimit: 10 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/resources.test.ts b/tests/integration/resources.test.ts index 770020f..d1f1f16 100644 --- a/tests/integration/resources.test.ts +++ b/tests/integration/resources.test.ts @@ -1,11 +1,11 @@ /** - * Integration tests for all 11 MCP resources against a live BookStack. + * Integration tests for all 5 MCP resources and 6 resource templates against a live BookStack. * * Resources are the read-only half of the MCP surface (distinct from tools): - * six classes under src/resources/ expose 11 URIs, five of them templated with - * an `{id}`. This suite constructs every class with a real BookStackClient and - * Logger, then drives each handler with a concrete URI and asserts on real data - * returned by the live API. + * six classes under src/resources/ expose 11 URIs, six of them templates (five with an + * `{id}`, search with a `{query}`). This suite constructs every class with a real + * BookStackClient and Logger, then drives each handler with a concrete URI and asserts on + * real data returned by the live API. * * Isolation: the instance is shared with other suites running right now, so the * list resources (`bookstack://books` and friends return *everything*) are only @@ -82,7 +82,6 @@ function makeConfig(harness: BookStackHarness): Config { rateLimit: { requestsPerMinute: 600, burstLimit: 50 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; } @@ -202,7 +201,7 @@ describe.skipIf(!runIntegration)('MCP resources against live BookStack', () => { await cleanup.run(harness); }, 60_000); - it('registers all 11 resources with declared metadata', () => { + it('registers all 5 resources and 6 resource templates with declared metadata', () => { expect(resources).toHaveLength(EXPECTED_URIS.length); expect(resources.map((resource) => resource.uri).sort()).toEqual([...EXPECTED_URIS].sort()); diff --git a/tests/integration/roles.test.ts b/tests/integration/roles.test.ts index 6741ead..ef3a52e 100644 --- a/tests/integration/roles.test.ts +++ b/tests/integration/roles.test.ts @@ -262,7 +262,6 @@ describe.skipIf(!runIntegration)('BookStack role tools (live)', () => { // see exactly what a deployed server does with the same input. validation: { enabled: true, strictMode: true }, logging: { level: 'info', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/integration/search.test.ts b/tests/integration/search.test.ts index 16b2fc2..37a6d46 100644 --- a/tests/integration/search.test.ts +++ b/tests/integration/search.test.ts @@ -60,7 +60,6 @@ function makeConfig(harness: BookStackHarness): Config { rateLimit: { requestsPerMinute: 600, burstLimit: 50 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; } diff --git a/tests/integration/shelves.test.ts b/tests/integration/shelves.test.ts index ed7d5c0..3466c78 100644 --- a/tests/integration/shelves.test.ts +++ b/tests/integration/shelves.test.ts @@ -50,7 +50,6 @@ function makeConfig(harness: BookStackHarness): Config { rateLimit: { requestsPerMinute: 600, burstLimit: 50 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; } diff --git a/tests/integration/system.test.ts b/tests/integration/system.test.ts index 8f0b72b..01e36d7 100644 --- a/tests/integration/system.test.ts +++ b/tests/integration/system.test.ts @@ -144,7 +144,6 @@ describe.skipIf(!runIntegration)('system + server-info tools (live BookStack)', rateLimit: { requestsPerMinute: 60, burstLimit: 10 }, validation: { enabled: true, strictMode: false }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); @@ -321,7 +320,7 @@ describe.skipIf(!runIntegration)('system + server-info tools (live BookStack)', expect(info.capabilities.tools.total).toBe(toolsMap.size); expect(info.capabilities.resources.total).toBe(resourcesMap.size); expect(info.capabilities.authentication.required).toBe(true); - expect(info.capabilities.authentication.methods).toContain('API Token'); + expect(info.capabilities.authentication.methods).toContain('BookStack API token'); expect(info.tool_categories.length).toBeGreaterThan(0); expect(info.resource_types.length).toBeGreaterThan(0); expect(info.error_handling.common_errors.length).toBeGreaterThan(0); diff --git a/tests/integration/users.test.ts b/tests/integration/users.test.ts index 7e11b9c..f35ad3f 100644 --- a/tests/integration/users.test.ts +++ b/tests/integration/users.test.ts @@ -219,7 +219,6 @@ describe.skipIf(!runIntegration)('BookStack user tools (live)', () => { // see exactly what a deployed server does with the same input. validation: { enabled: true, strictMode: true }, logging: { level: 'info', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; const logger = Logger.getInstance(); diff --git a/tests/transport/docker-smoke.test.ts b/tests/transport/docker-smoke.test.ts index 570baf6..87b56c4 100644 --- a/tests/transport/docker-smoke.test.ts +++ b/tests/transport/docker-smoke.test.ts @@ -30,7 +30,7 @@ const IMAGE = process.env.DOCKER_SMOKE_IMAGE ?? 'bookstack-mcp-server:ci'; /** Dummy credentials: syntactically valid, pointing nowhere. */ const MCP_AUTH_TOKEN = 'docker-smoke-inbound-secret'; const BOOKSTACK_API_TOKEN = 'docker-smoke-id:docker-smoke-secret'; -/** Port 9 (discard) inside the container: reliably closed, so health fails fast. */ +/** Port 9 (discard) inside the container: reliably closed, so health reports unhealthy. */ const UNREACHABLE_BOOKSTACK = 'http://127.0.0.1:9/api'; /** How long to wait for the container's HTTP transport to answer. */ diff --git a/tests/transport/http.test.ts b/tests/transport/http.test.ts index 4404dee..73a910c 100644 --- a/tests/transport/http.test.ts +++ b/tests/transport/http.test.ts @@ -15,7 +15,8 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; import type { Server } from 'node:http'; -import type { AddressInfo } from 'node:net'; +import { type AddressInfo, createServer as createNetServer } from 'node:net'; +import { join } from 'node:path'; import pkg from '../../package.json' with { type: 'json' }; import { type Config, @@ -30,6 +31,7 @@ import { resetSharedRateLimiters } from '../../src/utils/rateLimit'; /** Port 9 is the discard service: reliably closed, so BookStack calls fail fast. */ const UNREACHABLE_BOOKSTACK = 'http://127.0.0.1:9/api'; +const SERVER_ENTRY = join(import.meta.dir, '..', '..', 'src', 'server.ts'); const TEST_AUTH_TOKEN = 'test-inbound-secret-0123456789'; /** @@ -280,6 +282,33 @@ describe('HTTP transport configuration', () => { expect(loadHttpTransportConfig({ MCP_AUTH_TOKEN: '' }).authToken).toBeUndefined(); expect(loadHttpTransportConfig({}).authToken).toBeUndefined(); }); + + it('reads BOOKSTACK_ALLOWED_BASE_URLS as a comma-separated list, treating blank as unset', () => { + expect( + loadHttpTransportConfig({ + BOOKSTACK_ALLOWED_BASE_URLS: ' https://a.example/api , ,https://b.example/api', + }).allowedBaseUrls + ).toEqual(['https://a.example/api', 'https://b.example/api']); + expect( + loadHttpTransportConfig({ BOOKSTACK_ALLOWED_BASE_URLS: ' , ' }).allowedBaseUrls + ).toBeUndefined(); + expect(loadHttpTransportConfig({}).allowedBaseUrls).toBeUndefined(); + }); + + it('rejects an unusable BOOKSTACK_ALLOWED_BASE_URLS entry without echoing it', () => { + let message = ''; + try { + loadHttpTransportConfig({ + BOOKSTACK_ALLOWED_BASE_URLS: + 'https://a.example/api,https://b.example/api?api_token=allowlist-leak-marker', + }); + } catch (error) { + message = (error as Error).message; + } + + expect(message).toContain('BOOKSTACK_ALLOWED_BASE_URLS'); + expect(message).not.toContain('allowlist-leak-marker'); + }); }); describe('GET / identity', () => { @@ -365,6 +394,45 @@ describe('HTTP transport startup', () => { expect((await fetch(`${url}/`)).status).toBe(200); }); + + it('exits 1 with a clear message when the port is already in use', async () => { + const holder = createNetServer(); + await new Promise((resolve, reject) => { + holder.once('error', reject); + holder.listen(0, () => resolve()); + }); + const port = (holder.address() as AddressInfo).port; + + try { + const proc = Bun.spawn({ + cmd: [process.execPath, 'run', SERVER_ENTRY], + cwd: join(import.meta.dir, '..', '..'), + env: { + PATH: process.env.PATH ?? '', + HOME: process.env.HOME ?? '', + MCP_TRANSPORT: 'http', + MCP_AUTH_TOKEN: TEST_AUTH_TOKEN, + BOOKSTACK_API_TOKEN: 'stub-id:stub-secret', + BOOKSTACK_BASE_URL: UNREACHABLE_BOOKSTACK, + SERVER_PORT: String(port), + LOG_FORMAT: 'json', + }, + stdin: 'ignore', + stdout: 'pipe', + stderr: 'pipe', + }); + const timer = setTimeout(() => proc.kill(), 15_000); + const [exitCode, stderr] = await Promise.all([proc.exited, new Response(proc.stderr).text()]); + clearTimeout(timer); + + expect(exitCode).toBe(1); + expect(stderr).toContain(`Failed to start HTTP transport: cannot listen on port ${port}`); + expect(stderr).toContain('EADDRINUSE'); + expect(stderr).not.toContain('listening on port'); + } finally { + holder.close(); + } + }, 20_000); }); /** @@ -626,7 +694,7 @@ describe('an unusable configured base URL fails before the server listens', () = // Refused inside the authenticated route, where an authenticated caller's own mistake // belongs - not at startup, and not by being forwarded to axios. - expect(response.status).toBe(500); + expect(response.status).toBe(400); expect(text).not.toContain('override-leak-marker'); expect(text).not.toContain('books.example'); }, 15_000); @@ -815,6 +883,25 @@ describe('unauthenticated endpoints', () => { expect(response.status).toBe(404); expect(((await response.json()) as { error?: string }).error).toBe('Not Found'); }); + + it.each(['GET', 'DELETE'])( + 'answers %s /message with 405 and Allow: POST, before authentication', + async (method) => { + const { url } = await startApp({ + bodyLimitBytes: DEFAULT_HTTP_BODY_LIMIT_BYTES, + authToken: TEST_AUTH_TOKEN, + }); + + for (const headers of [{}, { authorization: `Bearer ${TEST_AUTH_TOKEN}` }]) { + const response = await fetch(`${url}/message`, { method, headers }); + + expect(response.status).toBe(405); + expect(response.headers.get('allow')).toBe('POST'); + expect(response.headers.get('www-authenticate')).toBeNull(); + expect(((await response.json()) as { error?: string }).error).toBe('Method Not Allowed'); + } + } + ); }); /* ------------------------------------------------------------- a stub BookStack -- */ diff --git a/tests/transport/oauth.test.ts b/tests/transport/oauth.test.ts index 111838b..8e1e97e 100644 --- a/tests/transport/oauth.test.ts +++ b/tests/transport/oauth.test.ts @@ -177,6 +177,19 @@ describe('OAuth discovery', () => { }); }); + it.each(['GET', 'DELETE'])( + 'answers %s /message with 405 and Allow: POST, before authentication', + async (method) => { + const url = await startApp(); + + const response = await fetch(`${url}/message`, { method }); + + expect(response.status).toBe(405); + expect(response.headers.get('allow')).toBe('POST'); + expect(response.headers.get('www-authenticate')).toBeNull(); + } + ); + it('challenges a request without a token with the metadata location', async () => { const url = await startApp(); diff --git a/tests/transport/stdio.test.ts b/tests/transport/stdio.test.ts index fe14421..d18e01d 100644 --- a/tests/transport/stdio.test.ts +++ b/tests/transport/stdio.test.ts @@ -21,6 +21,7 @@ import { afterEach, beforeAll, describe, expect, it } from 'bun:test'; import { join } from 'node:path'; import type { Subprocess } from 'bun'; import pkg from '../../package.json' with { type: 'json' }; +import { MISSING_API_TOKEN_MESSAGE } from '../../src/api/client'; import { type BookStackStub, startBookStackStub } from './stub-bookstack'; const REPO_ROOT = join(import.meta.dir, '..', '..'); @@ -42,6 +43,8 @@ interface JsonRpcMessage { result?: { serverInfo?: { name?: string; version?: string }; tools?: Array<{ name: string }>; + content?: Array<{ type: string; text: string }>; + isError?: boolean; }; error?: { code: number; message: string }; } @@ -374,6 +377,31 @@ describe('stdio entry point', () => { REPLY_TIMEOUT_MS + 5_000 ); + it( + 'returns a tool failure as an isError result over stdio', + async () => { + const server = spawnStdioServer(); + await initialize(server); + + server.send({ + jsonrpc: '2.0', + id: 4, + method: 'tools/call', + params: { name: 'bookstack_books_read', arguments: { id: 999 } }, + }); + const reply = parseProtocolLine(await server.nextStdoutLine()); + + expect(reply.error).toBeUndefined(); + expect(reply.result?.isError).toBe(true); + const text = reply.result?.content?.[0]?.text ?? ''; + expect(text).toContain('Requested resource not found'); + expect(text).toContain('"status": 404'); + expect(text).toContain('Book not found'); + expect(text).not.toContain('client.ts'); + }, + REPLY_TIMEOUT_MS + 5_000 + ); + it( 'sends its human-readable startup notice to stderr', async () => { @@ -390,6 +418,51 @@ describe('stdio entry point', () => { ); }); +/** Run the entry point until it exits, with an explicit environment. */ +async function runToExit( + env: Record +): Promise<{ exitCode: number; stdout: string; stderr: string }> { + const proc = Bun.spawn({ + cmd: [process.execPath, 'run', SERVER_ENTRY], + cwd: REPO_ROOT, + env: { PATH: process.env.PATH ?? '', HOME: process.env.HOME ?? '', ...env }, + stdin: 'pipe', + stdout: 'pipe', + stderr: 'pipe', + }); + const timer = setTimeout(() => proc.kill(), REPLY_TIMEOUT_MS); + const [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); + clearTimeout(timer); + return { exitCode, stdout, stderr }; +} + +describe('stdio startup without BOOKSTACK_API_TOKEN', () => { + it( + 'prints one clean line naming the variable to stderr and exits 1', + async () => { + const { exitCode, stdout, stderr } = await runToExit({ + MCP_TRANSPORT: 'stdio', + BOOKSTACK_BASE_URL: stub.baseUrl, + LOG_FORMAT: 'json', + }); + + expect(exitCode).toBe(1); + expect(stdout).toBe(''); + expect(stderr.split('\n').filter((line) => line.includes('BOOKSTACK_API_TOKEN'))).toEqual([ + `Failed to start stdio transport: ${MISSING_API_TOKEN_MESSAGE}`, + ]); + expect(stderr).not.toContain('client.ts'); + expect(stderr).not.toContain(' at '); + expect(stderr).not.toContain('listening on stdio'); + }, + REPLY_TIMEOUT_MS + 5_000 + ); +}); + /** * Redaction, proved against a real process's real streams. * @@ -579,8 +652,8 @@ describe('spawned stdio server never logs a secret', () => { }); await server.nextStdoutLine(); - // The upload guard interpolates the caller's path into its error message; the - // tool boundary then logs that message and its stack. + // The upload guard refuses this path, and the tool boundary logs the refusal and its + // stack; neither may carry the caller's path. server.send({ jsonrpc: '2.0', id: 13, @@ -670,7 +743,7 @@ describe('spawned stdio server never logs a secret', () => { expect(stderr).toContain('[redacted:'); // Frames survive: this is the "where", and it is a code location rather than // caller text. Losing it would be trading the leak for a blind spot. - expect(stderr).toContain('resolveRealPath'); + expect(stderr).toContain('readGuardedUploadFile'); expect(stderr).toContain('client.ts'); // THE CALL SITES, LINE BY LINE. diff --git a/tests/transport/tools.test.ts b/tests/transport/tools.test.ts index 3832d39..ec4547c 100644 --- a/tests/transport/tools.test.ts +++ b/tests/transport/tools.test.ts @@ -97,15 +97,19 @@ const EXPECTED_TOOLS = [ /** Same reasoning as EXPECTED_TOOLS: the resource URIs are a client-visible contract. */ const EXPECTED_RESOURCES = [ 'bookstack://books', - 'bookstack://books/{id}', 'bookstack://chapters', - 'bookstack://chapters/{id}', 'bookstack://pages', + 'bookstack://shelves', + 'bookstack://users', +] as const; + +/** The parameterised half of that contract, published on resources/templates/list. */ +const EXPECTED_RESOURCE_TEMPLATES = [ + 'bookstack://books/{id}', + 'bookstack://chapters/{id}', 'bookstack://pages/{id}', 'bookstack://search/{query}', - 'bookstack://shelves', 'bookstack://shelves/{id}', - 'bookstack://users', 'bookstack://users/{id}', ] as const; @@ -121,6 +125,8 @@ const PINNED_ENV = [ 'LOG_FORMAT', 'VALIDATION_ENABLED', 'VALIDATION_STRICT_MODE', + 'RATE_LIMIT_REQUESTS_PER_MINUTE', + 'RATE_LIMIT_BURST_LIMIT', ] as const; const savedEnv = new Map(); @@ -142,21 +148,39 @@ interface ResourceListEntry { uri: string; name: string; } +interface ResourceTemplateEntry { + uriTemplate: string; + name: string; + description?: string; + mimeType?: string; +} interface ToolCallContent { type: string; text: string; } -interface ValidationErrorData { - type: string; - validation: Array<{ field: string; message: string }>; -} interface JsonRpcReply { result?: { tools?: ToolListEntry[]; resources?: ResourceListEntry[]; + resourceTemplates?: ResourceTemplateEntry[]; + contents?: Array<{ uri: string; mimeType?: string; text: string }>; content?: ToolCallContent[]; + isError?: boolean; + }; + error?: { code: number; message: string; data?: unknown }; +} + +/** Split a tool error result's text into its message and the JSON hints below it. */ +function toolError(reply: JsonRpcReply): { message: string; data: Record } { + expect(reply.error).toBeUndefined(); + expect(reply.result?.isError).toBe(true); + const text = reply.result?.content?.[0]?.text ?? ''; + const split = text.indexOf('\n\n'); + expect(split).toBeGreaterThan(0); + return { + message: text.slice(0, split), + data: JSON.parse(text.slice(split + 2)) as Record, }; - error?: { code: number; message: string; data?: ValidationErrorData }; } /** The JSON a books list handler returns, once unwrapped from MCP's text content. */ @@ -179,6 +203,9 @@ beforeAll(() => { // silently turn that boundary into permissive forwarding before the singleton reloads. delete process.env.VALIDATION_ENABLED; delete process.env.VALIDATION_STRICT_MODE; + // Wide open: at the defaults the shared stub bucket paces calls to 1/s, past the test timeout. + process.env.RATE_LIMIT_REQUESTS_PER_MINUTE = '60000'; + process.env.RATE_LIMIT_BURST_LIMIT = '1000'; // reload() rather than getConfig(): another suite may already have populated the // singleton, and /message builds its server from whatever the singleton holds. @@ -309,7 +336,7 @@ async function runtimeAccepts( // A call that neither reached BookStack nor reported anything would read as a clean // rejection here while actually being a hole in dispatch. - if (!reachedBookStack && reply.error === undefined) { + if (!reachedBookStack && reply.error === undefined && reply.result?.isError !== true) { throw new Error( `${toolName} neither called BookStack nor errored for ${JSON.stringify(input)}` ); @@ -647,6 +674,11 @@ describe('published JSON Schema agrees with runtime validation', () => { describe('published JSON Schema agrees with runtime validation on scalar constraints', () => { /** A string of `length` ordinary characters. */ const chars = (length: number): string => 'x'.repeat(length); + /** A well-formed URL exactly `length` characters long. */ + const linkOfLength = (length: number): string => { + const prefix = 'https://example.com/'; + return `${prefix}${chars(length - prefix.length)}`; + }; interface ScalarCase { tool: string; @@ -913,6 +945,16 @@ describe('published JSON Schema agrees with runtime validation on scalar constra input: { uploaded_to: 1, name: chars(181), image: 'aGVsbG8=' }, accepted: false, }, + { label: 'empty base64 image', input: { uploaded_to: 1, image: '' }, accepted: false }, + { label: 'empty file_path', input: { uploaded_to: 1, file_path: '' }, accepted: false }, + ], + }, + { + tool: 'bookstack_images_update', + rows: [ + { label: 'control: a rename', input: { id: 1, name: 'Diagram' }, accepted: true }, + { label: 'empty base64 image', input: { id: 1, image: '' }, accepted: false }, + { label: 'empty file_path', input: { id: 1, file_path: '' }, accepted: false }, ], }, { @@ -938,6 +980,44 @@ describe('published JSON Schema agrees with runtime validation on scalar constra input: { uploaded_to: 1, name: chars(256), file: 'aGVsbG8=' }, accepted: false, }, + { + label: 'empty base64 file', + input: { uploaded_to: 1, name: 'Spec', file: '' }, + accepted: false, + }, + { + label: 'empty file_path', + input: { uploaded_to: 1, name: 'Spec', file_path: '' }, + accepted: false, + }, + { + label: 'link at the 2000-character maximum', + input: { uploaded_to: 1, name: 'Spec', link: linkOfLength(2000) }, + accepted: true, + }, + { + label: 'link one character over the maximum', + input: { uploaded_to: 1, name: 'Spec', link: linkOfLength(2001) }, + accepted: false, + }, + ], + }, + { + tool: 'bookstack_attachments_update', + rows: [ + { label: 'control: a rename', input: { id: 1, name: 'Spec' }, accepted: true }, + { label: 'empty base64 file', input: { id: 1, file: '' }, accepted: false }, + { label: 'empty file_path', input: { id: 1, file_path: '' }, accepted: false }, + { + label: 'link at the 2000-character maximum', + input: { id: 1, link: linkOfLength(2000) }, + accepted: true, + }, + { + label: 'link one character over the maximum', + input: { id: 1, link: linkOfLength(2001) }, + accepted: false, + }, ], }, { @@ -1054,10 +1134,10 @@ describe('malformed tools/call arguments are refused before BookStack is contact expect(stub.requests).toHaveLength(0); }); - it('returns an unknown argument in JSON-RPC validation data without contacting BookStack', async () => { + it('returns an unknown argument in the tool error result without contacting BookStack', async () => { // Issue #21: a client must receive the reason for a strict rejection, rather than only a // generic tool-failed banner. This is deliberately a wire assertion: a unit test of - // ErrorHandler would not prove the Streamable HTTP transport preserves McpError.data. + // ErrorHandler would not prove the Streamable HTTP transport delivers the details. const url = await startApp(); const { status, reply } = await rpc(url, 'tools/call', { @@ -1071,16 +1151,29 @@ describe('malformed tools/call arguments are refused before BookStack is contact }); expect(status).toBe(200); - expect(reply.result).toBeUndefined(); - expect(reply.error?.code).toBe(-32602); - expect(reply.error?.message).toContain('Validation failed'); - expect(reply.error?.data).toEqual({ + const { message, data } = toolError(reply); + expect(message).toBe('Validation failed'); + expect(data).toEqual({ type: 'validation_error', validation: [{ field: '', message: 'Unrecognized key: "description"' }], }); expect(stub.requests).toHaveLength(0); }); + it('names the failing field path in the tool error result', async () => { + const url = await startApp(); + + const { reply } = await rpc(url, 'tools/call', { + name: 'bookstack_books_read', + arguments: { id: 'one' }, + }); + + const { data } = toolError(reply); + expect(data.type).toBe('validation_error'); + expect(data.validation).toEqual([{ field: 'id', message: expect.any(String) }]); + expect(stub.requests).toHaveLength(0); + }); + it('sends no search for a query BookStack would reject as missing', async () => { const url = await startApp(); @@ -1130,17 +1223,63 @@ describe('malformed tools/call arguments are refused before BookStack is contact }); }); -describe('resources/list over HTTP', () => { - it('publishes exactly the expected resource surface', async () => { +describe('resources over HTTP', () => { + it('lists only concrete resources on resources/list', async () => { const url = await startApp(); const { status, reply } = await rpc(url, 'resources/list'); expect(status).toBe(200); const uris = (reply.result?.resources ?? []).map((resource) => resource.uri).sort(); - expect(uris).toHaveLength(11); + expect(uris).toHaveLength(5); expect(uris).toEqual([...EXPECTED_RESOURCES]); }); + + it('publishes the templated resources on resources/templates/list', async () => { + const url = await startApp(); + + const { status, reply } = await rpc(url, 'resources/templates/list'); + + expect(status).toBe(200); + expect(reply.error).toBeUndefined(); + const templates = reply.result?.resourceTemplates ?? []; + expect(templates.map((template) => template.uriTemplate).sort()).toEqual([ + ...EXPECTED_RESOURCE_TEMPLATES, + ]); + for (const template of templates) { + expect(Object.keys(template).sort()).toEqual([ + 'description', + 'mimeType', + 'name', + 'uriTemplate', + ]); + expect(template.name.length).toBeGreaterThan(0); + expect(template.description?.length).toBeGreaterThan(0); + expect(template.mimeType).toBe('application/json'); + } + }); + + it('reads a concrete URI built from a template', async () => { + const url = await startApp(); + + const { reply } = await rpc(url, 'resources/read', { uri: 'bookstack://books/1' }); + + expect(reply.error).toBeUndefined(); + const content = reply.result?.contents?.[0]; + expect(content?.uri).toBe('bookstack://books/1'); + expect(content?.mimeType).toBe('application/json'); + expect((JSON.parse(content?.text ?? '{}') as { name?: string }).name).toBe(STUB_BOOKS[0].name); + }); + + it('reports a failed resource read as a JSON-RPC error', async () => { + const url = await startApp(); + + const { reply } = await rpc(url, 'resources/read', { uri: 'bookstack://books/999' }); + + expect(reply.result).toBeUndefined(); + expect(reply.error?.message).toContain('Requested resource not found'); + expect(JSON.stringify(reply)).not.toContain('client.ts'); + }); }); describe('tools/call over HTTP', () => { @@ -1194,6 +1333,51 @@ describe('tools/call over HTTP', () => { expect(stub.requests[0]?.authorization).toBe(`Token ${config.bookstack.apiToken}`); }); + it('refuses a file_path upload without naming a server path or sending a stack', async () => { + const savedRoot = process.env.BOOKSTACK_UPLOAD_ROOT; + delete process.env.BOOKSTACK_UPLOAD_ROOT; + try { + const url = await startApp(); + + const { reply } = await rpc(url, 'tools/call', { + name: 'bookstack_attachments_create', + arguments: { uploaded_to: 1, name: 'Key', file_path: '/etc/upload-probe-marker' }, + }); + + const { message, data } = toolError(reply); + expect(message).toContain('BOOKSTACK_UPLOAD_ROOT is not set'); + expect(data).toEqual({ type: 'internal_error' }); + expect(JSON.stringify(reply)).not.toContain('upload-probe-marker'); + expect(JSON.stringify(reply)).not.toContain('client.ts'); + expect(stub.requests).toHaveLength(0); + } finally { + if (savedRoot !== undefined) { + process.env.BOOKSTACK_UPLOAD_ROOT = savedRoot; + } + } + }); + + it('returns a BookStack failure as a tool error result with its status and details', async () => { + const url = await startApp(); + + const { status, reply } = await rpc(url, 'tools/call', { + name: 'bookstack_books_read', + arguments: { id: 999 }, + }); + + expect(status).toBe(200); + const { message, data } = toolError(reply); + expect(message).toBe('Requested resource not found'); + expect(data).toMatchObject({ + type: 'not_found_error', + status: 404, + details: { error: { code: 404, message: 'Book not found' } }, + }); + expect(data).not.toHaveProperty('url'); + expect(JSON.stringify(reply)).not.toContain('client.ts'); + expect(JSON.stringify(reply)).not.toContain(' at '); + }); + it('reports an unknown tool as a JSON-RPC error, not a crash', async () => { const url = await startApp(); diff --git a/tests/transport/upstream-overrides.test.ts b/tests/transport/upstream-overrides.test.ts new file mode 100644 index 0000000..2099294 --- /dev/null +++ b/tests/transport/upstream-overrides.test.ts @@ -0,0 +1,213 @@ +/** + * Transport tests: the per-request `x-bookstack-url` / `x-bookstack-token` overrides. + * + * `x-bookstack-url` decides which host this server calls on the caller's behalf, so it is + * the server's outbound-request surface. Two BookStack stubs stand in for the configured + * instance and for a second one, and every assertion reads what each stub actually received: + * a refusal has to happen before any BookStack call, and the configured BOOKSTACK_API_TOKEN + * must never reach a host the caller chose. + */ + +import { afterAll, afterEach, beforeAll, describe, expect, it } from 'bun:test'; +import type { Server } from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { type Config, ConfigManager } from '../../src/config/manager'; +import { createHttpApp } from '../../src/server'; +import { resetSharedRateLimiters } from '../../src/utils/rateLimit'; +import { type BookStackStub, STUB_BOOKS, startBookStackStub } from './stub-bookstack'; + +const TEST_AUTH_TOKEN = 'overrides-inbound-secret-0123456789'; +const BODY_LIMIT_BYTES = 1024 * 1024; +/** Deliberately not a token either stub accepts, so it is recognisable wherever it lands. */ +const CONFIGURED_TOKEN = 'configured-id:configured-secret'; + +const PINNED_ENV = [ + 'BOOKSTACK_BASE_URL', + 'BOOKSTACK_API_TOKEN', + 'LOG_LEVEL', + 'LOG_FORMAT', +] as const; +const savedEnv = new Map(); + +let config: Config; +/** The BookStack this server is configured for. */ +let configured: BookStackStub; +/** A second BookStack, reachable only by naming it in x-bookstack-url. */ +let other: BookStackStub; + +beforeAll(() => { + for (const key of PINNED_ENV) { + savedEnv.set(key, process.env[key]); + } + configured = startBookStackStub(); + other = startBookStackStub(); + + process.env.BOOKSTACK_BASE_URL = configured.baseUrl; + process.env.BOOKSTACK_API_TOKEN = CONFIGURED_TOKEN; + process.env.LOG_LEVEL = 'error'; + process.env.LOG_FORMAT = 'json'; + config = ConfigManager.getInstance().reload(); +}); + +afterAll(async () => { + await configured.stop(); + await other.stop(); + for (const key of PINNED_ENV) { + const value = savedEnv.get(key); + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + ConfigManager.resetInstance(); + resetSharedRateLimiters(); +}); + +const running: Server[] = []; + +afterEach(async () => { + await Promise.all( + running.splice(0).map( + (server) => + new Promise((resolve) => { + server.close(() => resolve()); + }) + ) + ); + configured.requests.length = 0; + other.requests.length = 0; +}); + +async function startApp(allowedBaseUrls?: string[]): Promise { + const app = createHttpApp({ + config, + http: { + bodyLimitBytes: BODY_LIMIT_BYTES, + authToken: TEST_AUTH_TOKEN, + ...(allowedBaseUrls ? { allowedBaseUrls } : {}), + }, + }); + const server = await new Promise((resolve, reject) => { + const listener = app.listen(0, '127.0.0.1', () => resolve(listener)); + listener.on('error', reject); + }); + running.push(server); + return `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +} + +/** A books list call, which reaches BookStack whenever it is dispatched at all. */ +async function listBooks(url: string, headers: Record): Promise { + return fetch(`${url}/message`, { + method: 'POST', + headers: { + 'content-type': 'application/json', + accept: 'application/json, text/event-stream', + authorization: `Bearer ${TEST_AUTH_TOKEN}`, + ...headers, + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'tools/call', + params: { name: 'bookstack_books_list', arguments: { count: 1 } }, + }), + }); +} + +/** A 400 with the transport's JSON error shape, that does not repeat what it refused. */ +async function expectRefusal(response: Response, unechoed: string[]): Promise { + expect(response.status).toBe(400); + expect(response.headers.get('content-type')).toContain('application/json'); + const text = await response.text(); + for (const value of unechoed) { + expect(text).not.toContain(value); + } + const payload = JSON.parse(text) as { error?: string; message?: string }; + expect(payload.error).toBe('Bad Request'); + return payload.message ?? ''; +} + +describe('x-bookstack-url', () => { + it('is refused when BOOKSTACK_ALLOWED_BASE_URLS is unset', async () => { + const url = await startApp(); + + const response = await listBooks(url, { + 'x-bookstack-url': other.baseUrl, + 'x-bookstack-token': other.apiToken, + }); + + const message = await expectRefusal(response, [other.baseUrl, other.apiToken]); + expect(message).toContain('BOOKSTACK_ALLOWED_BASE_URLS'); + expect(configured.requests).toEqual([]); + expect(other.requests).toEqual([]); + }); + + it('is refused when it names a URL that is not on the list', async () => { + const url = await startApp([configured.baseUrl]); + + for (const named of [ + other.baseUrl, + 'https://books.example/api?api_token=override-leak-marker', + ]) { + const response = await listBooks(url, { + 'x-bookstack-url': named, + 'x-bookstack-token': other.apiToken, + }); + + await expectRefusal(response, [named, 'override-leak-marker', other.apiToken]); + } + expect(configured.requests).toEqual([]); + expect(other.requests).toEqual([]); + }); + + it('is refused without x-bookstack-token, so the configured token stays home', async () => { + const url = await startApp([other.baseUrl]); + + const response = await listBooks(url, { 'x-bookstack-url': other.baseUrl }); + + const message = await expectRefusal(response, [other.baseUrl, CONFIGURED_TOKEN]); + expect(message).toContain('x-bookstack-token'); + expect(configured.requests).toEqual([]); + expect(other.requests).toEqual([]); + }); + + it('calls an allowed URL with the caller token, matching it canonically', async () => { + // Trailing slash and upper-case scheme: the same upstream, spelled two other ways. + const url = await startApp([`${other.baseUrl}/`]); + + const response = await listBooks(url, { + 'x-bookstack-url': other.baseUrl.replace('http://', 'HTTP://'), + 'x-bookstack-token': other.apiToken, + }); + + expect(response.status).toBe(200); + const reply = (await response.json()) as { + result?: { content?: Array<{ text: string }> }; + error?: unknown; + }; + expect(reply.error).toBeUndefined(); + const books = JSON.parse(reply.result?.content?.[0]?.text ?? '{}') as { + data?: Array<{ name: string }>; + }; + expect(books.data?.map((book) => book.name)).toEqual([STUB_BOOKS[0].name]); + + expect(other.requests.map((request) => [request.path, request.authorization])).toEqual([ + ['/books', `Token ${other.apiToken}`], + ]); + expect(configured.requests).toEqual([]); + }); +}); + +describe('x-bookstack-token alone', () => { + it('is still spent against the configured BOOKSTACK_BASE_URL, with no allowlist needed', async () => { + const url = await startApp(); + + await listBooks(url, { 'x-bookstack-token': 'caller-id:caller-secret' }); + + expect(configured.requests.map((request) => [request.path, request.authorization])).toEqual([ + ['/books', 'Token caller-id:caller-secret'], + ]); + expect(other.requests).toEqual([]); + }); +}); diff --git a/tests/unit/books.test.ts b/tests/unit/books.test.ts index e840754..afa7154 100644 --- a/tests/unit/books.test.ts +++ b/tests/unit/books.test.ts @@ -3,7 +3,7 @@ import type { BookStackClient } from '../../src/api/client'; import { BookTools } from '../../src/tools/books'; import type { Book, BookWithContents, ListResponse, MCPTool } from '../../src/types'; import type { Logger } from '../../src/utils/logger'; -import type { ValidationHandler } from '../../src/validation/validator'; +import { ValidationHandler } from '../../src/validation/validator'; /** * Types only a subset of `T`'s methods, each as a bun:test `Mock` carrying that @@ -105,12 +105,20 @@ describe('BookTools', () => { const mockResponse: ListResponse = { data: [], total: 0 }; mockClient.listBooks.mockResolvedValue(mockResponse); - const listTool = findTool('bookstack_books_list'); + // The real validator: the defaults are the schema's, not this file's. + const listTool = new BookTools( + mockClient as unknown as BookStackClient, + new ValidationHandler({ enabled: true, strictMode: true }), + mockLogger as unknown as Logger + ) + .getTools() + .find((tool) => tool.name === 'bookstack_books_list'); + if (!listTool) throw new Error('Expected tool bookstack_books_list to be registered'); const result = await listTool.handler({}); - expect(mockValidator.validateParams).toHaveBeenCalled(); - expect(mockClient.listBooks).toHaveBeenCalled(); + expect(mockClient.listBooks).toHaveBeenCalledTimes(1); + expect(mockClient.listBooks).toHaveBeenCalledWith({ count: 20, offset: 0, sort: 'name' }); expect(result).toEqual(mockResponse); }); diff --git a/tests/unit/bookstack-credential.test.ts b/tests/unit/bookstack-credential.test.ts index dd22caa..0c240d2 100644 --- a/tests/unit/bookstack-credential.test.ts +++ b/tests/unit/bookstack-credential.test.ts @@ -14,7 +14,6 @@ function configFor(stub: BookStackStub, apiToken?: string): Config { rateLimit: { requestsPerMinute: 60_000, burstLimit: 10_000 }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'json' }, - development: { nodeEnv: 'test', debug: false }, }; } diff --git a/tests/unit/client-redirect.test.ts b/tests/unit/client-redirect.test.ts new file mode 100644 index 0000000..58aa742 --- /dev/null +++ b/tests/unit/client-redirect.test.ts @@ -0,0 +1,98 @@ +/** + * BookStackClient never follows a redirect. + * + * Every request carries the BookStack credential, and a redirect hands the next hop to whoever + * answered: a 3xx from an upstream, or from a proxy in front of it, would otherwise send this + * server to a host nobody configured. Exports go through the same axios instance, so both the + * JSON and the export path are driven here against two real local servers. + */ + +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; +import { BookStackClient } from '../../src/api/client'; +import type { Config } from '../../src/config/manager'; +import { ErrorHandler } from '../../src/utils/errors'; +import type { Logger } from '../../src/utils/logger'; +import { resetSharedRateLimiters } from '../../src/utils/rateLimit'; + +const noopLogger = { + debug() {}, + info() {}, + warn() {}, + error() {}, +} as unknown as Logger; + +interface RecordingServer { + url: string; + paths: string[]; + stop(): void; +} + +function startServer(answer: (path: string) => Response): RecordingServer { + const paths: string[] = []; + const server = Bun.serve({ + port: 0, + hostname: '127.0.0.1', + fetch(request: Request): Response { + const path = new URL(request.url).pathname; + paths.push(path); + return answer(path); + }, + }); + return { + url: `http://127.0.0.1:${server.port}`, + paths, + stop: () => server.stop(true), + }; +} + +describe('BookStackClient redirects', () => { + let elsewhere: RecordingServer; + let upstream: RecordingServer; + let client: BookStackClient; + + beforeAll(() => { + elsewhere = startServer(() => Response.json({ version: 'elsewhere' })); + upstream = startServer( + (path) => + new Response(null, { status: 302, headers: { location: `${elsewhere.url}${path}` } }) + ); + + const config: Config = { + bookstack: { + baseUrl: `${upstream.url}/api`, + apiToken: 'redirect-test-id:redirect-test-secret', + timeout: 5_000, + }, + server: { name: 'bookstack-mcp-server-redirect-test', version: '1.0.0', port: 3000 }, + rateLimit: { requestsPerMinute: 60_000, burstLimit: 10_000 }, + validation: { enabled: true, strictMode: true }, + logging: { level: 'error', format: 'pretty' }, + }; + client = new BookStackClient(config, noopLogger, new ErrorHandler(noopLogger)); + }); + + beforeEach(() => { + upstream.paths.length = 0; + elsewhere.paths.length = 0; + }); + + afterAll(() => { + upstream.stop(); + elsewhere.stop(); + resetSharedRateLimiters(); + }); + + it('fails a JSON call answered with a 302 instead of following it', async () => { + await expect(client.getSystemInfo()).rejects.toThrow(); + + expect(upstream.paths).toEqual(['/api/system']); + expect(elsewhere.paths).toEqual([]); + }); + + it('fails an export answered with a 302 instead of following it', async () => { + await expect(client.exportPage(1, 'markdown')).rejects.toThrow(); + + expect(upstream.paths).toEqual(['/api/pages/1/export/markdown']); + expect(elsewhere.paths).toEqual([]); + }); +}); diff --git a/tests/unit/client-transport.test.ts b/tests/unit/client-transport.test.ts new file mode 100644 index 0000000..69dc2cf --- /dev/null +++ b/tests/unit/client-transport.test.ts @@ -0,0 +1,392 @@ +/** + * Connection-level failures and timeouts in `BookStackClient` (src/api/client.ts) and how + * `ErrorHandler` (src/utils/errors.ts) reports them. + * + * Same approach as tests/unit/retry.test.ts: a real local server, the real axios stack and + * the real ErrorHandler, with `globalThis.setTimeout` replaced so every retry wait is + * recorded and served instantly. The server here is raw TCP rather than `Bun.serve`, + * because the failures under test - a connection dropped before any response - are below + * what an HTTP handler can express. Each response closes its connection, so every attempt + * arrives on a fresh socket and is counted exactly once. + * + * The timeout tests run on real timers: an instantly-firing clock would abort every request. + */ + +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'bun:test'; +import { createServer, type Server, type Socket } from 'node:net'; +import type { AxiosInstance, AxiosRequestConfig, AxiosResponse } from 'axios'; +import { BookStackClient } from '../../src/api/client'; +import type { Config } from '../../src/config/manager'; +import { ErrorHandler } from '../../src/utils/errors'; +import type { Logger } from '../../src/utils/logger'; +import { resetSharedRateLimiters } from '../../src/utils/rateLimit'; + +/** The retry policy's own constants, mirrored here so the bounds can be asserted. */ +const MAX_ATTEMPTS = 4; +const BASE_DELAY_MS = 500; +const JITTER_RATIO = 0.25; + +const noopLogger = { + debug() {}, + info() {}, + warn() {}, + error() {}, +} as unknown as Logger; + +/** What the stub does with one request: drop the connection, answer 200, or never answer. */ +type Step = 'drop' | 'ok' | 'hang'; + +interface TcpStub { + port: number; + baseUrl: string; + /** The request line of every attempt that arrived, in order. */ + requests: string[]; + plan: Step[]; + fallback: Step; + reset(): void; + stop(): Promise; +} + +const OK_BODY = JSON.stringify({ data: [], total: 0 }); + +async function startTcpStub(): Promise { + const sockets = new Set(); + const requests: string[] = []; + + const server: Server = createServer((socket) => { + sockets.add(socket); + socket.on('close', () => sockets.delete(socket)); + socket.on('error', () => {}); + + let head = ''; + let handled = false; + socket.on('data', (chunk: Buffer) => { + if (handled) return; + head += chunk.toString('latin1'); + if (!head.includes('\r\n\r\n')) return; + handled = true; + + requests.push(head.split('\r\n')[0] ?? ''); + const step = stub.plan[requests.length - 1] ?? stub.fallback; + if (step === 'drop') { + socket.destroy(); + } else if (step === 'ok') { + socket.end( + 'HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n' + + `Content-Length: ${Buffer.byteLength(OK_BODY)}\r\nConnection: close\r\n\r\n${OK_BODY}` + ); + } + }); + }); + + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address === null || typeof address === 'string') throw new Error('no TCP port'); + + const stub: TcpStub = { + port: address.port, + baseUrl: `http://127.0.0.1:${address.port}/api`, + requests, + plan: [], + fallback: 'ok', + reset() { + requests.length = 0; + stub.plan = []; + stub.fallback = 'ok'; + }, + async stop() { + for (const socket of sockets) socket.destroy(); + await new Promise((resolve) => server.close(() => resolve())); + }, + }; + return stub; +} + +/** A port nothing listens on: bound, read, released. */ +async function closedPort(): Promise { + const server = createServer(); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address === null || typeof address === 'string') throw new Error('no TCP port'); + await new Promise((resolve) => server.close(() => resolve())); + return address.port; +} + +function configFor(baseUrl: string, timeout: number, token: string): Config { + return { + bookstack: { baseUrl, apiToken: token, timeout }, + server: { name: 'bookstack-mcp-server-transport-test', version: '1.0.0', port: 3000 }, + rateLimit: { requestsPerMinute: 60_000, burstLimit: 10_000 }, + validation: { enabled: true, strictMode: true }, + logging: { level: 'error', format: 'pretty' }, + }; +} + +/** The private retry loop, for HEAD and OPTIONS, which no public method issues. */ +interface RetryDriver { + requestWithRetry(config: AxiosRequestConfig): Promise>; +} + +/** Settle a promise into its rejection, failing if it resolved. */ +async function rejectionOf(promise: Promise): Promise { + return promise.then( + (value) => { + throw new Error(`Expected a rejection, got ${JSON.stringify(value)}`); + }, + (caught: unknown) => caught + ); +} + +const realSetTimeout = globalThis.setTimeout; +const waits: number[] = []; + +describe('connection failures', () => { + let stub: TcpStub; + let client: BookStackClient; + let driver: RetryDriver; + + beforeAll(async () => { + stub = await startTcpStub(); + client = new BookStackClient( + configFor(stub.baseUrl, 0, 'transport-test-id:transport-test-secret'), + noopLogger, + new ErrorHandler(noopLogger) + ); + driver = client as unknown as RetryDriver; + + globalThis.setTimeout = (( + handler: (...args: unknown[]) => void, + ms?: number, + ...args: unknown[] + ) => { + // Retry waits are at least 250 ms; shorter timers belong to the runtime. + if (typeof ms === 'number' && ms >= 250) { + waits.push(ms); + } + return realSetTimeout(handler, 0, ...args); + }) as typeof setTimeout; + }); + + afterAll(async () => { + globalThis.setTimeout = realSetTimeout; + await stub.stop(); + resetSharedRateLimiters(); + }); + + beforeEach(() => { + stub.reset(); + waits.length = 0; + }); + + it('retries a GET whose connection dropped and returns the eventual success', async () => { + stub.plan = ['drop', 'ok']; + + const result = await client.listBooks(); + + expect(stub.requests).toHaveLength(2); + expect(result).toEqual({ data: [], total: 0 }); + expect(waits).toHaveLength(1); + expect(waits[0]).toBeGreaterThanOrEqual(BASE_DELAY_MS); + expect(waits[0]).toBeLessThanOrEqual(Math.round(BASE_DELAY_MS * (1 + JITTER_RATIO))); + }); + + for (const method of ['HEAD', 'OPTIONS']) { + it(`retries ${method} whose connection dropped`, async () => { + stub.plan = ['drop', 'ok']; + + const response = await driver.requestWithRetry({ method, url: '/books' }); + + expect(response.status).toBe(200); + expect(stub.requests.map((line) => line.split(' ')[0])).toEqual([method, method]); + }); + } + + it('gives up after the attempt cap and reports network_error', async () => { + stub.fallback = 'drop'; + + const error = await rejectionOf(client.listBooks()); + + expect(stub.requests).toHaveLength(MAX_ATTEMPTS); + expect(waits).toHaveLength(MAX_ATTEMPTS - 1); + expect(String(error)).toContain('BookStack could not be reached'); + }); + + const writes: { verb: string; call: () => Promise }[] = [ + { verb: 'POST', call: () => client.createBook({ name: 'must not be duplicated' }) }, + { verb: 'PUT', call: () => client.updateBook(5, { name: 'must not be replayed' }) }, + { verb: 'DELETE', call: () => client.deleteBook(5) }, + ]; + + for (const { verb, call } of writes) { + it(`does NOT retry ${verb} whose connection dropped`, async () => { + stub.plan = ['drop']; + stub.fallback = 'ok'; + + const error = await rejectionOf(call()); + + expect(stub.requests).toHaveLength(1); + expect(stub.requests[0]?.split(' ')[0]).toBe(verb); + expect(waits).toEqual([]); + expect(String(error)).toContain('BookStack could not be reached'); + }); + } + + it('retries a refused GET within the budget, then reports it without the address', async () => { + const port = await closedPort(); + const refused = new BookStackClient( + configFor(`http://127.0.0.1:${port}/api`, 0, 'refused-id:refused-secret'), + noopLogger, + new ErrorHandler(noopLogger) + ); + + const error = await rejectionOf(refused.listBooks()); + + expect(waits).toHaveLength(MAX_ATTEMPTS - 1); + const text = new ErrorHandler(noopLogger).toToolErrorResult(error).content[0]?.text ?? ''; + expect(text).toContain('BookStack could not be reached'); + expect(text).toContain('"type": "network_error"'); + expect(text).not.toContain('127.0.0.1'); + expect(text).not.toContain(String(port)); + expect(text).not.toContain('ECONNREFUSED'); + }); + + it('does not retry a refused POST', async () => { + const port = await closedPort(); + const refused = new BookStackClient( + configFor(`http://127.0.0.1:${port}/api`, 0, 'refused-post-id:refused-post-secret'), + noopLogger, + new ErrorHandler(noopLogger) + ); + + await expect(refused.createBook({ name: 'x' })).rejects.toThrow( + 'BookStack could not be reached' + ); + expect(waits).toEqual([]); + }); +}); + +describe('timeouts', () => { + const TIMEOUT_MS = 150; + let stub: TcpStub; + + beforeAll(async () => { + stub = await startTcpStub(); + }); + + afterAll(async () => { + await stub.stop(); + resetSharedRateLimiters(); + }); + + afterEach(() => { + stub.reset(); + }); + + it('does not retry a timed-out GET and names the configured timeout', async () => { + stub.fallback = 'hang'; + const client = new BookStackClient( + configFor(stub.baseUrl, TIMEOUT_MS, 'timeout-id:timeout-secret'), + noopLogger, + new ErrorHandler(noopLogger) + ); + + const error = await rejectionOf(client.listBooks()); + + expect(stub.requests).toHaveLength(1); + const text = new ErrorHandler(noopLogger).toToolErrorResult(error).content[0]?.text ?? ''; + expect(text).toContain(`BookStack did not respond within ${TIMEOUT_MS} ms`); + expect(text).toContain('"type": "timeout_error"'); + expect(text).not.toContain('127.0.0.1'); + expect(text).not.toContain(String(stub.port)); + }); +}); + +describe('ErrorHandler on transport failures', () => { + const handler = new ErrorHandler(noopLogger); + + /** An AxiosError as the http adapter raises it: a request went out, nothing came back. */ + function noResponse(code: string | undefined, message: string, timeout = 30_000): unknown { + return { + isAxiosError: true, + code, + message, + request: {}, + config: { method: 'get', url: '/books', timeout }, + }; + } + + const connectionFailures: { label: string; error: unknown }[] = [ + { + label: 'ECONNREFUSED', + error: noResponse('ECONNREFUSED', 'connect ECONNREFUSED 10.9.8.7:6875'), + }, + { label: 'ECONNRESET', error: noResponse('ECONNRESET', 'read ECONNRESET') }, + { label: 'EPIPE', error: noResponse('EPIPE', 'write EPIPE') }, + { + label: 'ERR_SOCKET_CONNECTION_TIMEOUT', + error: noResponse('ERR_SOCKET_CONNECTION_TIMEOUT', 'Socket connection timeout'), + }, + { label: 'socket hang up, no code', error: noResponse(undefined, 'socket hang up') }, + { + label: 'no response, unknown code', + error: noResponse('EHOSTUNREACH', 'connect EHOSTUNREACH'), + }, + ]; + + for (const { label, error } of connectionFailures) { + it(`maps ${label} to network_error and calls it retryable`, () => { + const mapped = handler.handleError(error); + + expect(mapped.message).toContain('BookStack could not be reached'); + expect((mapped.data as { type?: string }).type).toBe('network_error'); + expect(mapped.message).not.toContain('10.9.8.7'); + expect(handler.isRetryable(error)).toBe(true); + expect(handler.isRetryable(mapped)).toBe(true); + }); + } + + for (const code of ['ECONNABORTED', 'ETIMEDOUT']) { + it(`maps ${code} to timeout_error with the configured timeout and never retries it`, () => { + const error = noResponse(code, 'timeout of 12000ms exceeded', 12_000); + const mapped = handler.handleError(error); + + expect(mapped.message).toContain('BookStack did not respond within 12000 ms'); + expect((mapped.data as { type?: string }).type).toBe('timeout_error'); + expect(handler.isRetryable(error)).toBe(false); + expect(handler.isRetryable(mapped)).toBe(false); + }); + } + + it('does not treat a cancelled request as a network failure', () => { + const error = noResponse('ERR_CANCELED', 'canceled'); + + expect(handler.isRetryable(error)).toBe(false); + expect((handler.handleError(error).data as { type?: string }).type).not.toBe('network_error'); + }); +}); + +describe('connection pooling', () => { + it('shares one keep-alive agent per protocol across clients, keeping the per-client timeout', () => { + const make = (timeout: number, token: string): AxiosInstance => + ( + new BookStackClient( + configFor('http://127.0.0.1:9/api', timeout, token), + noopLogger, + new ErrorHandler(noopLogger) + ) as unknown as { client: AxiosInstance } + ).client; + + const first = make(1_000, 'pool-a-id:pool-a-secret'); + const second = make(2_000, 'pool-b-id:pool-b-secret'); + resetSharedRateLimiters(); + + expect(first.defaults.httpAgent).toBeDefined(); + expect(first.defaults.httpsAgent).toBeDefined(); + expect(second.defaults.httpAgent).toBe(first.defaults.httpAgent); + expect(second.defaults.httpsAgent).toBe(first.defaults.httpsAgent); + expect(first.defaults.httpAgent.keepAlive).toBe(true); + expect(first.defaults.httpsAgent.keepAlive).toBe(true); + expect(first.defaults.timeout).toBe(1_000); + expect(second.defaults.timeout).toBe(2_000); + }); +}); diff --git a/tests/unit/config-manager.test.ts b/tests/unit/config-manager.test.ts new file mode 100644 index 0000000..8fa1d42 --- /dev/null +++ b/tests/unit/config-manager.test.ts @@ -0,0 +1,44 @@ +import { afterEach, describe, expect, it } from 'bun:test'; +import { ConfigManager } from '../../src/config/manager'; + +const PINNED_ENV = ['BOOKSTACK_BASE_URL', 'BOOKSTACK_API_TOKEN', 'NODE_ENV', 'DEBUG'] as const; +const savedEnv = new Map(PINNED_ENV.map((key) => [key, process.env[key]])); + +afterEach(() => { + for (const [key, value] of savedEnv) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + ConfigManager.resetInstance(); +}); + +describe('ConfigManager environment handling', () => { + it('starts with a NODE_ENV outside development/production/test', () => { + process.env.BOOKSTACK_BASE_URL = 'http://127.0.0.1:9/api'; + process.env.BOOKSTACK_API_TOKEN = 'config-test-id:config-test-secret'; + process.env.NODE_ENV = 'staging'; + + ConfigManager.resetInstance(); + + expect(() => ConfigManager.getInstance()).not.toThrow(); + expect(ConfigManager.getInstance().getConfig().bookstack.apiToken).toBe( + 'config-test-id:config-test-secret' + ); + }); + + it('exposes no development section', () => { + process.env.BOOKSTACK_BASE_URL = 'http://127.0.0.1:9/api'; + process.env.BOOKSTACK_API_TOKEN = 'config-test-id:config-test-secret'; + process.env.DEBUG = 'true'; + + ConfigManager.resetInstance(); + + expect(Object.keys(ConfigManager.getInstance().getConfig()).sort()).toEqual([ + 'bookstack', + 'logging', + 'rateLimit', + 'server', + 'validation', + ]); + }); +}); diff --git a/tests/unit/error-handler.test.ts b/tests/unit/error-handler.test.ts new file mode 100644 index 0000000..8688cbb --- /dev/null +++ b/tests/unit/error-handler.test.ts @@ -0,0 +1,118 @@ +/** + * What ErrorHandler hands back to the caller, as opposed to what it logs. + * + * McpError `data` crosses the wire to the MCP client. A stack trace there tells any caller this + * server's source layout and install paths; the log keeps the frames for the operator. + */ + +import { describe, expect, it } from 'bun:test'; +import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'; +import { ErrorHandler } from '../../src/utils/errors'; +import type { Logger } from '../../src/utils/logger'; +import { PageContentError, PageStaleError } from '../../src/utils/page-content'; + +const logged: unknown[] = []; +const recordingLogger = { + debug() {}, + info() {}, + warn() {}, + error(_message: string, meta?: unknown) { + logged.push(meta); + }, +} as unknown as Logger; + +describe('ErrorHandler caller-facing data', () => { + it('sends no stack trace with an internal error, while still logging the error', () => { + const error = new Error('boom'); + logged.length = 0; + + const handled = new ErrorHandler(recordingLogger).handleError(error); + + expect(handled.code).toBe(ErrorCode.InternalError); + expect(handled.data).toEqual({ type: 'internal_error' }); + expect(JSON.stringify(handled.data)).not.toContain('error-handler.test.ts'); + expect(logged).toEqual([{ err: error }]); + }); +}); + +/** The message line and the parsed JSON hints of a tool error result. */ +function readToolError(result: ReturnType): { + message: string; + data: Record | undefined; + text: string; +} { + expect(result.isError).toBe(true); + expect(result.content).toHaveLength(1); + expect(result.content[0]?.type).toBe('text'); + const text = result.content[0]?.text ?? ''; + const split = text.indexOf('\n\n'); + if (split === -1) { + return { message: text, data: undefined, text }; + } + return { + message: text.slice(0, split), + data: JSON.parse(text.slice(split + 2)) as Record, + text, + }; +} + +describe('ErrorHandler tool error results', () => { + const handler = new ErrorHandler(recordingLogger); + + it('carries recovery hints from a page edit that could not be applied', () => { + const result = handler.toToolErrorResult( + new PageContentError('Section "Setpu" not found', { + available_sections: ['Intro', 'Setup'], + found_with_different_whitespace: 'Set up', + }) + ); + + const { message, data } = readToolError(result); + expect(message).toBe('Section "Setpu" not found'); + expect(data).toEqual({ + type: 'page_content_error', + available_sections: ['Intro', 'Setup'], + found_with_different_whitespace: 'Set up', + }); + }); + + it('carries the concurrent-modification type and its details', () => { + const result = handler.toToolErrorResult( + new PageStaleError('Page changed since it was read', { matched_sections: ['Setup'] }) + ); + + const { data } = readToolError(result); + expect(data).toEqual({ type: 'concurrent_modification', matched_sections: ['Setup'] }); + }); + + it('drops the JSON-RPC code prefix from the message', () => { + const result = handler.toToolErrorResult( + new McpError(ErrorCode.InvalidParams, 'Validation failed', { type: 'validation_error' }) + ); + + expect(readToolError(result).message).toBe('Validation failed'); + }); + + it('never puts a stack trace or the upstream request path in the text', () => { + const internal = new Error('boom'); + const upstream = new McpError(ErrorCode.InvalidRequest, 'Requested resource not found', { + type: 'not_found_error', + status: 404, + url: '/books/999', + details: { error: { message: 'Book not found' }, stack: internal.stack }, + stack: internal.stack, + }); + + for (const error of [internal, upstream]) { + const { text } = readToolError(handler.toToolErrorResult(error)); + expect(text).not.toContain('error-handler.test.ts'); + expect(text).not.toContain('"stack"'); + expect(text).not.toContain('/books/999'); + } + expect(readToolError(handler.toToolErrorResult(upstream)).data).toEqual({ + type: 'not_found_error', + status: 404, + details: { error: { message: 'Book not found' } }, + }); + }); +}); diff --git a/tests/unit/healthcheck.test.ts b/tests/unit/healthcheck.test.ts new file mode 100644 index 0000000..bc043f8 --- /dev/null +++ b/tests/unit/healthcheck.test.ts @@ -0,0 +1,78 @@ +/** + * The container health probes in Dockerfile and docker-compose.yml, run for real. + * + * Each probe is a `bun -e` one-liner. It is extracted from the file it ships in and run + * against a local server on an arbitrary port, so a probe that ignores SERVER_PORT fails. + */ + +import { afterAll, beforeAll, describe, expect, it } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const ROOT = join(import.meta.dir, '..', '..'); + +function dockerfileProbe(): string { + const text = readFileSync(join(ROOT, 'Dockerfile'), 'utf8'); + const match = /HEALTHCHECK[^\n]*\\\n\s*CMD bun -e "(.+)"\s*$/m.exec(text); + if (!match?.[1]) throw new Error('Dockerfile HEALTHCHECK bun probe not found'); + return match[1]; +} + +function composeProbe(): string { + const text = readFileSync(join(ROOT, 'docker-compose.yml'), 'utf8'); + const line = text.split('\n').find((l) => /^\s*test: \["CMD", "bun", "-e"/.test(l)); + if (!line) throw new Error('docker-compose.yml bun healthcheck not found'); + const command = JSON.parse(line.replace(/^\s*test:\s*/, '')) as string[]; + const script = command[3]; + if (!script) throw new Error('docker-compose.yml healthcheck has no script'); + return script; +} + +async function runProbe(script: string, env: Record): Promise { + const proc = Bun.spawn({ + cmd: [process.execPath, '-e', script], + env: { ...process.env, ...env }, + stdout: 'ignore', + stderr: 'ignore', + }); + return proc.exited; +} + +describe('container health probes', () => { + let server: ReturnType; + let status = 200; + + beforeAll(() => { + server = Bun.serve({ + port: 0, + hostname: '127.0.0.1', + fetch(request: Request): Response { + const path = new URL(request.url).pathname; + return path === '/health' + ? new Response('{}', { status }) + : new Response('', { status: 404 }); + }, + }); + }); + + afterAll(() => { + server.stop(true); + }); + + for (const [label, probe] of [ + ['Dockerfile', dockerfileProbe], + ['docker-compose.yml', composeProbe], + ] as const) { + it(`${label} probes the port in SERVER_PORT`, async () => { + status = 200; + expect(await runProbe(probe(), { SERVER_PORT: String(server.port) })).toBe(0); + + status = 503; + expect(await runProbe(probe(), { SERVER_PORT: String(server.port) })).toBe(1); + }); + } + + it('ships the same probe in both files', () => { + expect(composeProbe()).toBe(dockerfileProbe()); + }); +}); diff --git a/tests/unit/list-params.test.ts b/tests/unit/list-params.test.ts index 6af9360..7b18cbb 100644 --- a/tests/unit/list-params.test.ts +++ b/tests/unit/list-params.test.ts @@ -241,7 +241,6 @@ describe('list filters as BookStack receives them', () => { rateLimit: { requestsPerMinute: 60_000, burstLimit: 10_000 }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; const client = new BookStackClient(config, noopLogger, new ErrorHandler(noopLogger)); diff --git a/tests/unit/page-content.test.ts b/tests/unit/page-content.test.ts index d8d2e6b..d220b1f 100644 --- a/tests/unit/page-content.test.ts +++ b/tests/unit/page-content.test.ts @@ -25,6 +25,7 @@ import { buildOutline, containsNormalized, countOccurrences, + findSection, grepContent, insertContent, PageContentError, @@ -236,6 +237,150 @@ describe('buildOutline', () => { it('returns an empty outline for a page without headings', () => { expect(buildOutline('

Just a paragraph

', 'html')).toEqual([]); }); + + it('keeps the markdown heading rules for closing hashes, blank titles and line breaks', () => { + const markdown = [ + '# Title ##', + '## ###', + '####### seven', + '#nospace', + '# ', + '# ', + '# a #b', + '#\tTab title\t#', + '### foo# ', + ].join('\n'); + + expect( + buildOutline(markdown, 'markdown').map(({ level, text, offset }) => [level, text, offset]) + ).toEqual([ + [1, 'Title', 0], + [2, '#', 11], + [1, '', 41], + [1, 'a #b', 48], + [1, 'Tab title', 55], + [3, 'foo#', 69], + ]); + expect( + buildOutline('a\r# CR heading\r\n## CRLF\u2028### LS', 'markdown').map(({ text, offset }) => [ + text, + offset, + ]) + ).toEqual([ + ['CR heading', 2], + ['CRLF', 16], + ['LS', 24], + ]); + }); + + it('ignores heading-like lines inside fenced code blocks', () => { + const markdown = [ + '# Install', + '```bash', + '# not a heading', + '```', + ' ~~~~', + '## inside a tilde fence', + '~~~', + '```', + '~~~~', + '## Usage', + '````', + '```', + '# a shorter fence does not close it', + '````', + ' ```', + '## After an indented fence-lookalike', + '``` a`b', + '## After a backtick fence with a backtick in its info string', + '~~~', + '# an unclosed fence runs to the end', + ].join('\n'); + + expect(buildOutline(markdown, 'markdown').map(({ text }) => text)).toEqual([ + 'Install', + 'Usage', + 'After an indented fence-lookalike', + 'After a backtick fence with a backtick in its info string', + ]); + }); + + it('only maps html headings closed by their own level', () => { + const html = '

One

Two

never closedno

Four

'; + + expect( + buildOutline(html, 'html').map(({ level, text, offset }) => [level, text, offset]) + ).toEqual([ + [2, 'Two', 12], + [4, 'Four', 63], + ]); + }); +}); + +/** + * Inputs that each blocked the event loop for seconds when outlining and comparison used + * backtracking regular expressions. Every case must now finish well inside the budget. + */ +describe('pathological page content', () => { + const BUDGET_MS = 200; + + function elapsedMs(work: () => unknown): number { + const startedAt = performance.now(); + work(); + return performance.now() - startedAt; + } + + it('outlines a markdown heading padded with tens of thousands of blanks', () => { + for (const source of [`# a${' '.repeat(40_000)}x`, `# a${' \t'.repeat(20_000)}x\n## b`]) { + expect(elapsedMs(() => buildOutline(source, 'markdown'))).toBeLessThan(BUDGET_MS); + } + }); + + it('outlines markdown made of long fence runs', () => { + for (const source of ['`'.repeat(80_000), '```\n'.repeat(20_000), `~~~${'`'.repeat(80_000)}`]) { + expect(elapsedMs(() => buildOutline(source, 'markdown'))).toBeLessThan(BUDGET_MS); + } + }); + + it('outlines html made of unterminated heading tags', () => { + for (const source of [''.repeat(20_000), '

buildOutline(source, 'html'))).toBeLessThan(BUDGET_MS); + } + }); + + it('normalises a long run of unmatched "<" for comparison', () => { + const stray = '<'.repeat(80_000); + + expect(elapsedMs(() => containsNormalized(stray, 'x', 'html'))).toBeLessThan(BUDGET_MS); + }); + + it('bounds the whitespace-tolerant search for a long anchor', () => { + const page = 'word '.repeat(20_000); + const anchor = `${'word '.repeat(10_000)}missing`; + + let error: unknown; + const ms = elapsedMs(() => { + error = thrownBy(() => applyEdits(page, [{ old_string: anchor, new_string: 'x' }])); + }); + + expect(ms).toBeLessThan(BUDGET_MS); + expect((error as PageContentError).details?.found_with_different_whitespace).toBeNull(); + }); + + it('still finds an anchor of up to 256 words with different whitespace', () => { + const words = (count: number) => Array.from({ length: count }, (_, i) => `w${i}`); + const report = (count: number) => + ( + thrownBy(() => + applyEdits(words(count).join('\n '), [ + { old_string: words(count).join(' '), new_string: 'x' }, + ]) + ) as PageContentError + ).details?.found_with_different_whitespace; + + expect(report(256)).toStartWith('w0\n w1\n w2'); + expect(report(257)).toBeNull(); + }); }); describe('grepContent', () => { @@ -250,6 +395,34 @@ describe('grepContent', () => { expect(matches[0].offset).toBe(source.indexOf('Line one')); }); + it('returns an exact context, bounded on each side of the match, with truncation flags', () => { + const page = `${'a'.repeat(50)}

retention period of 6 months

${'z'.repeat(50)}`; + + const [clipped] = grepContent(page, 'retention period of 6 months', { + contextChars: 5, + }).matches; + const [whole] = grepContent(page, 'retention period', { contextChars: 500 }).matches; + + // No ellipses: the excerpt must be pasteable as an old_string anchor. + expect(clipped).toMatchObject({ + context: 'aa

retention period of 6 months

z', + context_truncated_start: true, + context_truncated_end: true, + }); + expect(whole).toMatchObject({ + context: page, + context_truncated_start: false, + context_truncated_end: false, + }); + }); + + it('never cuts a surrogate pair at the edge of the context', () => { + const source = '\u{1F600}x\u{1F600}'; + + expect(grepContent(source, 'x', { contextChars: 1 }).matches[0].context).toBe('x'); + expect(grepContent(source, 'x', { contextChars: 2 }).matches[0].context).toBe(source); + }); + it('honours maxMatches while still reporting the true total', () => { // A truncated result that under-reported the total would read as "there is one match", // and a caller would anchor on it believing it unique. @@ -274,6 +447,8 @@ describe('grepContent', () => { offset: 1, match: 'retention period', context: unicodeSource, + context_truncated_start: false, + context_truncated_end: false, }, ]); }); @@ -295,7 +470,7 @@ describe('insertContent', () => { const markdown = '# Title\n\nIntro\n\n## Measures\n\nExisting text\n\n## Other\n\nEnd'; it('appends at the end of the page', () => { - const result = insertContent(markdown, 'New sentence', { writeField: 'markdown' }); + const { result } = insertContent(markdown, 'New sentence', { writeField: 'markdown' }); expect(result.endsWith('End\n\nNew sentence')).toBe(true); }); @@ -303,7 +478,7 @@ describe('insertContent', () => { it('appends at the end of a named section, before the next heading', () => { // Section targeting only works if the text lands inside the section it was addressed to, // instead of being pushed past its boundary into the following one. - const result = insertContent(markdown, 'New sentence', { + const { result } = insertContent(markdown, 'New sentence', { writeField: 'markdown', section: 'Measures', }); @@ -312,7 +487,7 @@ describe('insertContent', () => { }); it('inserts directly after a section heading with position: start', () => { - const result = insertContent(markdown, 'New sentence', { + const { result } = insertContent(markdown, 'New sentence', { writeField: 'markdown', section: 'Measures', position: 'start', @@ -323,7 +498,7 @@ describe('insertContent', () => { it('inserts after an html section heading', () => { const html = '

Measures

Old

Other

End

'; - const result = insertContent(html, '

New

', { + const { result } = insertContent(html, '

New

', { writeField: 'html', section: 'Measures', position: 'start', @@ -334,7 +509,7 @@ describe('insertContent', () => { }); it('matches a section name case-insensitively', () => { - const result = insertContent(markdown, 'New sentence', { + const { result } = insertContent(markdown, 'New sentence', { writeField: 'markdown', section: 'measures', }); @@ -342,6 +517,88 @@ describe('insertContent', () => { expect(result).toContain('Existing text\n\nNew sentence'); }); + it('appends to a section after its code block, not inside it', () => { + const source = '# Install\n\n```bash\n# comment\nnpm install\n```\n\n# Usage\n\nRun it'; + + const { result } = insertContent(source, 'Then restart.', { + writeField: 'markdown', + section: 'Install', + }); + + expect(result).toBe( + '# Install\n\n```bash\n# comment\nnpm install\n```\n\nThen restart.\n\n# Usage\n\nRun it' + ); + }); + + describe('section boundaries and matching', () => { + const changelog = [ + '# Doc', + '## Changelog', + 'Intro', + '### 2026', + 'Entry A', + '### 2025', + 'Entry B', + '## Other', + 'End', + ].join('\n'); + + it('ends a section at the next heading of the same or higher level', () => { + const { result } = insertContent(changelog, 'Entry C', { + writeField: 'markdown', + section: 'Changelog', + separator: '\n', + }); + + expect(result).toContain('### 2025\nEntry B\nEntry C\n## Other'); + }); + + it('sizes an outline section to include its subsections', () => { + const headings = buildOutline(changelog, 'markdown'); + const section = headings.find((heading) => heading.text === 'Changelog'); + const other = headings.find((heading) => heading.text === 'Other'); + + expect((section?.offset ?? 0) + (section?.length ?? 0)).toBe(other?.offset ?? -1); + expect(headings.at(-1)?.text).toBe('Other'); + expect(headings[0].length).toBe(changelog.length); + }); + + it('prefers an exact heading over headings that merely contain the name', () => { + const source = '# Setup notes\n\nA\n\n# Setup\n\nB'; + + expect(findSection(source, ' setup ', 'markdown').text).toBe('Setup'); + }); + + it('accepts a substring that matches exactly one heading', () => { + const source = '# Setup notes\n\nA\n\n# Usage\n\nB'; + + expect(findSection(source, 'notes', 'markdown').text).toBe('Setup notes'); + }); + + it('refuses a name several headings match, and lists them', () => { + const source = '# Install on Linux\n\nA\n\n# Install on macOS\n\nB\n\n# Usage\n\nC'; + + const error = thrownBy(() => findSection(source, 'install', 'markdown')); + + expect(error).toBeInstanceOf(PageContentError); + expect((error as PageContentError).details?.matching_sections).toEqual([ + 'Install on Linux', + 'Install on macOS', + ]); + }); + + it('refuses a name two identical headings share', () => { + const error = thrownBy(() => + insertContent('## Notes\n\nA\n\n## Notes\n\nB', 'x', { + writeField: 'markdown', + section: 'notes', + }) + ); + + expect((error as PageContentError).details?.matching_sections).toEqual(['Notes', 'Notes']); + }); + }); + it('lists the real section names when the section is unknown', () => { const error = thrownBy(() => insertContent(markdown, 'x', { writeField: 'markdown', section: 'Absent' }) diff --git a/tests/unit/pages.test.ts b/tests/unit/pages.test.ts index 144815f..de17bb6 100644 --- a/tests/unit/pages.test.ts +++ b/tests/unit/pages.test.ts @@ -73,7 +73,13 @@ const page = (overrides: Partial = {}): PageWithContent => ({ interface GrepResult { total_matches: number; total_chars: number; - matches: Array<{ match: string; offset: number }>; + matches: Array<{ + match: string; + offset: number; + context: string; + context_truncated_start: boolean; + context_truncated_end: boolean; + }>; content?: string; } interface OutlineResult { @@ -82,7 +88,8 @@ interface OutlineResult { } interface WriteResult { written: boolean; - verified: boolean; + verified: boolean | null; + note?: string; unverified_fragment_count: number; revision_count: number; updated_at: string; @@ -218,6 +225,38 @@ describe('PageTools partial editing', () => { }); }); + describe('grep anchor round trip', () => { + it('accepts a grep context verbatim as the old_string of an edit', async () => { + const expected = '

First paragraph

Third paragraph

'; + mockClient.getPage + .mockResolvedValueOnce(page()) + .mockResolvedValueOnce(page()) + .mockResolvedValueOnce(page({ raw_html: expected })); + mockClient.updatePage.mockResolvedValue(page()); + + const grep = await call('bookstack_pages_read', { + id: PAGE_ID, + grep: 'Second', + context: 5, + }); + const [hit] = grep.matches; + + expect(hit).toMatchObject({ + context: 'p>

Second para', + context_truncated_start: true, + context_truncated_end: true, + }); + + const result = await call('bookstack_pages_edit', { + id: PAGE_ID, + edits: [{ old_string: hit.context, new_string: hit.context.replace(hit.match, 'Third') }], + }); + + expect(mockClient.updatePage).toHaveBeenCalledWith(PAGE_ID, { html: expected }); + expect(result.verified).toBe(true); + }); + }); + describe('bookstack_pages_outline', () => { it('returns headings and no page content', async () => { mockClient.getPage.mockResolvedValue( @@ -470,6 +509,64 @@ describe('PageTools partial editing', () => { expect(result.unverified_fragment_count).toBe(1); }); + it('verifies a formatting-only edit against the stored markup', async () => { + mockClient.getPage + .mockResolvedValueOnce(page({ raw_html: '

x rest

' })) + .mockResolvedValueOnce(page({ raw_html: '

x rest

' })); + mockClient.updatePage.mockResolvedValue(page()); + + const result = await call('bookstack_pages_edit', { + id: PAGE_ID, + edits: [{ old_string: 'x', new_string: 'x' }], + }); + + expect(mockClient.updatePage).toHaveBeenCalledWith(PAGE_ID, { + html: '

x rest

', + }); + expect(result.verified).toBe(true); + expect(result.unverified_fragment_count).toBe(0); + }); + + it('verifies an href change against the stored markup', async () => { + const link = (url: string) => `

link

`; + mockClient.getPage + .mockResolvedValueOnce(page({ raw_html: link('https://a.example/old') })) + .mockResolvedValueOnce( + page({ raw_html: link('https://a.example/new').replace('

', '

x rest

'; + mockClient.getPage + .mockResolvedValueOnce(page({ raw_html: stored })) + .mockResolvedValueOnce(page({ raw_html: stored })); + mockClient.updatePage.mockResolvedValue(page()); + + const result = await call('bookstack_pages_edit', { + id: PAGE_ID, + edits: [{ old_string: 'x', new_string: 'x' }], + }); + + expect(result.written).toBe(true); + expect(result.verified).toBe(false); + expect(result.unverified_fragment_count).toBe(1); + }); + it('rejects an empty edit list at the schema boundary', async () => { await expect( tool('bookstack_pages_edit').handler({ id: PAGE_ID, edits: [] }) @@ -516,6 +613,73 @@ describe('PageTools partial editing', () => { expect(result.section).toBe('Section A'); }); + it('reports the heading it matched, on a dry run too', async () => { + const withSections = '

Section A

Text

Section B

End

'; + mockClient.getPage.mockResolvedValue(page({ raw_html: withSections })); + + const result = await call( + 'bookstack_pages_append', + { id: PAGE_ID, content: '

Added

', section: 'section a', dry_run: true } + ); + + expect(result.section).toBe('section a'); + expect(result.section_matched).toBe('Section A'); + expect(mockClient.updatePage).not.toHaveBeenCalled(); + }); + + it('appends after a section heading inside a fenced code block, and after subsections', async () => { + const markdown = + '## Changelog\n\n```bash\n# comment\n```\n\n### 2026\n\nEntry\n\n## Other\n\nEnd'; + const expected = + '## Changelog\n\n```bash\n# comment\n```\n\n### 2026\n\nEntry\n\nAdded\n\n## Other\n\nEnd'; + mockClient.getPage + .mockResolvedValueOnce(page({ editor: 'markdown', markdown })) + .mockResolvedValueOnce(page({ editor: 'markdown', markdown: expected })); + mockClient.updatePage.mockResolvedValue(page()); + + const result = await call( + 'bookstack_pages_append', + { id: PAGE_ID, content: 'Added', section: 'changelog' } + ); + + expect(mockClient.updatePage).toHaveBeenCalledWith(PAGE_ID, { markdown: expected }); + expect(result.section_matched).toBe('Changelog'); + expect(result.verified).toBe(true); + }); + + it('reports a write it could not re-read as unverified instead of throwing', async () => { + // Throwing here would make a retrying client append the content a second time. + mockClient.getPage + .mockResolvedValueOnce(page()) + .mockRejectedValueOnce(new Error('rate limit retries exhausted')); + mockClient.updatePage.mockResolvedValue( + page({ updated_at: '2026-01-03T00:00:00.000000Z', revision_count: 8 }) + ); + + const result = await call('bookstack_pages_append', { + id: PAGE_ID, + content: '

New sentence

', + }); + + expect(mockClient.updatePage).toHaveBeenCalledTimes(1); + expect(result.written).toBe(true); + expect(result.verified).toBeNull(); + expect(result.note).toMatch(/Do not retry/); + expect(result.updated_at).toBe('2026-01-03T00:00:00.000000Z'); + expect(result.revision_count).toBe(8); + expect(mockLogger.warn).toHaveBeenCalled(); + }); + + it('still throws when the write itself fails', async () => { + mockClient.getPage.mockResolvedValue(page()); + mockClient.updatePage.mockRejectedValue(new Error('write refused')); + + await expect( + tool('bookstack_pages_append').handler({ id: PAGE_ID, content: '

New sentence

' }) + ).rejects.toThrow(/write refused/); + expect(mockClient.getPage).toHaveBeenCalledTimes(1); + }); + it('fails with the available headings when the section is unknown', async () => { mockClient.getPage.mockResolvedValue(page({ raw_html: '

Section A

Text

' })); diff --git a/tests/unit/rate-limit.test.ts b/tests/unit/rate-limit.test.ts index 2e39386..ba8f1c9 100644 --- a/tests/unit/rate-limit.test.ts +++ b/tests/unit/rate-limit.test.ts @@ -131,7 +131,6 @@ function testConfig(baseUrl: string, overrides: Partial = { rateLimit: { requestsPerMinute: 60, burstLimit: 1, ...overrides }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; } diff --git a/tests/unit/retry.test.ts b/tests/unit/retry.test.ts index 2b2b60a..170cf04 100644 --- a/tests/unit/retry.test.ts +++ b/tests/unit/retry.test.ts @@ -179,7 +179,6 @@ describe('BookStackClient retry policy', () => { rateLimit: { requestsPerMinute: 60_000, burstLimit: 10_000 }, validation: { enabled: true, strictMode: true }, logging: { level: 'error', format: 'pretty' }, - development: { nodeEnv: 'test', debug: false }, }; client = new BookStackClient(config, noopLogger, new ErrorHandler(noopLogger)); diff --git a/tests/unit/server-info.test.ts b/tests/unit/server-info.test.ts new file mode 100644 index 0000000..92d8aa8 --- /dev/null +++ b/tests/unit/server-info.test.ts @@ -0,0 +1,42 @@ +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import { ConfigManager } from '../../src/config/manager'; +import { ServerInfoTools } from '../../src/tools/server-info'; +import type { MCPResource, MCPServerInfo, MCPTool } from '../../src/types'; +import { Logger } from '../../src/utils/logger'; + +const PINNED_ENV = ['BOOKSTACK_BASE_URL', 'BOOKSTACK_API_TOKEN'] as const; +const savedEnv = new Map(PINNED_ENV.map((key) => [key, process.env[key]])); + +beforeEach(() => { + process.env.BOOKSTACK_BASE_URL = 'http://127.0.0.1:9/api'; + process.env.BOOKSTACK_API_TOKEN = 'server-info-id:server-info-secret'; + ConfigManager.resetInstance(); +}); + +afterEach(() => { + for (const [key, value] of savedEnv) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + ConfigManager.resetInstance(); +}); + +describe('bookstack_server_info authentication', () => { + it('names both supported authentication methods', async () => { + const tools = new ServerInfoTools( + Logger.getInstance(), + new Map(), + new Map() + ); + const tool = tools.getTools().find((t) => t.name === 'bookstack_server_info'); + if (!tool) throw new Error('bookstack_server_info tool is missing'); + + const info = (await tool.handler({})) as unknown as MCPServerInfo; + + expect(info.capabilities.authentication.required).toBe(true); + expect(info.capabilities.authentication.methods).toEqual([ + 'BookStack API token', + 'OAuth access token (HTTP transport with MCP_AUTH_MODE=oauth)', + ]); + }); +}); diff --git a/tests/unit/upload-guard.test.ts b/tests/unit/upload-guard.test.ts new file mode 100644 index 0000000..23abc39 --- /dev/null +++ b/tests/unit/upload-guard.test.ts @@ -0,0 +1,120 @@ +/** + * The `file_path` upload guard, on a real filesystem. + * + * `file_path` makes this process read a local file and upload it into BookStack, so whoever + * steers the tool arguments - including text planted in a page an agent later reads - can + * aim it at any file the process can open. The guard therefore holds under every transport, + * stdio included, and its refusals say nothing about where anything on the server lives. + */ + +import { afterAll, afterEach, beforeAll, describe, expect, it } from 'bun:test'; +import { mkdir, mkdtemp, realpath, rm, symlink, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { readGuardedUploadFile } from '../../src/api/client'; + +const GUARD_ENV = ['MCP_TRANSPORT', 'BOOKSTACK_UPLOAD_ROOT'] as const; +const savedEnv = new Map(); + +let base: string; +let uploadRoot: string; +let outside: string; +let inside: string; + +function setGuardEnv(transport: string, root: string | undefined): void { + process.env.MCP_TRANSPORT = transport; + if (root === undefined) { + delete process.env.BOOKSTACK_UPLOAD_ROOT; + } else { + process.env.BOOKSTACK_UPLOAD_ROOT = root; + } +} + +async function refusal(filePath: string): Promise { + try { + await readGuardedUploadFile(filePath); + } catch (error) { + return error as Error; + } + throw new Error('file_path was read when it should have been refused'); +} + +beforeAll(async () => { + for (const key of GUARD_ENV) { + savedEnv.set(key, process.env[key]); + } + + base = await realpath(await mkdtemp(join(tmpdir(), 'upload-guard-'))); + uploadRoot = join(base, 'uploads'); + await mkdir(uploadRoot); + inside = join(uploadRoot, 'inside.txt'); + await writeFile(inside, 'inside the root'); + outside = join(base, 'outside-secret.txt'); + await writeFile(outside, 'outside the root'); + await symlink(outside, join(uploadRoot, 'link-out.txt')); +}); + +afterEach(() => { + for (const key of GUARD_ENV) { + const value = savedEnv.get(key); + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } +}); + +afterAll(async () => { + await rm(base, { recursive: true, force: true }); +}); + +describe('file_path upload guard', () => { + for (const transport of ['stdio', 'http']) { + it(`refuses file_path under ${transport} when BOOKSTACK_UPLOAD_ROOT is unset`, async () => { + setGuardEnv(transport, undefined); + + const error = await refusal(outside); + + expect(error.message).toContain('BOOKSTACK_UPLOAD_ROOT'); + expect(error.message).not.toContain(base); + }); + + it(`reads a file inside BOOKSTACK_UPLOAD_ROOT under ${transport}`, async () => { + setGuardEnv(transport, uploadRoot); + + expect((await readGuardedUploadFile(inside)).toString()).toBe('inside the root'); + }); + + it(`refuses escapes and unreadable paths under ${transport} with one generic message`, async () => { + setGuardEnv(transport, uploadRoot); + + const messages = new Set(); + for (const candidate of [ + join(uploadRoot, '..', 'outside-secret.txt'), + join(uploadRoot, 'link-out.txt'), + join(uploadRoot, 'missing.txt'), + outside, + uploadRoot, + ]) { + const { message } = await refusal(candidate); + // Neither the resolved path nor the root: a refusal is not a map of the server. + expect(message).not.toContain(base); + messages.add(message); + } + + // Identical wording, so a refusal cannot reveal whether a path outside the root exists. + expect(messages.size).toBe(1); + }); + } + + it('does not name a BOOKSTACK_UPLOAD_ROOT that cannot be resolved', async () => { + const missingRoot = join(base, 'no-such-root'); + setGuardEnv('http', missingRoot); + + const { message } = await refusal(inside); + + expect(message).toContain('BOOKSTACK_UPLOAD_ROOT'); + expect(message).not.toContain(base); + }); +}); From 1f9de40c9979aa650f1652c01201858ddff55b70 Mon Sep 17 00:00:00 2001 From: gusocegodk Date: Tue, 6 Oct 2026 13:29:09 +0200 Subject: [PATCH 2/2] test(transport): read request bodies in the BookStack stub 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. --- tests/transport/stub-bookstack.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/transport/stub-bookstack.ts b/tests/transport/stub-bookstack.ts index 2f00e03..1b76c5b 100644 --- a/tests/transport/stub-bookstack.ts +++ b/tests/transport/stub-bookstack.ts @@ -116,7 +116,9 @@ export function startBookStackStub(): BookStackStub { const server = Bun.serve({ port: 0, hostname: '127.0.0.1', - fetch(request: Request): Response { + async fetch(request: Request): Promise { + // Read every body, as a real server does, so a reused keep-alive connection stays in sync. + await request.arrayBuffer(); const url = new URL(request.url); const authorization = request.headers.get('authorization') ?? undefined; const path = url.pathname.replace(/^\/api/, '');