How to review unsafe file path construction

For a path traversal code review, start with the filesystem operation, then trace every piece of the path back to its writer. Determine whether a person can make the application read, write, delete or expose a file outside the intended location. Inspect decoding, normalization, storage mappings and symlinks before deciding that a string check protects the operation. The answer changes with the caller's permission, the process's filesystem rights, the deployment's directory layout and whether another actor can replace files between a check and an open. A variable named safe_path is not evidence of confinement.

CWE-22 covers pathnames that escape a restricted directory; CWE-73 covers externally influenced file names and paths more broadly. The useful review question is not “does the input contain ..?” It is “can this actor cause this operation to act on a resource they were not meant to choose?” A permitted path can still point to another customer's file if object authorization is missing.

Illustrative case: downloading a support attachment

Illustrative service and paths; no real file was accessed. Harbor Support has a route /attachments/{id}. A database row stores the attachment owner and a server-generated storage key. The web process reads a file under a private storage directory and streams it to the authorized requester. A scanner flags code that joins the directory with a value returned from the database.

The initial claim is: “A member can influence the joined path so the download route returns a file outside their authorized attachment.” That statement contains two possible failures. The member might control a path component that escapes the storage directory. Or the joined path may stay within storage while the route fetches a row belonging to another account. The first is path traversal; the second is an authorization failure. Review both, and do not collapse them into one generic “file access” verdict.

A safer illustrative flow is:

authenticated member + opaque attachment ID
  -> fetch metadata row by ID
  -> verify stored account/parent and this member's attachment-read grant
  -> use server-generated, path-free storage key from that row
  -> storage adapter confines the key to a protected root
  -> stream only that object

The flow assumes the row's storage key was generated by the server and cannot be overwritten by a user-supplied filename, the root is not writable by untrusted actors, and the storage adapter has a reviewed confinement rule. If any assumption is unproved, keep it open. OWASP's file upload guidance recommends an application-generated filename; retaining a client's original filename for display does not mean using it as a filesystem path.

For a small fixed public-document endpoint, an allowlist can eliminate path interpretation at the request boundary:

# Illustrative selection only: PUBLIC_ROOT and its files are deployment-owned.
PUBLIC_FILES = {"guide": "guide-v4.pdf", "terms": "terms-v2.pdf"}

def selected_name(document_key: str) -> str:
    if type(document_key) is not str:
        raise LookupError("document not found")
    try:
        return PUBLIC_FILES[document_key]
    except KeyError:
        raise LookupError("document not found")

This returns one of two server-owned basenames. It does not by itself secure the later open: a mutable directory, a symlink replacing a file, or a second unreviewed route can defeat the intended boundary. For account-specific attachments, a static public list is not enough; retrieve the row, authorize the specific object and use an opaque storage key. OWASP's Authorization Cheat Sheet calls for permission checks on every request.

Trace what the filesystem actually sees

Use a table while reading the code:

Do not assume string prefix tests prove a path is inside a directory: a sibling directory may share the same text prefix, and lexical comparisons do not resolve symlinks. Python's pathlib documentation distinguishes lexical path operations from Path.resolve(), which resolves symlinks and parent components. Even a resolved-path check followed by a later open can face a race if another actor can change the directory entry in between. The more robust design is to avoid user-selected paths, keep the storage area immutable to the requester, and use a storage/open primitive with confinement appropriate to the platform. Review the exact implementation rather than claiming resolve() alone closes every case.

Resolve the same name in three filesystem layouts

A concrete fixture makes the confinement claim easier to inspect. These are synthetic paths, not files to open on a production host:

/srv/attachments/obj-a       ordinary file owned by Cedar
/srv/attachments/obj-b       ordinary file owned by Harbor
/srv/attachments-old/note    sibling directory, outside the root
/srv/attachments/current    symlink to /srv/attachments-old/note

A textual check that a selected path starts with /srv/attachments accepts the sibling's name too. A component-aware containment check avoids that particular error. But lexical containment of /srv/attachments/current does not say where the symlink resolves. Python's Path.resolve() resolves links and parent components; it is useful evidence about a path at the time of resolution, not a lock on future directory entries.

Suppose the source does this:

resolve candidate
check resolved target lies under root
open candidate by its original name

If an untrusted writer can replace the name between check and open, the second lookup can select a different object. Reopening the resolved name may reduce one ambiguity but is not a general solution when directories themselves can change. The implementation needs a platform-appropriate open operation whose enforced confinement and symlink policy apply during the actual lookup, or a storage design that removes the untrusted mutation capability. Review its documented semantics rather than copying a generic safe_open helper. No particular cross-platform primitive is claimed here.

Directory confinement and tenant ownership produce different answers

For Cedar's authorized attachment A17, metadata maps to obj-a. A request for Harbor's A18 maps to obj-b, which is still perfectly inside the root. A storage boundary would allow it; the object-level authorization boundary must deny it before streaming. Conversely, a correctly authorized Cedar row whose stored key was overwritten with an unsafe path is not repaired by the row's account check.

The synthetic review reaches four precise outcomes:

• A17 → immutable server key → protected ordinary file: the proposed user-path traversal is defeated if every key writer and the actual storage adapter preserve those facts.
• A18 belonging to Harbor: deny at metadata authorization; no file bytes should be read for delivery to Cedar.
• Legacy imported row containing a client filename: investigate that separate writer. Newly generated keys do not qualify historical rows.
• Writable link or directory encountered: current confinement evidence is insufficient until the open semantics or write authority is corrected.

The repair selected for Harbor is to migrate legacy rows to server-assigned opaque keys with verified ownership, reject unresolved rows, and remove requester write authority over storage names. Migration must preserve the attachment-to-object association; blindly renaming files can create an authorization mismatch. Platform evidence must still establish the actual root, directory ownership and adapter behavior.

For a future authorized check, use an isolated fixture with harmless sentinels and instrument the adapter to establish rejection before unintended content access. Expected outcomes should distinguish forbidden ownership, nonexistent object and confinement failure while following the application's information-disclosure policy. This article has performed none of those operations.

Counterevidence and limits that matter

Suppose Harbor's row contains a random server-generated key, the query and action policy authorize that attachment for the member, and the storage adapter enforces a fixed root with no user-writable links. That is meaningful counterevidence against the reported path at this revision. Record the producer of the key and the exact adapter. If a separate upload migration accepts a client filename into the same column, the invariant may fail on migrated rows; inspect that producer before closing the claim.

Suppose instead the route uses an authorized row but concatenates a stored relative path copied from an upload header. The row filter does not solve traversal. The correct next question is whether upload processing replaced the name with a server-generated key and whether later path resolution stays within the intended object set. If the storage root's write policy or mount layout is unavailable, call the conclusion inconclusive and ask the platform owner for those facts. Do not label a scanner candidate confirmed merely because a join exists, and do not label the repository safe because one route rejected a marker.

Review every writer of the stored storage key

Harbor's safest current upload path does not automatically qualify older rows. Search for all writers of the key column: normal uploads, imports, support repairs, migrations, and copying between accounts where those operations exist. For each, establish whether it generates a new opaque key, preserves an existing verified association, or accepts a caller-selected path.

A legacy migration needs two kinds of evidence. The new key must identify the intended file, and the attachment's account and parent must remain correct. Renaming a file while updating the wrong metadata row could remove traversal syntax yet create an authorization error. Use a controlled mapping and reconcile the resulting associations before enabling delivery for migrated rows.

Keep original display names separate from storage authority. A customer may reasonably expect to download a file with its original descriptive name. That requirement does not mean the name should select the server's filesystem object. Review the response presentation separately from the internal storage key so the product can preserve usability without reintroducing path selection.

Give the platform owner a concrete confinement question

If the source packet lacks directory ownership or mount layout, ask who can modify the actual root and its entries in the assessed deployment. A statement that the application uses /srv/attachments is not enough. The reviewer needs the relationship between the configured root, the process's filesystem authority, and any untrusted writer that could replace a name during access.

The answer may support the current immutable-storage design or show that the adapter needs a stronger lookup primitive. Do not prescribe a generic cross-platform resolve-and-open helper without checking those semantics. The useful handoff identifies the unresolved mutation capability and the operation it could affect.

The resulting closure can remain narrow: the reviewed key writers no longer accept customer paths, metadata preserves the authorized object association, and the actual storage operation enforces the selected confinement policy. It does not declare every file operation in the application safe, nor does it turn a source review into a claim that a real file was accessed.

The immediate action is to take one flagged file operation and fill the seven-stage table with real code locations and a deployment owner. If the input crosses a serializer or queue before it becomes a path, use the boundary trace. Once a harmful path is supported, use the remediation guide to choose a change that removes the user's ability to select a filesystem resource.