Add a processor to remove all stacktraces from reported exceptions

[ste93cry]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes

This PR implements a processor that removes all stacktraces from reported exceptions. Its code is a direct porting of the processor added to the Ruby client with getsentry/sentry-ruby#233

[dcramer]

tbh im not sure why we allowed this in ruby -- this basically breaks any good behavior within Sentry, and probably was accepted because someone kept complaining that they "needed" it. I'd rather just say "dont use Sentry if you just want shitty logs", rather than pretending the app is at all usable without stacktraces. I do think we can support "dont store source code", but we sitll need filenames, function names, and line numbers.

/cc @nateberkopec

[ste93cry]

While I agree with you, as specified above this is just a porting of a feature that's available on the Ruby client. It seems that that SDK is the most maintained, so I think until this processor is there it should also be here. Also, I think a user should still be able to do whatever he wants, including not sending stacktraces to Sentry

[dcramer]

@ste93cry except then that user emails support and asks why nothing works. I dont disagree that users should have choice, but you can use Sentry, or you can use logs. To use Sentry you need stacks. I'm also going to talk internally about deprecating/removing this from the Ruby SDK since it will cause the same concerns. In general filenames/etc should not be sensitive, and I'd bet the original reason for it was to remove source code (and it was the easiest way).

[ste93cry]

except then that user emails support and asks why nothing works

Why should the user complain that your product doesn't work because it doesn't show stacktraces if the processor is not enabled by default and won't be anyway even in a 2.x version of this package... Also, please take a look at this article about why this processor could not be so useless in the end.

[nateberkopec]

It seems that that SDK is the most maintained

yyeeeeee boiiii thx :)

please take a look at this article about why this processor could not be so useless in the end.

"To the user" being the operative words in that question. Events are transmitted over SSL to Sentry, there is no information leak to persons outside the organization.

Also, I think a user should still be able to do whatever he wants, including not sending stacktraces to Sentry

Then they can write their own processor. It's super easy, at least in the Ruby SDK.

I do think we can support "dont store source code", but we sitll need filenames, function names, and line numbers.

I can modify the Ruby processor to do this.

Here's a recap of the arguments in favor of this feature: getsentry/sentry-ruby#130

"You're okay with sharing your source code with 3rd party services?" we said, on GitHub.

IMO we might be better off just documenting how to write your own processors (it's trivial) and letting users take care of themselves.

[ste93cry]

+1 on making this a processor included in the repo, but not on by default.

This is what you voted on getsentry/sentry-ruby#130. I agree with that phrase, there is no disadvantage in having this processor available to users of your SDK, mostly because it's anyway turned off by default

[nateberkopec]

It's what I said 3 years ago to try and close an issue that was dragging on too long, raised by people who are probably not even Sentry users.

As I also said at the end of that issue, it's unclear to me exactly what data people think is a security concern. The current processor is like a flamethrower - it indiscriminately wipes out all data relating to the source. @dcramer has already proposed a processor which only wipes the actual source code but keeps line numbers/everything else intact.

I'll re-paste the data we're talking about here for clarity:

{"abs_path"=>"/Users/nateberkopec/Documents/code/raven-ruby/spec/raven/processors/removestacktrace_spec.rb",
        "function"=>"block (2 levels) in <top (required)>",
        "pre_context"=>["  end\n", "\n", "  it 'should remove stacktraces' do\n"],
        "post_context"=>
         ["    binding.pry\n", "    expect(data['exception']['stacktrace']).to_not eq(nil)\n", "    result = @processor.process(data)\n"],
        "context_line"=>"    data = Raven::Event.capture_exception(build_exception).to_hash\n",
        "lineno"=>12,
        "in_app"=>false,
        "filename"=>"raven/processors/removestacktrace_spec.rb"}

So, instead of removing all of this, we could remove pre_, post_, and context_line.

[ste93cry]

As I also said at the end of that issue, it's unclear to me exactly what data people think is a security concern.

According to the article and according to the people that were complaining about security on that issue I think their concern is that showing anything that include source code to third-parties is an information leak. While I agree with this statement, I also agree on that a product like Sentry is totally useless without that informations.

So, instead of removing all of this, we could remove pre_, post_, and context_line

If everyone agree on this, I can make the changes to do it and rename this processor to something like SanitizeStacktraceProcessor. To make everyone happy, we could even make it configurable, so users can decide whether they want to remove everything or only those parts of the stacktrace.

[dcramer]

I'd agree that both here (and eventually as a standard), we should provide a way to strip the source code context, but not the entire stacktrace (for the reasons stated).

[ste93cry]

Should I make it configurable, so that users can choose whether to strip just pre_context, post_context or context_line or should the processor just remove all of them when enabled? Honestly, I think there should be no need to just remove only one of them, but asking is always better 😄

[dcramer]

I'd say remove all three with no configuration and we describe it as "removing source code from stack traces". It will still weaken our grouping algorithm but I'm fine with the compromise.  On Fri, Mar 24, 2017 at 5:02 AM -0700, "Stefano Arlandini" <[email protected]> wrote: Should I make it configurable, so that users can choose whether to strip just pre_context, post_context or context_line or should the processor just remove all of them when enabled? Honestly, I think there should be no need to just remove only one of them, but asking is always better 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ste93cry]

Then PR is ready and tests are green

[dcramer]

Thanks!