Skip to content

Conversation

@DanielHougaard
Copy link
Member

No description provided.

@DanielHougaard DanielHougaard self-assigned this Sep 2, 2025
Copy link

@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 introduces a complete PHP SDK for Infisical's secret management platform. The implementation follows modern PHP 8.0+ standards with strict typing, readonly properties, and comprehensive tooling for quality assurance.

Core Architecture:

  • Main SDK Class (InfisicalSDK.php): Entry point that initializes HTTP client and manages service instances with authentication callback handling
  • HTTP Layer (HttpClient.php): Guzzle wrapper providing standardized GET, POST, PATCH, DELETE methods with JSON support and 30-second timeouts
  • Service Layer: AuthService (factory for auth services), UniversalAuthService (machine identity authentication), and SecretsService (CRUD operations for secrets)
  • Models: Comprehensive data models including Secret, SecretImport, MachineIdentityCredential, and parameter classes for each operation

Key Features:

  • Universal authentication for machine-to-machine communication
  • Full secret lifecycle management (list, get, create, update, delete)
  • Secret imports with precedence handling (imported secrets override regular ones)
  • Optional environment variable injection via putenv()
  • Recursive secret fetching with deduplication logic
  • Boolean parameter handling with string conversion for API compatibility

Development Infrastructure:

  • Testing: PHPUnit configuration with coverage reporting
  • Quality Assurance: PHPStan level 8 static analysis, PHP CS Fixer for PSR-12 compliance
  • CI/CD: GitHub Actions for multi-version testing (PHP 8.0-8.3) and automated Packagist publishing
  • Package Management: Composer configuration with Guzzle HTTP client and Valinor for data mapping

The SDK uses consistent parameter mapping where projectId becomes workspaceId in API calls, suggesting an effort to provide intuitive naming while maintaining API compatibility. The codebase follows service-oriented architecture with proper dependency injection and factory patterns.

Confidence score: 2/5

  • This PR has significant security and reliability concerns that could cause production issues
  • Score reflects critical issues in authentication recursion, unsafe putenv() usage, and inadequate error handling
  • Pay close attention to InfisicalSDK.php, SecretsService.php, and UpdateSecretParameters.php

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, 15 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=php-sdk_1)</sub>

@DanielHougaard DanielHougaard merged commit 1a2d00a into main Sep 2, 2025
4 checks passed
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