URL Handling: Removed referrer-based redirect handling

Swapped back handling to instead be pre-determined instead of being based upon session/referrer which would cause inconsistent results when referrer data was not available (redirect to app-loaded images/files). To support, this adds a mechansism to provide a URL through request data. Also cleaned up some imports in code while making changes. Closes #4656.
2024-09-20 00:06:42 +00:00 · 2023-12-10 12:37:21 +00:00 · 2023-12-10 12:37:21 +00:00 · 45ce7a7126
commit 45ce7a7126
parent 11955e270c
17 changed files with 71 additions and 47 deletions
--- a/app/Access/Controllers/ForgotPasswordController.php
+++ b/app/Access/Controllers/ForgotPasswordController.php
@ -9,11 +9,6 @@ use Illuminate\Support\Facades\Password;

 class ForgotPasswordController extends Controller
 {
-    /**
-     * Create a new controller instance.
-     *
-     * @return void
-     */
    public function __construct()
    {
        $this->middleware('guest');
@ -30,10 +25,6 @@ class ForgotPasswordController extends Controller

    /**
     * Send a reset link to the given user.
-     *
-     * @param \Illuminate\Http\Request $request
-     *
-     * @return \Illuminate\Http\RedirectResponse
     */
    public function sendResetLinkEmail(Request $request)
    {
@ -56,13 +47,13 @@ class ForgotPasswordController extends Controller
            $message = trans('auth.reset_password_sent', ['email' => $request->get('email')]);
            $this->showSuccessNotification($message);

-            return back()->with('status', trans($response));
+            return redirect('/password/email')->with('status', trans($response));
        }

        // If an error was returned by the password broker, we will get this message
        // translated so we can notify a user of the problem. We'll redirect back
        // to where the users came from so they can attempt this process again.
-        return back()->withErrors(
+        return redirect('/password/email')->withErrors(
            ['email' => trans($response)]
        );
    }
--- a/app/Access/Controllers/ResetPasswordController.php
+++ b/app/Access/Controllers/ResetPasswordController.php
@ -66,7 +66,7 @@ class ResetPasswordController extends Controller
        // redirect them back to where they came from with their error message.
        return $response === Password::PASSWORD_RESET
            ? $this->sendResetResponse()
-            : $this->sendResetFailedResponse($request, $response);
+            : $this->sendResetFailedResponse($request, $response, $request->get('token'));
    }

    /**
@ -83,7 +83,7 @@ class ResetPasswordController extends Controller
    /**
     * Get the response for a failed password reset.
     */
-    protected function sendResetFailedResponse(Request $request, string $response): RedirectResponse
+    protected function sendResetFailedResponse(Request $request, string $response, string $token): RedirectResponse
    {
        // We show invalid users as invalid tokens as to not leak what
        // users may exist in the system.
@ -91,7 +91,7 @@ class ResetPasswordController extends Controller
            $response = Password::INVALID_TOKEN;
        }

-        return redirect()->back()
+        return redirect("/password/reset/{$token}")
            ->withInput($request->only('email'))
            ->withErrors(['email' => trans($response)]);
    }
--- a/app/Access/Controllers/SocialController.php
+++ b/app/Access/Controllers/SocialController.php
@ -91,7 +91,7 @@ class SocialController extends Controller
            return $this->socialRegisterCallback($socialDriver, $socialUser);
        }

-        return redirect()->back();
+        return redirect('/');
    }

    /**
--- a/app/Activity/Controllers/FavouriteController.php
+++ b/app/Activity/Controllers/FavouriteController.php
@ -2,9 +2,6 @@

 namespace BookStack\Activity\Controllers;

-use BookStack\Activity\Models\Favouritable;
-use BookStack\App\Model;
-use BookStack\Entities\Models\Entity;
 use BookStack\Entities\Queries\TopFavourites;
 use BookStack\Entities\Tools\MixedEntityRequestHelper;
 use BookStack\Http\Controller;
@ -52,7 +49,7 @@ class FavouriteController extends Controller
            'name' => $entity->name,
        ]));

-        return redirect()->back();
+        return redirect($entity->getUrl());
    }

    /**
@ -70,6 +67,6 @@ class FavouriteController extends Controller
            'name' => $entity->name,
        ]));

-        return redirect()->back();
+        return redirect($entity->getUrl());
    }
 }
--- a/app/Activity/Controllers/WatchController.php
+++ b/app/Activity/Controllers/WatchController.php
@ -24,6 +24,6 @@ class WatchController extends Controller

        $this->showSuccessNotification(trans('activities.watch_update_level_notification'));

-        return redirect()->back();
+        return redirect($watchable->getUrl());
    }
 }
--- a/app/Entities/Controllers/ChapterController.php
+++ b/app/Entities/Controllers/ChapterController.php
@ -12,6 +12,7 @@ use BookStack\Entities\Tools\HierarchyTransformer;
 use BookStack\Entities\Tools\NextPreviousContentLocator;
 use BookStack\Exceptions\MoveOperationException;
 use BookStack\Exceptions\NotFoundException;
+use BookStack\Exceptions\NotifyException;
 use BookStack\Exceptions\PermissionsException;
 use BookStack\Http\Controller;
 use BookStack\References\ReferenceFetcher;
@ -170,7 +171,7 @@ class ChapterController extends Controller
    /**
     * Perform the move action for a chapter.
     *
-     * @throws NotFoundException
+     * @throws NotFoundException|NotifyException
     */
    public function move(Request $request, string $bookSlug, string $chapterSlug)
    {
@ -184,13 +185,13 @@ class ChapterController extends Controller
        }

        try {
-            $newBook = $this->chapterRepo->move($chapter, $entitySelection);
+            $this->chapterRepo->move($chapter, $entitySelection);
        } catch (PermissionsException $exception) {
            $this->showPermissionError();
        } catch (MoveOperationException $exception) {
            $this->showErrorNotification(trans('errors.selected_book_not_found'));

-            return redirect()->back();
+            return redirect($chapter->getUrl('/move'));
        }

        return redirect($chapter->getUrl());
@ -231,7 +232,7 @@ class ChapterController extends Controller
        if (is_null($newParentBook)) {
            $this->showErrorNotification(trans('errors.selected_book_not_found'));

-            return redirect()->back();
+            return redirect($chapter->getUrl('/copy'));
        }

        $this->checkOwnablePermission('chapter-create', $newParentBook);
--- a/app/Entities/Controllers/PageController.php
+++ b/app/Entities/Controllers/PageController.php
@ -391,7 +391,7 @@ class PageController extends Controller
        } catch (Exception $exception) {
            $this->showErrorNotification(trans('errors.selected_book_chapter_not_found'));

-            return redirect()->back();
+            return redirect($page->getUrl('/move'));
        }

        return redirect($page->getUrl());
@ -431,7 +431,7 @@ class PageController extends Controller
        if (is_null($newParent)) {
            $this->showErrorNotification(trans('errors.selected_book_chapter_not_found'));

-            return redirect()->back();
+            return redirect($page->getUrl('/copy'));
        }

        $this->checkOwnablePermission('page-create', $newParent);
--- a/app/Http/Controller.php
+++ b/app/Http/Controller.php
@ -9,6 +9,8 @@ use BookStack\Facades\Activity;
 use Illuminate\Foundation\Bus\DispatchesJobs;
 use Illuminate\Foundation\Validation\ValidatesRequests;
 use Illuminate\Http\JsonResponse;
+use Illuminate\Http\RedirectResponse;
+use Illuminate\Http\Request;
 use Illuminate\Routing\Controller as BaseController;

 abstract class Controller extends BaseController
@ -165,4 +167,20 @@ abstract class Controller extends BaseController
    {
        return ['image_extension', 'mimes:jpeg,png,gif,webp', 'max:' . (config('app.upload_limit') * 1000)];
    }
+
+    /**
+     * Redirect to the URL provided in the request as a '_return' parameter.
+     * Will check that the parameter leads to a URL under the root path of the system.
+     */
+    protected function redirectToRequest(Request $request): RedirectResponse
+    {
+        $basePath = url('/');
+        $returnUrl = $request->input('_return') ?? $basePath;
+
+        if (!str_starts_with($returnUrl, $basePath)) {
+            return redirect($basePath);
+        }
+
+        return redirect($returnUrl);
+    }
 }
--- a/app/Users/Controllers/RoleController.php
+++ b/app/Users/Controllers/RoleController.php
@ -154,7 +154,7 @@ class RoleController extends Controller
        } catch (PermissionsException $e) {
            $this->showErrorNotification($e->getMessage());

-            return redirect()->back();
+            return redirect("/settings/roles/delete/{$id}");
        }

        return redirect('/settings/roles');
--- a/app/Users/Controllers/UserPreferencesController.php
+++ b/app/Users/Controllers/UserPreferencesController.php
@ -3,9 +3,6 @@
 namespace BookStack\Users\Controllers;

 use BookStack\Http\Controller;
-use BookStack\Permissions\PermissionApplicator;
-use BookStack\Settings\UserNotificationPreferences;
-use BookStack\Settings\UserShortcutMap;
 use BookStack\Users\UserRepo;
 use Illuminate\Http\Request;

@ -23,7 +20,7 @@ class UserPreferencesController extends Controller
    {
        $valueViewTypes = ['books', 'bookshelves', 'bookshelf'];
        if (!in_array($type, $valueViewTypes)) {
-            return redirect()->back(500);
+            return $this->redirectToRequest($request);
        }

        $view = $request->get('view');
@ -34,7 +31,7 @@ class UserPreferencesController extends Controller
        $key = $type . '_view_type';
        setting()->putForCurrentUser($key, $view);

-        return redirect()->back(302, [], "/");
+        return $this->redirectToRequest($request);
    }

    /**
@ -44,7 +41,7 @@ class UserPreferencesController extends Controller
    {
        $validSortTypes = ['books', 'bookshelves', 'shelf_books', 'users', 'roles', 'webhooks', 'tags', 'page_revisions'];
        if (!in_array($type, $validSortTypes)) {
-            return redirect()->back(500);
+            return $this->redirectToRequest($request);
        }

        $sort = substr($request->get('sort') ?: 'name', 0, 50);
@ -55,18 +52,18 @@ class UserPreferencesController extends Controller
        setting()->putForCurrentUser($sortKey, $sort);
        setting()->putForCurrentUser($orderKey, $order);

-        return redirect()->back(302, [], "/");
+        return $this->redirectToRequest($request);
    }

    /**
     * Toggle dark mode for the current user.
     */
-    public function toggleDarkMode()
+    public function toggleDarkMode(Request $request)
    {
        $enabled = setting()->getForCurrentUser('dark-mode-enabled');
        setting()->putForCurrentUser('dark-mode-enabled', $enabled ? 'false' : 'true');

-        return redirect()->back();
+        return $this->redirectToRequest($request);
    }

    /**
--- a/resources/views/common/dark-mode-toggle.blade.php
+++ b/resources/views/common/dark-mode-toggle.blade.php
@ -1,6 +1,7 @@
 <form action="{{ url('/preferences/toggle-dark-mode') }}" method="post">
    {{ csrf_field() }}
    {{ method_field('patch') }}
+    <input type="hidden" name="_return" value="{{ url()->current() }}">
    @if(setting()->getForCurrentUser('dark-mode-enabled'))
        <button class="{{ $classes ?? '' }}"><span>@icon('light-mode')</span><span>{{ trans('common.light_mode') }}</span></button>
    @else
--- a/resources/views/common/sort.blade.php
+++ b/resources/views/common/sort.blade.php
@ -13,6 +13,7 @@
              method="post"
          @endif
    >
+        <input type="hidden" name="_return" value="{{ url()->current() }}">

        @if($useQuery ?? false)
            @foreach(array_filter(request()->except(['sort', 'order'])) as $key => $value)
--- a/resources/views/entities/view-toggle.blade.php
+++ b/resources/views/entities/view-toggle.blade.php
@ -1,7 +1,8 @@
 <div>
    <form action="{{ url("/preferences/change-view/" . $type) }}" method="POST" class="inline">
-        {!! csrf_field() !!}
-        {!! method_field('PATCH') !!}
+        {{ csrf_field() }}
+        {{ method_field('patch') }}
+        <input type="hidden" name="_return" value="{{ url()->current() }}">

        @if ($view === 'list')
            <button type="submit" name="view" value="grid" class="icon-list-item text-link">
@ -10,7 +11,7 @@
            </button>
        @else
            <button type="submit" name="view" value="list" class="icon-list-item text-link">
-                <span>@icon('list')</span>
+                <span class="icon">@icon('list')</span>
                <span>{{ trans('common.list_view') }}</span>
            </button>
        @endif
--- a/tests/Activity/WatchTest.php
+++ b/tests/Activity/WatchTest.php
@ -63,8 +63,7 @@ class WatchTest extends TestCase
        $editor = $this->users->editor();
        $book = $this->entities->book();

-        $this->actingAs($editor)->get($book->getUrl());
-        $resp = $this->put('/watching/update', [
+        $resp = $this->actingAs($editor)->put('/watching/update', [
            'type' => $book->getMorphClass(),
            'id' => $book->id,
            'level' => 'comments'
--- a/tests/FavouriteTest.php
+++ b/tests/FavouriteTest.php
@ -56,6 +56,23 @@ class FavouriteTest extends TestCase
        ]);
    }

+    public function test_add_and_remove_redirect_to_entity_without_history()
+    {
+        $page = $this->entities->page();
+
+        $resp = $this->asEditor()->post('/favourites/add', [
+            'type' => $page->getMorphClass(),
+            'id'   => $page->id,
+        ]);
+        $resp->assertRedirect($page->getUrl());
+
+        $resp = $this->asEditor()->post('/favourites/remove', [
+            'type' => $page->getMorphClass(),
+            'id'   => $page->id,
+        ]);
+        $resp->assertRedirect($page->getUrl());
+    }
+
    public function test_favourite_flow_with_own_permissions()
    {
        $book = $this->entities->book();
--- a/tests/User/RoleManagementTest.php
+++ b/tests/User/RoleManagementTest.php
@ -235,7 +235,7 @@ class RoleManagementTest extends TestCase
        /** @var Role $publicRole */
        $publicRole = Role::getSystemRole('public');
        $resp = $this->asAdmin()->delete('/settings/roles/delete/' . $publicRole->id);
-        $resp->assertRedirect('/');
+        $resp->assertRedirect('/settings/roles/delete/' . $publicRole->id);

        $this->get('/settings/roles/delete/' . $publicRole->id);
        $resp = $this->delete('/settings/roles/delete/' . $publicRole->id);
--- a/tests/User/UserPreferencesTest.php
+++ b/tests/User/UserPreferencesTest.php
@ -2,8 +2,6 @@

 namespace Tests\User;

-use BookStack\Activity\Tools\UserEntityWatchOptions;
-use BookStack\Activity\WatchLevels;
 use Tests\TestCase;

 class UserPreferencesTest extends TestCase
@ -40,7 +38,7 @@ class UserPreferencesTest extends TestCase
            'sort'  => 'name',
            'order' => 'asc',
        ]);
-        $updateRequest->assertStatus(500);
+        $updateRequest->assertRedirect();

        $this->assertNotEmpty('name', setting()->getForCurrentUser('bookshelves_sort'));
        $this->assertNotEmpty('asc', setting()->getForCurrentUser('bookshelves_sort_order'));
@ -141,7 +139,10 @@ class UserPreferencesTest extends TestCase
            ->assertElementNotExists('.featured-image-container')
            ->assertElementExists('.content-wrap .entity-list-item');

-        $req = $this->patch("/preferences/change-view/bookshelf", ['view' => 'grid']);
+        $req = $this->patch("/preferences/change-view/bookshelf", [
+            'view' => 'grid',
+            '_return' => $shelf->getUrl(),
+        ]);
        $req->assertRedirect($shelf->getUrl());

        $resp = $this->actingAs($editor)->get($shelf->getUrl())