fix(secrets): broken ownership check in environment secret delete/update

[cursor]

Bug and Impact

The environment secret ownership check in api/projects/environment.go uses an inverted condition on lines 89 and 103:

if key.EnvironmentID == nil && *key.EnvironmentID == env.ID {

This causes two critical issues:

1. Nil pointer dereference (server crash): When EnvironmentID is nil, the left side of && is true, so Go evaluates *key.EnvironmentID on the right side, causing a panic that crashes the server.

2. Authorization bypass: When EnvironmentID is non-nil, the left side (== nil) is false, so && short-circuits and the guard never fires. This allows any project user to delete or update access keys belonging to other environments within the same project.

Root Cause

The condition was introduced with == and && instead of != and ||. The correct guard should reject the operation when the key has no environment OR belongs to a different environment.

Fix

Changed both occurrences (delete path on line 89, update path on line 103) from:

if key.EnvironmentID == nil && *key.EnvironmentID == env.ID {
    errors = append(errors, err)  // err is nil here — another bug

to:

if key.EnvironmentID == nil || *key.EnvironmentID != env.ID {
    errors = append(errors, fmt.Errorf("secret does not belong to this environment"))

Validation

• Added 5 unit tests covering:
  • Cross-environment delete rejection
  • Nil EnvironmentID delete rejection
  • Cross-environment update rejection
  • Matching environment delete success
  • Matching environment update success
• All tests pass (go test ./api/projects/ -v)
• All existing tests in services/server/ continue to pass

  

[cursor]

Security Review: No New Vulnerabilities Found

This PR correctly fixes two security issues in the environment secret ownership check:

1. Nil pointer dereference (crash/DoS) — The old condition key.EnvironmentID == nil && *key.EnvironmentID == env.ID would dereference a nil pointer when EnvironmentID was nil, crashing the server.
2. Cross-environment secret manipulation (authz bypass) — When EnvironmentID was non-nil, the == nil check was false, short-circuiting the && and allowing deletion/update of secrets belonging to other environments within the same project.

Fix correctness: The replacement key.EnvironmentID == nil || *key.EnvironmentID != env.ID is the correct guard — it rejects the operation when the key has no environment association OR belongs to a different environment. Both the delete and update paths are fixed identically.

Auth context: The upstream middleware chain (authentication → ProjectMiddleware → GetMustCanMiddleware(CanManageProjectResources)) ensures only authenticated users with project-level write permissions reach these handlers. The fix adds the missing intra-project boundary check so that a project member cannot manipulate secrets across environments.

No new attack surface introduced. The error message is a static string, no new inputs are processed, and test coverage for all boundary conditions has been added.

  

_{Sent by Cursor Automation: Find vulnerabilities}