Validate port range in URL.build#1769
Open
HrachShah wants to merge 2 commits into
Open
Conversation
URL.build accepted ports outside the 0..65535 range and stored them
as-is. The error surfaced later, when _cache_netloc() ran split_netloc()
on the rendered netloc and raised ValueError('Port out of range 0-65535')
deep inside that helper rather than from the caller's frame. URL.with_port
already had the equivalent range check; align URL.build with it so the
failure happens eagerly and with a message that names the argument.
The encoded=True branch went through build_pre_encoded_url, which
calls make_netloc() and formats the port with f'{host}:{port}' without
any range check. The same eager check in URL.build now catches it too.
Add 8 regression cases across test_build_with_port, test_build_with_out_of_range_port
and test_build_with_out_of_range_port_encoded covering 65536, 99999 and -1 in
both encoded and non-encoded paths; all 8 fail on master before the fix.
psf-chronographer
Bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1769 +/- ##
=======================================
Coverage 99.86% 99.86%
=======================================
Files 21 21
Lines 4310 4320 +10
Branches 252 253 +1
=======================================
+ Hits 4304 4314 +10
Misses 4 4
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Coverage Report for CI Build 28216354684Coverage increased (+0.006%) to 98.746%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
2 participants
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.
What do these changes do?
URL.buildnow rejects ports outside0..65535at the call site, matching the check that already exists inURL.with_port. Previously the constructor accepted any integer, formatted it into the netloc, and the failure surfaced only later insidesplit_netlocwhen something forced a netloc cache rebuild (str(url), accessinghost_subcomponent, etc.). Theauthority=...path was unaffected because it goes throughsplit_netlocup front and raised immediately.The fix is two lines in
URL.buildthat mirror the wording ofwith_port, plus parametrized regression tests covering bothencoded=Falseandencoded=True, with three out-of-range inputs (65536,99999,-1).Are there changes in behavior for the user?
Yes.
URL.build(scheme="http", host="example.com", port=65536)now raisesValueError: port must be between 0 and 65535, got 65536at the call site rather than waiting until the URL is rendered or one of its properties is accessed. The error message and shape matchURL.with_port. Inputs in the existing range (including0) behave identically.Is it a substantial burden for the maintainers to support this?
No. It is one new guard plus a regression test and a CHANGES fragment; the public surface (
URL.build's signature and the error type) is unchanged.Related issue number
None.
Checklist
CHANGES/folderAgent run details (optional, for reviewers)
Tests:
PYTHONPATH=. python3 -m pytest tests -p no:cacheprovider --override-ini="addopts="-> 1462 passed, 101 skipped (benchmarks), 4 xfailed.The
tests/test_quoting.pycollection error reproduces on a clean checkout without the Cython extension built (it importsyarl._quoting_c); runningCC=gcc pip install -e .first builds the extension in place and the suite goes green. The 9doc-spellingwarnings come from the existingCHANGES.rsthistory and unrelated towncrier fragments, not from1769.bugfix.rst.Drafted with Zo (MiniMax M3); reviewed by HrachShah.