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

Wrapping of Function.prototype.toString causes inconsistent behavior of wrapped functions #12208

Open
DerGernTod opened this issue May 21, 2024 · 11 comments · May be fixed by #12212
Open

Wrapping of Function.prototype.toString causes inconsistent behavior of wrapped functions #12208

DerGernTod opened this issue May 21, 2024 · 11 comments · May be fixed by #12212
Assignees

Comments

@DerGernTod
Copy link

DerGernTod commented May 21, 2024

Environment

SaaS (https://sentry.io/)

Steps to Reproduce

  1. go to a sentry-injected page
  2. run XMLHttpRequest.prototype.open.toString() - it returns function () { [native code] } even though it's wrapped by sentry
  3. run XMLHttpRequest.prototype.open.name - it returns "", because it's wrapped by sentry but the wrapper doesn't forward the property access like it forwards the toString call

Expected Result

access to XMLHttpRequest.prototype.open.name returns "open"

Actual Result

access to XMLHttpRequest.prototype.open.name returns ""

This can cause interoperability issues with other tracking services.

Product Area

Issues - Suggested Fix

Link

No response

DSN

No response

Version

No response

@getsantry
Copy link

getsantry bot commented May 21, 2024

Assigning to @getsentry/support for routing ⏲️

@DerGernTod
Copy link
Author

my suggestion for a fix would be to implement the access to other Function.prototype properties similar to the toString function - forward the call to the native function to behave consistent

@dalnoki
Copy link

dalnoki commented May 22, 2024

hi @DerGernTod thanks for reaching out, are you using self-hosted Sentry? If not, would you mind checking the link you provided? It currently points to sentry.sentry.io. Thanks in advance!

@DerGernTod
Copy link
Author

i'm not using sentry on my own, i just had to deal with this issue as a third party vendor.

you can reproduce the issue on https://sentry.io/welcome/ for example. simply open the console and type XMLHttpRequest.prototype.open.name - it shouldn't return an empty string

@lforst
Copy link
Member

lforst commented May 24, 2024

@DerGernTod we can change this. Can I ask, because this has been a non-issue in many many years, how exactly is this causing interop issues?

@DerGernTod
Copy link
Author

sentry wraps native apis like XMLHttpRequest. if you build a wrapper for this api on your own and want to ensure no interop issues, you have to build it so that it behaves exactly like the native one. we're building a wrapper for XMLHttpRequest using the Proxy API, which gives us the possibility to forward calls to props or functions we're not interested in to the native api 1:1, while still grabbing information from calls we need. however, to guarantee interop with other wrappers and not cause illegal invocations, we need to differentiate which context we pass to callers (either their own wrapper object, or the native instance). for this to work, we need to know if a function is native or not - if not, we pass the wrapped instance.

i guess you already see the issue: syntry overriding Function.prototype.toString breaks the only possibility (i know) to check for native functions - the function() { [native code] } output - even though this is not the actual content of the function. so now we think we're working with a native function and simply switch on the function name property, which is an empty string because of sentry, to know which arguments we need to grab. the fallback applies for the switch statement since empty string doesn't match any case, and this breaks the interop - no arguments are grabbed and data is missing.

if i inject sentry after my wrapper, so that my wrapper can act and wrap first, everything works fine. but unfortunately, this is out of my hands since i'm not the owner of the website

@lforst
Copy link
Member

lforst commented May 24, 2024

We should probably also start using Proxies. We didn't so far for IE11 compat reasons but we can now. One question though, by "differentiate context", I assume you mean this. Can't you just always forward the thisArg from Proxy.apply?

@DerGernTod
Copy link
Author

DerGernTod commented May 24, 2024

well, it's... complicated. if you have an instance of the native object and you pass this to the native function, everything is fine. if you have a native object and you pass this to a wrapped function, the wrapper is going to be confused as to why it's getting the native object and not the wrapped object (potentially missing custom properties). if you have a wrapped object and you pass this to a native function, you end up with an illegal invocation.

when we started with proxies we thought it's the holy grail for wrappers. unfortunately there's prototype and other wrappers that makes the whole thing a bit more complex than expected 😅

unfortunately the fetch api is missing a few features so XMLHttpRequest will still stick around for a while... fetch is a LOT easier to wrap 😅

@lforst
Copy link
Member

lforst commented May 28, 2024

I am curious as to how you guys are wrapping things. It's odd to me that we went this long without any hiccups so I'd assume you have slightly different requirements that now clash with our implementation.

@DerGernTod
Copy link
Author

DerGernTod commented May 28, 2024

it's always related to the applications we're working with. our wrapper grew a lot over the years since all the applications we're injected in have very different implementations and third party tools, some of them also wrapping. mostly wrappers are not as sophisticated as ours is (talking, not behaving 1:1 like the native object), and of course ours has to compensate for that - just like with sentry (i hope this doesn't come across as bragging, sorry if it does).

the wrapper is so similar to the native object that we usually don't have problems with other wrappers, but only if they're injected after us. if we're late, we need to rely on the behavior consistency of the wrappers that are already active on that page, and we obviously can't compensate for the behavior quirks of every wrapper that exists (we did for some, but of course to an extent where it makes sense on a generic level, rather than building tailored code for specific wrapper support, since that would be unnecessary code for most of the applications we're injected in).

hope that makes sense.
there's always no problem, until there is 😅

@lforst
Copy link
Member

lforst commented May 31, 2024

I think we'll switch to a naive proxy approach but that's gonna be it for now. I hope that works out.

@lforst lforst self-assigned this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

4 participants