fix: cookie handling not working with Hono framework - #1689
Conversation
perpil
changed the title
|
@perpil thanks for the PR but this fails WPT tests, can you PTAL? |
|
This test is expected to fail now that cookies are enabled:
I believe the other failures are also expected, but don't have enough familiarity with the WPT tests to know for sure. Is the correct fix to add |
|
Personally, I am reluctant to exclude the entire file containing the tests that are currently failing. That is because there are other tests I want to pass. It would be great if the test runner could be improved to allow specifying files and test cases for exclusion. runSuite(import.meta.url, runTestDynamic, [
["request-headers.any.js", 'Adding invalid request header "Cookie: KO"'], // Exclude specific test cases.
["exclude-all-tests.any.js"], // Exclude all tests contained in that file.
]); |
|
Ok, that makes sense @nabetti1720 I had the filename wrong and didn't realize how many tests were in there. I'll see what I can do to have it skip certain errors. |
Update test harness to allow providing a filename and array of errors to ignore. If the array is provided, only the errors to ignore are allowed, the rest of the tests must pass. Update fetch.api.request.test.ts to ignore errors related to Cookie headers being allowed on Request objects
|
I've added the ability to skip individual tests in a file based on error string or regex and modified it to skip the 3 errors that are expected. Wpt tests are passing, should be good to review now. |
|
@richarddavison The modifications to the harness conflict with my PR, so please prioritize this one. :) |
| [ | ||
| "request-headers.any.js", | ||
| [ | ||
| '[Adding invalid request header "Cookie: KO"] assert_equals: expected (object) null but got (string) "KO"', | ||
| '[Adding invalid request header "Cookie2: KO"] assert_equals: expected (object) null but got (string) "KO"', | ||
| '[Check that request constructor is filtering headers provided as init parameter] assert_equals: expected (object) null but got (string) "potato"', | ||
| ], | ||
| ], | ||
| ]); |
There was a problem hiding this comment.
Can we do a more controlled way of skipping tests? I wonder if this is an XY problem. Why doesn't WPT allow cookie or cookie2 as request header? Is not none spec compliant?
https://fetch.spec.whatwg.org/#cookie-header
https://fetch.spec.whatwg.org/#set-cookie-header
There was a problem hiding this comment.
As I understand WPT is meant for browser based tests. Cookie headers in the browser aren't allowed on Request objects. Since one of the use cases for Node and llrt is server-side, some of the browser cookie related tests are expected to fail. The WPT tests in node seem to have a similar mechanism in the test harness to allow skipping tests: https://github.com/nodejs/node/blob/main/test/wpt/README.md
There are a couple of ways of solving this including commenting out the tests. Since the tests aren't named, I couldn't find a way of omitting tests by name, but let me know if you see a better way. There are multiple tests cases that emit '[Check that request constructor is filtering headers provided as init parameter] assert_equals: expected (object) null but got (string) "potato"' which is unfortunate because it isn't limited to a single test. I believe the way I've implemented it is the closest to how Node does it.
4 participants
Issue # (if available)
Resolves #1638
Description of changes
Exempt cookie headers from being stripped, they are ok for server-side runtimes.
Checklist
tests/unitand/or in Rust for my feature if neededmake fixto format JS and apply Clippy auto fixesmake checktypes/directoryBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.