IDOR protection

[hansott]

Summary by Aikido

Security Issues: 0	🔍 Quality Issues: 5	Resolved Issues: 0

🚀 New Features

• Added end-to-end IDOR protection feature across PHP and Go.

⚡ Enhancements

• Captured and serialized SQL parameters (including bound params) to JSON.
• Scoped IDOR configuration and tenant ID to per-request context cache.
• Made IDOR violations always trigger blocking before regular actions.

🔧 Refactors

• Loaded and wired idor analyzers from zen-internals with C bindings.

^{More info}

[aikido-pr-checks]

Function 'CheckForIdorViolation' is named with a 'Check' prefix which obscures whether it returns data or causes side effects; choose a clearer verb that reflects its behavior.

Details

✨ AI Reasoning
​The function CheckForIdorViolation appears to perform analysis and potentially returns an action string indicating a violation. Using the 'Check' prefix makes the intent ambiguous between returning data and causing side effects. This naming ambiguity hinders readability and maintainability when understanding the API at call sites.

🔧 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]

Function 'checkWhereFilters' uses the 'check' prefix, making its intent unclear (validation vs. side effects); prefer a name that states its purpose more explicitly.

Details

✨ AI Reasoning
​The function checkWhereFilters is an internal helper that validates filters and returns an error message string. The 'check' prefix still introduces ambiguity about intent (validation vs. mutation). Clearer naming would make its role more explicit.

🔧 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]

Function 'checkInsert' begins with 'check', which blurs whether it returns information or performs side effects; use a clearer name indicating its action.

Details

✨ AI Reasoning
​The function checkInsert examines INSERT columns and returns messages when violations are found. Beginning the name with 'check' obscures whether it only inspects or also mutates state. A more descriptive name would improve clarity.

🔧 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]

Logging tenantColumnName with %s may read past the buffer if it's not NUL-terminated. Use a length-limited format (e.g. "%.*s", (int)tenantColumnNameLength, tenantColumnName) to avoid out-of-bounds reads.

Details

✨ AI Reasoning
​The new enable_idor_protection handler receives a PHP string (pointer + length). Later it logs that pointer using a %s format without specifying the length. If the underlying string buffer is not NUL-terminated as expected by %s, the logging call may read past the buffer and cause a crash. The code does check that the pointer is non-null and length > 0 before logging, but that does not guarantee safe usage with %s. Using a length-limited format (e.g. "%.*s") or ensuring a NUL terminator prevents out-of-bounds reads.

🔧 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}

[aikido-pr-checks]

IsIdorViolation parses the event JSON, and action.Execute later reparses the same string. Avoid double JSON parsing by parsing once and reusing the result or by exposing a parsed-event check.

Show fix

Suggested change

	if (outputEvent.empty()) {
	return CONTINUE;
	}
	/* IDOR violations always throw, regardless of blocking mode */
	if (action.IsIdorViolation(outputEvent)) {
	if (outputEvent.empty()) {
	// Parse JSON once to avoid double parsing in IsIdorViolation and Execute
	json eventJson;
	bool isIdorViolation = false;
	try {
	eventJson = json::parse(outputEvent);
	isIdorViolation = eventJson.value("idor_violation", false);
	} catch (...) {
	// If parsing fails, treat as non-IDOR and let Execute handle it
	isIdorViolation = false;
	}
	/* IDOR violations always throw, regardless of blocking mode */
	if (isIdorViolation) {

Details

✨ AI Reasoning
​The code path now calls IsIdorViolation(outputEvent) early and then later calls action.Execute(outputEvent) which parses the same event string again. JSON parsing is non-trivial and can be frequent for events; parsing the same string twice doubles CPU and allocations. Consolidating parsing (parse once and reuse the parsed object or have Execute expose an API that allows checking the already-parsed event) would remove this wasted work. The regression was introduced by adding the IDOR pre-check before the existing Execute call.

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