Fix security issues and warnings (and improve code styling)

[sebastian-meyer]

Description

We are using some industry-standard static code analyzers for code and security scanning like Dependabot, CodeQL and Codacy. Those tools produced a lot of warnings of different severities from "Note" to "High".

This proposal's goal is to fix at least all security related issues. Those are currently:

• 0 critical security issues
• 1 high security issue
• 2 medium security issues
• 0 low security issues

Furthermore there are a lot of code-style related issues. The issues with severity "Warning" or higher should at least be assessed and fixed if this can be done with reasonable effort (most of them are duplicates, i. e. multiple occurences of the same issue). Those are currently:

• 0 errors
• 155 warnings
• 222 notes

Related Issues

The security alerts can be found on the security tab.

Expected Benefits of this Development

This proposal would make Kitodo.Presentation more stable and secure. It would also eliminate the abundance of security messages, making it easier to identify new problems as they arise.

Estimated Costs and Complexity

The complexity is medium and the cost are low (2-3 days).

[stweil]

Only "members" of the Kitodo organization or the repository can see the security related pages which are mentioned above. I was member in 2017, but obviously removed later.

[sebastian-meyer]

Access can only be granted to admins, members with push rights and specific user groups. I'd have to create a user group and add all current outside collaborators to that group... :o(

[stweil]

It's not a real problem for me personally, because my fork runs the same security checks and reports them (so the whole GitHub concept of hiding the security reports is strange and wasting energy for unnecessary computations).

[sebastian-meyer]

Votes: 11

[frank-ulrich-weber]

Can you please give me access to the security tab to fix the issues and warnings? Thanks!

[stweil]

It's usually sufficient to fork the repository and use the security tab in your own fork (see my comment above). Then you can either use the fixes which are generated by CodeQL or manual fixes to create a pull request.

[sebastian-meyer]

Can you please give me access to the security tab to fix the issues and warnings? Thanks!

I've granted you the security manager role. You should be able to access the security tab now.

[frank-ulrich-weber]

Thanks Stefan and Sebastian, that helps a lot!

[frank-ulrich-weber]

I think I'm blind, but now I see 40 security issues on my current fork of the kitodo-presentation master: What do I have to do/configure to see the same amount of issues? How can I apply the same checks on a branch? I think I yust need a hint... Thanks!

[sebastian-meyer]

• [MAINTENANCE] Adds curly braces around blocks in loops and conditionals #1032
• [BUGFIX] Adds missing semicolons #1037
• [BUGFIX] Removes unnecessary semicolons #1038
• [SECURITY] Fixes security issues #4526, #4523 und #4522 reported by CodeQL #1041
• [MAINTENANCE] Check object properties before using them #1043
• [MAINTENANCE] Fixes ESLint_camelcase warnings #1044
• [MAINTENANCE] Fixes Phpmd CamelCaseParameterName Warnings #1048
• [MAINTENANCE] Fixes Phpmd CamelCaseMethodName warnings #1049
• [BUGFIX] Fixes incomplete renaming $index_name to $indexName of #1048 #1058

[stweil]

GitHub code scanning currently reports 7 warnings and more than 6000 notes for the master branch.

[sebastian-meyer]

I don't see any notes and of the warnings we decided to ignore all that are reported for the 3D viewer javascripts (because those are still in a prototypical state and under heavy development). I've forwarded them to the developers and they will take care of those.
So, for me this looks fine, now.

[stweil]

I don't see any notes

Then go to settings/security_analysis and look for "Code scanning" and "Protection rules". There you can set the alert severity levels. Set both levels to "any" to get all the notes, too. Not all kinds of notes are useful, but some of them are.