fix(policy): allow bare references to approved functions in SpEL conditions#28946
Conversation
…itions The AST-based ExpressionValidator (introduced in #27987) rejected policy/alert conditions that use the bare, parenthesis-less form of a boolean function (e.g. `!isOwner`, `noOwner || isOwner`). SpEL parses these as PropertyOrFieldReference nodes, which were not on the node allowlist, so creating or updating any policy containing such a rule failed with "disallowed SpEL construct: PropertyOrFieldReference ('isOwner')". The bare form is functionally identical to the parenthesised call - SpEL resolves it to the same no-arg boolean getter - and is documented as valid in RuleEvaluator's @function examples, so existing policies from older releases use it. Allow standalone PropertyOrFieldReference nodes, gated on the same ALLOWED_FUNCTIONS allowlist used for method names. Compound references (e.g. `System.exit(0)`) parse as CompoundExpression and remain blocked, so no security case from #27987 is reopened. Adds unit tests for bare approved/disallowed references and an integration test covering create and update of a policy with a bare condition.
yan-3005
added
safe to test
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR updates the backend SpEL policy/alert expression safety validator to accept bare (parenthesis-less) references to approved @Function methods (e.g. !isOwner, noOwner || isOwner), restoring compatibility with legacy policies while keeping the same function-name allowlist gate.
Changes:
- Allowlist SpEL
PropertyOrFieldReferenceAST nodes and gate their names using the existingALLOWED_FUNCTIONSallowlist (same as method calls). - Add unit coverage ensuring bare references to approved functions are accepted and unknown bare references are rejected.
- Add an integration regression test validating both create and update of policies using bare-reference conditions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/ExpressionValidator.java | Allow bare function references by allowlisting PropertyOrFieldReference and applying the same name-gating as for MethodReference. |
| openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/ExpressionValidatorTest.java | Adds unit tests for allowed/disallowed bare references. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/PolicyResourceIT.java | Adds create+update integration regression test for bare-reference policy conditions. |
Address review: bare PropertyOrFieldReference acceptance was gated on the full ALLOWED_FUNCTIONS set, so a bare arg-taking function (e.g. `matchAnyTag`) passed validation but would fail at SpEL evaluation since it has no matching no-arg getter. Build a separate ALLOWED_BARE_FUNCTIONS set from @function methods that take no arguments and return boolean, and gate bare references on it. Method-call form is unaffected (still gated on ALLOWED_FUNCTIONS). All legitimate bare conditions (isOwner, noOwner, hasDomain, matchTeam, isReviewer, ...) remain valid; nonsensical bare arg-taking references are now rejected up front.
🟡 Playwright Results — all passed (9 flaky)✅ 3470 passed · ❌ 0 failed · 🟡 9 flaky · ⏭️ 80 skipped
🟡 9 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
Code Review ✅ Approved 1 resolved / 1 findingsAllows bare references to approved functions in SpEL conditions by updating the AST validator, resolving the legacy validation failure for no-arg boolean getters. ✅ 1 resolved✅ Edge Case: Bare refs to arg-requiring functions pass validation but fail at runtime
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Failed to cherry-pick changes to the 1.13 branch. |
|
Failed to cherry-pick changes to the 1.12.11 branch. |
…itions (#28946) * fix(policy): allow bare references to approved functions in SpEL conditions The AST-based ExpressionValidator (introduced in #27987) rejected policy/alert conditions that use the bare, parenthesis-less form of a boolean function (e.g. `!isOwner`, `noOwner || isOwner`). SpEL parses these as PropertyOrFieldReference nodes, which were not on the node allowlist, so creating or updating any policy containing such a rule failed with "disallowed SpEL construct: PropertyOrFieldReference ('isOwner')". The bare form is functionally identical to the parenthesised call - SpEL resolves it to the same no-arg boolean getter - and is documented as valid in RuleEvaluator's @function examples, so existing policies from older releases use it. Allow standalone PropertyOrFieldReference nodes, gated on the same ALLOWED_FUNCTIONS allowlist used for method names. Compound references (e.g. `System.exit(0)`) parse as CompoundExpression and remain blocked, so no security case from #27987 is reopened. Adds unit tests for bare approved/disallowed references and an integration test covering create and update of a policy with a bare condition. * fix(policy): restrict bare references to no-arg boolean functions Address review: bare PropertyOrFieldReference acceptance was gated on the full ALLOWED_FUNCTIONS set, so a bare arg-taking function (e.g. `matchAnyTag`) passed validation but would fail at SpEL evaluation since it has no matching no-arg getter. Build a separate ALLOWED_BARE_FUNCTIONS set from @function methods that take no arguments and return boolean, and gate bare references on it. Method-call form is unaffected (still gated on ALLOWED_FUNCTIONS). All legitimate bare conditions (isOwner, noOwner, hasDomain, matchTeam, isReviewer, ...) remain valid; nonsensical bare arg-taking references are now rejected up front. # Conflicts: # openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/ExpressionValidatorTest.java
…itions (#28946) * fix(policy): allow bare references to approved functions in SpEL conditions The AST-based ExpressionValidator (introduced in #27987) rejected policy/alert conditions that use the bare, parenthesis-less form of a boolean function (e.g. `!isOwner`, `noOwner || isOwner`). SpEL parses these as PropertyOrFieldReference nodes, which were not on the node allowlist, so creating or updating any policy containing such a rule failed with "disallowed SpEL construct: PropertyOrFieldReference ('isOwner')". The bare form is functionally identical to the parenthesised call - SpEL resolves it to the same no-arg boolean getter - and is documented as valid in RuleEvaluator's @function examples, so existing policies from older releases use it. Allow standalone PropertyOrFieldReference nodes, gated on the same ALLOWED_FUNCTIONS allowlist used for method names. Compound references (e.g. `System.exit(0)`) parse as CompoundExpression and remain blocked, so no security case from #27987 is reopened. Adds unit tests for bare approved/disallowed references and an integration test covering create and update of a policy with a bare condition. * fix(policy): restrict bare references to no-arg boolean functions Address review: bare PropertyOrFieldReference acceptance was gated on the full ALLOWED_FUNCTIONS set, so a bare arg-taking function (e.g. `matchAnyTag`) passed validation but would fail at SpEL evaluation since it has no matching no-arg getter. Build a separate ALLOWED_BARE_FUNCTIONS set from @function methods that take no arguments and return boolean, and gate bare references on it. Method-call form is unaffected (still gated on ALLOWED_FUNCTIONS). All legitimate bare conditions (isOwner, noOwner, hasDomain, matchTeam, isReviewer, ...) remain valid; nonsensical bare arg-taking references are now rejected up front. # Conflicts: # openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/ExpressionValidatorTest.java
Fixes #28947
Problem
Creating or updating any policy whose rule condition uses the bare, parenthesis-less form of a boolean function fails with:
Reported by a SaaS customer (Orsted, P0). Their roles have policies — created on older releases — whose rules use conditions like
!isOwner,noOwner || isOwner, etc. Adding or deleting a rule re-validates every rule in the policy, so the legacy bare conditions block the whole operation.Root cause
ExpressionValidatorwas rewritten in #27987 from a regex denylist to an AST allowlist (ALLOWED_NODE_CLASSES, default-deny). SpEL parsesisOwner()as aMethodReference(allowlisted) but parses the bareisOwneras aPropertyOrFieldReference, which was not on the allowlist — so the bare form is rejected.The bare form is functionally identical to the parenthesised call: SpEL's read-only data binding resolves the property
isOwnerto the no-arg boolean getterisOwner(). It is also documented as valid inRuleEvaluator's own@Functionexamples ("!isOwner","!noOwner"), so policies authored on older releases use it.Fix
Allow standalone
PropertyOrFieldReferencenodes, gated on the sameALLOWED_FUNCTIONSallowlist already used for method names (ensureCallableNameAllowed). This is generic — it covers every approved@Functionin bare form (isOwner,noOwner,isReviewer,matchTeam,hasDomain, …), not justisOwner.Security — no regression from #27987
Compound references such as
System.exit(0),Runtime.getRuntime().exec(...),entity.getClass()parse asCompoundExpression/TypeReference/ConstructorReference— none of which are allowlisted — so they remain blocked, and a bare disallowed name (System,entity) is rejected by the name gate. All existing negative tests still pass; the!System.exit(0)regression guard stays green.Tests
ExpressionValidatorTest): bare approved references (!isOwner,!noOwner,isReviewer,noOwner() || isOwner, …) accepted; bare disallowed names (System,entity,someObject,Runtime) rejected. Verified these fail without the production change.PolicyResourceIT): create and update a policy with a bare-reference condition.ExpressionValidatorTest: 34/34 pass.RuleEvaluatorTest(runtime evaluation): 21/21 pass.mvn spotless:applyclean.