Repository navigation
fix(security): MagicAI model-fetch SSRF + role permission escalation - #689
Merged
Merged
Conversation
…alation Role create/update wrote the submitted permissions array verbatim and only blocked promotion to all-access, so a delegated role manager (holding settings.roles.edit) could edit its own role to grant permissions it did not hold — vertical privilege escalation. RoleForm::authorize now also rejects any permission the acting admin does not itself hold, mirroring the assignment guard already in UserController. Adds the cannot-grant-unheld-permissions message across all 33 locales. MagicAI model discovery validated the submitted api_url with SafeWebhookUrl but then fetched it with a bare Guzzle client that follows redirects and re-resolves DNS, so a validated public URL could pivot to an internal host (cloud metadata / RFC1918) via a 30x redirect or DNS rebinding. The user-supplied fetches (Custom / Azure / Ollama) now merge SafeWebhookUrl::httpOptions — IP-pinning plus no redirect following — matching the already-hardened runtime AiApiClient. Fixed-host provider branches are unchanged. Claude-Session: https://claude.ai/code/session_01MWhxQzQ9X8rqQ8LqXzGeij
There was a problem hiding this comment.
🟡 Changes recommended
AiProvider references SafeWebhookUrl without importing/qualifying it, which will cause a runtime fatal error in the hardened request path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR applies two targeted security hardenings in Unopim’s admin area: preventing delegated role managers from self-escalating via role edits, and preventing SSRF pivots (redirect/DNS rebinding) during MagicAI “fetch models” discovery.
Changes:
- Tighten
RoleForm::authorize()to forbid granting permissions the acting admin does not hold (and deny promotion to full-access). - Harden MagicAI model discovery requests by applying
SafeWebhookUrl::httpOptions()(no redirects + IP pinning when available). - Add focused regression tests and propagate a new localized authorization message across locales.
File summaries
| File | Description |
|---|---|
| packages/Webkul/MagicAI/src/Enums/AiProvider.php | Apply SSRF-guarded HTTP options for user-supplied model discovery URLs. |
| packages/Webkul/Admin/src/Http/Requests/RoleForm.php | Add authorization guard preventing permission self-escalation on role edits. |
| packages/Webkul/Admin/tests/Feature/Settings/RolePermissionEscalationTest.php | New regression coverage for role permission escalation scenarios. |
| packages/Webkul/Admin/tests/Feature/MagicAI/FetchModelsSsrfTest.php | New regression coverage for redirect-based SSRF in model discovery. |
| packages/Webkul/Admin/src/Resources/lang/ar_AE/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/ca_ES/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/da_DK/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/de_DE/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/en_AU/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/en_GB/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/en_NZ/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/en_US/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/es_ES/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/es_VE/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/fi_FI/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/fr_FR/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/hi_IN/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/hr_HR/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/id_ID/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/it_IT/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/ja_JP/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/ko_KR/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/mn_MN/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/nl_NL/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/no_NO/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/pl_PL/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/pt_BR/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/pt_PT/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/ro_RO/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/ru_RU/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/sv_SE/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/tl_PH/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/tr_TR/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/uk_UA/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/vi_VN/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/zh_CN/app.php | Add cannot-grant-unheld-permissions translation. |
| packages/Webkul/Admin/src/Resources/lang/zh_TW/app.php | Add cannot-grant-unheld-permissions translation. |
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…st fixture - Restore the use Webkul\Webhook\Validators\SafeWebhookUrl import in AiProvider (dropped when the formatter ran before the usage existed); the unqualified reference resolved to the enum's own namespace and broke PHPStan. - Run Pint over the 33 Admin locale files so the added key aligns. - RoleUserAuthorizationTest: the acting admin now also holds `dashboard`, the permission it grants, so the ACL-gating assertion no longer trips the new "cannot grant unheld permissions" guard. Claude-Session: https://claude.ai/code/session_01MWhxQzQ9X8rqQ8LqXzGeij
httpOptions() pins the host to the validated address only when the URL validates as public; otherwise it returns the redirect guard alone. The docblock promised pinning unconditionally, which overstates the guarantee exactly where a reader is reasoning about SSRF. Claude-Session: https://claude.ai/code/session_01Es54dGoiX1pP64XqxvJmXw
navneetkumar-pim-webkul
added a commit
that referenced
this pull request
Sep 9, 2026
Resolves overlap with the MagicAI SSRF (#689) and purifier cache race (#675) fixes that landed on 3.x independently. Claude-Session: https://claude.ai/code/session_01JfF9ke2aCFppkN9pYbCfKn
navneetkumar-pim-webkul
added a commit
that referenced
this pull request
Sep 17, 2026
Eight merged pull requests since v3.1.0 carried no changelog entry: the role-escalation and MagicAI SSRF fixes (#689, #690, #691), the export column selection and category keyword matching fixes (#697, #698), the media replacement fix (#709), the Gemini endpoint (#699), the channel deletion fix (#688), the purifier cache race (#675), and the admin UI and configuration work squashed into #700. 3.1.1 is a patch release, so everything is filed under bug fixes and improvements; no feature section. The heading is stamped 3.1.1 to match Core::VERSION, which the branch already carries.
6 tasks
navneetkumar-pim-webkul
added a commit
that referenced
this pull request
Sep 17, 2026
Eight merged pull requests since v3.1.0 carried no changelog entry: the role-escalation and MagicAI SSRF fixes (#689, #690, #691), the export column selection and category keyword matching fixes (#697, #698), the media replacement fix (#709), the Gemini endpoint (#699), the channel deletion fix (#688), the purifier cache race (#675), and the admin UI and configuration work squashed into #700. 3.1.1 is a patch release, so everything is filed under bug fixes and improvements; no feature section. The heading is stamped 3.1.1 to match Core::VERSION, which the branch already carries.
sandeepp-webkul
pushed a commit
to sandeepp-webkul/unopim
that referenced
this pull request
Sep 21, 2026
* fix: validate empty files during product import instead of throwing code error (unopim#696) * fix: hide broken logo image in profile dropdown when remote fetch fails (unopim#700) * fix: disable @ attribute suggestions in profile image AI generation (unopim#701) * fix: set HTMLPurifier cache path to storage directory to prevent vendor write error during import (unopim#350) * fix: hide webhook logs tab and enforce ACL for unauthorized roles (unopim#545) * fix: EditImage tool now fetches product image by SKU instead of requiring upload (unopim#683) * fix: add XLSX export support to AI Agent ExportProducts tool (unopim#684) * fix: add SKU validation to AI Agent bulk import to reject special characters (unopim#689) * style: apply pint formatting to AiAgent lang file * fix: address copilot review on PR unopim#353
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two high-severity findings from a full-project security audit, both fixed with minimal, backward-compatible patches.
1. Privilege escalation via role editing (broken access control)
RoleController::store/updatewrote the submittedpermissions[]verbatim;RoleForm::authorize()blocked only promotion toall. A delegated role manager (custom role holdingsettings.roles.edit) could edit its own role to grant permissions it did not hold → vertical privilege escalation.Fix:
RoleForm::authorize()now also rejects any permission the acting admin does not itself hold — mirroring the guard already inUserController::roleAssignmentError(). Adds thecannot-grant-unheld-permissionsmessage across all 33 locales.2. SSRF in MagicAI model discovery
fetchModelsvalidated the submittedapi_urlwithSafeWebhookUrl, then fetched it with a bare Guzzle client that follows redirects and re-resolves DNS. A validated public URL could pivot to an internal host (cloud metadata / RFC1918) via a 30x redirect or DNS rebinding.Fix: the user-supplied fetches (Custom / Azure / Ollama) now merge
SafeWebhookUrl::httpOptions($url)— IP-pinning + no redirect following — matching the already-hardened runtimeAiApiClient. Fixed-host provider branches are unchanged.Backward compatibility
Tests
Settings/RolePermissionEscalationTest— self-escalation, other-role escalation, promote-to-all all forbidden; held-permission edit allowed.MagicAI/FetchModelsSsrfTest— validated URL still follows a redirect on a bare client (repro);SafeWebhookUrl::httpOptionsblocks it (fix primitive).Verification
pint, phpstan (level 2), pest, and
unopim:translations:check(418/418) all passed on identical code in the working checkout. CI runs the full suite on this clean branch.