Update webhook security docs and error handling

docs/security-report.md

@@ -1,9 +1,9 @@
 **1. Executive Summary**
-This repo’s remaining main security problems are real and concentrated in authorization, cross-tenant relationship integrity, and operational hardening. The highest-risk remaining issue is that most protected routes check only “has a valid JWT,” not “has the right permission.” The next tier is that cross-tenant user-role links are still possible if foreign UUIDs are known, while forwarded-header trust and Redis fail-open behavior still weaken abuse protection and revocation guarantees.
+This repo’s remaining main security problems are real and concentrated in authorization and operational hardening. The highest-risk remaining issue is that most protected routes check only “has a valid JWT,” not “has the right permission.” The next tier is that forwarded-header trust, Redis fail-open behavior, and weak JWT secret acceptance still weaken abuse protection and revocation guarantees.
 
-Update (2026-03-27): findings 1, 3, 5, and 6 have been fixed in the repo. The remaining findings below are still outstanding unless explicitly marked otherwise.
+Update (2026-03-28): findings 1, 3, 4, 5, and 6 have been fixed in the repo. The remaining findings below are still outstanding unless explicitly marked otherwise.
 
-This report began as a read-only review of the codebase across auth/session logic, authorization/tenant boundaries, and config/deployment. Findings 1, 3, 5, and 6 have since been remediated in the repo. `go test ./...` passed locally; `govulncheck` was not installed, so dependency-CVE verification remains open.
+This report began as a read-only review of the codebase across auth/session logic, authorization/tenant boundaries, and config/deployment. Findings 1, 3, 4, 5, and 6 have since been remediated in the repo. `go test ./...` passed locally; `govulncheck` was not installed, so dependency-CVE verification remains open.
 
 **2. Architecture and Attack Surface**
 - Entry points: public auth routes, `/health`, `/ready`, `/swagger/*`, and JWT-protected `/api/v1/*` in [router.go](/Users/osamamuhammed/uniauth/internal/api/router.go#L76).
@@ -29,7 +29,9 @@ This report began as a read-only review of the codebase across auth/session logi
    Status: Fixed.
    Remediation implemented: handlers now thread caller `org_id` into the user and RBAC service paths, user/role repository lookups and mutations enforce `org_id` in SQL, out-of-org mutations now resolve as `404`, and the related `/users/{id}/roles` and `/roles/{id}/permissions` service paths validate org ownership before mutating. Regression coverage was added in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1) and [rbac_scope_test.go](/Users/osamamuhammed/uniauth/internal/service/rbac_scope_test.go#L1).
 
-4. **Cross-tenant user-role links are possible** — High, High confidence. Affected: [rbac service](/Users/osamamuhammed/uniauth/internal/service/rbac.go#L78), [roles repo](/Users/osamamuhammed/uniauth/internal/repository/postgres/roles.go#L113), [migration](/Users/osamamuhammed/uniauth/migrations/000001_init_schema.up.sql#L91). Evidence: `AssignRoleToUser` inserts `(user_id, role_id)` without checking that both belong to the same org, and the `user_roles` table has no same-org enforcement. Risk: cross-tenant privilege contamination or integrity damage if a victim user UUID and a role UUID are known. Preconditions: attacker knows both IDs. Fix: validate same-org in service logic immediately, then add DB-level enforcement via schema redesign or trigger. Verify with cross-org assignment tests. OWASP: A01 Broken Access Control.
+4. **Cross-tenant user-role links are possible** — High, High confidence. Affected: [rbac service](/Users/osamamuhammed/uniauth/internal/service/rbac.go#L81), [roles repo](/Users/osamamuhammed/uniauth/internal/repository/postgres/roles.go#L121), [migration](/Users/osamamuhammed/uniauth/migrations/000002_user_roles_org_scope.up.sql#L1). Evidence: `AssignRoleToUser` inserted `(user_id, role_id)` without checking that both belonged to the same org, and the `user_roles` table had no same-org enforcement. Risk: cross-tenant privilege contamination or integrity damage if a victim user UUID and a role UUID are known. Preconditions: attacker knows both IDs. Fix: validate same-org in service logic immediately, then add DB-level enforcement via schema redesign or trigger. Verify with cross-org assignment tests. OWASP: A01 Broken Access Control.
+   Status: Fixed.
+   Remediation implemented: the existing org-scoped service prechecks remain in place, `Store.AssignRoleToUser` now persists `org_id` and maps DB-side tenant-integrity violations to `ErrNotFound`, and [000002_user_roles_org_scope.up.sql](/Users/osamamuhammed/uniauth/migrations/000002_user_roles_org_scope.up.sql#L1) adds composite same-org foreign keys while auto-removing any legacy cross-tenant `user_roles` rows during migration. Regression coverage was expanded in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1) and the service-layer `ErrNotFound` guard remains covered in [rbac_scope_test.go](/Users/osamamuhammed/uniauth/internal/service/rbac_scope_test.go#L1).
 
 5. **Password-reset tokens are logged in plaintext when SMTP is unset** — High, High confidence. Affected: [email.go](/Users/osamamuhammed/uniauth/internal/service/email.go#L21), [Helm values](/Users/osamamuhammed/uniauth/helm/uniauth/values.yaml#L12), [k8s configmap](/Users/osamamuhammed/uniauth/k8s/configmap.yaml#L9). Evidence: `SendPasswordReset` prints the live reset token to stdout, and production-oriented manifests default to SMTP unset. Risk: anyone with pod/log aggregation access can take over accounts by replaying reset tokens. Preconditions: SMTP is unset in a real environment and attacker can read logs. Fix: never print tokens; fail closed outside development; if dev mode is needed, log only that a reset was requested. Verify by requesting a reset in production mode with no SMTP and confirming no token reaches stdout/logs. OWASP: A02 Cryptographic Failures / A05 Security Misconfiguration.
    Status: Fixed.
@@ -63,14 +65,15 @@ This report began as a read-only review of the codebase across auth/session logi
 - Short-term hardening 2: replace naive forwarded-header trust with trusted-proxy-aware IP extraction. Complexity: Small-Medium. Regression risk: Medium. Order: 6.
 - Short-term hardening 3: enforce JWT secret quality, sanitize `/ready`, and define Redis degraded-mode behavior. Complexity: Small. Regression risk: Low-Medium. Order: 7.
 - Structural improvement 1: introduce reusable authorization middleware/policy mapping instead of ad hoc handler checks. Complexity: High. Regression risk: Medium.
-- Structural improvement 2: add DB-level same-org enforcement for role assignments. Complexity: High. Regression risk: Medium.
+- Completed structural improvement 2: add DB-level same-org enforcement for role assignments and auto-remove legacy cross-tenant links during migration. Complexity: Medium. Regression risk: Medium.
 - Structural improvement 3: pin CI actions/artifacts/images and add `govulncheck` to CI. Complexity: Small. Regression risk: Low.
 
 **6. Verification Checklist**
 - Add a test proving a refresh token gets `401` on a normal protected route like `/api/v1/users/me`.
 - Add tests proving logout, logout-all, password change, and password reset invalidate both access and refresh credentials in practice.
 - Add negative authz tests for every privileged endpoint using a valid but low-privilege JWT.
 - Implemented: cross-org repository tests now cover user/role lookup and mutation scoping in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1).
+- Implemented: user-role integrity tests now cover same-org assignment success, repository rejection of foreign-org pairs, and raw SQL schema-level enforcement of the same-org invariant in [tenant_scope_test.go](/Users/osamamuhammed/uniauth/internal/repository/postgres/tenant_scope_test.go#L1).
 - Implemented: RBAC service tests now reject foreign-org role assignment and permission assignment in [rbac_scope_test.go](/Users/osamamuhammed/uniauth/internal/service/rbac_scope_test.go#L1).
 - Add tests that spoofed `X-Forwarded-For` does not create separate limiter buckets unless the request came through a trusted proxy.
 - Implemented: webhook validation and delivery tests now reject private/link-local/metadata webhook targets and ensure blocked destinations are not dialed in [webhook_test.go](/Users/osamamuhammed/uniauth/internal/service/webhook_test.go#L1) and [webhooks_test.go](/Users/osamamuhammed/uniauth/internal/api/handlers/webhooks_test.go#L1).
@@ -85,4 +88,4 @@ This report began as a read-only review of the codebase across auth/session logi
 - I did not verify pod/network egress policy. Existing egress controls remain useful defense-in-depth on top of the new webhook validation and delivery guards.
 - I could not verify dependency-level vulns because `govulncheck` is not installed in this workspace.
 
-This report is now a current status snapshot of the repo; the outstanding findings above remain the next recommended remediation targets.
+This report is now a current status snapshot of the repo; 6 issues remain open in the report: findings 2, 7, 8, 9, plus the 2 lower-severity hardening items.

internal/repository/postgres/roles.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/jackc/pgx/v5"
+	"github.com/jackc/pgx/v5/pgconn"
 
 	"github.com/osama1998h/uniauth/internal/domain"
 )
@@ -117,10 +118,15 @@ func (s *Store) ListPermissionsByRole(ctx context.Context, roleID uuid.UUID) ([]
 	return collectPermissions(rows)
 }
 
-func (s *Store) AssignRoleToUser(ctx context.Context, userID, roleID uuid.UUID) error {
+func (s *Store) AssignRoleToUser(ctx context.Context, orgID, userID, roleID uuid.UUID) error {
 	_, err := s.pool.Exec(ctx,
-		`INSERT INTO user_roles (user_id, role_id) VALUES ($1, $2) ON CONFLICT DO NOTHING`, userID, roleID,
+		`INSERT INTO user_roles (org_id, user_id, role_id) VALUES ($1, $2, $3) ON CONFLICT DO NOTHING`,
+		orgID, userID, roleID,
 	)
+	var pgErr *pgconn.PgError
+	if errors.As(err, &pgErr) && pgErr.Code == "23503" {
+		return domain.ErrNotFound
+	}
 	return err
 }
 

internal/repository/postgres/tenant_scope_test.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/jackc/pgx/v5/pgconn"
 
 	"github.com/osama1998h/uniauth/internal/domain"
 	"github.com/osama1998h/uniauth/internal/testutil"
@@ -167,3 +168,58 @@ func TestStoreDeleteRoleReturnsNotFoundForMissingOrgMatch(t *testing.T) {
 		t.Fatalf("expected ErrNotFound for missing role, got %v", err)
 	}
 }
+
+func TestStoreUserRoleTenantIntegrity(t *testing.T) {
+	store := testutil.RequireTestStore(t)
+	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
+	defer cancel()
+
+	orgA := testutil.CreateOrganization(t, store, "user-roles-org-a")
+	orgB := testutil.CreateOrganization(t, store, "user-roles-org-b")
+	userA := testutil.CreateUser(t, store, orgA.ID, "user-roles-user-a")
+	userB := testutil.CreateUser(t, store, orgB.ID, "user-roles-user-b")
+	roleA := testutil.CreateRole(t, store, orgA.ID, "user-roles-role-a")
+	roleB := testutil.CreateRole(t, store, orgB.ID, "user-roles-role-b")
+
+	t.Run("AssignRoleToUser inserts same-org links", func(t *testing.T) {
+		if err := store.AssignRoleToUser(ctx, orgA.ID, userA.ID, roleA.ID); err != nil {
+			t.Fatalf("assign scoped role: %v", err)
+		}
+
+		roles, err := store.ListRolesByUser(ctx, userA.ID)
+		if err != nil {
+			t.Fatalf("list roles by user: %v", err)
+		}
+		if len(roles) != 1 {
+			t.Fatalf("expected 1 role for user, got %d", len(roles))
+		}
+		if roles[0].ID != roleA.ID {
+			t.Fatalf("role id = %s, want %s", roles[0].ID, roleA.ID)
+		}
+	})
+
+	t.Run("AssignRoleToUser rejects foreign-org user", func(t *testing.T) {
+		err := store.AssignRoleToUser(ctx, orgA.ID, userB.ID, roleA.ID)
+		if !errors.Is(err, domain.ErrNotFound) {
+			t.Fatalf("expected ErrNotFound for foreign-org user, got %v", err)
+		}
+	})
+
+	t.Run("AssignRoleToUser rejects foreign-org role", func(t *testing.T) {
+		err := store.AssignRoleToUser(ctx, orgA.ID, userA.ID, roleB.ID)
+		if !errors.Is(err, domain.ErrNotFound) {
+			t.Fatalf("expected ErrNotFound for foreign-org role, got %v", err)
+		}
+	})
+
+	t.Run("raw SQL insert rejects mismatched org tuples", func(t *testing.T) {
+		_, err := store.Pool().Exec(ctx,
+			`INSERT INTO user_roles (org_id, user_id, role_id) VALUES ($1, $2, $3)`,
+			orgA.ID, userB.ID, roleA.ID,
+		)
+		var pgErr *pgconn.PgError
+		if !errors.As(err, &pgErr) || pgErr.Code != "23503" {
+			t.Fatalf("expected foreign-key violation for mismatched org tuple, got %v", err)
+		}
+	})
+}

internal/service/rbac.go

@@ -85,7 +85,7 @@ func (s *RBACService) AssignRoleToUser(ctx context.Context, orgID, userID, roleI
 	if _, err := s.store.GetRoleByID(ctx, orgID, roleID); err != nil {
 		return fmt.Errorf("get role: %w", err)
 	}
-	if err := s.store.AssignRoleToUser(ctx, userID, roleID); err != nil {
+	if err := s.store.AssignRoleToUser(ctx, orgID, userID, roleID); err != nil {
 		return fmt.Errorf("assign role: %w", err)
 	}
 	s.auditSvc.Log(&domain.AuditLog{

migrations/000002_user_roles_org_scope.down.sql

@@ -0,0 +1,18 @@
+ALTER TABLE user_roles
+    DROP CONSTRAINT user_roles_user_org_fkey,
+    DROP CONSTRAINT user_roles_role_org_fkey;
+
+ALTER TABLE user_roles
+    DROP COLUMN org_id;
+
+ALTER TABLE user_roles
+    ADD CONSTRAINT user_roles_user_id_fkey
+        FOREIGN KEY (user_id) REFERENCES users (id) ON DELETE CASCADE,
+    ADD CONSTRAINT user_roles_role_id_fkey
+        FOREIGN KEY (role_id) REFERENCES roles (id) ON DELETE CASCADE;
+
+ALTER TABLE roles
+    DROP CONSTRAINT roles_id_org_id_key;
+
+ALTER TABLE users
+    DROP CONSTRAINT users_id_org_id_key;

migrations/000002_user_roles_org_scope.up.sql

@@ -0,0 +1,32 @@
+ALTER TABLE users
+    ADD CONSTRAINT users_id_org_id_key UNIQUE (id, org_id);
+
+ALTER TABLE roles
+    ADD CONSTRAINT roles_id_org_id_key UNIQUE (id, org_id);
+
+ALTER TABLE user_roles
+    ADD COLUMN org_id UUID;
+
+DELETE FROM user_roles ur
+USING users u, roles r
+WHERE ur.user_id = u.id
+  AND ur.role_id = r.id
+  AND u.org_id <> r.org_id;
+
+UPDATE user_roles ur
+SET org_id = u.org_id
+FROM users u
+WHERE ur.user_id = u.id;
+
+ALTER TABLE user_roles
+    ALTER COLUMN org_id SET NOT NULL;
+
+ALTER TABLE user_roles
+    DROP CONSTRAINT user_roles_user_id_fkey,
+    DROP CONSTRAINT user_roles_role_id_fkey;
+
+ALTER TABLE user_roles
+    ADD CONSTRAINT user_roles_user_org_fkey
+        FOREIGN KEY (user_id, org_id) REFERENCES users (id, org_id) ON DELETE CASCADE,
+    ADD CONSTRAINT user_roles_role_org_fkey
+        FOREIGN KEY (role_id, org_id) REFERENCES roles (id, org_id) ON DELETE CASCADE;