Skip to content
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: #664 Handle URL-encoded cookie values #686

Closed
wants to merge 1 commit into from

Conversation

dajinchu
Copy link

@dajinchu dajinchu commented Mar 22, 2024

Summary

Fixes #664

Cookie values are often URL-encoded. In Rails, for example, there is no way to opt out of url-encoding cookies. So this code in the docs does not work.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: no

Copy link

@seantarzy seantarzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May rails devs not have this issue again

@jeremyallison
Copy link

@kwalker3690 any chance someone with write access could approve and merge this fix in the near future? 🙏 😊

@Mercy811
Copy link
Contributor

Hi @dajinchu, Amplitude Experiment Ruby SDK encodes cookie values with URL encoding first and then base64 the same as Amplitude Browser SDK. I confirmed that they should work well with each other.

In your example, it's been url encoded twice rather than not been URL encoded.

Fixed flow, proves what is broken:
Manually set an Amplitude cookie in your browser, with a value that has not been URL encoded, for example AMP_1310df2f88=JTdCJTIyZGV2aWNlSWQlMjIlM0ElMjJkZDU1YjIxNC0yNmY1LTQ5OTAtYjFiZi0zNTkzYTIxOTJlNDIlMjIlN0Q= <-- ⚠️ notice the = is not encoded into %3D here

btoa(encodeURIComponent('{"deviceId":"dd55b214-26f5-4990-b1bf-3593a2192e42"}'))
// 'JTdCJTIyZGV2aWNlSWQlMjIlM0ElMjJkZDU1YjIxNC0yNmY1LTQ5OTAtYjFiZi0zNTkzYTIxOTJlNDIlMjIlN0Q='

encodeURIComponent(btoa(encodeURIComponent('{"deviceId":"dd55b214-26f5-4990-b1bf-3593a2192e42"}')))
// 'JTdCJTIyZGV2aWNlSWQlMjIlM0ElMjJkZDU1YjIxNC0yNmY1LTQ5OTAtYjFiZi0zNTkzYTIxOTJlNDIlMjIlN0Q%3D'

@Mercy811
Copy link
Contributor

Hi @dajinchu, @amplitude/[email protected] is released with more logs in the catch block. Please upgrade to the latest version to get the exact error when it breaks.

@jeremyallison
Copy link

jeremyallison commented Jun 18, 2024

Amplitude Experiment Ruby SDK encodes cookie values with URL encoding first and then base64 the same as Amplitude Browser SDK

@Mercy811 they do, you're right.
But then when rails responds with a Set-Cookie header it URL encodes that base64 again. To my knowledge there is no workaround in rails to avoid this on server-side.

Here's an example (following the Experiment documentation) :

  def persist_device_id_in_cookie(cookie_name, device_id, country)
    cookies[cookie_name] = {
      value: AmplitudeExperiment::AmplitudeCookie.generate(device_id, new_format: true),
      domain: ".our-domain.#{country.host_tld}",
      httponly: false,
      secure: false
    }
  end

Although the base64 returned by AmplitudeCookie.generate is perfectly okay, the rails framework then outputs the following Set-Cookie header:

AMP_56feca8084=JTdCJTIyZGV2aWNlSWQlMjIlM0ElMjI1ZTMzMzc2OC1iZmI2LTQ2MjQtYWIwMS03N2I3ZTE3ODU2NDclMjIlN0Q%3D; domain=.our-domain.fr; path=/; SameSite=Lax; secure

And sure enough, with @amplitude/[email protected] :
Capture d’écran 2024-06-18 à 10 18 34

@Mercy811
Copy link
Contributor

Hi @jeremyallison, cookies are automatically URL encoded by the cookies helper for example, cookies[:user_name] = "John Doe". But it looks like there are ways to manually set cookies, for example https://api.rubyonrails.org/v4.0.1/classes/ActionDispatch/Response.html#method-i-set_cookie

@Mercy811 Mercy811 reopened this Jun 18, 2024
@jeremyallison
Copy link

jeremyallison commented Jun 19, 2024

@Mercy811 we are actually using Rails v7.1 : https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html
Same syntax as in the Amplitude Experiment documentation. The set_cookie method is quite old (v4.0.1), it doesn't exist anymore in modern versions of Rails.

I think the most straightforward way to see this issue is the way @dajinchu explained it; the example in the official Amplitude Ruby gem documentation unfortunately cannot set a cookie that the TS @amplitude/analytics-browser can read.

Thank you very much for your time and for reopening the PR 🙏

@Mercy811
Copy link
Contributor

Hi @jeremyallison, @amplitude/[email protected] should handle double url encoded values properly.

@jeremyallison
Copy link

Thank you very much @Mercy811 ! 🙏

@Mercy811 Mercy811 closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser SDK cannot handle url-encoded cookie values
4 participants