Skip to content

Commit e636813

Browse files
kmendellkmendell
authored
fix: validate claim value and role ID in OIDC mapping functions (#2796)
1 parent 56b131b commit e636813

9 files changed

Lines changed: 192 additions & 25 deletions

File tree

‎backend/api/handlers/oidc.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"log/slog"
66
"net/http"
7+
"strings"
78
"time"
89

910
"github.com/danielgtaylor/huma/v2"
@@ -517,8 +518,19 @@ func (h *OidcHandler) CreateOidcRoleMapping(ctx context.Context, input *CreateOi
517518
if h.roleService == nil {
518519
return nil, huma.Error500InternalServerError("service not available")
519520
}
520-
mapping, err := h.roleService.CreateOidcMapping(ctx, input.Body.ClaimValue, input.Body.RoleID, input.Body.EnvironmentID)
521+
claimValue := strings.TrimSpace(input.Body.ClaimValue)
522+
roleID := strings.TrimSpace(input.Body.RoleID)
523+
if claimValue == "" {
524+
return nil, huma.Error400BadRequest("claim value is required")
525+
}
526+
if roleID == "" {
527+
return nil, huma.Error400BadRequest("role id is required")
528+
}
529+
mapping, err := h.roleService.CreateOidcMapping(ctx, claimValue, roleID, input.Body.EnvironmentID)
521530
if err != nil {
531+
if common.IsInvalidRoleAssignmentError(err) {
532+
return nil, huma.Error400BadRequest(err.Error())
533+
}
522534
return nil, huma.Error500InternalServerError("failed to create mapping: " + err.Error())
523535
}
524536
out := &CreateOidcRoleMappingOutput{}
@@ -531,14 +543,25 @@ func (h *OidcHandler) UpdateOidcRoleMapping(ctx context.Context, input *UpdateOi
531543
if h.roleService == nil {
532544
return nil, huma.Error500InternalServerError("service not available")
533545
}
534-
mapping, err := h.roleService.UpdateOidcMapping(ctx, input.ID, input.Body.ClaimValue, input.Body.RoleID, input.Body.EnvironmentID)
546+
claimValue := strings.TrimSpace(input.Body.ClaimValue)
547+
roleID := strings.TrimSpace(input.Body.RoleID)
548+
if claimValue == "" {
549+
return nil, huma.Error400BadRequest("claim value is required")
550+
}
551+
if roleID == "" {
552+
return nil, huma.Error400BadRequest("role id is required")
553+
}
554+
mapping, err := h.roleService.UpdateOidcMapping(ctx, input.ID, claimValue, roleID, input.Body.EnvironmentID)
535555
if err != nil {
536556
if common.IsOidcMappingNotFoundError(err) {
537557
return nil, huma.Error404NotFound("mapping not found")
538558
}
539559
if common.IsOidcMappingEnvManagedError(err) {
540560
return nil, huma.Error409Conflict(err.Error())
541561
}
562+
if common.IsInvalidRoleAssignmentError(err) {
563+
return nil, huma.Error400BadRequest(err.Error())
564+
}
542565
return nil, huma.Error500InternalServerError("failed to update mapping: " + err.Error())
543566
}
544567
out := &UpdateOidcRoleMappingOutput{}

‎backend/internal/services/role_service.go‎

Lines changed: 66 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -743,25 +743,45 @@ func (s *RoleService) GetOidcMapping(ctx context.Context, id string) (*models.Oi
743743
}
744744

745745
func (s *RoleService) CreateOidcMapping(ctx context.Context, claimValue, roleID string, environmentID *string) (*models.OidcRoleMapping, error) {
746-
if strings.TrimSpace(claimValue) == "" {
746+
claimValue = strings.TrimSpace(claimValue)
747+
roleID = strings.TrimSpace(roleID)
748+
if claimValue == "" {
747749
return nil, errors.New("claim value is required")
748750
}
749-
if strings.TrimSpace(roleID) == "" {
751+
if roleID == "" {
750752
return nil, errors.New("role id is required")
751753
}
752-
mapping := &models.OidcRoleMapping{
753-
ClaimValue: claimValue,
754-
RoleID: roleID,
755-
EnvironmentID: environmentID,
756-
Source: models.OidcMappingSourceManual,
757-
}
758-
if err := s.db.WithContext(ctx).Create(mapping).Error; err != nil {
759-
return nil, fmt.Errorf("failed to create oidc mapping: %w", err)
754+
var mapping models.OidcRoleMapping
755+
err := dbutil.WithTx(ctx, s.db.DB, func(tx *gorm.DB) error {
756+
if err := validateRoleIDsExistInternal(tx, []string{roleID}); err != nil {
757+
return err
758+
}
759+
mapping = models.OidcRoleMapping{
760+
ClaimValue: claimValue,
761+
RoleID: roleID,
762+
EnvironmentID: environmentID,
763+
Source: models.OidcMappingSourceManual,
764+
}
765+
if err := tx.Create(&mapping).Error; err != nil {
766+
return fmt.Errorf("failed to create oidc mapping: %w", err)
767+
}
768+
return nil
769+
})
770+
if err != nil {
771+
return nil, err
760772
}
761-
return mapping, nil
773+
return &mapping, nil
762774
}
763775

764776
func (s *RoleService) UpdateOidcMapping(ctx context.Context, id, claimValue, roleID string, environmentID *string) (*models.OidcRoleMapping, error) {
777+
claimValue = strings.TrimSpace(claimValue)
778+
roleID = strings.TrimSpace(roleID)
779+
if claimValue == "" {
780+
return nil, errors.New("claim value is required")
781+
}
782+
if roleID == "" {
783+
return nil, errors.New("role id is required")
784+
}
765785
var out models.OidcRoleMapping
766786
err := dbutil.WithTx(ctx, s.db.DB, func(tx *gorm.DB) error {
767787
var existing models.OidcRoleMapping
@@ -774,6 +794,9 @@ func (s *RoleService) UpdateOidcMapping(ctx context.Context, id, claimValue, rol
774794
if existing.Source == models.OidcMappingSourceEnv {
775795
return &common.OidcMappingEnvManagedError{}
776796
}
797+
if err := validateRoleIDsExistInternal(tx, []string{roleID}); err != nil {
798+
return err
799+
}
777800
existing.ClaimValue = claimValue
778801
existing.RoleID = roleID
779802
existing.EnvironmentID = environmentID
@@ -789,6 +812,38 @@ func (s *RoleService) UpdateOidcMapping(ctx context.Context, id, claimValue, rol
789812
return &out, nil
790813
}
791814

815+
func validateRoleIDsExistInternal(tx *gorm.DB, roleIDs []string) error {
816+
if len(roleIDs) == 0 {
817+
return nil
818+
}
819+
roleIDSet := make(map[string]struct{}, len(roleIDs))
820+
for _, roleID := range roleIDs {
821+
if roleID == "" {
822+
return errors.New("role id is required")
823+
}
824+
roleIDSet[roleID] = struct{}{}
825+
}
826+
827+
normalized := make([]string, 0, len(roleIDSet))
828+
for roleID := range roleIDSet {
829+
normalized = append(normalized, roleID)
830+
}
831+
var found []string
832+
if err := tx.Model(&models.Role{}).Where("id IN ?", normalized).Pluck("id", &found).Error; err != nil {
833+
return fmt.Errorf("failed to verify role ids: %w", err)
834+
}
835+
foundSet := make(map[string]struct{}, len(found))
836+
for _, id := range found {
837+
foundSet[id] = struct{}{}
838+
}
839+
for _, id := range normalized {
840+
if _, ok := foundSet[id]; !ok {
841+
return &common.InvalidRoleAssignmentError{RoleID: id}
842+
}
843+
}
844+
return nil
845+
}
846+
792847
func (s *RoleService) DeleteOidcMapping(ctx context.Context, id string) error {
793848
return dbutil.WithTx(ctx, s.db.DB, func(tx *gorm.DB) error {
794849
var existing models.OidcRoleMapping

‎backend/pkg/authz/access_policy.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,26 +57,39 @@ var accessSurfacesInternal = []AccessSurface{
5757

5858
routeSurfaceInternal("route.dashboard", "/dashboard", "Dashboard", AccessScopeModeSelectedEnvPlusGlobal, []string{PermDashboardRead}, 10),
5959
routeSurfaceInternal("route.projects", "/projects", "Projects", AccessScopeModeSelectedEnvPlusGlobal, []string{PermProjectsList, PermProjectsRead}, 30),
60+
routeSurfaceInternal("route.projects.new", "/projects/new", "Create project", AccessScopeModeSelectedEnvPlusGlobal, []string{PermProjectsList, PermProjectsRead, PermProjectsCreate}, 0),
61+
routeSurfaceInternal("route.projects.detail", "/projects/{projectId}", "Project", AccessScopeModeSelectedEnvPlusGlobal, []string{PermProjectsList, PermProjectsRead}, 0),
6062
routeSurfaceInternal("route.environments", "/environments", "Environments", AccessScopeModeGlobalOnly, []string{PermEnvironmentsList, PermEnvironmentsRead}, 130),
63+
routeSurfaceInternal("route.environments.detail", "/environments/{id}", "Environment", AccessScopeModeGlobalOnly, []string{PermEnvironmentsList, PermEnvironmentsRead}, 0),
6164
routeSurfaceInternal("route.environments.gitops", "/environments/{id}/gitops", "GitOps Syncs", AccessScopeModeSelectedEnvPlusGlobal, []string{PermGitOpsList, PermGitOpsRead}, 0),
6265
routeSurfaceInternal("route.containers", "/containers", "Containers", AccessScopeModeSelectedEnvPlusGlobal, []string{PermContainersList, PermContainersRead}, 20),
66+
routeSurfaceInternal("route.containers.detail", "/containers/{containerId}", "Container", AccessScopeModeSelectedEnvPlusGlobal, []string{PermContainersList, PermContainersRead}, 0),
6367
routeSurfaceInternal("route.images", "/images", "Images", AccessScopeModeSelectedEnvPlusGlobal, []string{PermImagesList, PermImagesRead}, 50),
68+
routeSurfaceInternal("route.images.detail", "/images/{imageId}", "Image", AccessScopeModeSelectedEnvPlusGlobal, []string{PermImagesList, PermImagesRead}, 0),
6469
routeSurfaceInternal("route.images.builds", "/images/builds", "Builds", AccessScopeModeSelectedEnvPlusGlobal, []string{PermImagesBuild}, 0),
6570
routeSurfaceInternal("route.images.vulnerabilities", "/images/vulnerabilities", "Vulnerabilities", AccessScopeModeSelectedEnvPlusGlobal, []string{PermVulnsRead}, 0),
6671
routeSurfaceInternal("route.updates", "/updates", "Image Updates", AccessScopeModeSelectedEnvPlusGlobal, []string{PermImageUpdatesRead}, 0),
6772
routeSurfaceInternal("route.networks", "/networks", "Networks", AccessScopeModeSelectedEnvPlusGlobal, []string{PermNetworksList, PermNetworksRead}, 70),
73+
routeSurfaceInternal("route.networks.detail", "/networks/{networkId}", "Network", AccessScopeModeSelectedEnvPlusGlobal, []string{PermNetworksList, PermNetworksRead}, 0),
6874
routeSurfaceInternal("route.ports", "/ports", "Ports", AccessScopeModeSelectedEnvPlusGlobal, []string{PermContainersList}, 0),
6975
routeSurfaceInternal("route.networks.topology", "/networks/topology", "Network Topology", AccessScopeModeSelectedEnvPlusGlobal, []string{PermNetworksRead}, 0),
7076
routeSurfaceInternal("route.volumes", "/volumes", "Volumes", AccessScopeModeSelectedEnvPlusGlobal, []string{PermVolumesList, PermVolumesRead}, 60),
77+
routeSurfaceInternal("route.volumes.detail", "/volumes/{volumeName}", "Volume", AccessScopeModeSelectedEnvPlusGlobal, []string{PermVolumesList, PermVolumesRead}, 0),
7178
routeSurfaceInternal("route.swarm", "/swarm", "Swarm", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmRead}, 0),
7279
routeSurfaceInternal("route.swarm.services", "/swarm/services", "Services", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmServices}, 80),
80+
routeSurfaceInternal("route.swarm.services.detail", "/swarm/services/{serviceId}", "Service", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmServices}, 0),
7381
routeSurfaceInternal("route.swarm.nodes", "/swarm/nodes", "Nodes", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmNodes}, 0),
7482
routeSurfaceInternal("route.swarm.tasks", "/swarm/tasks", "Tasks", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmRead}, 0),
7583
routeSurfaceInternal("route.swarm.stacks", "/swarm/stacks", "Stacks", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmStacks}, 90),
84+
routeSurfaceInternal("route.swarm.stacks.new", "/swarm/stacks/new", "Create stack", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmStacks}, 0),
85+
routeSurfaceInternal("route.swarm.stacks.detail", "/swarm/stacks/{name}", "Stack", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmStacks}, 0),
7686
routeSurfaceInternal("route.swarm.cluster", "/swarm/cluster", "Cluster", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmRead}, 100),
7787
routeSurfaceInternal("route.swarm.configs", "/swarm/configs", "Configs", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmConfigs}, 0),
7888
routeSurfaceInternal("route.swarm.secrets", "/swarm/secrets", "Secrets", AccessScopeModeSelectedEnvPlusGlobal, []string{PermSwarmSecrets}, 0),
7989
routeSurfaceInternal("route.events", "/events", "Events", AccessScopeModeGlobalOnly, []string{PermEventsRead}, 110),
90+
routeSurfaceInternal("route.customize.templates.create", "/customize/templates/create", "Create template", AccessScopeModeGlobalOnly, []string{PermCustomizeManage, PermTemplatesList, PermTemplatesRead}, 0),
91+
routeSurfaceInternal("route.customize.templates.default", "/customize/templates/default", "Default template", AccessScopeModeGlobalOnly, []string{PermCustomizeManage, PermTemplatesList, PermTemplatesRead}, 0),
92+
routeSurfaceInternal("route.customize.templates.detail", "/customize/templates/{id}", "Template", AccessScopeModeGlobalOnly, []string{PermCustomizeManage, PermTemplatesList, PermTemplatesRead}, 0),
8093

8194
settingsCategorySurfaceInternal("activity", "/settings/activity", "Activity", AccessScopeModeGlobalOnly, []string{PermSettingsRead}),
8295
settingsCategorySurfaceInternal("apikeys", "/settings/api-keys", "API Keys", AccessScopeModeGlobalOnly, []string{PermApiKeysList, PermApiKeysRead}),
@@ -86,6 +99,8 @@ var accessSurfacesInternal = []AccessSurface{
8699
settingsCategorySurfaceInternal("jobschedule", "", "Job Schedule", AccessScopeModeSelectedEnvPlusGlobal, []string{PermJobsManage}),
87100
settingsCategorySurfaceInternal("notifications", "/settings/notifications", "Notifications", AccessScopeModeSelectedEnvPlusGlobal, []string{PermNotificationsManage}),
88101
settingsCategorySurfaceInternal("roles", "/settings/roles", "Roles", AccessScopeModeGlobalOnly, []string{PermRolesList, PermRolesRead}),
102+
routeSurfaceInternal("route.settings.roles.new", "/settings/roles/new", "Create role", AccessScopeModeGlobalOnly, []string{PermRolesList, PermRolesRead}, 0),
103+
routeSurfaceInternal("route.settings.roles.detail", "/settings/roles/{id}", "Role", AccessScopeModeGlobalOnly, []string{PermRolesList, PermRolesRead}, 0),
89104
settingsCategorySurfaceInternal("timeouts", "/settings/timeouts", "Timeouts", AccessScopeModeGlobalOnly, []string{PermSettingsRead}),
90105
settingsCategorySurfaceInternal("users", "/settings/users", "Users", AccessScopeModeGlobalOnly, []string{PermUsersList, PermUsersRead}),
91106
settingsCategorySurfaceInternal("webhooks", "/settings/webhooks", "Webhooks", AccessScopeModeSelectedEnvPlusGlobal, []string{PermWebhooksList}),

‎backend/pkg/authz/permission_set.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,19 +44,26 @@ func (ps *PermissionSet) Allows(perm, envID string) bool {
4444
// Global set contains every defined permission. Used by the backward-compat
4545
// IsAdminFromContext helper and by last-admin guards.
4646
//
47-
// Implementation: every insertion path runs through
48-
// services.validatePermissionsInternal → authz.IsKnownPermission, which does
49-
// an exact-set check against AllPermissions(). ps.Global is therefore a true
50-
// subset of AllPermissions, and equal cardinality implies equality — no
51-
// per-call slice allocation or O(N) walk on the auth hot path.
47+
// Implementation: first checks cardinality against TotalPermissionsCount() for a
48+
// fast early exit, then walks AllPermissions() to confirm every known
49+
// permission is present. This guards against a ps.Global that contains the right
50+
// count but includes injected unknown permissions instead of all real ones.
5251
func (ps *PermissionSet) IsGlobalAdmin() bool {
5352
if ps == nil {
5453
return false
5554
}
5655
if ps.Sudo {
5756
return true
5857
}
59-
return len(ps.Global) >= TotalPermissionsCount()
58+
if len(ps.Global) != TotalPermissionsCount() {
59+
return false
60+
}
61+
for _, permission := range AllPermissions() {
62+
if _, ok := ps.Global[permission]; !ok {
63+
return false
64+
}
65+
}
66+
return true
6067
}
6168

6269
// SudoPermissionSet returns a PermissionSet that allows every action. Used for

‎backend/pkg/authz/permission_set_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,46 @@ func TestPermissionSetEnvScopedDoesNotLeak(t *testing.T) {
3232
}
3333
}
3434

35+
func TestPermissionSetIsGlobalAdminRejectsUnknownPermissions(t *testing.T) {
36+
ps := NewPermissionSet()
37+
perms := AllPermissions()
38+
for i, perm := range perms {
39+
if i == 0 {
40+
continue
41+
}
42+
ps.AddGlobal(perm)
43+
}
44+
ps.AddGlobal("projects:made-up")
45+
46+
if ps.IsGlobalAdmin() {
47+
t.Fatal("global admin should reject injected unknown permissions")
48+
}
49+
}
50+
51+
func TestPermissionSetIsGlobalAdminRequiresExactKnownSet(t *testing.T) {
52+
ps := NewPermissionSet()
53+
perms := AllPermissions()
54+
for _, perm := range perms[1:] {
55+
ps.AddGlobal(perm)
56+
}
57+
ps.AddGlobal("containers:does-not-exist")
58+
59+
if ps.IsGlobalAdmin() {
60+
t.Fatal("global admin should require the complete known permission set")
61+
}
62+
}
63+
64+
func TestPermissionSetIsGlobalAdmin(t *testing.T) {
65+
ps := NewPermissionSet()
66+
for _, perm := range AllPermissions() {
67+
ps.AddGlobal(perm)
68+
}
69+
70+
if !ps.IsGlobalAdmin() {
71+
t.Fatal("complete known permission set should be global admin")
72+
}
73+
}
74+
3575
func TestSudoAllowsEverything(t *testing.T) {
3676
ps := SudoPermissionSet()
3777
if !ps.Allows(PermContainersDelete, "any-env") {

‎frontend/src/lib/utils/access-policy.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,12 @@ export function getRouteAccessSurfaces(manifest: PermissionsManifest | null | un
3434
return (manifest?.accessSurfaces ?? [])
3535
.filter((surface) => !!surface.url)
3636
.slice()
37-
.sort((a, b) => (b.url?.length ?? 0) - (a.url?.length ?? 0));
37+
.sort((a, b) => {
38+
const aTemplates = (a.url?.match(/\{[^}]+\}/g) ?? []).length;
39+
const bTemplates = (b.url?.match(/\{[^}]+\}/g) ?? []).length;
40+
if (aTemplates !== bTemplates) return aTemplates - bTemplates;
41+
return (b.url?.length ?? 0) - (a.url?.length ?? 0);
42+
});
3843
}
3944

4045
export function getFallbackAccessSurfaces(manifest: PermissionsManifest | null | undefined): AccessSurface[] {
@@ -50,7 +55,7 @@ export function pathMatchesAccessSurface(path: string, surface: AccessSurface):
5055

5156
const pathSegments = splitPathInternal(path);
5257
const templateSegments = splitPathInternal(template);
53-
if (templateSegments.length > pathSegments.length) return false;
58+
if (templateSegments.length !== pathSegments.length) return false;
5459

5560
for (let i = 0; i < templateSegments.length; i++) {
5661
const expected = templateSegments[i];

‎frontend/src/lib/utils/auth.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,8 @@ export function getAuthRedirectPath(
109109
path: string,
110110
user: User | null,
111111
envId?: string,
112-
accessManifest?: PermissionsManifest | null
112+
accessManifest?: PermissionsManifest | null,
113+
accessManifestLoadFailed = false
113114
): string | null {
114115
const isSignedIn = !!user;
115116

@@ -125,6 +126,16 @@ export function getAuthRedirectPath(
125126
return '/dashboard';
126127
}
127128

129+
if (
130+
isSignedIn &&
131+
!accessManifestLoadFailed &&
132+
path !== '/no-access' &&
133+
!accessManifest?.accessSurfaces?.length &&
134+
matchesAny(path, PROTECTED_PREFIXES)
135+
) {
136+
return '/no-access';
137+
}
138+
128139
if (!isSignedIn || !user) return null;
129140

130141
if (path !== '/no-access' && !userHasAnyAccess(user)) {

‎frontend/src/routes/(app)/+layout.svelte‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
const user = $derived(data.user);
2121
const settings = $derived(data.settings);
2222
const permissionsManifest = $derived(data.permissionsManifest);
23+
const permissionsManifestLoadFailed = $derived(data.permissionsManifestLoadFailed);
2324
const swarmEnabled = $derived(data.swarmEnabled === true);
2425
2526
const isMobile = new IsMobile();
@@ -46,7 +47,13 @@
4647
});
4748
4849
$effect(() => {
49-
const redirectPath = getAuthRedirectPath(page.url.pathname, user, currentEnvId, permissionsManifest);
50+
const redirectPath = getAuthRedirectPath(
51+
page.url.pathname,
52+
user,
53+
currentEnvId,
54+
permissionsManifest,
55+
permissionsManifestLoadFailed
56+
);
5057
if (redirectPath) {
5158
goto(redirectPath);
5259
}

0 commit comments

Comments
 (0)