fix(docker): prevent case-duplicate user templates#2620
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:
WalkthroughAdd helpers (PHP and shell) to deterministically resolve per-container user template XML paths (exact or case-insensitive), prefer resolved user templates when creating/editing/autostarting/networking containers, update rename/unlink to operate on resolved paths, and defer startup when template resolution or XML load fails. Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as "docker_init / rc.docker"
participant PHP as "DockerTemplates::getUserTemplatePath"
participant FS as "Filesystem"
participant Loader as "XML Loader"
Shell->>PHP: request template path for CONTAINER
PHP->>FS: ensure templates-user dir exists
PHP->>FS: check exact `my-CONTAINER.xml`
alt exact match
FS-->>PHP: return exact path
else case-insensitive match
FS-->>PHP: return matched path
else no match
FS-->>PHP: return default target path
end
Shell->>Loader: load(resolved_path)
alt load succeeds
Loader-->>Shell: parsed -> continue start/network
else load fails or no file
Loader-->>Shell: error -> defer container
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
elibosley
force-pushed
the
codex/fix-docker-template-case-duplicates
branch
from
57939a8 to
5865a7f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php`:
- Line 84: When renaming/cleaning up templates use the resolved path from
getUserTemplatePath instead of constructing "my-$existing.xml"; replace the
deletion logic that builds "my-$existing.xml" (lines referencing $existing) so
it calls $DockerTemplates->getUserTemplatePath($existing) and deletes that
resolved $oldFilename (same approach used to set $filename from
getUserTemplatePath($Name)), ensuring case-sensitive files are correctly
targeted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cfde94bf-8314-411c-a03c-434ee6592530
📒 Files selected for processing (4)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpemhttp/plugins/dynamix.docker.manager/include/DockerClient.phpemhttp/plugins/dynamix.docker.manager/scripts/docker_initetc/rc.d/rc.docker
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix.docker.manager/scripts/docker_init
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php`:
- Around line 83-85: The POSTed $sourceTemplate path is only checked as a user
template file but not validated against the container being edited; change the
logic that sets $sourceUserTemplate so after
unscript(urldecode($sourceTemplate)) and the existing file/dirname/preg_match
checks you also parse the XML at $sourceTemplate and verify its <Name> equals
the container name being edited (the target container identifier used by
CreateDocker.php); if it doesn’t match, treat it as invalid and instead call
getUserTemplatePath() to obtain the safe template path before continuing (so
subsequent code that overwrites or deletes the template uses a verified path).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7bbdd4ee-2efa-4ca2-aa04-592fccbc15f3
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpemhttp/plugins/dynamix.docker.manager/include/DockerClient.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
83-91:⚠️ Potential issue | 🟡 MinorValidate
sourceTemplatebelongs to the container being edited.While the current validation (lines 85-89) confirms
sourceTemplateis a valid user-template file, it doesn't verify that the template's<Name>element matches the container being edited ($existing). A crafted POST could setsourceTemplate=my-OtherContainer.xml, causing edits to overwrite a different container's template when$existing === $Name.Given Unraid's admin-only context this is a data integrity concern rather than privilege escalation, but it could cause silent corruption of unrelated container configurations.
💡 Suggested validation
$sourceUserTemplate = $sourceTemplate && basename($sourceTemplate)==$sourceTemplate && preg_match('/^my-[^\/\\\\]+\.xml$/', $sourceTemplate); if ($sourceUserTemplate) { $sourceTemplate = "$userTmplDir/$sourceTemplate"; - $sourceUserTemplate = is_file($sourceTemplate); + if (is_file($sourceTemplate)) { + $sourceXml = `@simplexml_load_file`($sourceTemplate); + $sourceUserTemplate = $sourceXml && (string)($sourceXml->Name ?? '') === $existing; + } else { + $sourceUserTemplate = false; + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15d9c424-feb7-4ccd-8256-02fd3da1b807
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpemhttp/plugins/dynamix.docker.manager/include/DockerClient.php
|
Not taking this one for now. The posted value is no longer trusted as a path: the form posts only the basename, the server requires it to match |
- Purpose: keep Docker user template saves consistent when container names differ only by case. - Before: saving a container template used `my-$Name.xml` directly, while other paths performed case-insensitive lookup for legacy FAT boot media. - Problem: on case-sensitive internal/ZFS boot media, edits or case-only renames could create a new `my-Name.xml` instead of reusing an existing `my-name.xml`. - New behavior: Docker template saves reuse an existing exact or case-insensitive user-template path before writing. - Implementation: add deterministic user-template path lookup and use it from create/edit, docker init, and Docker startup network restore. - Scope: this change does not add cleanup or delete duplicate templates.
- Purpose: address PR review feedback on Docker template rename handling. - Before: the renamed-container cleanup path compared and deleted a manually constructed my-<name>.xml path. - Problem: on case-sensitive boot storage, that constructed path can miss the actual existing template filename when only case differs. - Now: the existing template path is resolved through DockerTemplates::getUserTemplatePath before comparison and deletion. - How: reuse the same case-aware lookup helper used for saving the current container template, without adding any duplicate cleanup pass.
- Purpose: address duplicate case-variant template feedback from PR testing. - Before: the Docker edit path could open one case-variant XML while saving preferred another exact my-<Name>.xml path. - Problem: on case-sensitive boot storage, existing duplicate templates could silently split edits across two files. - Now: edit forms carry the opened user-template path and unchanged-name saves write back to that source file. - How: getUserTemplate and name-scoped getTemplateValue now prioritize the same case-aware resolver used by saves, while CreateDocker preserves the opened source path for normal edits and uses it as the old template during renames.
- Purpose: simplify the duplicate case-variant template fix after review. - Before: the branch reordered template-value lookups and changed getUserTemplate selection rules to keep edit and save paths aligned. - Problem: that was broader and harder to reason about than necessary for the reported split-state edit/save bug. - Now: edit forms post only the opened user-template basename, and unchanged-name saves write back to that exact template file. - How: CreateDocker reconstructs accepted my-*.xml basenames under templates-user, rejects path separators such as ../../, and falls back to getUserTemplatePath for new saves and renames while DockerClient sanitizes container-derived template path names.
elibosley
force-pushed
the
codex/fix-docker-template-case-duplicates
branch
from
7bbe2aa to
4bdaba8
Compare
2 participants
Summary
my-*.xmlbasenames undertemplates-userDockerTemplates::getUserTemplatePath()when no source template was postedfind -inametemplate lookup with deterministic single-file lookupTesting
php -l emhttp/plugins/dynamix.docker.manager/include/DockerClient.phpphp -l emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpphp -l emhttp/plugins/dynamix.docker.manager/scripts/docker_initphp -d short_open_tag=1 -l emhttp/plugins/dynamix.docker.manager/include/DockerClient.phpphp -d short_open_tag=1 -l emhttp/plugins/dynamix.docker.manager/include/CreateDocker.phpphp -d short_open_tag=1 -l emhttp/plugins/dynamix.docker.manager/scripts/docker_initbash -n etc/rc.d/rc.dockergit diff --checkroot@192.168.0.206and verified edit/save updates an existing mixed-case template instead of creating a lowercase duplicateroot@192.168.0.206with a mismatched-case autostart template probe and verified no duplicate template was createdroot@192.168.0.206(my-FIREfox2.xmlplusmy-Firefox2.xml) and verified unchanged-name save from the openedmy-FIREfox2.xmlbasename updates only that opened filesourceTemplate=../../my-FIREfox2.xmlis rejected and falls back to normal canonical save behavior rather than traversing pathsNotes
This PR is intentionally separate from
codex/docker-endpoint-mac-addressand contains only the template case load/save/startup lookup fix plus existing rename-time old-template deletion path correction. It does not add a duplicate cleanup pass and does not remove duplicate template files.Summary by CodeRabbit
New Features
Bug Fixes
Stability