-
-
Notifications
You must be signed in to change notification settings - Fork 585
Description
Description
Hi there!
Thank you for developing and maintaining this library. We used this library extensively and love the type hints, this really helped us when migrating API versions!
We discovered that it is possible for a client to utilize a URL like this:
GET /books/..
GET /books/finance/..
A sample code:
const response = await client.GET("/books/{category}/", {
params: {
path: { category: ".." },
},
});
In the above case, it will do an actual path traversal and will access /
and /books
, respectively. I believe there is a possibility for a malicious actor to perform path traversal attacks in this case. While encodeURIComponent
stops file system traversal like ../../etc/passwd
by encoding the /
, it doesn't URL path canonicalization.
I believe when an API receives a URL like /api/books/finance/the-psychology-of-money
, it understands the structure. If it receives /api/books/..
, many systems will automatically resolve the path, so that GET /api/books/..
becomes GET /api/
and GET /api/books/finance/../
effectively becomes GET /api/books/
.
This is not a file system breach, but it's a logical vulnerability. An attacker could use this to escape the intended API endpoint and access a parent endpoint, potentially bypassing security middleware, logging, or authorization rules that were specific to the child path.
Proposal
My proposed fix is that we should escape the literal ..
characters to prevent it from being interpreted as a traversal command. Fortunately, the /
character is already escaped through encodeURIComponent
, but ..
is not yet escaped and will do an actual path traversal. Since .
is an unreserved character, a specific check is needed.
I see two potential solutions and would love the maintainers' input on the preferred approach:
Simple Encoder
My suggestion is to add a simple serializer that checks whether the path parameter is ..
and then replace it with %2E%2E
, in this line:
const value = pathParams[name]; |
Stricter Fix: Throw an Error
Arguably, a parameter like ..
is an invalid input. We could throw an error when this is detected. This is a more robust fail-fast security posture, but I recognize it could be a breaking change if any users rely on the current behavior for some reason.
I'm more than happy to open a PR related to this and fix it. Please let me know which direction you'd prefer!
Let me know what you think and feel free to assign me to work on this. Thank you for your time and consideration!
Extra
- I’m willing to open a PR (see CONTRIBUTING.md)