Skip to content

security: gate removeBookPath* on root-folder containment check (defense in depth) #865

@vavallee

Description

@vavallee

Why

`removeBookPath` / `removeBookPathScoped` (internal/api/books.go) trust that the path `p` they receive falls under one of the admin-configured root folders. Today that's true because every writer (`importer`, `calibre` import, ABS sync, library scan) computes paths from root + sanitized author/title via `importer.sanitizePath`, which strips traversal components (`..`, `/`, `\`, etc.). So the current behaviour is safe.

But the code has no enforcement of this invariant. If a future writer skips the sanitizer, or a DB row is hand-edited, or there's an SQL-injection bug we don't know about, `removeBookPath` will happily `os.Remove` whatever path the DB says — including files and empty directories anywhere on disk the Bindery process can write.

gosec's G703 (path-traversal taint analysis) flags this — currently suppressed with `//nosec G304 G703` comments at lines 524, 530, 538. The suppressions reflect the current state, not a defense.

What

Before any `os.Remove` / `os.RemoveAll` in `removeBookPathScoped`:

  1. Resolve `p` to its absolute, symlink-free form (`filepath.EvalSymlinks` then `filepath.Abs`).
  2. Compare against the union of:
    • the `root_folders` table (admin-configured library roots)
    • `BINDERY_LIBRARY_DIR` env var (legacy single-root fallback)
  3. Refuse with a logged error if `p` doesn't resolve to a path under any of those roots.

Same gate applies to the `os.Remove(parent)` empty-directory cleanup at line 538 — `parent` must also be under a root.

Constraints

  • Must not break the existing call sites — current callers expect a non-error return when the file simply doesn't exist (`os.IsNotExist`). Containment-failure should be a distinct error type the caller can log but the user-facing response should still progress (don't fail the whole `/api/v1/book/{id}` delete because one path failed the gate).
  • The root-folders repo isn't currently plumbed into `bookHandler`. Either inject it via the handler constructor or pass the resolved root list to the remove helpers as a parameter.
  • Symlink resolution can be slow on first call. Cache the root list on the handler if needed.

Out of scope

  • Changing the admin-vs-user role for who can call `/api/v1/book/{id}?deleteFiles=true`. The current model intentionally lets any authenticated user manage the shared library — see auth gating in `cmd/bindery/main.go:687-703`.
  • gosec G304/G703 suppressions remain in place; the gate replaces `//nosec` rationale but the comments stay so the scanner stays quiet.

Test plan

  • Unit test: path under root → allowed, path outside root → refused with clear error.
  • Unit test: symlinked path that resolves outside root → refused.
  • Unit test: empty-dir cleanup on parent outside root → refused.
  • Integration test: `DELETE /api/v1/book/{id}?deleteFiles=true` against a fixture with one in-root file and one out-of-root file → in-root deleted, out-of-root left, response 200.

Related

  • Suppressions: `internal/api/books.go:524,530,538`
  • Sanitizer: `internal/importer/renamer.go:489` `sanitizePath`

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestsecuritySecurity-relevant defect

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions