From 13040a85123bb652f4bc2ed9d136bad1ba96cd62 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 26 May 2026 18:21:36 +0100 Subject: [PATCH 1/3] Fix the comments Signed-off-by: kerthcet --- alphatrion/log/load.py | 4 ++-- alphatrion/log/log.py | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/alphatrion/log/load.py b/alphatrion/log/load.py index 7456905..530c66f 100644 --- a/alphatrion/log/load.py +++ b/alphatrion/log/load.py @@ -38,8 +38,8 @@ async def load_checkpoint( ) -> list[str]: """ Load checkpoint from artifact registry, the path is expected to be in the format of: - - OCI: "org_id/team_id/exp_id/ckpt:version" - - S3: "org_id/team_id/exp_id/ckpt/filename" + - OCI: "org_id/team_id/exp_id/ckpt:version_or_filename", it should be a version. + - S3: "org_id/team_id/exp_id/ckpt/version_or_filename", it should be a filename. :param id: the id of the experiment. :param version_or_filename: the version or filename of the checkpoint to load, default is "latest". diff --git a/alphatrion/log/log.py b/alphatrion/log/log.py index 96a9037..b0a3bc8 100644 --- a/alphatrion/log/log.py +++ b/alphatrion/log/log.py @@ -182,11 +182,6 @@ async def log_metrics(metrics: dict[str, float]): exp.done() -# log_records is used to log a list of records, which is similar to log_metrics -# but for tracing the execution of the code. -# async def log_records(): - - async def log_dataset( name: str, data_or_path: dict[str, Any] | str | list[str], From 4548f2ff8a6731a20626405cd787ca82d8730068 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 26 May 2026 19:43:16 +0100 Subject: [PATCH 2/3] Fix permission issue when querying from the database Signed-off-by: kerthcet --- SECURITY_ANALYSIS.md | 330 ++++++++++++++++++ alphatrion/server/graphql/resolvers.py | 87 ++--- alphatrion/storage/sqlstore.py | 34 +- .../server/test_graphql_mutation.py | 8 +- tests/integration/test_s3_backend.py | 4 +- tests/unit/storage/test_sql.py | 76 ++++ 6 files changed, 472 insertions(+), 67 deletions(-) create mode 100644 SECURITY_ANALYSIS.md diff --git a/SECURITY_ANALYSIS.md b/SECURITY_ANALYSIS.md new file mode 100644 index 0000000..adc540e --- /dev/null +++ b/SECURITY_ANALYSIS.md @@ -0,0 +1,330 @@ +# Security Analysis: SQL Access Control Vulnerabilities + +## Executive Summary + +Analysis Date: 2026-05-26 +Analyzed File: `alphatrion/storage/sqlstore.py` +Status: **1 CRITICAL BUG FOUND** + Several potential vulnerabilities identified + +--- + +## 🚨 CRITICAL: Bug in `dataset_is_accessible_to_user` + +### Location +File: `alphatrion/storage/sqlstore.py` +Lines: 1496-1523 + +### The Bug +```python +def dataset_is_accessible_to_user( + self, dataset_id: uuid.UUID, user_id: uuid.UUID +) -> bool: + session = self._session() + dst = ( + session.query(Dataset) + .filter(Dataset.uuid == dataset_id, Agent.is_del == 0) # ← BUG HERE! + .first() + ) +``` + +**Problem**: The query filters by `Agent.is_del == 0` instead of `Dataset.is_del == 0` + +**Impact**: +- This will cause a SQL error or unexpected behavior +- Deleted datasets may still be accessible +- Query might fail completely + +**Fix**: +```python +.filter(Dataset.uuid == dataset_id, Dataset.is_del == 0) # Correct +``` + +--- + +## 🔍 Security Vulnerability Analysis + +### 1. ✅ `user_is_super_admin_in_org` - SECURE + +**Query Pattern**: +```python +session.query(TeamMember) + .filter( + TeamMember.user_id == user_id, + TeamMember.org_id == org_id, + TeamMember.role == MemberRole.SUPER_ADMIN, + ) +``` + +**Security Assessment**: ✅ SAFE +- Checks all three required fields +- No cross-org privilege escalation possible +- Properly validates org membership + +--- + +### 2. ⚠️ `team_is_accessible_to_user` - POTENTIAL ISSUE + +**Query Pattern**: +```python +# First get team (no org check!) +team = self.get_team(team_id) + +# Then check super admin +if self.user_is_super_admin_in_org(user_id, team.org_id): + return True + +# Or check team membership +membership = session.query(TeamMember).filter( + TeamMember.team_id == team_id, + TeamMember.user_id == user_id, +).first() +``` + +**Potential Vulnerability**: +```python +def get_team(self, team_id): + # Returns team WITHOUT checking if user's org matches! + return session.query(Team).filter(Team.uuid == team_id).first() +``` + +**Attack Scenario**: +1. Attacker is user in Org A +2. Target team is in Org B +3. `get_team()` returns Team B (no org filter!) +4. Super admin check: `user_is_super_admin_in_org(user_A, org_B)` → False ✅ +5. Team membership check: `TeamMember.team_id == team_B AND user_id == user_A` → False ✅ + +**Conclusion**: ✅ SAFE - Both checks will fail, but inefficient + +**Recommendation**: Add org validation early: +```python +def team_is_accessible_to_user(self, team_id, user_id): + team = self.get_team(team_id) + if team is None: + return False + + # Get user's org + user = self.get_user(user_id) + if user is None or user.org_id != team.org_id: + return False # Different orgs + + # Now check permissions + ... +``` + +--- + +### 3. ✅ `org_is_accessible_to_user` - SECURE + +**Query Pattern**: +```python +session.query(User).filter( + User.uuid == user_id, + User.org_id == org_id, + User.is_del == 0 +) +``` + +**Security Assessment**: ✅ SAFE +- Directly validates user belongs to org +- Checks soft delete flag +- No bypass possible + +--- + +### 4. ✅ `user_and_team_in_same_org` - SECURE + +**Query Pattern**: +```python +# Step 1: Get team +team = query(Team).filter(Team.uuid == team_id, Team.is_del == 0) + +# Step 2: Verify team in target org +if team.org_id != target_org_id: + return False + +# Step 3: Verify user in team's org +user = query(User).filter( + User.uuid == user_id, + User.org_id == team.org_id, # ← Key security check + User.is_del == 0 +) +``` + +**Security Assessment**: ✅ SAFE +- Transitive check ensures: `user.org_id == team.org_id == target_org_id` +- Cannot bypass with cross-org IDs +- Properly validates all relationships + +--- + +### 5. ⚠️ `experiment_is_accessible_to_user` - MISSING ORG CHECK + +**Query Pattern**: +```python +# Step 1: Get experiment (no org validation!) +exp = session.query(Experiment).filter( + Experiment.uuid == experiment_id, + Experiment.is_del == 0 +).first() + +# Step 2: Check if super admin in exp's org +if self.user_is_super_admin_in_org(user_id, exp.org_id): + return True + +# Step 3: Check team membership +membership = session.query(TeamMember).filter( + TeamMember.team_id == exp.team_id, + TeamMember.user_id == user_id, +).first() +``` + +**Potential Vulnerability**: +The team membership check does NOT verify the user is in the same org! + +**Attack Scenario**: +``` +Org A: + - User Alice (user_id: alice_id) + - Team X (team_id: team_x) + +Org B: + - User Bob (user_id: bob_id) + - Team Y (team_id: team_y) + - Experiment Z (exp_id: exp_z, team_id: team_y) + +Attack: +1. Bob creates TeamMember(user_id=alice_id, team_id=team_y) +2. Alice calls get_experiment(exp_z) +3. Check: user_is_super_admin_in_org(alice_id, org_b) → False ✅ +4. Check: TeamMember where team_id=team_y AND user_id=alice_id → FOUND! ❌ +5. Alice can access Org B's experiment! +``` + +**Root Cause**: `TeamMember` table doesn't enforce org consistency in the query! + +**Critical Check Required**: Does `TeamMember` table have an `org_id` column? + +Let me check: + +--- + +### 6. Similar Issues in Other Methods + +The same pattern appears in: +- `run_is_accessible_to_user` (line 1415) +- `agent_is_accessible_to_user` (line 1438) +- `session_is_accessible_to_user` (line 1467) +- `dataset_is_accessible_to_user` (line 1496) + +All use the same vulnerable pattern: +```python +membership = session.query(TeamMember).filter( + TeamMember.team_id == resource.team_id, + TeamMember.user_id == user_id, + # ← MISSING: TeamMember.org_id == resource.org_id +).first() +``` + +--- + +## 🎯 Recommended Fixes + +### Fix 1: Verify TeamMember Schema + +Check if `TeamMember` table has `org_id` column: +```python +class TeamMember(Base): + user_id = Column(...) + team_id = Column(...) + org_id = Column(...) # ← Does this exist? +``` + +**If YES**: Update all queries to include org check: +```python +membership = session.query(TeamMember).filter( + TeamMember.team_id == resource.team_id, + TeamMember.user_id == user_id, + TeamMember.org_id == resource.org_id, # ← ADD THIS +).first() +``` + +**If NO**: Add foreign key constraint or join with User table: +```python +membership = ( + session.query(TeamMember) + .join(User, User.uuid == TeamMember.user_id) + .filter( + TeamMember.team_id == resource.team_id, + TeamMember.user_id == user_id, + User.org_id == resource.org_id, # ← Verify via User + ) + .first() +) +``` + +### Fix 2: Critical Bug in dataset_is_accessible_to_user + +```python +# Line 1502 - Change from: +.filter(Dataset.uuid == dataset_id, Agent.is_del == 0) + +# To: +.filter(Dataset.uuid == dataset_id, Dataset.is_del == 0) +``` + +### Fix 3: Add Integration Tests + +Test cross-org attack scenarios: +1. User in Org A tries to access resource in Org B +2. Malicious admin adds cross-org team membership +3. Verify all `*_is_accessible_to_user` methods reject + +--- + +## 🔒 Security Best Practices Violations + +1. **Lack of Defense in Depth**: Single point of failure if TeamMember table is corrupted +2. **Missing Org Validation**: Many methods don't validate org_id in JOIN queries +3. **Inconsistent Patterns**: `org_is_accessible_to_user` validates org directly, but others rely on team membership +4. **No Audit Logging**: Access control checks don't log failed attempts + +--- + +## ✅ What IS Secure + +1. Super admin checks properly validate org_id +2. User-to-org relationships are validated +3. Soft delete flags are consistently checked +4. UUID-based lookups prevent SQL injection +5. `user_and_team_in_same_org` has correct transitive validation + +--- + +## 📊 Risk Assessment + +| Vulnerability | Severity | Exploitable? | Fix Priority | +|---------------|----------|--------------|--------------| +| dataset_is_accessible bug | HIGH | YES | CRITICAL | +| Missing org check in TeamMember queries | HIGH | Depends on schema | HIGH | +| Inefficient team lookup | LOW | NO | MEDIUM | +| No audit logging | MEDIUM | N/A | LOW | + +--- + +## 🎬 Action Items + +1. **IMMEDIATE**: Fix `dataset_is_accessible_to_user` bug (line 1502) +2. **URGENT**: Verify TeamMember table schema for org_id column +3. **HIGH**: Add org_id validation to all TeamMember queries if column exists +4. **MEDIUM**: Add integration tests for cross-org attacks +5. **LOW**: Add audit logging for failed access attempts + +--- + +## Next Steps + +Need to check: +1. TeamMember table schema definition +2. Whether cross-org team memberships are possible +3. Database constraints preventing invalid memberships diff --git a/alphatrion/server/graphql/resolvers.py b/alphatrion/server/graphql/resolvers.py index 2f8fe75..5078d93 100644 --- a/alphatrion/server/graphql/resolvers.py +++ b/alphatrion/server/graphql/resolvers.py @@ -78,9 +78,7 @@ def get_team(info: Info[GraphQLContext, None], id: strawberry.ID) -> Team | None user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -206,9 +204,7 @@ def list_experiments( ) -> list[Experiment]: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -352,9 +348,7 @@ def list_agents( user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -517,9 +511,7 @@ def list_runs_by_session_id( def total_agents(info: Info[GraphQLContext, None], team_id: strawberry.ID) -> int: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -530,9 +522,7 @@ def total_agents(info: Info[GraphQLContext, None], team_id: strawberry.ID) -> in def total_sessions(info: Info[GraphQLContext, None], team_id: strawberry.ID) -> int: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -596,9 +586,7 @@ def total_experiments( ) -> int: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -609,9 +597,7 @@ def total_experiments( def total_runs(info: Info[GraphQLContext, None], team_id: strawberry.ID) -> int: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -622,9 +608,7 @@ def total_runs(info: Info[GraphQLContext, None], team_id: strawberry.ID) -> int: def total_datasets(info: Info[GraphQLContext, None], team_id: strawberry.ID) -> int: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -648,9 +632,7 @@ def aggregate_team_usage( org_id = uuid.UUID(info.context.org_id) user_id = uuid.UUID(info.context.user_id) metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=org_id - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -831,9 +813,7 @@ def aggregate_model_distributions( org_id = uuid.UUID(ctx.org_id) user_id = uuid.UUID(ctx.user_id) metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=org_id - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -856,9 +836,7 @@ def list_exps_by_timeframe( ) -> list[Experiment]: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -919,9 +897,7 @@ async def list_artifact_tags( """List tags for a repository.""" user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -940,9 +916,7 @@ async def list_artifact_files( """List files in an artifact without loading content.""" user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -995,9 +969,7 @@ async def get_artifact_content( """Get artifact content from registry.""" user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -1091,7 +1063,6 @@ async def get_artifact_download_urls( if not metadb.team_is_accessible_to_user( team_id=dataset.team_id, user_id=uuid.UUID(user_id), - org_id=uuid.UUID(info.context.org_id), ): raise RuntimeError( "Not allowed to access dataset that user does not belong to" @@ -1515,9 +1486,7 @@ def get_daily_cost_usage( org_id = uuid.UUID(ctx.org_id) user_id = uuid.UUID(ctx.user_id) metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=org_id - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): return [] try: @@ -1593,9 +1562,7 @@ def list_datasets( ) -> list[Dataset]: user_id = info.context.user_id metadb = runtime.storage_runtime().metadb - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=uuid.UUID(info.context.org_id) - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to access team that user does not belong to" ) @@ -1838,6 +1805,13 @@ def add_user_to_team( ): raise RuntimeError("Only super admin can add users to teams") + if not metadb.user_and_team_in_same_org( + user_id=uuid.UUID(input.user_id), + team_id=uuid.UUID(input.team_id), + target_org_id=uuid.UUID(info.context.org_id), + ): + raise RuntimeError("User and team must belong to the same organization") + user_id = uuid.UUID(input.user_id) team_id = uuid.UUID(input.team_id) @@ -1856,7 +1830,6 @@ def remove_user_from_team( if not metadb.team_is_accessible_to_user( team_id=team_id, user_id=uuid.UUID(info.context.user_id), - org_id=uuid.UUID(info.context.org_id), ): raise RuntimeError( "Not allowed to modify team that user does not belong to" @@ -1878,9 +1851,7 @@ def create_experiment( metadb = runtime.storage_runtime().metadb # Verify user has access to the team - if not metadb.team_is_accessible_to_user( - team_id=team_id, user_id=user_id, org_id=org_id - ): + if not metadb.team_is_accessible_to_user(team_id=team_id, user_id=user_id): raise RuntimeError( "Not allowed to create experiments in team that user does not belong to" ) @@ -1936,7 +1907,6 @@ def update_experiment( """Update an existing experiment.""" user_id = uuid.UUID(info.context.user_id) - org_id = uuid.UUID(info.context.org_id) experiment_id = uuid.UUID(input.id) metadb = runtime.storage_runtime().metadb @@ -1946,9 +1916,7 @@ def update_experiment( if not exp: raise RuntimeError(f"Experiment with id '{input.id}' not found") - if not metadb.team_is_accessible_to_user( - team_id=exp.team_id, user_id=user_id, org_id=org_id - ): + if not metadb.team_is_accessible_to_user(team_id=exp.team_id, user_id=user_id): raise RuntimeError( "Not allowed to update experiment in team that user does not belong to" ) @@ -2066,7 +2034,6 @@ def abort_experiment( Only works if the experiment is in PENDING status.""" user_id = uuid.UUID(info.context.user_id) - org_id = uuid.UUID(info.context.org_id) experiment_id_uuid = uuid.UUID(experiment_id) metadb = runtime.storage_runtime().metadb @@ -2076,9 +2043,7 @@ def abort_experiment( if not exp: raise RuntimeError(f"Experiment with id '{experiment_id}' not found") - if not metadb.team_is_accessible_to_user( - team_id=exp.team_id, user_id=user_id, org_id=org_id - ): + if not metadb.team_is_accessible_to_user(team_id=exp.team_id, user_id=user_id): raise RuntimeError( "Not allowed to update experiment in team that user does not belong to" ) diff --git a/alphatrion/storage/sqlstore.py b/alphatrion/storage/sqlstore.py index eac71df..890b919 100644 --- a/alphatrion/storage/sqlstore.py +++ b/alphatrion/storage/sqlstore.py @@ -1324,12 +1324,17 @@ def user_is_super_admin_in_org(self, user_id: uuid.UUID, org_id: uuid.UUID) -> b return admin_membership is not None def team_is_accessible_to_user( - self, team_id: uuid.UUID, user_id: uuid.UUID, org_id: uuid.UUID + self, team_id: uuid.UUID, user_id: uuid.UUID ) -> bool: session = self._session() + team = self.get_team(team_id) + if team is None: + session.close() + return False + # Check if user is super admin - if self.user_is_super_admin_in_org(user_id, org_id): + if self.user_is_super_admin_in_org(user_id, team.org_id): session.close() return True @@ -1355,6 +1360,29 @@ def org_is_accessible_to_user(self, org_id: uuid.UUID, user_id: uuid.UUID) -> bo session.close() return user is not None + def user_and_team_in_same_org( + self, user_id: uuid.UUID, team_id: uuid.UUID, target_org_id: uuid.UUID + ) -> bool: + session = self._session() + team = ( + session.query(Team).filter(Team.uuid == team_id, Team.is_del == 0).first() + ) + if team is None: + session.close() + return False + + if team.org_id != target_org_id: + session.close() + return False + + user = ( + session.query(User) + .filter(User.uuid == user_id, User.org_id == team.org_id, User.is_del == 0) + .first() + ) + session.close() + return user is not None + def experiment_is_accessible_to_user( self, experiment_id: uuid.UUID, user_id: uuid.UUID ) -> bool: @@ -1471,7 +1499,7 @@ def dataset_is_accessible_to_user( session = self._session() dst = ( session.query(Dataset) - .filter(Dataset.uuid == dataset_id, Agent.is_del == 0) + .filter(Dataset.uuid == dataset_id, Dataset.is_del == 0) .first() ) if dst is None: diff --git a/tests/integration/server/test_graphql_mutation.py b/tests/integration/server/test_graphql_mutation.py index a3debc5..86f1188 100644 --- a/tests/integration/server/test_graphql_mutation.py +++ b/tests/integration/server/test_graphql_mutation.py @@ -326,7 +326,9 @@ def test_add_user_to_team_with_invalid_team( user_id=test_user_id, ) assert response.errors is not None - assert "not found" in str(response.errors[0]) + # When team doesn't exist, user_and_team_in_same_org returns False + # which triggers "must belong to the same organization" error + assert "same organization" in str(response.errors[0]).lower() def test_add_user_to_team_with_invalid_user( @@ -355,7 +357,9 @@ def test_add_user_to_team_with_invalid_user( user_id=test_user_id, ) assert response.errors is not None - assert "not found" in str(response.errors[0]) + # When user doesn't exist, user_and_team_in_same_org returns False + # which triggers "must belong to the same organization" error + assert "same organization" in str(response.errors[0]).lower() def test_user_workflow(execute_graphql, test_org_id, test_user_id, test_team_id): diff --git a/tests/integration/test_s3_backend.py b/tests/integration/test_s3_backend.py index 7346f00..51d32e4 100644 --- a/tests/integration/test_s3_backend.py +++ b/tests/integration/test_s3_backend.py @@ -46,7 +46,9 @@ def s3_env_vars(): else: os.environ[key] = value - storage_runtime_module.__STORAGE_RUNTIME__ = None # Reset again to clear any cached runtime + storage_runtime_module.__STORAGE_RUNTIME__ = ( + None # Reset again to clear any cached runtime + ) @pytest.fixture diff --git a/tests/unit/storage/test_sql.py b/tests/unit/storage/test_sql.py index 62e9af9..805029f 100644 --- a/tests/unit/storage/test_sql.py +++ b/tests/unit/storage/test_sql.py @@ -164,3 +164,79 @@ def test_create_metrics_batch(db): assert metric_dict["loss"] == 0.1 assert metric_dict["precision"] == 0.92 assert metric_dict["recall"] == 0.88 + + +def test_user_and_team_in_same_org_success(db): + """Test that user and team in same org returns True""" + org_id = uuid.uuid4() + + # Create team + team_id = db.create_team(org_id=org_id, name="Test Team") + + # Create user in same org + user_id = db.create_user( + org_id=org_id, name="test_user", email="user@example.com", team_id=team_id + ) + + # Verify user and team are in same org + assert db.user_and_team_in_same_org(user_id, team_id, org_id) is True + + +def test_user_and_team_in_same_org_different_orgs(db): + """Test that user and team in different orgs returns False""" + org1_id = uuid.uuid4() + org2_id = uuid.uuid4() + + # Create team in org1 + team_id = db.create_team(org_id=org1_id, name="Team in Org1") + + # Create user in org2 (different org) + user_id = db.create_user(org_id=org2_id, name="test_user", email="user@example.com") + + # Verify user and team are NOT in same org + assert db.user_and_team_in_same_org(user_id, team_id, org1_id) is False + + +def test_user_and_team_in_same_org_nonexistent_team(db): + """Test that nonexistent team returns False""" + org_id = uuid.uuid4() + + # Create user + user_id = db.create_user(org_id=org_id, name="test_user", email="user@example.com") + + # Use nonexistent team ID + nonexistent_team_id = uuid.uuid4() + + # Verify returns False for nonexistent team + assert db.user_and_team_in_same_org(user_id, nonexistent_team_id, org_id) is False + + +def test_user_and_team_in_same_org_nonexistent_user(db): + """Test that nonexistent user returns False""" + org_id = uuid.uuid4() + + # Create team + team_id = db.create_team(org_id=org_id, name="Test Team") + + # Use nonexistent user ID + nonexistent_user_id = uuid.uuid4() + + # Verify returns False for nonexistent user + assert db.user_and_team_in_same_org(nonexistent_user_id, team_id, org_id) is False + + +def test_user_and_team_in_same_org_wrong_target_org(db): + """Test that wrong target org returns False""" + org_id = uuid.uuid4() + wrong_org_id = uuid.uuid4() + + # Create team in org_id + team_id = db.create_team(org_id=org_id, name="Test Team") + + # Create user in same org + user_id = db.create_user( + org_id=org_id, name="test_user", email="user@example.com", team_id=team_id + ) + + # Verify returns False when checking against wrong target org + assert db.user_and_team_in_same_org(user_id, team_id, wrong_org_id) is False From 50a8aae8d4977b94fb8fe27f8e81834f3eec59e9 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Tue, 26 May 2026 18:21:36 +0100 Subject: [PATCH 3/3] remove markdown Signed-off-by: kerthcet --- SECURITY_ANALYSIS.md | 330 ------------------------------------------- 1 file changed, 330 deletions(-) delete mode 100644 SECURITY_ANALYSIS.md diff --git a/SECURITY_ANALYSIS.md b/SECURITY_ANALYSIS.md deleted file mode 100644 index adc540e..0000000 --- a/SECURITY_ANALYSIS.md +++ /dev/null @@ -1,330 +0,0 @@ -# Security Analysis: SQL Access Control Vulnerabilities - -## Executive Summary - -Analysis Date: 2026-05-26 -Analyzed File: `alphatrion/storage/sqlstore.py` -Status: **1 CRITICAL BUG FOUND** + Several potential vulnerabilities identified - ---- - -## 🚨 CRITICAL: Bug in `dataset_is_accessible_to_user` - -### Location -File: `alphatrion/storage/sqlstore.py` -Lines: 1496-1523 - -### The Bug -```python -def dataset_is_accessible_to_user( - self, dataset_id: uuid.UUID, user_id: uuid.UUID -) -> bool: - session = self._session() - dst = ( - session.query(Dataset) - .filter(Dataset.uuid == dataset_id, Agent.is_del == 0) # ← BUG HERE! - .first() - ) -``` - -**Problem**: The query filters by `Agent.is_del == 0` instead of `Dataset.is_del == 0` - -**Impact**: -- This will cause a SQL error or unexpected behavior -- Deleted datasets may still be accessible -- Query might fail completely - -**Fix**: -```python -.filter(Dataset.uuid == dataset_id, Dataset.is_del == 0) # Correct -``` - ---- - -## 🔍 Security Vulnerability Analysis - -### 1. ✅ `user_is_super_admin_in_org` - SECURE - -**Query Pattern**: -```python -session.query(TeamMember) - .filter( - TeamMember.user_id == user_id, - TeamMember.org_id == org_id, - TeamMember.role == MemberRole.SUPER_ADMIN, - ) -``` - -**Security Assessment**: ✅ SAFE -- Checks all three required fields -- No cross-org privilege escalation possible -- Properly validates org membership - ---- - -### 2. ⚠️ `team_is_accessible_to_user` - POTENTIAL ISSUE - -**Query Pattern**: -```python -# First get team (no org check!) -team = self.get_team(team_id) - -# Then check super admin -if self.user_is_super_admin_in_org(user_id, team.org_id): - return True - -# Or check team membership -membership = session.query(TeamMember).filter( - TeamMember.team_id == team_id, - TeamMember.user_id == user_id, -).first() -``` - -**Potential Vulnerability**: -```python -def get_team(self, team_id): - # Returns team WITHOUT checking if user's org matches! - return session.query(Team).filter(Team.uuid == team_id).first() -``` - -**Attack Scenario**: -1. Attacker is user in Org A -2. Target team is in Org B -3. `get_team()` returns Team B (no org filter!) -4. Super admin check: `user_is_super_admin_in_org(user_A, org_B)` → False ✅ -5. Team membership check: `TeamMember.team_id == team_B AND user_id == user_A` → False ✅ - -**Conclusion**: ✅ SAFE - Both checks will fail, but inefficient - -**Recommendation**: Add org validation early: -```python -def team_is_accessible_to_user(self, team_id, user_id): - team = self.get_team(team_id) - if team is None: - return False - - # Get user's org - user = self.get_user(user_id) - if user is None or user.org_id != team.org_id: - return False # Different orgs - - # Now check permissions - ... -``` - ---- - -### 3. ✅ `org_is_accessible_to_user` - SECURE - -**Query Pattern**: -```python -session.query(User).filter( - User.uuid == user_id, - User.org_id == org_id, - User.is_del == 0 -) -``` - -**Security Assessment**: ✅ SAFE -- Directly validates user belongs to org -- Checks soft delete flag -- No bypass possible - ---- - -### 4. ✅ `user_and_team_in_same_org` - SECURE - -**Query Pattern**: -```python -# Step 1: Get team -team = query(Team).filter(Team.uuid == team_id, Team.is_del == 0) - -# Step 2: Verify team in target org -if team.org_id != target_org_id: - return False - -# Step 3: Verify user in team's org -user = query(User).filter( - User.uuid == user_id, - User.org_id == team.org_id, # ← Key security check - User.is_del == 0 -) -``` - -**Security Assessment**: ✅ SAFE -- Transitive check ensures: `user.org_id == team.org_id == target_org_id` -- Cannot bypass with cross-org IDs -- Properly validates all relationships - ---- - -### 5. ⚠️ `experiment_is_accessible_to_user` - MISSING ORG CHECK - -**Query Pattern**: -```python -# Step 1: Get experiment (no org validation!) -exp = session.query(Experiment).filter( - Experiment.uuid == experiment_id, - Experiment.is_del == 0 -).first() - -# Step 2: Check if super admin in exp's org -if self.user_is_super_admin_in_org(user_id, exp.org_id): - return True - -# Step 3: Check team membership -membership = session.query(TeamMember).filter( - TeamMember.team_id == exp.team_id, - TeamMember.user_id == user_id, -).first() -``` - -**Potential Vulnerability**: -The team membership check does NOT verify the user is in the same org! - -**Attack Scenario**: -``` -Org A: - - User Alice (user_id: alice_id) - - Team X (team_id: team_x) - -Org B: - - User Bob (user_id: bob_id) - - Team Y (team_id: team_y) - - Experiment Z (exp_id: exp_z, team_id: team_y) - -Attack: -1. Bob creates TeamMember(user_id=alice_id, team_id=team_y) -2. Alice calls get_experiment(exp_z) -3. Check: user_is_super_admin_in_org(alice_id, org_b) → False ✅ -4. Check: TeamMember where team_id=team_y AND user_id=alice_id → FOUND! ❌ -5. Alice can access Org B's experiment! -``` - -**Root Cause**: `TeamMember` table doesn't enforce org consistency in the query! - -**Critical Check Required**: Does `TeamMember` table have an `org_id` column? - -Let me check: - ---- - -### 6. Similar Issues in Other Methods - -The same pattern appears in: -- `run_is_accessible_to_user` (line 1415) -- `agent_is_accessible_to_user` (line 1438) -- `session_is_accessible_to_user` (line 1467) -- `dataset_is_accessible_to_user` (line 1496) - -All use the same vulnerable pattern: -```python -membership = session.query(TeamMember).filter( - TeamMember.team_id == resource.team_id, - TeamMember.user_id == user_id, - # ← MISSING: TeamMember.org_id == resource.org_id -).first() -``` - ---- - -## 🎯 Recommended Fixes - -### Fix 1: Verify TeamMember Schema - -Check if `TeamMember` table has `org_id` column: -```python -class TeamMember(Base): - user_id = Column(...) - team_id = Column(...) - org_id = Column(...) # ← Does this exist? -``` - -**If YES**: Update all queries to include org check: -```python -membership = session.query(TeamMember).filter( - TeamMember.team_id == resource.team_id, - TeamMember.user_id == user_id, - TeamMember.org_id == resource.org_id, # ← ADD THIS -).first() -``` - -**If NO**: Add foreign key constraint or join with User table: -```python -membership = ( - session.query(TeamMember) - .join(User, User.uuid == TeamMember.user_id) - .filter( - TeamMember.team_id == resource.team_id, - TeamMember.user_id == user_id, - User.org_id == resource.org_id, # ← Verify via User - ) - .first() -) -``` - -### Fix 2: Critical Bug in dataset_is_accessible_to_user - -```python -# Line 1502 - Change from: -.filter(Dataset.uuid == dataset_id, Agent.is_del == 0) - -# To: -.filter(Dataset.uuid == dataset_id, Dataset.is_del == 0) -``` - -### Fix 3: Add Integration Tests - -Test cross-org attack scenarios: -1. User in Org A tries to access resource in Org B -2. Malicious admin adds cross-org team membership -3. Verify all `*_is_accessible_to_user` methods reject - ---- - -## 🔒 Security Best Practices Violations - -1. **Lack of Defense in Depth**: Single point of failure if TeamMember table is corrupted -2. **Missing Org Validation**: Many methods don't validate org_id in JOIN queries -3. **Inconsistent Patterns**: `org_is_accessible_to_user` validates org directly, but others rely on team membership -4. **No Audit Logging**: Access control checks don't log failed attempts - ---- - -## ✅ What IS Secure - -1. Super admin checks properly validate org_id -2. User-to-org relationships are validated -3. Soft delete flags are consistently checked -4. UUID-based lookups prevent SQL injection -5. `user_and_team_in_same_org` has correct transitive validation - ---- - -## 📊 Risk Assessment - -| Vulnerability | Severity | Exploitable? | Fix Priority | -|---------------|----------|--------------|--------------| -| dataset_is_accessible bug | HIGH | YES | CRITICAL | -| Missing org check in TeamMember queries | HIGH | Depends on schema | HIGH | -| Inefficient team lookup | LOW | NO | MEDIUM | -| No audit logging | MEDIUM | N/A | LOW | - ---- - -## 🎬 Action Items - -1. **IMMEDIATE**: Fix `dataset_is_accessible_to_user` bug (line 1502) -2. **URGENT**: Verify TeamMember table schema for org_id column -3. **HIGH**: Add org_id validation to all TeamMember queries if column exists -4. **MEDIUM**: Add integration tests for cross-org attacks -5. **LOW**: Add audit logging for failed access attempts - ---- - -## Next Steps - -Need to check: -1. TeamMember table schema definition -2. Whether cross-org team memberships are possible -3. Database constraints preventing invalid memberships