Enforced provider and projectID in db call by DharunMR · Pull Request #6303 · mindersec/minder

DharunMR · 2026-04-08T08:22:10Z

Summary

This PR implements strict security boundaries for entity retrieval and property management by enforcing the Security Triple (EntityID, ProjectID, ProviderID) across the database and service layers.

Previously, certain database calls and service methods relied on incomplete identifiers, which theoretically could allow for cross-provider or cross-project data access if an ID was known. This refactor ensures that every entity-related operation is strictly bounded by both the project and the provider that owns the entity.

Key Changes:

Database Layer (SQL)
Enforced Constraints: Updated GetEntityByID, GetEntityByName, and DeleteEntity queries in entities.sql to require project_id and provider_id. Property Security: Updated property-related queries (GetProperty, GetAllPropertiesForEntity, DeleteProperty) to utilize JOIN or USING clauses against entity_instances, ensuring property operations cannot bypass entity ownership boundaries. Optimized Flush Cache: Updated ListFlushCache to join with entity_instances, allowing background workers to retrieve the provider_id directly from the database without extra Go-side lookups.
Service Layer (Go)
Property Service: Refactored EntityWithPropertiesByID and related methods to accept and validate the full Security Triple. Repository Service: Updated GetRepositoryById, GetRepositoryByName, and Delete methods to pass mandatory security parameters. EEA & Background Workers: Updated the Event Aggregator and reconcilers to use the ProviderID now provided by the enhanced flush cache queries.
API & Middleware
Controlplane: Updated API handlers (Artifacts, Repositories, Profiles) to translate provider names from the request context into UUIDs via providerStore.GetByName to satisfy the new database constraints.
Fixes Enforce provider and project ID in GetEntityByID db call #4417
Fixes Enforce provider and project ID in EntityWithProperties in the properties service #4419

Testing

Test Suite
Ran go test ./... (100% passing) to verify that the embedded database correctly respects the new project_id slices and that all handlers propagate context properly.
Manual Verification
- Spun up the local environment using make run-docker.
- Executed a manual SQL injection test via docker exec -it postgres_container psql.
- Created a mock project, provider, and entity instance.
- Verified that querying the exact entity UUID using the authorized project_id array returned the entity (1 row).
- Verified that simulating a cross-tenant IDOR attack by querying the exact entity UUID with an unauthorized/different project_id array correctly returned 0 rows (Access Denied).

coveralls · 2026-04-08T08:32:45Z

Coverage is 60.204% — DharunMR:Enforce-provider-and-projectID into mindersec:main. No base build found for mindersec:main.

evankanderson

It looks like you may also have a merge conflict, but this looks good other than a few small comments -- I'd like to avoid adding remote (serial) lookups in loop contexts -- I didn't look in enough detail to know for sure here if that was happening..

evankanderson

I think you just force-pushed to the wrong branch. :-/

DharunMR · 2026-04-10T07:02:56Z

I think you just force-pushed to the wrong branch. :-/

Unfortunatly! now solved it

DharunMR · 2026-04-10T14:44:25Z

@evankanderson i've verified the tests pass locally ,but fails ci

evankanderson

One small concern about duplicating getAuthorizedProjects logic twice, and some code that I think is functionally dead and should probably be removed.

evankanderson · 2026-04-10T22:11:28Z

 	}

-	ewp, err := ps.getEntityWithProperties(ctx, *ent, opts)
+	projects, err := ps.getAuthorizedProjects(ctx, ent.ProjectID)


Verified that this is only called by getEntityInner, which is only called by the strategies in handlers/strategies/entity for handling updates of resources which were auto-created due to parent resources.

(In response to the question whether this was called in a loop)

I had actually removed getAuthorizedProjects in my latest commit

evankanderson · 2026-04-20T17:56:17Z

(This has a merge conflict again, sorry!)

DharunMR · 2026-04-21T12:13:34Z

(This has a merge conflict again, sorry!)

resolved it now

Signed-off-by: DharunMR <maddharun56@gmail.com>

evankanderson

This PR is pretty "big" (hard for me to fit in my head) -- is there a good way to break it into smaller chunks?

evankanderson · 2026-04-28T19:36:49Z

 ) AS exists;

-- GetEntitiesByProvider retrieves all entities of a given provider.
+-- GetEntitiesByProvider retrieves all entities of a given provider scoped by project hierarchy.


Providers are already scoped to a project (and its children, though we don't have all the hierarchy decisions ironed out yet).

evankanderson · 2026-04-28T19:40:44Z

 -- name: FlushCache :one
 DELETE FROM flush_cache


I don't believe there's a user-controlled way to affect the entity cache, so it's not clear we need to enforce project checks here (and it may add cost).

evankanderson · 2026-04-28T19:43:02Z


 	// Fetch the entity with properties
-	ewp, err := s.props.EntityWithPropertiesByID(ctx, entities[0].ID, nil)
+	ewp, err := s.props.EntityWithPropertiesByID(ctx, entities[0].ID, projectID, provider.ID, nil)


FWIW, it looks like we've already constrained the entity to the current project and provider on line 78-85.

evankanderson · 2026-04-28T19:44:03Z

+
 	// Fetch artifact entity
-	ewp, err := s.props.EntityWithPropertiesByID(ctx, parsedArtifactID, nil)
+	ewp, err := s.props.EntityWithPropertiesByID(ctx, parsedArtifactID, projectID, provider.ID, nil)


It looks like we're adding some database queries to replace the lines 155-158. I'm not sure how I feel about that.

evankanderson · 2026-04-28T19:46:25Z


 	// Call service to get entity
-	entity, err := s.entityService.GetEntityByID(ctx, entityID, projectID)
+	entity, err := s.entityService.GetEntityByID(ctx, entityID, projectID, provider.ID)


It looks like we only check the provider here because of the changes to the DB queries? We're mostly concerned about the ability of a user in one project to "peek" into another project that they don't have access to -- we assume that users in a given project have equivalent permissions on all the entities in that project (though I supposed this will also now return an error if you try to look up an entity from provider A, but pass provider B's name in the context).

evankanderson · 2026-04-28T19:46:44Z

 	}

 	// Telemetry logging
+	logger.BusinessRecord(ctx).Provider = providerName // Added provider telemetry!


This comment is strange.

evankanderson · 2026-04-28T19:51:23Z

 	var topic string

-	msg, err = getRepositoryReconciliationMessage(ctx, s.store, entity.GetId(), entityCtx)
+	msg, err = getRepositoryReconciliationMessage(ctx, s.store, entity.GetId(), entityCtx, dbProvider.ID)


It looks like we're again adding some database lookups to protect against the same limit as 101-105 in the old code. I think this might be an improvement on making accidental errors, but the cost feels higher than I'd like.

Is there a way that we can split the methods into "safe to be called with user values" and "only to be called after entity validation"? I suspect we don't have a good way right now because we tend to vend our internal database IDs directly through the API to end users. 🤦

evankanderson · 2026-04-28T19:54:05Z

+	if len(repoEnts) == 0 {
+		return nil, nil
+	}
+
+	entityIDs := make([]uuid.UUID, len(repoEnts))
+	for i, ent := range repoEnts {
+		entityIDs[i] = ent.ID
+	}
+
+	dbProps, err := qtx.GetPropertiesForEntities(ctx, db.GetPropertiesForEntitiesParams{
+		EntityIds:  entityIDs,
+		Projects:   []uuid.UUID{projectID},
+		ProviderID: providerID,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("error fetching bulk properties: %w", err)
+	}
+
+	propsMap := make(map[uuid.UUID][]db.Property)
+	for _, p := range dbProps {
+		propsMap[p.EntityID] = append(propsMap[p.EntityID], p)
+	}
+
 	ents = make([]*models.EntityWithProperties, 0, len(repoEnts))
 	for _, ent := range repoEnts {
-		ewp, err := r.propSvc.EntityWithPropertiesByID(ctx, ent.ID,
-			service.CallBuilder().WithStoreOrTransaction(qtx))
+		modelProps, err := models.DbPropsToModel(propsMap[ent.ID])
 		if err != nil {
-			return nil, fmt.Errorf("error fetching properties for repository: %w", err)
-		}
-
-		if err := r.propSvc.RetrieveAllPropertiesForEntity(ctx, ewp, r.providerManager,
-			service.ReadBuilder().WithStoreOrTransaction(qtx).TolerateStaleData()); err != nil {
-			return nil, fmt.Errorf("error fetching properties for repository: %w", err)
+			return nil, fmt.Errorf("failed to convert properties for %s: %w", ent.ID, err)
 		}

-		ents = append(ents, ewp)
+		ents = append(ents, models.NewEntityWithProperties(ent, modelProps))
 	}


This looks like a performance refactor (batching property lookups). That seems handy, but separate from other parts of this PR?

github-actions · 2026-05-29T05:41:43Z

This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days.

DharunMR requested a review from a team as a code owner April 8, 2026 08:22

DharunMR changed the title ~~Enforced provider and project in db call~~ Enforced provider and projectID in db call Apr 8, 2026

evankanderson reviewed Apr 8, 2026

View reviewed changes

Comment thread internal/db/entity_helpers.go Outdated

Comment thread internal/entities/handlers/strategies/entity/refresh_by_id.go Outdated

Comment thread internal/entities/properties/service/helpers.go Outdated

Comment thread internal/entities/service/service.go Outdated

DharunMR force-pushed the Enforce-provider-and-projectID branch 2 times, most recently from 8e99b54 to 5c1b61d Compare April 10, 2026 06:52

evankanderson reviewed Apr 10, 2026

View reviewed changes

Comment thread internal/db/entity_helpers.go Outdated

Comment thread internal/entities/handlers/strategies/entity/refresh_by_id.go Outdated

DharunMR force-pushed the Enforce-provider-and-projectID branch from 5c1b61d to 6933e2f Compare April 10, 2026 07:01

DharunMR force-pushed the Enforce-provider-and-projectID branch 3 times, most recently from 1063a3e to d7067a1 Compare April 10, 2026 13:44

DharunMR requested a review from evankanderson April 10, 2026 15:41

evankanderson reviewed Apr 10, 2026

View reviewed changes

DharunMR force-pushed the Enforce-provider-and-projectID branch 3 times, most recently from 676c0cb to 32fc7d8 Compare April 13, 2026 04:34

DharunMR requested a review from evankanderson April 13, 2026 04:47

DharunMR force-pushed the Enforce-provider-and-projectID branch 3 times, most recently from d655ebe to 2c90a2a Compare April 13, 2026 05:23

DharunMR force-pushed the Enforce-provider-and-projectID branch from 2c90a2a to 6699a36 Compare April 21, 2026 01:59

Enforced provider and project ID

4a67fc4

Signed-off-by: DharunMR <maddharun56@gmail.com>

DharunMR force-pushed the Enforce-provider-and-projectID branch from 6699a36 to 4a67fc4 Compare April 21, 2026 12:47

evankanderson reviewed Apr 28, 2026

View reviewed changes

github-actions Bot added the Stale label May 29, 2026

Conversation

DharunMR commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes:

Testing

Test Suite

Manual Verification

Uh oh!

coveralls commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DharunMR commented Apr 10, 2026

Uh oh!

DharunMR commented Apr 10, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evankanderson commented Apr 20, 2026

Uh oh!

DharunMR commented Apr 21, 2026

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DharunMR commented Apr 8, 2026 •

edited

Loading

coveralls commented Apr 8, 2026 •

edited

Loading