Fix KV and R2 list() with long (≥50ch) prefixes (#691)

* Add failing test for KV `.list()` with long prefix

* Remove `LIKE` clause from `KV#list`

* Remove escaping code

* Fix linter errors

* Use lower-case for SQL functions

* Ensure KV long-prefix test uses exact namespace length

* Allow max-length R2 keys

* Fix R2 `list()` with long prefix

---------

Co-authored-by: bcoll <[email protected]>

Author: huw

packages/miniflare/src/workers/r2/bucket.worker.ts

@@ -15,7 +15,6 @@ import {
   all,
   base64Decode,
   base64Encode,
-  escapeLike,
   get,
   maybeApply,
   readPrefix,
@@ -231,12 +230,12 @@ function sqlStmts(db: TypedSql) {
     ];
     // TODO: consider applying same `:start_after IS NULL` trick to KeyValueStore
     return db.stmt<
-      { limit: number; escaped_prefix: string; start_after: string | null },
+      { limit: number; prefix: string; start_after: string | null },
       Omit<ObjectRow, "blob_id"> & Pick<ObjectRow, ExtraColumns[number]>
     >(`
       SELECT ${columns.join(", ")}
       FROM _mf_objects
-      WHERE key LIKE :escaped_prefix || '%' ESCAPE '\\'
+      WHERE substr(key, 1, length(:prefix)) = :prefix
       AND (:start_after IS NULL OR key > :start_after)
       ORDER BY key LIMIT :limit
     `);
@@ -380,7 +379,6 @@ function sqlStmts(db: TypedSql) {
     listMetadata: db.stmt<
       {
         limit: number;
-        escaped_prefix: string;
         start_after: string | null;
         prefix: string;
         delimiter: string;
@@ -407,7 +405,7 @@ function sqlStmts(db: TypedSql) {
         -- NOTE: we'll ignore metadata for delimited prefix rows, so it doesn't matter which keys' we return
         version, size, etag, uploaded, checksums, http_metadata, custom_metadata
       FROM _mf_objects
-      WHERE key LIKE :escaped_prefix || '%' ESCAPE '\\'
+      WHERE substr(key, 1, length(:prefix)) = :prefix
       AND (:start_after IS NULL OR key > :start_after)
       GROUP BY delimited_prefix_or_key -- Group keys with same delimited prefix into a row, leaving others in their own rows
       ORDER BY last_key LIMIT :limit;
@@ -878,7 +876,7 @@ export class R2BucketObject extends MiniflareDurableObject {
 
     // Run appropriate query depending on options
     const params = {
-      escaped_prefix: escapeLike(prefix),
+      prefix,
       start_after: startAfter ?? null,
       // Increase the queried limit by 1, if we return this many results, we
       // know there are more rows. We'll truncate to the original limit before
@@ -891,9 +889,7 @@ export class R2BucketObject extends MiniflareDurableObject {
     let nextCursorStartAfter: string | undefined;
 
     if (delimiter !== undefined) {
-      const rows = all(
-        this.#stmts.listMetadata({ ...params, prefix, delimiter })
-      );
+      const rows = all(this.#stmts.listMetadata({ ...params, delimiter }));
 
       // If there are more results, we'll be returning a cursor
       const hasMoreRows = rows.length === limit + 1;

packages/miniflare/src/workers/r2/validator.worker.ts

@@ -178,7 +178,7 @@ export class Validator {
 
   key(key: string): Validator {
     const keyLength = Buffer.byteLength(key);
-    if (keyLength >= R2Limits.MAX_KEY_SIZE) {
+    if (keyLength > R2Limits.MAX_KEY_SIZE) {
       throw new InvalidObjectName();
     }
     return this;

packages/miniflare/src/workers/shared/index.worker.ts

@@ -45,7 +45,7 @@ export {
 } from "./router.worker";
 export type { RouteHandler } from "./router.worker";
 
-export { get, all, drain, escapeLike } from "./sql.worker";
+export { get, all, drain } from "./sql.worker";
 export type {
   TypedValue,
   TypedResult,

packages/miniflare/src/workers/shared/keyvalue.worker.ts

@@ -81,26 +81,21 @@ function sqlStmts(db: TypedSql) {
     list: db.stmt<
       {
         now: number;
-        escaped_prefix: string;
+        prefix: string;
         start_after: string;
         limit: number;
       },
       Omit<Row, "blob_id">
     >(
       `SELECT key, expiration, metadata FROM _mf_entries
-        WHERE key LIKE :escaped_prefix || '%' ESCAPE '\\'
+        WHERE substr(key, 1, length(:prefix)) = :prefix
         AND key > :start_after
         AND (expiration IS NULL OR expiration >= :now)
         ORDER BY key LIMIT :limit`
     ),
   };
 }
 
-function escapePrefix(prefix: string) {
-  // Prefix all instances of `\`, `_` and `%` with `\`
-  return prefix.replace(/[\\_%]/g, "\\$&");
-}
-
 function rowEntry<Metadata>(entry: Omit<Row, "blob_id">): KeyEntry<Metadata> {
   return {
     key: entry.key,
@@ -238,7 +233,7 @@ export class KeyValueStorage<Metadata = unknown> {
   async list(opts: KeyEntriesQuery): Promise<KeyEntries<Metadata>> {
     // Find non-expired entries matching query after cursor
     const now = this.#timers.now();
-    const escaped_prefix = escapePrefix(opts.prefix ?? "");
+    const prefix = opts.prefix ?? "";
     // Note the "" default here prohibits empty string keys. The consumers
     // of this class are KV and Cache. KV validates keys are non-empty.
     // Cache keys are usually URLs, but can be customised with `cf.cacheKey`.
@@ -252,7 +247,7 @@ export class KeyValueStorage<Metadata = unknown> {
     const limit = opts.limit + 1;
     const rowsCursor = this.#stmts.list({
       now,
-      escaped_prefix,
+      prefix,
       start_after,
       limit,
     });

packages/miniflare/src/workers/shared/sql.worker.ts

@@ -124,8 +124,3 @@ export function drain<R extends TypedResult>(cursor: TypedSqlStorageCursor<R>) {
   for (const _ of cursor) {
   }
 }
-
-export function escapeLike(prefix: string) {
-  // Prefix all instances of `\`, `_` and `%` with `\`
-  return prefix.replace(/[\\_%]/g, "\\$&");
-}

packages/miniflare/test/plugins/kv/index.spec.ts

@@ -461,6 +461,14 @@ test("paginates keys matching prefix", listMacro, {
     [{ name: "section1key3" }],
   ],
 });
+test("list: accepts long prefix", async (t) => {
+  const { kv, ns } = t.context;
+  // Max key length, minus padding for `context.ns`
+  const longKey = "x".repeat(512 - ns.length);
+  await kv.put(longKey, "value");
+  const page = await kv.list({ prefix: ns + longKey });
+  t.deepEqual(page.keys, [{ name: ns + longKey }]);
+});
 test("list: paginates with variable limit", async (t) => {
   const { kv, ns } = t.context;
   await kv.put("key1", "value1");

packages/miniflare/test/plugins/r2/index.spec.ts

@@ -737,7 +737,15 @@ test(
     ],
   }
 );
-
+test("list: accepts long prefix", async (t) => {
+  const { r2, ns } = t.context;
+  // Max key length, minus padding for `context.ns`
+  const longKey = "x".repeat(1024 - ns.length);
+  await r2.put(longKey, "value");
+  const { objects } = await r2.list({ prefix: ns + longKey });
+  t.is(objects.length, 1);
+  t.is(objects[0].key, ns + longKey);
+});
 test("list: returns metadata with objects", async (t) => {
   const { r2, ns } = t.context;
   const start = Date.now();