Skip to content

fix(knowledge): keep PATCH category moves inside knowledge/ - #469

Merged
gloryfromca merged 4 commits into
mainfrom
fix/knowledge-patch-category-traversal
Sep 30, 2026
Merged

gloryfromca merged 4 commits into
mainfrom
fix/knowledge-patch-category-traversal

Conversation

@dani1005

Copy link
Copy Markdown
Member

Summary

PATCH /knowledge/documents/{doc_id} sanitized a new category_id with a private copy of the dirname rule (_safe_category) that lacked the . / .. fallback of the shared sanitize_dirname. With category_id: ".." the document directory was moved from knowledge/<category>/ up into the project directory, where the md-scan fallback no longer finds it (and an index rebuild from markdown would drop it). Reported privately; impact is limited to data the caller can already modify through the API, so it is handled as hardening per the threat model in SECURITY.md.

  • Fix (service/knowledge.py): drop _safe_category; the move target is knowledge_dir(app, project) / sanitize_dirname(category, "Others") / <doc dir>, so . / .. fall back to Others exactly as on create. The resolved target is asserted to stay inside knowledge/ before any directory is created or moved (PathTraversalError otherwise). A document an earlier move left outside is moved back on its next category change.
  • SECURITY.md: supported versions 1.2.x → 1.4.x; add a "what counts as a vulnerability" line under Scope.
  • CHANGELOG.md: [Unreleased] → Fixed entry.

An audit of the other places where external input becomes a path segment (app/project/sender ids, upload filename, create-time category/title/topic, skill/reference/script names, profile writes) found no other instance; they all go through PathSafeId, sanitize_dirname, or MarkdownWriter._ensure_within_root.

Area

  • Architecture method
  • Benchmark
  • Use case
  • Documentation
  • Developer experience
  • CI, build, or release

Verification

make lint                     # ruff, import-linter (4 contracts kept), asset/size/datetime/openapi/docs gates
uv run pytest tests/unit      # 2618 passed, 4 skipped
uv run pytest tests/integration  # 186 passed, 5 skipped, 7 deselected
make package                  # wheel/sdist smoke OK

New tests in tests/unit/test_service/test_knowledge_crud.py (real filesystem): category Research Notes / .. / . stay in knowledge/; an escaped doc is repaired; a category dir symlinked outside knowledge/ raises PathTraversalError. The .., . and symlink cases fail against the previous code.

Checklist

  • I kept the change scoped to the relevant area.
  • I am opening this from a separate branch, not pushing directly to main.
  • I updated docs, examples, or setup notes when behavior changed.
  • I added or updated tests when the change affects behavior.
  • I did not commit secrets, .env files, dependency folders, or generated output.
  • Active relative links in Markdown files resolve.

Notes for Reviewers

  • The stored category_id metadata keeps the raw value (as on create); only the directory segment is sanitized.
  • _require_knowledge_capabilities is a feature-tier gate (embed + rerank), not authorization, so PATCH intentionally stays without it.
  • The SECURITY.md scope wording is a policy statement — please review it as such.

By submitting this pull request, I agree that my contribution is licensed under
the Apache License 2.0.

🤖 Generated with Claude Code

dani1005 and others added 3 commits September 28, 2026 16:27
PATCH /knowledge/documents/{doc_id} sanitized the new category with a
private copy of the dirname rule that lacked the "." / ".." fallback, so
category_id ".." moved the document directory out of knowledge/ into
the project directory, where the md scan no longer finds it.

Route the category through the shared sanitize_dirname (the rule the
create path already uses, so "." / ".." fall back to "Others"), build
the target from the project's knowledge_dir, and assert the resolved
target stays inside it before any directory is created or moved. A
document an earlier move left outside knowledge/ is moved back on its
next category change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The supported-versions table still named 1.2.x as current; 1.4.x is the
live line. Also state where the advisory line sits: issues that let
untrusted input reach beyond what the caller can already touch get an
advisory, while issues a trusted caller can trigger only against its
own data are fixed as hardening and noted in the release notes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@gloryfromca
gloryfromca merged commit 6aff735 into main Sep 30, 2026
10 of 11 checks passed
@gloryfromca
gloryfromca deleted the fix/knowledge-patch-category-traversal branch September 30, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants