laminas / laminas-diactoros

PSR HTTP Message implementations
https://docs.laminas.dev/laminas-diactoros/
BSD 3-Clause "New" or "Revised" License
487 stars 63 forks source link

`Uri::__toString()` can yield malformed URIs #172

Open TimWolla opened 1 year ago

TimWolla commented 1 year ago

Bug Report

Q A
Version(s) Current git 3.3.x

Summary

The Uri class is able to parse malformed URIs but this results in Uri::__toString() generating a malformed URI. Attempting to pass that URI back into Uri will yield a InvalidArgumentException.

Current behavior

Certain malformed URIs do not round-trip through \Laminas\Diactoros\Uri.

How to reproduce

<?php

require('vendor/autoload.php');

$value = 'http://invalid:%20https://example.com';
$uri = new \Laminas\Diactoros\Uri($value);
$uri2 = new \Laminas\Diactoros\Uri($uri->__toString()); // Exception is thrown here.

Expected behavior

Either both constructors throw, or neither.


Note: This issue also exists in guzzlehttp/psr7 and was reported at guzzle/psr7#583.

froschdesign commented 1 year ago
$value = 'http://invalid:%20https://example.com';

PHP's function parse_url can not handle this wrong URL: https://www.php.net/manual/function.parse-url.php#refsect1-function.parse-url-notes

array(3) {
  'scheme' =>
  string(4) "http"
  'host' =>
  string(16) "invalid:%20https"
  'path' =>
  string(13) "//example.com"
}

https://github.com/laminas/laminas-diactoros/blob/4c6ae0026430244f10e9f59e6c31b605c55ce359/src/Uri.php#L80-L87

https://github.com/laminas/laminas-diactoros/blob/4c6ae0026430244f10e9f59e6c31b605c55ce359/src/Uri.php#L384-L392

boesing commented 1 year ago

Thanks, @TimWolla for cross posting.

Lets see how guzzle will handle this, after my latest hassle with php-http/discovery I would prefer to keep this in-sync. We might also want to cross-post this to https://github.com/php-http/psr7-integration-tests (which is used by diactoros as well).

Xerkus commented 2 months ago

I introduce basic host validation with the linked PR but anything more comprehensive would need to be done via integration tests.

The changes I introduced need to be elevated to the integration tests as well to ensure it is handled across all implementations.