Skip to content

fix(security): prevent path traversal in delete endpoint - #161

Draft
prathamesh04 wants to merge 2 commits into
metacall:masterfrom
prathamesh04:fix/delete-path-traversal
Draft

fix(security): prevent path traversal in delete endpoint#161
prathamesh04 wants to merge 2 commits into
metacall:masterfrom
prathamesh04:fix/delete-path-traversal

Conversation

@prathamesh04

Copy link
Copy Markdown
Contributor

Description

The delete endpoint was passing unsanitized user input to recursive rm without path boundary check. A malicious suffix like ../../../etc could escape the appsDirectory and delete arbitrary files.

Fixes #121

Changes

  • Add safeResolve utility for path traversal protection
  • Add isValidDeploymentName validator (alphanumeric, dots, hyphens, underscores)
  • Validate suffix before using in delete endpoint
  • Apply safeResolve in repositoryDelete for defense-in-depth
  • Return 400 error for invalid deployment names

Security Impact

This is a critical security fix that prevents:

  • Path traversal attacks via crafted deployment names
  • Accidental deletion of files outside the apps directory
  • Exploitation through the delete API endpoint

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Build passes (npm run build)
  • Lint passes (npm run lint)

Fixes infinite loop in assignColorToWorker when all 16 ANSI colors
are allocated. Previously, the do...while loop would spin forever
looking for an unassigned color, blocking the Node.js event loop
and hanging the server at 17+ concurrent deployments.

Changes:
- Replace random retry with deterministic round-robin index
- Remove assignedColorCodes tracking map (no longer needed)
- Remove unused AssignedColorCodesType interface
- Remove stale TODO comments

Colors cycle safely when more than 16 deployments are active.

Fixes metacall#116
The delete endpoint was passing unsanitized user input to recursive
rm without path boundary check. A malicious suffix like '../../../etc'
could escape the appsDirectory and delete arbitrary files.

Changes:
- Add safeResolve utility for path traversal protection
- Add isValidDeploymentName validator (alphanumeric, dots, hyphens, underscores)
- Validate suffix before using in delete endpoint
- Apply safeResolve in repositoryDelete for defense-in-depth
- Return 400 error for invalid deployment names

Fixes metacall#121
@prathamesh04

Copy link
Copy Markdown
Contributor Author

cc @viferga @josead

@prathamesh04

prathamesh04 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Testing Complete ✅

I've tested PR #161 and verified the security fix works:

Tests Performed

1. isValidDeploymentName validation:

  • ../etc/passwd → ✅ Blocked
  • my app; rm -rf / → ✅ Blocked (spaces)
  • ../../../etc → ✅ Blocked (path traversal)
  • normal-name → ✅ Allowed
  • my_app.v1 → ✅ Allowed

2. safeResolve path protection:

  • safeResolve("/apps", "my-app") → ✅ Resolves normally
  • safeResolve("/apps", "../etc") → ✅ Throws path traversal error

3. TypeScript compilation:

  • Build passes ✅
  • Module exports work correctly ✅

Testing video:
Screencast from 2026-07-28 20-21-29.webm

Summary

The fix properly prevents path traversal attacks by:

  1. Validating deployment names with regex ^[a-zA-Z0-9._-]+$
  2. Using safeResolve() instead of join() to ensure paths stay within appsDirectory
  3. Returning 400 for invalid deployment names

cc @viferga @josead

@viferga

viferga commented Aug 3, 2026

Copy link
Copy Markdown
Member

You have merged multiple things in the same post. This is not the way of working man. You should not spam the repository with random PRs.

The way of working is with TDD. You first find an issue, you create a test that reproduces it. Then you create a new PR only with this, we run the CI we validate it fails and the bug is reproduced. Then you make a new commit and you verify all tests pass, then I merge it. We run the CI on each step so we verify tests fail first, and then tests pass.

If you keep spamming like this, pushing random PRs, probably vibe coded. I will close all of them. For now I left all as draft.

@viferga
viferga marked this pull request as draft August 3, 2026 15:04
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.

Security: delete endpoint passes unsanitized user input to recursive rm without path boundary check

2 participants