denoland / fresh

The next-gen web framework.
https://fresh.deno.dev
MIT License
12.17k stars 623 forks source link

context params accessed via ctx.params in the handlers need to be decoded #2310

Open king8fisher opened 7 months ago

king8fisher commented 7 months ago

Imagine a route file named [product].tsx. When you handle a request, you are reaching for the param named "product". If the product sent contains a space, it comes with a %20 when reaching its value via ctx.params["product"]. The feeling is that it would be more convenient if returning value was already decoded according to how a browser would so that the user wouldn't need to guess which function to utilize.

export const handler: Handlers<ProductsPageProps> = {
  GET(_req, ctx) {
    const productName = decodeURI(ctx.params["product"]);
deer commented 7 months ago

Cool idea. I wrote a test like this:

Deno.test("param with encoding", async () => {
  await withFakeServe("./tests/fixture/dev.ts", async (server) => {
    const doc = await server.getHtml(`/decode-params/Hello%20World`);
    assertEquals(
      doc.querySelector("body")?.textContent,
      "Hello World",
    );
  });
});

which invokes:

import { defineRoute } from "$fresh/src/server/defines.ts";

export default defineRoute((req, ctx) => {
  return ctx.params.id;
});

Prior to the change, the test fails. But then I do this:

--- a/src/server/router.ts
+++ b/src/server/router.ts
@@ -189,9 +189,13 @@ export function getParamsAndRoute(
       const res = route.pattern.exec(url);

       if (res !== null) {
+        const decodedParams: Record<string, string | undefined> = {};
+        for (const [key, value] of Object.entries(res.pathname.groups)) {
+          decodedParams[key] = value ? decodeURIComponent(value) : value;
+        }
         return {
           route: route,
-          params: res.pathname.groups,
+          params: decodedParams,
           isPartial,
         };
       }

and the test starts passing 🎉

But I think such a brutal approach would be a breaking change. Who knows what sort of logic people have written with the expectation that things remain encoded. Tomorrow I'll do a more subtle approach involving a new configuration option that allows this to be enabled.

king8fisher commented 7 months ago

A possible why behind returning a raw value. decodeURI throws if original value arrives wrongly formatted.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURI

If decoding is the user's responsibility the they will handle the error.

Alternative would be to return a null for wrongly encoded values.

king8fisher commented 7 months ago

I just realized that I needed to encode my params in the urls too, otherwise whatever is passed may lack some portions of the data containing reserved symbols (for instance, after a # symbol that's part of the passed url).

It must be an overlook that proper constructions of the urls should indeed include an encoder.

<a href={`/products/${encodeURIComponent(product)}`}>

Now the mirrored decoding does not look too strange.

If everything I said is right, would it make sense to add a note somewhere in the docs? If so I will gladly PR this and it wouldn't hurt #2311