Sonar scan fixes sniff test

[wbittles]

Description

This PR fixes the
2 high level "Security Hotspot" alerts
4 low level security hotspots
30+ of "incidental alerts", these may get lost in the 20K "code smells"

Related Issue(s)

Testing

automated tests only

Release Notes & Documentation

N/A

Additional notes

this PR is just a sniff to test to try and assess
The scale of the CodeQL issues
Check the reliability of the cloud scan as a metric on progress made

@mandy-chessell I know the indentation is wrong on your files, but I can only see the difference in white space when the code is pushed remotely.
If I can't figure out how to fix it I'll pull those changes

[planetf1]

Thanks for the proposals

• Agree with many of the changes which clean up the code
• Disagree with the change to stdout (but understand the reasons for the proposal) - what do you think?
• Suggest leaving the first unknown variables in with referenced guids

Interested in your comments also @davidradl

[planetf1]

type on constants (very minor - appreciate the comments)

I cannot see these constants referenced in the code - so they are unusued.
However I do see that they guid is referenced in the code, and present in the types archive.

One use of the guid is in an FVT making use of notelogs - though unrelated to the generic handlers

Given this, and whilst agreeing with the proposal from a pure java perspective, I'd be inclined to keep this definition and instead make the omission explicit with @SuppressWarnings("unused")

[wbittles]

If the code was actually being used then I agree we should just turn that rule off in this case.
However the fact that code wasn't being used meant that I would like the option to track the "unused", rather than the original security alert , but I also wanted rid of the High sev alert.

[mandy-chessell]

The security alert is clearly invalid in this case and needs to be flagged in the tool rather than making changes to the code. This way the tool vendors improve their rules.

As new types are defined contributors are supposed to add the name and GUID into this mapping file so that they are ready for use in the generic handlers. This is to increase the chances that the GUID values are right and discourage each OMAS from creating their own definitions.

I am not sure it is helpful to flag the values that are not used in this list - it just creates more work when they are used (in fact they are likely to stay flagged as unused as they are used which is more misleading). If it is a real problem then I woudl suggest putting one suppressWarnings at the top of the file.

[mandy-chessell]

After thinking about this overnight, I realised my comment on adding the SuppressWarnings to the top of this file is wrong. We should not be suppressing valid warnings. In this case, the variable is not used because the handler that should use it has some empty methods. The warning is correct and the "fix" for this warning is to implement the appropriate methods in the NoteLogHandler and NoteHandler.

Having the unused types and properties in the mapper flagged by the scanner is useful to show what has been implemented and what still needs to be done. As the build out of the OMASs continues, these handlers will get completed and the variables in the mapper will be used. We can see our progress to completion as the warnings in the mapper diminish :).

I agree that is it useful to suppress warnings for code where the unused field is deliberate. This shows that there is no more work to do. I also agree that obsolete code should be removed. We get this, for example, when the callers of a deprecated method are recoded to use the new version of the method. Normally the last user removes the deprecated method, but it is easy to miss it.

The tricky question is how to distinguish between code that is obsolete and code that is work in progress. This is probably only something the people working on the code know. The component owner should know at least.

[wbittles]

Thanks @mandy-chessell for responding ,
another issue with this code is that it's also wrongly being confused for a High Sev Security Hotspot.

A constant containing AUTH being assigned a Hex string , looks exactly like a developer has a hard code auth token in the source code.

public static final String NOTE_LOG_AUTHOR_RELATIONSHIP_TYPE_GUID = "8f798c0c-6769-4a2d-b489-12714d89e0a4";
in this case that is wrong and could be turned off either locally or centrally and as I don't know how to change the the rules centrally , I've pushed a suggest fix that just turns off that specific alert. If you and Nigel find it acceptable I will open a new PR which turns off warnings for only the 2 lines that are causing this alert to be raised, which also happen to be our Highest Sev Security alerts in this scan.

[mandy-chessell]

The tool is creating a false positive since clearly this is setting a variable to a string that is in no way related to security.

Ideally we need to get the scanning tool fixed. If we can configure the tool so that the false posititive is recorded and this is passed back to the developers of the tool then there is a chance they will fix it. If we just turn off the scanning then it will come back when an external party runs their own scan.

If there is no way to configure the tool then disabiling the rule as you suggest is the best we can do.

[planetf1]

I see this security hotspot skip is now in #6633 - so this pr probably needs rebasing/updating for any delta once that has been merged.

I think we need to do what we sensibly can to reduce the counts on our scan tools. This helps external reviewers/observers of our metrics, which will help us with adoption (or at least remove adopters). it also helps us see better what remains.

Previously tools - onsite - tended to allow for a lot of rule flexibility. The trend with cloud services seems to be more to standardized rule configurations which can be somewhat opinionated

It does cause some clutter, and it's somewhat annoying for sure - but I think even for code in progress we should have these checks - unless we can very clearly isolate (ie diff github repository etc, possibly subdirectory).

[planetf1]

should that last word be LISTENER ?

[wbittles]

fixed thank you

[mandy-chessell]

This module is in development - ie still being written - and so fixing so called security errors in it is likely to interfere with the developer doing the work. I thin you should check with the component owner to be sure they are happy with these changes being made.

[mandy-chessell]

The message does not appear to have any inserts - need at least the connector name that failed. Also, I think it is the connector implementation that is registering the listener not Egeria. If it is in the Egeria code then need to state which component

[planetf1]

I think it's a good idea to check if there's recent activity in the module, and if so consider if it might have an impact . Less so in my view for small, isolated change. New events etc - more so as that affects behaviour. I think this helps build community and engagement.

regardless, Github should automatically assign the code owner a review. That should be the formal way of ensuring they are notified and have the opportunity to review. Since I don't see any reviews assigned I suspect the CODEOWNER is not set (cc: @bogdan-sava ) so I would consider updating /CODEOWNERS accordingly

[planetf1]

In general we are using System.out.println across our 'interactive' applications and FVTs with the expectation of the I/O always going to console.

However we are seeing too much clutter in our build logs from FVTs, and ensuring any output generally uses logging (or audit) is the right approach. And our FVTs should really be run purely by the test runners like surefire, failsafe (for fvt)

But a main() that implements interactive tty (which is the intent here), and in a test case (vs library/production code) should probably be an exception - but only in the 'test driver' aspect ie main() or tightly related to that interaction. Indeed this part of the FVT is using a buffered input stream to query the URL if needed. Given that I think the change to log causes more obfuscation

[planetf1]

Not sure how we can mark this as good in the tool as detected, and whether that will then hide for good. Or whether an annotation is needed (I can't see how)

[wbittles]

I hoped that the output provided by the build tool (maven/gradle) would be good enough and we could simply redirect them to log. The only thing that actually needs to change is the call to printStackTrace everything else is an incidental change so isn't important enough to change just yet.

[planetf1]

There is some history in the way these FVTs came about. they started as interactive applications - and so the main() allowed for the code to be run directly at the cli, or in IntelliJ. If appropriate parms were not passed, or perhaps always, the code prompted for input using an input stream. unless redirected this uses stdin, so in that context of interactive tty, using stdout seems entirely reasonable, and using log less so.

That changes when we get to the actual test case implementations as these will either be called from this code (which could control output) or most likely from a test runner - via maven/gradle. Once there I strongly agree, we should use logging, and this has big benefits to improving the quality of output from the build by hiding noise, yet making it easy to capture.

[planetf1]

I agree with the removal of the stack trace.

If we wanted to cleanup the FVT behaviour more, I would remove this interactive prompting for input parms & the use of input streams/stdout, probably even remove the main, and instead expect the code to always be launched from a test runner or IDE. But that would be a broader community question

[wbittles]

While recognising the history of this FVT I think we should look to move it to one of the standard frameworks, but I think this will happen as part of improving the Egeria test infrastructure as a whole. Something that will hopefully make it on to our backlog soon.

As for Sonar scans
There are 31 calls to printStackTrace in the code base.
28 are calls made from the FVT
24 are causing Low Sev Security Alerts in Sonar

printing stacktraces to the console are pretty useless , unless someone is observing the stream at the time.
@davidradl @planetf1 I'm happy to move all the calls to printStackTrace that are raising Sonar Alerts if we feel having zero Security Hotspots is worth the effort ?

[planetf1]

I think in general removing calls to printStackTrace is appropriate - in our runtime libraries they should be managed through our auditog framework if needed. Inadvertent disclosure is a potential security issue.

I'm less worried about FVTs in not being operational code, but it is best practice, so I would also agree with those changes. The one caveat is to be sure that if someone runs the FVTs as a java application via main() that we don't lose the exceptions completely - ie they should still be logged, or we at least report they are in the tool.. or otherwise update docs to say how they work. can comment more when the pr is ready for review

[davidradl]

I agree with both your points. The main thing is that we can debug an error s in the FVT when they occur in the build. We have moved to using audit logging - that writes to the console for this. I agreed with Nigel - that as a developer if I run it locally as an app to debug it outside of the build - it would be necessary to see all Exceptions.

[planetf1]

Gradle PR build failed with FAILURE:

Gradle build daemon disappeared unexpectedly (it may have been killed or may have crashed)

This is likely resource exhaustion - not an issue with the PR, and an ongoing issue for which I've just submitted another attempt for a fix. See #6130.

I've also restarted the gradle build for this PR

[davidradl]

@wbittles The changes look good to me, once the blocking conversations are resolved.

[wbittles]

Thanks for all the input , the Sonar Security Hotspot alerts have now been resolved via another PR.