Make URL building more resilient and add tests. (#38)

Since the base URL and base path are user inputs, I'd like tkr to be resilient to any combination of leading and trailing slashes so people don't have to worry about that. This adds some helper functions to normalize URLs and adds tests to confirm that all combinations are handled correctly.

Reviewed-on: https://gitea.subcultureofone.org/greg/tkr/pulls/38
Co-authored-by: Greg Sarjeant <greg@subcultureofone.org>
Co-committed-by: Greg Sarjeant <greg@subcultureofone.org>

public/index.php

@@ -77,7 +77,7 @@ if ($method === 'POST' && $path != 'setup') {
         if (!Session::isValid($_POST['csrf_token'])) {
             // Invalid session - redirect to /login
             Log::info('Attempt to POST with invalid session. Redirecting to login.');
-            header('Location: ' . $config->basePath . '/login');
+            header('Location: ' . Util::buildRelativeUrl($config->basePath, 'login'));
             exit;
         }
     } else {

src/Controller/AdminController/AdminController.php

@@ -30,7 +30,7 @@ class AdminController extends Controller {
 
     public function handleSave(){
         if (!Session::isLoggedIn()){
-            header('Location: ' . $config->basePath . '/login');
+            header('Location: ' . Util::buildRelativeUrl($config->basePath, 'login'));
             exit;
         }
 

src/Controller/AuthController/AuthController.php

@@ -30,7 +30,7 @@ class AuthController extends Controller {
                 Log::info("Successful login for {$username}");
 
                 Session::newLoginSession($user);
-                header('Location: ' . $config->basePath);
+                header('Location: ' . Util::buildRelativeUrl($config->basePath));
                 exit;
             } else {
                 Log::warning("Failed login for {$username}");
@@ -48,7 +48,7 @@ class AuthController extends Controller {
         Session::end();
 
         global $config;
-        header('Location: ' . $config->basePath);
+        header('Location: ' . Util::buildRelativeUrl($config->basePath));
         exit;
     }
 }

src/Controller/EmojiController/EmojiController.php

@@ -29,7 +29,7 @@
                 break;
             }
 
-            header('Location: ' . $config->basePath . 'admin/emoji');
+            header('Location: ' . Util::buildRelativeUrl($config->basePath, 'admin/emoji'));
             exit;
         }
 

src/Controller/FeedController/FeedController.php

@@ -6,7 +6,7 @@ class FeedController extends Controller {
     public function __construct(){
         $this->config = ConfigModel::load();
         $tickModel = new TickModel();
-        $this->ticks = iterator_to_array($tickModel->stream($this->config->itemsPerPage));
+        $this->ticks = $tickModel->getPage($this->config->itemsPerPage);
 
         Log::debug("Loaded " . count($this->ticks) . " ticks for feeds");
     }

src/Controller/HomeController/HomeController.php

@@ -10,10 +10,10 @@ class HomeController extends Controller {
         $tickModel = new TickModel();
         $limit = $config->itemsPerPage;
         $offset = ($page - 1) * $limit;
-        $ticks = iterator_to_array($tickModel->stream($limit, $offset));
+        $ticks = $tickModel->getPage($limit, $offset);
 
-        $view = new HomeView();
+        $view = new TicksView($config, $ticks, $page);
-        $tickList = $view->renderTicksSection($config->siteDescription, $ticks, $page, $limit);
+        $tickList = $view->getHtml();
 
         $vars = [
             'config'     => $config,
@@ -39,7 +39,7 @@ class HomeController extends Controller {
         global $config;
 
         // redirect to the index (will show the latest tick if one was sent)
-        header('Location: ' . $config->basePath);
+        header('Location: ' . Util::buildRelativeUrl($config->basePath));
         exit;
     }
 

src/Controller/MoodController/MoodController.php

@@ -35,7 +35,7 @@
                 $user = $user->save();
 
                 // go back to the index and show the updated mood
-                header('Location: ' . $config->basePath);
+                header('Location: ' . Util::buildRelativeUrl($config->basePath));
                 exit;
             }
         }

src/Framework/Util/Util.php

@@ -79,4 +79,28 @@ class Util {
         
         return $baseUrl . $basePath . $path;
     }
+
+    public static function buildRelativeUrl(string $basePath, string $path = ''): string {
+        // Ensure basePath starts with / for relative URLs
+        $basePath = '/' . ltrim($basePath, '/');
+        
+        // Remove trailing slash unless it's just '/'
+        if ($basePath !== '/') {
+            $basePath = rtrim($basePath, '/');
+        }
+        
+        // Add path
+        $path = ltrim($path, '/');
+        
+        if ($path === '') {
+            return $basePath;
+        }
+        
+        // If basePath is root, don't add extra slash
+        if ($basePath === '/') {
+            return '/' . $path;
+        }
+        
+        return $basePath . '/' . $path;
+    }
 }

src/Model/TickModel/TickModel.php

@@ -1,18 +1,12 @@
 <?php
 class TickModel {
-    public function stream(int $limit, int $offset = 0): Generator {
+    public function getPage(int $limit, int $offset = 0): array {
         global $db;
 
         $stmt = $db->prepare("SELECT id, timestamp, tick FROM tick ORDER BY timestamp DESC LIMIT ? OFFSET ?");
         $stmt->execute([$limit, $offset]);
 
-        while ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
+        return $stmt->fetchAll(PDO::FETCH_ASSOC);
-            yield [
-                'id' => $row['id'],
-                'timestamp' => $row['timestamp'],
-                'tick' => $row['tick'],
-            ];
-        }
     }
 
     public function insert(string $tick, ?DateTimeImmutable $datetime = null): void {

src/View/TicksView/TicksView.php

@@ -1,7 +1,16 @@
 <?php
-class HomeView {
+class TicksView {
-    public function renderTicksSection(string $siteDescription, array $ticks, int $page, int $limit){
+    private $html;
-        global $config;
+
+    public function __construct(ConfigModel $config, array $ticks, int $page){
+        $this->html = $this->render($config, $ticks, $page);
+    }
+
+    public function getHtml(): string {
+        return $this->html;
+    }
+
+    private function render(ConfigModel $config, array $ticks, int $page): string{
         ob_start();
         ?>
 
@@ -22,7 +31,7 @@ class HomeView {
                 <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
                    href="?page=<?php echo $page - 1 ?>">&laquo; Newer</a>
             <?php endif; ?>
-            <?php if (count($ticks) === $limit): ?>
+            <?php if (count($ticks) === $config->itemsPerPage): ?>
                 <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
                    href="?page=<?php echo $page + 1 ?>">Older &raquo;</a>
             <?php endif; ?>

templates/main.php

@@ -10,10 +10,10 @@
         <meta charset="UTF-8">
         <meta name="viewport" content="width=device-width, initial-scale=1.0">
         <link rel="stylesheet"
-              href="<?= Util::escape_html($config->basePath) ?>css/default.css">
+              href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'css/default.css')) ?>">
 <?php if (!empty($config->cssId)): ?>
         <link rel="stylesheet"
-              href="<?= Util::escape_html($config->basePath) ?>css/custom/<?= Util::escape_html($config->customCssFilename()) ?>">
+              href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'css/custom/' . $config->customCssFilename())) ?>">
 <?php endif; ?>
         <link rel="alternate"
               type="application/rss+xml"

templates/partials/admin.php

@@ -4,7 +4,7 @@
         <h1><?php if ($isSetup): ?>Setup<?php else: ?>Admin<?php endif; ?></h1>
         <main>
             <form
-                action="<?php echo $config->basePath . ($isSetup ? 'setup' : 'admin') ?>"
+                action="<?php echo Util::buildRelativeUrl($config->basePath, ($isSetup ? 'setup' : 'admin')) ?>"
                 method="post">
                 <input type="hidden" name="csrf_token" value="<?= Util::escape_html($_SESSION['csrf_token']) ?>">
                 <fieldset>

templates/partials/css.php

@@ -2,7 +2,7 @@
 <?php /** @var Array $customCss */ ?>
         <h1>CSS Management</h1>
         <main>
-            <form action="<?= $config->basePath ?>admin/css" method="post" enctype="multipart/form-data">
+            <form action="<?= Util::buildRelativeUrl($config->basePath, 'admin/css') ?>" method="post" enctype="multipart/form-data">
                 <input type="hidden" name="csrf_token" value="<?= Util::escape_html($_SESSION['csrf_token']) ?>">
                 <fieldset>
                     <legend>Manage</legend>

templates/partials/emoji.php

@@ -2,7 +2,7 @@
 <?php /** @var array $emojiList */ ?>
         <h1>Emoji Management</h1>
         <main>
-            <form action="<?= $config->basePath ?>admin/emoji" method="post" enctype="multipart/form-data">
+            <form action="<?= Util::buildRelativeUrl($config->basePath, 'admin/emoji') ?>" method="post" enctype="multipart/form-data">
                 <input type="hidden" name="csrf_token" value="<?= Util::escape_html($_SESSION['csrf_token']) ?>">
                 <fieldset>
                     <legend>Add Emoji</legend>
@@ -24,7 +24,7 @@
                 </fieldset>
             </form>
 <?php if (!empty($emojiList)): ?>
-            <form action="<?= $config->basePath ?>admin/emoji" method="post" enctype="multipart/form-data">
+            <form action="<?= Util::buildRelativeUrl($config->basePath, 'admin/emoji') ?>" method="post" enctype="multipart/form-data">
                 <input type="hidden" name="csrf_token" value="<?= Util::escape_html($_SESSION['csrf_token']) ?>">
                 <fieldset class="delete-emoji-fieldset">
                     <legend>Delete Emoji</legend>

templates/partials/home.php

@@ -14,7 +14,7 @@
 <?php if (Session::isLoggedIn()): ?>
                         <a
                             <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-                            href="<?= Util::escape_html($config->basePath) ?>mood"
+                            href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'mood')) ?>"
                             class="change-mood">Change mood</a>
 <?php endif ?>
                     </dd>

templates/partials/login.php

@@ -2,7 +2,7 @@
 <?php /** @var string $csrf_token */ ?>
 <?php /** @var string $error */ ?>
     <h2>Login</h2>
-    <form method="post" action="<?= $config->basePath ?>login">
+    <form method="post" action="<?= Util::buildRelativeUrl($config->basePath, 'login') ?>">
         <div class="fieldset-items">
             <input type="hidden" name="csrf_token" value="<?= Util::escape_html($csrf_token) ?>">
             <label for="username">Username:</label>

templates/partials/navbar.php

@@ -2,32 +2,32 @@
 <?php /* https://www.w3schools.com/howto/howto_css_dropdown.asp */ ?>
         <nav aria-label="Main navigation">
             <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-               href="<?= Util::escape_html($config->basePath) ?>">home</a>
+               href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath)) ?>">home</a>
             <details>
                 <summary aria-haspopup="true">feeds</summary>
                 <div class="dropdown-items">
                     <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-                       href="<?= Util::escape_html($config->basePath) ?>feed/rss">rss</a>
+                       href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'feed/rss')) ?>">rss</a>
                     <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-                       href="<?= Util::escape_html($config->basePath) ?>feed/atom">atom</a>
+                       href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'feed/atom')) ?>">atom</a>
                 </div>
             </details>
 <?php if (!Session::isLoggedIn()): ?>
             <a tabindex="0"
-               href="<?= Util::escape_html($config->basePath) ?>login">login</a>
+               href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'login')) ?>">login</a>
 <?php else: ?>
             <details>
                 <summary aria-haspopup="true">admin</summary>
                 <div class="dropdown-items">
                     <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-                       href="<?= Util::escape_html($config->basePath) ?>admin">settings</a>
+                       href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'admin')) ?>">settings</a>
                     <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-                       href="<?= Util::escape_html($config->basePath) ?>admin/css">css</a>
+                       href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'admin/css')) ?>">css</a>
                     <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-                       href="<?= Util::escape_html($config->basePath) ?>admin/emoji">emoji</a>
+                       href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'admin/emoji')) ?>">emoji</a>
                 </div>
             </details>
             <a <?php if($config->strictAccessibility): ?>tabindex="0"<?php endif; ?>
-               href="<?= Util::escape_html($config->basePath) ?>logout">logout</a>
+               href="<?= Util::escape_html(Util::buildRelativeUrl($config->basePath, 'logout')) ?>">logout</a>
 <?php endif; ?>
         </nav>

tests/Framework/Util/UtilTest.php

@@ -26,4 +26,51 @@ final class UtilTest extends TestCase
         $this->assertSame($relativeTime, $display);
     }
 
+    public static function buildUrlProvider(): array {
+        return [
+            'basic path' => ['https://example.com', 'tkr', 'admin', 'https://example.com/tkr/admin'],
+            'baseUrl with trailing slash' => ['https://example.com/', 'tkr', 'admin', 'https://example.com/tkr/admin'],
+            'empty basePath' => ['https://example.com', '', 'admin', 'https://example.com/admin'],
+            'root basePath' => ['https://example.com', '/', 'admin', 'https://example.com/admin'],
+            'basePath no leading slash' => ['https://example.com', 'tkr', 'admin', 'https://example.com/tkr/admin'],
+            'basePath with leading slash' => ['https://example.com', '/tkr', 'admin', 'https://example.com/tkr/admin'],
+            'basePath with trailing slash' => ['https://example.com', 'tkr/', 'admin', 'https://example.com/tkr/admin'],
+            'basePath with both slashes' => ['https://example.com', '/tkr/', 'admin', 'https://example.com/tkr/admin'],
+            'complex path' => ['https://example.com', 'tkr', 'admin/css/upload', 'https://example.com/tkr/admin/css/upload'],
+            'path with leading slash' => ['https://example.com', 'tkr', '/admin', 'https://example.com/tkr/admin'],
+            'no path - empty basePath' => ['https://example.com', '', '', 'https://example.com/'],
+            'no path - root basePath' => ['https://example.com', '/', '', 'https://example.com/'],
+            'no path - tkr basePath' => ['https://example.com', 'tkr', '', 'https://example.com/tkr/'],
+        ];
+    }
+
+    #[DataProvider('buildUrlProvider')]
+    public function testBuildUrl(string $baseUrl, string $basePath, string $path, string $expected): void {
+        $result = Util::buildUrl($baseUrl, $basePath, $path);
+        $this->assertEquals($expected, $result);
+    }
+
+    public static function buildRelativeUrlProvider(): array {
+        return [
+            'empty basePath with path' => ['', 'admin', '/admin'],
+            'root basePath with path' => ['/', 'admin', '/admin'],
+            'tkr basePath with path' => ['tkr', 'admin', '/tkr/admin'],
+            'tkr with leading slash' => ['/tkr', 'admin', '/tkr/admin'],
+            'tkr with trailing slash' => ['tkr/', 'admin', '/tkr/admin'],
+            'tkr with both slashes' => ['/tkr/', 'admin', '/tkr/admin'],
+            'complex path' => ['tkr', 'admin/css/upload', '/tkr/admin/css/upload'],
+            'path with leading slash' => ['tkr', '/admin', '/tkr/admin'],
+            'no path - empty basePath' => ['', '', '/'],
+            'no path - root basePath' => ['/', '', '/'],
+            'no path - tkr basePath' => ['tkr', '', '/tkr'],
+            'no path - tkr with slashes' => ['/tkr/', '', '/tkr'],
+        ];
+    }
+
+    #[DataProvider('buildRelativeUrlProvider')]
+    public function testBuildRelativeUrl(string $basePath, string $path, string $expected): void {
+        $result = Util::buildRelativeUrl($basePath, $path);
+        $this->assertEquals($expected, $result);
+    }
+
 }