fix: fix cookie parsing to allow invalid uri encoding #4915

jessezhang91 · 2023-08-08T14:37:12Z

Overview

Cookies with invalid URI encoding throws a URIError: URI malformed error and bails out of processing.

Cookie values do not need to be URL encoded. Looking at the cookie library used by cookie-parser and many others, they do a tryDecode and return the raw value on failure to decode.

What is it?

Feature / enhancement
Bug
Docs / tests / types / typos

Description

I updated the cookie parsing function to mimic this tryDecode functionality and added a test to validate that.

Use cases and why

A cookie header with foo=%bar should have a cookie with key foo have value %bar and the rest of the app should continue to function. Currently, it gets a URIError: URI malformed and bails with a server error.

Checklist:

My code follows the developer guidelines of this project
I have performed a self-review of my own code
I have made corresponding changes to the documentation
Added new tests to cover the fix / functionality

netlify · 2023-08-08T14:37:14Z

❌ Deploy Preview for qwik-insights failed.

Name	Link
🔨 Latest commit	`2578d22`
🔍 Latest deploy log	https://app.netlify.com/sites/qwik-insights/deploys/64e39f411620a400081a42bf

stackblitz · 2023-08-08T14:37:14Z

Run & review this pull request in StackBlitz Codeflow.

jessezhang91 · 2023-08-21T16:51:05Z

Bump on this PR. We're running into production issues with this due to what looks like ad tracking cookies. It's not that high volume but it's not insignificant.

mhevery · 2023-08-21T17:27:40Z

packages/qwik-city/middleware/request-handler/cookie.ts

@@ -52,15 +52,26 @@ const createSetCookieValue = (cookieName: string, cookieValue: string, options:
  return c.join('; ');
 };

+function tryDecodeUriComponent(str: string) {
+  if (str.indexOf('%') === -1) {


I think this check is not-needed. If not %, then decodeURIComponent just returns a normal string.

Yeah, agreed. I added that only because the cookie library had something like that (probably for optimization?).

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@builder.io/qwik](https://qwik.builder.io/) ([source](https://togithub.com/BuilderIO/qwik)) | [`1.2.7` -> `1.2.10`](https://renovatebot.com/diffs/npm/@builder.io%2fqwik/1.2.7/1.2.10) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@builder.io%2fqwik/1.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@builder.io%2fqwik/1.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@builder.io%2fqwik/1.2.7/1.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@builder.io%2fqwik/1.2.7/1.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>BuilderIO/qwik (@builder.io/qwik)</summary> ### [`v1.2.10`](https://togithub.com/BuilderIO/qwik/releases/tag/v1.2.10) [Compare Source](https://togithub.com/BuilderIO/qwik/compare/v1.2.9...v1.2.10) ##### What's Changed - fix: docs/cookbook/index.mdx link to portal page by [@Craiqser](https://togithub.com/Craiqser) in [QwikDev/qwik#5022 - qwik-labs: fix prettier estree error by [@gioboa](https://togithub.com/gioboa) in [QwikDev/qwik#5027 - insights: add existsSync check by [@gioboa](https://togithub.com/gioboa) in [QwikDev/qwik#5026 - chore(insights): correct DB migration script by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#5024 - fix(core): Report errors to browser global error handler by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#5029 - style(pnpm-lock.yaml): synchronize lockfile version with version of pnpm in use by [@jensmeindertsma](https://togithub.com/jensmeindertsma) in [QwikDev/qwik#4905 - fix(starter): missing key attrib on `routerHead` component on `1.2.9` by [@zanettin](https://togithub.com/zanettin) in [QwikDev/qwik#5025 - 1.2.10 by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#5035 **Full Changelog**: QwikDev/qwik@v1.2.9...v1.2.10 ### [`v1.2.9`](https://togithub.com/BuilderIO/qwik/releases/tag/v1.2.9) [Compare Source](https://togithub.com/BuilderIO/qwik/compare/v1.2.8...v1.2.9) #### What's Changed - docs: Vercel environment variables by [@gioboa](https://togithub.com/gioboa) in [QwikDev/qwik#4993 - refactor(create-qwik): add yargs in create-qwik for better DX by [@MrWaip](https://togithub.com/MrWaip) in [QwikDev/qwik#4932 - feat(adapter): firebase adapter by [@leifermendez](https://togithub.com/leifermendez) in [QwikDev/qwik#4778 - Pr chore by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4994 - fix: Link component `reload` functionality by [@zanettin](https://togithub.com/zanettin) in [QwikDev/qwik#4917 - docs(qwikcity): html attributes docs page by [@bab2683](https://togithub.com/bab2683) in [QwikDev/qwik#4961 - feat(qwik-city): add a way to get server-side env vars from `onStaticGenerate` by [@Kocal](https://togithub.com/Kocal) in [QwikDev/qwik#4912 - fix(eslint): allow eslint to accept FunctionComponent inside lexical scopes by [@nynevi](https://togithub.com/nynevi) in [QwikDev/qwik#4900 - refactor(cloudflare-pages): allow PlatformCloudflarePages.env to be undefined by [@bangonkali](https://togithub.com/bangonkali) in [QwikDev/qwik#4941 - docs: dynamic alt image by [@the-r3aper7](https://togithub.com/the-r3aper7) in [QwikDev/qwik#4999 - docs: add responsive image recipe by [@fabiobiondi](https://togithub.com/fabiobiondi) in [QwikDev/qwik#5002 - docs: community project update image url by [@the-r3aper7](https://togithub.com/the-r3aper7) in [QwikDev/qwik#5000 - docs: enhance image documentation by [@fabiobiondi](https://togithub.com/fabiobiondi) in [QwikDev/qwik#5005 - added qwik-meet in showcase by [@harshmangalam](https://togithub.com/harshmangalam) in [QwikDev/qwik#5007 - docs: Update index.mdx by [@the-r3aper7](https://togithub.com/the-r3aper7) in [QwikDev/qwik#5010 - docs: update side bar by [@the-r3aper7](https://togithub.com/the-r3aper7) in [QwikDev/qwik#5009 - fix(qwik-city): enable matching route and pathname with an optional t… by [@pleclech](https://togithub.com/pleclech) in [QwikDev/qwik#5004 - docs(image): add details by [@fabiobiondi](https://togithub.com/fabiobiondi) in [QwikDev/qwik#5014 - insights(feat): qwikInsights Vite Plugin + save symbol details by [@gioboa](https://togithub.com/gioboa) in [QwikDev/qwik#5011 - chore(insights): Remove deprecated InferModel by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#5018 - fix: add options to qwik vite plugin by [@jessezhang91](https://togithub.com/jessezhang91) in [QwikDev/qwik#4983 - 1.2.9 by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#5021 #### New Contributors - [@MrWaip](https://togithub.com/MrWaip) made their first contribution in [QwikDev/qwik#4932 - [@bab2683](https://togithub.com/bab2683) made their first contribution in [QwikDev/qwik#4961 - [@Kocal](https://togithub.com/Kocal) made their first contribution in [QwikDev/qwik#4912 - [@nynevi](https://togithub.com/nynevi) made their first contribution in [QwikDev/qwik#4900 - [@bangonkali](https://togithub.com/bangonkali) made their first contribution in [QwikDev/qwik#4941 - [@pleclech](https://togithub.com/pleclech) made their first contribution in [QwikDev/qwik#5004 **Full Changelog**: QwikDev/qwik@v1.2.8...v1.2.9 ### [`v1.2.8`](https://togithub.com/BuilderIO/qwik/releases/tag/v1.2.8) [Compare Source](https://togithub.com/BuilderIO/qwik/compare/v1.2.7...v1.2.8) ##### What's Changed - docs: cleanup by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4945 - chore: update issue templates by [@shairez](https://togithub.com/shairez) in [QwikDev/qwik#4962 - docs: correctly generate edit URLs for github by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4963 - Typo in FAQ by [@ThatJSGuy](https://togithub.com/ThatJSGuy) in [QwikDev/qwik#4953 - feat(qwik-city): Show current route in container by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4954 - fix(docs): add overview link by [@fabiobiondi](https://togithub.com/fabiobiondi) in [QwikDev/qwik#4958 - Towards personalized SSG by [@eric-burel](https://togithub.com/eric-burel) in [QwikDev/qwik#4951 - fix(qwik-auth): remove qaction param from defaultCallbackUrl by [@ulic75](https://togithub.com/ulic75) in [QwikDev/qwik#4936 - fix(qwik-city): cleanup matchRouteRequest by [@gioboa](https://togithub.com/gioboa) in [QwikDev/qwik#4967 - fix(auth): Cookies get updated if session returns Updated Cookies by [@aliyss](https://togithub.com/aliyss) in [QwikDev/qwik#4960 - cli(library): fix release script by [@fabiobiondi](https://togithub.com/fabiobiondi) in [QwikDev/qwik#4957 - docs: Improved media, fixed height thumbnails by [@the-r3aper7](https://togithub.com/the-r3aper7) in [QwikDev/qwik#4970 - feat(qwik-city): Adding Script Tag to Head Tag via DocumentHead by [@Harkunwar](https://togithub.com/Harkunwar) in [QwikDev/qwik#3230 - fix: fix cookie parsing to allow invalid uri encoding by [@jessezhang91](https://togithub.com/jessezhang91) in [QwikDev/qwik#4915 - fix(qwikcity): respect X-Forwarded-Host header by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4982 - chore(docs): fix broken cloudflare build by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4984 - feat(insights): manage routes and timeline metrics by [@gioboa](https://togithub.com/gioboa) in [QwikDev/qwik#4971 - feat: complex form data by [@ulic75](https://togithub.com/ulic75) in [QwikDev/qwik#4634 - docs(portals): update the portal cookbook example by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4968 - chore(insights): fix build by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4986 - fix: only show qwik eslint errors in dev mode by [@jessezhang91](https://togithub.com/jessezhang91) in [QwikDev/qwik#4985 - docs: some progress on the update of the react-cheat-sheet section re… by [@nsdonato](https://togithub.com/nsdonato) in [QwikDev/qwik#4976 - chore(docs): Remove home-page from SSR to improve TTFB metric by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4988 - Pr no ssr by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4989 - cli: Add new Markdown route command by [@brandonpittman](https://togithub.com/brandonpittman) in [QwikDev/qwik#4955 - docs: update Prisma limitations by [@ruheni](https://togithub.com/ruheni) in [QwikDev/qwik#4241 - 1.2.8 by [@mhevery](https://togithub.com/mhevery) in [QwikDev/qwik#4992 ##### New Contributors - [@ThatJSGuy](https://togithub.com/ThatJSGuy) made their first contribution in [QwikDev/qwik#4953 - [@fabiobiondi](https://togithub.com/fabiobiondi) made their first contribution in [QwikDev/qwik#4958 - [@eric-burel](https://togithub.com/eric-burel) made their first contribution in [QwikDev/qwik#4951 - [@aliyss](https://togithub.com/aliyss) made their first contribution in [QwikDev/qwik#4960 - [@brandonpittman](https://togithub.com/brandonpittman) made their first contribution in [QwikDev/qwik#4955 - [@ruheni](https://togithub.com/ruheni) made their first contribution in [QwikDev/qwik#4241 **Full Changelog**: QwikDev/qwik@v1.2.7...v1.2.8 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 9pm on sunday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/ascorbic/unpic-img).

fix: fix cookie parsing to allow invalid uri encoding

d7307f4

zanettin requested a review from adamdbradley August 8, 2023 20:39

zanettin added the COMP: qwik-city label Aug 8, 2023

zanettin added the STATUS-2: PR waiting for review This PR is waiting for review and approval before merge label Aug 18, 2023

mhevery approved these changes Aug 21, 2023

View reviewed changes

fixup! fix: fix cookie parsing to allow invalid uri encoding

2578d22

mhevery enabled auto-merge (squash) August 21, 2023 17:31

mhevery merged commit 288ce73 into QwikDev:main Aug 21, 2023
19 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix cookie parsing to allow invalid uri encoding #4915

fix: fix cookie parsing to allow invalid uri encoding #4915

jessezhang91 commented Aug 8, 2023 •

edited

netlify bot commented Aug 8, 2023 •

edited

stackblitz bot commented Aug 8, 2023

jessezhang91 commented Aug 21, 2023

mhevery Aug 21, 2023

jessezhang91 Aug 21, 2023

fix: fix cookie parsing to allow invalid uri encoding #4915

fix: fix cookie parsing to allow invalid uri encoding #4915

Conversation

jessezhang91 commented Aug 8, 2023 • edited

Overview

What is it?

Description

Use cases and why

Checklist:

netlify bot commented Aug 8, 2023 • edited

❌ Deploy Preview for qwik-insights failed.

stackblitz bot commented Aug 8, 2023

jessezhang91 commented Aug 21, 2023

mhevery Aug 21, 2023

Choose a reason for hiding this comment

jessezhang91 Aug 21, 2023

Choose a reason for hiding this comment

jessezhang91 commented Aug 8, 2023 •

edited

netlify bot commented Aug 8, 2023 •

edited