Core: [Bug] Relation to resource with a colon in the identifier needs to use encoded colon - %3A

[silverbackdan]

API Platform version(s) affected: 2.6.x-dev

Description
When referencing an IRI which includes a : I used to be able to use a plain : and now need to use url encoded %3A Is this intentional?

How to reproduce
Attempt to post/put/patch with a field as an IRI which includes a colon (as a reference to another resource)

Possible Solution
https://github.com/api-platform/core/blob/master/src/Bridge/Symfony/Routing/Router.php#L81
Above this line we could have str_replace(':', '%3A', $pathInfo) - but I'd like to better understand why this is happening before this fix for just a single instance. It's possible a string ID will need URL encoding, I haven't checked.

Additional Context
n/a

Any thoughts on this? Please reach out if to discuss if more info is needed.

[silverbackdan]

I will look to take the test here: https://github.com/api-platform/core/blob/master/features/main/url_encoded_id.feature

And add in a field which references another of the same resource with an IRI that has a colon as well and ensure we can reference it in the same way.

[silverbackdan]

Right OK I get it, when creating a request using Request::create, that method will split the $uri into parts using PHP's parse_url - this will split the IRI at the colon and ignore it (I imagine it's because a colon should signify a port, but then it is ignored, probably because the numbers after the colon would not be a valid port number; not sure if it's a PHP bug TBH). I don't think we will need to look at any other characters to make sure they are encoded. Sorry I know this use case is quite specific, but I'm working with an ontology and their identifiers need to be hard coded and all use colons.

Would you be happy with a PR that adds tests and just replaces colons with encoded colons in the APIPlatform router before the Request::create call?

Example IRIs: /terms/ADDICTO:001 /terms/ADDICTO:24406194

[dunglas]

Hi, and thanks for the detailed report!

Have you been able to identify the specific commit causing the regression? I'm not a fond of doing a replacement of this special character, we should be able to find a more generic way.

I made some tests, and it looks like Request::create() expects an absolute URL as first parameter, not a relative one: https://3v4l.org/t8jOs

A better fix could be to use the absolute URL when available, and to prefix the relative URL with https://example.com when not available.

/cc @soyuka @jockos

[jockos]

@silverbackdan Could you give us the version on which you were able to use : in the IDs, I'll have a look and try to find which commit introduced the regression

[silverbackdan]

Thanks for all the notes and for the 3v4l example output for further debugging on the PHP version side of things showing that it does not appear to be a PHP update as it tests on a version lower than I would ever have used on this project. I quite like the possible solution for using an absolute URL if it'll work.

I'll go through this tomorrow to investigate exactly which version causes the regression. I won't write my speculations until I've proven the version. I'll do my best to trace back the exact change as well.

[silverbackdan]

I've done some work into this. I am unable to downgrade to version 2.5.6 or lower though due to Doctrine conflicts that I'm having trouble resolving without spending a lot of time.

I thought it may have been related to a data transformer I am using (and injecting the API Platform Normalizer - I'll give context if needed). I created a simple dummy API Platform resource to test

<?php

declare(strict_types=1);

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiProperty;
use ApiPlatform\Core\Annotation\ApiResource;

/**
 * @ApiResource()
 */
class DummyResource
{
    /**
     * @ApiProperty(identifier=true)
     */
    public int $id = 1;
    public Term $term;
}

This also failed with the same error when the colon was not encoded.

My tests had been using encoded colons, but the primary API user has never used colons encoded and said in their scripts it would be hard to implement. I do have a feeling now that their dependencies may have updated. If they try to submit with %3A instead, it encodes the % so perhaps it was automatically encoding the colon before.

I've also tested down to Symfony v5.1.5 to see if the router implementation changed. I get the same issue. I'm removing the regression title as I no longer believe it ever worked.

Current workings

• parse_url docs: On seriously malformed URLs, parse_url() may return false. - perhaps worth asking PHP maintainers whether /terms/ADDICTO:001 should be considered seriously malformed while example.com/terms/ADDICTO:001 is not - this results in no components being set for the URI and the path falls back to /
• There may be some need for the path to be relative as there is code specifically removing the baseURL https://github.com/api-platform/core/blob/master/src/Bridge/Symfony/Routing/Router.php#L78 - the HTTP_HOST is already set in the $server parameter
• If I do just prepend the host example.com this works. In older versions there was some double encoding issues related to this, but it doesn't seem to be the case now 2.6.x-dev

Solutions

• We could change from $request = Request::create(=$pathInfo, 'GET', [], [], [], ['HTTP_HOST' => $baseContext->getHost()]); to $request = Request::create($baseContext->getHost() . $pathInfo, 'GET'); (prepending the host to the URI instead of passing as a server parameter, it'll be populated anyway with parse_url)
• As the issue seems to be solely related to the colon indicating a port number it seems like it may be a more specific fix to encode the character.

Would a simple PR to implement solution 1 with a test be OK?

[dunglas]

Yet it looks ok to me. This part is tricky in Symfony so we need to be 100% sure to not introduce a regression.

[soyuka]

If this is linked to indentifiers, I'd suggest to add an IdentifierDenormalizer that supports strings and that does the proper encoding. This doesn't exist currently but may be added to the core repository just like this one for integers: https://github.com/api-platform/core/blob/master/src/Identifier/Normalizer/IntegerDenormalizer.php

Also related to https://github.com/api-platform/core/pull/3552

[silverbackdan]

@soyuka I've begun to work on this. The issue I have is that the IdentifierDenormalizer are used for outputting the identifier. I see that IdentifierDenormalizer will be changed to IdentifierTransformer in future so I could adapt the convert method of IdentifierConverter to have the ability to normalize... but then we are doing quite a lot I think. We would be saving the IDs as url encoded in the database, and then decoding them for all string identifiers to solve an issue which could be pretty simple and mainly related to routing. WDYT?

I'll make a draft PR so you can see in any case.