LDAP support to create user if it does not exist in indiko but exists in linked ldap dir. Currently does not support deletion from ldap dir. if the user is deleted in ldap they will not be deleted in indiko
Add ldap support #1
expand 1 commit
hide 1 commit
Ran this through amp, and these are the main things it pointed out.
The rate limiting is fine, we should just document that in the readme that it should be rate limited. The other concerns seem reasonable, and then please also make sure you run this through biome as well :)
๐ด Security Issues (Must Fix)#
1. No rate limiting on ldapVerify#
This endpoint is effectively a password oracle against your LDAP directory. An attacker could brute-force LDAP credentials through this endpoint.
Recommendation: Add per-IP/username rate limiting, or document it as a deployment requirement at the reverse proxy layer.
2. Input validation missing#
No validation on username in ldapVerify. Should enforce:
- Length limits (e.g., โค 128 chars)
- Character restrictions (
^[A-Za-z0-9._@-]+$) to prevent LDAP injection if the library interpolates values into DNs or filters
// Add validation at the start of ldapVerify
if (!username || username.length > 128 || !/^[A-Za-z0-9._@-]+$/.test(username)) {
return Response.json(
{ error: "Invalid username format" },
{ status: 400 },
);
}
3. Information leakage#
The error messages could reveal whether a username exists in LDAP vs incorrect password. Use generic "Invalid credentials" for all auth failures.
Current: Different error paths may leak info
Recommended: Single generic error for all LDAP auth failures
4. Race condition in invite usage#
current_uses < max_uses check followed by separate increment can race under concurrent requests.
Recommendation: Use atomic update:
UPDATE invites SET current_uses = current_uses + 1
WHERE id = ? AND current_uses < max_uses
Then check affected rows โ if 0, the invite was already fully used.
๐ก Logic/Architecture Issues#
5. Orphaned accounts after LDAP deletion#
Users provisioned via LDAP will remain active even if deleted from LDAP. This is mentioned in the PR description but should be:
- Prominently documented for admins
- Consider adding a
provisioned_via_ldapcolumn on users for audit purposes - Recommend an admin review process or script for deprovisioning
6. No LDAP group/authorization check#
Any LDAP user matching the search base can self-provision. Consider adding optional group membership requirements via environment config (e.g., LDAP_REQUIRED_GROUP).
Required before merge:#
- Add input validation on username in
ldapVerify - Use generic error messages for LDAP auth failures
- Fix race condition in invite usage with atomic UPDATE
- Document rate limiting as a deployment requirement
- Add
provisioned_via_ldapflag on users table - Consider optional group membership requirement
expand 2 commits
hide 2 commits
I addressed those!
expand 3 commits
hide 3 commits
@dunkirk.sh I made the fixes. let me know what you think
expand 4 commits
hide 4 commits
i'm adding this directly to the main branch to get around the merge conflicts; i'll sign the commit with your email so it credits you properly
fixes #2