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.
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 },
);
}
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_ldap column 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: #
