Skip to content

feat(policy)!: harden and finalize the policy contract - #109

Merged
Benoît Cortier (CBenoit) merged 14 commits into
masterfrom
cbenoit-reject-duplicate-policy-members
Sep 17, 2026
Merged

Benoît Cortier (CBenoit) merged 14 commits into
masterfrom
cbenoit-reject-duplicate-policy-members

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Sep 15, 2026

Copy link
Copy Markdown
Member

Summary

  • reject duplicate JSON property names before typed .NET policy/broker deserialization, including nested and Unicode-escape-equivalent names; all policy input paths also reject unknown members
  • expose one strict-by-definition .NET policy serializer surface: PolicySerializer.Options, Deserialize<T>, and the document ParseJson helpers; remove redundant strict/non-strict model contexts and entry points
  • replace the eight boolean match arrays with optional scalar booleans (null/omitted = unrestricted, canonical output omitted)
  • canonicalize empty collection filters to omitted output; reject duplicate collection values consistently across schemas/Rust/.NET; preserve effective-nonempty persisted rules
  • enforce operational validity windows: when both metadata bounds are present, ValidFrom must be strictly earlier than ValidUntil by normalized instant
  • make Constraints valid only on Allow rules
  • remove serialized PolicyType, RulePrecedence, and PackageNames; precedence is fixed as lower priority first, Deny wins Allow/Deny ties, then document order
  • rename Elevation to ExecutionElevation and Sources to exact SourceNames; runtime collection bounds match the published schemas, and nonempty source names require exactly one manager
  • make package identifiers explicit Exact or Patterns modes and versions explicit Exact or semantic Range modes; provide atomic .NET mode-switch APIs
  • regenerate policy/draft schemas and OpenAPI 3.1 schemas with JSON Schema null unions (no legacy nullable)

PolicyFormatVersion remains in the compatible 1.x line. This is an intentional pre-release package/API break with no legacy aliases or conversion.

Canonical semantics

  • boolean condition omitted or null: does not narrow; false/true: exact request characteristic
  • collection omitted or []: does not narrow; canonical output omits empty collections; duplicate values are invalid
  • validity bound omitted or null: that side is unbounded; canonical output omits it; equal or inverted two-sided windows are invalid
  • brokers reject operations before ValidFrom or after ValidUntil on every request, with no fallback policy
  • PackageIdentifiers.Exact: validated stable identifiers; wildcard characters rejected
  • PackageIdentifiers.Patterns: explicit wildcard patterns that may authorize multiple identifiers
  • Version.Exact: one or more arbitrary real package version strings; Version.Range: semantic versions only
  • SourceNames: exact configured source names, no URLs/pattern matching, exactly one selected manager, at most 128 names
  • Managers: at most 16 distinct manager values
  • ExecutionElevation: Elevated when scope is Machine or requested elevation is Elevated; Standard otherwise

Consumer migration

Gateway

  • remove construction/references for PolicyType, RulePrecedence, PackageNames, Sources, Elevation, Versions, and VersionRange
  • replace removed model serializer APIs with PolicySerializer.Deserialize<T> or the document ParseJson helpers
  • preserve fixed evaluator precedence and remove the old PackageNames fail-closed branch/tests
  • assume constraints are absent on Deny rules
  • compute ExecutionElevation exactly from Machine scope or requested elevation
  • implement SourceNames, package identifier modes, and version modes
  • retain per-request UTC validity enforcement with no fallback and add exact ValidFrom/ValidUntil boundary tests if missing

UniGetUI

  • remove old field display/edit/help; show constraints only for Allow rules
  • replace removed model serializer APIs with PolicySerializer.Deserialize<T> or the document ParseJson helpers
  • keep friendly-name search only as a UI lookup that resolves stable identifiers
  • auto-generate unique priorities by visible order
  • use date/time controls with inline ValidFrom < ValidUntil validation
  • incomplete blank rules may exist transiently in the editor but must never be serialized/saved

Validation

  • .NET 9: Model 174/174, Client 262/262
  • .NET 10: Model 174/174, Client 262/262
  • Rust: now-policy 36/36, now-policy-api 18/18, server 7/7, server samples 28/28
  • clippy -D warnings, dotnet/cargo formatting, deterministic schema/OpenAPI regeneration
  • Cargo package verification for model/API/server and NuGet pack for Model/API/Client
  • reflection-disabled NativeAOT smoke covering duplicate rejection, validity windows, and canonical contract paths
  • GPT-6 Astra full review and follow-ups; all material findings addressed, final confirmation clean

Release impact

Required coordinated Rust releases: now-policy 0.5.0, now-policy-api 0.6.0 (updating its now-policy dependency), and now-policy-server-template 0.6.0. Publish the Model/API/Client NuGets together using the next date-based workflow version (validated with 2026.09.17.0, normalized to 2026.9.17). No release, package publication, merge, or auto-merge is included.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 15, 2026 18:46

Copilot AI left a comment

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.

🟡 Changes recommended

Public broker options still accept duplicates when directly deserializing several exposed nested DTO types.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds strict duplicate JSON-member rejection across policy and broker deserialization, with shared Rust/.NET fixtures and client error handling.

Changes:

  • Adds duplicate-name scanning and serializer converters.
  • Covers nested, escaped, and conflicting duplicates.
  • Adds cross-language and broker-client tests.
File summaries
File Description
policies/rust/now-policy/tests/policy_samples.rs Tests duplicate fixtures.
policies/rust/now-policy/assets/samples/invalid/duplicates/duplicate-rule-id.policy.json Nested rule duplicate fixture.
.../duplicate-policy-format-version-same.policy.json Equal-value duplicate fixture.
.../duplicate-policy-format-version-escaped.policy.json Escaped-name duplicate fixture.
.../duplicate-policy-format-version-conflicting.policy.json Conflicting duplicate fixture.
.../duplicate-metadata-publisher.policy.json Metadata duplicate fixture.
.../duplicate-metadata-id.policy.json Escaped metadata duplicate fixture.
.../duplicate-match-field.policy.json Match duplicate fixture.
.../duplicate-constraints-field.policy.json Constraints duplicate fixture.
policies/dotnet/Devolutions.Now.Policy.Model/README.md Documents strict parsing.
...Model/PolicySerializer.cs Integrates duplicate rejection.
...Model/PolicyJsonInput.cs Implements scanner and converter.
...Model.Tests/PolicyTests.cs Tests policy entry points.
...Client.Tests/PolicyManagementClientTests.cs Tests management and error responses.
...Client.Tests/BrokerClientTests.cs Tests client response rejection.
...Api/README.md Documents broker behavior.
...Api/BrokerSerializer.cs Integrates broker converters.
Review details
  • Files reviewed: 17/17 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs
Comment thread policies/dotnet/Devolutions.Now.Policy.Model/README.md Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🟢 Approval recommended

The implementation consistently covers documented entry points, nested objects, escaped names, caller options, AOT operation, and client error handling with comprehensive tests.

Review details
  • Files reviewed: 17/17 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@CBenoit
Benoît Cortier (CBenoit) marked this pull request as ready for review September 15, 2026 19:08
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit Benoît Cortier (CBenoit) changed the title fix(policy): reject duplicate JSON members feat(policy)!: harden and finalize the policy contract Sep 17, 2026

Copilot AI left a comment

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.

🟡 Changes recommended

The invalid validation-response fixture still exposes removed match paths and boolean-array semantics.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 54/54 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

Duplicate collection values have inconsistent validity and canonicalization across schemas, Rust, and .NET.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

policies/rust/now-policy/src/policy.rs:640

  • This cardinality check runs after Managers has been deserialized into a BTreeSet, so {"Managers":["Winget","Winget"],"SourceNames":["corp"]} collapses to one manager and is accepted by Rust. The generated schema rejects that array via uniqueItems, and .NET sees Count == 2 and rejects it, so the new “exactly one manager” rule differs across implementations. Reject duplicate manager elements before set conversion (or otherwise align all three contracts).
    policies/rust/now-policy/src/policy.rs:811
  • uniqueItems: true makes duplicate exact identifiers invalid under the schema, but both typed readers accept them: Rust checks the Vec length and then silently deduplicates it into a BTreeSet (lines 748-752), while .NET validates count/characters without checking uniqueness (PolicySerializer.cs:191-197) and preserves duplicates. This gives the same document different validity and canonical output across schema, Rust, and .NET. Either reject duplicate elements in both readers or remove this constraint and define one canonicalization behavior.

This issue also appears in the following locations of the same file:

  • line 825
  • line 913

policies/rust/now-policy/src/policy.rs:825

  • The schema rejects duplicate Patterns, but Rust accepts and deduplicates them when collecting the input Vec into a BTreeSet (lines 753-756), while .NET accepts and retains them (PolicySerializer.cs:199-205). Align runtime validation and canonicalization with uniqueItems, or remove the schema restriction consistently.
    policies/rust/now-policy/src/policy.rs:913
  • Version.Exact has the same contract mismatch: the schema requires unique values, Rust silently deduplicates duplicate input at lines 856-860, and .NET accepts and preserves duplicates (PolicySerializer.cs:236-243). This prevents consistent validation and canonical output; make both readers reject duplicates or remove uniqueItems and standardize canonicalization.
  • Files reviewed: 54/54 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🟡 Changes recommended

Version ranges still accept empty or non-SemVer boundaries across both model implementations and generated schemas.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 54/54 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs
Comment thread policies/rust/now-policy/src/policy.rs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🟡 Changes recommended

Public broker serializer options currently bypass standalone VersionRange semantic validation.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 54/54 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread policies/dotnet/Devolutions.Now.Policy.Api/BrokerSerializer.cs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

Rust, .NET, and generated schemas disagree on accepted semantic-version boundaries, leaving a cross-platform contract inconsistency.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

policies/rust/now-policy/src/newtypes.rs:45

  • This schema regex accepts numeric components larger than u64 (for example 1.18446744073709551616.0), while SemanticVersion::parse delegates to semver::Version::parse, whose major/minor/patch fields are u64, so Rust rejects the same value. The new .NET regex also accepts it. This leaves the generated schemas and .NET contract accepting a version that Rust rejects; align the numeric bounds or use the same SemVer validation semantics across implementations.
    policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs:385
  • ExecutionElevation now represents effective execution privilege, but its referenced Elevation enum is still documented as “Requested elevation level” in both Enums.cs:85 and policies/rust/now-policy/src/enums.rs:64; that stale text is also emitted for PolicyModelElevation in the generated schemas. Update or rename the model enum so generated API documentation does not describe this effective value as a client request.
  • Files reviewed: 54/54 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

Rust condition serializers can emit schema-invalid pattern and version strings constructed through public newtype fields.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

policies/rust/now-policy/src/policy.rs:829

  • StringPattern exposes its inner String, so callers can construct an empty or oversized pattern without parse. This branch checks only the collection size and then serializes those invalid elements, producing JSON rejected by the generated schema and by deserialization. Revalidate every pattern on output, as the other new condition serializers do for their mutable invariants.
    policies/rust/now-policy/src/policy.rs:927
  • VersionString has a public inner String, so a programmatically constructed VersionCondition::Exact can contain empty or over-128-scalar values. This serializer validates only the number of entries and emits those invalid strings, despite the new input/output invariant and schema bounds. Revalidate each exact version before serializing.
  • Files reviewed: 55/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

Rust and .NET accept more than 128 source names despite generated schemas rejecting them.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs:173

  • The generated policy schemas cap SourceNames at 128 items, but model validation only checks duplicates and each string's length. Thus .NET accepts and serializes 129 unique source names that schema-based consumers reject. Add the collection-count check here to keep runtime and schema validation aligned.
    policies/rust/now-policy/src/policy.rs:669
  • SourceNames is declared with maxItems: 128, but this deserializer only checks duplicates and per-name validity. A policy containing 129 unique source names is therefore accepted by Rust while the committed schemas reject it. Enforce the collection bound before converting the vector to a set.
  • Files reviewed: 55/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

Runtime manager-list validation remains inconsistent with the 16-item limit published by the generated schemas.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

policies/dotnet/Devolutions.Now.Policy.Model/PolicySerializer.cs:170

  • ManagerName has 17 variants (Enums.cs:46-64), but the generated schemas cap Managers at 16 while this validation only rejects duplicates. A policy containing all 17 distinct managers is therefore accepted and serialized by the runtime but rejected by schema clients. Align the contract by either enforcing 16 in both runtimes or raising every generated maxItems to 17, and add a parity test.
  • Files reviewed: 55/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

Mutable package and version mode objects retain stale presence flags, preventing valid in-place mode changes.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs:459

  • ExactSpecified and RangeSpecified are permanent latches, so a consumer cannot switch this public mutable condition from Exact to Range: after setting Exact = null and assigning Range, both flags remain true and every serializer/conversion rejects the object even though null properties are omitted from its JSON. Track property presence only while deserializing (for example in a converter), or provide an atomic mode-switch API that clears the previous value/state while still rejecting documents that contain both properties.

This issue also appears on line 492 of the same file.

policies/dotnet/Devolutions.Now.Policy.Model/PolicyModels.cs:496

  • These presence flags never reset, so changing a valid public condition from Exact to Patterns by clearing Exact and assigning Patterns leaves both flags true. Serialization and draft conversion then reject an object whose canonical JSON would contain only Patterns. Separate wire-property presence tracking from mutable model state, or expose an atomic mode-switch API that clears the stale mode without accepting both properties from JSON.
        set
        {
            ExactSpecified = true;
            _exact = value;
        }
  • Files reviewed: 55/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Also add atomic .NET condition mode switching and preserve structured validation paths across broker response nesting.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

The coordinated breaking contract changes span security-sensitive deserialization, two runtimes, generated schemas, and numerous consumer-facing APIs.

Review details
  • Files reviewed: 55/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@CBenoit
Benoît Cortier (CBenoit) marked this pull request as ready for review September 17, 2026 07:08
@CBenoit
Benoît Cortier (CBenoit) marked this pull request as draft September 17, 2026 09:25
Remove the redundant strict/non-strict model contexts and entry points now that every policy input uses the closed contract.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

The coordinated breaking wire-contract, serializer, and schema changes across Rust and .NET warrant final human integration review.

Review details
  • Files reviewed: 55/55 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@CBenoit
Benoît Cortier (CBenoit) marked this pull request as ready for review September 17, 2026 09:39
@CBenoit
Benoît Cortier (CBenoit) merged commit 3ab49ad into master Sep 17, 2026
10 checks passed
@CBenoit
Benoît Cortier (CBenoit) deleted the cbenoit-reject-duplicate-policy-members branch September 17, 2026 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants