Skip to content

Conversation

x032205
Copy link
Contributor

@x032205 x032205 commented Aug 26, 2025

Description 📣

Lockout for LDAP

Related to: #4394

@x032205 x032205 requested a review from akhilmhdh August 26, 2025 07:11
@maidul98
Copy link
Collaborator

maidul98 commented Aug 26, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR implements a comprehensive LDAP authentication lockout feature for machine identities to prevent brute force attacks. The changes span both frontend and backend components to add configurable account lockout mechanisms after failed authentication attempts.

Backend Changes:

  • Added four new lockout configuration fields to both IdentityLdapAuth and IdentityUniversalAuth schemas: lockoutEnabled (default true), lockoutThreshold (default 3 attempts), lockoutDuration (default 300 seconds), and lockoutCounterReset (default 30 seconds)
  • Implemented lockout logic in authentication flows that tracks failed attempts using Redis keystore with TTL-based expiry
  • Added PostgreSQL advisory locks to prevent race conditions during concurrent login attempts
  • Created new API endpoints for manually clearing lockouts with proper authorization
  • Enhanced error responses to use UnauthorizedError instead of NotFoundError to prevent identity enumeration attacks
  • Updated audit logging to track lockout configuration changes and clear lockout operations

Frontend Changes:

  • Added new "Lockout" tab to both LDAP and Universal Auth configuration forms with validation for threshold (1-30), duration (30s-1day), and counter reset (5s-1hour)
  • Implemented duration conversion utilities (durationToSeconds, getObjectFromSeconds) in a centralized datetime helper module
  • Added LockoutFields component to display lockout configuration and provide "Clear All Lockouts" functionality with proper permission checks
  • Enhanced identity views to show lockout status with red lock icons and pass lockout state through component hierarchy
  • Added new audit log event types and constants for tracking lockout operations

Database Changes:

  • Created migrations to add lockout columns to both IdentityUniversalAuth and IdentityLdapAuth tables
  • Updated identity service to fetch active lockout status from Redis using key pattern matching
  • Added activeLockoutAuthMethods field to identity responses

Integration:

  • Added keystore dependency to identity services for encrypted lockout state management
  • Enhanced API documentation with lockout parameter descriptions
  • Maintained consistency between Universal Auth and LDAP Auth implementations

The feature provides administrators with flexible security controls while maintaining backward compatibility through sensible defaults. The implementation follows established patterns in the codebase and includes comprehensive error handling and user feedback mechanisms.

Confidence score: 4/5

  • This PR implements a complex security feature with potential for issues in concurrent scenarios and edge cases
  • Score reflects the comprehensive implementation but concerns about race conditions, error handling consistency, and validation complexity
  • Pay close attention to authentication flow logic, lockout validation, and database migration files

Context used:

Rule - # Greptile Code Review Prompt: OR Query Safety Check (knex.js)

Objective

Flag database queries that use or conditions without proper grouping, which can break outer filters and cause unintended data exposure.

What to Flag

Look for query builder patterns where or methods are called directly on the main query object without being wrapped in a subquery or callback function.

Flag these patterns:

  • .orWhere(), .orWhereRaw(), .orWhereILike(), .orWhereIn(), etc. called directly on the main query
  • Multiple chained or conditions without proper grouping
  • Any or condition that could bypass security filters or WHERE clauses applied elsewhere

Examples to FLAG:

// ❌ DANGEROUS - or conditions break outer filters
query.where('status', 'active')
  .orWhere('name', 'like', '%search%')
  .orWhere('email', 'like', '%search%');

// ❌ DANGEROUS - mixed with other conditions
query.where('tenantId', userId)
  .where('deleted_at', null)
  .orWhere('name', 'like', '%search%')
  .orWhereRaw('email ilike ?', ['%search%']);

What NOT to Flag

Do NOT flag or conditions that are properly grouped within a callback function or subquery.

Examples that are SAFE:

// ✅ SAFE - or conditions grouped in callback
query.where('status', 'active')
  .where((qb) => {
    qb.where('name', 'like', '%search%')
      .orWhere('email', 'like', '%search%');
  });

// ✅ SAFE - explicit subquery grouping
query.where('tenantId', userId)
  .where('deleted_at', null)
  .where(function() {
    this.orWhere('name', 'like', '%search%')
        .orWhere('email', 'like', '%search%');
  });

Detection Pattern

Flag any line containing:

  • .orWhere*() methods called directly on a query object
  • NOT preceded by .where(( or .where(function
  • NOT inside a callback function block

Review Message Template

When flagging, use this message:

⚠️ **Unsafe OR Query Detected**

This query uses `or` conditions directly on the main query object, which can bypass outer filters and security constraints.

**Issue:** OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.

**Fix:** Wrap OR conditions in a grouped WHERE clause:
```javascript
// Instead of this:
query.where('important_filter', value)
  .orWhere('field1', condition)
  .orWhere('field2', condition);

// Do this:
query.where('important_filter', value)
  .where((qb) => {
    qb.where('field1', condition)
      .orWhere('field2', condition);
  });

Security Impact: High - Could expose unauthorized data


## Additional Context
This pattern is particularly dangerous in multi-tenant applications, permission systems, or any query with security-critical WHERE clauses. Always ensure OR conditions are logically grouped to maintain the integrity of outer security filters. ([link](https://app.greptile.com/review/custom-context?memory=c4ca0367-148d-42b9-bcbd-958caf88aa07))
**Rule -** Add validation for string fields that have database column length limits to prevent insertion failures. For username template fields, add a 255 character limit validation. ([link](https://app.greptile.com/review/custom-context?memory=ab5293aa-4452-426c-a274-f034c06d0066))

<sub>37 files reviewed, 10 comments</sub>

<sub>[Edit Code Review Bot Settings](https://app.greptile.com/review/github) | [Greptile](https://greptile.com?utm_source=greptile_expert&utm_medium=github&utm_campaign=code_reviews&utm_content=infisical_4417)</sub>

@x032205
Copy link
Contributor Author

x032205 commented Sep 3, 2025

@greptile review this

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This review covers only the changes made since the last review, not the entire PR. The changes implement LDAP authentication lockout functionality for machine identities by adding lockout configuration fields, validation logic, and UI components. The key additions include:

  • Database Schema Updates: Migration adds four lockout fields (lockoutEnabled, lockoutThreshold, lockoutDurationSeconds, lockoutCounterResetSeconds) to the LDAP auth table with sensible defaults
  • Backend Service Enhancement: The LDAP auth service now includes distributed locking mechanisms using Redis keystore to track failed authentication attempts and enforce lockouts
  • API Extensions: New endpoints for clearing lockouts and updated LDAP auth endpoints to accept lockout configuration parameters
  • Frontend Components: New reusable LockoutTab and LockoutFields components extracted from Universal Auth to provide consistent lockout UI across authentication methods
  • Form Validation: Shared validation logic in superRefineLockout function to ensure lockout parameters fall within security bounds (30s-1day for duration, 5s-1hr for counter reset)
  • Audit Logging: Complete audit trail for LDAP lockout operations including configuration changes and lockout clearance events

The implementation mirrors the existing Universal Auth lockout pattern, providing consistent security controls across authentication methods. The lockout mechanism prevents brute force attacks by temporarily blocking authentication after configurable failed attempt thresholds, with administrative override capabilities.

Confidence score: 4/5

  • This PR implements important security features with well-structured code that follows existing patterns
  • Score reflects good implementation quality but potential for security-related edge cases in lockout logic
  • Pay close attention to the lockout validation logic and Redis key management patterns

Context used:

Rule - # Greptile Code Review Prompt: OR Query Safety Check (knex.js)

Objective

Flag database queries that use or conditions without proper grouping, which can break outer filters and cause unintended data exposure.

What to Flag

Look for query builder patterns where or methods are called directly on the main query object without being wrapped in a subquery or callback function.

Flag these patterns:

  • .orWhere(), .orWhereRaw(), .orWhereILike(), .orWhereIn(), etc. called directly on the main query
  • Multiple chained or conditions without proper grouping
  • Any or condition that could bypass security filters or WHERE clauses applied elsewhere

Examples to FLAG:

// ❌ DANGEROUS - or conditions break outer filters
query.where('status', 'active')
  .orWhere('name', 'like', '%search%')
  .orWhere('email', 'like', '%search%');

// ❌ DANGEROUS - mixed with other conditions
query.where('tenantId', userId)
  .where('deleted_at', null)
  .orWhere('name', 'like', '%search%')
  .orWhereRaw('email ilike ?', ['%search%']);

What NOT to Flag

Do NOT flag or conditions that are properly grouped within a callback function or subquery.

Examples that are SAFE:

// ✅ SAFE - or conditions grouped in callback
query.where('status', 'active')
  .where((qb) => {
    qb.where('name', 'like', '%search%')
      .orWhere('email', 'like', '%search%');
  });

// ✅ SAFE - explicit subquery grouping
query.where('tenantId', userId)
  .where('deleted_at', null)
  .where(function() {
    this.orWhere('name', 'like', '%search%')
        .orWhere('email', 'like', '%search%');
  });

Detection Pattern

Flag any line containing:

  • .orWhere*() methods called directly on a query object
  • NOT preceded by .where(( or .where(function
  • NOT inside a callback function block

Review Message Template

When flagging, use this message:

⚠️ **Unsafe OR Query Detected**

This query uses `or` conditions directly on the main query object, which can bypass outer filters and security constraints.

**Issue:** OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.

**Fix:** Wrap OR conditions in a grouped WHERE clause:
```javascript
// Instead of this:
query.where('important_filter', value)
  .orWhere('field1', condition)
  .orWhere('field2', condition);

// Do this:
query.where('important_filter', value)
  .where((qb) => {
    qb.where('field1', condition)
      .orWhere('field2', condition);
  });

Security Impact: High - Could expose unauthorized data


## Additional Context
This pattern is particularly dangerous in multi-tenant applications, permission systems, or any query with security-critical WHERE clauses. Always ensure OR conditions are logically grouped to maintain the integrity of outer security filters. ([link](https://app.greptile.com/review/custom-context?memory=c4ca0367-148d-42b9-bcbd-958caf88aa07))

<sub>21 files reviewed, 1 comment</sub>

<sub>[Edit Code Review Bot Settings](https://app.greptile.com/review/github) | [Greptile](https://greptile.com?utm_source=greptile_expert&utm_medium=github&utm_campaign=code_reviews&utm_content=infisical_4417)</sub>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants