diff --git a/ISSUES.md b/ISSUES.md new file mode 100644 index 0000000..a4d3679 --- /dev/null +++ b/ISSUES.md @@ -0,0 +1,318 @@ +# MokoSuiteOpenGraph — Code Assessment Issues + +Generated: 2026-06-06 +Updated: 2026-06-21 +Reviewed: Full codebase (all PHP, SQL, XML, JS, CSS, templates) + +--- + +## Status Legend + +- FIXED — Verified resolved in codebase +- OPEN — Still present, needs work +- WONTFIX — Intentional or acceptable as-is + +--- + +## Bugs + +### BUG-01: Batch generation offset pagination skips articles — FIXED + +**Severity:** High +**File:** `source/packages/com_mokoog/src/Controller/BatchController.php:89` + +The `process()` method now correctly uses `$db->setQuery($query, 0, $limit)` with a comment explaining that processed articles are automatically excluded by the LEFT JOIN filter. + +--- + +### BUG-02: License key session flag set before check completes — FIXED + +**Severity:** Medium +**File:** `source/packages/plg_system_mokoog/src/Extension/MokoOG.php:543` + +Session flag is now set after the DB query succeeds, inside the try block but after query setup. If the query throws, the catch block runs without the flag being set. + +--- + +### BUG-03: Hardcoded og:image dimensions are often wrong — FIXED + +**Severity:** Medium +**File:** `source/packages/plg_system_mokoog/src/Extension/MokoOG.php:129-134` + +Now uses `$this->getImageDimensions($image)` which calls `getimagesize()` to detect actual dimensions. Dimension meta tags only emitted when dimensions are successfully detected. + +--- + +### BUG-04: `strlen()` vs `mb_strlen()` inconsistency in truncation — FIXED + +**Severity:** Low +**Files:** MokoOG.php, BatchController.php, HikaShopAdapter.php, K2Adapter.php + +All instances now consistently use `mb_strlen()` for length checks with `mb_substr()` for truncation. + +--- + +### BUG-05: `ImageGenerator::wrapText()` can produce broken output — FIXED + +**Severity:** Low +**File:** `source/packages/plg_system_mokoog/src/Helper/ImageGenerator.php:156` + +Now checks `mb_strlen($lines[2]) > 3` before truncating. Short lines get `'...'` appended instead. + +--- + +## Potential Issues + +### ISSUE-01: ContentType adapters exist but are never wired up — OPEN + +**Severity:** High (wasted code) +**Files:** +- `source/packages/com_mokoog/src/ContentType/ContentTypeInterface.php` +- `source/packages/com_mokoog/src/ContentType/HikaShopAdapter.php` +- `source/packages/com_mokoog/src/ContentType/K2Adapter.php` +- `source/packages/com_mokoog/src/ContentType/VirtueMartAdapter.php` + +The system plugin (`MokoOG.php`) still never references or loads these adapters. The `findImage()` and `loadOgData()` methods only handle `com_content`. Third-party content types get no auto-generated OG tags. + +**Action:** Wire adapters into the system plugin's `onBeforeCompileHead` flow, or remove them if not planned for v1. + +--- + +### ISSUE-02: `applySeoTags()` accesses internal `$doc->_links` property — OPEN + +**Severity:** Medium +**File:** `source/packages/plg_system_mokoog/src/Extension/MokoOG.php:257-259` + +Still directly accessing `$doc->_links` (protected/internal property). Fragile across Joomla versions. + +**Fix:** Use `$doc->getHeadData()` to read links and `$doc->addHeadLink()` with proper clearing logic. + +--- + +### ISSUE-03: No input sanitization on OG values before output — OPEN + +**Severity:** Medium +**File:** `source/packages/plg_content_mokoog/src/Extension/MokoOGContent.php` + +No `htmlspecialchars()` or `InputFilter` found in the content plugin's save path. While Joomla's `setMetaData()` escapes on output, defense-in-depth recommends sanitizing on input. + +**Fix:** Apply `htmlspecialchars()` or Joomla's `InputFilter` when saving OG data. + +--- + +### ISSUE-04: `loadOgDataByType()` and `loadOgDataByMenu()` ignore language — OPEN + +**Severity:** Medium +**Files:** +- `source/packages/plg_system_mokoog/src/Extension/MokoOG.php:324-337` (`loadOgDataByType`) +- `source/packages/plg_system_mokoog/src/Extension/MokoOG.php:346-359` (`loadOgDataByMenu`) + +These methods still have no language filter. On multilingual sites, category fallback or menu OG data could come from any language. The unique key is now `(content_type, content_id, language)` but these queries don't filter by language, so `loadObject()` returns an arbitrary match. + +**Fix:** Add the same language filter pattern used in `loadOgData()`. + +--- + +### ISSUE-05: VirtueMart adapter interpolates language into table name — OPEN (low risk) + +**Severity:** Low (defense-in-depth) +**File:** `source/packages/com_mokoog/src/ContentType/VirtueMartAdapter.php:34,47` + +Language tag is interpolated into the table name. While `quoteName()` wraps the result, the language tag itself is not validated against an allowlist. + +**Fix:** Validate tag format with a regex before interpolation. + +--- + +### ISSUE-06: No admin list controller for publish/delete operations — OPEN + +**Severity:** Medium +**File:** `source/packages/com_mokoog/src/Controller/` + +No `TagsController extends AdminController` exists. The admin list view toolbar buttons for delete/publish/unpublish will produce task routing errors. + +**Fix:** Add a `TagsController extends AdminController` with proper CSRF and ACL checks. + +--- + +### ISSUE-07: CSV import/export does not handle `language` column — OPEN + +**Severity:** Low +**File:** `source/packages/com_mokoog/src/Controller/ImportExportController.php` + +No reference to `language` found in the controller. Export omits the column, import creates records with default `*` language. Multilingual sites cannot bulk import/export language-specific OG data. + +**Fix:** Add `language` as a column in export, and parse it on import with a fallback to `*`. + +--- + +### ISSUE-08: No ACL check in content plugin form injection — WONTFIX + +**Severity:** Low +**File:** `source/packages/plg_content_mokoog/src/Extension/MokoOGContent.php:49` + +Any user who can edit an article can modify OG tags. This is acceptable behavior for most sites — if you can edit the article, you should be able to control its social sharing appearance. + +--- + +## New Issues (Found 2026-06-21) + +### ISSUE-09: ImageGenerator uses @ error suppression on GD functions + +**Severity:** Medium +**File:** `source/packages/plg_system_mokoog/src/Helper/ImageGenerator.php` + +All GD library calls use the `@` suppression operator, making debugging difficult. If the GD extension is missing or a font file is not found, failures are completely silent. + +**Fix:** Replace `@` suppression with proper error checking and logging via `Log::add()`. + +--- + +### ISSUE-10: No TTF font file bundled or documented + +**Severity:** Medium +**File:** `source/packages/plg_system_mokoog/src/Helper/ImageGenerator.php` + +The image generator requires a TTF font file for text overlay, but no font is included in the package and no fallback or documentation exists for configuring the font path. + +**Fix:** Bundle a permissively-licensed font (e.g., Open Sans, Noto Sans) or document the required configuration. + +--- + +### ISSUE-11: ImageGenerator cache grows unbounded + +**Severity:** Low +**File:** `source/packages/plg_system_mokoog/src/Helper/ImageGenerator.php` + +Generated images in `images/mokoog/generated/` are never cleaned up. On sites with many articles, this directory grows indefinitely. + +**Fix:** Add a cleanup CLI command or admin button (see FEAT-07), or implement LRU/TTL-based cache eviction. + +--- + +### ISSUE-12: JSON-LD missing common schema types + +**Severity:** Low +**File:** `source/packages/plg_system_mokoog/src/Helper/JsonLdBuilder.php` + +Only 4 schema types are implemented (Article, WebPage, BreadcrumbList, Organization). Missing: NewsArticle, BlogPosting, Product, VideoObject, Event — some of which correspond to existing `og_type` dropdown values. + +**Fix:** Add at least NewsArticle and BlogPosting as Article subtypes. + +--- + +### ISSUE-13: No API input validation beyond field whitelisting + +**Severity:** Low +**Files:** +- `source/packages/com_mokoog/api/src/Controller/TagsController.php` +- `source/packages/com_mokoog/api/src/View/Tags/JsonapiView.php` + +The REST API exposes full CRUD but has no validation for field content (e.g., max lengths, valid URLs for og_image/canonical_url, valid og_type values). + +**Fix:** Add validation rules matching the form XML constraints. + +--- + +## Feature Expansion Opportunities + +### FEAT-01: Wire up ContentType adapter system — NOT IMPLEMENTED + +Connect the existing `ContentTypeInterface` adapters to the system plugin so HikaShop products, K2 items, and VirtueMart products automatically get OG tags. Blocked by ISSUE-01. + +--- + +### FEAT-02: Admin edit view for individual OG tag records — NOT IMPLEMENTED + +A `TagModel` and `tag.xml` form exist but there's no edit template (`tmpl/tag/`) or `TagController`. Users can only manage OG tags through article/menu editors. + +--- + +### FEAT-03: Publish/unpublish toggle in admin list — NOT IMPLEMENTED + +Blocked by ISSUE-06 (no TagsController). The list view shows published status as text but has no clickable toggle. + +--- + +### FEAT-04: Actual image dimension detection for og:image meta — FIXED + +Implemented via `getImageDimensions()` method using `getimagesize()`. See BUG-03. + +--- + +### FEAT-05: Duplicate OG tag detection — NOT IMPLEMENTED + +No detection for conflicting OG meta tags from other extensions. + +--- + +### FEAT-06: Support og:video and og:audio URLs — NOT IMPLEMENTED + +No `og_video` or `og_audio` columns, form fields, or rendering logic found anywhere in the codebase. + +--- + +### FEAT-07: Generated image cache cleanup — NOT IMPLEMENTED + +No CLI command or admin purge button. See ISSUE-11. + +--- + +### FEAT-08: Sitemap integration — NOT IMPLEMENTED + +No sitemap generation or integration exists. + +--- + +### FEAT-09: Social share preview in admin list — NOT IMPLEMENTED + +No thumbnails or inline validation in the admin list view. Live preview only exists in the article/menu editor (via plg_content_mokoog). + +--- + +### FEAT-10: Bulk OG tag editing — NOT IMPLEMENTED + +No batch edit modal for selecting multiple items and changing common fields. + +--- + +## Security Fixes (from CHANGELOG [Unreleased]) + +All 4 claimed security fixes have been **verified as implemented**: + +| Fix | Status | Evidence | +|-----|--------|----------| +| JSON-LD XSS (#34) | IMPLEMENTED | `_links` access (Joomla version fragility) +- ISSUE-03: Input sanitization on save (defense-in-depth) +- ISSUE-09: GD error suppression (debuggability) +- ISSUE-10: Bundle or document TTF font requirement + +**Nice to have for v1.0.0:** +- FEAT-02: Admin edit view +- FEAT-03: Publish/unpublish toggle +- ISSUE-07: Language column in CSV import/export diff --git a/source/packages/com_mokoog/src/Controller/TagsController.php b/source/packages/com_mokoog/src/Controller/TagsController.php new file mode 100644 index 0000000..00d2000 --- /dev/null +++ b/source/packages/com_mokoog/src/Controller/TagsController.php @@ -0,0 +1,33 @@ + + * @copyright Copyright (C) 2026 Moko Consulting. All rights reserved. + * @license GNU General Public License version 3 or later; see LICENSE + */ + +namespace Joomla\Component\MokoOG\Administrator\Controller; + +defined('_JEXEC') or die; + +use Joomla\CMS\MVC\Controller\AdminController; +use Joomla\CMS\MVC\Model\BaseDatabaseModel; + +class TagsController extends AdminController +{ + /** + * Proxy for getModel. + * + * @param string $name Model name + * @param string $prefix Model prefix + * @param array $config Configuration array + * + * @return BaseDatabaseModel + */ + public function getModel($name = 'Tag', $prefix = 'Administrator', $config = ['ignore_request' => true]) + { + return parent::getModel($name, $prefix, $config); + } +} diff --git a/source/packages/plg_system_mokoog/src/Extension/MokoOG.php b/source/packages/plg_system_mokoog/src/Extension/MokoOG.php index 5655dc8..d18c81d 100644 --- a/source/packages/plg_system_mokoog/src/Extension/MokoOG.php +++ b/source/packages/plg_system_mokoog/src/Extension/MokoOG.php @@ -1,7 +1,7 @@ * @copyright Copyright (C) 2026 Moko Consulting. All rights reserved. @@ -253,11 +253,17 @@ final class MokoOG extends CMSPlugin implements SubscriberInterface // Canonical URL if (!empty($ogData->canonical_url)) { - // Remove any existing canonical link first - foreach ($doc->_links as $link => $attribs) { - if (isset($attribs['relation']) && $attribs['relation'] === 'canonical') { - unset($doc->_links[$link]); + // Remove any existing canonical link via public API + $headData = $doc->getHeadData(); + + if (!empty($headData['links'])) { + foreach ($headData['links'] as $link => $attribs) { + if (isset($attribs['relation']) && $attribs['relation'] === 'canonical') { + unset($headData['links'][$link]); + } } + + $doc->setHeadData($headData); } $doc->addHeadLink($ogData->canonical_url, 'canonical'); @@ -323,15 +329,20 @@ final class MokoOG extends CMSPlugin implements SubscriberInterface */ private function loadOgDataByType(string $contentType, int $contentId): ?object { - $db = Factory::getDbo(); + $db = Factory::getDbo(); + $lang = Factory::getLanguage()->getTag(); + $query = $db->getQuery(true) ->select('*') ->from($db->quoteName('#__mokoog_tags')) ->where($db->quoteName('content_type') . ' = ' . $db->quote($contentType)) ->where($db->quoteName('content_id') . ' = ' . $contentId) - ->where($db->quoteName('published') . ' = 1'); + ->where($db->quoteName('published') . ' = 1') + ->where('(' . $db->quoteName('language') . ' = ' . $db->quote($lang) + . ' OR ' . $db->quoteName('language') . ' = ' . $db->quote('*') . ')') + ->order('CASE WHEN ' . $db->quoteName('language') . ' = ' . $db->quote('*') . ' THEN 1 ELSE 0 END ASC'); - $db->setQuery($query); + $db->setQuery($query, 0, 1); return $db->loadObject(); } @@ -345,15 +356,20 @@ final class MokoOG extends CMSPlugin implements SubscriberInterface */ private function loadOgDataByMenu(int $menuId): ?object { - $db = Factory::getDbo(); + $db = Factory::getDbo(); + $lang = Factory::getLanguage()->getTag(); + $query = $db->getQuery(true) ->select('*') ->from($db->quoteName('#__mokoog_tags')) ->where($db->quoteName('content_type') . ' = ' . $db->quote('menu')) ->where($db->quoteName('content_id') . ' = ' . $menuId) - ->where($db->quoteName('published') . ' = 1'); + ->where($db->quoteName('published') . ' = 1') + ->where('(' . $db->quoteName('language') . ' = ' . $db->quote($lang) + . ' OR ' . $db->quoteName('language') . ' = ' . $db->quote('*') . ')') + ->order('CASE WHEN ' . $db->quoteName('language') . ' = ' . $db->quote('*') . ' THEN 1 ELSE 0 END ASC'); - $db->setQuery($query); + $db->setQuery($query, 0, 1); return $db->loadObject(); } @@ -534,7 +550,7 @@ final class MokoOG extends CMSPlugin implements SubscriberInterface $query = $db->getQuery(true) ->select($db->quoteName('extra_query')) ->from($db->quoteName('#__update_sites')) - ->where($db->quoteName('name') . ' = ' . $db->quote('MokoJoomOpenGraph Updates')) + ->where($db->quoteName('name') . ' = ' . $db->quote('MokoSuiteOpenGraph Updates')) ->setLimit(1); $db->setQuery($query); $extraQuery = (string) $db->loadResult(); @@ -555,7 +571,7 @@ final class MokoOG extends CMSPlugin implements SubscriberInterface . 'No download key is configured. Updates will not be available until a valid license key is entered. ' . 'Go to System → Update Sites ' . 'and enter your license key (MOKO-XXXX-XXXX-XXXX-XXXX) in the Download Key field ' - . 'for the MokoJoomOpenGraph update site.', + . 'for the MokoSuiteOpenGraph update site.', 'warning' ); } catch (\Throwable $e) {