Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ThemeManager service (ZIP upload, validation, storage, activation), admin upload UI and API endpoint with CSRF/permission checks, propagates templateSet through TwigWrapper and tenant provisioning, registers the service, and adds unit tests and docs for theme workflows. Changes
Sequence DiagramsequenceDiagram
actor Admin as Admin User
participant UI as Admin UI
participant API as Config API
participant TM as ThemeManager
participant Storage as Storage
participant Config as Configuration
participant TW as TwigWrapper
Admin->>UI: submit theme name + ZIP
UI->>API: POST /api/configuration/themes/upload (FormData)
API->>API: verify permissions & CSRF
API->>TM: uploadTheme(themeName, archivePath)
TM->>TM: validate name, ZIP, require index.twig, enforce allowed files
TM->>Storage: write validated theme files
TM->>Config: persist layout.templateSet (activation)
API->>UI: return JSON success (activeTheme)
UI->>TW: reinitialize/load templates with new templateSet
TW->>Storage: load templates (falls back to default if missing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpmyfaq/src/phpMyFAQ/Instance/Client.php (1)
477-490:⚠️ Potential issue | 🟡 MinorInconsistent escaping:
clientUrlon line 482 is not escaped, butclientTemplateSeton line 489 is.Pre-existing issue, but now visible by contrast: Line 482 interpolates
$this->clientUrldirectly into SQL withoutescape(), while the new template-set query on line 489 correctly calls$this->configuration->getDb()->escape(...). Consider escapingclientUrlas well for consistency.Proposed fix
$this->configuration ->getDb() ->query(sprintf( "UPDATE %sfaqconfig SET config_value = '%s' WHERE config_name = 'main.referenceURL'", $prefix, - $this->clientUrl, + $this->configuration->getDb()->escape($this->clientUrl), ));
🤖 Fix all issues with AI agents
In `@docs/development.md`:
- Around line 499-541: The "9.2.3 Dynamic template set and theme upload" block
is in the wrong place and conflicts with duplicate numbering; move the entire
section (the header "9.2.3 Dynamic template set and theme upload" plus its
content) to follow the existing "9.2.2" content for Templating (the block around
the current "9.2.2 Admin backend templates"), then rename the existing duplicate
header "9.2.2 Admin backend templates" to "9.2.3" and rename the moved "9.2.3
Dynamic template set and theme upload" to "9.2.4", updating any internal
cross-references or numbering in the document accordingly so numbering is
sequential under section 9.2.
In `@phpmyfaq/assets/templates/admin/configuration/themes.upload.twig`:
- Line 7: Fix the grammatical error in the help text inside the
themes.upload.twig template: find the sentence that reads "ZIP must include
<code>index.twig</code>. Select the uploaded theme below using and save the
configuration." and replace it with a corrected sentence such as "ZIP must
include <code>index.twig</code>. Select the uploaded theme below and save the
configuration." ensuring the change is made in the template string rendered by
themes.upload.twig.
In `@phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php`:
- Around line 55-97: The loop in import logic currently writes each entry via
$this->storage->put() (see themeStoragePath, normalizeArchivePath,
assertAllowedFileExtension) before full validation, so failures (missing
index.twig or disallowed extension) leave orphaned files; change the flow to
first validate and collect entries (e.g., build an array of
[$normalizedEntryPath => $contents] while tracking containsIndexTemplate and
uploadedFiles) and only call $this->storage->put() after all validation passes,
or implement a rollback: track stored storage paths during the loop and on any
exception delete them via $this->storage->delete() (ensure cleanup runs in the
finally/catch surrounding zip processing) so partial uploads are removed.
In `@tests/phpMyFAQ/Template/ThemeManagerTest.php`:
- Around line 113-122: The test named testUploadThemeRejectsInvalidThemeName is
misleading because it calls ThemeManager::activateTheme('../invalid') instead of
testing ThemeManager::uploadTheme; either rename the test to
testActivateThemeRejectsInvalidThemeName to match the exercised code (update the
method name and any references), or add a new test that calls
$manager->uploadTheme(...) with an invalid name to specifically cover
uploadTheme validation while keeping the existing activateTheme test as-is.
🧹 Nitpick comments (7)
phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php (2)
93-95: Unreachable code: the$uploadedFiles === 0check can never be true.If
$containsIndexTemplateistrue(i.e., line 89 didn't throw), then at leastindex.twigwas stored, meaning$uploadedFiles >= 1. This block is dead code.
142-166: Path traversal check usingstr_contains($rebuiltPath, '..')may produce false positives.A legitimate filename like
something..else.twigwould be rejected. A more precise check would verify individual path segments:More precise path traversal check
- if (str_contains($rebuiltPath, '..')) { + if (in_array('..', $pathParts, true)) { throw new RuntimeException('Theme archive contains invalid relative paths.'); }This checks the
$pathPartsarray (already computed above) for a literal..segment rather than matching a substring anywhere in the path.phpmyfaq/src/phpMyFAQ/Instance/Client.php (1)
63-71:setClientTemplateSetdoes not validate the theme name, unlikeThemeManager::assertValidThemeName.
ThemeManagerenforces^[A-Za-z0-9_-]{2,64}$for theme names, butsetClientTemplateSetonly trims and defaults to'default'for empty strings. SinceclientTemplateSetis interpolated into SQL queries (even though it's escaped), applying the same validation here would add a defense-in-depth layer and keep the two code paths consistent.Add theme name validation
public function setClientTemplateSet(string $templateSet): void { $templateSet = trim($templateSet); if ($templateSet === '') { $templateSet = 'default'; } + if (!preg_match('/^[A-Za-z0-9_-]{2,64}$/', $templateSet)) { + throw new \InvalidArgumentException('Invalid template set name.'); + } + $this->clientTemplateSet = $templateSet; }tests/phpMyFAQ/Instance/ClientTest.php (1)
63-77: Consider asserting the actual template set value in the captured query.The test calls
setClientTemplateSet('tenant-theme')but only checks the query contains the config key name, not the value. This means the test would pass even if the wrong value was written.Stronger assertion
- $this->assertTrue($this->queryContains($queries, "WHERE config_name = 'layout.templateSet'")); + $this->assertTrue($this->queryContains($queries, "config_value = 'tenant-theme'")); + $this->assertTrue($this->queryContains($queries, "config_name = 'layout.templateSet'"));tests/phpMyFAQ/Template/ThemeManagerTest.php (1)
149-200:InMemoryStoragetest double is well implemented.Clean and minimal. Consider extracting it to a shared test utilities namespace if other test classes need a similar fake storage.
phpmyfaq/admin/assets/src/configuration/configuration.ts (1)
275-293: Potential duplicate listeners ifhandleThemesis called without DOM replacement.Currently this is safe because
handleConfigurationTab('#layout')(line 285) replaces the DOM beforehandleThemes()re-attaches the listener. However, the recursive self-call on line 287 is fragile—if the refresh flow changes to not replace the DOM, duplicate submit handlers will stack up. Consider either using{ once: true }on the listener or removing the old listener before attaching.♻️ Safer alternative: avoid recursive re-wiring
export const handleThemes = async (): Promise<void> => { const uploadForm = document.getElementById('theme-upload-form') as HTMLFormElement | null; if (uploadForm) { uploadForm.addEventListener('submit', async (event: Event): Promise<void> => { event.preventDefault(); const response = (await uploadThemeArchive(new FormData(uploadForm))) as unknown as Response; if (typeof response.success === 'string') { pushNotification(response.success); await handleConfigurationTab('#layout'); await handleTemplates(); + await handleThemes(); - await handleThemes(); } else { pushErrorNotification(response.error || 'Theme upload failed.'); } - }); + }, { once: true }); } };Using
{ once: true }ensures the listener auto-removes after firing, so the recursivehandleThemes()call safely re-attaches a fresh one-shot listener without risk of stacking.phpmyfaq/src/phpMyFAQ/Twig/TwigWrapper.php (1)
49-59: Constructor sets global static state as a side effect.Every
TwigWrapperinstantiation mutatesself::$templateSetName(line 56). This is likely intentional for the current architecture, but be aware that constructing aTwigWrapperin tests or transient contexts will change the template set globally for all subsequent code relying ongetTemplateSetName().
phpmyfaq/assets/templates/admin/configuration/themes.upload.twig
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php`:
- Around line 67-72: Skip ZIP directory entries before calling
normalizeArchivePath to avoid stripping trailing slashes and triggering
assertAllowedFileExtension on directory names: check the raw $entryName for a
trailing '/' (or empty) and continue early in the loop (before calling
normalizeArchivePath), so functions normalizeArchivePath and
assertAllowedFileExtension only receive actual file paths; update the loop
around normalizeArchivePath/assertAllowedFileExtension in ThemeManager to
perform this pre-normalization directory check.
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php (1)
147-171: Path traversal check runs after prefix stripping — ordering is correct but the..check is overly broad.The
str_contains($rebuiltPath, '..')check on line 166 will reject legitimate filenames containing..as a substring (e.g.,my..file.twig). While uncommon, a more precise check would inspect individual path segments:♻️ More precise traversal check
- if (str_contains($rebuiltPath, '..')) { - throw new RuntimeException('Theme archive contains invalid relative paths.'); - } + foreach ($pathParts as $part) { + if ($part === '..') { + throw new RuntimeException('Theme archive contains invalid relative paths.'); + } + }Note: move this check before the
implodeon line 161, operating on$pathPartsdirectly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php`:
- Around line 105-109: The activateTheme method in ThemeManager sets the theme
name without confirming the theme actually exists, so update
activateTheme(string $themeName) to first call assertValidThemeName($themeName)
and then verify the theme storage path contains the required template (e.g.,
check for existence of "index.twig" under the theme directory via the
ThemeManager's storage access method or filesystem helper) before calling
$this->configuration->set('layout.templateSet', $themeName); if the file is
missing, throw or return false (consistent with the method signature) and do not
update configuration.
🧹 Nitpick comments (2)
phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php (2)
166-168: Path traversal check usingstr_contains('..')is slightly overly broad.This would also reject legitimate filenames containing consecutive dots (e.g.,
backup..old.twig). A more precise check would inspect individual path segments:if (in_array('..', explode('/', $rebuiltPath), true)) {This is unlikely to matter in practice for theme files, so flagging as optional.
152-159: Root folder flattening only triggers when the archive's root directory matches$themeNameexactly.This means if a ZIP has a root folder named differently (e.g.,
mytheme-main/), the uploaded files will be nested under that extra directory, potentially preventingindex.twigfrom being found at the expected path. This could silently produce a broken theme that appears to upload successfully but fails at activation.Consider making the flattening more resilient — for example, stripping any single common root directory regardless of its name when the archive has one.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php`:
- Around line 120-123: activateDefaultTheme() currently sets
configuration->set('layout.templateSet','default') without verifying the theme
exists; update it to mirror activateTheme() by checking storage for the default
theme's index.twig (or whatever presence check activateTheme() uses) before
calling configuration->set('layout.templateSet','default'), and return false if
the default theme is missing or invalid so rendering isn't broken; reference
activateDefaultTheme(), activateTheme(), index.twig and the configuration->set
call when making the change.
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Template/ThemeManager.php (1)
156-180: Overly broad path-traversal check rejects legitimate filenames containing...
str_contains($rebuiltPath, '..')on line 175 will reject files with..anywhere in the path, including legitimate filenames likesome..file.twigor directories likea..b/style.css. Consider checking for actual traversal sequences instead.♻️ Suggested improvement
- if (str_contains($rebuiltPath, '..')) { + if (in_array('..', $pathParts, true)) { throw new RuntimeException('Theme archive contains invalid relative paths.'); }This checks for
..as a discrete path segment rather than as a substring, which is both more precise and still secure. Note that$pathPartsis already split by/and filtered for empty strings, so this catches actual traversal attempts.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests