Skip to content

Fix denyValues to block all value references; add enforceExcludedValues for RBAC enforcement#18

Merged
gblikas merged 6 commits intomainfrom
copilot/fix-denyvalues-negation-issue
Mar 11, 2026
Merged

Fix denyValues to block all value references; add enforceExcludedValues for RBAC enforcement#18
gblikas merged 6 commits intomainfrom
copilot/fix-denyvalues-negation-issue

Conversation

Copy link
Contributor

Copilot AI commented Mar 11, 2026

denyValues was incorrectly modified to allow negated references to denied values (NOT status:"archived"). The original blocking behavior was correct — users should not be able to reference sensitive values in any form. The missing piece was a separate mechanism to ensure denied records never appear in results regardless of query phrasing.

Changes

src/security/validator.ts

  • Reverted validateDenyValues to block all mentions of denied values, including NOT field:"val", field != "val", field NOT IN [...], and any depth of negation nesting

src/security/types.ts

  • Clarified denyValues JSDoc: explicit allow-listing design, any reference to a denied value is rejected regardless of operator
  • Documents the distinction from enforceExcludedValues — e.g. NOT status:"published" implicitly includes archived/deleted rows, which denyValues alone cannot prevent

src/index.ts / enforceExcludedValues

  • Kept the enforceExcludedValues option introduced in the prior session — this is the correct RBAC enforcement tool
  • Injects AND field NOT IN (values) after validation so denied records never appear, regardless of how the user phrases their query

Tests

  • Replaced incorrect negation-allowing tests in validator.test.ts: all negated forms now assert QuerySecurityError
  • Fixed integration test that incorrectly expected NOT status:"archived" to pass denyValues — it should throw
  • Improved enforceExcludedValues integration tests to assert field-level correctness: tests now verify the NOT IN clause targets the correct column (e.g. status) and the exact excluded values, not just that "not in" appears somewhere in the SQL
  • Added critical interaction test: with denyValues + enforceExcludedValues both configured, NOT status:deleted is blocked by denyValues; NOT status:published passes validation but gets AND status NOT IN ('archived', 'deleted') injected, ensuring only non-excluded records (e.g. status:active) can ever be returned
  • Added comprehensive robustness test suite covering attack forms and bypass attempts for the denyValues + enforceExcludedValues combination
// denyValues: blocks any query that references the value
security: { denyValues: { status: ['archived', 'deleted'] } }
// status:"archived"                        → throws
// NOT status:"archived"                    → throws (value is still referenced)
// NOT status:"deleted"                     → throws (each denied value individually enforced)
// priority:>1 AND status:archived          → throws (denied value in AND branch)
// title:"test" OR status:deleted           → throws (denied value in OR branch)
// (priority:>1 AND title:"x") AND status:archived → throws (deep nesting)
// status IN [active, archived]             → throws (mixed allowed/denied IN)
// status IN [archived, deleted]            → throws (all-denied IN)
// status:"active"                          → passes

// enforceExcludedValues: guarantees records never returned
security: { enforceExcludedValues: { status: ['archived', 'deleted'] } }
// status:"active"         → SELECT … WHERE status = 'active' AND status NOT IN ('archived', 'deleted')
// NOT status:"published"  → SELECT … WHERE status != 'published' AND status NOT IN ('archived', 'deleted')

// combined: denyValues blocks references to denied values;
// enforceExcludedValues ensures denied records are never returned regardless of query phrasing
security: {
  denyValues: { status: ['archived', 'deleted'] },
  enforceExcludedValues: { status: ['archived', 'deleted'] }
}
// NOT status:deleted    → throws (archived/deleted are denied values)
// NOT status:published  → SELECT … WHERE status != 'published' AND status NOT IN ('archived', 'deleted')
//                         (only status:active records returned)
// priority:>1           → SELECT … WHERE priority > 1 AND status NOT IN ('archived', 'deleted')
// title:"test"          → SELECT … WHERE title = 'test' AND status NOT IN ('archived', 'deleted')
Original prompt

Summary

Fix issue #17: denyValues blocks queries with negated values (NOT operator)

GitHub Issue: #17

Problem

The denyValues security feature currently blocks any query that mentions a denied value, including queries that explicitly exclude that value using negation (NOT, !=, NOT IN).

Current Behavior

  • status:"archived" → Blocked ✅
  • status:"deleted" → Blocked ✅
  • NOT status:"archived" → Blocked ❌ (should be allowed)
  • status != "deleted" → Blocked ❌ (should be allowed)

Expected Behavior

  • status:"archived" → Blocked (user trying to access archived items)
  • status:"deleted" → Blocked (user trying to access deleted items)
  • NOT status:"archived"Allowed (user explicitly excluding archived items)
  • status != "deleted"Allowed (user explicitly excluding deleted items)

Solution

Implement two features:

1. Negation Context Fix for denyValues

Modify validateDenyValues in src/security/validator.ts to track negation context during AST traversal:

private validateDenyValues(
  expression: QueryExpression,
  negated: boolean = false
): void {
  if (Object.keys(this.options.denyValues).length === 0) {
    return;
  }

  if (expression.type === 'comparison') {
    const { field, operator, value } = expression;
    
    // Check if this is an exclusion (negated context OR negative operator)
    const isExclusion = negated || operator === '!=' || operator === 'NOT IN';
    
    if (isExclusion) {
      return; // User is excluding the value, allow it
    }

    const deniedValues = this.options.denyValues[field];

    if (deniedValues && deniedValues.length > 0) {
      if (Array.isArray(value)) {
        for (const item of value) {
          if (this.isValueDenied(item, deniedValues)) {
            throw new QuerySecurityError('Invalid query parameters');
          }
        }
      } else {
        if (this.isValueDenied(value, deniedValues)) {
          throw new QuerySecurityError('Invalid query parameters');
        }
      }
    }
  } else if (expression.type === 'logical') {
    // Flip negation context when encountering NOT operator
    const childNegated = expression.operator === 'NOT' ? !negated : negated;
    
    this.validateDenyValues(expression.left, childNegated);
    if (expression.right) {
      this.validateDenyValues(expression.right, childNegated);
    }
  }
}

Key points:

  • Pass negated: boolean = false through recursive calls
  • Toggle negation with !negated when hitting NOT operator (handles double-negation correctly)
  • Also treat != and NOT IN operators as exclusions
  • Skip validation for exclusion comparisons

2. New enforceExcludedValues Option

Add a new security option that automatically injects exclusion filters into every query at the adapter layer. This ensures sensitive records are never returned regardless of how users phrase their queries.

Add to src/security/types.ts:

/**
 * Values that are automatically excluded from ALL query results.
 * Unlike `denyValues` (which validates user input), this option
 * injects `AND NOT field IN (values)` into every query.
 *
 * Use this for RBAC enforcement where certain records must never
 * be returned regardless of user query.
 *
 * @example
 * ```typescript
 * enforceExcludedValues: {
 *   status: ['archived', 'deleted'],
 *   visibility: ['internal']
 * }
 * // Every query will have:
 * // AND status NOT IN ('archived', 'deleted')
 * // AND visibility NOT IN ('internal')
 * ```
 */
enforceExcludedValues?: Record<string, Array<string | number | boolean | null>>;

Implement the query transformation that applies these exclusions before SQL generation.

Files to Modify

  1. src/security/validator.ts: Update validateDenyValues method with negation context
  2. src/security/types.ts: Add enforceExcludedValues option and update documentation for denyValues
  3. src/security/validator.test.ts: Add extensive tests for negation handling

Required Tests

Add the following test cases to src/security/validator.test.ts:

describe('validateDenyValues with negation', () => {
  it('should allow NOT operator with denied values', () => {
    const validator = new QuerySecurityValidator({
      denyValues: { status: ['archived', 'deleted'] }
    });
    const query = parser.parse('NOT status:"archived"');
    expect(() => validator.validate(query)).not.toThrow();
  });

  it('should allow != operator with denied values', () => {
    const validator = new QuerySecurityValidator({
      denyValues: { status: ['deleted'] }
    });
    // Create a mock expression since parser may not support !=
    const mockQuery: QueryExpression = {
      type: 'comparison',
      field: 'status',
      operator: '!=',
      value: 'deleted'
    };
    expect(() => validator.validate(mockQuery)).not.toThrow();
  });

  it('should allow NOT IN operator with ...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

@vercel
Copy link
Contributor

vercel bot commented Mar 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
querykit Ready Ready Preview, Comment Mar 11, 2026 6:45pm

Co-authored-by: gblikas <13577108+gblikas@users.noreply.github.com>
- Update validateDenyValues to track negation context during AST traversal,
  allowing NOT/!=/NOT IN comparisons with denied values to pass validation
- Add enforceExcludedValues security option that injects NOT IN filters at
  the adapter layer for server-side RBAC enforcement
- Add 22 new tests covering negation handling, double/triple negation,
  complex AND/OR expressions, and enforceExcludedValues query injection

Co-authored-by: gblikas <13577108+gblikas@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix denyValues blocking negated queries Fix denyValues negation handling + add enforceExcludedValues for server-side RBAC enforcement Mar 11, 2026
…e docs and tests

- Revert validateDenyValues to original behavior: block all mentions of
  denied values regardless of operator (NOT, !=, NOT IN, etc.)
- Update denyValues JSDoc to explain the explicit allow-listing design and
  when to use enforceExcludedValues instead
- Replace incorrect negation-allowing tests with tests that correctly
  expect all negated forms to throw QuerySecurityError
- Fix integration test that incorrectly expected NOT status:"archived" to
  pass denyValues validation

Co-authored-by: gblikas <13577108+gblikas@users.noreply.github.com>
Copilot AI changed the title Fix denyValues negation handling + add enforceExcludedValues for server-side RBAC enforcement Fix denyValues to block all value references; add enforceExcludedValues for RBAC enforcement Mar 11, 2026
Copy link
Owner

@gblikas gblikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot tests are incomplete, and inconclusive.

Feedback

You've checked that generated SQL expressions (do not) have not in, but didn't check what the value of the not in corresponded to.

Further, you didn't check a critical test:

const qk = createQueryKit({
        adapter,
        schema: { todos: { id: {}, title: {}, priority: {}, status: {} } },
        security: {
          // denyValues blocks any query that references these values
          denyValues: { status: ['archived', 'deleted'] },
          // enforceExcludedValues injects NOT IN so those records never appear
          enforceExcludedValues: { status: ['archived', 'deleted'] }
        }
      });

Should check something like

await qk.query('todos').where('NOT status:deleted');

our goal is to make sure that only status:active elements are returned, since we've denies archived and deleted.

…l denyValues+enforceExcludedValues interaction test

- All existing NOT IN tests now also assert the field name ('status') to verify the exclusion targets the correct column
- Added critical test: NOT status:deleted is blocked by denyValues; NOT status:published passes but gets status NOT IN ('archived', 'deleted') injected by enforceExcludedValues, ensuring archived/deleted records are never returned

Co-authored-by: gblikas <13577108+gblikas@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot tests are incomplete, and inconclusive.

Feedback ...

Fixed in commit 52d91a2. Two changes made to the integration tests:

  1. Field-level assertions: All NOT IN tests now also assert that status appears in the WHERE clause (confirming the exclusion targets the correct column, not just that "not in" appears somewhere in the SQL).

  2. Critical interaction test added (should block queries referencing denied values, and enforce exclusions on safe queries):

    • NOT status:deleted → rejected by denyValues (throws)
    • NOT status:published → passes denyValues (published isn't denied), but enforceExcludedValues injects AND status NOT IN ('archived', 'deleted') — ensuring those records can never be returned even though the query would otherwise implicitly include them

Copy link
Owner

@gblikas gblikas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot generate more test cases for the integration tests, that more throughly test the relationship between denyValues and enforceExcludedValues. We should make sure that our solution is robust to "hacks" and various other SQL manipulations.

New test suite covers:
- denyValues blocking: direct equality, NOT negation for each denied value,
  compound AND/OR with denied value, deep nesting, IN with mixed/all denied values
- enforceExcludedValues injection: status:active, title query, priority:>1,
  compound AND/OR on safe fields, NOT status:published (key bypass attempt),
  separate QK instance
Each 'safe' test verifies field name ('status'), operator ('not in'), and
both excluded values ('archived', 'deleted') in the generated SQL

Co-authored-by: gblikas <13577108+gblikas@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot generate more test cases for the integration tests, that more throughly test the relationship between denyValues and enforceExcludedValues. We should make sure that our solution is robust...

Added a new denyValues + enforceExcludedValues robustness describe block in commit 79e44d5 with 17 tests covering both attack blocking and enforced exclusions:

denyValues blocks all reference forms:

  • status:archived (direct equality)
  • NOT status:archived and NOT status:deleted (NOT negation, one per denied value)
  • priority:>1 AND status:archived (denied value in AND branch)
  • title:"test" OR status:deleted (denied value in OR branch)
  • (priority:>1 AND title:"test") AND status:archived (deep nesting)
  • status IN [active, archived] (mixed allowed/denied IN)
  • status IN [archived, deleted] (all-denied IN)

enforceExcludedValues always injects status NOT IN ('archived', 'deleted'):

  • status:active, title:"Buy groceries", priority:>1 (various safe queries on different fields)
  • priority:>1 AND title:"test", priority:>5 OR title:"urgent" (compound safe queries)
  • NOT status:published — the key bypass: published is not a denied value so denyValues passes it, but enforceExcludedValues still injects the NOT IN, preventing archived/deleted rows from slipping through the implicit inclusion

Each "safe query" test asserts status, not in, archived, and deleted all appear in the generated SQL, confirming the correct field and values are targeted.

@gblikas gblikas marked this pull request as ready for review March 11, 2026 18:51
@gblikas gblikas merged commit a20d5a0 into main Mar 11, 2026
6 checks passed
@gblikas gblikas deleted the copilot/fix-denyvalues-negation-issue branch March 11, 2026 19:18
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.

2 participants