Skip to content

Conversation

@carlosmonastyrski
Copy link
Contributor

Description 📣

Introduced a new tool at /upgrade-path to simplify version upgrades. It keeps track of known version issues and guides users on the correct upgrade path from version X to Y, including any required intermediate steps for fixes or adaptations.

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

@maidul98
Copy link
Collaborator

maidul98 commented Sep 15, 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 introduces a new Infisical Version Upgrade Tool that provides users with guided upgrade paths between Infisical versions. The implementation consists of both backend services and frontend components working together to analyze version compatibility and upgrade requirements.

Backend Implementation:
The new upgrade path service (backend/src/services/upgrade-path/) integrates with GitHub's API to fetch Infisical releases and processes a YAML configuration file (upgrade-path.yaml) to track breaking changes and database migrations. The service uses proper caching with the existing keyStore, implements rate limiting, and follows the established service factory pattern. Two new API endpoints are exposed at /api/v1/upgrade-path/versions (GET) and /api/v1/upgrade-path/calculate (POST) that are publicly accessible without authentication.

Frontend Implementation:
A new public page at /upgrade-path (frontend/src/pages/public/UpgradePathPage/) allows users to select source and target versions, then displays calculated upgrade paths including breaking changes, database migration warnings, and step-by-step instructions. The component uses React Query for data fetching and includes comprehensive error handling and user feedback.

Security & Architecture:
The implementation follows security best practices by using the RE2 regex library throughout to prevent ReDoS attacks, implementing proper input validation with Zod schemas, and including rate limiting on public endpoints. The service integrates cleanly with the existing Fastify service architecture and TypeScript type system.

Configuration & Dependencies:
The feature requires new dependencies (js-yaml for YAML parsing) and includes an extensible YAML configuration template for defining version-specific breaking changes and migration requirements. The tool is documented in the self-hosting upgrade guide with a link to the public tool.

PR Description Notes:

  • The "Type" checkboxes are not checked - this should be marked as "New feature"
  • The "Tests" section is empty and should include testing instructions
  • The acknowledgment checkbox is unchecked

Confidence score: 4/5

  • This PR introduces a substantial new feature with proper architecture but has some potential security considerations that need attention
  • Score reflects well-structured implementation with good security practices, though some areas like cache key construction and GitHub API integration need careful review
  • Pay close attention to the upgrade path service implementation, particularly around input validation and external API integration 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))
**Context -** For better readability, consider splitting complex expressions into multiple variables. ([link](https://app.greptile.com/review/custom-context?memory=891fb737-20dd-4e83-a0f7-907a2d3aa5ad))

<sub>18 files reviewed, 6 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_4531)</sub>

@carlosmonastyrski carlosmonastyrski requested review from DanielHougaard and removed request for akhilmhdh September 20, 2025 01:45
Copy link
Member

@DanielHougaard DanielHougaard left a comment

Choose a reason for hiding this comment

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

Few comments; I also think we should also add a CI test to validate the upgrade-path.yaml file when a PR is raised that makes changes to the upgrade-path.yaml file

@gitguardian
Copy link

gitguardian bot commented Sep 24, 2025

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9605380 Triggered Generic Private Key af085db backend/e2e-test/routes/v4/secrets.spec.ts View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Copy link
Member

@DanielHougaard DanielHougaard left a comment

Choose a reason for hiding this comment

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

Seems like we also didn't add the upgrade path link here:

CleanShot 2025-09-24 at 04 32 28@2x

Copy link
Member

@DanielHougaard DanielHougaard left a comment

Choose a reason for hiding this comment

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

This still doesn't seem to work. I get no errors on the backend - am I doing something wrong?

versions:
  v0.147.1:
    breaking_changes:
      - title: "Test test test"
        description: "Test"
        action: "Test"
    db_schema_changes: "Test"
    notes: "Test"
  v0.63.1:
    breaking_changes:
      - title: "Major update: Moved database system From MongoDB to PostgreSQL"
        description: "This update includes a major database schema change. Please review the migration guide and test thoroughly before deploying to production."
        action: "Review the migration guide and test thoroughly before deploying to production. Upgrade guide: https://infisical.com/docs/self-hosting/guides/mongo-to-postgres"
    db_schema_changes: "Major schema restructuring with table reorganization. Extended migration time: 3 minutes."
    notes: "Critical update requiring maintenance window. Test thoroughly before production deployment."
Image

Copy link
Member

@DanielHougaard DanielHougaard left a comment

Choose a reason for hiding this comment

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

few more comments

Copy link
Member

@DanielHougaard DanielHougaard left a comment

Choose a reason for hiding this comment

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

Dismissed stale review

@carlosmonastyrski carlosmonastyrski merged commit 73da2ad into main Sep 26, 2025
12 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.

4 participants