alert: Add CWE Alert Tag when building and CWE ID has been set

[kingthorin]

I also looked at doing this for WASC IDs but they don't have a handy URL, we'd have to add a pre-populated map which isn't impossible, but did seem like too much work for this morning 😀

[ricekot]

The function Alert#setCweId should be updated too?

zaproxy/zap/src/main/java/org/parosproxy/paros/core/scanner/Alert.java

Line 931 in 24a4eb5

public void setCweId(int cweId) {

[kingthorin]

I dunno we didn't update the old alert functionality when we implemented addTag/removeTag.

I'm happy to do either, but was trying to stay consistent.

[thc202]

Whatever we do we should document these side effects to avoid surprises (e.g. I can see this breaking our unit tests as there will be more tags now).

[kingthorin]

Okay how about this?

1. I'll deprecate the existing setCwe(int) and add setCwe(int, boolean). This can easily be handled with a regex search and replace (with grouping/interpolation) when we update to new core. Where boolean true is to add the tag.

2. In Builder.build() use boolean true then it's added (this behavior can be documented, and a build(boolean) added?)

3. Add functionality/tests to not set multiple tags.

4. In the older non-Builder Alert functionality I'll add an addCweTag() method? Or also do 1-3 there?

[kingthorin]

Or, we could go a totally different way.

Implement convenience methods in Common Lib for it and then make sure that it gets used in all scan rules. We could even add generic tests for it 🤷‍♂️

In guess that'd be more maintenance friendly as far as the URL goes.

[kingthorin]

Crew I need some feedback on this so that it can be moved ahead.

[kingthorin]

Anyone out there?

[ricekot]

I like the original approach of adding the tags in the builder (i.e. in Builder#build), if a valid CWE ID is set.

[thc202]

I'm also happy with the builder way, my only concern is the URL (though that one should stay stable…).

[kingthorin]

Thank you both! I'll get this reworked.

[kingthorin]

Tweaked.

[kingthorin]

Tweaked

[kingthorin]

Okay think I got this into a more expected state.

[kingthorin]

Tweaked again.

[kingthorin]

Okay hopefully this is finally good to go.

[kingthorin]

I had to re-work it a bit more because I was getting NPE and unmodifiable map exceptions while working on something else this evening. Hopefully it's in a good state now.

[thc202]

Thank you!

[kingthorin]

For the record this will mean that we need to update "expected mappings" type unit tests when zap-extensions migrates to 2.15.

[github-actions]

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.