Project

General

Profile

Actions

Bug #104440

open

stripPathSitePrefix breaks file path

Added by Franz Holzinger 4 months ago. Updated about 2 months ago.

Status:
Needs Feedback
Priority:
Should have
Assignee:
-
Category:
Frontend
Target version:
-
Start date:
2024-07-19
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
12
PHP Version:
8.2
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

The internal method PathUtility::stripPathSitePrefix breaks the formerly correct path to files.
This happens if the class ImageContentObject (IMAGE) is used to create an Image HTML out of an image file.
There is a second error when TYPO3 wrongly generates an image file with duplicate slashes. As a side effect of stripPathSitePrefix the duplicate slash is fixed. But when a Middleware call is used then there is no such duplicate slash bug. This leads to the result that the slash before fileadmin is removed for a Middleware and the HTML image source is:

src="fileadmin/myimage.png" 

instead of

src="/fileadmin/myimage.png" 

Therefore the image is not found by the browser with the new TYPO3 routing enhancer.

    /**
     * Strip first part of a path, equal to the length of public web path including trailing slash
     *
     * @internal
     */
    public static function stripPathSitePrefix(string $path): string
    {
        debug ($path, 'stripPathSitePrefix $path: demoshop12//fileadmin/ Doppelter falscher Slash');
        debug (Environment::getPublicPath(), 'stripPathSitePrefix Environment::getPublicPath()');
        $result = substr($path, strlen(Environment::getPublicPath() . '/'));
        debug ($result, 'stripPathSitePrefix $result /fileadmin mit Slash vorne');

        return $result;
    }


Files

pathutility-debug.html (4.92 KB) pathutility-debug.html Franz Holzinger, 2024-07-19 19:51
pathutility-middleware-debug.html (20.1 KB) pathutility-middleware-debug.html Franz Holzinger, 2024-07-20 06:24
Actions #2

Updated by Garvin Hicking 4 months ago

  • Status changed from New to Needs Feedback

Thanks for reporting! Could you maybe provide a minimal example on how to reproduce the issue, ideally with v13/main? This would help analysis and help create tests for the behaviour.

Actions #3

Updated by Franz Holzinger 4 months ago

Here is the PathUtility::stripPathSitePrefix debug output coming from the xajax extension's Middleware.

Actions #4

Updated by Franz Holzinger 4 months ago

You can reproduce this with tt_products if you build a simple view with a product variant and add 2 articles with different prices to this product. The image must change if the variant select box is clicked by the mouse pointer. The tt_products_articles exclude extension settings must not exclude the image_uid.

A bug fix is also needed for TYPO3 12.

templateFile = EXT:addons_tt_products/Resources/Private/Templates/collection/example_template_bill_de.tmpl

I can build an environment for TYPO3 13 and give you access.

Actions #5

Updated by Garvin Hicking 4 months ago

That is sadly not a "minimal" example and has too many dependencies. I would need an example with just a few lines to reproduce this behavior at some point, maybe this is possible for you to create?

Personally, I can not offer to debug a complex environment due to time restrictions.

Actions #6

Updated by Franz Holzinger 4 months ago · Edited

I have no idea where else the duplicate slash inside of file paths will happen.
I cannot produce an example with some lines of PHP code which uses the xajax extension to create an image after running its middleware.
I have already added the necessary debug output in the attached HTML files to view them in a HTML browser. Tell me if you need more debug infos.

Actions #7

Updated by Garvin Hicking 4 months ago

I can only help with an example code that I can run to receive the debug output. Maybe the xajax middleware is doing something that triggers this. I would need a minimal middleware that makes the issue reproducible.

Of course if someone else knows what to fix with the given information, feel free to take over.

Actions #8

Updated by Markus Hofmann 4 months ago

We had the same issue within a SolR indexing task on CLI. The main issue, according to @Stefan Bürk was, that no Site Configuration and TSFE is loaded. The stripPathSitePrefix needs the site and TSFE to correct load all related prefixes, like project root, public root and so on, which are needed for a full qualified prefix according to the current call.

You have to change your middleware resolving after loading Site configuration and TSFE, if possible.

Actions #9

Updated by Garvin Hicking 3 months ago

  • Status changed from Needs Feedback to Closed

Closed due to inactivity/details and probably solution is pointed out in Markus Hofmann's answer

Actions #10

Updated by Franz Holzinger 3 months ago

Sorry for my "inactivity". However there are still bugs in TYPO3 13 which needed workarounds in order to be able to test it there.
And it does not work with TYPO3 13!

The develop branch of the extension taxajax shows that the objects $GLOBALS['TSFE'] and the Site Configuration are set.

I do not understand your argumentation.

public static function stripPathSitePrefix(string $path): string
{
return substr($path, strlen(Environment::getPublicPath() . '/'));
}

Can you explain where the above mentioned objects are needed here? Why does it remove the leading slash and why should this be correct?

Actions #11

Updated by Garvin Hicking 3 months ago

  • Status changed from Closed to New

I cannot answer that specifically. One would need to inspect the middleware chain order.

I've re-opened this issue.

Actions #12

Updated by Stefan Bürk 3 months ago · Edited

  • Status changed from New to Needs Feedback

@Franz Holzinger

The issue targets two different things. Albeit legit and stripPathSitePrefix
could need some hardening to ensure that leading slashes are stripped as the
comment implies, the second part is the ImageObject thing and the third the
not nearer descriped additional issue (which is a bad thing).

However, your issue makes not clear what should be changed within the method
and why "it breaks". At first, the current code at least does what the comment
states. It can be argueed that this is not enough. And the prodvided debus are
not helpfull to investigate this (verify it/reproduce it).

Can you please provide steps or code - eventually as a simplified extension
public on github showcasing this issue (without using other third party
extensions which may introduce additional flaws).

This is required to really investigate your reported issue, reproduce it and
analyse which needs to be done to finally fix it - and eventually split it
into different issues.

Without something to reproduce and investigate this, changing this method
is a no-go due to the impact a change may have on other code (hidden follow up bugs etc).

Optionally - you could create a public repo with a project/instance (ddev) with your
extension and stripped down data which show caseses these issue so it can be debugged.
Personally I would not invest hours to get your extensions runnng and find out how to
reproduce it.

Actions #13

Updated by Stefan Bürk 3 months ago

Markus Hofmann wrote in #note-8:

We had the same issue within a SolR indexing task on CLI. The main issue, according to @Stefan Bürk was, that no Site Configuration and TSFE is loaded. The stripPathSitePrefix needs the site and TSFE to correct load all related prefixes, like project root, public root and so on, which are needed for a full qualified prefix according to the current call.

You have to change your middleware resolving after loading Site configuration and TSFE, if possible.

That is a good guess. The issue was, that in cli context neither the Backend nor the Frontend eventlister for prefixing FAL resource links is registered, which happens in the corresponding RequestHandlers. Beside that, the Frontend event listener only workes, even if registered, when `GLOBALS['TSFE']` is available. That was the issue. For SolR indexer on CLI we implemented a custom url prefixer being able to work with "dynamicalys" use TSFE created without writing them to the global and allow also siwitching between different sites while still having a full FE context simulated.

That means, that adding the site absPrefix (slash in most cases) will not be done if something uses FAL API stuff before the TSFE has been prepared/bootstrapped (which also relies on at least a resolved site.

https://github.com/TYPO3/typo3/blob/d9323c00a3f61cfc142b598a63205f433b50971b/typo3/sysext/frontend/Classes/Http/RequestHandler.php#L126-L131

// Make sure all FAL resources are prefixed with absPrefPrefix
$this->listenerProvider->addListener(
    GeneratePublicUrlForResourceEvent::class,
    PublicUrlPrefixer::class,
    'prefixWithAbsRefPrefix'
);

which registers the frontend resource url prefixer TYPO3\CMS\Frontend\Resource\PublicUrlPrefixer

https://github.com/TYPO3/typo3/blob/main/typo3/sysext/frontend/Classes/Resource/PublicUrlPrefixer.php

However, that eventlister does nothing if

    private function getCurrentFrontendController(): ?TypoScriptFrontendController
    {
        return $GLOBALS['TSFE'] ?? null;
    }

returns null - which happens if a middleware or any code is executed calling FAL related APIS before the TYPO3\CMS\Frontend\Middleware\TypoScriptFrontendInitialization

That also counts for ImageContentObject (which must be created by code somehow) or using fluid viewhelpers using FAL apis and so on.

Reading your issue, it is correct that the slash is missing because you use FAL API related stuff before this eventlister can do it's job.

That would render the correct resource/image with the slash (or better say with the correct absPrefix).

That means, you need to fix that on your side.

Beside that, a dedicated issue HOW to produce the double slash with some code snippet in the core would help to ensure that the double slash is not created.
After that, it can still be considered to ensure that stripPathSitePrefix does not leave leading slashes.

We should still priorities the source of the issue befor hardening this method which makes debugging more harder.

To summerize:

  • check if your middleware or the code used to generate that image tag is executed BEFORE $GLOBAL['TSFE'] is available and set.
  • if yes, either move it to a later point or a) ensure a TSFE for your middleware and also in the global b) use a custom urlprefixer added within your middleware if you handle the request to use a simulated TSFE etc.

Beside that, not knowing your extension code - you should also ensure to set the request to the content object instances if you create them manually in PHP code.

Actions #14

Updated by Franz Holzinger 3 months ago

"not knowing your extension code"?
The extension code of taxajax is publicly available on Github.

https://github.com/franzholz/taxajax/tree/develop

"$GLOBALS['TSFE']" is not empty, but initialized!

returns null - which happens if a middleware or any code is executed calling FAL >> related APIS before the

    TYPO3\CMS\Frontend\Middleware\TypoScriptFrontendInitialization

Nothing is done before it!

<?php
use JambageCom\Taxajax\Middleware\XajaxHandler;
return [
'frontend' => [
'jambagecom/taxajax/preprocessing' => [
'target' => XajaxHandler::class,
'description' => 'The Ajax calls will be processed and no page will be generated.',
'after' => [
'typo3/cms-frontend/prepare-tsfe-rendering'
],
],
],
];

you should also ensure to set the request to the content object instances if you create them manually in PHP code.

Do you meant the request object or what else? Must the Image Content Object be initialized by a request object?

Actions #15

Updated by Stefan Bürk 3 months ago

https://github.com/franzholz/taxajax/tree/develop only provides a middleware,
nothing which renders any kind of template or stuff or image.

Can you provide a "simple" addition (custom ajax handler) as a simple extension
which can be used along witht the taxajax extension to reproduce this issue ?

Along with steps how to install both extension and what is needed to configure
so it can be reproduced.

Please do not add huge extension now, just a simplified example to reproduce this.
I rembemer that some time along there was also another extension involved with a
lot of code and code paths, doing stuff as handler of your taxajax. But i was not
able to "setup" it easily so no debugging. And just from reading code it was not
really possible to follow the code path -just from reading trying to phrase it
in a politly manner.

If I remember right you created contentobjects in php code, even if a chain - not
sure about the current code anymore. But it cannot be that we debug your extension
code, and thus the request to provide a stripped down example code which can be
used to reproduce it.

At best as a single "middleware" without your extensions at all so it can directly
be used within the core and eventually also used to create functional tests out of
it (we will not pull in your or any other extensions to create tests).

Can you please provide something which can be used to reproduce it without telling
us to read code/readme/documentation of your extensions, install and setup yourxtensions
and invest time to get "something" running ?

I have no idea where else the duplicate slash inside of file paths will happen.

Okay, thats literally okay for now.

I cannot produce an example with some lines of PHP code which uses the xajax extension to create an image after running its middleware.

Why ? I mean you have something which produces the issue with that middleware. It's hard to "blindly" do something if you cannot provide
the steps to reproduce it.

I have already added the necessary debug output in the attached HTML files to view them in a HTML browser. Tell me if you need more debug infos.

The debug info is not really helpfull. Manual changing code and addeing debug() call changes code and thus LINES, which only you can map to
the correct place. Thus the general question for a guide how to reproduce an issue within TYPO3 core monorepo or providing a simplefied
extension with minor steps.

However, I know invested the last four and half hours to look through the extraordinary code of your tt_products extension and tried to guess
and follow the flow in the one or other way. I guess that following core line

v12: https://github.com/TYPO3/typo3/blob/208b3445ef763c010cf019e8a922be2182a22dc4/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php#L3850

            3 => Environment::getPublicPath() . '/' . $processedFileObject->getPublicUrl(),

which has changed in main(v13) and should be that line now:
https://github.com/TYPO3/typo3/blob/d9323c00a3f61cfc142b598a63205f433b50971b/typo3/sysext/core/Classes/Imaging/ImageResource.php#L61

Environment::getPublicPath() . '/' . $processedFile->getPublicUrl(),

which still does not explain why no one ever found/reported images/processed images with double slashes in it OR not getting the correct image output.

Need to thing about how to reproduce or verify this in pure core.

But sadly, don't have much time for that currently. So if you can at least provide something simple to verify/reproduce it would be helpfull.

For the stripPathSitePrefix() and the double slash issue.

And again why in "middleware" it is not prefixed (fileadmin instead of /fileadmin).

Maybe it is some messup with handling in your code ? I saw quite some places which does not use the getPublicUrl() part of FAL resources and instead using path info and manual concatenation with (`fileadmin`) without slash etc ... maybe that triggers some code path in the core it was not intented because it is feeded wrong.

Actions #16

Updated by Stefan Bürk 3 months ago

@Franz Holzinger

stripPathSitePrefix $path: demoshop12//fileadmin/ Doppelter falscher Slash
PathUtility.php                              #431    ->debug
ImageContentObject.php                      #80    ->stripPathSitePrefix
ImageContentObject.php                      #46    ->cImage
class.tx_ttproducts_field_media_view.php  #511    ->render
class.tx_ttproducts_db.php              #789    ->getCodeMarkerArray
class.tx_ttproducts_db.php              #468    ->generateResponse

The linenumbers does not really match - checked it against ttproducts main and develop.
For following line (you need to find the correct place):

class.tx_ttproducts_field_media_view.php  #511    ->render

where the content object render method is called. It would be helpfull
to get the exect content object configuration used for the render process
in that exact scenario - maybe that helps to get something reproducable.

Can you provide the data used to configure the created contentobject/objectrenderer
for that specific rendering (endin in the doubleslash) ?

Actions #17

Updated by Stefan Bürk 2 months ago

Investigated it further (aka debugging your extensions).

The fact, that the leading slash is missing when using a middleware like your xajax dispatcher middleware is,
that the related core code which registers the aforementioned frontend `PublicUrlPrefixer` is contained within
the frontend RequestHandler class, which itself is a "middleware". But it is the tip middleware and will be shifted
and thus beeing the very last middleware in the chain.

That class literally does two basic things (beside a lot of other stuff):

  • ensure that TypoScriptFrontendController properly determines the absRefPrefix based on TypoScript, SiteConfig and
    request normalizedParams
  • register dynamically the PublicUrlPrefix listener for the frontend

Your xajax middleware is executed "before" that, which means that neither the event is registered, nor the
absRefPrexis calculated and set as TSFE property.

The absRefPrefix calculation is already covered by your middleware:

$GLOBALS['TSFE']->preparePageContentGeneration($request);

First thing, for v13 you should not use the global variable anymore and instead retrieve it from the request:

$controller = $request->getAttribute('frontend.controller');
if ($controller instanceof TypoScriptFrontendController) {
  // required to calculate/set absRefPrefix correctly
  $controller->preparePageContentGeneration($request);
}

a dual version code could look like:

$controller = $request->getAttribute('frontend.controller') ?? $GLOBALS['TSFE'] ?? null;
if ($controller instanceof TypoScriptFrontendController) {
  // required to calculate/set absRefPrefix correctly
  $controller->preparePageContentGeneration($request);
}

The second thing would be to register the PublicUrlPrefixer event listener:

// use TYPO3\CMS\Core\EventDispatcher\ListenerProvider;
// use TYPO3\CMS\Core\Resource\Event\GeneratePublicUrlForResourceEvent;
// use TYPO3\CMS\Frontend\Resource\PublicUrlPrefixer;

$listenerProvider = GeneralUtility::makeInstance(ListenerProvider::class);
$listenerProvider->addListener(
  GeneratePublicUrlForResourceEvent::class,
  PublicUrlPrefixer::class,
  'prefixWithAbsRefPrefix'
);

Better would be to get the listener provider as constuctor injection (DI),
but your taxajax extension misses the default DI Sevices.yaml snippent and
you completly deniy DI in your extension.

I can still reproduce now the "double slash" in the path (not in middleware),
for which I will create a dedicated issue and fix it - however, TYPO3 itself
seems not to have an issue with that yet and that needs some more digging.

I also leave this issue open, and decide later if we really change the
stripPathSitePrefix to make it more tighten (ensure no leading slash)

To summerize it - you execute stuff to early and thus missing setup is
missing, which is not a bug of the core. In these cases, you need to do
it yourself like described above. On top of that, you should ensure to
provide the correct combined storage identifier instead of a simple
fileadmin path to the content object to avoid using the "fallback" FAL
storage:

1:/some/file.png

instead of

fileamdin/some/file.png

I have created a gist for a demo middleware doing a content object
rendering within a middleware, literally simulating the taxajax ext
approach and containing the aforementioned requrired changes:

https://gist.github.com/sbuerk/7a83fdad71c002796903738f7c2e5f90

Actions #18

Updated by Franz Holzinger about 2 months ago

Thank you Stefan Bürk.
I have incorporated most of your recommendations into the extension taxajax version 1.3.0 . With these modifications this error does not happen.

It however would be helpful if the `stripPathSitePrefix` method would always behave in the same manner with leading slashes.

Actions

Also available in: Atom PDF