elysiajs / elysia

Ergonomic Framework for Humans
https://elysiajs.com
MIT License
9.1k stars 193 forks source link

Cookie schema validation doesn't work with `aot: true` #641

Open bogeychan opened 1 month ago

bogeychan commented 1 month ago

What version of Elysia.JS is running?

1.0.20

What platform is your computer?

No response

What steps can reproduce the bug?

import { Elysia, t } from "elysia";

const app = new Elysia()
  .get(
    "/",
    ({ cookie: { hello } }) => {
      console.log(
        `The value of the "hello" cookie is ${
          hello.value
        } and it is of type ${typeof hello.value}`
      );
      return "Hello Elysia";
    },
    { cookie: t.Cookie({ hello: t.String() }) }
  )
  .listen(8080);

console.log(`${app.server!.url}`);

What is the expected behavior?

No response

What do you see instead?

  1. Doesn't throw an ValidationError when the hello cookie does not exist
  2. If I set the hello cookie equal to the string 123, it gets parsed as a number even though I explicitly stated it's of type string

Additional information

Reported on discord


works as expected if you configure:

new Elysia({ aot: false })
SaltyAom commented 1 month ago

Should be fix on 321a7c5, published under 1.0.21

bogeychan commented 1 month ago

@SaltyAom, It still converts the cookie value to number even though I specified t.String as the type

nvm, bun installed wrong version 😭

bogeychan commented 1 month ago

BUT i got an error when passing 123:

{
  "type": "validation",
  "on": "cookie",
  "property": "/hello",
  "message": "Expected string",
  "expected": {
    "hello": ""
  },
  "found": {
    "hello": 123
  },
  "errors": [
    {
      "type": 54,
      "schema": {
        "type": "string"
      },
      "path": "/hello",
      "value": 123,
      "message": "Expected string"
    }
  ]
}
bogeychan commented 1 month ago

same bug with new Elysia({ aot: false })

SaltyAom commented 1 month ago

Does this error still occur on the latest (1.0.22)?

Screenshot 2567-06-09 at 18 00 24
bogeychan commented 1 month ago

@SaltyAom, yes, it occurs on 1.0.23, you have to pass number instead of some char sequence:

image

and the handler has this schema:

{ cookie: t.Cookie({ hello: t.String() }) }

32 is a valid string, idk why it errors there. it doesnt do that in other cases

bogeychan commented 1 month ago

Like this test doesn't pass...

import { describe, it, expect } from "bun:test";
import { Elysia, t } from "elysia";

describe("Cookie", () => {
  it("???", async () => {
    const app = new Elysia().get("/", ({ cookie: { hello } }) => hello.value, {
      cookie: t.Cookie({
        hello: t.String(),
      }),
    });

    const response = await app.handle(
      new Request("http://localhost/", {
        headers: {
          Cookie: "hello=42",
        },
      })
    );

    expect(await response.text()).toEqual("42");
  });
});
SaltyAom commented 1 week ago

This is because cookie parser behavior will automatically parse a numeric string to numbers, which is inconsistent with other parsers and should be documented.

Maybe we should enforce this behavior on other parsers as well since it seems to improve DX. At least the t.Number() and t.Numeric() problems would be solved for new users.

What do you think @bogeychan?

bogeychan commented 1 week ago

I'd prefer no automatic parsing and use string as default because you end up with a lot of code like this doing type checking/casting and unnecessary number parsing could affect performance.

import { Elysia } from "elysia";

function libraryFunction(param: string) {} // code from external library

new Elysia().get("/", ({ cookie: { hello } }) => {
  return libraryFunction(`${hello.value}`);
  //                                 ^ (property) Cookie<number | string>.value: number | string
});

Having both t.Number() and t.Numeric() is actually annoying and can be confusing for new users.

Perhaps t.Number should accept both Number and Numeric values by default. (same with t.Boolean, ...) if explicite set so:

new Elysia().get("/", ({ cookie: { hello } }) => hello.value, {
  //                                                      ^ (property) Cookie<number>.value: number
  cookie: t.Cookie({
    hello: t.Number(),
  }),
});

// hello = "123" ✅
// hello = 123 ✅
// hello = "yay"❌
SaltyAom commented 1 week ago
  1. I agree with replacing number with numeric if possible, which probably will do later.
  2. Unfortunately many people seem to disagree with expanding auto-parsing behavior to query and header.
  3. I think cookie being the only inconsistent might be ok (just needs to be documented) because it's the only property that is stateful.

The counter-argument is that, what if a user sets a cookie as a number then once it's re-read it becomes a string? Most of the time this would not be an expected behavior because there's an illusion of treating cookie as a value

For example, a simple counter (which is a lot of cases, eg rate-limiting)

new Elysia()
    .get('/a', ({ cookie: { counter } }) => counter.value = (counter.value ?? 0) + 1)

Since the user set a counter as a number, if we don't do auto-parsing, a cookie would be a string which is not what the user set thus making it consistent with the user value.

What do you think?

bogeychan commented 1 week ago
  1. I agree with these people; i don't like side effects too (including cookie-parsing)

You won't end up with this weird behavior if you only allow string by default (instead of any), and if a user wants something else, they should specify that (somehow):

new Elysia().get(
  "/a",
  ({ cookie: { counter } }) => (counter.value = 42)
 //                                       ^ Type 'number' is not assignable to type 'string'.
);
new Elysia().get(
  "/a",
  ({ cookie: { counter } }) => (counter.value = (counter.value ?? 0) + 1),
 //                                                       ^ (property) Cookie<number>.value: number
  {
    cookie: t.Cookie({
      counter: t.Optional(t.Number()), // I want `number`
    }),
  }
);

Or you change some of elysia's cookie api:

new Elysia().get("/a", ({ cookie: { counter } }) => {
  console.log(counter.value); // incoming cookie value (type: readonly string)
  counter.set.value = 42; // outgoing cookie value (type: any)
});

Another option is to perform a code analysis and check how the cookie value is used, but this is not trivial and will have many bugs

SaltyAom commented 5 days ago

New behavior since Elysia 1.1.0-exp.18 will always parse value as string for all validators unless explicitly specified. Except query string as defined in RFC 3986, TLDR; query string could be either string or array of string.

See 50a5d92 44bf279.

Pending for stable 1.1.0 release.