Skip to content

feat: Provide normalizeDepth option and sensible default for scope methods #2404

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

Merged
merged 1 commit into from
Feb 3, 2020

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Jan 30, 2020

The initial issue wasn't with an incorrect circular algorithm implementation. It was with recursive references with a large payload, that were outgrowing the stack.

The most basic way to crash it:

class C {
  constructor(count) {
    if (count > 1000) return;
    this.tail = new C(count + 1);
  }
  data = Array.from({ length: 1000 }).fill('A'.repeat(1000));
}

Sentry.normalize(new C(0)));

The only notable change here is a conflict with ExtractErrorData that keeps error details one level deeper than regular "scope data", eg. contexts.errorName.data, not contexts.key so it's own depth option will be effectively ignored when normalizeDepth value is smaller.

Similar thing would happen for console breadcrumbs integration, as it keeps the data in breadcrumb.data.arguments, but because I only normalize breadcrumb.data, it doesn't affect it.

Fixes #2178
Fixes #2311
Fixes #2359
Fixes #2366
Fixes #2375

@getsentry-bot
Copy link
Contributor

getsentry-bot commented Jan 30, 2020

Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖 ✅ TSLint passed
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 16.6855 kB) (ES6: 15.6865 kB)

Generated by 🚫 dangerJS against f0a2d9a

@lobsterkatie
Copy link
Member

👍🏻

@kamilogorek kamilogorek force-pushed the normalize-configuration branch from cdda656 to e562bc5 Compare January 31, 2020 14:02
extra: {
arguments: normalize(handlerData.args, 3),
},
arguments: handlerData.args,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is "kinda" breaking if someone is relying on such an odd data point, but if we won't change it, it'll have same issue as the one described for ExtractErrorData

@@ -115,7 +115,7 @@ export class Scope implements ScopeInterface {
* @inheritDoc
*/
public setUser(user: User | null): this {
this._user = normalize(user);
this._user = user || {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation here was incorrect from the beginning. normalize returned any, so we did this._user = <any>, which shouldn't be allowed, as this._user is a non-optional User type. Also, clear sets it to {} not null.

Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

One nit + Changelog please.
Otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants