Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contao 3 methods: Controller::sendFileToBrowser() #4696

Closed
aschempp opened this issue Aug 22, 2012 · 29 comments

Comments

Projects
None yet
6 participants
@aschempp
Copy link
Contributor

commented Aug 22, 2012

@contao/workgroup-core

This should be File::sendToBrowser(). The current routine is already creating an instance of File class. The check for "hacking the file system" should probably be done in the File constructor then?

@Toflar

This comment has been minimized.

Copy link
Member

commented Nov 6, 2012

Can we please change File::__construct($strFile); to File::__construct($strFile, $blnCreateIfNotExists=true) together with that ticket?
It doesn't break BC and the devs don't have to wrap new File($path)-calls in if (is_file()) or if (file_exists()) everytime they use the class. They can just do new File($path, false); instead :-) Thx!

@leofeyer

This comment has been minimized.

Copy link
Member

commented Nov 6, 2012

Isn't that even more effort? If a file does not exist, why would you want to create a related File object – except if you want to create the file? The File class does the is_file() check anyways, but if the file does not exist, no unnecessary File object is created when you do the is_file() check before.

@Toflar

This comment has been minimized.

Copy link
Member

commented Nov 6, 2012

Because I want to work with the file and I don't necessarily know if it exists or not. But currently if I'm checking for a file e.g. in the temporary folder, I have to check if it's there first because I don't want it to be created automatically.
But the File class provides a lot of super nice things like getting information about size or mtime and provides methods to move the file around.

e.g. let's say you want to display the image with its size if it exists or a message if it doesn't exist. Current way to go

<?php

if (!is_file($strPath))
{
      echo 'No way!';
}
else
{
   $objFile = new File($strPath);
   print_r($objFile->size);
}

Best:

<?php

$objFile = new File($strPath);
if ($objFile->exists())
{
   print_r($objFile->size);
}
else
{
   echo 'No way!';
}

But this is not quite possible right now due to BC breaks. But what is possible is this:

<?php

$objFile = new File($strPath, false);
if ($objFile->exists())
{
   print_r($objFile->size);
}
else
{
   echo 'No way!';
}

The whole thing is about OOP. I want to work with an object, not with functions :-)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Nov 9, 2012

I agree to the third variant. But with the current implementation, exists() would always return true.

@Toflar

This comment has been minimized.

Copy link
Member

commented Nov 9, 2012

Yes, you got it :) It would always return true unless you add false as second parameter.
In Contao 4 the second parameter should be removed and the file shouldn't be created automatically but only by calling File::createIfNotExists() or something similar. But this is a matter of taste :)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jan 29, 2013

@Toflar, it turned out that my actual Files implementation candidate for Contao 3 somehow got lost in a merge conflict, which leads to quite some problems (see #5307 and #5259). I have therefore added a separate branch with the actual implementation in c75e2a9 and since the whole file creation logic is moved to the close() method, I have also added your idea (an exists() method).

Can you please review and also let me know what you think about when to add this BC breaking fix?

@Toflar

This comment has been minimized.

Copy link
Member

commented Jan 30, 2013

Difficult to say. Although I like your implementation in general, it definitely is a BC break. Before new File() always created a file, now it only does when you first write into it. That's going to cause troubles in many extensions - I'm pretty sure about that. So I have to vote for introducing it in 3.1.

Is there no way to fix the current caching problem without changing the File class? A temporary one for C3 only?

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2013

How about a flag for construct, that defaults to $blnCreate=true ?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jan 30, 2013

The only other fix I can think of would be using file locks, however, those are known to cause more issues than they solve. BTW, if we should decide not to add the fix because it breaks BC, the next possible release would be Contao 4!

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 6, 2013

We were drifting away a little bit, so back to the actual issue:

I think that certain parts of Controller::sendFileToBrowser() do not belong into the File class. I am talking about "Make sure there are no attempts to hack the file system", "Limit downloads to the files directory" and "Check whether the file type is allowed to be downloaded".

If we added a File::sendToBrowser() method, it should be able to send any file to the browser, not just the ones from the downloads folder or the allowed types. The Controller method cannot be removed, because it also validates the user input and adds extra security.

So I vote for leaving it as is.

@tristanlins

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2013

Limit downloads to the files directory

This can also be done in the File::sendToBrowser() but I think it is the wrong place for this.
This "security feature" make it impossible for a developer to send generated files or files from a module directory.
The benefit of this is just to protect the download modules, but imo this protection should be done in the module instead of the send method.

Anyway what about three methods to provide:
Controller::sendFileToBrowser() same as it is, only files from uploadDir are allowed.
File::sendToBrowser() for developers, does not limit download to uploadDir.
Controller::sendToBrowser($string, $mime) send a string as file to the browser.

I needed the last one many times and I always have to "copy&paste" the ~10 lines or more to send generated files to the browser. :-/

@backbone87

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2013

Or we could be more OOP and use an FileSend object. I have one in my download extension that I am willing to sponsor, if it would be included in the Core.
It supports additional Features like:

  • Cache-Headers (may be extended)
  • Content-Disposition
  • Ranges (Full-Spec)
  • Bandwidth limiting
  • Partial sends
  • Binary prepends (useful for FLV pseudo streaming)

Edit:
Although stream support could be easily included, so we can use stream_copy_to_stream, which should support DMA/MemoryMapping someday.

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 7, 2013

If we added a File::sendToBrowser() method, it should be able to send any file to the browser, not just the ones from the downloads folder or the allowed types. The Controller method cannot be removed, because it also validates the user input and adds extra security.

Your absolutely right. That's why different purposes should be in different methods :) So it should be

$objFile = new File('$strPath, true);
if ($objFile->isAllowedForDownload())
{
    $objFile->sendToBrowser();
}

Probably we can include Oliver's FileSend object or ask him to do a pull request once the sendToBrowser is included in the core ;-)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 8, 2013

@Toflar, as I said above, I don't think that isAllowedForDownload() belongs into the File class. Try to see the library classes independent from the CMS, so they can be reused in other contexts. The File class needs a method to send a file to the browser. However, whether this action is allowed or not, has to be decided by the controller.

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 8, 2013

I see your point. Still the Controller method should rely on File::sendToBrowser() but I guess that was your initial plan anyway :) Not sure checking for allowed file downloads is a Controller responsibility though. But I agree that if you want to separate CMS functionality from the file handling context (which is the right way to go) it should not be done as I suggested in the first place.
But also, if you explain your decision this way, you have to modify a lot. Especially the whole Files class is heavily CMS dependent because all the config things like $GLOBALS['TL_CONFIG']['useFTP'] should be injected from the outside, not by global configuration parameters that are set by the CMS ;-)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 8, 2013

I did not say that the libraries shall be independent from the global configuration! That is hardly possible and not necessary either. I only said that the library classes shall be independent from the CMS, so they can be reused in other contexts. And other contexts are likely to have a configuration file, too :)

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 8, 2013

But the global configuration IS the CMS! So what you're saying is that the method isAllowedForDownload is CMS dependent but what would it look like? Something like this, right?

public function isAllowedForDownload()
{
    $allowedDownload = trimsplit(',', strtolower($GLOBALS['TL_CONFIG']['allowedDownload']));
    return in_array($this->extension, $allowedDownload);
}

So this is also a global configuration, isn't it? Where's the difference between this idea and the Files class that establishes the connection based on the global configuration? ;-)
I don't want you to implement a method isAllowedForDownload() by all means. I just think if you don't want to do this because this is "a CMS specific thing" it should also be handled the same way in other classes ;-)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 8, 2013

It could. Still the access control logic belongs to the Controller.

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 8, 2013

Yes, it does not belong to the File class, I agree. So changing this would be a good first step that should be followed by a second one that makes the Files class system independent. But that's for another framework maybe.
This is okay for me!

@backbone87

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2013

In my opinion it belongs to the Config class:
Config::isFileDownloadAllowed($extensionOrMime)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 15, 2013

Implemented in 4e30f9a.

@leofeyer leofeyer closed this Feb 15, 2013

@Toflar

This comment has been minimized.

Copy link
Member

commented Feb 15, 2013

Imho

        // Make sure no output buffer is active
        // @see http://ch2.php.net/manual/en/function.fpassthru.php#74080
        while (@ob_end_clean());

        // Prevent session locking (see #2804)
        session_write_close();

should also go in there ;-)

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 15, 2013

I was not sure about it. It is actually only required if a session is opened and ob_start() is used. This is the case in Contao, but since we are seeing the classes as decoupled libraries, we should not enforce it.

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2013

IMO The problem is in bigger scope.

With the current implementation the developer is forced to clean the ob on his own, write the session and exit the script then, marginal benefit for the devs, as the only thing they gained is not writing the headers by themselves.

In addition, a File::passthru() method would come in handy here as well.

Please prove me wrong on this but to my understanding an developer either wants:

  • a file sent to the broweser for download
  • or send one or multiple files concatenated in RAW to the browser.

For the first one, the script should alter the headers clean the outputbuffer (if any), write the session and exit PHP which will then break the postDownload HOOK in Controller. BUMMER!

For the second one, the headers MUST NOT be altered as the File::sendToBrowser() does currently, as it will most likely corrupt everything.

By implementing an optional bool $blnExit parameter defaulting to false in addition to also implementing above mentioned File::passthru() method, both approaches can be satisfied IMO.

    /**
     * Send the file to the browser
     * @param bool $blnAsDownload optional parameter, if true, the output buffer will get cleaned and the session will get closed (if started). The script will also be terminated
     */
    public function sendToBrowser($blnAsDownload = false)
    {
        if ($blnAsDownload)
        {
            // Make sure no output buffer is active
            // @see http://ch2.php.net/manual/en/function.fpassthru.php#74080
            while (@ob_end_clean());

            // Prevent session locking (see #2804)
            if(session_id())
            {
                session_write_close();
            }
        }
        header('Content-Type: ' . $this->mime);
        header('Content-Transfer-Encoding: binary');
        header('Content-Disposition: attachment; filename="' . $this->basename . '"');
        header('Content-Length: ' . $this->filesize);
        header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
        header('Pragma: public');
        header('Expires: 0');
        header('Connection: close');

        $this->passThru();
        if ($blnAsDownload)
        {
            exit;
        }
    }

    /**
     * Passthru the file
     */
    public function passThru()
    {
        $resFile = fopen(TL_ROOT . '/' . $this->strFile, 'rb');
        fpassthru($resFile);
        fclose($resFile);
    }

What do you think?

@backbone87

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2013

Just as a sidenote: This is all available in my FileSend class.

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2013

How can there ever be a case where passing a file to the browser is not a download?

@discordier

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2013

Of course there can... dump multiple *.md files as one big file i.e. or something like that.

concatenating several XML files (with echoing the encapsulating tag before/after of course) also comes into mind.

Last but not least, what if I want do dump a file which will NOT be presented with "content-disposition: foobar.ext" and therefore be rendered inline in the browser?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Mar 5, 2013

We have discussed all the options in the workgroup and here is how the method should look like in our opinion:

/**
 * Send the file to the browser
 */
public function sendToBrowser()
{
    // Make sure no output buffer is active
    // @see http://ch2.php.net/manual/en/function.fpassthru.php#74080
    while (@ob_end_clean());

    // Prevent session locking (see #2804)
    session_write_close();

    header('Content-Type: ' . $this->mime);
    header('Content-Transfer-Encoding: binary');
    header('Content-Disposition: attachment; filename="' . $this->basename . '"');
    header('Content-Length: ' . $this->filesize);
    header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
    header('Pragma: public');
    header('Expires: 0');
    header('Connection: close');

    $resFile = fopen(TL_ROOT . '/' . $this->strFile, 'rb');
    fpassthru($resFile);
    fclose($resFile);

    exit;
}

@leofeyer leofeyer reopened this Mar 5, 2013

@leofeyer

This comment has been minimized.

Copy link
Member

commented Mar 15, 2013

Changed in 848beda.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.