Validate ASCII host values by mjpieters · Pull Request #954 · aio-libs/yarl

mjpieters · 2023-11-20T15:03:26Z

Host values must match the unreserved, sub-delims or pct-encoded grammar
rules. The IDNA encoder already checks for this, but for ASCII values we
skip IDNA encoding.

If the invalid character is @, or : followed by @ further in the
host value, tack on advice about using authority instead of host.

Fixes #880

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> (e.g. 588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the PR
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

CHANGES/8080.bugfix.rst

webknjaz · 2023-11-20T15:17:09Z

@mjpieters do you want me to reject the release build, wait and include this PR?

webknjaz · 2023-11-20T15:18:26Z

Not sure why the changelog bot is failing to notice the towncrier fragment... Not sure why but it's safe to ignore for now.

webknjaz · 2023-11-20T15:21:33Z

[towncrier-fragments]:13: : Spell check: hostnames: Started rejecting ASCII hostnames with invalid characters. For host strings that.

@mjpieters this can be fixed by adding "hostnames" to docs/spelling_wordlist.txt. Though, I hope to integrate codespell later that would replace sphinxcontrib-spelling and seems to have less false positives.

mjpieters · 2023-11-20T15:21:38Z

While implementing my PR I noticed that if you used a non-ASCII hostname with @ or any other invalid character (so not unreserved, percent encoded or sub-delims), then idna.encode() would raise an exception. But when I tried to reproduce this behaviour, just now, I noticed we swallow that exception, because it is a UnicodeError subclass 🤦 .

I'm not sure why the encoder uses except UnicodeError with a fallback to host.encode('idna').decode('ascii'), because that completely defeats the valid codepoint check.

I can hoist the validation to apply to non-ASCII hostnames too but that means I need to explicitly test for all disallowed ASCII codepoint (instead of the negative character classes I use now).

I'll go search through the commit history to see if I cannot find out more here.

mjpieters · 2023-11-20T15:22:23Z

@mjpieters this can be fixed by adding "hostnames" to docs/spelling_wordlist.txt. Though, I hope to integrate codespell later that would replace sphinxcontrib-spelling and seems to have less false positives.

:-D I already have that change staged locally :-P

webknjaz · 2023-11-20T15:23:39Z

@mjpieters so do you think it's worth putting into a separate release? The automation is there so it wouldn't take as long to make new releases from now on...

mjpieters · 2023-11-20T15:26:16Z

Not sure why the changelog bot is failing to notice the towncrier fragment... Not sure why but it's safe to ignore for now.

It is looking for the files in the wrong location, news/ instead of CHANGES/.

mjpieters · 2023-11-20T15:28:53Z

@mjpieters do you want me to reject the release build, wait and include this PR?

I think it's fine to put this in a next release. I don't see this as urgent.

webknjaz · 2023-11-20T15:33:04Z

@mjpieters do you want me to reject the release build, wait and include this PR?

I think it's fine to put this in a next release. I don't see this as urgent.

Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.

mjpieters · 2023-11-20T15:37:49Z

I'll raise a separate issue for IDNA host encoding. I can see why the UnicodeError exception handler is there but I think it needs some reworking.

This PR can focus on ASCII hostnames only, as a first step.

CHANGES/880.bugfix.rst

mjpieters · 2023-11-20T15:39:06Z

Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.

What a glorious deployment pipeline we have! 😁

webknjaz · 2023-11-20T15:39:57Z

Not sure why the changelog bot is failing to notice the towncrier fragment... Not sure why but it's safe to ignore for now.

It is looking for the files in the wrong location, news/ instead of CHANGES/.

Oh, that makes sense — this is because I moved the config to a dedicated file and haven't updates the bot to account for that. Thanks for pointing it out!

Host values must match the unreserved, sub-delims or pct-encoded grammar rules. The IDNA encoder already checks for this, but for ASCII values we skip IDNA encoding. If the invalid character is `@`, or `:` followed by `@` further in the host value, tack on advice about using `authority` instead of `host`.

webknjaz · 2023-11-20T15:44:02Z

Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.

What a glorious deployment pipeline we have! 😁

Hopefully, I'll make it scalable and reuse for other projects. I have similar things in many other repos but they all have tiny differences which makes them hard to reuse as is.
I made the CI run tests from the exact sdists we publish and install the wheels instead of rebuilding from Git source in each job. Though, I broke codecov reporting along the way so ignore it for now, I'll fix it later, hopefully.

webknjaz · 2023-11-20T22:23:36Z

Okay, will proceed with releasing, then. The build is half-way ready: https://github.com/aio-libs/yarl/actions/runs/6931722870.

It took a few reattempts to get the last bit right... But now we have v1.9.3:

webknjaz · 2023-11-21T02:14:03Z

Though, I broke codecov reporting along the way so ignore it for now, I'll fix it later, hopefully.

Fixed it. The coverage metrics are back up!

webknjaz · 2023-11-21T02:17:41Z

I'm not sure why the encoder uses except UnicodeError with a fallback to host.encode('idna').decode('ascii'), because that completely defeats the valid codepoint check.

I checked the coverage reported before I broke it temporarily, and turns out that the line you're talking about is the only one uncovered in the entire project: https://app.codecov.io/gh/aio-libs/yarl/commit/65333fab5f5b2bcce9fbb6aa810e9618fa10c6f0/blob/yarl/_url.py#L1176.

So I guess this might also need some extra extensive testing...

for more information, see https://pre-commit.ci

yarl/_url.py

bdraco · 2024-09-27T19:53:39Z

Ironically we were even doing it wrong in aiohttp aio-libs/aiohttp#9307

fixes #1441 regression was introduced in #954

webknjaz reviewed Nov 20, 2023

View reviewed changes

CHANGES/8080.bugfix.rst Show resolved Hide resolved

webknjaz reviewed Nov 20, 2023

View reviewed changes

CHANGES/8080.bugfix.rst Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

mjpieters force-pushed the validate_host branch 2 times, most recently from 3f8f6a8 to 874b429 Compare November 20, 2023 15:25

webknjaz reviewed Nov 20, 2023

View reviewed changes

CHANGES/880.bugfix.rst Show resolved Hide resolved

mjpieters force-pushed the validate_host branch from 874b429 to 7f21ff2 Compare November 20, 2023 15:41

webknjaz added the bug label Nov 21, 2023

bdraco mentioned this pull request Aug 14, 2024

Make raw_host and port cached_property #1044

Merged

Merge branch 'master' into validate_host

e14f451

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 1, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

749afd0

for more information, see https://pre-commit.ci

bdraco reviewed Sep 1, 2024

View reviewed changes

yarl/_url.py Outdated Show resolved Hide resolved

bdraco marked this pull request as draft September 26, 2024 21:04

bdraco added 16 commits September 26, 2024 16:11

more coverage

918da42

cleanup

fdd12b9

cleanup

d5923f0

cleanup

d9332cc

Merge branch 'host_subcomp' into ipv6_fix

2aae488

more cover

ca5942b

Merge branch 'host_subcomp' into ipv6_fix

2b2e668

cover

299d97e

fixes

d518fd8

Merge branch 'ipv6_fix' into validate_host

700df5d

Merge branch 'master' into ipv6_fix

90f13f4

changelog

c464239

Apply suggestions from code review

8ec8fef

Update CHANGES/1158.bugfix.rst

ed85d2b

Merge branch 'ipv6_fix' into validate_host

d4563d0

Merge branch 'master' into validate_host

a13eadd

bdraco marked this pull request as ready for review September 26, 2024 22:08

bdraco approved these changes Sep 26, 2024

View reviewed changes

bdraco merged commit 3273936 into master Sep 26, 2024

bdraco deleted the validate_host branch September 26, 2024 22:18

bdraco mentioned this pull request Sep 27, 2024

BaseRequest.url passes the host and port to URL.build when the port is included in the host header (non-default port) -- fixed in 3.10.7 aio-libs/aiohttp#9307

Closed

Chaoses-Ib mentioned this pull request Sep 27, 2024

use comfyscript on modal return "ValueError: Host '127.0.0.1:8199' cannot contain ':'" Chaoses-Ib/ComfyScript#73

Closed

bdraco mentioned this pull request Sep 30, 2024

Broken in with yarl 1.13.0+ working in 3.10.7+, contains test case: aiohttp parses port numbers as part of the host (ValueError: Host x.x.0.0:8080 cannot contain ':') aio-libs/aiohttp#9360

Closed

1 task

javajawa mentioned this pull request Oct 8, 2024

aiohttp server reuqest.url broken with yarl >= 1.13 aio-libs/aiohttp#9435

Closed

1 task

musicinmybrain mentioned this pull request Oct 20, 2024

Test failure in opentelemetry-instrumentation-aiohttp-client with yarl 1.13 open-telemetry/opentelemetry-python-contrib#2916

Closed

bdraco mentioned this pull request Nov 30, 2024

Support uppercase characters in host #1441

Closed

5 tasks

bdraco added a commit that referenced this pull request Nov 30, 2024

Ensure ascii host case normalization happens before validation

1e9e9fa

fixes #1441 regression was introduced in #954

bdraco mentioned this pull request Nov 30, 2024

Ensure ASCII host case normalization happens before validation #1442

Merged

Uh oh!

Conversation

mjpieters commented Nov 20, 2023 • edited by bdraco Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

This comment was marked as outdated.

webknjaz commented Nov 20, 2023

Uh oh!

mjpieters commented Nov 20, 2023

Uh oh!

mjpieters commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

mjpieters commented Nov 20, 2023

Uh oh!

mjpieters commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

mjpieters commented Nov 20, 2023

Uh oh!

Uh oh!

mjpieters commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 20, 2023

Uh oh!

webknjaz commented Nov 21, 2023

Uh oh!

webknjaz commented Nov 21, 2023

Uh oh!

Uh oh!

bdraco commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjpieters commented Nov 20, 2023 •

edited by bdraco

Loading