IDOR protection by hansott · Pull Request #391 · AikidoSec/firewall-php

hansott · 2026-02-13T10:53:03Z

Summary by Aikido

⚠️ Security Issues: 1	🔍 Quality Issues: 4	Resolved Issues: 0

🚀 New Features

Added IDOR protection feature with PHP API and Go integration

⚡ Enhancements

Captured and serialized SQL parameter values into JSON for queries
Made IDOR violations immediately throw regardless of blocking mode
Implemented lazy-loaded, request-scoped IDOR config parsing and validation

🔧 Refactors

Added PHP compatibility typedef and adjusted zend_result/type handling

^{More info}

aikido-pr-checks · 2026-02-13T12:38:07Z

lib/request-processor/handle_sql_queries.go

 		return attack.ReportAttackDetected(res)
 	}
+
+	if idor.IsIdorEnabled() && !context.IsIdorDisabled() {


Function name 'CheckForIdorViolation' uses the 'Check' prefix which is ambiguous. Rename to a clearer verb that indicates its behavior or return type (e.g., 'DetectIdorViolation' or 'IsIdorViolation') to clarify intent.

Details

✨ AI Reasoning
A new code path was added that calls a function whose name begins with 'Check' (CheckForIdorViolation). The rule discourages 'check' prefixes because they ambiguously suggest either a boolean predicate or side-effecting behavior. The change introduced this usage, so it is a new violation. The problematic symbol appears in the newly added logic that runs IDOR detection, specifically at the call site where the function is invoked. Renaming the function to a name that clearly indicates its behavior or return type would resolve the issue.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-02-13T12:38:08Z

lib/request-processor/vulnerabilities/idor/idor.go

+	return ""
+}
+
+func checkWhereFilters(queryResult SqlQueryResult, tenantId string, params *SqlParams) string {


Function 'checkWhereFilters' starts with 'check', making its intent ambiguous (return vs side effects). Use a clearer verb that indicates behavior.

Details

✨ AI Reasoning
A new function named 'checkWhereFilters' was added. Function names beginning with 'check' are ambiguous about return/value semantics versus side effects. This harms clarity when reading the code and when reasoning about whether the function mutates state or returns data.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-02-13T12:38:08Z

lib/request-processor/vulnerabilities/idor/idor.go

+	return ""
+}
+
+func checkInsert(queryResult SqlQueryResult, tenantId string, params *SqlParams) string {


Function 'checkInsert' starts with 'check', making its intent ambiguous (return vs side effects). Use a clearer verb that indicates behavior.

Details

✨ AI Reasoning
A new function named 'checkInsert' was added. Function names beginning with 'check' are ambiguous about return/value semantics versus side effects. This harms clarity when reading the code and when reasoning about whether the function mutates state or returns data.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-02-13T12:38:09Z

lib/php-extension/HandleIdorProtection.cpp

+            ZVAL_COPY_VALUE(return_value, &retval);
+        }
+    } else {
+        zval_ptr_dtor(&retval);


Calling zval_ptr_dtor(&retval) when 'retval' may still be ZVAL_UNDEF can cause a crash; ensure 'retval' is initialized or check Z_ISUNDEF(retval) before dtor.

Details

✨ AI Reasoning
The wrapper prepares a local zval 'retval' and sets it to undefined. The callable is invoked; if invocation fails, code calls zval_ptr_dtor(&retval). If the function never populated 'retval' (it remains ZVAL_UNDEF), calling the destructor can operate on an invalid/undefined value, risking a segmentation fault. The check for definedness only occurs in the success branch, so the destructor call on failure is not guarded.

🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

… load (#392) * fix: scope IDOR config to request context, single JSON callback, lazy load

aikido-pr-checks · 2026-02-17T12:22:01Z

lib/request-processor/context/cache.go

+// ContextSetIdorConfig loads IDOR config lazily (on first GetIdorConfig() use)
+// from PHP via CONTEXT_IDOR_CONFIG callback.


Comment on ContextSetIdorConfig restates what the function does. Remove or replace with rationale (why lazy loading and callback are required) instead of repeating behavior.

Details

✨ AI Reasoning
The added comment above ContextSetIdorConfig merely restates the function's behavior (lazy load of IDOR config from PHP/CONTEXT_IDOR_CONFIG). It does not explain why the lazy load is required, why the CONTEXT_IDOR_CONFIG callback exists, nor any constraints or design decisions. Such "what" comments increase maintenance burden without added value.

🔧 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'.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

Show Fix

Remediation - low confidence
This patch mitigates the mechanical comment issue by replacing the comment that restated the lazy loading mechanism with a comment that explains the rationale for on-demand initialization (avoiding cross-language overhead when IDOR protection is not needed).

Suggested change

// ContextSetIdorConfig loads IDOR config lazily (on first GetIdorConfig() use)

// from PHP via CONTEXT_IDOR_CONFIG callback.

// ContextSetIdorConfig initializes IDOR config on-demand to avoid cross-language

// overhead when IDOR protection is not used for the current request.

hansott added 8 commits February 13, 2026 11:52

Initial commit IDOR protection

7a0298a

Fix compile

d1413ed

Add HandleIdorProtection.cpp

ef22c29

Fix zend_result type

55c6871

Cast to zend_result

43cacbf

Fix SQL injection test

975ac92

Handle error when converting invalid UTF-8 bytes to JSON

d8a19d4

Use json::error_handler_t::replace instead of try/catch

d8031c8

hansott marked this pull request as ready for review February 13, 2026 12:37

aikido-pr-checks bot reviewed Feb 13, 2026

View reviewed changes

fix: scope IDOR config to request context, single JSON callback, lazy…

c222de1

… load (#392) * fix: scope IDOR config to request context, single JSON callback, lazy load

aikido-pr-checks bot reviewed Feb 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

IDOR protection#391

IDOR protection#391
hansott wants to merge 9 commits intomainfrom
idor

hansott commented Feb 13, 2026 •

edited by aikido-pr-checks bot

Loading

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Uh oh!

aikido-pr-checks bot Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// ContextSetIdorConfig loads IDOR config lazily (on first GetIdorConfig() use)
		// from PHP via CONTEXT_IDOR_CONFIG callback.

Comments

Conversation

hansott commented Feb 13, 2026 • edited by aikido-pr-checks bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Aikido

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks bot Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks bot Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hansott commented Feb 13, 2026 •

edited by aikido-pr-checks bot

Loading