-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(remix-server-runtime): fix invalid character error in cookie serialize #1290
Conversation
Hi @nimaa77, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
This should close #1282 instead of 128 ❤️ |
ah, my bad @shamsup thanks man |
Binary data is out of my wheelhouse, @mjackson can you take a look at this? If you approve it I'll deal w/ the merge conflicts and merge it in. |
Hi @nimaa77 A user of our lib, (remix-auth-supabase), needing Korean char encoding, encounters a bug (mitchelvanbever/remix-auth-supabase#15) and points me to your pull request. We both console.log our session cookie and face an other issue : the size increase a lot (≈ x2) and trigger cookie max size limit (4096 bytes) With current latest remix version : 3204 bytes console.log("before", btoa(JSON.stringify(value))) With the fix : 6404 bytes console.log("after", btoa(converted)) I could deal with it and open a discussion with our main maintainer (to keep only minimum needed session datas). |
What do you think using I try this and unit test seems still ok : function encodeData(value) {
let stringified = JSON.stringify(value);
let converted = encodeURIComponent(stringified);
return btoa(converted);
}
function decodeData(value) {
try {
let decodedData = atob(value);
let converted = decodeURIComponent(decodedData);
return JSON.parse(converted);
} catch (error) {
return {};
}
} NB : the output value will not be a json but an escaped string Before : {"sb:session":{"provider_token":"9d9l4spm After (decodeURIComponent): %7B%22sb%3Asession%22%3A%7B%22provider_token%22%3A%229d9 |
Why not set encoding to UTf8 in Buffer. from? |
Yes, and I also submit a pr about this, and I think would better resolve this in the root |
This would work if the cookie code were environment specific, but it looks like it is shared for every platform. Changing the implementation in the node-specific |
As @rphlmr mentions, conversions using Here are the results of an actual experiment using Google auth data for encoding.
Using Although I didn't know this PR existed, so I created another one (#2257). I'll leave it up to you guys to decide if @nimaa77 will continue to respond here or merge mine. |
@nimaa77 Could you please fix conflicts so that this PR can be properly reviewed by the team? 🙏🏼 |
Closing this, as we're fixing this in #2257 (see @mjackson's #1290 (comment)) |
Description
you save a cookie that contains utf-8 characters as it's value, you don't get the right content after reading the cookie.
this is because atob and btoa function that used inside cookie module and these functions don't support utf-8.
as MDN suggested in their docs, you can convert the utf8 content into binary string and then pass it to
btoa
function and vice versa for ...this is what this Pull Request is all about, fixing the issue with saving a cookie with utf8 text content
How to reproduce
https://codesandbox.io/s/dazzling-monad-0wt38?file=/app/routes/index.jsx
closes: #1282