feat: add support for Spring AOP/Interceptor #100
Conversation
CyanM0un
requested review from
AntJiuFo,
Arielwyy and
alipaydeshui
as code owners
Summary of ChangesHello @CyanM0un, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Java taint analysis capabilities for Spring applications by introducing a dedicated Spring-specific taint checker. The core changes enable the analyzer to understand and process complex execution flows involving Spring AOP aspects and web interceptors. This leads to more accurate and thorough security analysis by correctly identifying and tracing data through these common Spring features, including the dynamic parsing of AOP annotations and the handling of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for Spring AOP and Interceptors. The changes include a new checker for Spring, modifications to the Spring analyzer to handle AOP pointcuts and advice, and updates to the entry point collector to include interceptors. My review has identified a critical typo and a high-severity bug in the AOP advice handling logic. I've also included several medium-severity suggestions to improve code quality and maintainability, such as addressing code duplication, improving type safety, and refactoring for clarity.
| processAop(scope: any, node: any, state: any, aopLogic: any, current_fclos: any) { | ||
| let argvalues: any[] = [] | ||
| let fclos | ||
| for (const logicM of aopLogic.logicMethod) { | ||
| fclos = logicM.fclos | ||
| argvalues = this.prepareArgValues(scope, node, state, fclos) | ||
| switch (logicM.logic) { | ||
| case '@After': | ||
| super.processCallExpression(scope, node, state) | ||
| break | ||
| case '@Before': | ||
| case '@Around': | ||
| let isNeedProcessARg = this.needProceedArg(fclos.fdef?.parameters) | ||
| if (isNeedProcessARg) { | ||
| argvalues.unshift(current_fclos) | ||
| this.topScope.proceedMap.set(current_fclos._qid, current_fclos) | ||
| } | ||
| break | ||
| default: | ||
| logger.warn(`AOP logic type: ${logicM.logic} need to be supported`) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return this.processCall(node, fclos, argvalues, state, scope) | ||
| } |
There was a problem hiding this comment.
The current implementation of processAop incorrectly handles multiple AOP advices for a single pointcut. The for loop over aopLogic.logicMethod overwrites the fclos and argvalues variables in each iteration. As a result, only the last advice in the list is executed by the processCall at the end of the function. To correctly support multiple advices, they need to be chained. For example, with @Around advices, they should wrap each other, with the innermost one wrapping the original method call.
| * constructor | ||
| * @param resultManager | ||
| */ | ||
| constructor(resultManager: any) { |
There was a problem hiding this comment.
The codebase uses the any type extensively, for example in this constructor's resultManager parameter. While this might be a broader issue, gradually introducing more specific types would improve type safety and code clarity. For instance, if resultManager has a known interface, it should be used here.
| const defaultSpringAnnotations = [ // copy from spring-default-entrypoint.ts | ||
| 'RequestMapping', | ||
| 'GetMapping', | ||
| 'PostMapping', | ||
| 'PutMapping', | ||
| 'DeleteMapping', | ||
| 'PatchMapping', | ||
| 'Path', | ||
| ] |
There was a problem hiding this comment.
The defaultSpringAnnotations array is duplicated from src/engine/analyzer/java/spring/entrypoint-collector/spring-default-entrypoint.ts. To improve maintainability and avoid inconsistencies, this array should be defined in a shared constants file and imported in both places.
| const hasAopLogic = this.AOPLogicAnnotationOnFunction.some((anno: string) => { | ||
| if (modifier.includes(anno)) { | ||
| aopLogicTag = anno; | ||
| return true; | ||
| } | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
Using .some() with a side effect (assigning to aopLogicTag) can be confusing. A more explicit loop like for...of or using .find() would make the intent clearer. This would separate the act of finding an annotation from capturing its value. For example:
let hasAopLogic = false;
for (const anno of this.AOPLogicAnnotationOnFunction) {
if (modifier.includes(anno)) {
aopLogicTag = anno;
hasAopLogic = true;
break;
}
}
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing return statement causes implicit undefined
Medium Severity
The extractUrlPath method returns '' for certain early-exit conditions but lacks a return statement at the end of the function. When the loop iterates through all modifiers and annotations without finding a match (i.e., the annotation is present but the path format doesn't match either explicitMatch or implicitMatch patterns), the function implicitly returns undefined instead of an empty string. This inconsistency could cause issues when entryPoint.urlPath is assigned and used downstream.
| processAop(scope: any, node: any, state: any, aopLogic: any, current_fclos: any) { | ||
| let argvalues: any[] = [] | ||
| let fclos | ||
| for (const logicM of aopLogic.logicMethod) { |
There was a problem hiding this comment.
Crashes when accessing undefined logicMethod property
High Severity
The processAop method iterates over aopLogic.logicMethod without checking if it exists. At lines 344-357, an aopMap entry can be created with only targetClass, targetMethod, and cutType properties (when processing a @Pointcut annotation). If this entry is matched later in processCallExpression but no corresponding @Around/@Before/@After was defined to add logicMethod, the for-of loop will throw a runtime error when iterating over undefined.
Additional Locations (1)
| argvalues.unshift(current_fclos) | ||
| this.topScope.proceedMap.set(current_fclos._qid, current_fclos) | ||
| } | ||
| break |
There was a problem hiding this comment.
@before advice fails to execute original target method
High Severity
The processAop method handles @Before and @Around identically, but their semantics differ. For @Around, the advice controls execution flow via proceed(). However, @Before advice in Spring AOP runs first and then the original method should execute automatically afterward. Currently, for @Before, only the advice method is executed via processCall at line 501, but the original target method (current_fclos) is never invoked. This causes taint analysis to miss data flow through the actual target method.
| if (param.type === 'VariableDeclaration' && param.id?.type === 'Identifier') { | ||
| if (interceptorMethods.includes(entrypoint._sid) && param.id.name != 'request') { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Property name mismatch breaks interceptor parameter filtering
Medium Severity
The interceptor detection at line 46 uses n.sid to match method names like preHandle, and line 67 also uses entrypoint.sid. However, line 73 checks entrypoint._sid (with underscore) instead of entrypoint.sid. This property name mismatch causes the interceptor check to always fail, meaning all parameters from interceptor methods will be incorrectly added as taint sources instead of filtering to only include the request parameter.
Additional Locations (1)
| entryPoint.functionName = main.functionName | ||
| entryPoint.attribute = main.attribute | ||
| entryPoint.funcReceiverType = main.funcReceiverType | ||
| entryPoint.urlPath = this.extractUrlPath(main.ast._meta) |
There was a problem hiding this comment.
Missing null check on ast property causes crash
High Severity
The code accesses main.ast._meta without checking if main.ast exists. Interceptor entry points are collected at lines 44-50 with only a check for vtype === 'fclos' and method name matching, with no validation that ast exists. When processing these interceptors in spring-checker.ts, accessing main.ast._meta on an entry without an ast property will throw a TypeError. The original code at line 60 uses optional chaining (entrypoint.ast?.loc), but the new code in spring-checker.ts does not.
Additional Locations (1)
| } | ||
| } | ||
|
|
||
| return this.processCall(node, fclos, argvalues, state, scope) |
There was a problem hiding this comment.
Multiple @after advices execute original method repeatedly
Medium Severity
When multiple AOP advices target the same method, the loop iterates over all of them but handles them incorrectly. For each @After advice, super.processCallExpression is called, executing the original method multiple times. Additionally, only the last advice's fclos is used in the final processCall at line 501, meaning all other advices in the array are never actually executed. This results in incorrect taint analysis where the original method is over-analyzed and intermediate advices are skipped.
1 participant
Note
Introduces Spring framework support in taint analysis and enhances Java analysis for AOP.
taint_flow_spring_inputchecker (checker/taint/java/spring-checker.ts) that auto-collects Spring entrypoints/sources and captures URL paths from annotationsspring-analyzer.tsadds bean injection resolution, interceptor and AOP handling (@pointcut, @Around/@Before/@after, wildcard matching,proceedrouting) with supporting AOP maps inspring-initializer.tsspring-default-entrypoint.tsto include interceptor methods (preHandle,postHandle,afterCompletion) and to add appropriate taint sourcesjava-analyzer.ts: extractprepareArgValuesfromprocessCallExpressionand reuse it (consumed by Spring AOP flow)resource/checker/checker-config.jsonWritten by Cursor Bugbot for commit 27eaa06. This will update automatically on new commits. Configure here.