Skip to content

feat: conditional data masking with row-level filters in access policies#10803

Merged
paveltiunov merged 16 commits intomasterfrom
cursor/data-masking-row-filter-case-when-032b
May 6, 2026
Merged

feat: conditional data masking with row-level filters in access policies#10803
paveltiunov merged 16 commits intomasterfrom
cursor/data-masking-row-filter-case-when-032b

Conversation

@paveltiunov
Copy link
Copy Markdown
Member

@paveltiunov paveltiunov commented May 3, 2026

Check List

  • Tests have been run in packages where changes have been made if available
  • Tests for the changes have been added if not covered yet
  • Linter has been run for changed code
  • Docs have been added / updated if required

Description of Changes Made

When an access policy grants full member_level access with row_level filters, and another policy provides masked access, the masking should be conditional on the row filter. Previously, masking was skipped entirely when any policy granted "full access" for a member, even if that full access was gated by row-level filters.

Problem

Given an access policy like:

access_policy:
  - group: sensitive_data_access
    member_level:
      includes:
        - users_id
        - users_first_name
    member_masking:
      includes: '*'
  
  - group: very_sensitive_data_access
    member_level:
      includes: "*"
    row_level:
      filters:
        - member: users_data_region
          operator: equals
          values:
            - RESEARCH
            - DEMO

The very_sensitive_data_access policy grants full access but only to rows matching the filter. Previously, the system saw "full access exists in some policy" and skipped masking entirely — exposing unmasked values for all rows, including those outside the filter.

Solution

The maskedMembers type is extended to support both simple strings and objects with filter info:

maskedMembers?: (string | { member: string; filter: any })[]

When a member has an associated filter, the generated SQL uses:

CASE WHEN {rowFilter} THEN {originalValue} ELSE {maskedValue} END

This ensures:

  • Rows matching the row filter see unmasked values (full access applies)
  • Rows outside the filter range see masked values (fallback to mask)

Tesseract Support

The Rust native SQL planner (MaskedSqlNode) is updated to support the new maskedMembers format with MaskedMemberItem enum (untagged serde), and generates CASE WHEN SQL for conditional masks.

Changes

File Change
packages/cubejs-server-core/src/core/CompilerApi.ts Distinguish unconditional vs conditional full access; emit maskedMembers with inline filter objects
packages/cubejs-schema-compiler/src/adapter/BaseQuery.js Parse both string and object formats from maskedMembers; add conditionalMemberMaskSql() and maskFilterToSql()
packages/cubejs-api-gateway/src/types/query.ts Update maskedMembers type to (string | { member, filter })[]
packages/cubejs-api-gateway/src/query.js Update Joi validation for new format
rust/.../base_query_options.rs Add MaskedMemberItem enum with untagged serde
rust/.../query_tools.rs Store mask filters alongside masked members set
rust/.../masked.rs Generate CASE WHEN for conditional masks in MaskedSqlNode
rust/.../test_fixtures/ Update to use MaskedMemberItem type
packages/cubejs-testing/.../conditional_masking_test.yaml New fixture for smoke test
packages/cubejs-testing/test/smoke-rbac.test.ts Add conditional masking smoke test
packages/cubejs-testing/birdbox-fixtures/rbac/cube.js Add conditional_mask_user auth
packages/cubejs-schema-compiler/test/unit/yaml-schema.test.ts 4 unit tests for conditional masking

Test Results

  • All 73 yaml-schema unit tests pass (including 4 new tests)
  • All 899 Rust unit tests pass
  • Smoke test added for integration validation (requires Docker)
Open in Web Open in Cursor 

@github-actions github-actions Bot added the javascript Pull requests that update Javascript code label May 3, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 3, 2026

Claude finished @paveltiunov's task in 4m 52s —— View job


Code Review: Conditional Data Masking with Row-Level Filters

  • Gather context and read all changed files
  • Review CompilerApi.ts changes (core logic)
  • Review BaseQuery.js changes (SQL generation)
  • Review API gateway changes (type/validation)
  • Review test coverage
  • Post detailed review feedback

Summary

The approach is sound — using CASE WHEN to conditionally mask based on row-level filters is the right pattern. The SQL generation in BaseQuery.js is clean, and the recursive maskFilterToSql handles nested and/or structures well. However, I found one critical logic bug and several smaller issues.

Critical

AND vs OR when combining filters across multiple conditional policies (CompilerApi.ts:718-729)

When multiple policies grant conditional full access (e.g., Policy A for data_region = 'RESEARCH', Policy B for data_region = 'DEMO'), the code flatMaps all filters into a flat list and combines them with { and: evaluatedFilters }. This produces:

CASE WHEN (data_region = 'RESEARCH' AND data_region = 'DEMO') THEN original ELSE masked END

This is always false — no row matches both. The correct behavior is OR across policies (any policy granting access suffices), AND within a policy (all filters in one policy must match). This is consistent with how row-level security elsewhere uses union (see the "R1 ∪ R2 rows visible" diagram at line 617). See inline comment with suggested fix.

Medium

  • Time dimension timezone handling (BaseQuery.js:3496-3504): The THEN branch uses autoPrefixAndEvaluateSql which doesn't apply convertTz, while the normal unmasked path for time dimensions does. This could produce inconsistent timezone results for conditionally masked time dimensions.

Minor / Suggestions

Issue Location Details
Unused parameter BaseQuery.js:3496 memberPathArray is accepted but never used in conditionalMemberMaskSql
Permissive validation query.js:199 Joi.any() for filter values — consider a recursive filter schema
Loose typing query.ts:170 Record<string, any> — consider defining a MaskFilter interface

Testing

The 4 new tests cover the core cases well (basic CASE WHEN, AND filters, custom mask, no-filter fallback). Missing scenarios:

  • Multiple conditional policies — would have caught the AND/OR bug
  • OR filter combinationsmaskFilterToSql supports them but no CompilerApi-level test exercises this path
  • Integration through CompilerApi.applyRLSandMasking() — current tests only exercise BaseQuery with pre-computed memberMaskFilters

What Looks Good

  • The conditionalMemberMaskSql / maskFilterToSql separation is clean and reuses initFilter for SQL generation
  • Defense-in-depth: even when row filters would exclude non-matching rows, the CASE WHEN provides a second layer of protection
  • The early-exit via hasUnconditionalFullAccess avoids unnecessary CASE WHEN overhead for the common case

Comment on lines +718 to +729
if (conditionalFullAccessPolicies.length > 0 && policiesWithMemberAccess.length > 0) {
maskedMembersSet.add(memberName);
const evaluatedFilters = conditionalFullAccessPolicies.flatMap(
policy => (policy.rowLevel.filters || []).map(
(filter: any) => this.evaluateNestedFilter(filter, cube, context, cubeEvaluator)
)
);
if (evaluatedFilters.length > 0) {
memberMaskFiltersMap[memberName] = evaluatedFilters.length === 1
? evaluatedFilters[0]
: { and: evaluatedFilters };
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug (Critical): Filters from multiple policies should be OR'd, not AND'd

When multiple conditional full-access policies exist (e.g., Policy A grants full access for data_region = 'RESEARCH' rows, Policy B grants full access for data_region = 'DEMO' rows), the member should be unmasked when either filter matches.

The current code flatMaps all filters from all policies into a single list and combines them with { and: evaluatedFilters }. This produces:

CASE WHEN (data_region = 'RESEARCH' AND data_region = 'DEMO') THEN ...

This is always false — no row can match both simultaneously.

The correct logic should OR across policies (any policy granting access is sufficient) while ANDing within a policy (all filters within one policy must match):

Suggested change
if (conditionalFullAccessPolicies.length > 0 && policiesWithMemberAccess.length > 0) {
maskedMembersSet.add(memberName);
const evaluatedFilters = conditionalFullAccessPolicies.flatMap(
policy => (policy.rowLevel.filters || []).map(
(filter: any) => this.evaluateNestedFilter(filter, cube, context, cubeEvaluator)
)
);
if (evaluatedFilters.length > 0) {
memberMaskFiltersMap[memberName] = evaluatedFilters.length === 1
? evaluatedFilters[0]
: { and: evaluatedFilters };
}
if (conditionalFullAccessPolicies.length > 0 && policiesWithMemberAccess.length > 0) {
maskedMembersSet.add(memberName);
const policyFilterGroups = conditionalFullAccessPolicies.map(
policy => {
const filters = (policy.rowLevel.filters || []).map(
(filter: any) => this.evaluateNestedFilter(filter, cube, context, cubeEvaluator)
);
return filters.length === 1 ? filters[0] : { and: filters };
}
);
if (policyFilterGroups.length > 0) {
memberMaskFiltersMap[memberName] = policyFilterGroups.length === 1
? policyFilterGroups[0]
: { or: policyFilterGroups };
}

This is consistent with how row-level security elsewhere in this file uses union (OR) between policies — see the "R1 ∪ R2 rows visible" diagram at line 617.

Fix this →

return this.defaultMaskSql(symbol.type);
}

conditionalMemberMaskSql(cubeName, name, symbol, memberPathArray, maskFilter) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Unused parameter memberPathArray

This parameter is accepted but never used in the method body. Consider removing it.

Suggested change
conditionalMemberMaskSql(cubeName, name, symbol, memberPathArray, maskFilter) {
conditionalMemberMaskSql(cubeName, name, symbol, maskFilter) {

subqueryJoins: Joi.array().items(subqueryJoin),
joinHints: Joi.array().items(joinHint),
maskedMembers: Joi.array().items(Joi.string()),
memberMaskFilters: Joi.object().pattern(Joi.string(), Joi.any()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Security: Joi.any() is very permissive for filter values

While memberMaskFilters is server-generated (from CompilerApi), Joi.any() provides no structural validation. If this ever becomes reachable from external input, arbitrary objects would pass validation.

Consider adding a recursive filter schema that validates the expected structure ({member, operator, values} or {and: [...]} / {or: [...]}), similar to how queryFilter is validated elsewhere in this file.

rowLimit?: null | number;
order?: { id: string; desc: boolean }[];
maskedMembers?: string[];
memberMaskFilters?: Record<string, any>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Tighten the type

Record<string, any> loses all type information about the filter structure. Consider defining a proper filter type:

interface MaskFilter {
  member?: string;
  operator?: string;
  values?: string[];
  and?: MaskFilter[];
  or?: MaskFilter[];
}

Then: memberMaskFilters?: Record<string, MaskFilter>;

Comment on lines +3496 to +3504
conditionalMemberMaskSql(cubeName, name, symbol, memberPathArray, maskFilter) {
const maskedSql = this.memberMaskSql(cubeName, name, symbol);
const filterSql = this.maskFilterToSql(maskFilter);
if (!filterSql) {
return maskedSql;
}
const originalSql = this.autoPrefixAndEvaluateSql(cubeName, symbol.sql);
return `CASE WHEN ${filterSql} THEN ${originalSql} ELSE ${maskedSql} END`;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: Time dimensions with conditional masking skip timezone conversion

When a time dimension has a conditional mask, the originalSql in the THEN branch is produced via autoPrefixAndEvaluateSql, which does not apply convertTz. The normal (unmasked) path for time dimensions applies convertTz (see around line 3456). This could produce inconsistent timezone behavior for conditionally masked time dimensions.

This may be an edge case in practice, but worth noting for correctness.

expect(sql).not.toContain('CASE WHEN');
expect(sql).toContain('NULL');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Testing gap: Missing test for multiple conditional policies

This test suite doesn't include a scenario where multiple conditional full-access policies contribute filters for the same member. Such a test would expose the AND vs OR issue noted in CompilerApi.ts.

Additionally, consider adding tests for:

  • or filter combinations (the maskFilterToSql method supports them but they're untested at the CompilerApi level)
  • Measures with conditional masking (interaction with ungrouped query path)
  • An integration test through CompilerApi.applyRLSandMasking() that exercises the full policy evaluation, not just BaseQuery with pre-computed memberMaskFilters

@github-actions github-actions Bot added the rust Pull requests that update Rust code label May 3, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 3, 2026

Codecov Report

❌ Patch coverage is 48.00000% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.79%. Comparing base (87f4dc1) to head (f9c94d0).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...ackages/cubejs-server-core/src/core/CompilerApi.ts 0.00% 21 Missing ⚠️
...es/cubejs-schema-compiler/src/adapter/BaseQuery.js 88.46% 3 Missing ⚠️
packages/cubejs-api-gateway/src/gateway.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10803      +/-   ##
==========================================
- Coverage   78.79%   78.79%   -0.01%     
==========================================
  Files         470      470              
  Lines       92135    92179      +44     
  Branches     3424     3436      +12     
==========================================
+ Hits        72597    72630      +33     
- Misses      19035    19046      +11     
  Partials      503      503              
Flag Coverage Δ
cube-backend 58.15% <48.00%> (+0.06%) ⬆️
cubesql 83.43% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@paveltiunov paveltiunov marked this pull request as ready for review May 4, 2026 17:01
@paveltiunov paveltiunov requested review from a team as code owners May 4, 2026 17:01
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

Claude finished @paveltiunov's task in 3m 44s —— View job


Code Review: Conditional Data Masking with Row-Level Filters

  • Read all changed files and gather context
  • Review core logic in CompilerApi.ts
  • Review SQL generation in BaseQuery.js
  • Review API gateway changes (types/validation)
  • Review Rust changes (Tesseract support)
  • Review test coverage
  • Check if previous review issues were addressed
  • Post comprehensive review

Overview

The approach is sound — using CASE WHEN to conditionally mask based on row-level filters is the right pattern for this problem. The implementation touches all the right layers (CompilerApi → BaseQuery/Tesseract → API types), and the skipMasking context flag to prevent infinite recursion is well-designed.

Critical

AND vs OR when combining filters across multiple conditional policies (CompilerApi.ts:724-732)

This was identified in the first review and remains unfixed. When multiple policies grant conditional full access, their filters are flatMap'd and combined with { and: ... }. This means if Policy A grants access for region = 'RESEARCH' and Policy B for region = 'DEMO', the result is region = 'RESEARCH' AND region = 'DEMO' — always false. The correct semantic is OR across policies, AND within a policy. A code suggestion with the fix is in the inline comment.

Medium

Time dimension timezone handling (BaseQuery.js:3509): The THEN branch uses autoPrefixAndEvaluateSql which doesn't apply convertTz, while the normal unmasked path for time dimensions does. Could produce inconsistent timezone results for conditionally masked time dimensions.

Minor / Suggestions

Issue Location Details
Unused parameter BaseQuery.js:3501 memberPathArray is accepted but never used
Validation removed query.js:197 Old Joi validation removed entirely rather than updated for new format
Loose typing query.ts:169 filter?: any — consider defining a MaskFilter interface
Limited filter context masked.rs:96-123 Rust compile_filter_to_sql uses empty CubeRefEvaluator — fine for simple members but won't resolve FILTER_PARAMS or {CUBE} refs

What Looks Good

  • Recursion protection is well-handled: skipMasking: true in conditionalMemberMaskSql prevents infinite loops when filter members are themselves masked (tested in the "does not recurse" test)
  • Clean separation of conditionalMemberMaskSql / maskFilterToSql — reuses initFilter and extractFiltersAsTree rather than building custom SQL generation
  • hasUnconditionalFullAccess early-exit avoids unnecessary CASE WHEN overhead for the common case where a policy grants unconditional access
  • Rust implementation mirrors the JS logic faithfully, using FilterCompiler for proper SQL generation
  • Defense-in-depth: even when row filters would exclude non-matching rows at the WHERE level, the CASE WHEN provides a second layer of masking protection

Testing

The 4 new unit tests cover the key scenarios well (basic CASE WHEN, AND filters, custom mask, no-filter fallback), plus a good recursion guard test. The smoke test provides integration coverage.

Missing test scenarios:

  • Multiple conditional policies for the same member — would catch the AND/OR bug
  • OR filter combinations at the CompilerApi level
  • Integration test through CompilerApi.applyRLSandMasking() (current tests exercise BaseQuery with pre-computed memberMaskFilters)

Summary

The core design is solid, but the AND/OR filter combination bug is critical and should be fixed before merging — it causes conditional masking to be overly aggressive (always masking) when multiple conditional policies exist. The other issues are lower severity and can be addressed at your discretion.

Comment on lines +724 to +732
const evaluatedFilters = conditionalFullAccessPolicies.flatMap(
policy => (policy.rowLevel.filters || []).map(
(filter: any) => this.evaluateNestedFilter(filter, cube, context, cubeEvaluator)
)
);
if (evaluatedFilters.length > 0) {
memberMaskFiltersMap[memberName] = evaluatedFilters.length === 1
? evaluatedFilters[0]
: { and: evaluatedFilters };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug (Critical): Filters from multiple conditional policies should be OR'd, not AND'd

This is the same issue identified in the previous review — it remains unfixed.

When multiple policies grant conditional full access (e.g., Policy A: data_region = 'RESEARCH', Policy B: data_region = 'DEMO'), the member should be unmasked when either filter matches.

flatMap flattens all filters from all policies into a single list and { and: evaluatedFilters } combines them all with AND. Example with two policies:

  • Policy A: filters = [region = 'RESEARCH', lock = 0]
  • Policy B: filters = [region = 'DEMO']

Current result: region = 'RESEARCH' AND lock = 0 AND region = 'DEMO' — always false.

Correct result: (region = 'RESEARCH' AND lock = 0) OR (region = 'DEMO')

The correct logic is: AND within a single policy's filters, OR across policies. This is consistent with how row-level security uses union (OR) between policies — see the "R1 ∪ R2 rows visible" diagram at line 618.

Suggested change
const evaluatedFilters = conditionalFullAccessPolicies.flatMap(
policy => (policy.rowLevel.filters || []).map(
(filter: any) => this.evaluateNestedFilter(filter, cube, context, cubeEvaluator)
)
);
if (evaluatedFilters.length > 0) {
memberMaskFiltersMap[memberName] = evaluatedFilters.length === 1
? evaluatedFilters[0]
: { and: evaluatedFilters };
const policyFilterGroups = conditionalFullAccessPolicies.map(
policy => {
const filters = (policy.rowLevel.filters || []).map(
(filter: any) => this.evaluateNestedFilter(filter, cube, context, cubeEvaluator)
);
return filters.length === 1 ? filters[0] : { and: filters };
}
);
if (policyFilterGroups.length > 0) {
memberMaskFiltersMap[memberName] = policyFilterGroups.length === 1
? policyFilterGroups[0]
: { or: policyFilterGroups };
}

return this.defaultMaskSql(symbol.type);
}

conditionalMemberMaskSql(cubeName, name, symbol, memberPathArray, maskFilter) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Unused parameter memberPathArray

This parameter is accepted but never referenced in the method body.

Suggested change
conditionalMemberMaskSql(cubeName, name, symbol, memberPathArray, maskFilter) {
conditionalMemberMaskSql(cubeName, name, symbol, maskFilter) {

And the call site at line 3319 should be updated accordingly:

return this.conditionalMemberMaskSql(cubeName, name, symbol, maskFilter);

if (!filterSql) {
return maskedSql;
}
const originalSql = this.autoPrefixAndEvaluateSql(cubeName, symbol.sql);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Medium: Time dimensions with conditional masking skip timezone conversion

The originalSql in the THEN branch uses autoPrefixAndEvaluateSql, which does not apply convertTz. The normal (unmasked) path for time dimensions calls convertTz (around line 3465). If a time dimension has a conditional mask, the THEN branch will return a raw timestamp while the unmasked path would return a converted one.

This may be rare in practice (masking time dimensions conditionally), but could produce inconsistent timezone behavior when it does happen.

@@ -195,7 +195,7 @@ const querySchema = Joi.object().keys({
responseFormat: Joi.valid('default', 'compact', 'columnar'),
subqueryJoins: Joi.array().items(subqueryJoin),
joinHints: Joi.array().items(joinHint),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: Validation removed entirely rather than updated

The old maskedMembers: Joi.array().items(Joi.string()) was removed with no replacement for the new { member, filter? }[] format. I understand maskedMembers is server-generated (from CompilerApi), so this is low risk. However, if there's any path where it could be influenced by external input, having no validation at all is less safe than even a basic structural check.

If you want to keep validation:

maskedMembers: Joi.array().items(Joi.object({
  member: Joi.string().required(),
  filter: Joi.any(),
})),

rowLimit?: null | number;
order?: { id: string; desc: boolean }[];
maskedMembers?: string[];
maskedMembers?: { member: string; filter?: any }[];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Define a proper type for the filter

filter?: any loses all type safety. Consider defining a recursive MaskFilter interface:

interface MaskFilter {
  member?: string;
  operator?: string;
  values?: string[];
  and?: MaskFilter[];
  or?: MaskFilter[];
}

Then: maskedMembers?: { member: string; filter?: MaskFilter }[];

This would help catch bugs at compile time (e.g., the AND/OR issue in CompilerApi could be type-checked more easily).

Comment on lines +96 to 123
let filter_item = {
let mut compiler = query_tools.evaluator_compiler().borrow_mut();
let mut filter_compiler = FilterCompiler::new(&mut compiler, query_tools.clone());
filter_compiler.add_item(native_filter)?;
let (dimension_filters, _, _) = filter_compiler.extract_result();
if dimension_filters.is_empty() {
return Ok(None);
}
if dimension_filters.len() == 1 {
dimension_filters.into_iter().next().unwrap()
} else {
FilterItem::Group(Rc::new(crate::plan::filter::FilterGroup::new(
crate::plan::filter::FilterGroupOperator::And,
dimension_filters,
)))
}
};
let context = Rc::new(VisitorContext::new_with_node_processor(
query_tools.clone(),
self.input.clone(),
));
let sql = filter_item.to_sql(templates, context)?;
if sql.is_empty() {
Ok(None)
} else {
Ok(Some("(NULL)".to_string()))
Ok(Some(sql))
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note: compile_filter_to_sql uses a fresh VisitorContext without the main query's filter context

The new VisitorContext::new_with_node_processor creates a context with an empty CubeRefEvaluator and no all_filters. This is intentional since we just need member resolution for the mask filter, but it means:

  1. FILTER_PARAMS references inside the mask filter's member SQL won't resolve (they'll get empty CubeRefEvaluator)
  2. If a member's SQL uses {CUBE} references via cube_ref_evaluator, they won't resolve in this context

This is probably fine for typical filter members (simple column references), but could be surprising for members with complex SQL definitions. Worth documenting as a known limitation.

Comment on lines +2014 to +2057
it('does not recurse when filter member is also masked', async () => {
const compilers = prepareYamlCompiler(`
cubes:
- name: items
sql_table: public.items
dimensions:
- name: id
sql: id
type: number
primary_key: true
- name: product_id
sql: product_id
type: number
- name: price
sql: price
type: number
mask: -1
measures:
- name: count
type: count
`);

await compilers.compiler.compile();

const query = new PostgresQuery(compilers, {
measures: ['items.count'],
dimensions: ['items.product_id', 'items.price'],
maskedMembers: [
{
member: 'items.product_id',
filter: { member: 'items.product_id', operator: 'lte', values: ['3'] }
},
{
member: 'items.price',
filter: { member: 'items.product_id', operator: 'lte', values: ['3'] }
},
],
});
const [sql] = query.buildSqlAndParams();
expect(sql).toContain('CASE WHEN');
expect(sql).toMatch(/product_id/);
expect(sql).not.toMatch(/Maximum call stack/);
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good: Infinite recursion test

This test is well-constructed — it verifies the skipMasking context flag works when the filter's member (items.product_id) is itself masked. This was a real bug that was caught and fixed in commit 169c640.

Missing test: Consider adding a test for multiple conditional policies with different filters for the same member — this would exercise the AND/OR combination logic in CompilerApi.ts and would catch the bug noted above.

cursoragent and others added 7 commits May 5, 2026 00:07
When an access policy grants full member_level access with row_level
filters, and another policy provides masked access, the masking should
be conditional on the row filter. Previously, masking was skipped
entirely when any policy granted full access, even if that access was
restricted by row-level filters.

Now, masked members with conditional full access generate:
  CASE WHEN {rowFilter} THEN {originalValue} ELSE {maskedValue} END

This ensures that rows matching the row filter see unmasked values,
while rows outside the filter range see masked values.

Changes:
- CompilerApi: Distinguish unconditional vs conditional full access
  when determining masking. Track row filters for conditionally
  accessible members as memberMaskFilters.
- BaseQuery: Add conditionalMemberMaskSql() and maskFilterToSql()
  methods to generate CASE WHEN SQL for masked members with
  associated row filters.
- NormalizedQuery type: Add memberMaskFilters field.
- Tests: Add 4 new tests for conditional masking behavior.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Instead of a separate memberMaskFilters field, extend maskedMembers to
support both string and {member, filter} object formats:
  maskedMembers: (string | { member: string, filter: FilterItem })[]

This ensures Tesseract/native SQL planner also supports conditional
masking. The Rust MaskedSqlNode now generates CASE WHEN SQL when a
member has an associated mask filter.

Changes:
- CompilerApi: Emit maskedMembers with inline filter objects
- BaseQuery.js: Parse both formats from maskedMembers array
- Rust (base_query_options.rs): Add MaskedMemberItem enum (untagged)
- Rust (query_tools.rs): Store mask filters alongside masked members
- Rust (masked.rs): Generate CASE WHEN for conditional masks
- Rust test fixtures: Update to use MaskedMemberItem type
- NormalizedQuery type: Update maskedMembers type signature
- query.js: Update Joi validation for new format
- smoke-rbac.test.ts: Add conditional masking test case
- conditional_masking_test.yaml: Add fixture for smoke test

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
- Fix infinite recursion in conditionalMemberMaskSql by adding
  skipMaskFor context guard to prevent re-entrance
- Fix CompilerApi logic: only apply conditional masking when a
  memberMasking policy actually exists (prevents false positive
  masking for cubes that only have row_level filters without
  memberMasking)
- Fix lint: object-property-newline and object-curly-spacing in tests
- Run cargo fmt on all Rust files

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
BaseQuery.js: Replace custom and/or rendering in maskFilterToSql with
the standard extractFiltersAsTree + initFilter + filterToWhere() pipeline
that all other filters use.

Rust MaskedSqlNode: Replace custom render_native_filter +
render_filter_condition with FilterCompiler::add_item +
FilterItem::to_sql using a standard VisitorContext - the same approach
used by QueryProperties and Select for all other filter rendering.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
- CompilerApi.ts: Replace continue statements with nested if/else
  to satisfy the no-continue lint rule
- Rust MaskedSqlNode: Fix RefCell already borrowed panic by using
  VisitorContext::new_with_node_processor with self.input (the
  underlying EvaluateSqlNode) instead of SqlNodesFactory::new()
  which creates a node processor chain that includes MaskedSqlNode,
  causing re-entrant borrow when evaluating filter member SQL
- Add VisitorContext::new_with_node_processor constructor for
  creating contexts with custom node processors

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
The stack overflow occurred when a mask filter referenced a member that
was itself masked (e.g., product_id has a mask filter on product_id).
The evaluateSymbolSql for the filter member would re-enter the masking
code, creating infinite recursion.

Fix: Use skipMasking context flag to disable all masking during CASE
WHEN evaluation (both the filter SQL and the original value SQL).
Also set currentMember to null to prevent memberChildren cycle tracking
that caused hasMultiStageMembers infinite loop.

Added test case that verifies no recursion when filter member is masked.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
…lidation

- maskedMembers now always uses { member: string, filter?: FilterItem }
  objects instead of string | object alternatives
- Remove maskedMembers from Joi query validation schema since users
  should never provide masking params in the query directly
- Rust MaskedMemberItem: replace untagged enum with simple struct
- Update all consumers and tests to use object format

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
cursoragent and others added 5 commits May 5, 2026 00:07
The normalizeQuery validation runs after applyRowLevelSecurity sets
maskedMembers on the query, so the field must be allowed in the schema.
Use Joi.array().items(Joi.object()) to accept the internal format
without exposing strict typing to users.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
- Strip maskedMembers from user query before first normalizeQuery
  validation so users cannot inject masking params
- After queryRewrite, always restore maskedMembers from the post-RLS
  query, ensuring rewrites cannot override RLS-determined masking

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Instead of silently stripping maskedMembers, throw a UserError if the
user attempts to pass it in their query. The Joi schema keeps the
correct type definition for internal validation after RLS sets it.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
BaseQuery.js: Use caseWhenStatement() method (same template used by
measure filters and dimension case types) instead of inline template
string.

Rust MaskedSqlNode: Use templates.case() from PlanSqlTemplates (same
template used by CaseSqlNode for dimension case rendering) instead of
inline format! string.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Instead of manually joining clauses with ' AND ', create a proper
and-group filter via newGroupFilter when there are multiple filter
items, reusing the standard BaseGroupFilter.filterToWhere() rendering.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@cursor cursor Bot force-pushed the cursor/data-masking-row-filter-case-when-032b branch from 964b369 to 2835b7d Compare May 5, 2026 00:07
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@cursor cursor Bot force-pushed the cursor/data-masking-row-filter-case-when-032b branch from d42113f to 23e71cb Compare May 6, 2026 00:49
cursoragent and others added 3 commits May 6, 2026 02:11
When multiple policies grant conditional full access, the member should
be unmasked when ANY policy's filter matches (OR), not when ALL match
(AND). Within a single policy, multiple filters are still AND'd.

Example with two policies:
  Policy A: filters = [region = 'RESEARCH', lock = 0]
  Policy B: filters = [region = 'DEMO']

Before: region = 'RESEARCH' AND lock = 0 AND region = 'DEMO' (always false)
After:  (region = 'RESEARCH' AND lock = 0) OR (region = 'DEMO')

This is consistent with row-level security which uses union (OR) across
policies for row visibility.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
Adds a test that verifies when a user has two conditional mask roles:
  - conditional_mask_role: product_id <= 3
  - conditional_mask_role_extra: product_id = 5

The price is unmasked when EITHER filter matches (OR across policies),
confirming the fix for the AND vs OR bug.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
…berMaskSql

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@paveltiunov paveltiunov merged commit 4d3bcbe into master May 6, 2026
128 of 131 checks passed
@paveltiunov paveltiunov deleted the cursor/data-masking-row-filter-case-when-032b branch May 6, 2026 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants