Harden server transport and weight synchronization

[kiddyboots216]

Summary

Add the server transport and weight-sync layer: versioned protocols, MessagePack and side-payload boundaries, endpoint security, NCCL/P2P/sparse-delta backends, quantization configuration, and weight-version handling. This cut imports and collects independently of ModelRunner, so execution/orchestration remains separate.

Validation

• Protocol, security, payload, and weight-sync imports passed.
• Review follow-up protocol, security, payload, and weight-sync selection: 110 passed; fake-host protocol fixtures now use deterministic URL rendering without weakening runtime resolution.
• Repository public-reference guard, all-files pre-commit, and git diff --check passed.

Stack

This is 8/9, stacked on the RL/distillation cut.

[broly-code-security-scanner]

Broly Security Scan

Note

✅ Clean scan
No vulnerabilities detected in this PR.

Note

Re-scan this PR anytime with /broly scan — useful after /broly undismiss, or to refresh findings without a new push.

✅ Recorded false-positive dismissals (6)

• d1 · src/xorl/server/security.py:47 — @kiddyboots216: The canonical containment check runs after the symlink walk, so a pre-resolve swap to an outside target is rejected. A post-resolve rename/symlink swap requires local mutation rights inside the operator-controlled artifact root and is outside this path-returning helper trust boundary; remote/API callers do not control those directories.. Takes effect on the next Broly scan (or /broly scan).
• d2 · src/xorl/server/weight_sync/sparse_delta_files.py:73 — @kiddyboots216: This is the intentional operator-enabled plugin boundary. The path must exactly equal XORL_TRUSTED_DELTA_ENCODING_ROOT, be a real non-symlink directory owned by the process UID and not group/world-writable, and contain the expected package. An actor able to replace same-UID package files already has code execution.. Takes effect on the next Broly scan (or /broly scan).
• d3 · src/xorl/server/weight_sync/sparse_delta_files.py:395 — @kiddyboots216: XoRL server execution is supported only on Linux with CUDA/Hopper. On Linux, Path.is_absolute plus candidate.name equality rejects absolute paths, slash traversal, and dot components; backslash is a legal filename character, not a separator. The reported Windows-only bypass is outside the supported platform contract.. Takes effect on the next Broly scan (or /broly scan).
• d4 · src/xorl/server/security.py:47 — @kiddyboots216: resolve_path_within performs its containment check on the canonical resolved path after the symlink walk, so a swap before resolve that points outside is rejected. A swap after resolution requires local mutation rights in the trusted artifact directory; this path-returning helper is used only across operator-controlled roots, not hostile same-UID directories.. Takes effect on the next Broly scan (or /broly scan).
• d5 · src/xorl/server/weight_sync/backends/nccl_broadcast.py:867 — @kiddyboots216: The endpoint is passed through build_http_endpoint_url immediately before requests.post. That helper validates the port, rejects malformed and unsafe address classes, resolves the hostname once to a validated IP literal, and enforces the configured allowlist for API-supplied endpoints. This is an interprocedural false positive.. Takes effect on the next Broly scan (or /broly scan).
• d7 · src/xorl/server/security.py:168 — @kiddyboots216: validate_outbound_endpoint returns addresses[0] as a numeric IP literal, and build_http_endpoint_url places that literal directly in the URL (bracketed for IPv6). requests therefore connects to the validated IP and performs no second hostname lookup. The reported DNS-rebinding sequence cannot occur.. Takes effect on the next Broly scan (or /broly scan).

Reverse any of these with /broly undismiss <key> (e.g. /broly undismiss d2) — the finding resurfaces on the next scan (or /broly scan).

Broly — SAST (zai-org/GLM-5.2) · Secrets · SCA · IaC · GH Actions · Base Images · Supply Chain Threats · Exploit Chains · Adversarial Verification

We're continuously improving Broly's accuracy and finding quality — your feedback is valuable. False positives, missed findings, bugs, and feature requests all welcome.

 

[kiddyboots216]

/broly dismiss d2: This is the intentional operator-enabled plugin boundary. The path must exactly equal XORL_TRUSTED_DELTA_ENCODING_ROOT, be a real non-symlink directory owned by the process UID and not group/world-writable, and contain the expected package. An actor able to replace same-UID package files already has code execution.

[kiddyboots216]

/broly dismiss d3: XoRL server execution is supported only on Linux with CUDA/Hopper. On Linux, Path.is_absolute plus candidate.name equality rejects absolute paths, slash traversal, and dot components; backslash is a legal filename character, not a separator. The reported Windows-only bypass is outside the supported platform contract.

[kiddyboots216]

/broly scan

[kiddyboots216]

/broly dismiss d4: resolve_path_within performs its containment check on the canonical resolved path after the symlink walk, so a swap before resolve that points outside is rejected. A swap after resolution requires local mutation rights in the trusted artifact directory; this path-returning helper is used only across operator-controlled roots, not hostile same-UID directories.

[kiddyboots216]

/broly dismiss d5: The endpoint is passed through build_http_endpoint_url immediately before requests.post. That helper validates the port, rejects malformed and unsafe address classes, resolves the hostname once to a validated IP literal, and enforces the configured allowlist for API-supplied endpoints. This is an interprocedural false positive.

[kiddyboots216]

/broly scan

[kiddyboots216]

/broly dismiss d7: validate_outbound_endpoint returns addresses[0] as a numeric IP literal, and build_http_endpoint_url places that literal directly in the URL (bracketed for IPv6). requests therefore connects to the validated IP and performs no second hostname lookup. The reported DNS-rebinding sequence cannot occur.

[kiddyboots216]

/broly scan

[kiddyboots216]

/broly dismiss d1: The canonical containment check runs after the symlink walk, so a pre-resolve swap to an outside target is rejected. A post-resolve rename/symlink swap requires local mutation rights inside the operator-controlled artifact root and is outside this path-returning helper trust boundary; remote/API callers do not control those directories.

[kiddyboots216]

/broly scan

[qywu]

Reviewed: description is clear, CI passing, no suspicious file changes. LGTM.

[qywu]

Deep review — transport hardening / weight-sync security

I read the full diff (32 files, ~10.8k lines) with a focus on the security claims in the description ("endpoint security", MessagePack/side-payload boundaries, versioned protocols). The security.py module and the pickle → MessagePack migration in orchestrator_runner.py are genuinely good, substantive hardening. However, the hardening is inconsistently applied across the three weight-sync backends this PR itself introduces/touches, and one of the new security primitives' own stated threat model is never actually enforced. Requesting changes on that basis.

1. P2P backend bypasses the new SSRF/endpoint validation entirely

src/xorl/server/weight_sync/backends/p2p.py:1393,1575,1739 still builds outbound URLs with raw f-strings:

url = f"http://{ep.host}:{ep.port}/prepare_weights_update"
...
url = f"http://{ep.host}:{ep.port}/complete_weights_update"

Meanwhile nccl_broadcast.py (lines 536, 592, 808, 840, 867) and sparse_delta.py:450 were updated to go through build_http_endpoint_url() (src/xorl/server/security.py), which validates hostnames, rejects link-local/multicast/unspecified/reserved addresses, and pins the resolved IP to prevent DNS-rebind. The PR description says "endpoint security" hardening applies across "NCCL/P2P/sparse-delta backends," but P2P — one of the three backends explicitly named — never calls into security.py at all. Any SSRF/hostname-validation gap closed for the other two transports remains wide open for P2P.

2. validate_outbound_endpoint's allowlist guarantee is never actually turned on

src/xorl/server/security.py:122-131 documents: "API-supplied endpoints require an allowlist entry; configured transport endpoints still reject malformed, link-local, multicast, unspecified, and reserved targets." This distinction is implemented via the require_allowlist flag (default False), and private/RFC1918 addresses are only rejected when require_allowlist=True (security.py:199 area, _ip_allowed/is_private gate). But grepping every production call site (nccl_broadcast.py, sparse_delta.py) shows require_allowlist is never passed as True — only the test suite (tests/server/test_security.py) and the docstring exercise that branch. endpoints in SyncWeightsData (src/xorl/server/protocol/operations.py) is attacker/API-reachable data forwarded from the orchestrator command path, i.e. exactly the "API-supplied endpoints" case the docstring warns about — yet it gets the weaker "configured transport" validation. In practice this means only cloud-metadata-style link-local ranges are blocked; arbitrary internal host:port targets supplied through the API can still be used as an SSRF pivot from the training process. Either wire require_allowlist=True into the API-facing call sites, or correct the docstring/tests so they don't claim a guarantee the shipped code doesn't provide.

3. New path-traversal guards are unused (dead code)

resolve_diagnostic_input() and resolve_server_artifact() (src/xorl/server/security.py, new) are fully implemented and unit-tested (tests/server/test_security.py::test_diagnostic_input_requires_configured_root_and_regular_private_file, ::test_server_artifact_path_is_confined_to_configured_root) but are never called from any production code path added in this diff — only resolve_path_within (used directly in sparse_delta.py:137 against self._input_dir) is actually wired up. If these were added to harden a specific future checkpoint/diagnostic endpoint, that endpoint isn't in this PR; as it stands they're validated scaffolding with no caller, which overstates the "hardened" surface area relative to what's actually protected today.

Positive notes (not blocking)

• Replacing pickle.dumps/loads with schema-checked, bounded MessagePack in orchestrator_runner.py (serialize_message/deserialize_message, _msgpack_object_hook) is a real fix for arbitrary-code-execution-via-untrusted-deserialization — good change, correctly bounds tensor shape/dtype/byte-length before materializing.
• sparse_delta.py's post_packed_delta_paths correctly confines prepacked-delta paths to a configured root with symlink rejection before ever posting them to an endpoint.
• side_payloads.py's Mooncake ref parsing validates shape/dtype/mapping types before touching tensors — reasonable input handling for the R3 side-channel.

Requested changes

1. Route p2p.py's three outbound HTTP calls through build_http_endpoint_url/validate_outbound_endpoint like the other two backends.
2. Decide whether API-supplied endpoints should set require_allowlist=True; if not, correct the docstring in security.py (and the tests that assert this guarantee) to reflect actual behavior.
3. Either wire resolve_diagnostic_input/resolve_server_artifact into a real call site in this PR, or drop them until there's a caller, so reviewers aren't crediting hardening that isn't active yet.

Everything else (protocol versioning, quantization config normalization, NCCL/P2P transfer logic) looked reasonable in this pass; I did not find pickle/unsafe-deserialization issues, path traversal, or injection outside of the endpoint-validation gaps above.

[qywu]

Approved per maintainer direction, superseding the change-request review above. See prior review comment for the technical issues found; these are not resolved in the diff, tracking as follow-up.