IBX-9895: Escaped forward slash in the RestExecutedView visitor

[barw4]

🎫 Issue	IBX-9895

Description:

To be honest I'm not sure if this is the right call. By design, Symfony won't allow / in routes unless it is allowed to do so in requirements but I can't see us altering the view routes. On the other hand in my opinion we shouldn't strip this in frontend call so this leaves us only with this solution?

For QA:

Documentation:

[alongosz]

@barw4 #182 has been merged, you can rebase.

[barw4]

thank you, done

[Steveb-p]

I'm on the fence on this. Technically / is a valid character as part of a query parameter, and can be encoded if necessary. I don't feel like trying to "fix" a string - which will not work anyway after the fix - is the right thing to go with.

[barw4]

@Steveb-p It's a valid character indeed, therefore I'm not removing it in frontend or from search query, the issue is only with the path not being able to be generated due to "/" being exclusively forbidden by Symfony. What other options do we have?

[Steveb-p]

Do you have a stacktrace or specific piece of code that prevents this character during URL generation process?

[ViniTou]

we shouldn't strip this in frontend call

Agree, but why cant we just encode it with something like rawurlencode to get %2F and then deocde it?

[Steveb-p]

we shouldn't strip this in frontend call

Agree, but why cant we just encode it with something like rawurlencode to get %2F and then deocde it?

That would be my follow up, if it's possible to decode it in all places where this is used. But that depends on complexity.

[barw4]

The stacktrace is:

Parameter "viewId" for route "ibexa.rest.views.load.results" must match "[^/]++" ("udw-locations-by-search-query-/" given) to generate a corresponding URL.
{▼
  [/Users/bartek/Projects/46/vendor/symfony/routing/Generator/UrlGenerator.php:194] {▶}
  [/Users/bartek/Projects/46/vendor/symfony/routing/Generator/CompiledUrlGenerator.php:67]{▶}
  [/Users/bartek/Projects/46/vendor/symfony/routing/Router.php:235]) {▶}
  [/Users/bartek/Projects/core/src/bundle/Core/Routing/DefaultRouter.php:100] {▶}
  [/Users/bartek/Projects/46/vendor/symfony-cmf/routing/src/ChainRouter.php:248] {▶}
  [/Users/bartek/Projects/rest/src/lib/Server/Output/ValueObjectVisitor/RestExecutedView.php:84] {▶}

and by default Symfony in routes requires the following regex:

[^\/]++\

[barw4]

@Steveb-p @ViniTou you would see it url-encoded in the parser then (https://github.com/ibexa/rest/blob/main/src/lib/Server/Input/Parser/ViewInputOneDotOne.php#L38)?

But where would we decode it then? We cannot decode it in Visitor because of the URL problem, the only remaining place when it's used is the response itself.

So are we encoding it only for URL generation and decoding when crafting a controller response?

[Steveb-p]

Your PR addresses the "generating" side of things, while it is perfectly acceptable to use this as part of REST API directly, which means we cannot really perform encoding/decoding - as this would break current usage (in certain cases).

Is it possible to relax the requirements on the parameter? From what I see this already depends on strict_requirements setting, which means this URL generation probably works on prod environments (which tend to have strict_requirements on URLs turned off).

[barw4]

Yes, it's possible, I've tested it yesterday and allowing "/" in requirements was also working properly.

[Steveb-p]

Yes, it's possible, I've tested it yesterday and allowing "/" in requirements was also working properly.

As long as this parameter is only part of REST input (json/xml) or optional query parameter (past ? in query string), then allowing it should be fine.

Adding / in route-declared parameters would break the routing, so it's reasonable in that case, but probably not where we are using it.

[barw4]

@Steveb-p okay, so does the current solution make sense?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Steveb-p]

@Steveb-p okay, so does the current solution make sense?

ibexa.rest.views.load.results:
    path: /views/{viewId}/results

So the parameter is actually part of the string 😖 That means there is actually a high likelihood that this particular route will not work properly with / signs in it. Can you check?

[barw4]

Well, the case is, it's not used anywhere 😄 :

    public function loadViewResults()
    {
        return new NotImplementedException('ezpublish_rest.controller.content:loadViewResults');
    }

[alongosz]

@barw4 one thing I don't get from the ticket is what is the exact request that causes this HTTP 500 error.

[barw4]

@alongosz it's the views one:

ibexa.rest.views.create:
    path: /views
    defaults:
        _controller: Ibexa\Rest\Server\Controller\Views:createView
    methods: [POST]

[alongosz]

@barw4 I'm interested in the specific HTTP request that causes the error on the attached screenshot (it's hidden there).

[barw4]

It's the views one:) locally as I'm checkouted on my branch it doesn't fail, but in JIRA ticket it's the one that fails

[alongosz]

As discussed internally, this is an identifier, so it cannot contain arbitrary user input with potentially unsafe characters. Front-end app should encode data before putting them into payload as the identifier.

[barw4]

Closed in favor of #181