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

Add support to set withCredentials on ahoy config #19

Closed
wants to merge 3 commits into from

Conversation

gualopezb
Copy link

This option is required on ajax requests to send credentials such as cookies, authorization headers or TLS client certificates from a different domain

This option is required on ajax requests to send credentials such as cookies, authorization headers or TLS client certificates from a different domain
@ankane
Copy link
Owner

ankane commented Feb 23, 2018

Hey @gualopezb, thanks for the PR 👍 and sorry for the delay. I think it'd be good to document this feature in the readme, including the security implications.

@gualopezb
Copy link
Author

@ankane I just documented this feature in the readme: edf4acb

@ankane
Copy link
Owner

ankane commented Feb 24, 2018

Thanks @gualopezb 👍 I'll test this out once things are in a good place for the 1.0 release. Will see if I can include it there (otherwise, a release shortly after).

@ankane
Copy link
Owner

ankane commented Feb 28, 2018

Hey @gualopezb, can you explain more about your setup for cross-domain tracking? (where cookies are set, etc?)

Also, looks like the option was only added to jQuery. Can you add it to non-jQuery AJAX requests? From what I can tell, navigator.sendBeacon (the new default) doesn't support this, so we probably need to switch to AJAX if the option is set.

@gualopezb
Copy link
Author

gualopezb commented Mar 7, 2018

@ankane sure, I can add more details about the setup for cross-domain tracking. Do you mean improve the documentation in the README, right?

Regarding the non-jQuery AJAX requests, I think we can use the fetch API with keepalive flag set to true instead of navigator.sendBeacon, so we'd have the ability to customize the request and therefore send cookies. We have to check the current support of this flag from the browsers.

@gualopezb
Copy link
Author

@ankane according to the fetch spec, a request has an associated keepalive flag:

This can be used to allow the request to outlive the environment settings object, e.g., navigator.sendBeacon and the HTML img element set this flag. Requests with this flag set are subject to additional processing requirements.

I was trying to make a request using this option set, and including the credentials option set to "include" as well. However I got the following error:

Preflight request for request with keepalive specified is currently not supported

So we definitely would need to make AJAX requests when the option crossDomain is set to true in the ahoy config.

@gualopezb
Copy link
Author

@ankane I renamed the crossDomain option to withCredentials taking as inspiration the axios documentation. I also made a change on the canTrackNow function to not track now events by using navigator.sendBeacon when the option withCredentials is set, and finally I updated the readme. I tested it out and it worked very well for me. I guess this PR is ready to be merged.

@kieranklaassen
Copy link

👍 great addition

@ankane ankane closed this in ecc63b7 Feb 18, 2019
@ankane
Copy link
Owner

ankane commented Feb 18, 2019

Merged, thanks @gualopezb 👍

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.

3 participants