Skip to content

Comments

fix: Prevent SQL injection in metadata filter queries#1274

Merged
creatorrr merged 8 commits intodevfrom
fix-sql-injection-issue-1266
Mar 26, 2025
Merged

fix: Prevent SQL injection in metadata filter queries#1274
creatorrr merged 8 commits intodevfrom
fix-sql-injection-issue-1266

Conversation

@creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Mar 19, 2025

User description

Summary

  • Fixes potential SQL injection vulnerabilities identified in issue [Refactor]: Possible SQL Injection #1266
  • Creates a utility function build_metadata_filter_conditions that safely builds SQL conditions from metadata filters
  • Refactors queries using metadata filtering to utilize this utility function

Test plan


PR Type

Bug fix, Enhancement, Documentation


Description

  • Fixed potential SQL injection vulnerabilities in metadata filter queries.

    • Introduced build_metadata_filter_conditions utility for safe SQL condition building.
    • Refactored metadata filtering logic in bulk_delete_docs, list_docs, and list_agents.
  • Updated AlgoliaSearchArguments model to use list[str] for attributes_to_retrieve.

  • Enhanced documentation in CLAUDE.md with updated instructions for maintaining directory-specific files.

  • Updated schemas and TypeSpec files to align with new metadata filtering and Algolia model changes.


Changes walkthrough 📝

Relevant files
Enhancement
8 files
Tools.py
Updated `AlgoliaSearchArguments` model for attributes type
+2/-2     
utils.py
Added utility for safe metadata filter condition building
+38/-0   
Tools.py
Updated `AlgoliaSearchArguments` model for attributes type
+2/-2     
algolia.py
Adjusted Algolia search request to use updated attributes type
+1/-1     
create_agent_request.json
Added `BulkDeleteDocsRequest` schema for metadata filtering
+32/-0   
create_task_request.json
Added `BulkDeleteDocsRequest` schema for metadata filtering
+32/-0   
algolia.tsp
Updated TypeSpec for `AlgoliaSearchArguments` attributes type
+1/-1     
openapi-1.0.0.yaml
Updated OpenAPI spec for bulk delete and Algolia attributes
+8/-6     
Bug fix
3 files
list_agents.py
Refactored metadata filtering logic for listing agents     
+12/-7   
bulk_delete_docs.py
Refactored metadata filtering logic for bulk document deletion
+13/-9   
list_docs.py
Refactored metadata filtering logic for listing documents
+11/-6   
Documentation
2 files
helpers.py
Added clarification on `workflow` usage in parallel execution
+3/-0     
CLAUDE.md
Enhanced instructions for maintaining directory-specific documentation
+13/-2   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.

  • Important

    Fixes SQL injection vulnerabilities in metadata filter queries by introducing a safe SQL condition utility and refactoring affected queries.

    • Security Fix:
      • Introduced build_metadata_filter_conditions in utils.py to prevent SQL injection in metadata filter queries.
      • Refactored bulk_delete_docs, list_docs, and list_agents to use the new utility function.
    • Model Updates:
      • Changed attributes_to_retrieve in AlgoliaSearchArguments from dict[str, Any] to list[str] in Tools.py and algolia.py.
    • Documentation:
      • Enhanced CLAUDE.md with instructions for maintaining directory-specific files.
    • Schema and TypeSpec Updates:
      • Updated schemas in create_agent_request.json and create_task_request.json for new metadata filtering.
      • Updated algolia.tsp and openapi-1.0.0.yaml to align with model changes.
    • Testing:
      • Added tests for metadata filtering and SQL injection prevention in test_agent_metadata_filtering.py, test_docs_metadata_filtering.py, and test_metadata_filter_utils.py.

    This description was created by Ellipsis for f4d252f. It will automatically update as commits are pushed.

    creatorrr and others added 3 commits March 19, 2025 23:49
    - Changed AlgoliaSearchArguments.attributes_to_retrieve type from dict to list[str] in TypeSpec
    - Updated Algolia integration to use attributesToRetrieve parameter correctly
    - Added comment to explain workflow module usage in task_execution/helpers.py
    - Fixed SQL injection vulnerability in bulk_delete_docs.py with proper parameterization
    - Enhanced CLAUDE.md with instructions for working with nested CLAUDE.md files
    - Added note about modifying TypeSpec files instead of autogen files
    …──────────────────────�[0m
    
           �[38;5;238m│ �[0m�[1mSTDIN�[0m
    �[38;5;238m───────┼────────────────────────────────────────────────────────────────────────�[0m
    �[38;5;238m   1�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mfix: Prevent SQL injection in metadata filter queries�[0m
    �[38;5;238m   2�[0m   �[38;5;238m│�[0m
    �[38;5;238m   3�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mThis commit addresses issue #1266 by:�[0m
    �[38;5;238m   4�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m1. Creating a new `build_metadata_filter_conditions` utility function that safely builds SQL conditions from metadata filters �[0m
    �[38;5;238m   5�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m2. Refactoring queries that use metadata filtering to use this utility function�[0m
    �[38;5;238m   6�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m3. Ensuring proper parameterization for all SQL queries to prevent injection attacks�[0m
    �[38;5;238m   7�[0m   �[38;5;238m│�[0m
    �[38;5;238m   8�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m🤖 Generated with [Claude Code](https://claude.ai/code)�[0m
    �[38;5;238m   9�[0m   �[38;5;238m│�[0m
    �[38;5;238m  10�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mCo-Authored-By: Claude <noreply@anthropic.com>�[0m
    �[38;5;238m───────┴────────────────────────────────────────────────────────────────────────�[0m
    @qodo-free-for-open-source-projects
    Copy link
    Contributor

    qodo-free-for-open-source-projects bot commented Mar 19, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 522ac81)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    1266 - Fully compliant

    Compliant requirements:

    • Fix potential SQL injection vulnerabilities in metadata filter queries
    • Focus on bulk_delete_docs.py and other files where metadata filtering is used

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Parameter

    There's a comment indicating that the 'workflow' parameter is missing in the execute_map_reduce_step call, but it's not clear if this is intentional or if it could cause issues.

    # Note: 'workflow' parameter is missing here, but it's not needed because
    # 'workflow' is imported at the top of the file (`from temporalio import workflow`)
    # and used directly as a module, not as a parameter
    return await execute_map_reduce_step(
        context=context,
        execution_input=execution_input,
    SQL Condition Logic

    The build_metadata_filter_conditions function always adds " AND " prefix to conditions, which might cause issues if used in a context where this prefix isn't needed.

    sql_conditions = " AND ".join(conditions)
    if sql_conditions:
        sql_conditions = " AND " + sql_conditions

    @qodo-free-for-open-source-projects
    Copy link
    Contributor

    qodo-free-for-open-source-projects bot commented Mar 19, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 522ac81
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix Algolia API parameter handling
    Suggestion Impact:The commit implemented the exact suggestion by restructuring the search request construction to conditionally add attributesToRetrieve only when attributes_to_retrieve is not None, rather than using an empty list fallback

    code diff:

    -    search_request = {
    -        "requests": [
    -            {
    -                "indexName": arguments.index_name,
    -                "query": arguments.query,
    -                "hitsPerPage": arguments.hits_per_page,
    -                "attributesToRetrieve": arguments.attributes_to_retrieve or [],
    -            }
    -        ]
    +    search_params = {
    +        "indexName": arguments.index_name,
    +        "query": arguments.query,
    +        "hitsPerPage": arguments.hits_per_page,
         }
    +
    +    # Add attributes to retrieve if provided
    +    if arguments.attributes_to_retrieve is not None:
    +        search_params["attributesToRetrieve"] = arguments.attributes_to_retrieve
    +
    +    # Finalize the search request
    +    search_request = {"requests": [search_params]}

    The code is using the attributesToRetrieve key in the search request, but it's
    not properly handling the case when arguments.attributes_to_retrieve is None.
    The current implementation will set it to an empty list, but this might not be
    the expected behavior for the Algolia API.

    integrations-service/integrations/utils/integrations/algolia.py [30-38]

    -search_request = {
    -    "requests": [
    -        {
    -            "indexName": arguments.index_name,
    -            "query": arguments.query,
    -            "hitsPerPage": arguments.hits_per_page,
    -            "attributesToRetrieve": arguments.attributes_to_retrieve or [],
    -        }
    -    ]
    +search_params = {
    +    "indexName": arguments.index_name,
    +    "query": arguments.query,
    +    "hitsPerPage": arguments.hits_per_page,
     }
     
    +if arguments.attributes_to_retrieve is not None:
    +    search_params["attributesToRetrieve"] = arguments.attributes_to_retrieve
    +    
    +search_request = {
    +    "requests": [search_params]
    +}
    +

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: This suggestion improves the Algolia API integration by conditionally including the attributesToRetrieve parameter only when it's explicitly provided. This is a better approach than always including it with an empty list fallback, as it respects the API's default behavior when the parameter is omitted.

    Medium
    Add missing function parameter

    The comment indicates that the workflow parameter is missing in the function
    call, but it's unclear if this is intentional or an oversight. If
    execute_map_reduce_step() expects a workflow parameter, it should be provided to
    avoid potential runtime errors.

    agents-api/agents_api/workflows/task_execution/helpers.py [327-334]

     # Fall back to sequential map-reduce
    -# Note: 'workflow' parameter is missing here, but it's not needed because
    -# 'workflow' is imported at the top of the file (`from temporalio import workflow`)
    -# and used directly as a module, not as a parameter
     return await execute_map_reduce_step(
         context=context,
         execution_input=execution_input,
         map_defn=map_defn,
    +    workflow=workflow,  # Add the workflow parameter if required by the function
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion addresses a potential issue with a missing parameter, but the comment in the code already explains that the workflow parameter isn't needed because it's imported at the module level. The suggestion might improve code clarity but isn't critical since the code likely works as is.

    Low
    • Update

    Previous suggestions

    Suggestions up to commit c52217f
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Missing required function parameter

    The comment indicates that the 'workflow' parameter is not needed because it's
    imported as a module, but this is misleading. The function signature for
    'execute_map_reduce_step' likely requires the 'workflow' parameter. Check the
    function definition to confirm if this parameter is required and add it to
    prevent a potential runtime error.

    agents-api/agents_api/workflows/task_execution/helpers.py [327-334]

     # Fall back to sequential map-reduce
    -# Note: 'workflow' parameter is missing here, but it's not needed because
    -# 'workflow' is imported at the top of the file (`from temporalio import workflow`)
    -# and used directly as a module, not as a parameter
     return await execute_map_reduce_step(
    +    workflow=workflow,
         context=context,
         execution_input=execution_input,
         map_defn=map_defn,
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical issue where the 'workflow' parameter is missing in the function call, which could cause runtime errors. The comment is misleading as it suggests the parameter isn't needed, but the function likely requires it.

    High

    @entelligence-ai-pr-reviews
    Copy link
    Contributor

    LGTM 👍

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Reviewed everything up to c52217f in 1 minute and 26 seconds

    More details
    • Looked at 434 lines of code in 13 files
    • Skipped 0 files when reviewing.
    • Skipped posting 7 drafted comments based on config settings.
    1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:1452
    • Draft comment:
      The metadata_filter parameter is defined as an object with additionalProperties. Verify that the backend properly sanitizes these inputs so that the OpenAPI definition remains consistent with the SQL injection prevention measures.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    2. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:7070
    • Draft comment:
      The definition for Tools.AlgoliaSearchArguments now declares 'attributes_to_retrieve' as an array of strings instead of an object. This change appears correct given the new typespec model—ensure that all client integrations and documentation are updated accordingly.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    3. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:1452
    • Draft comment:
      The metadata_filter parameter is defined as an object with additionalProperties. Confirm that backend validation (and any related query building) safely handles this field to prevent SQL injection vulnerabilities.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    4. agents-api/agents_api/autogen/Tools.py:258
    • Draft comment:
      Typo in the docstring for 'download_pdf': the phrase 'The download the pdf of the results' is awkward and unclear. Consider rephrasing it to something like 'Whether to download the PDF of the results' or 'Download the PDF of the results' (with proper capitalization of 'PDF').
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    5. agents-api/agents_api/queries/utils.py:83
    • Draft comment:
      The definition of pg_query uses an unusual syntax "def pg_query**P", which seems like a typographical error. It might be intended to use generic type parameters without the extra asterisks (possibly 'def pg_queryP' or another correct syntax). Please double-check this syntax.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    6. agents-api/agents_api/workflows/task_execution/helpers.py:131
    • Draft comment:
      Typo found: 'seprated_workflow_name' should be corrected to 'separated_workflow_name' for clarity and consistency.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    7. integrations-service/integrations/autogen/Tools.py:258
    • Draft comment:
      Typo in the docstring for the 'download_pdf' field in ArxivSearchArguments. It currently says "The download the pdf of the results". Consider rephrasing it to something like "Download the PDF of the results" for clarity.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.

    Workflow ID: wflow_kxAlq8kifxNcwRlj


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    creatorrr and others added 2 commits March 20, 2025 00:29
    …──────────────────────�[0m
    
           �[38;5;238m│ �[0m�[1mSTDIN�[0m
    �[38;5;238m───────┼────────────────────────────────────────────────────────────────────────�[0m
    �[38;5;238m   1�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mfix: Add table_alias parameter to prevent ambiguous column references�[0m
    �[38;5;238m   2�[0m   �[38;5;238m│�[0m
    �[38;5;238m   3�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mThis commit enhances the SQL injection prevention fixes from the previous commit by:�[0m
    �[38;5;238m   4�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m1. Adding an optional table_alias parameter to the build_metadata_filter_conditions utility function�[0m
    �[38;5;238m   5�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m2. Updating metadata filtering code to use table aliases for columns to prevent ambiguous column references�[0m
    �[38;5;238m   6�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m3. Specifically fixing the "column reference 'metadata' is ambiguous" error�[0m
    �[38;5;238m   7�[0m   �[38;5;238m│�[0m
    �[38;5;238m   8�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m🤖 Generated with [Claude Code](https://claude.ai/code)�[0m
    �[38;5;238m   9�[0m   �[38;5;238m│�[0m
    �[38;5;238m  10�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mCo-Authored-By: Claude <noreply@anthropic.com>�[0m
    �[38;5;238m───────┴────────────────────────────────────────────────────────────────────────�[0m
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Incremental review on 5906da7 in 44 seconds

    More details
    • Looked at 66 lines of code in 3 files
    • Skipped 0 files when reviewing.
    • Skipped posting 6 drafted comments based on config settings.
    1. agents-api/agents_api/queries/docs/bulk_delete_docs.py:50
    • Draft comment:
      Good use of the utility with a table alias to ensure the metadata column is correctly referenced.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    2. agents-api/agents_api/queries/docs/list_docs.py:107
    • Draft comment:
      Properly updated call to build_metadata_filter_conditions with table alias 'd.'; ensures the WHERE clause references the correct table.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    3. agents-api/agents_api/queries/utils.py:343
    • Draft comment:
      The updated build_metadata_filter_conditions correctly accepts a table_alias parameter to prefix the metadata column. Ensure that any provided alias is safe (here it is hardcoded as 'd.' in calling functions).
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    4. agents-api/agents_api/queries/docs/bulk_delete_docs.py:50
    • Draft comment:
      Good use of the table_alias parameter ('d.') with build_metadata_filter_conditions to ensure the metadata column is correctly qualified. This helps mitigate SQL injection risks.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    5. agents-api/agents_api/queries/docs/list_docs.py:107
    • Draft comment:
      Consistently applying table_alias ('d.') in the metadata filter call is appropriate. Note that sort_by and direction are validated elsewhere, keeping the query safe from injection.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    6. agents-api/agents_api/queries/utils.py:343
    • Draft comment:
      The update to build_metadata_filter_conditions to accept a table_alias parameter improves flexibility and security. As an enhancement, consider grouping multiple metadata conditions in parentheses to clearly define precedence in complex queries.
    • Reason this comment was not posted:
      Confidence changes required: 33% <= threshold 50%
      None

    Workflow ID: wflow_8ed8koSXF8tO3qeO


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @creatorrr creatorrr marked this pull request as draft March 20, 2025 06:38
    @creatorrr creatorrr marked this pull request as ready for review March 25, 2025 12:12
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Reviewed everything up to 522ac81 in 1 minute and 26 seconds

    More details
    • Looked at 438 lines of code in 13 files
    • Skipped 0 files when reviewing.
    • Skipped posting 14 drafted comments based on config settings.
    1. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:7071
    • Draft comment:
      The change for AlgoliaSearchArguments now uses an array for attributes_to_retrieve instead of an object. This aligns with the intended model update. Please confirm that all associated endpoints and schema references (both for creation and updates) have been updated consistently.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    2. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:3024
    • Draft comment:
      BulkDeleteDocsRequest schema now clearly requires both metadata_filter and delete_all. Verify that the frontend and backend correctly interpret an empty metadata_filter (i.e. empty object) when delete_all is not set, to avoid unintentional deletions.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    3. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:1440
    • Draft comment:
      Ensure that any updated parameter definitions for metadata_filter in common pagination options are reflected in all endpoints (e.g. /agents/{id}/docs, /users/{id}/docs, etc.) to fully protect against SQL injection, by enforcing proper type as defined here.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    4. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:7077
    • Draft comment:
      AlgoliaSearchArguments now defines 'attributes_to_retrieve' as an array of strings, improving type safety.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    5. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:3025
    • Draft comment:
      BulkDeleteDocsRequest schema now requires both 'metadata_filter' and 'delete_all', ensuring consistent handling of metadata filtering.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    6. typespec/tsp-output/@typespec/openapi3/openapi-1.0.0.yaml:1452
    • Draft comment:
      The common parameter 'metadata_filter' is defined as an object with additionalProperties and a default empty object, aligning with the safe query filtering approach.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    7. CLAUDE.md:16
    • Draft comment:
      Typo: Consider changing 'fastAPI' to 'FastAPI' for consistency with the proper naming convention.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    8. CLAUDE.md:79
    • Draft comment:
      Typo: In the testing expression line, consider removing the space in '$ your_expr_here' to read '$your_expr_here'. This will make the example clearer.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    9. agents-api/agents_api/autogen/Tools.py:1344
    • Draft comment:
      Typo: In the FunctionDef class docstrings, "overriden" should be corrected to "overridden" for clarity.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    10. agents-api/agents_api/autogen/Tools.py:258
    • Draft comment:
      Typo in the ArxivSearchArguments (and ArxivSearchArgumentsUpdate) docstring: "The download the pdf of the results" is awkwardly phrased. Consider rephrasing to something like "Whether to download the PDF of the results".
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    11. agents-api/agents_api/workflows/task_execution/helpers.py:131
    • Draft comment:
      Typographical Error: The variable 'seprated_workflow_name' is misspelled. Consider renaming it to 'separated_workflow_name' for clarity.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    12. integrations-service/integrations/autogen/Tools.py:258
    • Draft comment:
      The docstring for 'download_pdf' currently reads 'The download the pdf of the results'. Consider revising it to something like 'Flag indicating whether to download the PDF of the results' to improve clarity.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    13. integrations-service/integrations/autogen/Tools.py:1344
    • Draft comment:
      There is a typographical error in the comment for FunctionDef: 'overriden' should be corrected to 'overridden' for proper spelling.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    14. schemas/create_task_request.json:3120
    • Draft comment:
      There's a typographical error in the schema: 'CompetionUsage' appears to be a misspelling. It should be 'CompletionUsage'.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.

    Workflow ID: wflow_v17F8DJWSc0nKC69


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @creatorrr
    Copy link
    Contributor Author

    @Vedantsahai18 @Ahmad-mtos - should the qodo comment be fixed?

    The code is using the attributesToRetrieve key in the search request, but it's
    not properly handling the case when arguments.attributes_to_retrieve is None.
    The current implementation will set it to an empty list, but this might not be
    the expected behavior for the Algolia API.

    integrations-service/integrations/utils/integrations/algolia.py [30-38]

    -search_request = {
    -    "requests": [
    -        {
    -            "indexName": arguments.index_name,
    -            "query": arguments.query,
    -            "hitsPerPage": arguments.hits_per_page,
    -            "attributesToRetrieve": arguments.attributes_to_retrieve or [],
    -        }
    -    ]
    +search_params = {
    +    "indexName": arguments.index_name,
    +    "query": arguments.query,
    +    "hitsPerPage": arguments.hits_per_page,
     }
     
    +if arguments.attributes_to_retrieve is not None:
    +    search_params["attributesToRetrieve"] = arguments.attributes_to_retrieve
    +    
    +search_request = {
    +    "requests": [search_params]
    +}
    +

    @Vedantsahai18
    Copy link
    Contributor

    Vedantsahai18 commented Mar 25, 2025

    @creatorrr the qodo comment is relevant. We should return ["*"] instead of []. Or we can keep it as None. Just not []
    image

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Incremental review on ba55e80 in 22 seconds

    More details
    • Looked at 33 lines of code in 1 files
    • Skipped 0 files when reviewing.
    • Skipped posting 2 drafted comments based on config settings.
    1. integrations-service/integrations/utils/integrations/algolia.py:36
    • Draft comment:
      Good refactor: splitting search_params and adding attributesToRetrieve conditionally improves clarity. No issues observed here.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    2. integrations-service/integrations/utils/integrations/algolia.py:30
    • Draft comment:
      Good refactoring: Separating the search parameters and conditionally adding 'attributesToRetrieve' improves clarity. This explicit check avoids unintended defaults when an empty list is passed. Note that this change doesn’t affect SQL injection prevention but aligns with safe practices elsewhere.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None

    Workflow ID: wflow_Wu2wdImt3r9OET4Z


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    …──────────────────────�[0m
    
           �[38;5;238m│ �[0m�[1mSTDIN�[0m
    �[38;5;238m───────┼────────────────────────────────────────────────────────────────────────�[0m
    �[38;5;238m   1�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mtest: Add tests for SQL injection prevention in metadata filters�[0m
    �[38;5;238m   2�[0m   �[38;5;238m│�[0m
    �[38;5;238m   3�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mThis commit adds test coverage for the SQL injection prevention fixes:�[0m
    �[38;5;238m   4�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m1. Tests for the new build_metadata_filter_conditions utility function�[0m
    �[38;5;238m   5�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m2. Tests for preventing SQL injection in list_docs and bulk_delete_docs�[0m
    �[38;5;238m   6�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m3. Tests for preventing SQL injection in list_agents�[0m
    �[38;5;238m   7�[0m   �[38;5;238m│�[0m
    �[38;5;238m   8�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mThe tests ensure that metadata filters containing special characters or SQL injection patterns�[0m
    �[38;5;238m   9�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mare properly escaped and parameterized, preventing SQL injection attacks.�[0m
    �[38;5;238m  10�[0m   �[38;5;238m│�[0m
    �[38;5;238m  11�[0m   �[38;5;238m│�[0m �[38;2;131;148;150m🤖 Generated with [Claude Code](https://claude.ai/code)�[0m
    �[38;5;238m  12�[0m   �[38;5;238m│�[0m
    �[38;5;238m  13�[0m   �[38;5;238m│�[0m �[38;2;131;148;150mCo-Authored-By: Claude <noreply@anthropic.com>�[0m
    �[38;5;238m───────┴────────────────────────────────────────────────────────────────────────�[0m
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Incremental review on c94f5be in 1 minute and 31 seconds

    More details
    • Looked at 475 lines of code in 3 files
    • Skipped 0 files when reviewing.
    • Skipped posting 6 drafted comments based on config settings.
    1. agents-api/tests/test_agent_metadata_filtering.py:107
    • Draft comment:
      Ensure SQL injections are properly handled even when metadata keys include quotes or dashes. The tests appear comprehensive.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    2. agents-api/tests/test_docs_metadata_filtering.py:60
    • Draft comment:
      Test injection filters for docs queries look adequate; verifying no unexpected deletions. Good job!
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    3. agents-api/tests/test_metadata_filter_utils.py:37
    • Draft comment:
      Be cautious of relying on dictionary key order for multiple filters. Consider sorting keys in tests or in build_metadata_filter_conditions for deterministic order.
    • Reason this comment was not posted:
      Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
      Python 3.7+ dictionaries maintain insertion order, but the test is relying on specific parameter ordering which could make it brittle. However, this is a test file, not production code. The test's purpose is to verify SQL injection prevention, not ordering. The exact string matching is perhaps overly strict but that's a different issue.
      The comment identifies a real issue with test brittleness, but is it important enough to fix? The test will still catch SQL injection issues regardless of parameter order.
      While technically correct, this is a minor test implementation detail that doesn't affect the core functionality being tested. The test's primary purpose of verifying SQL injection prevention is achieved either way.
      The comment should be deleted as it suggests a change that, while technically valid, is not important enough to warrant modifying the tests. The tests still effectively verify the core security functionality.
    4. agents-api/tests/test_agent_metadata_filtering.py:108
    • Draft comment:
      Injection filter tests cover common SQL injection patterns effectively. The test verifies that malicious keys/values return no results.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    5. agents-api/tests/test_docs_metadata_filtering.py:60
    • Draft comment:
      SQL injection tests for both list_docs and bulk_delete_docs are well-structured to ensure that unexpected matches or deletions do not occur.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    6. agents-api/tests/test_metadata_filter_utils.py:72
    • Draft comment:
      The utility tests for build_metadata_filter_conditions properly validate that SQL injection inputs are parameterized. Note that the test for multiple filters assumes a consistent dictionary order, which is safe in Python 3.7+ but might need attention if running on older versions.
    • Reason this comment was not posted:
      Confidence changes required: 33% <= threshold 50%
      None

    Workflow ID: wflow_jDV0CwNjuXmN0tuO


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    Signed-off-by: Diwank Singh Tomer <diwank.singh@gmail.com>
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

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

    👍 Looks good to me! Incremental review on f4d252f in 1 minute and 3 seconds

    More details
    • Looked at 56 lines of code in 2 files
    • Skipped 0 files when reviewing.
    • Skipped posting 7 drafted comments based on config settings.
    1. schemas/create_agent_request.json:196
    • Draft comment:
      Changed 'attributes_to_retrieve' type from object to an array of strings. Confirm that existing consumers are updated accordingly.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    2. schemas/create_task_request.json:196
    • Draft comment:
      Modified 'attributes_to_retrieve' schema to be an array of strings instead of an object. Verify that this schema change aligns with the application's expectations.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    3. schemas/create_agent_request.json:196
    • Draft comment:
      The change from type 'object' to an array with string items for 'attributes_to_retrieve' is clear and aligns with the intended model update.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    4. schemas/create_task_request.json:196
    • Draft comment:
      The update enforcing 'attributes_to_retrieve' as an array of strings (instead of an object) is consistent with the new AlgoliaSearchArguments model.
    • Reason this comment was not posted:
      Confidence changes required: 0% <= threshold 50%
      None
    5. schemas/create_task_request.json:1190
    • Draft comment:
      In the JSON schema definition for 'BrowserbaseContextArguments', the title is 'Projectid'. Consider changing this to 'Project ID' for improved readability and consistency with common naming conventions.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    6. schemas/create_task_request.json:1440
    • Draft comment:
      In the 'BrowserbaseExtensionArguments' definition, the title is given as 'Repositoryname'. For clarity and consistency, consider changing this to 'Repository Name'.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.
    7. schemas/create_task_request.json:1320
    • Draft comment:
      In the 'BrowserbaseCreateSessionArguments' definition, the title for the 'extensionId' property is given as 'Extensionid'. Consider updating this to 'Extension ID' to improve readability.
    • Reason this comment was not posted:
      Comment was not on a location in the diff, so it can't be submitted as a review comment.

    Workflow ID: wflow_vKCvnjrfRpZUoVSJ


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @creatorrr creatorrr merged commit bd07e8f into dev Mar 26, 2025
    13 checks passed
    @creatorrr creatorrr deleted the fix-sql-injection-issue-1266 branch March 26, 2025 13:36
    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.

    [Refactor]: Possible SQL Injection

    4 participants