Harden file-backed key-value store keys

.changeset/eff-219-key-value-store-file-keys.md

@@ -0,0 +1,5 @@
+---
+"effect": patch
+---
+
+Reject empty, `.` and `..` keys in file-backed key-value stores.

packages/effect/src/unstable/persistence/KeyValueStore.ts

@@ -341,7 +341,12 @@ export const layerMemory: Layer.Layer<KeyValueStore> = Layer.sync(KeyValueStore)
  *
  * **Details**
  *
- * The directory is created if needed, and each key is encoded as a file name.
+ * The directory is created if needed, and each key is percent-encoded as a
+ * single file name. Empty keys, `.` and `..` are rejected. Keys are only
+ * guaranteed to be distinct on case-sensitive file systems.
+ *
+ * `clear` removes the directory recursively, so it must not be shared with
+ * unrelated data.
  *
  * @category layers
  * @since 4.0.0
@@ -352,68 +357,86 @@ export const layerFileSystem = (
   Layer.effect(KeyValueStore)(Effect.gen(function*() {
     const fs = yield* FileSystem.FileSystem
     const path = yield* Path.Path
-    const keyPath = (key: string) => path.join(directory, encodeURIComponent(key))
+    const withKeyPath = <A>(
+      method: string,
+      key: string,
+      f: (path: string) => Effect.Effect<A, KeyValueStoreError>
+    ): Effect.Effect<A, KeyValueStoreError> =>
+      key.length === 0 || key === "." || key === ".."
+        ? Effect.fail(
+          new KeyValueStoreError({
+            method,
+            key,
+            message: `Invalid key ${key}`
+          })
+        )
+        : f(path.join(directory, encodeURIComponent(key)))
 
     if (!(yield* fs.exists(directory))) {
       yield* fs.makeDirectory(directory, { recursive: true })
     }
 
     return make({
       get: (key: string) =>
-        Effect.catchTag(
-          fs.readFileString(keyPath(key)),
-          "PlatformError",
-          (cause) =>
-            cause.reason._tag === "NotFound" ? Effect.undefined : Effect.fail(
-              new KeyValueStoreError({
-                method: "get",
-                key,
-                message: `Unable to get item with key ${key}`,
-                cause
-              })
-            )
-        ),
+        withKeyPath("get", key, (path) =>
+          Effect.catchTag(
+            fs.readFileString(path),
+            "PlatformError",
+            (cause) =>
+              cause.reason._tag === "NotFound" ? Effect.undefined : Effect.fail(
+                new KeyValueStoreError({
+                  method: "get",
+                  key,
+                  message: `Unable to get item with key ${key}`,
+                  cause
+                })
+              )
+          )),
       getUint8Array: (key: string) =>
-        Effect.catchTag(
-          fs.readFile(keyPath(key)),
-          "PlatformError",
-          (cause) =>
-            cause.reason._tag === "NotFound" ? Effect.undefined : Effect.fail(
+        withKeyPath("getUint8Array", key, (path) =>
+          Effect.catchTag(
+            fs.readFile(path),
+            "PlatformError",
+            (cause) =>
+              cause.reason._tag === "NotFound" ? Effect.undefined : Effect.fail(
+                new KeyValueStoreError({
+                  method: "getUint8Array",
+                  key,
+                  message: `Unable to get item with key ${key}`,
+                  cause
+                })
+              )
+          )),
+      set: (key: string, value: string | Uint8Array) =>
+        withKeyPath("set", key, (path) =>
+          Effect.mapError(
+            typeof value === "string" ? fs.writeFileString(path, value) : fs.writeFile(path, value),
+            (cause) =>
               new KeyValueStoreError({
-                method: "getUint8Array",
+                method: "set",
                 key,
-                message: `Unable to get item with key ${key}`,
+                message: `Unable to set item with key ${key}`,
                 cause
               })
-            )
-        ),
-      set: (key: string, value: string | Uint8Array) =>
-        Effect.mapError(
-          typeof value === "string" ? fs.writeFileString(keyPath(key), value) : fs.writeFile(keyPath(key), value),
-          (cause) =>
+          )),
+      remove: (key: string) =>
+        withKeyPath("remove", key, (path) =>
+          Effect.mapError(fs.remove(path), (cause) =>
             new KeyValueStoreError({
-              method: "set",
+              method: "remove",
               key,
-              message: `Unable to set item with key ${key}`,
+              message: `Unable to remove item with key ${key}`,
               cause
-            })
-        ),
-      remove: (key: string) =>
-        Effect.mapError(fs.remove(keyPath(key)), (cause) =>
-          new KeyValueStoreError({
-            method: "remove",
-            key,
-            message: `Unable to remove item with key ${key}`,
-            cause
-          })),
+            }))),
       has: (key: string) =>
-        Effect.mapError(fs.exists(keyPath(key)), (cause) =>
-          new KeyValueStoreError({
-            method: "has",
-            key,
-            message: `Unable to check existence of item with key ${key}`,
-            cause
-          })),
+        withKeyPath("has", key, (path) =>
+          Effect.mapError(fs.exists(path), (cause) =>
+            new KeyValueStoreError({
+              method: "has",
+              key,
+              message: `Unable to check existence of item with key ${key}`,
+              cause
+            }))),
       clear: Effect.mapError(
         Effect.andThen(
           fs.remove(directory, { recursive: true }),

packages/platform-node/test/KeyValueStore.test.ts

@@ -0,0 +1,51 @@
+import * as NodeFileSystem from "@effect/platform-node/NodeFileSystem"
+import * as NodePath from "@effect/platform-node/NodePath"
+import { assert, describe, it } from "@effect/vitest"
+import * as Effect from "effect/Effect"
+import * as FileSystem from "effect/FileSystem"
+import * as Layer from "effect/Layer"
+import * as KeyValueStore from "effect/unstable/persistence/KeyValueStore"
+
+const platformLayer = Layer.merge(NodeFileSystem.layer, NodePath.layer)
+
+describe("KeyValueStore / layerFileSystem", () => {
+  it.effect("rejects invalid keys without modifying the file system", () =>
+    Effect.gen(function*() {
+      const fs = yield* FileSystem.FileSystem
+      const root = yield* fs.makeTempDirectoryScoped()
+      const directory = `${root}/store`
+      const sibling = `${root}/sibling.txt`
+      yield* fs.makeDirectory(directory)
+      yield* fs.writeFileString(sibling, "sibling")
+
+      yield* Effect.gen(function*() {
+        const store = yield* KeyValueStore.KeyValueStore
+
+        for (const key of ["", ".", ".."]) {
+          const operations = [
+            ["get", Effect.asVoid(store.get(key))],
+            ["getUint8Array", Effect.asVoid(store.getUint8Array(key))],
+            ["set", Effect.asVoid(store.set(key, "value"))],
+            ["remove", Effect.asVoid(store.remove(key))],
+            ["has", Effect.asVoid(store.has(key))]
+          ] as const
+
+          for (const [method, operation] of operations) {
+            const error = yield* Effect.flip(operation)
+            assert.instanceOf(error, KeyValueStore.KeyValueStoreError)
+            assert.strictEqual(error.method, method)
+            assert.strictEqual(error.key, key)
+          }
+        }
+      }).pipe(
+        Effect.provide(KeyValueStore.layerFileSystem(directory).pipe(Layer.provide(platformLayer)))
+      )
+
+      assert.isTrue(yield* fs.exists(directory))
+      assert.deepStrictEqual(yield* fs.readDirectory(directory), [])
+      assert.strictEqual(yield* fs.readFileString(sibling), "sibling")
+    }).pipe(
+      Effect.scoped,
+      Effect.provide(NodeFileSystem.layer)
+    ))
+})