Skip to content

Fix GH-20366 ext/uri: Do not throw ValueError on null-byte#20489

Merged
kocsismate merged 2 commits intophp:PHP-8.5from
kocsismate:fix-20366-null-byte-handling
Nov 19, 2025
Merged

Fix GH-20366 ext/uri: Do not throw ValueError on null-byte#20489
kocsismate merged 2 commits intophp:PHP-8.5from
kocsismate:fix-20366-null-byte-handling

Conversation

@kocsismate
Copy link
Member

@kocsismate kocsismate commented Nov 14, 2025 •
edited
Loading

Fixes #20366

We still need to verify if ext/uri can handle null bytes properly.

@kocsismate kocsismate requested a review from TimWolla as a code owner November 14, 2025 20:34
@kocsismate kocsismate requested review from ndossche and removed request for TimWolla November 14, 2025 20:34
@DanielEScherzer
Copy link
Member

DanielEScherzer commented Nov 14, 2025

Is this for master or 8.5? And can it wait for 8.5.1?

@kocsismate kocsismate changed the base branch from master to PHP-8.5 November 14, 2025 20:36
@kocsismate
Copy link
Member Author

kocsismate commented Nov 14, 2025 •
edited
Loading

Is this for master or 8.5? And can it wait for 8.5.1?

Sorry! As always: I didn't set the target... It's a bug fix which fixes null-byte handling for WHATWG. It is completely broken in the RC-s right now.

I think it's not a deal-breaker for 8.5.0... It seems like an edge case for me. @DanielEScherzer

@kocsismate kocsismate requested a review from TimWolla November 14, 2025 20:40
ndossche
ndossche approved these changes Nov 16, 2025
View reviewed changes
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right

TimWolla
TimWolla approved these changes Nov 18, 2025
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would prefer the hexadecimal \x00 instead of the octal \0 in tests, because the latter can accidentally capture additional characters afterwards if they happen to be numeric.

nyamsprod added a commit to thephpleague/uri-src that referenced this pull request Nov 18, 2025
@nyamsprod
Backport PHP fix php/php-src#20489
9b527d2
nyamsprod added a commit to thephpleague/uri-polyfill that referenced this pull request Nov 18, 2025
@nyamsprod
Backport PHP fix php/php-src#20489
18519e1
@DanielEScherzer
Copy link
Member

DanielEScherzer commented Nov 18, 2025

Okay, this isn't in 8.5.0, to RM objections to the bug fix for 8.5.1

@kocsismate kocsismate force-pushed the fix-20366-null-byte-handling branch from 5b94e0c to 139983f Compare November 19, 2025 19:39
@kocsismate kocsismate merged commit 9743977 into php:PHP-8.5 Nov 19, 2025
9 of 10 checks passed
@kocsismate kocsismate deleted the fix-20366-null-byte-handling branch November 19, 2025 19:41
@kocsismate
Copy link
Member Author

kocsismate commented Nov 19, 2025

LGTM, but I would prefer the hexadecimal \x00 instead of the octal \0 in tests, because the latter can accidentally capture additional characters afterwards if they happen to be numeric.

I can workaround this in a followup test improvement.

kocsismate added a commit that referenced this pull request Nov 19, 2025
@kocsismate
Merge branch 'PHP-8.5'
15ca366 
* PHP-8.5:
  Fix GH-20366 ext/uri: Do not throw ValueError on null-byte (#20489)
jmarble pushed a commit to jmarble/php-src that referenced this pull request Nov 21, 2025
@kocsismate @jmarble
Fix phpGH-20366 ext/uri: Do not throw ValueError on null-byte (php#20489
fccf991 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TimWolla TimWolla TimWolla approved these changes

@ndossche ndossche ndossche approved these changes

@DanielEScherzer DanielEScherzer DanielEScherzer approved these changes

@edorian edorian Awaiting requested review from edorian

Assignees

No one assigned

Labels

Extension: uri

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ext/uri incorrectly throws ValueError when encountering null byte

4 participants

@kocsismate @DanielEScherzer @TimWolla @ndossche