Implement URL normalization function and enhance SSRF detection #252
Conversation
PopoviciMarian
changed the title
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| case "https": | ||
| port = 443 | ||
| } | ||
| parsedURL.Host = parsedURL.Host + ":" + strconv.Itoa(int(port)) |
There was a problem hiding this comment.
Defaulting to port 0 and appending ":0" for unknown schemes results in invalid host values (e.g., example.com:0).
Details
✨ AI Reasoning
1) The new code appends a port to parsedURL.Host when parsedURL.Port() == "" by defaulting port to 0 for unknown schemes and always concatenating ":"; 2) This will produce incorrect hosts like "example.com:0" for non-http/https schemes (and generally a bogus :0) harming correctness and potentially downstream URL handling; 3) It was introduced by the new default-port logic in this PR;
🔧 How do I fix it?
Remove debugging statements like console.log, debugger, dd(), or logic bypasses like || true. Keep legitimate logging for monitoring and error handling.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| if len(userInput) <= 1 { | ||
| return false | ||
| } | ||
| // if hostname contains : we need to add the [ and ] to the hostname (ipv6) |
There was a problem hiding this comment.
Function parameter 'hostname' is reassigned (modified) in-place; avoid reassigning function arguments.
Details
✨ AI Reasoning
1) The code updates findHostnameInUserInput to modify the function parameters 'hostname' and 'userInput' in-place (lines 25-28 and 36): it wraps hostname in brackets for IPv6 and replaces userInput with a normalized value.
2) Reassigning parameters harms clarity because callers' original values are shadowed and readers can't easily see original inputs; it can confuse debugging and reasoning about the function.
3) This is a maintainability issue introduced by this change (the old code did not reassign these parameters at these locations). Therefore it's a valid violation of the no-argument-reassignment rule.
🔧 How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
|
|
||
| func getHostNameAndPort(urlCallbackId int) (string, uint32) { // urlcallbackid is the type of data we request, eg C.OUTGOING_REQUEST_URL | ||
| urlStr := Context.Callback(urlCallbackId) | ||
| // remove all control characters (< 32) and 0x7f(DEL) also replace \@ with @ and remove all whitespace |
There was a problem hiding this comment.
Comment describes what the code does (removing control chars and whitespace) instead of explaining why URL normalization is required or what invariants it preserves.
Details
✨ AI Reasoning
1) The added comment on line 52-53 describes the exact string transformations performed (removing control characters, replacing @, removing whitespace) rather than explaining why these transformations are necessary or any constraints around them.
2) This harms maintainability because such 'what' comments can become outdated when the helper implementation changes and do not convey the rationale behind normalizing the URL before parsing.
3) The violation is introduced by this PR because the comment was added in the changed lines and is not present before.
🔧 How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| if err == nil { | ||
| parsedURL.Host = parsedHost | ||
| } | ||
| // If the port is not present, we need to add it based on the scheme |
There was a problem hiding this comment.
Comment states the mechanical action (adding a port) rather than explaining why default ports are added; replace with a goal/rationale comment.
Details
✨ AI Reasoning
1) The added comment at line 22 describes what the following code does (adding a port when absent) rather than why this behavior is necessary; 2) This is a redundant "what" comment that doesn't add design or rationale and may become outdated; 3) It harms maintainability by duplicating obvious behavior instead of documenting the reason for adding default ports (e.g., normalizing for SSRF checks or downstream expectations); 4) Flagging is appropriate because replacing it with a brief rationale would be more valuable and is a small, local improvement; 5) The code context (URL normalization) makes the intent clear from code, so the comment is unnecessary; 6) Fix is a small change within the PR scope (update comment to explain rationale).
🔧 How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| ) | ||
|
|
||
| // remove all control characters (< 32) and 0x7f(DEL) + whitespace | ||
| func removeCTLByte(urlStr string) string { |
There was a problem hiding this comment.
Function parameter 'urlStr' is reassigned/modified inside removeCTLByte, harming clarity and risking logic errors when mutating while iterating.
Details
✨ AI Reasoning
1) The function removeCTLByte iterates over the input string and modifies the parameter 'urlStr' in-place by slicing it while using the original loop bounds.
2) Reassigning function parameters can confuse readers and makes it harder to reason about the original value; modifying the parameter while iterating also risks skipping characters due to changing length during iteration.
3) This was introduced in this diff as a new function and therefore is a regression relative to the prior code.
🔧 How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| return urlStr | ||
| } | ||
|
|
||
| func removeUserInfo(raw string) string { |
There was a problem hiding this comment.
Function parameter 'raw' is reused/reassigned within removeUserInfo instead of using distinct local variables, reducing clarity.
Details
✨ AI Reasoning
1) The function removeUserInfo treats its input 'raw' as mutable and reassigns it (splitting into scheme/rest and reusing 'raw' slices) instead of using new local variables for transformed values.
2) Reassigning parameters reduces clarity about original input and can lead to subtle bugs; the function was added in this diff.
3) This is a maintainability concern introduced by the change.
🔧 How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
|
|
||
| // ConvertIPv6Mapped converts IPv6-mapped IPv4 (only if it contains ::ffff:) | ||
| // Example: "http://[::ffff:10.0.0.1]" -> "http://10.0.0.1" | ||
| func convertIPv6Mapped(input string) string { |
There was a problem hiding this comment.
Function parameter 'input' is reassigned multiple times within convertIPv6Mapped instead of using separate local variables.
Details
✨ AI Reasoning
1) convertIPv6Mapped uses its 'input' parameter as a working variable, reassigning it multiple times (removing scheme, trimming brackets, replacing content).
2) Reassigning parameters harms readability and can obscure original input semantics; this function was added in the diff.
3) This is a maintainability concern introduced by the change.
🔧 How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| return scheme + ip | ||
| } | ||
|
|
||
| func NormalizeRawUrl(urlStr string) string { |
There was a problem hiding this comment.
Function parameter 'urlStr' is repeatedly reassigned in NormalizeRawUrl rather than using a new local variable for the normalized value.
Details
✨ AI Reasoning
1) NormalizeRawUrl takes 'urlStr' and repeatedly reassigns it through a series of transformations (UnescapeUrl, removeCTLByte, FixURL, removeUserInfo, convertIPv6Mapped).
2) Reassigning the parameter across multiple transforms reduces ability to reference the original input and can confuse readers; this top-level function was added in the diff.
3) This is a clarity/maintainability regression introduced here.
🔧 How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
No description provided.