Conversation
Fix shell injection
Fix shell injection
Fix shell injection
Merge main
Removed the 'ref' parameter for the checkout action.
Merge main into run-qa-tests
Improve shell injection detector for qa tests
Merge main
Keep both encoded and unencoded http data
Don't skip whoami%00
Merge main
Merge main
Pr/outbound domain blocking
Fix run-qa-tests
Fix punycode domains
Force protection off shouldn't stop outbound request blocking
Move OutboundRequest Helper into Patcher
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| var prefix = typeof(WebRequestPatches).GetMethod(prefixMethodName, BindingFlags.Static | BindingFlags.NonPublic); | ||
| harmony.Patch(method, new HarmonyMethod(prefix)); | ||
| } | ||
| catch |
There was a problem hiding this comment.
PatchMethod swallows all exceptions in a bare catch block, hiding failures and reducing transparency.
Details
✨ AI Reasoning
An empty catch block was added around reflection/patching logic. Silently swallowing all exceptions without logging or rethrowing makes failures invisible and can mask runtime errors or malicious bypasses. This reduces transparency and hinders auditing/debugging. The suppression was introduced in this patch and worsens observability.
🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| var prefix = typeof(WebRequestPatches).GetMethod(prefixMethodName, BindingFlags.Static | BindingFlags.NonPublic); | ||
| harmony.Patch(method, new HarmonyMethod(prefix)); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Empty/bare catch in PatchMethod silently swallows exceptions. Add logging or explicit handling to surface patching failures.
Details
✨ AI Reasoning
A new catch block was added around the reflection/patching attempt that swallows all exceptions without any runtime reporting or handling beyond a comment. Silent catches around dynamic patching can hide failures in applying patches, making debugging and runtime visibility difficult. The try block invokes AccessTools.Method and harmony.Patch which are sensitive to runtime differences across framework versions; swallowing exceptions here can hide compatibility issues or broken instrumentation. The comment indicates intent to ignore but no logging or metrics are recorded, so failures will be invisible.
🔧 How do I fix it?
Add proper error handling in catch blocks. Log the error, show user feedback, or rethrow if needed.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| } | ||
| } | ||
|
|
||
| private static bool PrefixGetResponse(WebRequest __instance, MethodBase __originalMethod, ref WebResponse __result) |
There was a problem hiding this comment.
PrefixGetResponse and PrefixGetResponseAsync duplicate the same inspection flow; consolidate into a shared helper to avoid duplicated business logic.
Details
✨ AI Reasoning
Two newly added prefix methods perform the same outbound-request inspection flow: null-check the instance/URI, call OutboundRequestPatcher.Inspect with identical parameters, branch on inspection.ShouldProceed, and either allow the call or return/throw an inspection-driven exception. This is a substantial, non-trivial logical duplication introduced by the change and suitable for consolidation into a shared helper to avoid maintaining the same logic in multiple places.
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| private static bool PrefixSendAsyncWithCompletionOption(HttpRequestMessage request, HttpClient __instance, MethodBase __originalMethod, ref Task<HttpResponseMessage> __result, CancellationToken cancellationToken) | ||
| { | ||
| return InspectRequest(request, __instance, __originalMethod, ref __result); |
There was a problem hiding this comment.
PrefixSendAsyncWithCompletionOption and PrefixSendAsync are identical wrappers that duplicate the same logic. Consolidate into a single prefix or delegate to one method to avoid duplicated function bodies.
| private static bool PrefixSendAsyncWithCompletionOption(HttpRequestMessage request, HttpClient __instance, MethodBase __originalMethod, ref Task<HttpResponseMessage> __result, CancellationToken cancellationToken) | |
| { | |
| return InspectRequest(request, __instance, __originalMethod, ref __result); | |
| private static bool PrefixSendAsyncWithCompletionOption(HttpRequestMessage request, HttpClient __instance, MethodBase __originalMethod, ref Task<HttpResponseMessage> __result, CancellationToken cancellationToken) | |
| return PrefixSendAsync(request, __instance, __originalMethod, ref __result, cancellationToken); |
Details
✨ AI Reasoning
Two newly added methods in the same file perform the exact same operation: PrefixSendAsyncWithCompletionOption and PrefixSendAsync simply forward to InspectRequest and return its boolean result. This duplicates the entire function body (single-line wrapper) within HttpClientPatches, increasing maintenance surface because any change to the wrapper behaviour would need to be applied in two places. Consolidating them (e.g., using a single prefix method or extracting a shared helper) would reduce duplication.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Don't block aikido requests
| Agent.Instance.Context.BlockList.IsIPBypassed(context.RemoteAddress); | ||
| } | ||
|
|
||
| private static bool IsAikidoInternalTarget(Uri targetUri) |
There was a problem hiding this comment.
IsAikidoInternalTarget calls MatchesConfiguredAikidoEndpoint twice per inspection, duplicating work per outbound request; consider caching/parsing configured endpoints once.
Details
✨ AI Reasoning
The new Inspect flow calls IsAikidoInternalTarget, which in turn calls MatchesConfiguredAikidoEndpoint twice with different configured URLs. Each invocation performs repeated work (including URI parsing). Because Inspect runs per outbound request, these repeated calls scale linearly with requests and duplicate work that could be cached or precomputed. This harms throughput when many outbound requests occur.
🔧 How do I fix it?
Move constant work outside loops. Use StringBuilder instead of string concatenation in loops. Cache compiled regex patterns. Use hash-based lookups instead of nested loops. Batch database operations instead of N+1 queries.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Pr/outbound domain blocking
Skip inspect if no context
Pr/outbound domain blocking
Context needs to be built for outbound inspector to detect bypassed ip's
Pr/outbound domain blocking
Simplify IsIPBypassed
| return true; | ||
| } | ||
|
|
||
| if (context != null && Agent.Instance.Context.BlockList.IsIPBypassed(context.RemoteAddress)) |
There was a problem hiding this comment.
Early return for IP-bypassed contexts skips path traversal detection for the supplied 'paths', allowing access to filesystem resources identified by user-controlled paths. Ensure per-resource authorization/ownership checks before allowing file operations.
Details
✨ AI Reasoning
The change adds an early bypass in OnFileOperation that returns true when the context's RemoteAddress is marked as bypassed by BlockList. This causes the method to skip PathTraversalHelper.DetectPathTraversal and all downstream inspection/recording logic for the provided file paths. Because file paths are user-controlled identifiers that determine which filesystem objects are accessed, skipping the path traversal check effectively allows access to arbitrary filesystem paths for bypassed IPs without verifying that the requester is authorized to access those specific files. This creates an exposure of resources based on user-supplied identifiers without ownership/authorization checks.
🔧 How do I fix it?
Implement server-side access control check(s) to verify that the currently authenticated user is authorized to access the specific object or resource ID being requested.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Merge main
3 participants
Summary by Aikido
🚀 New Features
⚡ Enhancements
More info