Skip to content

fix(api): reject persona ids that are not a single directory name - #3

Open
sergiobuilds wants to merge 1 commit into
LilMGenius:ralphthonfrom
sergiobuilds:fix/persona-id-validation
Open

fix(api): reject persona ids that are not a single directory name#3
sergiobuilds wants to merge 1 commit into
LilMGenius:ralphthonfrom
sergiobuilds:fix/persona-id-validation

Conversation

@sergiobuilds

Copy link
Copy Markdown

:id goes straight into the path:

const personasDir = './personas'
readPloonFile(`${personasDir}/${id}/persona.md`)

Nothing validates it, so an encoded ..%2F walks out of personas/.

Measured

Planted a file outside the personas directory, then asked for it through the API:

$ curl 'http://127.0.0.1:38217/api/personas/..%2F..%2Foutside%2Fsecret'
HTTP 200
{"id":"../../outside/secret","persona":{"core":{"- leaked":"read from outside the personas directory"}}}

All three /personas/:id routes answered 200 the same way, including /qa-simulation.

Reach is bounded — the filenames are hardcoded, so it only ever returns a persona.md, nuance.md, or accounts.md. That bound is a property of today's code, not a check, and it disappears the first time a filename becomes dynamic.

After

request expected actual
/personas/testuser 200 200
/personas/..%2F..%2Foutside%2Fsecret 404 404
same, /qa-simulation 404 404
same, /interview-log 404 404

A persona id names one directory under personas/, so the guard says exactly that: ^[A-Za-z0-9._-]+$, with . and .. excluded. It sits next to the id in each of the three routes rather than in middleware, so the constraint is visible where the value is read.

- `:id` was interpolated into `./personas/${id}/persona.md` unvalidated, so an encoded `..%2F..%2F` walked out of the personas directory and returned any `persona.md`, `nuance.md`, or `accounts.md` on the host with HTTP 200; the three `/personas/:id` routes now require `^[A-Za-z0-9._-]+$` and 404 on anything else
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.

1 participant