Update qa tests to v1.0.12#280
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
|
|
||
| // Block if the hostname is in the blocked domains list | ||
| if (ServiceConfigStore.shouldBlockOutgoingRequest(hostname)) { | ||
| if (ServiceConfigStore.shouldBlockOutgoingRequest(hostname) && !BypassedContextStore.isBypassed()) { |
There was a problem hiding this comment.
report(...) now includes enforcement via BypassedContextStore.isBypassed(), mixing reporting and blocking. Rename/split the method or document the enforcement behavior.
Details
✨ AI Reasoning
The report(...) method in the DNSRecordCollector previously performed DNS-related reporting and SSRF detection. The recent change adds a bypass check and conditional blocking via BypassedContextStore.isBypassed(), introducing enforcement behavior into a method named 'report'. This increases mixing of responsibilities and makes the method's purpose less self-evident, especially when callers expect reporting-only side effects. The problematic change occurs where the blocking condition was tightened to include the bypass check.
🔧 How do I fix it?
Use descriptive verb-noun function names, add docstrings explaining the function's purpose, or provide meaningful return type hints.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
draft: add DangerousBodyException
| if (!bypassed) { | ||
| // Bypassed IPs are trusted — don't report their outbound hostnames in heartbeats. | ||
| if (!ports.isEmpty()) { | ||
| for (int port : ports) { | ||
| HostnamesStore.incrementHits(hostname, port); | ||
| } | ||
| } else { | ||
| HostnamesStore.incrementHits(hostname, 0); | ||
| } |
There was a problem hiding this comment.
The if (!bypassed) block in report adds nesting around the main hostname-hit logic; invert the condition or use an early return/guard to flatten the method for clarity.
Show fix
| if (!bypassed) { | |
| // Bypassed IPs are trusted — don't report their outbound hostnames in heartbeats. | |
| if (!ports.isEmpty()) { | |
| for (int port : ports) { | |
| HostnamesStore.incrementHits(hostname, port); | |
| } | |
| } else { | |
| HostnamesStore.incrementHits(hostname, 0); | |
| } | |
| if (bypassed) { | |
| // Bypassed IPs are trusted — don't report their outbound hostnames in heartbeats. | |
| } else if (!ports.isEmpty()) { | |
| for (int port : ports) { | |
| HostnamesStore.incrementHits(hostname, port); | |
| } | |
| } else { | |
| HostnamesStore.incrementHits(hostname, 0); |
Details
✨ AI Reasoning
The report method now computes a bypassed flag and then wraps hostname hit-incrementing logic inside an if (!bypassed) else branch, increasing nesting. This buries the normal-path behavior and makes the flow harder to follow; using an early return or inverting the condition would flatten the logic and improve readability.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| // before the real lookup runs. Without this, getAllByName throws UnknownHostException | ||
| // and OnMethodExit never fires — meaning DNSRecordCollector can't block or track | ||
| // the hostname. | ||
| @Advice.OnMethodEnter(suppress = Throwable.class) |
There was a problem hiding this comment.
Converting and replacing the original 'hostname' argument (IDN.toASCII) before lookup can obscure the original domain and enable bypasses. Preserve or log the original hostname (or avoid in-place mutation) so downstream checks see the true input.
Details
✨ AI Reasoning
The change adds an entry advice that converts non-ASCII hostnames to ASCII (Punycode) before DNS lookup. Transforming inputs prior to resolution can hide the original domain representation from later inspection (e.g., logs or collectors) and may be used to bypass domain-based detection or allowlist checks. The transformation is done silently (catching IllegalArgumentException and returning) and replaces the hostname argument in place, which can obscure the original value from downstream code. This behavior changes how hostnames are presented to the rest of the system and therefore can degrade transparency and enable hiding malicious domains.
🔧 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
| if (hostname.charAt(i) > 0x7F) { | ||
| try { | ||
| hostname = IDN.toASCII(hostname, IDN.ALLOW_UNASSIGNED); | ||
| } catch (IllegalArgumentException ignored) { |
There was a problem hiding this comment.
Empty catch of IllegalArgumentException in the hostname normalization block silently swallows errors; add logging or explicit handling so failures are observable.
| } catch (IllegalArgumentException ignored) { | |
| } catch (IllegalArgumentException e) { | |
| // IDN conversion failed; log and continue with original hostname | |
| System.out.println("AIKIDO: Failed to convert hostname to ASCII: " + e.getMessage()); |
Details
✨ AI Reasoning
A new OnMethodEnter wrapper was added to convert internationalized hostnames to ASCII. Inside that addition, an empty catch block catches IllegalArgumentException and does nothing. Silently swallowing this exception hides parsing/conversion failures and makes debugging harder. The catch contains no handling, logging, or comment explaining intent; it neither rethrows nor records the error. This harms maintainability and can obscure failures in hostname normalization during DNS resolution.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
1 participant
Summary by Aikido
🚀 New Features
⚡ Enhancements
🔧 Refactors
More info