fix: handle non ASCII characters in cache-tag headers

lukasholzer commented 1 month ago

Description

Fixes https://linear.app/netlify/issue/FRB-1352/handle-non-ascii-characters-in-cache-tag-headers

Handles non ASCII characters in cache-tag headers by URI encoding them (does not encode / or valid characters just the non ASCII ones)

Documentation

Tests

Added e2e test case for cacheable page router pages with non-ascii paths

TODO

Relevant links (GitHub issues, etc.) or a picture of cute animal

github-actions[bot] commented 1 month ago

📊 Package size report `0%↑`

File	Before (Size / Gzip)	After (Size / Gzip)
`dist/run/handlers/cache.cjs`	`16.6 kB` / `4.5 kB`	^0.6%↑`16.7 kB` / ^0.8%↑`4.5 kB`
Total _{(Includes all files)}	`5.3 MB` / `959.1 kB`	^0%↑`5.3 MB` / ^0%↑`959.1 kB`
Tarball size	`912.0 kB`	^0%↑`912.1 kB`

Unchanged files

| File | Size (Size / Gzip) | | -------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------: | | `dist/build/advanced-api-routes.js` | `3.8 kB` / `1.2 kB` | | `dist/build/cache.js` | `1.0 kB` / `414 B` | | `dist/build/content/next-shims/telemetry-storage.cjs` | `1.6 kB` / `659 B` | | `dist/build/content/prerendered.js` | `8.6 kB` / `2.6 kB` | | `dist/build/content/server.js` | `8.7 kB` / `2.8 kB` | | `dist/build/content/static.js` | `3.6 kB` / `1.2 kB` | | `dist/build/functions/edge.js` | `20.2 kB` / `5.3 kB` | | `dist/build/functions/server.js` | `5.0 kB` / `1.6 kB` | | `dist/build/image-cdn.js` | `54.0 kB` / `11.1 kB` | | `dist/build/plugin-context.js` | `9.2 kB` / `2.7 kB` | | `dist/build/templates/handler-monorepo.tmpl.js` | `1.6 kB` / `671 B` | | `dist/build/templates/handler.tmpl.js` | `1.5 kB` / `622 B` | | `dist/build/verification.js` | `4.5 kB` / `1.5 kB` | | `dist/esm-chunks/chunk-5QSXBV7L.js` | `2.4 kB` / `842 B` | | `dist/esm-chunks/chunk-APO262HE.js` | `61.2 kB` / `11.1 kB` | | `dist/esm-chunks/chunk-GNGHTHMQ.js` | `55.6 kB` / `9.7 kB` | | `dist/esm-chunks/chunk-KGYJQ2U2.js` | `186.5 kB` / `32.9 kB` | | `dist/esm-chunks/chunk-OEQOKJGE.js` | `2.3 kB` / `977 B` | | `dist/esm-chunks/package-USM376UP.js` | `3.6 kB` / `1.4 kB` | | `dist/index.js` | `3.4 kB` / `1.1 kB` | | `dist/run/config.js` | `1.2 kB` / `595 B` | | `dist/run/constants.js` | `516 B` / `308 B` | | `dist/run/handlers/request-context.cjs` | `4.6 kB` / `1.5 kB` | | `dist/run/handlers/server.js` | `141.1 kB` / `33.0 kB` | | `dist/run/handlers/tracer.cjs` | `29.9 kB` / `6.3 kB` | | `dist/run/handlers/tracing.js` | `3.0 MB` / `418.4 kB` | | `dist/run/headers.js` | `7.3 kB` / `2.2 kB` | | `dist/run/next.cjs` | `23.4 kB` / `5.8 kB` | | `dist/run/regional-blob-store.cjs` | `19.5 kB` / `5.4 kB` | | `dist/run/revalidate.js` | `1.0 kB` / `475 B` | | `dist/shared/blobkey.js` | `742 B` / `399 B` | | `dist/shared/cache-types.cjs` | `1.3 kB` / `566 B` | | [`edge-runtime/lib/headers.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/headers.ts) | `1.9 kB` / `841 B` | | [`edge-runtime/lib/logging.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/logging.ts) | `115 B` / `121 B` | | [`edge-runtime/lib/middleware.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/middleware.ts) | `1.9 kB` / `815 B` | | [`edge-runtime/lib/next-request.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/next-request.ts) | `3.3 kB` / `1.1 kB` | | [`edge-runtime/lib/response.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/response.ts) | `9.2 kB` / `2.9 kB` | | [`edge-runtime/lib/routing.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/routing.ts) | `15.1 kB` / `3.9 kB` | | [`edge-runtime/lib/util.test.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/util.test.ts) | `1.6 kB` / `356 B` | | [`edge-runtime/lib/util.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/lib/util.ts) | `3.7 kB` / `1.3 kB` | | [`edge-runtime/matchers.json`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/matchers.json) | `3 B` / `23 B` | | [`edge-runtime/middleware.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/middleware.ts) | `2.4 kB` / `1.0 kB` | | [`edge-runtime/next.config.json`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/next.config.json) | `3 B` / `23 B` | | [`edge-runtime/README.md`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/README.md) | `992 B` / `509 B` | | [`edge-runtime/shim/index.js`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/shim/index.js) | `1.5 kB` / `717 B` | | [`edge-runtime/vendor.ts`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/edge-runtime/vendor.ts) | `807 B` / `330 B` | | `edge-runtime/vendor/deno.land/std@0.134.0/fmt/colors.ts` | `11.9 kB` / `2.5 kB` | | `edge-runtime/vendor/deno.land/std@0.134.0/testing/_diff.ts` | `9.6 kB` / `3.0 kB` | | `edge-runtime/vendor/deno.land/std@0.134.0/testing/asserts.ts` | `24.7 kB` / `5.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/_util/asserts.ts` | `854 B` / `461 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/_util/os.ts` | `644 B` / `355 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/abortable.ts` | `4.0 kB` / `1.0 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/deadline.ts` | `974 B` / `544 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/debounce.ts` | `2.2 kB` / `956 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/deferred.ts` | `1.5 kB` / `798 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/delay.ts` | `1.8 kB` / `845 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/mod.ts` | `465 B` / `241 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/mux_async_iterator.ts` | `2.5 kB` / `1.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/pool.ts` | `3.2 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/retry.ts` | `2.4 kB` / `1.0 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/async/tee.ts` | `2.1 kB` / `924 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/bytes/index_of_needle.ts` | `1.4 kB` / `668 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/crypto/timing_safe_equal.ts` | `875 B` / `442 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/datetime/to_imf.ts` | `1.3 kB` / `681 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/encoding/base64.ts` | `2.5 kB` / `1.0 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/encoding/base64url.ts` | `2.0 kB` / `872 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/flags/mod.ts` | `22.6 kB` / `5.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/fmt/colors.ts` | `12.4 kB` / `2.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/fmt/printf.ts` | `27.7 kB` / `7.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/http/cookie.ts` | `11.5 kB` / `3.6 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_core.ts` | `2.3 kB` / `716 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_events.d.ts` | `27.2 kB` / `5.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_events.mjs` | `28.0 kB` / `7.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_global.d.ts` | `1.7 kB` / `650 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_next_tick.ts` | `5.0 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_process/exiting.ts` | `138 B` / `138 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_process/process.ts` | `3.8 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_process/stdio.mjs` | `336 B` / `233 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_process/streams.mjs` | `4.0 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_stream.d.ts` | `53.2 kB` / `11.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_stream.mjs` | `91.2 kB` / `25.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_util/_util_callbackify.ts` | `4.3 kB` / `1.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/_utils.ts` | `5.9 kB` / `2.0 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/assert.ts` | `23.1 kB` / `4.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/assertion_error.ts` | `19.6 kB` / `6.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/async_hooks.ts` | `7.7 kB` / `2.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/buffer.ts` | `262 B` / `204 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/events.ts` | `303 B` / `221 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/_libuv_winerror.ts` | `7.8 kB` / `1.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/_listen.ts` | `561 B` / `342 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/_node.ts` | `443 B` / `335 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/_timingSafeEqual.ts` | `479 B` / `268 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/_utils.ts` | `2.4 kB` / `938 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/_winerror.ts` | `354.4 kB` / `64.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/ares.ts` | `2.4 kB` / `1.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/async_wrap.ts` | `4.0 kB` / `1.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/buffer.ts` | `3.5 kB` / `1.3 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/cares_wrap.ts` | `15.2 kB` / `3.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/config.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/connection_wrap.ts` | `2.6 kB` / `1.3 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/constants.ts` | `21.5 kB` / `5.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/contextify.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/credentials.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/crypto.ts` | `448 B` / `244 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/errors.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/fs_dir.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/fs_event_wrap.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/fs.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/handle_wrap.ts` | `1.8 kB` / `1.0 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/heap_utils.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/http_parser.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/icu.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/inspector.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/js_stream.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/messaging.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/mod.ts` | `3.1 kB` / `955 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/module_wrap.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/native_module.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/natives.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/node_file.ts` | `2.9 kB` / `1.5 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/node_options.ts` | `1.8 kB` / `989 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/options.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/os.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/performance.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/pipe_wrap.ts` | `10.4 kB` / `3.3 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/process_methods.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/report.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/serdes.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/signal_wrap.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/spawn_sync.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/stream_wrap.ts` | `9.3 kB` / `2.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/string_decoder.ts` | `504 B` / `261 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/symbols.ts` | `1.4 kB` / `828 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/task_queue.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/tcp_wrap.ts` | `13.1 kB` / `3.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/timers.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/tls_wrap.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/trace_events.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/tty_wrap.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/types.ts` | `5.7 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/udp_wrap.ts` | `12.4 kB` / `3.6 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/url.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/util.ts` | `4.0 kB` / `1.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/uv.ts` | `20.1 kB` / `3.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/v8.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/worker.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal_binding/zlib.ts` | `87 B` / `104 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/buffer.d.ts` | `73.6 kB` / `12.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/buffer.mjs` | `66.1 kB` / `10.6 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/crypto/_keys.ts` | `463 B` / `262 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/crypto/constants.ts` | `252 B` / `173 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/error_codes.ts` | `322 B` / `250 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/errors.ts` | `78.9 kB` / `17.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/fixed_queue.ts` | `4.4 kB` / `1.2 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/hide_stack_frames.ts` | `550 B` / `377 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/net.ts` | `3.1 kB` / `1.5 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/normalize_encoding.mjs` | `2.1 kB` / `500 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/options.ts` | `1.7 kB` / `959 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/primordials.mjs` | `1.8 kB` / `431 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/process/per_thread.mjs` | `7.8 kB` / `2.3 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/readline/callbacks.mjs` | `3.8 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/readline/utils.mjs` | `14.3 kB` / `3.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/streams/destroy.mjs` | `6.9 kB` / `1.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/streams/end-of-stream.mjs` | `7.1 kB` / `1.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/streams/utils.mjs` | `5.9 kB` / `1.2 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/util.mjs` | `4.0 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/util/comparisons.ts` | `16.6 kB` / `3.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/util/debuglog.ts` | `3.2 kB` / `1.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/util/inspect.mjs` | `71.5 kB` / `19.8 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/util/types.ts` | `3.7 kB` / `1.3 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/internal/validators.mjs` | `8.0 kB` / `2.1 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/process.ts` | `19.4 kB` / `5.2 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/stream.ts` | `671 B` / `346 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/string_decoder.ts` | `10.3 kB` / `3.3 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/util.ts` | `7.8 kB` / `2.2 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/node/util/types.ts` | `199 B` / `153 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/_constants.ts` | `2.0 kB` / `727 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/_interface.ts` | `728 B` / `369 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/_util.ts` | `5.0 kB` / `1.6 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/common.ts` | `1.2 kB` / `607 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/glob.ts` | `12.7 kB` / `3.9 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/mod.ts` | `1.4 kB` / `690 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/posix.ts` | `13.9 kB` / `3.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/separator.ts` | `259 B` / `209 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/path/win32.ts` | `28.5 kB` / `6.4 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/streams/write_all.ts` | `2.2 kB` / `598 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/testing/_diff.ts` | `11.6 kB` / `3.6 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/testing/_format.ts` | `705 B` / `462 B` | | `edge-runtime/vendor/deno.land/std@0.175.0/testing/asserts.ts` | `25.5 kB` / `5.7 kB` | | `edge-runtime/vendor/deno.land/std@0.175.0/types.d.ts` | `4.2 kB` / `1.2 kB` | | `edge-runtime/vendor/deno.land/x/html_rewriter@v0.1.0-pre.17/index.ts` | `4.5 kB` / `1.7 kB` | | `edge-runtime/vendor/deno.land/x/html_rewriter@v0.1.0-pre.17/vendor/asyncify.js` | `2.6 kB` / `931 B` | | `edge-runtime/vendor/deno.land/x/html_rewriter@v0.1.0-pre.17/vendor/html_rewriter.d.ts` | `2.7 kB` / `622 B` | | `edge-runtime/vendor/deno.land/x/html_rewriter@v0.1.0-pre.17/vendor/html_rewriter.js` | `28.4 kB` / `4.5 kB` | | `edge-runtime/vendor/deno.land/x/path_to_regexp@v6.2.1/index.ts` | `15.4 kB` / `4.2 kB` | | `edge-runtime/vendor/import_map.json` | `365 B` / `180 B` | | `edge-runtime/vendor/raw.githubusercontent.com/worker-tools/resolvable-promise/master/index.ts` | `1.8 kB` / `657 B` | | `edge-runtime/vendor/v1-7-0--edge-utils.netlify.app/logger/logger.ts` | `3.2 kB` / `747 B` | | `edge-runtime/vendor/v1-7-0--edge-utils.netlify.app/logger/mod.ts` | `29 B` / `49 B` | | [`manifest.yml`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/manifest.yml) | `31 B` / `51 B` | | [`package.json`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/package.json) | `3.2 kB` / `1.1 kB` | | [`README.md`](https://github.com/netlify/next-runtime/blob/fix/FRB-1352-ascii-characters-in-cache-tag/README.md) | `2.7 kB` / `1.1 kB` |

_{🤖 This report was automatically generated by pkg-size-action}

pieh commented 1 month ago

I wonder whether we might need to expand the encoding? AFAIK, encodeURL will leave reserved characters unencoded, which means a URL like http://foo.com/resource,123 would emit a cache tag with a comma in it. Should we use encodeURIComponent instead?

I don't think it's that simple - I was running some tests here and it seems to me that comma case need to be handled on different level than current change - current change only applies to page router, because app router already actually handles it fine (I did add test cases in last commit here for it)

The comma problem however applies right now to both Pages and App router in similar way, so just swapping currently added encodeURI with encodeURIComponent would be insufficient to fully support it.

Also encodeURIComponent would replace / with encoded character so this would be bigger departure from what we currently generate vs using encodeURI - maybe we would only want to target comma additionally here to limit changes? Or we treat those completely as implementation detail that can change at any time, then using that would be fine (tho debugging might be a bit more annoying with so many extra characters being encoded in header we produce)

pieh commented 1 month ago

I was looking into comma case, and this is getting quite not straight forward with our usage of split(',') in places like: https://github.com/netlify/next-runtime/blob/0b74e135b213de07e9b31bf27f517becd125c212/src/run/handlers/cache.cts#L142 https://github.com/netlify/next-runtime/blob/0b74e135b213de07e9b31bf27f517becd125c212/src/run/handlers/cache.cts#L422

it basically looks like we can't really encode this specifically because we can't distinguish between between comma being a separator and being actual part of one of the items.

It does feel like we kind of have to apply same handling and actually produce 2 (or more) cache tag if actual tag contains comma and instead make sure the way we interact with tag manifest blobs and purge cdn API also need to split on it - this might mean accidentally purging more content than desired but because we don't handle entire pipeline at least for app router - we just can't make it work well in this scenario in which case it would make sense to arrive and similar handling for both app and pages router?

pieh commented 1 month ago

I was doing much more tests with comma and this getting super confusing and annoying ...

cache tags for app router are generated differently when prerendered and when rendering on demand later ...

in prerendered comma does get encoded:

{
  path: '/product/事前レンダリングされていない,test'
  responseCacheTags: [
    '_N_T_/layout',
    '_N_T_/product/layout',
    '_N_T_/product/[slug]/layout',
    '_N_T_/product/[slug]/page',
    // notice '%2Ctest' which is ',test'
    '_N_T_/product/%E4%BA%8B%E5%89%8D%E3%83%AC%E3%83%B3%E3%83%80%E3%83%AA%E3%83%B3%E3%82%B0%2Ctest' 
  ]
}

while non-prerendered gets this:

{
  path: '/product/事前レンダリングされていない,test',
  responseCacheTags: [
    '_N_T_/layout',
    '_N_T_/product/layout',
    '_N_T_/product/[slug]/layout',
    '_N_T_/product/[slug]/page',
    // in here comma was not encoded so our splitting on ',' made 'test' into separate cache tag
    '_N_T_/product/%E4%BA%8B%E5%89%8D%E3%83%AC%E3%83%B3%E3%83%80%E3%83%AA%E3%83%B3%E3%82%B0%E3%81%95%E3%82%8C%E3%81%A6%E3%81%84%E3%81%AA%E3%81%84',
    'test'
  ]
}

I'm thinking that maybe we should split on both , and %2c to get ~consistent handling - even if it is not the exactly correct one, because it will always generate 2 cache tags if cache tag contain comma (encoded or not) and then some unrelated purge might purge those too, but because we are not getting encoded comma consistently - we can't really handle this in any other way?

orinokai commented 1 month ago

I'm thinking that maybe we should split on both , and %2c to get ~consistent handling

Maybe we could make the splitting a bit smarter and continue to split on comma, but only if the comma is followed by _N_T_?

pieh commented 1 month ago

Maybe we could make the splitting a bit smarter and continue to split on comma, but only if the comma is followed by _N_T_?

fetch cache tags are not prepended with _N_T_ and instead we get them as they are set by users - otherwise that would be awesome idea to be able to accurately distinguish between entries (that they all must start with _N_T_)

orinokai commented 1 month ago

fetch cache tags are not prepended with _NT

true, but the fetch cache tags are always prepended in the cacheValue.headers[NEXT_CACHE_TAGS_HEADER] string, so we can start splitting by ,_N_T_ after the first _N_T_

pieh commented 1 month ago

fetch cache tags are always prepended

If that's true, then it probably would work, but this feels way more complex than it should be and also more fragile to next.js changes I think. I'll give it a try regardless just to see how messy setup would have to be for it

pieh commented 1 month ago

Ughhh. There is another layer here with comma in fetch cache tags ... sigh

next: { tags: ["tagstart,tagend"] },

and

next: { tags: ["tagstart","tagend"] },

Will produce same thing (both prerendered and generated on demand) - this would still work with current code in this PR (albeit it would always treat both of above as the last one in practice), but smart splitting idea (which I hoped would result in more exact purges) wouldn't help here - how should:

revalidateTag('tagstart')
revalidateTag('tagstart,tagend')

those work when you can't distinguish them? the only thing that does work is splitting on comma again - but then how do you handle

revalidatePath('/foo,baz')

?

because we don't know which one (revalidateTag or revalidatePath) user called and both result in calling CacheHandler.revalidateTag and we would need different behavior based on wether it was path or a tag

Now I'm even more in favor of current approach that just works with both of those cases (it might purge more than needed if there is overlap with different cache tag after splitting on both comma and encoded comma, but this feels better than not being able to invalidate something at all)

orinokai commented 1 month ago

I think you're right - the current solution is the best because the impact of any problems is limited - the worst scenario I can think of is that a URL has a comma in it and, after splitting, the first part matches a layout, so a whole bunch of unintended pages get purged, but that's an edge case and why would you use a URL scheme like that anyway?!

opennextjs / opennextjs-netlify