Allow double slashes in the path according to the RFC3986

[marcelthole]

Q	A
Documentation	no
Bugfix	yes
BC Break	maybe
New Feature	no
RFC	no
QA	no

Description

This resolves #74
maybe it could be a BC break, because the behavior of the getPath has changed.
its since this commit: 68fc742

[Ocramius]

The "to prevent XSS" is the scariest bit here - I would suggest @weierophinney to look at it, considering the original 68fc742 commit.

I don't know the context for that initial commit myself, sorry :-\

The original text for the release:

## 1.0.4 - 2015-06-23

This is a security release.

A patch has been applied to `Zend\Diactoros\Uri::filterPath()` that ensures that
paths can only begin with a single leading slash. This prevents the following
potential security issues:

- XSS vectors. If the URI path is used for links or form targets, this prevents
  cases where the first segment of the path resembles a domain name, thus
  creating scheme-relative links such as `//example.com/foo`. With the patch,
  the leading double slash is reduced to a single slash, preventing the XSS
  vector.
- Open redirects. If the URI path is used for `Location` or `Link` headers,
  without a scheme and authority, potential for open redirects exist if clients
  do not prepend the scheme and authority. Again, preventing a double slash
  corrects the vector.

If you are using `Zend\Diactoros\Uri` for creating links, form targets, or
redirect paths, and only using the path segment, we recommend upgrading
immediately.

[weierophinney]

Honestly, I think the PSR-7 integration tests need to be updated to reflect this security fix.

[Ocramius]

I've added this to the 2.9.0 milestone to prevent an accidental release that sails out without this, waiting for feedback first.

[Ocramius]

@laminas/technical-steering-committee I don't know what to do with this one: can I get an opinion, please?

[weierophinney]

I'd consider the PSR-7 test suite in error here, as it leads to an XSS vulnerability, and I recommend providing them our security test instead.

[weierophinney]

The part that particularly concerns me is the removal of the security related test - this would be not just a BC break, but reintroduction of a previously patched vulnerability.

[lcobucci]

I'd consider the PSR-7 test suite in error here, as it leads to an XSS vulnerability, and I recommend providing them our security test instead.

I think the tricky thing here is context since the XSS vulnerability happens depending on the usage.
A URI that looks like https://example.org//google.com is completely fine according to the RFC but might introduce the vulnerability if only the path is used to redirect users.

My question, though, is: should a low-level component such as a PSR-7 implementation be responsible for preventing those redirection vulnerabilities? If so, we should probably look at solving this on the specification level IMHO.

[weierophinney]

should a low-level component such as a PSR-7 implementation be responsible for preventing those redirection vulnerabilities? If so, we should probably look at solving this on the specification level IMHO.

I think it should, as:

• A URI instance can be used to make client requests; if the HTTP client is not robust, we've created a vulnerability for them.
• The string representation is often used to create URIs for API payloads and/or web pages; again, if we spit these out without normalization, we create a vulnerability for them.

This latter is exactly what was reported, and the CVE was not limited to Diactoros, but was also reported against a number of libraries that generate/represent URIs.

In terms of solving it at the specification level, that's absolutely do-able (we recently did it for a class of vulnerabilities related to multi-line headers); I think we can still push a patch now to the upstream PSR-7 test suite with the test we created when patching the vulnerability reported against us.

[Ocramius]

Maybe handled by #128 ? Unsure 🤔

[weierophinney]

Maybe handled by #128 ? Unsure

Yes - #128 provides the correct solution for this.