Hi guys,
I'm not sure if this is a bug or an intended feature. At face value it seems like a security issue.
My setup:
Feathers typescript app generated via Feathers CLI generator (V4.5.2)
Feathers-objection database adapter (V5.2.0)
Node 12.16.1
Postgres 12
Service endpoints created via Feathers CLI generator
The problem:
Using a REST client, I POST data to the service endpoint to create a row in the database. Everything works as intended.
However, if I POST data with an "id" parameter. It will create a new entry in the database as intended, but it will return the data of the user provided "id". (I define the primary key "id" is in the $beforeInsert hook of the Objection Model.)
Temporary workaround:
Sanitize input for "id" parameter before it reaches the database adapter
Where the potential bug was introduced:
I dug through the git history, and it looks like the logic was introduced between v0.4 and v0.5. Line 220 to be exact.
Here is a link to the specific commit
Proposed fix:
Remove the logic that defines the "id" parameter for the _get method via the "data" variable, and only allow the value in the "row" variable to set the "id"
Risk levels:
- If your primary keys are generated via UUIDs, then it's pretty hard for end users to guess the keys at face value. But if your PK are exposed via your app. Then bad actors can bypass any security logic in the "find" or "get" service methods by just using the "create" service method.
- If the primary keys are sequentially incremented via the database, then a malicious actor can repeatably POST the service endpoint with incrementing id values, allowing them to scrape private data.
- If you generate your primary key "id" parameter in a "before" feathers hook, then it would just overwrite the user provided "id" parameter, and there is no security issue.
My question:
What was the logic in following line? (This is line 220 from the previous link)
const id = typeof data[this.id] !== 'undefined' ? data[this.id] : row[this.id]
At face value the code above basically transforms a "create" service request into a leaky "get" service request.
Proposed fix:
Change line 220 (from the link) or line 485/488 (of v5.3.0)
const id = row[this.id]
The code above only returns the data from new entry created by the database.
Can someone double check my logic?