Add firewall-tester-action to run on every commit #238
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
|
|
||
| public static boolean startsWithUnsafePath(String filePath) { | ||
| String lowerCasePath = filePath.toLowerCase(); | ||
| public static boolean startsWithUnsafePath(String filePathRaw) { |
There was a problem hiding this comment.
Public method behavior changed: startsWithUnsafePath(String) now normalizes leading slashes and alters matching logic, which may break existing callers.
Details
✨ AI Reasoning
1) The code modifies public methods startsWithUnsafePath(String) and startsWithUnsafePath(String,String) to normalize leading slashes and change case handling via ensureOneLeadingSlash; 2) This alters the observable behavior of these public methods (they now change input paths by forcing a single leading slash when one or more slashes are present and change how matching is performed); 3) Changing a library's public method semantics can break existing callers that depended on the previous matching logic; 4) This is a meaningful behavioral change (not just refactoring) that clients may need to adapt to; 5) The change is not limited to internal/private methods and thus can break consumers; 6) The change is localized and could be addressed in the PR (e.g., keep previous behavior or provide a new method), so flagging is appropriate.
🔧 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 startsWithUnsafePath(filePath) && filePath.startsWith(userInput); | ||
| } | ||
|
|
||
| private static String ensureOneLeadingSlash(String path) { |
There was a problem hiding this comment.
Method ensureOneLeadingSlash does not ensure a leading slash for inputs without one, contradicting its name and likely caller expectations.
Details
✨ AI Reasoning
1) What the code does: A new helper ensureOneLeadingSlash(String) was added and is used to normalize inputs before path checks; it only collapses multiple leading slashes into one when the path already starts with '/', otherwise it leaves the path unchanged.
2) Does it harm maintainability: Yes — the method name suggests it will guarantee a leading slash for any input, but its implementation only removes extra slashes and does not add a leading slash when missing, creating a mismatch between name and behavior.
3) Why violation is true/false: This is a logic/contract bug introduced in this change because callers (and future maintainers) will reasonably expect the method to ensure at least one leading slash; the actual behavior can cause subtle mismatches in path comparisons.
🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
No description provided.