Improve SourceCodeSecurityCheckStep

[martinmladenov]

At the moment we use the SourceCodeSecurityCheckStep for two things - to prevent students from using certain classes, and to inform them that they should not instantiate their own Selenium driver. We do this using simple string comparisons.

We should consider the following:

• Renaming the step to something more suitable (it's not just security)
• Ignoring comments
• Highlighting the correct line (at the moment it highlights line 1 even if the error is further down)
• Redesigning the detection method entirely, so that we do proper static analysis instead of string comparisons (hard)

[mauricioaniche]

Let's keep it with string manipulation for now, static analysis will be tough, and I'm not sure if it will bring waaay more soundness in the analysis!

[Arraying]

@mauricioaniche why do you think static analysis will be tough? I thought JavaParser has a really elegant and easy to use API. It also features a way to completely ignore all comments while parsing.

[mauricioaniche]

Thanks for the question. There are a few points here: - Andy’s static analysis engine (which uses JDT and not JavaParser) is only triggered later in the process. We’d need to refactor that. - If we do, most of what we will do is to visit nodes and check whether they match with a hard coded list of classes and methods. Not so different from what happens now. For the hard cases where someone avoids typing the full name of the class or method name, say by concatenating the name of the method through a list of integers, static analysis won’t help us anyways. Sure, it can avoid some bad matching, but we haven’t had a single case so far where this happened. - There’s very little incentive to do it. Andy runs in a container with very little permissions and no internet access. Due to a recent PR, in exam mode, the entire rubric file is deleted, so no way to get the rubrics. - Running full blown static analysis, which has a computational cost higher than string matching, just to identify security cases (literally 0 cases so far) isn’t really worth it. Cheers! -- -- *Maurício Aniche* Author of Effective Software Testing: A Developer's Guide <https://www.effective-software-testing.com> https://www.mauricioaniche.com

[alexcojocaru2002]

@martinmladenov What is the state of this issue ? Could I maybe work on it ?

[martinmladenov]

Sure, go ahead!