Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(langchain): Fix local file store traversal issue #6736

Merged
merged 3 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/core_docs/docs/integrations/stores/file_system.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@
"\n",
":::\n",
"\n",
":::warning\n",
"\n",
"\n",
"This file store can alter any text file in the provided directory and any subfolders.\n",
"Make sure that the path you specify when initializing the store is free of other files.\n",
"\n",
":::\n",
"\n",
"```\n",
"\n",
"### Integration details\n",
Expand Down
30 changes: 28 additions & 2 deletions langchain/src/storage/file_system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ import { BaseStore } from "@langchain/core/stores";
* await store.mdelete([key]);
* }
* ```
*
* @security **Security Notice** This file store
* can alter any text file in the provided directory and any subfolders.
* Make sure that the path you specify when initializing the store is free
* of other files.
*/
export class LocalFileStore extends BaseStore<string, Uint8Array> {
lc_namespace = ["langchain", "storage"];
Expand All @@ -43,6 +48,12 @@ export class LocalFileStore extends BaseStore<string, Uint8Array> {
* @returns Promise that resolves to the parsed file content.
*/
private async getParsedFile(key: string): Promise<Uint8Array | undefined> {
// Validate the key to prevent path traversal
if (!/^[a-zA-Z0-9_\-:.]+$/.test(key)) {
throw new Error(
"Invalid key. Only alphanumeric characters, underscores, hyphens, colons, and periods are allowed."
);
}
try {
const fileContent = await fs.readFile(this.getFullPath(key));
if (!fileContent) {
Expand Down Expand Up @@ -87,11 +98,26 @@ export class LocalFileStore extends BaseStore<string, Uint8Array> {
private getFullPath(key: string): string {
try {
const keyAsTxtFile = `${key}.txt`;
const fullPath = path.join(this.rootPath, keyAsTxtFile);

// Validate the key to prevent path traversal
if (!/^[a-zA-Z0-9_.\-/]+$/.test(key)) {
throw new Error(`Invalid characters in key: ${key}`);
}

const fullPath = path.resolve(this.rootPath, keyAsTxtFile);
const commonPath = path.resolve(this.rootPath);

if (!fullPath.startsWith(commonPath)) {
throw new Error(
`Invalid key: ${key}. Key should be relative to the root path. ` +
`Root path: ${this.rootPath}, Full path: ${fullPath}`
);
}

return fullPath;
} catch (e) {
throw new Error(
`Error getting full path for key: ${key}.\nError: ${JSON.stringify(e)}`
`Error getting full path for key: ${key}.\nError: ${String(e)}`
);
}
}
Expand Down
19 changes: 19 additions & 0 deletions langchain/src/storage/tests/file_system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,23 @@ describe("LocalFileStore", () => {
).toEqual([value1, value2]);
await fs.promises.rm(secondaryRootPath, { recursive: true, force: true });
});

test("Should disallow attempts to traverse paths outside of a subfolder", async () => {
const encoder = new TextEncoder();
const store = await LocalFileStore.fromPath(secondaryRootPath);
const value1 = new Date().toISOString();
await expect(
store.mset([["../foo", encoder.encode(value1)]])
).rejects.toThrowError();
await expect(
store.mset([["/foo", encoder.encode(value1)]])
).rejects.toThrowError();
await expect(
store.mset([["\\foo", encoder.encode(value1)]])
).rejects.toThrowError();
await expect(store.mget(["../foo"])).rejects.toThrowError();
jacoblee93 marked this conversation as resolved.
Show resolved Hide resolved
await expect(store.mget(["/foo"])).rejects.toThrowError();
await expect(store.mget(["\\foo"])).rejects.toThrowError();
await fs.promises.rm(secondaryRootPath, { recursive: true, force: true });
});
});
Loading