fix(network): address Copilot review (allow-list typing + private-range hoisting) - #4
Merged
Merged
Conversation
Four items from the Copilot review of the network/allow-list code (a port of Vercel's TS just-bash), all in the ported allow-list/private-range surface: - Widen `NetworkConfig.allowed_url_prefixes` and `_validate_allow_list` from `list[str | AllowedUrl]` to `list[str | AllowedUrl | dict[str, Any]]`. The runtime already accepts dict-shaped entries (`_entry_url`, `_validate_allow_list`, `firewall_headers` all branch on `isinstance(entry, dict)`); the annotation just didn't model the third shape carried over from the TS object literals. - Hoist the private-range CIDR tables out of `_is_private_ipv4`/`_is_private_ipv6` into module-level constants (`_PRIVATE_IPV4_NETWORKS`, `_PRIVATE_IPV6_NETWORKS`, `_SIXTOFOUR_NETWORK`). They were rebuilt on every call — and this is the hot path for `deny_private_ranges=True` (hostname + each resolved address). - Annotate `firewall_headers`' local `transforms` as `Sequence[...]` (covariant) instead of `list[...]` (invariant), clearing the assignment-type mismatch between the dict and `AllowedUrl.transform` branches. Behavior unchanged; 2297 tests pass (incl. all private-range tests). NOT included (deliberately scoped out): the DNS-resolver typing cluster (`_PinnedResolver.resolve` not conforming to aiohttp's `List[ResolveResult]`, the `_resolve_host` tuple type, `aiohttp.abc.AbstractResolver` access). Giving pyright the real base type surfaces a genuine interface-conformance question on security-sensitive rebinding-protection code that deserves its own grounded change.
Owner
Author
|
@coderabbitai can you please review? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the 4 Copilot review comments on the network wiring (upstream PR dbreunig#7 / our fork's ported allow-list code). All 4 are in the TS→Python port of Vercel's
allow-list.ts— behavior was faithful, Python typing/idiom was not.Fixes
types.pyallowed_url_prefixestypedlist[str | AllowedUrl]but dict entries are acceptedlist[str | AllowedUrl | dict[str, Any]]_validate_allow_list(entries: list[str | AllowedUrl])same gap_is_private_ipv4rebuilds 14IPv4Networkobjects every call_PRIVATE_IPV4_NETWORKS_is_private_ipv6same (incl.2002::/16)_PRIVATE_IPV6_NETWORKS+_SIXTOFOUR_NETWORKPlus a related typing cleanup the type-checker flagged in the same family:
firewall_headers' localtransformsannotatedSequence[...](covariant) instead oflist[...](invariant), clearing thereportAssignmentTypebetween the dict andAllowedUrl.transformbranches.Verification
pytest tests/test_commands/ tests/test_network.py→ 2297 passed, 1 skipped; all private-range tests pass.reportAssignmentTypecleared.Deliberately scoped out (separate, grounded follow-up)
Importing
AbstractResolverproperly (to clear theaiohttp.abc.AbstractResolveraccess warning) surfaces a genuine deeper issue:_PinnedResolver.resolvedoesn't conform to aiohttp's typedList[ResolveResult]return, plus a pre-existing tuple-type mismatch in_resolve_host. That's security-sensitive DNS-rebinding code and needs aiohttp's resolver contract grounded — not a drive-by here. Left as-is. Also out of scope: 6 pre-existingUP035/UP045ruff style lints intypes.py(unrelated; CI doesn't run ruff).