Ai Code Review Reviewed
So yesterday, I figured I would finally try and see how AI Code Review does and tested Copilot by asking it to do a code review of one of my more stable repos; Here is that outcome…
Critical Issues(???)
These are the issues that the AI tool rated as ‘CRITICAL’ (including the fact that printlns were used in places that needed logging)…
SQL Injection / Parameter Validation Risk
- AI Assumption: The
createApiDescriptor()method processes user-supplied values from JSON configuration files without sufficient validation. While mockData is validated, foreign key references and parameter types are not escaped before being used in cache operations. - Actual Risk: LOW. The application uses external properties files for loading configuration settings. They are stored in the running users home directory. This means that in order to craft a malicious file, the user would nee WRITE privileges locally on that machine for that user. In other words, you are already FUCKED if this happens.
Hardcoded Credentials & String Interpolation
- AI Assumption: Mail sender credentials use string interpolation (
"${apiProperties.getMail().getUsername()}"), which means they're rendered as strings, not actual values. This suggests incomplete implementation. - Actual Risk: None. This is merely bad formatting
Unhandled Exceptions with System.exit(0)
- AI Assumption: The framework calls
System.exit(0)on initialization failures, which is too aggressive for a library (not an application). This crashes the entire JVM without allowing graceful degradation. - Actual Risk: None. We want the server to DIE if key configuration fails; it should not continue processing.
Insecure Error Messages with println() in Production Code
- AI Assumption: Direct use of
println()for logging errors to stdout. No structured logging for production environments; errors may be lost. - Actual Risk: None. Merely bad logging
Deprecated AGPL License Conflict in Documentation
- AI Assumption:
BeapiWebAutoConfiguration.groovyheader mentions "AGPL v2 License," butREADME.mdstates "Reciprocal Public License (RPL-1.5)". Inconsistent licensing. - Actual Risk: This is laughable that this registered as CRITICAL. Merely did not update license in classes
High Priority Issues
I don’t know how AI thinks a ‘println’ is more critical than a NullPointerException but this is the ranking I got…
Get API Expert’s stories in your inbox
Join Medium for free to get updates from this writer.
NullPointerException Risk in ApiCacheService.getApiCache()
- AI Assumption:
cacheManager?.getCache('ApiCache')?.get(controllername)?.get()chains optional operations but throws generic exceptions on failure. - Actual Risk: Very Low (on Blocking/Synchronous apis). BUT… it is a risk that SHOULD be mitigated. So score one for AI.
Security Filter Chain Ordering Issues
- AI Assumption:: Filters are added without verifying their order relative to other Spring Security filters.
JwtRequestFilterandRequestInitializationFiltermay not execute in intended order. - Actual Risk: None. You can declare the order in the FilterRegistrationBean otherwise the order defaults.
etc etc etc
Review?
The AI code review greatly exagerated typos and issues and thought there was greater threat than there was in almost everything.
It was good at finding typos and minor issues but does not understand ‘architecture’
Now I would not say it is not without value but the value that it adds in greatly exaggerated. I was pleased to see some things that could be fixed and issues that had gotten past me.
Code reviews still require eyeballs on the code but this is a nice additive feature for assists.
