laminas / laminas-diactoros

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

Uri::__toString adds extra delimiters #29

Closed weierophinney closed 1 year ago

weierophinney commented 4 years ago

I'm finishing coding League\Url v4 and it exposed the following class in public API

Url::sameValueAs(Psr\Http\Message\UriInterface $url); 

To compare two PSR-7 UriInterface object based on their __toString method output.

I tried to make it work with Diactoros but I failed with the following edge cases URLs:

In both cases the object is instantiated and the respective Uri components are correctly set but the __toString method adds extra path and/or authority delimiters.

Below the code to reproduce the bugs:

use Zend\Diactoros\Uri;
$url = new Uri('http:/example.com');
echo $url->__toString(); //expected http:/example.com but got http:///example.com
new Uri($url->__toString()); //throw InvalidArgumentException

The authority delimiters are added (not required according to PSR-7) and the method returns an invalid Uri.

use Zend\Diactoros\Uri;
$url = new Uri('http:example.com');
echo $url->__toString(); //expected http:example.com but got http:///example.com
new Uri($url->__toString()); //throw InvalidArgumentException

The authority delimiters and a path delimiter are added (not required according to PSR-7) and the method returns an invalid Uri.


Originally posted by @nyamsprod at https://github.com/zendframework/zend-diactoros/issues/65

weierophinney commented 4 years ago

@nyamsprod Two questions:

A couple notes from the UriInterface:

My takeaway from this (and, recall, I was editor of the spec) is that how Diactoros returns the URI is not unexpected; this is an edge-case. When I first looked at the URIs, before I examined what you presented them doing, I expected both would be spit back out as http:///example.com. This goes hand-in-hand with our recent security disclosure, which would try and create a URI that will not have unintended XSS/redirect consequences.

With that information in mind, how would you proceed?


Originally posted by @weierophinney at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-119009411

weierophinney commented 4 years ago

@weierophinney here's what I think

while the // is not required if no authority is present, it's up to the implementation to decide whether or not to add the delimiters.

My interpretation was that if no authority is present then the authority delimiters were never added. This is the approach I took with League\Url. IMHO This is the reason why you end up with the triple /. Since the path is formatted in accordance with the presence or absence of an authority part. So it seems to be an implementation disagreement .

Since these are edge cases we should leave them as they are without trying to fix them. I will remove them from my psr7-uri-test-suite repo and from the other PR I have sent when I have time. When PHP will allow exceptions in the __toString() method we may have a better solution for that with a RuntimeException thrown in those cases.

In the meantime, I think at least documenting this behavior should be done. Since reproducing it is fairly easy in any UriInterface implemented object.

use Zend\Diactoros\Uri;

$uri = new Uri('http://www.example.com');
$invalidUri = $uri->withHost('')->withPath('foo/bar');

echo $invalidUri;

We should state that some submitted URIs or URI manipulations may return invalid URI or else we will be ask again about this behavior since any implemented solution I have encountered so far returns invalid URIs:

So the question which remains is were do we document this ?


Originally posted by @nyamsprod at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-119114899

weierophinney commented 4 years ago

@nyamsprod I'd recommend the following:


Originally posted by @weierophinney at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-122083389

weierophinney commented 4 years ago

@weierophinney seems that @icywolfy as already open an issue about this problem with a more in depth explanation than I did on the fig repo issue 584. It would be nice to have a general answer to the issues he raised there


Originally posted by @nyamsprod at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-125699267

weierophinney commented 4 years ago

@weierophinney I thought about an errata/clarification around this issue. I hope my wording is not breaking errata rules by adding new rules to PSR-7. Anyway this is just a draft. I'm looking forward for your input/remarks


Originally posted by @nyamsprod at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-136835042

weierophinney commented 4 years ago

:) Add, please, tests:

public function testToString()
    {
        $url = 'http://user:pass@local.example.com:8080/foo?bar=baz#quz';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'mailto:someone@example.com,someoneelse@example.com';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'http://local.example.com/foo?return=\'http://local.example.com:8080\'';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'https://local.example.com/login/\'https://local.example.com/admin\'';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'http://local.example.com/blog#news=last[month&tag=sport]';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = '../../book/catalog.xml';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'file:///C:/test.html';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'file://192.168.0.100/home/test.html';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);

        $url = 'ftp://guest:qwerty@local.example.com/readme.txt';
        $uri = new Uri($url);
        $this->assertSame($url, (string) $uri);
    }

So, it works:

    protected static function createUriString($scheme, $authority, $path, $query, $fragment)
    {
        $uri = '';
        if ($scheme) {
            $uri .= $scheme . ':';
        }
        if ($authority) {
            $uri .= '//';
            if ('file' === $scheme && 0 === strpos($authority, 'localhost')) {
                $uri .= substr($authority, 9);
            } else {
                $uri .= $authority;
            }
        }
        if ($path) {
            if ($scheme) {
                $path = ltrim($path, '/');
            }
            if ($authority) {
                $path = '/' . ltrim($path, '/');
            }
            $uri .= $path;
        }
        if ($query) {
            $uri .= sprintf('?%s', $query);
        }
        if ($fragment) {
            $uri .= sprintf('#%s', $fragment);
        }

        return $uri;
    }

    public function getHost()
    {
        if ($this->scheme === 'file' && ! $this->host) {
            return 'localhost';
        }

        return $this->host;
    }

    public function getStandartPort()
    {
        return getservbyname($this->scheme, 'tcp');
    }

Originally posted by @easy-system at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-193516304

weierophinney commented 4 years ago

Sorry, it is i`m now :) I speek wery bad, so see this: http://download.java.net/jdk7/archive/b123/docs/api/java/net/URI.html


Originally posted by @easy-system at https://github.com/zendframework/zend-diactoros/issues/65#issuecomment-193523698

Xerkus commented 1 year ago

I see this is no longer the case in current version. It was fixed at some point.