🐛 Fixed navigation icons failing to save on local installs - #30476
🐛 Fixed navigation icons failing to save on local installs#30476acburdine wants to merge 1 commit into
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-09-02 20:55:28 UTC
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (11)
🧰 Additional context used📓 Path-based instructions (6)Review API contract semantics: authentication and permissions, validation at untrusted boundaries, writable-field allowlists, accidental response-data exposure, stable error codes/statuses, pagination/filter consistency, cache invalidation,...⚙️ CodeRabbit configuration file Files:
Review whether tests prove changed behaviour, meaningful error/edge paths, and externally observable contracts without coupling to implementation details.⚙️ CodeRabbit configuration file Files:
New source files must be TypeScript: flag new JS files as a required change unless exempt (DB migrations, apps/ember-admin/, tool/config files, scripts/, docker/, generated code).⚙️ CodeRabbit configuration file Files:
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.⚙️ CodeRabbit configuration file Files:
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB migration (ghost/core/core/server/data/migrations/), under apps/ember-admin/, a tool/config file, under scripts/ or docker/, or generated...📄 CodeRabbit inference engine (Custom checks) Files:
Always use `pnpm`, never npm or Yarn.📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (5)
WalkthroughThe icon URL validator now accepts HTTP and HTTPS URLs with the hostname Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change permits navigation icons hosted on localhost while preserving existing URL restrictions; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Full details: Type-Safe BoundariesExplanation PASS. The PR adds runtime validation for the boundary icon string. Full details: New Files Are TypescriptExplanation PASS. The pull request changes only two existing ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30476 +/- ##
==========================================
- Coverage 67.47% 67.46% -0.02%
==========================================
Files 1656 1656
Lines 59993 59997 +4
Branches 10379 10379
==========================================
- Hits 40482 40476 -6
- Misses 17223 17235 +12
+ Partials 2288 2286 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Could we keep the existing TLD requirement and make an exception for exactly One option is to leave function isValidNavigationIcon(value) {
if (!_.isString(value) || /\s/.test(value)) {
return false;
}
if (validator.isURL(value, iconUrlOptions) || iconUrlRegex.test(value)) {
return true;
}
try {
return (
new URL(value).hostname === 'localhost' &&
validator.isURL(value, { ...iconUrlOptions, require_tld: false })
);
} catch {
return false;
}
}This allows |
ref https://forum.ghost.org/t/working-locally-on-navigation-and-cant-save-icons/63703 Navigation icon URLs are validated with `validator.isURL`, which defaults to `require_tld: true`. Admin uploads an icon and sends back the absolute site URL, so on a local install that is `http://localhost:2368/content/images/...`. `localhost` has no TLD, the icon regex only covers `/...` and `__GHOST_URL__/...`, and input validation runs before the URL transform in the serializer — so every save was rejected with a 422 "Please enter a valid navigation item". The navigation `url` field never hit this because its regex accepts any scheme prefix. Rather than dropping `require_tld` altogether, which would also accept arbitrary single-label hosts such as `http://intranet/icon.svg` on production sites, the TLD requirement is kept and an exception is made for a parsed hostname of exactly `localhost`. The protocol allowlist still applies, so `ftp://localhost:2368/icon.svg` stays invalid. The existing test passed because the test config URL is an IP address, which `isURL` accepts — the same reason `127.0.0.1:2368` was the forum workaround. The new tests use a literal localhost icon URL and lock down the scope of the exception from both sides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0c0a94b to
02964ed
Compare
1 participant
ref https://forum.ghost.org/t/working-locally-on-navigation-and-cant-save-icons/63703
Why
Navigation items with icons could not be saved on a local install. Every save came back as a 422
Please enter a valid navigation item, with no indication of which field was at fault. The forum workaround was to run the site on127.0.0.1:2368instead oflocalhost:2368.What
Navigation icon URLs are validated in
core/server/api/endpoints/utils/validators/input/settings.jswithvalidator.isURL, whoserequire_tldoption defaults totrue. Admin uploads an icon and gets back the absolute site URL, so on a local install the value sent ishttp://localhost:2368/content/images/...:localhosthas no TLD, soisURLreturns falseiconUrlRegexonly covers/...and__GHOST_URL__/..., so it doesn't rescue itThe navigation
urlfield was never affected becausenavUrlRegex(^(\/|#|[a-zA-Z0-9-]+:)) accepts anything with a scheme prefix. Icons had no equivalent escape hatch.Dropping
require_tldaltogether would also start accepting arbitrary single-label hosts such ashttp://intranet/icon.svgon production sites, so instead the TLD requirement stays and a narrow fallback allows a parsed hostname of exactlylocalhost. The protocol allowlist still applies on that path, softp://localhost:2368/icon.svgremains invalid.Notes for reviewers
The existing coverage passed because it builds the icon URL from
config.get('url'), which in the test environment ishttp://127.0.0.1:2369— an IP address, whichisURLaccepts. That is the same reason the forum workaround worked. The new tests hardcode alocalhost:2368icon URL so it can't drift back, and bound the exception from both sides.Verified behaviour against the real validator:
http://localhost:2368/content/images/x.svghttps://example.com/x.svg,http://127.0.0.1:2368/x.svg__GHOST_URL__/content/images/x.svg,/content/images/x.svghttp://intranet/icon.svgftp://localhost:2368/icon.svgmailto:,http://local host/x.svgThe legacy suite needs MySQL on port 3306, which isn't available in this environment, so the new tests were not executed locally — CI will run them.
🤖 Generated with Claude Code