deer-flow/backend/docs/rfc-extract-shared-modules.md

RFC: Extract Shared Skill Installer and Upload Manager into Harness

1. Problem

Gateway (app/gateway/routers/skills.py, uploads.py) and Client (deerflow/client.py) each independently implement the same business logic:

Skill Installation

Logic	Gateway (skills.py)	Client (client.py)
Zip safety check	_is_unsafe_zip_member()	Inline Path(info.filename).is_absolute()
Symlink filtering	_is_symlink_member()	p.is_symlink() post-extraction delete
Zip bomb defence	total_size += info.file_size (declared)	total_size > 100MB (declared)
macOS metadata filter	_should_ignore_archive_entry()	None
Frontmatter validation	_validate_skill_frontmatter()	_validate_skill_frontmatter()
Duplicate detection	HTTPException(409)	ValueError

Two implementations, inconsistent behaviour: Gateway streams writes and tracks real decompressed size; Client sums declared file_size. Gateway skips symlinks during extraction; Client extracts everything then walks and deletes symlinks.

Upload Management

Logic	Gateway (uploads.py)	Client (client.py)
Directory access	get_uploads_dir() + mkdir	_get_uploads_dir() + mkdir
Filename safety	Inline Path(f).name + manual checks	No checks, uses src_path.name directly
Duplicate handling	None (overwrites)	None (overwrites)
Listing	Inline iterdir()	Inline os.scandir()
Deletion	Inline unlink() + traversal check	Inline unlink() + traversal check
Path traversal	resolve().relative_to()	resolve().relative_to()

The same traversal check is written twice — any security fix must be applied to both locations.

The final shared design below supersedes this historical baseline: both adapters now use the same atomic no-replace publisher and owned conversion layout.

2. Design Principles

Dependency Direction

app.gateway.routers.skills  ──┐
app.gateway.routers.uploads ──┤── calls ──→  deerflow.skills.installer
deerflow.client             ──┘              deerflow.uploads.manager

• Shared modules live in the harness layer (deerflow.*), pure business logic, no FastAPI dependency
• Gateway handles HTTP adaptation (UploadFile → bytes, exceptions → HTTPException)
• Client handles local adaptation (Path → copy, exceptions → Python exceptions)
• Satisfies test_harness_boundary.py constraint: harness never imports app

Exception Strategy

Shared Layer Exception	Gateway Maps To	Client
FileNotFoundError	HTTPException(404)	Propagates
ValueError	HTTPException(400)	Propagates
SkillAlreadyExistsError	HTTPException(409)	Propagates
PermissionError	HTTPException(403)	Propagates

Replaces stringly-typed routing ("already exists" in str(e)) with typed exception matching (SkillAlreadyExistsError).

3. New Modules

3.1 deerflow.skills.installer

# Safety checks
is_unsafe_zip_member(info: ZipInfo) -> bool     # Absolute path / .. traversal
is_symlink_member(info: ZipInfo) -> bool         # Unix symlink detection
should_ignore_archive_entry(path: Path) -> bool  # __MACOSX / dotfiles

# Extraction
safe_extract_skill_archive(zip_ref, dest_path, max_total_size=512MB)
  # Streaming write, accumulates real bytes (vs declared file_size)
  # Dual traversal check: member-level + resolve-level

# Directory resolution
resolve_skill_dir_from_archive(temp_path: Path) -> Path
  # Auto-enters single directory, filters macOS metadata

# Install entry point
install_skill_from_archive(zip_path, *, skills_root=None) -> dict
  # is_file() pre-check before extension validation
  # SkillAlreadyExistsError replaces ValueError

# Exception
class SkillAlreadyExistsError(ValueError)

3.2 deerflow.uploads.manager

# Directory management
get_uploads_dir(thread_id: str) -> Path      # Pure path, no side effects
ensure_uploads_dir(thread_id: str) -> Path   # Creates directory (for write paths)

# Filename safety and atomic primary publication
normalize_filename(filename: str) -> str
  # Path.name extraction + rejects ".." / "." / backslash / >255 bytes
create_upload_staging_file(base_dir: Path) -> StagedUpload
publish_staged_upload(staged, preferred_filename) -> Path
publish_upload_bytes(base_dir, preferred_filename, data) -> Path
publish_upload_copy(base_dir, preferred_filename, source_path) -> Path
  # Complete same-directory staging + atomic hard-link no-replace publication.
  # Collisions retry name.ext, name_1.ext, name_2.ext across requests/processes.

# Path safety
validate_path_traversal(path: Path, base: Path) -> None
  # resolve().relative_to(), raises PermissionError on failure

# File operations
list_files_in_dir(directory: Path) -> dict
  # scandir with stat inside context (no re-stat)
  # follow_symlinks=False to prevent metadata leakage
  # Non-existent directory returns empty list
delete_file_safe(base_dir: Path, filename: str) -> dict
  # Deletes the primary and only its exact owned conversion

# URL helpers
upload_artifact_url(thread_id, filename) -> str   # Percent-encoded for HTTP safety
upload_virtual_path(filename) -> str               # Sandbox-internal path
enrich_file_listing(result, thread_id) -> dict     # Adds URLs, stringifies sizes

deerflow.uploads.layout owns primary/conversion physical paths, sandbox virtual paths, and artifact URLs. Generated Markdown is published through deerflow.uploads.conversion at user-data/.upload-conversions/<actual-primary-filename>.md; it is outside the primary listing namespace and never inferred from an uploads/<stem>.md sibling.

4. Changes

4.1 Gateway Slimming

app/gateway/routers/skills.py:

• Remove _is_unsafe_zip_member, _is_symlink_member, _safe_extract_skill_archive, _should_ignore_archive_entry, _resolve_skill_dir_from_archive_root (~80 lines)
• install_skill route becomes a single call to install_skill_from_archive(path)
• Exception mapping: SkillAlreadyExistsError → 409, ValueError → 400, FileNotFoundError → 404

app/gateway/routers/uploads.py:

• Remove inline get_uploads_dir (replaced by ensure_uploads_dir/get_uploads_dir)
• upload_files uses normalize_filename() instead of inline safety checks
• Streamed bytes are completed in shared staging and publish_staged_upload() returns the actual collision-safe name
• list_uploaded_files uses list_files_in_dir() + enrichment
• delete_uploaded_file uses delete_file_safe() for exact owned-conversion cleanup

4.2 Client Slimming

deerflow/client.py:

• Remove _get_uploads_dir static method
• Remove ~50 lines of inline zip handling in install_skill
• install_skill delegates to install_skill_from_archive()
• upload_files uses publish_upload_copy() + ensure_uploads_dir()
• list_uploads uses get_uploads_dir() + list_files_in_dir()
• delete_upload uses get_uploads_dir() + delete_file_safe()
• update_mcp_config / update_skill now reset _agent_config_key = None

4.3 Read/Write Path Separation

Operation	Function	Creates dir?
upload (write)	ensure_uploads_dir()	Yes
list (read)	get_uploads_dir()	No
delete (read)	get_uploads_dir()	No

Read paths no longer have mkdir side effects — non-existent directories return empty lists.

5. Security Improvements

Improvement	Before	After
Zip bomb detection	Sum of declared file_size	Streaming write, accumulates real bytes
Symlink handling	Gateway skips / Client deletes post-extract	Unified skip + log
Traversal check	Member-level only	Member-level + resolve().is_relative_to()
Filename backslash	Gateway checks / Client doesn't	Unified rejection
Filename length	No check	Reject > 255 bytes (OS limit)
thread_id validation	None	Reject unsafe filesystem characters
Listing symlink leak	follow_symlinks=True (default)	follow_symlinks=False
409 status routing	"already exists" in str(e)	SkillAlreadyExistsError type match
Artifact URL encoding	Raw filename in URL	urllib.parse.quote()
Concurrent same-name writes	Scan/claim then replace	Atomic no-replace publication with _N retry
Generated Markdown ownership	Guessed uploads/<stem>.md sibling	Exact .upload-conversions/<full-primary-name>.md asset

6. Alternatives Considered

Alternative	Why Not
Keep logic in Gateway, Client calls Gateway via HTTP	Adds network dependency to embedded Client; defeats the purpose of DeerFlowClient as an in-process API
Abstract base class with Gateway/Client subclasses	Over-engineered for what are pure functions; no polymorphism needed
Move everything into client.py and have Gateway import it	Violates harness/app boundary — Client is in harness, but Gateway-specific models (Pydantic response types) should stay in app layer
Merge Gateway and Client into one module	They serve different consumers (HTTP vs in-process) with different adaptation needs

7. Breaking Changes

None. All public APIs (Gateway HTTP endpoints, DeerFlowClient methods) retain their existing signatures and return formats. The SkillAlreadyExistsError is a subclass of ValueError, so existing except ValueError handlers still catch it.

8. Tests

Module	Test File	Count
skills.installer	tests/test_skills_installer.py	22
uploads.manager	tests/test_uploads_manager.py	20
client hardening	tests/test_client.py (new cases)	~40
client e2e	tests/test_client_e2e.py (new file)	~20

Coverage: unsafe zip / symlink / zip bomb / frontmatter / duplicate / extension / macOS filter / normalize / deduplicate / traversal / list / delete / agent invalidation / upload lifecycle / thread isolation / URL encoding / config pollution.