Insecure file-serving pattern: `GetItemPictureById` allows path traversal / arbitrary file read via `PictureFileName'

[luuhung1217]

Summary

Catalog.API's product-image endpoint builds a filesystem path by concatenating a database-stored, client-controllable PictureFileName directly into Path.Combine(..., "Pics", pictureFileName) and streams the result with TypedResults.PhysicalFile, with no containment check. Because Path.Combine lets a rooted or ..-containing final segment escape the Pics directory, a caller can make the endpoint return arbitrary files from the host filesystem (e.g. appsettings.json, project source, /etc/passwd).

Since this is a reference application that many developers copy patterns from, the file-serving idiom here teaches an insecure practice (CWE-22). The fix is small and self-contained.

Affected code

src/Catalog.API/Apis/CatalogApi.cs

// GetItemPictureById (~line 205) — PictureFileName comes from the stored CatalogItem
var path = GetFullPath(environment.ContentRootPath, item.PictureFileName);
...
return TypedResults.PhysicalFile(path, mimetype, lastModified: lastModified);

// ~line 420
public static string GetFullPath(string contentRootPath, string pictureFileName) =>
    Path.Combine(contentRootPath, "Pics", pictureFileName);   // <-- no normalization / containment

PictureFileName is bound straight from the request body in CreateItem/UpdateItem (product.PictureFileName), so it is fully attacker-controlled.

Steps to reproduce

Observed scope: any real on-disk file the process can read is returned. (/proc/* pseudo-files are not returned — PhysicalFile reports Content-Length: 0 for procfs and Kestrel aborts the response — so process environment variables are not exfiltrable through this particular sink.)

Impact

Unrestricted local file read of the Catalog.API host: appsettings*.json, mounted secret files, K8s service-account tokens (/var/run/secrets/...), data-protection keys, application source, etc. In the default topology Catalog.API is an internal service, so realistic exposure requires it to be network-reachable (direct exposure, misconfiguration, or via SSRF/lateral movement) — but the file-read defect itself is unconditional.

Suggested fix

Constrain the filename to the Pics directory. Either reduce it to a bare filename, or verify the resolved path stays inside the base:

public static string GetFullPath(string contentRootPath, string pictureFileName)
{
    // Reject any directory component supplied by the caller.
    var safeName = Path.GetFileName(pictureFileName);
    var picsRoot = Path.Combine(contentRootPath, "Pics");
    var full = Path.GetFullPath(Path.Combine(picsRoot, safeName));

    // Defense in depth: ensure we never escape the Pics directory.
    if (!full.StartsWith(picsRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal))
        throw new InvalidOperationException("Invalid picture path.");

    return full;
}

Optionally also validate PictureFileName on write (CreateItem/UpdateItem) to reject path separators and ...

Notes

Related hardening for the same endpoints (lower priority, may be intentional for the sample): the Catalog.API write endpoints (POST/PUT/DELETE /api/catalog/items) have no authorization, which is what lets the malicious PictureFileName be stored. If the reference app intends these to be callable only via an authenticated path, adding RequireAuthorization on the write routes would also break the chain. I'm happy to open a PR with the fix above if it's welcome.