Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR plausible#5904

Type: Corrupted (contains bugs)

Original PR Title: CRM: enable team search by identifier via team:{UUID}
Original PR Description: ### Changes

image image

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

Original PR URL: plausible#5904


PR Type

Enhancement


Description

  • Enable team search by UUID identifier via team:{UUID} syntax

  • Add UUID detection logic to distinguish identifier vs name searches

  • Refactor Ecto query syntax with explicit parentheses for clarity

  • Expand test coverage with separate test cases for each search scenario


Diagram Walkthrough

flowchart LR
  A["Search Input: team:input"] --> B{"UUID Format?"}
  B -->|Yes| C["Query by identifier"]
  B -->|No| D["Query by name/email"]
  C --> E["Return matching team"]
  D --> E
  E --> F["Apply filters: +sub, +sso"]
Loading

File Walkthrough

Relevant files
Enhancement
team.ex
Add UUID identifier search with conditional query logic   

extra/lib/plausible/customer_support/resource/team.ex

  • Add conditional logic to detect UUID input and query by identifier
    field
  • Refactor query syntax with explicit from() parentheses for consistency
  • Separate UUID-based search from name/email-based search paths
  • Maintain existing filter logic for subscription and SSO integration
    queries
+33/-18 
search.ex
Implement UUID detection for team search input                     

extra/lib/plausible_web/live/customer_support/components/search.ex

  • Add Ecto.UUID.cast() validation to detect UUID format in search input
  • Set uuid_provided?: true option when valid UUID is detected
  • Remove unnecessary String.trim() call on search input
  • Pass UUID detection flag to resource search function
+9/-1     
Documentation
layout.ex
Update help documentation for team identifier search         

extra/lib/plausible_web/live/customer_support/components/layout.ex

  • Update help text to document team identifier search capability
  • Clarify that identifier must be provided complete and as-is
  • Mention team identifier as additional search field alongside
    name/email
+1/-1     
Tests
customer_support_test.exs
Expand team search tests with identifier and filter scenarios

test/plausible_web/live/customer_support_test.exs

  • Split monolithic team search test into separate focused test cases
  • Add dedicated test for UUID identifier-based team search
  • Add tests for partial name search, subscription filter, and SSO filter
    scenarios
  • Improve test organization and readability with clear test names
+58/-9   

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Inefficient UUID comparison

Description: The UUID search path uses fragment("?::text = ?", t.identifier, ^input) to compare
identifiers as text, which may bypass database index usage on the UUID column and enable
timing/DoS vectors via expensive casts; prefer binding the UUID type directly (e.g.,
where: t.identifier == ^uuid) after validated casting.
team.ex [41-43]

Referred Code
  where: fragment("?::text = ?", t.identifier, ^input),
  preload: [owners: o]
)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Auditing: The new team search paths (including UUID-based search) execute queries without any added
audit logging of the action, user, or outcome, which may be required for critical support
tooling.

Referred Code
if opts[:uuid_provided?] do
  from(t in Plausible.Teams.Team,
    as: :team,
    inner_join: o in assoc(t, :owners),
    where: fragment("?::text = ?", t.identifier, ^input),
    preload: [owners: o]
  )
else
  from(t in Plausible.Teams.Team,
    as: :team,
    inner_join: o in assoc(t, :owners),
    where:
      ilike(t.name, ^"%#{input}%") or
        ilike(o.name, ^"%#{input}%") or
        ilike(o.email, ^"%#{input}%"),
    limit: ^limit,
    order_by: [
      desc: fragment("?.name = ?", t, ^input),
      desc: fragment("?.name = ?", o, ^input),
      desc: fragment("?.email = ?", o, ^input),
      asc: t.name


 ... (clipped 4 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Input Edge Cases: The new UUID detection and removal of String.trim/1 may mishandle whitespace or malformed
inputs without explicit error handling or feedback paths visible in the diff.

Referred Code
defp maybe_focus_search("team:" <> rest) do
  [input | mods] = String.split(rest, "+", trim: true)

  opts =
    if "sub" in mods do
      [limit: 90, with_subscription_only?: true]
    else
      [limit: 90]
    end

  opts =
    if "sso" in mods do
      Keyword.merge(opts, with_sso_only?: true)
    else
      opts
    end

  opts =
    case Ecto.UUID.cast(input) do
      {:ok, _uuid} ->
        Keyword.merge(opts, uuid_provided?: true)


 ... (clipped 7 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Validation: While UUIDs are flagged via Ecto.UUID.cast/1 upstream, the name/email search path accepts
raw input for ilike filters and SSO domain or_where without visible normalization or
explicit sanitization beyond parameterization, requiring verification of safety and
intended behavior.

Referred Code
    from(t in Plausible.Teams.Team,
      as: :team,
      inner_join: o in assoc(t, :owners),
      where:
        ilike(t.name, ^"%#{input}%") or
          ilike(o.name, ^"%#{input}%") or
          ilike(o.email, ^"%#{input}%"),
      limit: ^limit,
      order_by: [
        desc: fragment("?.name = ?", t, ^input),
        desc: fragment("?.name = ?", o, ^input),
        desc: fragment("?.email = ?", o, ^input),
        asc: t.name
      ],
      preload: [owners: o]
    )
  end

q =
  if opts[:with_subscription_only?] do
    from(t in q,


 ... (clipped 24 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect SSO filter logic

Fix the +sso filter logic for UUID-based searches. When a UUID is provided, use
an inner_join to ensure the team has an SSO integration, instead of an or_where
which incorrectly broadens the search.

extra/lib/plausible/customer_support/resource/team.ex [78-89]

 q =
   if opts[:with_sso_only?] do
-    from t in q,
-      inner_join: sso_integration in assoc(t, :sso_integration),
-      as: :sso_integration,
-      left_join: sso_domains in assoc(sso_integration, :sso_domains),
-      as: :sso_domains,
-      or_where: ilike(sso_domains.domain, ^"%#{input}%")
+    if opts[:uuid_provided?] do
+      from(t in q, inner_join: _ in assoc(t, :sso_integration))
+    else
+      from(t in q,
+        inner_join: sso_integration in assoc(t, :sso_integration),
+        as: :sso_integration,
+        left_join: sso_domains in assoc(sso_integration, :sso_domains),
+        as: :sso_domains,
+        or_where: ilike(sso_domains.domain, ^"%#{input}%")
+      )
+    end
   else
     q
   end
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a significant bug where combining a UUID search with an +sso filter produces incorrect results due to an or_where clause, and it provides a correct fix.

High
General
Add limit to UUID query

Add limit: 1 to the team search query when searching by UUID. This ensures
consistency and prevents potential issues, as only one team is expected to
match.

extra/lib/plausible/customer_support/resource/team.ex [37-44]

 if opts[:uuid_provided?] do
   from(t in Plausible.Teams.Team,
     as: :team,
     inner_join: o in assoc(t, :owners),
     where: fragment("?::text = ?", t.identifier, ^input),
+    limit: 1,
     preload: [owners: o]
   )
 else
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out an inconsistency and potential (though unlikely) issue by omitting limit: 1. Adding the limit improves query robustness and aligns it with best practices for unique key lookups.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants