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

[DerGernTod]

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]

Assigning to @getsentry/support for routing ⏲️

[DerGernTod]

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]

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]

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]

@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]

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]

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]

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]

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]

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]

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

[getsantry]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[DerGernTod]

does the "Waiting for: Community" label mean you need someone to implement it, or you need the community to vote for it?

[lforst]

Sorry, this fell under the table. It seems off to me that you're the only ones running into this issue but I opened a pr some time ago to fix the concrete symptom you're describing. #12212 It's hard to tell without knowing what you are doing exactly to check for the native implementation.

Maybe you can also check it out before I merge?