Remove multi-threading and error-handling from tags matched on by Security widgets.

[davewichers]

Remove multi-threading from tags to be included by these widgets because multi-threading is more of a quality/performance issue, than a security specific issue. Remove exception handling for the same reason. It's a general quality concern not a security specific issue. In my experience, including exception handling issues in these widgets tends to overwhelm the rest of the issues the widgets report because there usually are way more exception handling issues than true security issues.

If you don't want to remove one or both of these, then make the widget's configurable so a customer can removed them if they want to. But I think they should simply be removed, particularly multi-threading.

[InfoSec812]

Not sure that this would be desirable by all users of this plugin; but if you would like to submit a PR which makes this an optional configuration, I would be more than happy to review and probably merge.

[davewichers]

I'd like to understand your rationale for including them in the first
place. Can you provide a brief rationale?
On Jun 27, 2016 10:21 PM, "Deven Phillips" [email protected] wrote:

Not sure that this would be desirable by all users of this plugin; but if
you would like to submit a PR which makes this an optional configuration, I
would be more than happy to review and probably merge.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AExbBRyn0ZmR8EelFswvpoqnYYmxHhevks5qQIU5gaJpZM4I_pfG
.

[InfoSec812]

Seeing as how I did not write the original code, I cannot comment on the original intent. That said, concurrency issues can certainly have a bearing on security in mutiple ways. One simple example would be race conditions. As for error handling, again, a simple example would be leaking secure/private information via error handlers.

[InfoSec812]

To further comment, removing existing functionality which may be useful to some people would require a high burden of proof on your part. I am unlikely to merge this as is, but you are welcome to modify the PR to make it so that you can override the included classes of errors via the configuration settings.

[davewichers]

Deven,

The problem with this rationale for including these in the security issues
widget is that virtually ANY bug COULD introduce a security issue. So why
stop with these 2? Based on this logic, ALL findings should be included in
the security issues panel, which is clearly NOT the intent.

As such, I think the rationale should be if it clearly IS a security issue,
it should be reported by the wdiget. But if it MIGHT be a security issue,
it should not. (Ignoring the question about whether the reported issue
might be a false positive or not). So, direct security issues like XSS,
SQL Injection, etc. SHOULD be reported. But things like threading or error
handling should NOT (in my opinion).

That's why I think they should simply be removed.

-Dave

On Tue, Jun 28, 2016 at 12:30 AM, Deven Phillips [email protected]
wrote:

Seeing as how I did not write the original code, I cannot comment on the
original intent. That said, concurrency issues can certainly have a bearing
on security in mutiple ways. One simple example would be race conditions.
As for error handling, again, a simple example would be leaking
secure/private information via error handlers.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AExbBTjoBDpB1LNKrD-eX2GMcKTwCIHmks5qQKNggaJpZM4I_pfG
.

[InfoSec812]

Dave,

I appreciate your position, and I appreciate you submitting code which probably took as while to come up with. The main reason I would prefer not to merge this PR as is has to do with changing the way the widgets work for current users could be disruptive to other users of the plugin. That said, perhaps we can work together to accomplish this is a way that can satisfy everyone... I'll have a look and see what it would take to make this configurable and try to update you later today.

Cheers,

Deven

[davewichers]

Ok. Sounds good.

Thanks
On Jun 30, 2016 6:54 AM, "Deven Phillips" [email protected] wrote:

Dave,

I appreciate your position, and I appreciate you submitting code which probably took as while to come up with. The main reason I would prefer not to merge this PR as is has to do with changing the way the widgets work for current users could be disruptive to other users of the plugin. That said, perhaps we can work together to accomplish this is a way that can satisfy everyone... I'll have a look and see what it would take to make this configurable and try to update you later today.

Cheers,

Deven

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AExbBU9eGlNLHh39KuF1vCNlBxqNW9-Uks5qQ6BMgaJpZM4I_pfG
.

[InfoSec812]

Have a look at #14 and Issue #13

[InfoSec812]

If making this configurable is acceptable to you, I would like to propose closing this PR and just proceeding with merging #14 .

[davewichers]

That's totally fine with me. Feel free to close this PR, and merge the other one.

-Dave