Skip to content

fix(server): serve only the public namespace from the static files route - #1837

Open
ethicnology-agent wants to merge 1 commit into
slopus:mainfrom
ethicnology-agent:fix/server-files-route-allowlist
Open

ethicnology-agent wants to merge 1 commit into
slopus:mainfrom
ethicnology-agent:fix/server-files-route-allowlist

Conversation

@ethicnology-agent

Copy link
Copy Markdown

Fixes the GET /files/* defect reported in #1832's sibling, #1830 — filed as part of the /files/* finding in the audit issues (#1829–#1833).

Problem

publicLocalFilePath splits public from private with a deny-list holding exactly one rule:

if (parts[0] === "sessions" && parts[2] === "avatar") return null;
return fullPath;

GET /files/* (api.ts:84-98, registered whenever isLocalStorage()) has no authentication. So every namespace under the shared storage root that the rule does not name was served to any anonymous caller who knew the path — including session attachments (sessions/<id>/attachments/<uuid>.enc) and project avatars (projects/<id>/avatar/<uuid>.enc), whose own authenticated routes do check ownership before handing over the same bytes.

The single rule also compares segments case-sensitively while the open() that follows does not, so sessions/<id>/AVATAR/<file> reached the one object the rule existed to protect on a case-insensitive filesystem — which is the default on macOS.

What this changes

Invert the filter. A path is returned only when its first segment is public/, the namespace uploadImage writes (public/users/<userId>/<directory>/<filename>) and the only tree meant to be readable without a bearer token. A namespace added later is then refused by default instead of being exposed until someone remembers to extend the list.

Four cases in the existing spec, verified red/green:

  • against the deny-list, sessions/<id>/attachments/… and sessions/<id>/AVATAR/… both return a path where the test expects null;
  • with the allow-list, all four pass, including the pre-existing traversal cases and public/users/u1/avatars/a.webp.

tsc --noEmit -p tsconfig.build.json is clean and the server suite passes 123/123.

Scope of the impact, stated honestly

This is not a break of confidentiality on its own: the blobs are client-side encrypted, the filenames are random UUIDs, and no route hands one account another account's ref. It is the access control that was meant to sit in front of that encryption, and it was open. Treat it as defence in depth restored, not as a disclosure.

It affects local-storage deployments only — happy server and the standalone Docker image. S3-backed deployments never register the route.

What this deliberately does not change

  • Symlinks inside public/. path.resolve does not resolve them, so a link planted in that tree would still escape. The server writes that tree itself, so there is no untrusted writer today; worth hardening separately rather than widening this diff.
  • The missing Content-Type and X-Content-Type-Options on the route's responses. Reported separately in the audit issues; it needs a decision about what to send, not a filter change.
  • The existence oracle. The route still answers 404 versus 403 distinguishably for session and project ids. Bounded by cuid identifiers, and out of scope here.

publicLocalFilePath split public from private with a deny-list holding one
rule: refuse sessions/<id>/avatar/<file>. Everything else under the shared
storage root was returned, and GET /files/* has no authentication, so in
local-storage deployments session attachments and project avatars were
served to anyone who knew or guessed the path — bypassing the ownership
checks their own authenticated routes perform on the same bytes.

The single rule also compared segments case-sensitively while the open()
that followed did not, so sessions/<id>/AVATAR/<file> reached the one object
the rule existed to protect on a case-insensitive filesystem.

Invert it: return a path only when its first segment is public/, the
namespace uploadImage writes and the only tree meant to be readable without
a bearer token. A namespace added later is then refused by default instead
of being exposed until someone remembers to extend the list.

Confidentiality of those blobs still rests on client-side encryption; this
restores the access control that was supposed to sit in front of it.
@ethicnology-agent
ethicnology-agent force-pushed the fix/server-files-route-allowlist branch from 6595e17 to 26cdb95 Compare October 1, 2026 19:02
@ethicnology-agent

Copy link
Copy Markdown
Author

Rebased onto current main (4cf54d1). Clean rebase, diff unchanged.

This branch has not been deployed

No deployments
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.

2 participants