Skip to content

fix(security): prevent IDOR in DataSourceOauthBinding by adding tenant_id check#33840

Closed
xr843 wants to merge 6 commits intolanggenius:mainfrom
xr843:fix/idor-datasource-binding
Closed

fix(security): prevent IDOR in DataSourceOauthBinding by adding tenant_id check#33840
xr843 wants to merge 6 commits intolanggenius:mainfrom
xr843:fix/idor-datasource-binding

Conversation

@xr843
Copy link
Copy Markdown
Contributor

@xr843 xr843 commented Mar 21, 2026

Summary

  • The patch method in DataSourceApi fetches a DataSourceOauthBinding by binding_id without verifying it belongs to the current user's tenant, allowing an authenticated attacker to enable/disable data source bindings belonging to other tenants (classic IDOR / horizontal privilege escalation).
  • This PR adds tenant_id filtering to the patch query, consistent with how the get method in the same class already scopes bindings to the current tenant.
  • Adds a unit test that verifies the compiled SQL includes tenant_id in the WHERE clause.

Changes

  • api/controllers/console/datasets/data_source.py: Add current_account_with_tenant() call and tenant_id=current_tenant_id to the filter_by() in the patch method.
  • api/tests/unit_tests/controllers/console/datasets/test_data_source.py: Add test_patch_binding_scoped_to_current_tenant to verify tenant scoping is present in the query.

Test plan

  • cd api && python -m pytest tests/unit_tests/controllers/console/datasets/test_data_source.py -q — all existing tests pass
  • New test test_patch_binding_scoped_to_current_tenant asserts tenant_id appears in the compiled WHERE clause

Fixes #31839

🤖 Generated with Claude Code

…t_id check

The patch method in DataSourceApi fetched a DataSourceOauthBinding by
binding_id without verifying it belongs to the current user's tenant.
An authenticated attacker could enable or disable data source bindings
belonging to other tenants by supplying their IDs.

Add tenant_id filtering to the query, consistent with how the get
method already scopes bindings to the current tenant.

Fixes langgenius#31839

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Mar 21, 2026
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical security vulnerability (IDOR) by enforcing tenant-level access control for data source OAuth binding modifications. It ensures that users can only enable or disable data source bindings that belong to their own tenant, preventing unauthorized access and manipulation of other tenants' data. The change enhances data isolation and system security, backed by a dedicated unit test to confirm the correct implementation of tenant scoping.

Highlights

  • Security Fix: Implemented a fix to prevent Insecure Direct Object Reference (IDOR) vulnerabilities in DataSourceOauthBinding by ensuring all patch operations are scoped to the current user's tenant.
  • Tenant Scoping: Added tenant_id filtering to the patch method in DataSourceApi, aligning its security posture with the existing get method to enforce proper data isolation.
  • Unit Testing: Introduced a new unit test to explicitly verify that the tenant_id is included in the WHERE clause of the compiled SQL query for patch operations, confirming the security enhancement.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly addresses a critical IDOR security vulnerability in the DataSourceApi's patch method by adding a tenant_id check when fetching a DataSourceOauthBinding. The fix is straightforward and effective. A new unit test is also added to verify that the query is correctly scoped to the tenant. I've added a suggestion to make the new test assertion more robust and pointed out some unused imports that can be cleaned up. Overall, this is a great security fix.

Comment on lines +179 to +210
def test_patch_binding_scoped_to_current_tenant(self, app, patch_tenant, mock_engine):
"""Verify that the patch query includes tenant_id to prevent IDOR attacks."""
from sqlalchemy import select as real_select

from models import DataSourceOauthBinding

api = DataSourceApi()
method = unwrap(api.patch)

binding = MagicMock(id="b1", disabled=True)

with (
app.test_request_context("/"),
patch("controllers.console.datasets.data_source.Session") as mock_session_class,
patch("controllers.console.datasets.data_source.db.session.add"),
patch("controllers.console.datasets.data_source.db.session.commit"),
):
mock_session = MagicMock()
mock_session_class.return_value.__enter__.return_value = mock_session
mock_session.execute.return_value.scalar_one_or_none.return_value = binding

method(api, "b1", "enable")

# Inspect the SELECT statement passed to session.execute
call_args = mock_session.execute.call_args
stmt = call_args[0][0]
compiled = stmt.compile(compile_kwargs={"literal_binds": True})
compiled_where = str(compiled)

assert "tenant_id" in compiled_where, (
"The patch query must filter by tenant_id to prevent IDOR vulnerabilities"
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The assertion assert "tenant_id" in compiled_where is a bit weak, as it only checks for the presence of the string "tenant_id". A more robust test would be to assert that the tenant_id is correctly used in the WHERE clause with the expected value from the tenant_ctx fixture (tenant-1). This ensures the filter is applied correctly.

Additionally, there are a couple of unused imports in this test method that can be removed:

  • from sqlalchemy import select as real_select on line 181.
  • from models import DataSourceOauthBinding on line 183.
    def test_patch_binding_scoped_to_current_tenant(self, app, patch_tenant, mock_engine):
        """Verify that the patch query includes tenant_id to prevent IDOR attacks."""
        api = DataSourceApi()
        method = unwrap(api.patch)

        binding = MagicMock(id="b1", disabled=True)

        with (
            app.test_request_context("/"),
            patch("controllers.console.datasets.data_source.Session") as mock_session_class,
            patch("controllers.console.datasets.data_source.db.session.add"),
            patch("controllers.console.datasets.data_source.db.session.commit"),
        ):
            mock_session = MagicMock()
            mock_session_class.return_value.__enter__.return_value = mock_session
            mock_session.execute.return_value.scalar_one_or_none.return_value = binding

            method(api, "b1", "enable")

            # Inspect the SELECT statement passed to session.execute
            call_args = mock_session.execute.call_args
            stmt = call_args[0][0]
            compiled = stmt.compile(compile_kwargs={"literal_binds": True})
            compiled_where = str(compiled)

            assert "tenant_id = 'tenant-1'" in compiled_where, (
                "The patch query must filter by tenant_id to prevent IDOR vulnerabilities"
            )

@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

@xr843
Copy link
Copy Markdown
Contributor Author

xr843 commented Mar 24, 2026

Superseded by #33986 (consolidated with constant-time API key comparison fix).

@xr843 xr843 closed this Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gemini warn about vulnerable to Insecure Direct Object Reference

2 participants