[3.0] curl fix

[enygma]

Not that there's any content there, but you can hit that page without a user being logged in.

[panique]

I'm sure you can not. dashboard/index clearly directs to login/index. Can you give details where exactly you see that error ? Maybe I've missed something...

[enygma]

$url = 'https://demo-professional.php-login.net/dashboard/index';
$ch = curl_init();
$opts = array(
    CURLOPT_URL => $url,
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_HEADER => true,
    CURLOPT_POST => true
);
curl_setopt_array($ch, $opts);

$result = curl_exec($ch);
echo 'RESULT: '.var_export($result, true)."\n\n";

I see it redirecting in the browser and get the 302, but unless my script is told to follow redirects, I still get the dashboard page.

[panique]

Excellent! Can you give a direction how to solve this ? On Apache-side (with settings) or replacing header('location ... or something else ?

Current implementation: https://github.com/panique/php-login/blob/master/application/libs/Auth.php does header('location: ' . URL . 'login'); when there's no active session.

[enygma]

Seems like you're already catching the "logged in" error in other places (wherever that 302 is thrown), so I wonder if it'd be possible to switch out the view with an "access denied" one and return that immediately instead of dropping into the controller. I haven't looked to see if this would be feasible, but it's an idea.

[Dominic28]

I can confirm this "error". Try out something like this:

Auth::handleLogin();

if (1 == 1) {
    header("Location: ".URL);
}

If you're not logged-in you should be redirected to the login page, but you'll be redirected to the page of the second header().

Of course this can be simple fixed in this case, but this is just a simple example.

[slaveek]

Terminate script at very end of handleLogin() method in Auth class work in both ( @enygma and @Dominic28 ) cases.

if (!isset($_SESSION['user_logged_in'])) {
    Session::destroy();
    header('location: ' . URL . 'login');
    exit();
}

However I am not quite sure that is good solution / idea / practice.

[ghost]

It's a nice idea, although it will break tests (it will not return any evaluation of the test), but (!) for the production version it could be a good solution.

[panique]

@slaveek @ALL Should we introduce slaveek's fix into the 2.0 version or leave everything like it is and then implement it into the 3.0 version in early 2015 ?

3.0 might need another architecture approach to avoid this.

And a little side-fact, just to let you sleep better: Even if you can reach the VIEW (!) in this case via curl, you'll not see any real information, as the methods don't return anything, as there's no active session which could be used to fetch a user's information. No session, no data.

[Dominic28]

I think we should fix this for 2.0 as logged out users could see contents they shouldn't. In my case I got functions which don't need any session data but should only be visible for logged in users.

[slaveek]

@Dominic28 is right.

For a quick test I changed the mysql query in getAllNotes() method in NoteModel from this:
SELECT user_id, note_id, note_text FROM notes WHERE user_id = :user_id
to this
'SELECT user_id, note_id, note_text FROM notes

In this case the method don't need any session data ( user_id ).
Then I run @enygma script

$url = 'https://127.0.0.1/note/index';
$ch = curl_init();
$opts = array(
    CURLOPT_URL => $url,
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_HEADER => true,
    CURLOPT_POST => true
);
curl_setopt_array($ch, $opts);

$result = curl_exec($ch);
echo 'RESULT: '.var_export($result, true)."\n\n";

I can see all notes but should only be visible for logged in users.

So if someone already extended their project with some independent from session data functions, should check that and fix it.

[panique]

@slaveek Okay! But in all fairness: Showing user data without checking user's session first is a really bad idea in general. I'll add the fix after some tests...

[Dominic28]

Showing user data without checking sessions is bad of course, but let's think about internal posts or articles which should only be readable for logged in users.

[panique]

@ALL Are there ANY negative effects of implementing this one-line exit(); by @slaveek ? I couldn't find one, just tested this on in-development and live-projects without problems.

The breaking unit tests (reported by @KatzArie) are a problem for sure, but as this project ships without tests, the fact that these potential tests are fixable quite easy (at least i think so) and the increased application security are pro-arguments FOR a quick implementation into the master branch.

If there're no big concerns, then I'll merge a patch into master-branch within the next few hours.

Big thanks to everybody involved in discovering, testing and reporting this bug!

[panique]

Okay, it's fixed in master branch (which holds 2.0).
Side-note: I've fixed this via a patch-branch, not via develop, as develop already holds changes for the 3.0 version.

[panique]

Fixed in develop branch, the hard way: via exit();. This is not nice for unit tests (as it stops the entire PHP process), but security is more important for now.

I have this on my todo-list, a proper solution will come for sure!