[spmurrayzzz]

Totally agree. I love the idea of SAST-in-CI, but I ran this on a handful of repos I manage (ranging from 40k-100k SLOC) and there were too many false positives for me want to add this as build-breaking criteria to our CI pipeline. Not unique at all to bearer in any way of course, as you point out, but still a real problem.

I suppose an alternative would be to not have this be a zero-sum part of CI, but maybe as a qualitative summary that gets autogenerated as part of the PR / code review process. The noise issue is still a real one as people will eventually ignore the noisy summaries or filter/whitelist them into relative oblivion.

I like the idea of "only applies if these other conditions are true". In all the false positives I encountered so far, if given the option I would be able to declaratively express when and when not to apply the rule. I'd even be ok with inline ignore comments to that end which, while not ideal, is something folks are already used to for other idioms like test coverage et al.

[gmontard]

We need to open for configuration the filtering and prioritization logic that essentially does that today, but so you can apply your own logic.

I advise to start today by looking first only to critical alerts, with our scoring based on sensitive data impact that should be a good first step in triaging.

[gmontard]

Btw if you have some exemple please share or even better write an issue, we’d be super happy to look at it and fine tune the rules.

It’s just a 1.0, we can do much better for sure :)

[spmurrayzzz]

I'll cherry pick an example: the default cookie config rule(https://github.com/bearer/bearer/blob/main//pkg/commands/pro...).

We have many places where `cookie: <EncryptedString>` is used in our code and it triggers that rule. There are a few issues with this:

- Most of the expressions where we use that pattern are used to send a full encrypted cookie string. The use of `cookie` is not the name of a key in the cookie string, its the whole cookie.

- All of the data in the cookie string itself is encrypted and also sent over https. Just matching on a regex expression won't tell you this information without an accompanying AST to verify.

Notably, we're using hapi and not express but my notes above would still apply to some use cases in express as well. Its possible I am missing the actual value of that rule, but just matching on the expression is going to generate a ton of false positives.

[_elsapet]

Thanks for the feedback here; it is much appreciated :) I do know your point around catching encryption is more general than this example, but I’ve made a small improvement to the default cookie config rule regex to address one of the false positive cases mentioned https://github.com/Bearer/bearer/pull/754

[spmurrayzzz]

This still generates the same false positive for me, in all of the previous repos I tested on.

[_elsapet]

Thanks for the report back; that's interesting. Perhaps I misunderstood your example. Feel free to write an issue if you like, and I can investigate further.

[cfabianski]

I’ve introduced the `absence` trigger that does that If express is present but helmet is missing then break.

Do you think that’d help achieve what you have in mind?

[spmurrayzzz]

I think the design flaw in most of the problematic rules was from too simple of regex matching. Looking for a string pattern should be a clue to do some deeper analysis (maybe verify via AST), not necessarily to flag the string alone as security failure.

[didroe]

The rules do work on the AST but the current cookie rule is not as advanced as it could/should be. For example, we really should treat encryption as sanitizing the value.

We'll take another look at the rules with this in mind. If you are able to share the (rough) approach you take to build the cookie string it would help us to ensure we're covering the specific case(s) you have.