Skip to content

fix(deps): update noyalib and preserve policy null rejection - #4348

Merged
drew merged 6 commits into
mainfrom
codex/update-noyalib
Oct 9, 2026
Merged

drew merged 6 commits into
mainfrom
codex/update-noyalib

Conversation

@drew

@drew drew commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Update noyalib to 0.0.53 to clear the published upstream advisory while preserving OpenShell's authored-YAML input semantics and exported string types.

Related Issue

Published upstream advisory: RUSTSEC-2026-0333.
Release failure: https://github.com/NVIDIA/OpenShell/actions/runs/37836897433/job/113539048121
Compatibility review: #4348 (comment)

This is maintainer-requested remediation for an already-public upstream advisory, not a new OpenShell vulnerability report. No public vulnerability issue was filed, per SECURITY.md.

Changes

  • Raise all three direct noyalib requirements to 0.0.53 and refresh the five affected lockfiles.
  • Centralize authored YAML compatibility in openshell-policy-schema, re-exported through openshell-core.
  • Remove the custom Serde deserializer. Load a bounded Value with duplicate-key rejection, validate schema-declared non-null object paths, then use noyalib’s ordinary typed deserializer. Preserve valid optional nulls and null values inside untyped middleware configuration. Empty/comment-only/null top-level documents remain rejected.
  • Use the shared decoder for bounded policy parsing, provider profiles, and the four prover loaders. Preserve policy limits and field-path diagnostics; explicitly reject duplicate keys.
  • Preserve string types in policy/profile exports and CLI YAML output for legacy readers: quote reader-sensitive dates, timestamps, numeric-looking strings, boolean words, and mapping keys. Actual numbers, booleans, and null remain unchanged. Duration strings do not require cosmetic quotes.
  • Keep the three maintainer-approved duration assertions semantic; preserve the protobuf round-trip assertions. Add regressions for nested null objects, duplicate keys, and cross-reader-sensitive strings.
  • Document the shared boundary and authored-YAML null rules. No CLI commands or skill workflows change.

Testing

  • Full Rust nextest suite on the simplified revision: 7,785 passed, 13 skipped.
  • Expanded nested-null regression rerun passes.
  • Focused policy-schema, provider, and prover suites pass.
  • Actual exported YAML loaded with PyYAML: dates, timestamps, underscore-separated numeric strings, boolean words, durations, and reader-sensitive mapping keys retain their exact string types and values.
  • cargo deny check advisories passes.
  • mise run pre-commit passes, including the commit hook.
  • mise run e2e passed before the schema-validation simplification (Rust, Python, MCP); not rerun on the simplified revision.
  • Simplified loader: focused policy-schema/provider/prover suites pass; added empty-document, optional-null, sequence-element, and dotted-map-key regressions.
  • Full mise run test hit a timeout in proxy::tests::plaintext_websocket_middleware_inspects_compressed_ws_messages (missing HTTP 101 upgrade). The unchanged test passed in isolation in 0.65s, and the full process-isolated Rust nextest suite subsequently passed all 7,785 tests. The full mise aggregate is not claimed green.
  • Cross-platform branch CI: awaiting the pushed revision.
  • Full mise run ci was attempted earlier and failed in the Go SDK task; full repository CI is not claimed green.

Checklist

  • Conventional Commit and DCO sign-off.
  • No advisory ignore or gate bypass.
  • Existing policy validation assertions remain intact.
  • Compatibility feedback addressed with production fixes and regression coverage.

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew
drew requested review from a team, derekwaynecarr, mrunalp and sjenning as code owners October 8, 2026 21:25
matthewgrossman
matthewgrossman previously approved these changes Oct 8, 2026
TaylorMutch
TaylorMutch previously approved these changes Oct 8, 2026
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew
drew dismissed stale reviews from TaylorMutch and matthewgrossman via 75631ab October 8, 2026 23:07
@EmilienM

EmilienM commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

I reviewed up to 75631ab: the duration fix clears the provider test failure (137/137 pass locally), but the new null guard still only covers part of the schema, so I'd hold the merge on that.

Verdict: request changes.

Side note on exposure: I don't think RUSTSEC-2026-0333 ever reached OpenShell. parse_policy_with_limits decodes into a Value with MergeKeyPolicy::Error, so it never takes the streaming typed path, and the typed callers (providers, prover) run with default budgets anyway.

Major

crates/openshell-policy-schema/src/lib.rs:521: explicit null is now rejected only for Option fields. 0.0.53 deserializes null as an empty map or a default struct for any map or struct target, on the typed path and the Value path alike. I ran the same inputs through parse_policy on main and on this branch. All of these fail on main and parse here:

  • network_policies: null, and network_policies: {x: null} (same with ~ or a bare key)
  • network_middlewares: null, and config: null on a middleware
  • endpoints: [null]
  • rules: [{allow: null}] and deny_rules: [null]
  • query: null and params: null under an allow rule
  • graphql_persisted_queries: null and {op: null}

Each one means the same as writing {}, so nothing new becomes expressible. It still matters, though:

  • It drops the contract that explicit_null_does_not_collapse_to_omission and the comment at opa.rs:1449 describe ("present malformed collections must not be mistaken for an omitted configuration").
  • The typed parser and the raw OPA loader now disagree. validate_opa_data_structure (opa.rs:1451-1466) still rejects network_policies.x: null and null endpoint or rule entries, so a file the CLI and gateway accept can fail when the supervisor loads it locally.
  • Generated policies fail open instead of closed. A template that renders query: null gives an allow rule with no constraints, and deny_rules: [null] turns into a deny rule that never matches.

Provider profiles get the same coercion through parse_profile_yaml (query, params, graphql_persisted_queries, annotations, discovery).

Rather than adding more per-field deserialize_with, I'd reject null once in the existing Value-first walk (inspect_document) before typed decoding, skip values inside user-data maps like config, and give provider profiles the same guard. The explicit-null test covers four cases today. It would be good to add the nested ones above plus the other helper-covered fields (landlock, credential_binding, json_rpc, the MCP options, allow/deny tool, middleware endpoints).

Minor

  • Duplicate fields: parse_profile_yaml (profiles.rs:1977) and the prover loaders (credentials.rs:271 and :289, registry.rs:197, accepted_risks.rs:60) use the default config, where a repeated struct field now silently keeps the last value. 0.0.28 rejected it through serde's duplicate-field check. The policy parser is fine because it sets DuplicateKeyPolicy::Error (lib.rs:586), and using from_str_with_config with the same policy here would bring these in line.
  • YAML output: dates, RFC3339 timestamps and 1_000 now come out unquoted from profile_to_yaml, serialize_policy and CLI -o yaml. noyalib reads them back fine, but PyYAML loads an MCP version like 2025-11-25 as a date and 1_000 as an int. No code change needed, though it deserves a line in the description.

Nits

  • The duration re-quoting in profile_yaml_with_quoted_durations looks correct to me (span_at includes existing quotes, and the [i].credentials[j] paths resolve on a top-level sequence). Two thoughts, though. 0s and 1.500s are strings for every YAML reader, so the quotes are cosmetic, and updating the three assertions would have been less code. If the goal is output that YAML 1.1 tooling reads back unchanged, the MCP versions above are the values that actually change type. Also, no test exercises profiles_to_yaml with durations, so the collection path is untested.
  • deserialize_non_null_mcp_options in providers is the same helper as the new one with different error text. That goes away if the null check moves to one place.

drew added 2 commits October 8, 2026 16:15
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

drew added 2 commits October 8, 2026 17:33
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>

@EmilienM EmilienM 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.

Thanks Drew, this round covers everything from my last comment. I re-ran my main-vs-branch probe and all 22 null cases now behave exactly like main, including query/params values and empty documents, while null inside middleware config still works. yaml::to_string round-tripped every awkward key and value I threw at it, and the affected crates' tests pass locally.

LGTM. A few follow-ups, none blocking:

  • The null path lists in parse_policy_with_limits and parse_profile_yaml are complete today, but nothing ties them to the structs, so a new map or struct field would quietly accept null again. A small wrapping Deserializer that rejects null only in deserialize_map/deserialize_struct would cover every loader without lists, and the per-field deserialize_non_null_* helpers could go with it.
  • yaml::from_str decodes from a Value without serde_path_to_error, so profile and prover errors carry no field path. Same as main, but parse_policy already shows how to get it back.
  • yaml::to_string re-parses its output and looks up each quoted string from the root, so large -o yaml exports get a lot slower (about 25x on a 16k-item list). Fine at today's sizes.

@drew
drew added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit e1f3c82 Oct 9, 2026
75 checks passed
@drew
drew deleted the codex/update-noyalib branch October 9, 2026 05:17
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.

4 participants