Skip to content

fix: correct operator subset check to be more restrictive and add tests#5749

Merged
scott-ray-wilson merged 1 commit intomainfrom
fix-operator-subset
Mar 18, 2026
Merged

fix: correct operator subset check to be more restrictive and add tests#5749
scott-ray-wilson merged 1 commit intomainfrom
fix-operator-subset

Conversation

@scott-ray-wilson
Copy link
Copy Markdown
Contributor

Context

This PR improves the casl sub-operator boundary check and adds relevant test cases

Screenshots

N/A

Steps to verify the change

  • comment out addition, watch tests fail, add back - pass

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Updated CLAUDE.md files (if needed)
  • Read the contributing guide

@maidul98
Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR closes a privilege-escalation vulnerability in the CASL permission-boundary checker: a child role using $NEQ (e.g. "everything except dev") was incorrectly being accepted as a subset of a parent using $EQ or $IN (e.g. "only qa"), allowing a child to request access to a much broader set of resources than its parent permitted.

Key changes:

  • boundary.ts — adds a concise early-return guard: if the child operator is $NEQ and the parent is $EQ or $IN, the check immediately returns false. This is logically correct because no "all-except-X" set can be a subset of a finite allow-list.
  • boundary.test.ts — adds three regression tests that would fail on the old code and pass with the fix, plus corrects a pre-existing mislabeled test case (the $NEQ falsy-case row was accidentally using $GLOB: "staging" in its conditions instead of $NEQ: "staging").
  • One edge-case combination is tested implicitly but not explicitly: $NEQ child where the excluded value is inside the parent's $IN list (e.g. $NEQ: "dev" vs $IN: ["dev","staging"]). Adding a test for that variant would further strengthen coverage.

Confidence Score: 5/5

  • Safe to merge — the fix is minimal, logically sound, and covered by tests.
  • The change is a single targeted guard in a well-isolated helper function. It correctly rejects all $NEQ child conditions against $EQ/$IN parent conditions with no false positives for previously valid cases. Tests confirm both the new behaviour and pre-existing edge cases. No API contracts, database schemas, or external interfaces are altered.
  • No files require special attention.

Important Files Changed

Filename Overview
backend/src/lib/casl/boundary.ts Adds an early-return guard to correctly reject $NEQ child conditions against $EQ/$IN parent conditions — closing a real privilege-escalation path. The one-liner fix is logically correct and does not break any previously valid case.
backend/src/lib/casl/boundary.test.ts Adds three focused regression tests for the $NEQ-child bypass, and corrects an existing mislabeled test case (was using $GLOB: "staging" for the $NEQ falsy-case row, now correctly uses $NEQ: "staging"). Coverage is good but misses one combination (see comment).

Last reviewed commit: "fix: correct operato..."

@scott-ray-wilson scott-ray-wilson merged commit 0f87e59 into main Mar 18, 2026
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants