fix(rbac): harden policy validation and error handling across RBAC write paths by PatAKnight · Pull Request #9121 · backstage/community-plugins

PatAKnight · 2026-05-14T08:33:41Z

Hey, I just made a Pull Request!

This PR hardens RBAC policy ingestion and write paths to prevent Casbin CSV poisoning and reduce confusing failure modes. It rejects unsafe permission values before persistence, rethrows loadPolicy() failures so root causes surface correctly, strengthens API validation/error typing (400/404 vs generic 500), and adds bounded limits for conditional payloads, plugin-id registration payloads, and YAML conditional file ingestion (with config-based tuning).

It also improves resilience for CSV file-based policy loading by skipping malformed lines with warnings instead of aborting the full load, and updates docs/config schema/changelog to describe new validation behavior and compatibility implications.

✔️ Checklist

A changeset describing the change and affected packages. (more info)
Added or updated documentation
Tests for new functionality and regression tests for bug fixes
Screenshots attached (for UI changes)
All your commits have a Signed-off-by line in the message. (more info)

backstage-goalie · 2026-05-14T08:34:14Z

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage-community/plugin-rbac-backend	workspaces/rbac/plugins/rbac-backend	patch	`v7.12.4`

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Hardens RBAC policy ingestion and write paths to prevent Casbin CSV poisoning and improve error surfacing/typing across REST, config-driven defaults, and file-based loaders.

Changes:

Adds stricter validation for policy fields, plugin-id payloads, conditional policy shapes, and configurable validation limits.
Improves error typing (InputError/NotFoundError/NotAllowedError) and rethrows enforcer loadPolicy failures after audit logging.
Makes CSV/YAML file ingestion more resilient (skip malformed CSV lines; enforce YAML size/doc limits).

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
workspaces/rbac/plugins/rbac-backend/src/validation/policies-validation.ts	Rejects unsafe `permission` values containing double quotes.
workspaces/rbac/plugins/rbac-backend/src/validation/policies-validation.test.ts	Adds regression test for quoted `permission` rejection.
workspaces/rbac/plugins/rbac-backend/src/validation/plugin-validation.ts	Tightens plugin ID registration validation (bounds/dup/length) and uses InputError.
workspaces/rbac/plugins/rbac-backend/src/validation/plugin-validation.test.ts	Adds tests for new plugin ID validation rules.
workspaces/rbac/plugins/rbac-backend/src/validation/condition-validation.ts	Adds distinct-action checks and configurable limits for conditional criteria complexity.
workspaces/rbac/plugins/rbac-backend/src/validation/condition-validation.test.ts	Adds tests for conditional validation limits and duplicates.
workspaces/rbac/plugins/rbac-backend/src/service/policies-rest-api.ts	Centralizes body parsing validation; improves missing-role handling (404/403).
workspaces/rbac/plugins/rbac-backend/src/service/policies-rest-api.test.ts	Updates REST API tests for new error messages and 404 missing-role behavior.
workspaces/rbac/plugins/rbac-backend/src/service/permission-definition-routes.test.ts	Adds REST tests for plugin-id validation edge cases.
workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.ts	Rethrows `loadPolicy` failures after audit logging.
workspaces/rbac/plugins/rbac-backend/src/service/enforcer-delegate.test.ts	Adds test ensuring `loadPolicy` failures are rethrown and audited.
workspaces/rbac/plugins/rbac-backend/src/policies/permission-policy.ts	Wires config-based conditional validation and YAML file ingestion limits; renames watcher import.
workspaces/rbac/plugins/rbac-backend/src/helper.ts	Aligns owner-filter semantics with rule behavior; handles default roles and unknown keys.
workspaces/rbac/plugins/rbac-backend/src/helper.test.ts	Updates owner-key tests and adds coverage for default roles/unknown keys.
workspaces/rbac/plugins/rbac-backend/src/file-permissions/yaml-conditional-file-watcher.ts	Renames watcher; enforces YAML max bytes/docs and validates each document.
workspaces/rbac/plugins/rbac-backend/src/file-permissions/yaml-conditional-file-watcher.test.ts	Adds tests for YAML size/document limits.
workspaces/rbac/plugins/rbac-backend/src/file-permissions/lowercase-file-adapter.ts	Skips malformed Casbin policy lines with warnings instead of aborting.
workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts	Parses CSV line-by-line; skips malformed lines with warnings; passes logger to adapter.
workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts	Adds fixture-based test asserting malformed CSV lines are skipped and logged.
workspaces/rbac/plugins/rbac-backend/src/default-permissions/default-permissions.ts	Validates default permissions against new policy validation rules; uses InputError.
workspaces/rbac/plugins/rbac-backend/src/default-permissions/default-permissions.test.ts	Updates tests to assert InputError and new invalid-policy behavior.
workspaces/rbac/plugins/rbac-backend/src/database/role-metadata.ts	Prevents duplicate inclusion of cached default role metadata.
workspaces/rbac/plugins/rbac-backend/src/admin-permissions/admin-creation.ts	Validates admin entity refs and throws InputError on invalid config.
workspaces/rbac/plugins/rbac-backend/src/admin-permissions/admin-creation.test.ts	Adds test for invalid admin entity reference handling.
workspaces/rbac/plugins/rbac-backend/docs/conditions.md	Documents distinct action requirement for `permissionMapping`.
workspaces/rbac/plugins/rbac-backend/docs/apis.md	Documents `permission` must not contain double quotes.
workspaces/rbac/plugins/rbac-backend/config.d.ts	Adds config schema for conditional validation and YAML file limits.
workspaces/rbac/plugins/rbac-backend/fixtures/data/invalid-csv/malformed-unquoted-quote.csv	Adds malformed CSV fixture to validate skip behavior.
workspaces/rbac/plugins/rbac-backend/README.md	Documents new hardening behavior and validation limits configuration.
workspaces/rbac/.gitignore	Ignores local CSV policy files.
workspaces/rbac/.changeset/olive-shirts-nail.md	Adds changeset describing hardening and compatibility impact.

Comments suppressed due to low confidence (1)

workspaces/rbac/plugins/rbac-backend/src/validation/policies-validation.ts:1

The helper name/comment suggests it detects unescaped quotes, but the implementation rejects any double quote. Please rename to reflect the actual behavior (e.g., permissionContainsDoubleQuote) and/or adjust the comment to avoid implying support for escaped quotes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

PatAKnight · 2026-05-14T14:30:33Z

+let conditionValidationLimits: ConditionValidationLimits = {
+  ...DEFAULT_CONDITION_VALIDATION_LIMITS,
+};
+
+export function configureConditionValidationLimits(
+  limits: Partial<ConditionValidationLimits>,
+): void {
+  const nextLimits = {
+    ...conditionValidationLimits,
+    ...limits,
+  };
+
+  assertPositiveInteger(nextLimits.maxConditionDepth, 'maxConditionDepth');
+  assertPositiveInteger(
+    nextLimits.maxConditionNodeCount,
+    'maxConditionNodeCount',
+  );
+  assertPositiveInteger(nextLimits.maxCriteriaItems, 'maxCriteriaItems');
+
+  conditionValidationLimits = nextLimits;
+}
+
+function getConditionValidationLimits(): ConditionValidationLimits {
+  return conditionValidationLimits;
+}


Should have been fixed in: c26f4b4

PatAKnight · 2026-05-14T14:30:49Z

+| Json field        | Description                                                                                             | Type         |
+| ----------------- | ------------------------------------------------------------------------------------------------------- | ------------ |
+| result            | Always has the value "CONDITIONAL"                                                                      | String       |
+| roleEntityRef     | String entity reference to the RBAC role ('role:default/dev')                                           | String       |
+| pluginId          | Corresponding plugin ID (e.g., "catalog")                                                               | String       |
+| permissionMapping | Distinct Backstage permission actions only (`create`, `read`, `update`, `delete`, `use`); no duplicates | String array |
+| resourceType      | Resource type provided by the plugin (e.g., "catalog-entity")                                           | String       |
+| conditions        | Condition JSON with parameters or array parameters joined by criteria                                   | JSON         |


Skipping this as I believe this comment was made in error

PatAKnight · 2026-05-14T14:31:13Z

+    expectAuditorLog([
+      {
+        event: { eventId: ConditionEvents.CONDITIONAL_POLICIES_FILE_CHANGE },
+        fail: {
+          error: new Error(
+            'conditional policies file exceeds maximum size of 1048576 bytes',
+          ),
+        },
+      },
+    ]);


Should have been fixed in: 0624dac

PatAKnight · 2026-05-14T14:31:34Z

+  private static readConditionalFileLimits(
+    configApi: ConfigApi,
+  ): Partial<ConditionalPoliciesFileLimits> {
+    const maxBytes = configApi.getOptionalNumber(
+      'permission.rbac.validation.conditionalPoliciesFile.maxBytes',
+    );
+    const maxDocuments = configApi.getOptionalNumber(
+      'permission.rbac.validation.conditionalPoliciesFile.maxDocuments',
+    );
+
+    return {
+      maxBytes: maxBytes ?? DEFAULT_CONDITIONAL_POLICIES_FILE_LIMITS.maxBytes,
+      maxDocuments:
+        maxDocuments ?? DEFAULT_CONDITIONAL_POLICIES_FILE_LIMITS.maxDocuments,
+    };
+  }


Should have been fixed in: 350d0bf

Copilot

Pull request overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated 1 comment.

PatAKnight · 2026-05-14T15:00:09Z

    const isPluginEnabled = env.config.getOptionalBoolean('permission.enabled');
+    const conditionValidationLimits = resolveConditionValidationLimits(
+      readConditionValidationLimitsFromConfig(env.config),
+    );


Should have been fixed in: aff454c

PatAKnight · 2026-05-14T14:59:29Z

workspaces/rbac/plugins/rbac-backend/src/validation/policies-validation.ts:1

The helper name/comment suggests it detects unescaped quotes, but the implementation rejects any double quote. Please rename to reflect the actual behavior (e.g., permissionContainsDoubleQuote) and/or adjust the comment to avoid implying support for escaped quotes.

A low priority item from GitHub copilot that I think was relevant, should have been fixed in: 9406e14

dzemanov · 2026-05-15T14:29:38Z

+        handler(lowercasedLine, model);
+      } catch (error) {
+        const message = error instanceof Error ? error.message : String(error);
+        this.logger?.warn(


Maybe we don't need to warn here and CSVFileWatcher.parse() can be single source of warnings?
Because when testing, I am getting twice the same error logs:

2026-05-15T14:24:35.733Z permission warn Skipping invalid CSV policy line 5 in /Users/dzemanov/Documents/Configs/rbac/permissions.csv: Invalid Opening Quote: a quote is found on field 2 at line 1, value is "catalog-entity"; line=p, role:default/catalog-different, catalog-entity"fuzz, read, allow 2026-05-15T14:24:35.734Z permission warn Skipping invalid policy line 5 in /Users/dzemanov/Documents/Configs/rbac/permissions.csv: Invalid Opening Quote: a quote is found on field 2 at line 1, value is "catalog-entity"; line=p, role:default/catalog-different, catalog-entity"fuzz, read, allow

Or we can change it to debug?

Lines can still be fed to Helper.loadPolicyLine when we rebuild the temp enforcer from the file. Casbin may throw on lines that didn’t fail csv-parse, so I ended up going with debug log to avoid the duplicating warn with while still leaving something for deep troubleshooting.

dzemanov

Thank you @PatAKnight, works great except one small comment.

Tested:

Conditional policies file exceeds maximum of 1 YAML documents
Conditional criteria exceeds maximum of 2 nodes
Unable to convert config value for key 'permission.rbac.validation.conditionalPoliciesFile.maxDocuments' in 'app-config.local.yaml' to a number
comments in files
duplicates: Found condition with conflicted permission action '["delete"]'. Role could have multiple conditions for the same resource type

Invalid policy / role lines from csv file, like "INVALID LINE", are silently skipped. This can be handled in a separate PR if we want to take a look at it.

…silient policy CSV loading Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

… API errors Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…th depth and payload limits Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…rable Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…ility notes Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…ule globals Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…ror identity checks Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…s positive integers Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…tion Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

… disabled Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

Copilot

Pull request overview

Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.

+      if (!metadata) {
+        if (policy.entityReference?.startsWith('role:default')) {
+          throw new NotFoundError(
+            `Corresponding role ${policy.entityReference} was not found`,
+          );
+        }
+        throw new NotAllowedError(); // 403
+      }


 export function validatePermissionDependentPlugin(
  plugin: PermissionDependentPluginList,
 ) {
  if (!plugin.ids) {
-    throw new Error(
+    throw new InputError(
      `'ids' must be specified in the permission dependent plugin`,
    );
  }


PatAKnight · 2026-05-15T16:10:21Z

Thank you @PatAKnight, works great except one small comment.

Tested:

Conditional policies file exceeds maximum of 1 YAML documents
Conditional criteria exceeds maximum of 2 nodes
Unable to convert config value for key 'permission.rbac.validation.conditionalPoliciesFile.maxDocuments' in 'app-config.local.yaml' to a number
comments in files
duplicates: Found condition with conflicted permission action '["delete"]'. Role could have multiple conditions for the same resource type
Invalid policy / role lines from csv file, like "INVALID LINE", are silently skipped. This can be handled in a separate PR if we want to take a look at it.

I think it is fine to leave for now as it’s pre-existing and doesn’t currently break RBAC. I do agree on a follow-up PR to warn on junk rows that parse as CSV but aren’t real policies though.

…parseEntityRef Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

…r invalid Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

dzemanov

Thank you for the changes!

Copilot AI review requested due to automatic review settings May 14, 2026 08:33

PatAKnight requested review from a team, AndrienkoAleksandr, HusneShabbir, christoph-jerolimov, debsmita1, divyanshiGupta, djanickova, dzemanov, jrichter1, karthikjeeyar and rohitkrai03 as code owners May 14, 2026 08:33

PatAKnight requested a review from awanlin May 14, 2026 08:33

backstage-service added the workspace/rbac label May 14, 2026

Copilot AI reviewed May 14, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings May 14, 2026 14:04

Copilot started reviewing on behalf of PatAKnight May 14, 2026 14:05 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

AndrienkoAleksandr approved these changes May 15, 2026

View reviewed changes

dzemanov reviewed May 15, 2026

View reviewed changes

PatAKnight added 7 commits May 15, 2026 12:02

fix(rbac): harden Casbin CSV policy validation, reload errors, and re…

9114260

…silient policy CSV loading Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): harden policy write/config validation and add missing-role…

1052558

… API errors Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): enforce InputError-based condition/plugin-id validation wi…

ef4f90f

…th depth and payload limits Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): cap YAML conditional policy file size and document count

40b0520

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): make conditional and YAML policy validation limits configu…

94ebe4f

…rable Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

docs(rbac): align changeset and README with policy hardening compatib…

935a178

…ility notes Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): fix failing tests

6f1decd

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

PatAKnight added 8 commits May 15, 2026 12:02

fix(rbac): code cleanup, renaming, stick to similar error type

d9d28c0

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): swap to duplication check over maxPermissionMappingItems

0e64a00

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): pass condition validation limits explicitly instead of mod…

6c12e3b

…ule globals Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): align auditor fail assertions with InputError and relax er…

877734f

…ror identity checks Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): validate conditional policies file maxBytes/maxDocuments a…

775bd63

…s positive integers Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): rename permission quote helper to match double-quote rejec…

9a9d936

…tion Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): skip condition validation limit config when RBAC plugin is…

40ec581

… disabled Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): swap from warn to debug when logging casbin csv errors

b4f979f

Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

Copilot AI review requested due to automatic review settings May 15, 2026 16:02

PatAKnight force-pushed the fix-csv-parse-error branch from aff454c to b4f979f Compare May 15, 2026 16:02

Copilot started reviewing on behalf of PatAKnight May 15, 2026 16:03 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

PatAKnight added 2 commits May 15, 2026 12:17

fix(rbac): return 404 for missing role policies in any namespace via …

55c9c78

…parseEntityRef Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

fix(rbac): return InputError when plugin id request body is missing o…

e904c77

…r invalid Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>

dzemanov approved these changes May 15, 2026

View reviewed changes

Conversation

PatAKnight commented May 14, 2026

Hey, I just made a Pull Request!

✔️ Checklist

Uh oh!

backstage-goalie Bot commented May 14, 2026

Changed Packages

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

PatAKnight May 14, 2026

Choose a reason for hiding this comment

Uh oh!

PatAKnight May 14, 2026

Choose a reason for hiding this comment

Uh oh!

PatAKnight May 14, 2026

Choose a reason for hiding this comment

Uh oh!

PatAKnight May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

PatAKnight May 14, 2026

Choose a reason for hiding this comment

Uh oh!

PatAKnight commented May 14, 2026

Uh oh!

dzemanov May 15, 2026

Choose a reason for hiding this comment

Uh oh!

PatAKnight May 15, 2026

Choose a reason for hiding this comment

Uh oh!

dzemanov left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

PatAKnight commented May 15, 2026

Uh oh!

dzemanov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants