clean_path() Path Traversal: rel_path Escapes USERDATA Dir

Introduction

This path traversal issue let an attacker turn a routine file-lookup helper into an arbitrary file reader. The affected function, clean_path(), builds a filesystem path by joining a relative path onto a fixed USERDATA root. The problem: the rel_path argument comes from user-controlled input — route parameters like :waktu and :nomor_surah, and a number query parameter — and was split and joined without ever checking whether the final path actually stayed inside USERDATA.

The vulnerable pattern is deceptively simple:

paths = rel_path.split('/')
for path in paths:
  cleaned = os.path.join(cleaned, path)

os.path.join() happily accepts .. as a path component. Splitting on / and joining each segment back on gives an attacker full control over how many directories the resulting path climbs before descending again. This is a classic case where "the code looks like it's building a safe path" and "the code is actually safe" are two different things — the function name promises cleaning, but nothing in the body validates the outcome.

Affected Versions

Because this is first-party application code rather than a published package, there's no version range to check — any deployment running the pre-fix clean_path() implementation is exposed.

The Vulnerability Explained

Here's the vulnerable function in full:

def clean_path(rel_path: str):
  """Cleans a relative path by splitting on forward slash and os.path.joining."""
  cleaned = USERDATA
  paths = rel_path.split('/')
  for path in paths:
    cleaned = os.path.join(cleaned, path)
  return cleaned

Notice what's missing: there is no check anywhere that cleaned ends up inside USERDATA. The function's docstring describes splitting and joining, not validating. Each segment of rel_path is joined in sequence, so a value like ../../etc/passwd splits into ['..', '..', 'etc', 'passwd'] and is joined one directory at a time — each .. walks back up a level in the real filesystem, exactly as it would from a shell.

Given that rel_path is assembled from route parameters such as :waktu and :nomor_surah plus a number query parameter, an attacker doesn't need any special access — just the ability to send an HTTP request with a crafted path segment. A request where :nomor_surah is set to something like ../../../../etc/passwd (URL-encoded as needed) would cause clean_path() to return a path far outside USERDATA, and whatever code calls it next — a JSON loader, in this case — would happily read (or, via save_userdata_json(), write) that file.

The real-world impact is significant: any JSON-readable file on the host becomes a potential read target, and because clean_path() is also used on the write path, an attacker could potentially overwrite files outside the sandbox too, depending on process permissions. For a service storing per-user data under USERDATA, this collapses the entire isolation model that directory is supposed to provide.

The Fix

The fix adds exactly the check that was missing: normalize the constructed path and confirm it's still rooted under USERDATA before returning it.

paths = os.path.normpath(rel_path).split(os.sep)
for path in paths:
  cleaned = os.path.join(cleaned, path)
cleaned = os.path.normpath(cleaned)
userdata_root = os.path.normpath(USERDATA)
if os.path.commonpath([cleaned, userdata_root]) != userdata_root:
  raise ValueError(f'Invalid path: "{rel_path}" is not under userdata.')

Three things changed, each closing a specific gap:

1. rel_path is now run through os.path.normpath() before splitting, so sequences like ../.. collapse predictably rather than being treated as opaque segments.
2. After building cleaned, it's normalized again, since os.path.join() can reintroduce .. segments that weren't resolved during the loop.
3. The decisive change: os.path.commonpath([cleaned, userdata_root]) is compared against userdata_root. If the normalized target path doesn't share USERDATA as its common ancestor, the function raises ValueError instead of silently returning an escaped path.

This turns clean_path() from a function that describes cleaning into one that enforces a boundary — any caller, whether loading JSON for a :waktu/:nomor_surah lookup or saving user data, now gets either a path guaranteed to be under USERDATA or an exception.

Key Takeaways

• os.path.join() does not sanitize .. segments — joining a path.split('/') list segment-by-segment is functionally identical to pasting the raw string into the filesystem call.
• A function named clean_path() is not automatically safe just because it sounds like it validates input; the docstring here described splitting/joining, not boundary enforcement — read the implementation, not the name.
• When a route parameter (:waktu, :nomor_surah) or query parameter (number) feeds into any path-construction helper, treat it as hostile input, not as a filename.
• Validate after normalization: checking rel_path for .. before calling os.path.normpath() can be bypassed with encoding tricks; the fix instead checks the final, normalized result against the root using os.path.commonpath().
• The same unvalidated helper served both reads and writes (save_userdata_json()), so a single missing check doubled as both an information-disclosure and a file-tampering bug.

How Orbis AppSec Detected This

• Source: route parameters :waktu and :nomor_surah, and the number query parameter, flowing into the rel_path argument of clean_path()
• Sink: the dynamic file path returned by clean_path() being used to load/save JSON data from disk
• Missing control: no normalization or root-containment check on the joined path before it was used for file I/O
• CWE: CWE-22 (Path Traversal)
• Fix: normalize the input and resulting path, then raise ValueError unless os.path.commonpath() confirms the result is still under the USERDATA root

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

This finding is a reminder that path-building helpers are only as safe as their weakest validation step — and in clean_path(), there was no validation step at all. Splitting on / and joining each piece back together looks like sanitization but performs none; .. segments pass straight through to the filesystem. By normalizing both the input and the final joined path, and rejecting anything whose common ancestor isn't the USERDATA root, the fix closes the gap for every caller that depends on clean_path() — whether it's reading surah data by route parameter or writing user state back to disk.