Skip to content

Fix ada::url host_end one byte short of its documented position - #1244

Merged
anonrig merged 1 commit into
ada-url:mainfrom
lenamonj:fix-url-components-host-end
Sep 1, 2026
Merged

anonrig merged 1 commit into
ada-url:mainfrom
lenamonj:fix-url-components-host-end

Conversation

@lenamonj

@lenamonj lenamonj commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

ada::url::get_components().host_end is one byte short of the position the url_components documentation diagram describes (one past the host) in both branches of url-inl.h: the no-credentials branch subtracts one, and the credentials branch omits the '@' byte that host_start sits on. ada::url_aggregator reports the documented position on the same inputs, so the two implementations of the one contract disagree, and slicing href by [host_start, host_end) from ada::url truncates the host:

url             https://example.com/path            host_end=18  substr -> "example.co"
url_aggregator  https://example.com/path            host_end=19  substr -> "example.com"
url             https://user:[email protected]/path  host_end=28  substr -> "@example.co"
url_aggregator  https://user:[email protected]/path  host_end=29  substr -> "@example.com"

The computation was internally self-correcting - running_index added the missing byte back - which is why every other offset agrees between the two implementations and only the exposed host_end is wrong. The existing test slices by get_hostname().size() and never reads host_end, which is how it survived. This change fixes both branches, updates running_index accordingly (it is now already one past the host), and pins [host_start, host_end) to the hostname in url_components.cpp, which runs over the full WPT urltestdata corpus. The new assertion fails before the fix and passes after; the full suite (353 tests) is green with the change.

Per AI_USAGE_POLICY.md: this defect was found by an automated analysis loop built on Claude; the fix and test were reviewed by me before filing, I am accountable for them, and I am happy to answer questions during review. clang-format 22 has been applied to the touched files.

@anonrig

anonrig commented Sep 1, 2026

Copy link
Copy Markdown
Member

thank you

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.27%. Comparing base (e675447) to head (04e32e2).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1244   +/-   ##
=======================================
  Coverage   63.27%   63.27%           
=======================================
  Files          39       39           
  Lines        7705     7705           
  Branches     3514     3514           
=======================================
  Hits         4875     4875           
  Misses        752      752           
  Partials     2078     2078           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@anonrig
anonrig merged commit 839f887 into ada-url:main Sep 1, 2026
54 checks passed
@lenamonj
lenamonj deleted the fix-url-components-host-end branch September 5, 2026 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants