FPA filename is defined differently, in two places

sozzled commented 4 years ago

In https://github.com/ForumPostAssistant/FPA/tree/2020-UI-Facelift,

lines 103-108 ` //$fpafile = _FPA_SELF; $fpafile = 'fred.php';

    if ( file_exists($fpafile) ) {
        $fileinfo = stat( $fpafile );
    }

and, again, lines 514-552 //$fpaFilename = _FPA_SELF; $fpaFilename = 'fred.php';

    // try to set script to 777 to make sure we have permission to delete
    @chmod($fpaFilename, 0777);  // octal; correct value of mode

    // Delete the file.
    @unlink($fpaFilename);

    // Message and link to home page of site.
    // if SSL return to https:// otherwise http://
    if ( @$_SERVER['HTTPS'] == 'on' ? $hostPrefix = 'https://' : $hostPrefix = 'http://');
    $page = $hostPrefix . $host;

    // Something went wrong and the script was not deleted so it must be removed manually so we tell the user to do so - PhilD 8-07-12
    if ( file_exists($fpaFilename) ) {
        @chmod($fpaFilename, 0644);  // octal; correct value of mode

        echo '<div id="deleteMessage" style="padding:20px;border:1px solid #e99002;background-color:#fff8ee;margin:0 auto;margin-top:50px;margin-bottom:20px;max-width:70%;position:relative;z-index:9999;top:10%;font-family:sans-serif, arial;" align="center">';
        echo '<h1 style="color:#e99002;font-size:44px;">SOMETHING WENT WRONG!</h1>';
        if ( defined('_FPA_SELF_DESTRUCT_DOIT') ) {
            echo '<h2 style="color:#43ac6a;">As a security measure, FPA attempted to self-delete itself due to the time it has been present on the server, but was not successful.</h2>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">Please remove the file manually using FTP or through your hosting File Manager, or upload a new copy to continue using it.</p>';

        } else {
            echo '<h1 style="color:#e99002;font-size:44px;">SOMETHING WENT WRONG!</h1>';
            echo '<p style="color:#e99002;font-size:30px;">We could not delete the FPA file ('. $fpaFilename .').</p>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">For your website security, please remove the file <em style="color:#f04124;">'. $filename .'</em> manually using FTP or through your hosting File Manager.</p>';
        }

    } else {
        echo '<div id="deleteMessage" style="padding:20px;border:1px solid #43ac6a;background-color:#effff5;margin:0 auto;margin-top:50px;margin-bottom:20px;max-width:70%;position:relative;z-index:9999;top:10%;font-family:sans-serif, arial;" align="center">';
        if ( defined('_FPA_SELF_DESTRUCT_DOIT') ) {
            echo '<h2 style="color:#43ac6a;">As a security measure, this copy of FPA has been self-deleted due to the time it has been present on the server.</h2>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">You will need to upload another copy of FPA to continue.</p>';
        } else {
            echo '<h1 style="color:#43ac6a;">Thank You For Using The FPA.</h1>';
        }
    }

`

If I knew how to create a PR I would write the code. Instead, all I can do is explain in words:

[ ] Define (in one place) the variable $fpaFilename before line 104 (say, at line 86).
[ ] Delete lines 103-104
[ ] Change lines 106-107, replace $fpafile with $fpaFilename
[ ] Delete lines 514-515
[ ] Change line 541, replace $filename with $fpaFilename ($filename is not defined anywhere)

RussW commented 4 years ago

As mentioned on several occasions, from my perspective at least;

This version was/is primarily a GUI Rewrite with a few new features - refactoring of existing functions can be done later when we start looking more in to the post output changes that maybe needed to match the GUI and cater for the larger post problems.

sozzled commented 4 years ago

@RussW

The line number references relate to your rewrite. There's bit of dodgy code (esp. at line 541). If I could write a PR I would make the changes and do things for you. I can't write a PR because GitHub to me is like practising my religion in someone else's temple.

RussW commented 4 years ago

"dodgy"? quite what's dodgy about it?

If ya can't PR it, then rewrite it to a single function and cut a new branch for review

sozzled commented 4 years ago

I don't know how to "rewrite it to a single function and cut a new branch for review"; you're talking a who new language to me.

Look, it's simple. I could explain it in less than one minute with a telephone call (or a Skype videoconference call).

In your rewrite, you use three different variable names to refer to, essentially, the same thing. One of those variables is undefined (and, therefore, it that line of code is ever executed, it will result in a run-time error). That's what I call dodgy.

sozzled commented 4 years ago

There are two sets of changes:

$fpaFilename = _FPA_SELF;

/**
 * FPA Self Destruct
 * comment-out _FPA_SELF_DESTRUCT in Default Settings to disable
 * (there is no need to comment-out the _FPA_SELF_DESTRUCT_AGE constant)
 *
 * if enabled, checks the FPA file date and if over _FPA_SELF_DESTRUCT_AGE days old then run the self-delete script
 *
 * CONSTANTS are used throughout this feature as a security measure because they cannot be overriden at runtime
 * added @RussW 30/05/2020
 *
 */
define ( '_FPA_SELF_DESTRUCT_AGE', 7 );       // age of FPA file before _FPA_SELF_DESTRUCT runs (set as CONSTANT so it can't be changed/overridden at runtime)
if ( defined('_FPA_SELF_DESTRUCT') AND ( !defined('_FPA_DEV') AND !defined('_FPA_DIAG') ) ) {

    if ( file_exists($fpaFilename) ) {
        $fileinfo = stat($fpaFilename);
    }

Here's the second set

if ( ( isset($_POST['act']) AND $_POST['act']  == 'delete' ) OR ( defined('_FPA_SELF_DESTRUCT') AND defined('_FPA_SELF_DESTRUCT_DOIT') ) ) {
    $host        = $_SERVER['HTTP_HOST'];
    $uri         = rtrim(dirname(_FPA_SELF), '/\\');
    $extra       = ''; // add index (or other) page if desired

    // try to set script to 777 to make sure we have permission to delete
    @chmod($fpaFilename, 0777);  // octal; correct value of mode

    // Delete the file.
    @unlink($fpaFilename);

    // Message and link to home page of site.
    // if SSL return to https:// otherwise http://
    if ( @$_SERVER['HTTPS'] == 'on' ? $hostPrefix = 'https://' : $hostPrefix = 'http://');
    $page = $hostPrefix . $host;

    // Something went wrong and the script was not deleted so it must be removed manually so we tell the user to do so - PhilD 8-07-12
    if ( file_exists($fpaFilename) ) {
        @chmod($fpaFilename, 0644);  // octal; correct value of mode

        echo '<div id="deleteMessage" style="padding:20px;border:1px solid #e99002;background-color:#fff8ee;margin:0 auto;margin-top:50px;margin-bottom:20px;max-width:70%;position:relative;z-index:9999;top:10%;font-family:sans-serif, arial;" align="center">';
        echo '<h1 style="color:#e99002;font-size:44px;">SOMETHING WENT WRONG!</h1>';
        if ( defined('_FPA_SELF_DESTRUCT_DOIT') ) {
            echo '<h2 style="color:#43ac6a;">As a security measure, FPA attempted to self-delete itself due to the time it has been present on the server, but was not successful.</h2>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">Please remove the file manually using FTP or through your hosting File Manager, or upload a new copy to continue using it.</p>';

        } else {
            echo '<h1 style="color:#e99002;font-size:44px;">SOMETHING WENT WRONG!</h1>';
            echo '<p style="color:#e99002;font-size:30px;">We could not delete the FPA file ('. $fpaFilename .').</p>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">For your website security, please remove the file <em style="color:#f04124;">'. $fpaFilename .'</em> manually using FTP or through your hosting File Manager.</p>';
        }

    } else {
        echo '<div id="deleteMessage" style="padding:20px;border:1px solid #43ac6a;background-color:#effff5;margin:0 auto;margin-top:50px;margin-bottom:20px;max-width:70%;position:relative;z-index:9999;top:10%;font-family:sans-serif, arial;" align="center">';
        if ( defined('_FPA_SELF_DESTRUCT_DOIT') ) {
            echo '<h2 style="color:#43ac6a;">As a security measure, this copy of FPA has been self-deleted due to the time it has been present on the server.</h2>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">You will need to upload another copy of FPA to continue.</p>';
        } else {
            echo '<h1 style="color:#43ac6a;">Thank You For Using The FPA.</h1>';
        }
    }

You'll have to figure out, for yourself, which lines I've added, which lines I've changed and which lines I've deleted.

I don't know who, or why, someone came up with the idea to create a variable to store the FPA filename as a string literal (seems to have been introduced a dozen releases or more ago). The re-working I've done will obviate the need to remember to change some filename used in testing (e.g. "fred.php") packaging the FPA script for release.

sozzled commented 4 years ago

Sorry, the first line of my changes isn't right. The line should read instead

$fpaFilename = basename($_SERVER['PHP_SELF']);

RussW commented 4 years ago

@sozzled last commit updates both the auto-destruct and delete routines now consistently use the same _FPA_SELF constant plus added the full return URL to the 4page variable (this takes you back to the site now instead of the top-level domain and caters for being in sub-directories, that I removed in error previously NOTE: _FPA_SELF NOW points to fpa itself, not a test file (fred.php) there is a commented out test constant still there for the moment if preferred

sozzled commented 4 years ago

Tried it (by uncommenting the line or lines that are commented for test purposes): fails.

RussW commented 4 years ago

without any further information about quite what fails or how...

I would assume that you uncommented the test constant pointing to the fred.php file as the delete target, BUT didn't comment out the line above, so there would be a "_FPA_SELF already defined error"

Either use one or the other definition, NOT BOTH (the fred.php target definition will be deleted once this has been tested, it's only there to avoid having to keep copying the fpa script around, if it's easier and less confusing I'll re-commit with only the one constant defined that will target the fpa script)

sozzled commented 4 years ago

I don't want a big argument over this but this area of the FPA has been buggy for a long time.

Theoretically, it shouldn't matter diddly-squat what the file is called (as long as the file is placed in the correct folder in the filesystem). The filename is only important when it comes to needing to (a) obtain the time when it was created, compare it to now, and if now is more than x days after the file was created then delete the mongrel, and (b) when you need to know the file to be deleted when you press the "delete now" button.

Those are the two requirements.

The constant _FPA_SELF is used in a couple of places (but the constant could be replaced with a variable). Defining this as constant may be suitable if the constant is translatable into another language, I suppose, but why bother? It sufficient to store the filename in a variable, somewhere, but we only need one definition of that variable. The problem with the way things are built now is that the usage (constant/variable) is inconsistent, multiple-defined, difficult to follow and will make things harder to maintain.

(On a personal note, I dislike "elegant" programming; I like minimalist programming but only insofa as I can pick up a program in a few years from now and say, "Ahh, that's what I was thinking at the time"

Why is there a need to look for a file called "fred.php" for testing purposes? That's actually the real question.

The attached file (sort of works): I haven't been able to get the auto self-delete after n days to work, yet.

fpa-en.txt

RussW commented 4 years ago

the latest commit has removed the test/dev _FPA_SELF constant, there is no need to comment/uncomment anything any more

the _FPA_SELF constant targets basename( $_SERVER['PHP_SELF'] )

this is done as is takes in to account if the user has changed the filename
use of a constant (instead of a variable, as mentioned in the comments) is because a variable cannot be changed after run-time, unlike a variable, which means that the delete routines cannot be traversed or injected with other filenames at run-time.

Notes:

_there maybe inconsistencies throughout fpa at the moment that do not use this new constant ( _FPASELF), they will be found, interrogated and shot at dawn once the delete routines are done
the delete routines may not work in all cases, on all platforms based on permissions, ownership and hardening, we can only try

sozzled commented 4 years ago

Fair enough. I'll try it later.

sozzled commented 4 years ago

Lines 523-551 of latest commit (reworked by me) are shown below:

    // Something went wrong and the script was not deleted so it must be removed manually so we tell the user to do so - PhilD 8-07-12
    if ( file_exists(_FPA_SELF) ) {
        @chmod(_FPA_SELF, 0644);  // octal; correct value of mode

        echo '<div id="deleteMessage" style="padding:20px;border:1px solid #e99002;background-color:#fff8ee;margin:0 auto;margin-top:50px;margin-bottom:20px;max-width:70%;position:relative;z-index:9999;top:10%;font-family:sans-serif, arial;" align="center">';
        echo '<h1 style="color:#e99002;font-size:44px;">SOMETHING WENT WRONG!</h1>';
        if ( defined('_FPA_SELF_DESTRUCT_DOIT') ) {
            echo '<h2 style="color:#43ac6a;">As a security measure, FPA attempted to self-delete itself due to the time it has been present on the server, but was not successful.</h2>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">Please remove the file manually using FTP or through your hosting File Manager, or upload a new copy to continue using it.</p>';

        } else {
            echo '<h1 style="color:#e99002;font-size:44px;">SOMETHING WENT WRONG!</h1>';
            echo '<p style="color:#e99002;font-size:30px;">We could not delete the FPA file ('. _FPA_SELF .').</p>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">For your website security, please remove the file <em style="color:#f04124;">'. _FPA_SELF .'</em> manually using FTP or through your hosting File Manager.</p>';
        }

    } else {
        echo '<div id="deleteMessage" style="padding:20px;border:1px solid #43ac6a;background-color:#effff5;margin:0 auto;margin-top:50px;margin-bottom:20px;max-width:70%;position:relative;z-index:9999;top:10%;font-family:sans-serif, arial;" align="center">';
        if ( defined('_FPA_SELF_DESTRUCT_DOIT') ) {
            echo '<h2 style="color:#43ac6a;">As a security measure, this copy of FPA has been self-deleted due to the time it has been present on the server.</h2>';
            echo '<p style="color:#e99002;font-size:20px;margin:0 auto;max-width:80%;">You will need to upload another copy of FPA to continue.</p>';
        } else {
            echo '<h1 style="color:#43ac6a;">Thank You For Using The FPA.</h1>';
        }
    }

    echo '<p><a href="'. $page .'">Go to your Home Page.</a></p>';
    echo '</div>';
    exit;

Look carefully at lines 535 and 536. I've changed the use of a variable to the constant _FPA_SELF. This did not cause any run-time errors but I don't know how the "if the file wasn't deleted" path is executed. How can I test that? Change the file to "read only" perhaps? (I tried it but it didn't do anything on my Wampserver installation.) Do you have a better idea?

The new work (even with my two changes) works, regardless of whether the file is names fpa_en.php or something.php.

These changes fix a long-standing problem with the production version of the FPA.

sozzled commented 4 years ago

Not sure of the meaning of line 102

if ( defined('_FPA_SELF_DESTRUCT') AND ( !defined('_FPA_DEV') AND !defined('_FPA_DIAG') ) ) {

What is the purpose? Does it matter if _FPA_DEV and _FPA_DIAG are undefined? Suggest that line 101 should be changed in production to the following:

define ( '_FPA_SELF_DESTRUCT_AGE', 1 );       // age of FPA file before _FPA_SELF_DESTRUCT runs (set as CONSTANT so it can't be changed/overridden at runtime)

One day should be enough. If the file is auto deleted, the owner can put a new version there. Reducing the time wait reduces the opportunity for someone else running the FPA and (possibly) obtaining some private information.

RussW commented 4 years ago

Yeah, I missed those two variable changes, thanks - updated in the latest commit.

Regarding line 102 - just covering all the bases ... The _FPA_SELF_DESTRUCT is being disabled there if _FPA_DEV or _FPA_DIAG (which enable error_display) are enabled, this means if required we can enable either of them and test or troubleshoot without fpa hitting _FPA_SELF_DESTRUCT_AGE, alternatively, _FPA_SELF_DESTRUCT can be disabled itself during testing and development

As for the _FPA_SELF_DESTRUCT_AGE, 7 days was an arbitrary choice I made at the time, I guess it's up for discussion, but I wouldn't want it lower than 4-5 days, otherwise users may end up keep having to upload fpa if troubleshooting takes a little longer than usual and it'll bug the crap out of them and they'll stop using it

RussW commented 4 years ago

Regarding testing if the file wasn't deleted - On Windows home PC environments the only thing I can think of is to change the file owner to another user and change the permissions to something else but the usual "Everyone, Full Control", but even then I'm not that will work because of the way windows ACLs work on PC editions against Server Editions. You may have to put it up on a server and mess with the ownership & permissions there.

I'm going to open a separate issue for the _FPA_SELF_DESTRUCT_AGE as this is a different query to the original purpose of this issue. https://github.com/ForumPostAssistant/FPA/issues/68

ForumPostAssistant / FPA

FPA filename is defined differently, in two places #66