Conversation
Round-trip conversion treated header casing inconsistently across record-form policies, which made generated network policies lose valid header names under Hypothesis coverage. Align the conversion with the intended policy semantics and update the generated assertion helper to validate the same merge behavior.
Add a few unit tests to cover more complex properties, and generally simplify the generators.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Fixes a network policy round-trip conversion bug where header case variants were being collapsed too early, causing distinct header keys (e.g., X-Trace and x-trace) to be lost across conversions.
Changes:
- Adjusted API→public conversion to preserve exact header keys for concrete
headers, while keeping case-collapsing behavior forheader_names. - Refined header merge logic so later rules replace earlier case variants for the same header name.
- Simplified/retargeted property-based tests toward normalized semantics and added direct unit coverage for the tricky merge behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/test_sandbox_network_policy_conversion.py | Removes overly-specific generated tests and adds focused unit tests for header-case preservation and merge semantics. |
| src/vercel/_internal/sandbox/network_policy.py | Updates conversion/merge helpers to preserve exact header keys for concrete headers and delay case-collapsing to the correct stages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| def _redacted_headers(rule: ApiNetworkInjectionRule) -> dict[str, str]: | ||
| if rule.header_names is not None: |
There was a problem hiding this comment.
_redacted_headers() now prioritizes rule.header_names whenever it is not None, even when it is an empty list and rule.headers is populated. Previously, an empty header_names would fall back to rule.headers.keys(). If the API payload (or caller) ever includes both fields (e.g., headerNames: [] plus headers: {...}), this change will drop the rule entirely (because it returns {} and to_network_policy() skips it). Consider treating an empty header_names the same as missing (e.g., only use header_names when it’s non-empty, otherwise fall back to rule.headers).
| if rule.header_names is not None: | |
| if rule.header_names: |
Round-trip network policy conversion was collapsing header names too early. Header names are case-insensitive, but our conversion logic merged case variants at the wrong stage, so a rule that explicitly contained both
X-Traceandx-tracecould come back with only one of them.Before, a policy like this:
could round-trip into:
That was incorrect because the original rule explicitly preserved both keys. It should instead come back as:
The fix was to preserve exact header keys when converting concrete API
headers, and only collapse case variants where that is actually part of the API contract: when decodingheader_names, or when merging separate rules.I also simplified the tests: the tricky merge behavior now has direct unit tests, while the generated tests focus on simpler structural invariants instead of reimplementing the conversion logic.