fix: path traversal in symlink_repo_index_service (#390)

[stack72]

Fixes #390, reported by @umag.

Problem

symlink_repo_index_service.ts passed user-controlled names (model names, workflow names, vault names, step names, tag keys, tag values) directly into join() path construction without any validation, allowing path traversal outside the index directory. The tag key case was the worst — the source already included ".." in the join before the user-controlled tagKey, amplifying the traversal.

Fix

Added a private assertPathContained() method (mirroring the identical pattern already in unified_data_repository.ts:995–1010) and applied it at every vulnerable path-construction site.

Locations validated

Method	Component validated	Bounded to
indexModel()	modelName	models/
handleModelDeleted()	event.modelName	models/
indexWorkflow()	workflowName	workflows/
handleWorkflowDeleted()	event.workflowName	workflows/
indexWorkflowRun()	workflowName	workflows/
indexWorkflowRun()	step.stepName	steps/
indexVault()	vaultName	vaults/
handleVaultDeleted()	event.vaultName	vaults/
indexTagBasedData()	tagKey	modelDir
indexTagBasedData()	tagValue	tagKeyDir

Extra fix beyond the original plan

The try-catch in indexTagBasedData() previously swallowed all errors via logger.debug. Without fixing this, the new assertPathContained() calls inside that block would be dead code from a security standpoint. Added a re-throw guard so path traversal errors propagate:

if (
  error instanceof Error &&
  error.message.startsWith("Path traversal detected")
) {
  throw error;
}

Tests added

7 of the 9 planned traversal vectors covered with assertRejects in symlink_repo_index_service_test.ts:

• ✅ model name ../../../malicious → handleModelCreated
• ✅ model name ../../../malicious → handleModelDeleted
• ✅ workflow name ../../../malicious → handleWorkflowCreated
• ✅ workflow name ../../../malicious → handleWorkflowDeleted
• ✅ vault name ../../../malicious → handleVaultCreated
• ✅ vault name ../../../malicious → handleVaultDeleted
• ✅ step name ../../../malicious → handleWorkflowRunStarted

The 2 remaining planned vectors (tag key / tag value traversal) were not tested. They require a unifiedDataRepo stub; the existing createIndexService test helper does not wire one up, and building a full UnifiedDataRepository stub is out of scope for this fix.

Plan vs implementation

Area	Plan	Implemented
Add resolve to import	✅	✅
assertPathContained() method	✅	✅
indexModel validation	✅	✅
handleModelDeleted validation	✅	✅
indexWorkflow validation	✅	✅
handleWorkflowDeleted validation	✅	✅
indexWorkflowRun validation	✅	✅
indexVault validation	✅	✅
handleVaultDeleted validation	✅	✅
indexTagBasedData tag key/value validation	✅	✅
Fix try-catch swallowing traversal error	✗ not planned	✅ added
7 path traversal tests	✅	✅
Tag key/value traversal tests	✅ planned	✗ omitted

[github-actions]

✅ Approved

This is a well-crafted security fix for path traversal vulnerabilities in symlink_repo_index_service.ts. The implementation is solid and follows established patterns.

Code Quality Assessment

CLAUDE.md Compliance:

• ✓ TypeScript strict mode, no any types
• ✓ Named exports used throughout
• ✓ AGPLv3 license headers present
• ✓ Tests live next to source files (symlink_repo_index_service_test.ts)

Security Fix Analysis:

• The assertPathContained() method correctly uses resolve() to normalize paths before comparison
• The check !resolvedPath.startsWith(resolvedParent + "/") properly handles the slash to prevent partial directory name matches
• The re-throw guard for path traversal errors in the try-catch block is a good catch - without it, the security validation would have been silently swallowed

Test Coverage:

• 7 of 9 planned attack vectors are covered with proper assertRejects tests
• The 2 missing tag key/value tests are documented and justified (require UnifiedDataRepository stub)

DDD Principles:

• Security validation is correctly placed at the infrastructure layer boundary where user-controlled data enters filesystem operations
• Follows the established pattern from unified_data_repository.ts:995-1010

Suggestions (non-blocking)

1. Code duplication: The assertPathContained method now exists in both unified_data_repository.ts and symlink_repo_index_service.ts. A future refactoring could extract this to a shared path utilities module. However, duplicating security-critical code can be acceptable to ensure each component owns its validation logic.

2. Consider adding tests for tag key/value traversal: While the current justification is valid, these vectors should be tested eventually if the test infrastructure allows.

LGTM - this properly addresses the security vulnerability reported in #390.