re-use vulnerability type instances to avoid unnecessary mem usage #247
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| } | ||
| public static final class SQLInjectionVulnerability implements Vulnerability { | ||
|
|
||
| private static final class SQLInjectionVulnerability implements Vulnerability { |
There was a problem hiding this comment.
Changed inner vulnerability classes from public to private and replaced type access with single instances, breaking the previous public API (type-level access).
Details
🔧 How do I fix it?
Support both old and new parameter names during transition periods. Add new optional parameters with defaults. Keep existing response fields while adding new ones. Focus on backwards compatibility.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| return new Attack( | ||
| operation, | ||
| new Vulnerabilities.SSRFVulnerability(), | ||
| Vulnerabilities.SSRF, |
There was a problem hiding this comment.
Replacing per-call new Vulnerabilities.SSRFVulnerability() with shared Vulnerabilities.SSRF may introduce thread-safety issues if the shared instance is mutable.
Details
✨ AI Reasoning
1) The run method constructs an Attack object; previously it created a fresh Vulnerabilities.SSRFVulnerability() instance per call, now it uses a shared Vulnerabilities.SSRF instance.
2) Reusing a shared vulnerability instance can introduce thread-safety issues if that instance is mutable or carries per-attack state, leading to data races or corrupted state in concurrent executions.
3) This change replaces per-call allocation with a shared object, increasing the risk compared to previous behavior.
4) Fixing would likely require ensuring the shared instance is immutable or synchronized, which is reasonable within this PR's scope.
5) Reusing singletons is a common pattern but only safe when instances are immutable or thread-safe.
6) The change is localized to the Attack construction and thus actionable here.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| return new Attack( | ||
| operation, | ||
| new Vulnerabilities.StoredSSRFVulnerability(), | ||
| Vulnerabilities.STORED_SSRF, |
There was a problem hiding this comment.
Replacing a per-call Vulnerability object with a shared Vulnerabilities.STORED_SSRF instance may introduce unsafe shared mutable state access across threads.
Details
✨ AI Reasoning
1) The method creates an Attack object and previously instantiated a fresh Vulnerabilities.StoredSSRFVulnerability per call; the change replaces that with a shared Vulnerabilities.STORED_SSRF instance.
2) Reusing a shared vulnerability instance can introduce shared mutable state if the Vulnerability object is not immutable, risking data races or accidental cross-request mutation in multi-threaded environments.
3) This harms maintainability and could cause concurrency bugs if the shared instance is mutated elsewhere.
4) Changing from per-call allocation to a shared instance is a localized change and its thread-safety implications are directly introduced by this PR.
5) It's not known here whether the instance is immutable, so the conservative action is to flag the potential issue.
6) The fix may be to ensure the shared object is immutable or to synchronize access, which is reasonable within the PR scope.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| // scan | ||
| Vulnerabilities.Vulnerability vulnerability = new Vulnerabilities.ShellInjectionVulnerability(); | ||
| Scanner.scanForGivenVulnerability(vulnerability, "runtime.Exec", new String[]{commandStr}); | ||
| Scanner.scanForGivenVulnerability(Vulnerabilities.SHELL_INJECTION, "runtime.Exec", new String[]{commandStr}); |
There was a problem hiding this comment.
Replaced per-call new Vulnerabilities.ShellInjectionVulnerability() with shared Vulnerabilities.SHELL_INJECTION, which can introduce thread-safety/data-sharing issues if the vulnerability object is mutable.
Details
✨ AI Reasoning
1) The change replaces creating a new Vulnerabilities.ShellInjectionVulnerability() with reusing a shared instance Vulnerabilities.SHELL_INJECTION when calling Scanner.scanForGivenVulnerability.
2) If the Vulnerability instances are mutable or Scanner/consumers modify instance state, reusing a single static instance can introduce data races or incorrect cross-request/scan contamination in multithreaded use.
3) This harms maintainability and can cause subtle concurrency bugs if the object was previously isolated per-call. Therefore this change potentially introduces a thread-safety regression.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| // scan | ||
| Vulnerabilities.Vulnerability vulnerability = new Vulnerabilities.SQLInjectionVulnerability(); | ||
| Scanner.scanForGivenVulnerability(vulnerability, operation, new String[]{sql, dialect}); | ||
| Scanner.scanForGivenVulnerability(Vulnerabilities.SQL_INJECTION, operation, new String[]{sql, dialect}); |
There was a problem hiding this comment.
Reusing a shared Vulnerabilities.SQL_INJECTION instance may introduce thread-safety issues if the vulnerability object is mutable or mutated during scanning.
Details
✨ AI Reasoning
1) The code replaces creating a new Vulnerabilities.SQLInjectionVulnerability() per call with reusing a shared instance Vulnerabilities.SQL_INJECTION when calling Scanner.scanForGivenVulnerability.
2) Reusing a shared/static vulnerability instance can introduce data races or inconsistent behavior if that instance is mutable or if Scanner or other code mutates it during scanning, harming thread-safety in a multi-threaded environment.
3) This is a change introduced by the diff (previously per-call instantiation avoided shared mutable state), so the risk is newly introduced by this PR.
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
No description provided.