fix: validate icon URL to prevent server-side request forgery (SSRF) - #1590
Open
bunlongheng wants to merge 459 commits into
Open
fix: validate icon URL to prevent server-side request forgery (SSRF)#1590bunlongheng wants to merge 459 commits into
bunlongheng wants to merge 459 commits into
Conversation
handle issue-pr close and review submitted actions
Spelling and Grammar, included a few apostrophes for correct plural form or possessive. Added Capitalization to proper nouns Included a single colon for consistency to match the styling of it's predecessor paragraph
Fix and add SVG support
…n-item add search via select2 for application dropdown
Since PHP 5.4 the short array syntax `[]` may be used instead of `array()`.
PHP 5.5.9 adds the new static `class` property which provides the fully qualified class name. This is preferred over using strings for class names since the `class` property references are checked by PHP.
Laravel 8 adopts the tuple syntax for controller actions. Since the old options array is incompatible with this syntax, Shift converted them to use modern, fluent methods.
In an effort to make upgrading the constantly changing config files easier, Shift defaulted them and merged your true customizations - where ENV variables may not be used.
`<env>` tags have a lower precedence than system environment variables making it easier to overwrite PHPUnit configuration values in additional environments, such a CI. Review this blog post for more details on configuration precedence when testing Laravel: https://jasonmccreary.me/articles/laravel-testing-configuration-precedence/
ItemController::appload() was declared ': ?string', so its two error branches that 'return response()->json([...], 404)' had the JsonResponse coerced through Response::__toString() into a raw HTTP message served as an HTTP 200 body. Widen the return type to '\Illuminate\Http\JsonResponse|string|null' so those branches emit real 404 JSON responses. The method body is unchanged, so the happy path still returns the same JSON string and the frontend contract is preserved. Flip the endpoint characterization test to assert the corrected 404.
PHPUnit 12 renamed PHPT -> Phpt (src/Framework/Exception/PhptAssertionFailedError.php and the src/Runner/Phpt/ directory). Because this repo commits vendor/ and the macOS dev filesystem is case-insensitive, git kept the old-case paths in the index while Composer wrote the new case to disk, so the change went undetected locally. On CI's case-sensitive Linux filesystem the checked-out old-case files don't satisfy PHPUnit's require of the new-case names and 'php artisan test' fatals before running. Re-track these 8 files under their correct case.
- itemImport: check response.ok in fetchAppDetails so a genuine 404 (from the appload return-type fix) is reported as 'Failed to find app id' instead of being parsed as a successful import; applied to the source and the committed compiled bundle. - phpunit.xml: point the schema URL at 12.5 to match the installed PHPUnit 12.5.x. - ColorHelpersTest: exercise the get_brightness() non-hex stripping the test name promised (interior separators), which the prior assertion never covered.
…-upgrade Upgrade to Laravel 13, bump to v2.8.0, and remediate vulnerabilities
The :memory: overrides in phpunit.xml had been commented out since 2024, so RefreshDatabase ran migrate:fresh against the real .env database and wiped it on every local test run. Enable the overrides, let the special :memory: identifier bypass database_path() resolution and the boot-time touch(), and add a TestCase guard that aborts the suite unless it is pointed at in-memory sqlite. ItemExportTest only ever passed by reading the populated dev database; seed the root dashboard item it depends on so it passes on a fresh DB.
Tiles could vanish without ever being deleted: - Editing an item merged the editor's user_id into every save, so updating a visible item (e.g. a shared user_id=0 tile) silently reassigned ownership and hid it from everyone else. user_id is now set on create only, and excluded from update input in both Item and Tag controllers since it is mass-assignable. - Deleting a user left their items orphaned with a dangling user_id, invisible to all users forever. The user's items are now hard-deleted with the account, and a data migration reassigns already-orphaned items to the admin user so previously "lost" tiles reappear. - The Item global scope's ownership filter had an ungrouped orWhere, breaking operator precedence in any query that adds further clauses. Includes regression coverage for ownership on create/update, user deletion cleanup, and the orphan-recovery migration.
…-upgrade Fix to try and mitigate any disappearing tiles
TrueNAS is deprecating the REST API (api/v2.0/) in version 26.04, requiring migration to JSON-RPC 2.0 over WebSocket. This commit adds: - phrity/websocket dependency for WebSocket communication - TrueNASWebSocketClient helper class that handles: - Connection to ws(s)://host/api/current - Authentication via auth.login_with_api_key - JSON-RPC 2.0 request/response formatting - TLS verification toggle - Proper connection cleanup Refs: linuxserver#1530
Adds a new boolean setting in the Advanced settings group that allows users to globally skip TLS certificate verification for all enhanced apps. This is useful for users who have self-signed certificates on their services. When enabled, the Guzzle HTTP client will set 'verify' => false for all API requests made by enhanced apps. Resolves linuxserver/Heimdall-Apps#687
Checkbox-type config options (e.g. ignore_tls) always tested as "1"
regardless of the checkbox state. The Test-button config gatherer used
$(this).val() for every .config-item, and jQuery .val() on a checkbox
returns its value attribute ("1") regardless of checked state. App
config blades pair a hidden input (0) with a checkbox (1) sharing the
same data-config, and the checkbox is last in DOM order, so it always
overwrote the value with "1". Normal form saves were unaffected.
Use the :checked state for checkboxes so unchecking now tests as "0".
Also rebuilds the committed compiled bundle (public/js/app.js) so the
fix takes effect at runtime; the edit is byte-identical to the Laravel
Mix development build output for this block.
Fixes linuxserver/Heimdall-Apps#782
The WebSocket support added for TrueNAS JSON-RPC 2.0 requires the phrity/websocket library to actually be available at runtime. This repo commits the vendor/ tree (CI does not run composer install), so the dependency and composer.lock must be committed for class_exists() checks in the TrueNAS app to succeed. - composer require phrity/websocket:^3.6 (resolves to 3.7.3) with lock and vendor/ committed - Fix TrueNASWebSocketClient to catch WebSocket\Exception\Exception (phrity/websocket v3 namespace) instead of the non-existent WebSocket\ConnectionException from the old textalk/websocket v1/v2 API, so connection/call failures are logged and wrapped as intended
…skip Add global TLS verification skip setting
…t-config Fix checkbox config values always returning "1" in Test config
…socket-client Add WebSocket support for TrueNAS JSON-RPC 2.0 API
Bump version to 2.8.1
…pdate 🇩🇪 Update German translation
SVG icons with explicit width/height attributes rendered tiny because max-width/max-height only caps intrinsic size and never scales up, while dimensionless SVGs defaulted large and were capped to 60px. Pin .app-icon to a fixed 60x60 box with object-fit: contain so every icon renders at a consistent size while preserving aspect ratio. Fixes linuxserver#1582
…sistent-size Fix inconsistent SVG tile icon sizes
Heimdall doesn't ship a config/session.php, so the cookie name falls
back to Laravel's framework default:
Str::snake((string) env('APP_NAME', 'laravel')) . '_session'
Str::snake() only inserts underscores before capital letters; it does
not strip characters like "." or spaces. PHP mangles dots in incoming
cookie/GET/POST variable names to underscores when parsing a request,
so an APP_NAME containing a dot (e.g. "example.com") produces a cookie
the app can never read back: it looks for "example.com_session" but
PHP only ever hands it "example_com_session" in $_COOKIE.
That desyncs the session on every single request, so the CSRF token
embedded in any page never matches the token generated on submit,
producing a 419 Page Expired on every POST. Reproduced this with a
bare curl round-trip (fetch page, extract token+cookie, POST back
immediately) with no browser involved at all, which rules out any
browser-specific cause like cookie caching or extensions - this is a
server-side PHP request-parsing behavior, not a browser quirk.
Root cause and fix were worked out with Claude Code: it decrypted the
session cookie server-side and confirmed the mismatch with a $_COOKIE
probe against config('session.cookie') on a live instance where this
was happening.
This matches the symptoms in several previously-reported, unresolved
419 issues:
- linuxserver#398 - Getting 419 error code when trying to add apps
- linuxserver#443 - 419 "Sorry, your session has expired" when submitting new app
- linuxserver#590 - 419 error when hitting save
- linuxserver#873 - 419 Page Expired doing any POST requests
Setting SESSION_COOKIE explicitly in .env already works around this
today (the framework default respects it), but nothing points users
at that until they've already hit the bug. Adding config/session.php
pins the cookie name to a fixed value by default (still overridable
via SESSION_COOKIE) so it no longer depends on APP_NAME at all, fixing
it out of the box for new and existing installs alike.
App-level config files merge over the framework's defaults key by key
(Illuminate\Foundation\Bootstrap\LoadConfiguration), so this only
needs to override the one key - verified the rest of session.* still
resolves from the framework defaults unchanged.
…-appname-dot Pin session cookie name, stop deriving it from APP_NAME
Bump version to 2.8.2
The add/edit-item handler fetches a user-supplied icon URL with file_get_contents and no host validation, so an attacker can point it at internal services or cloud metadata (e.g. http://169.254.169.254/) and use Heimdall as a request proxy. Heimdall runs without authentication by default, so this is reachable on default installs. Restrict the URL to http/https, resolve the host and reject private or reserved addresses, and stop following redirects so a public URL cannot bounce to an internal one.
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
20 participants
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.
What
The add/edit-item handler (
ItemController@store/@update) fetches a user-supplied icon URL:There's no check that the URL's host isn't internal, so a request like
icon=http://169.254.169.254/latest/meta-data/x.pngoricon=http://127.0.0.1:PORT/x.pngmakes Heimdall issue that request server-side (SSRF). The image validation only decides whether the response body is stored, so blind SSRF (internal port scan, cloud metadata, hitting internal-only HTTP services) works regardless.verify_peeris also disabled.Since Heimdall runs without authentication by default, this is reachable on a default install.
Fix
http/https.FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE(covers loopback, link-local169.254.0.0/16, and RFC1918).302to an internal one.Legitimate remote icons (public hosts) work exactly as before.
Verification
Ran the exact guard logic under PHP 8.5 against representative payloads - 9/9 as expected:
http://169.254.169.254/...(metadata)http://127.0.0.1/...(loopback)http://10/172.16/192.168...(private)ftp://,file://http://8.8.8.8/..., public IPsphp -lclean.Honest scope / caveats
This closes the practical SSRF vectors (metadata, loopback, RFC1918). Two residual notes so it's not oversold: there's a small DNS-rebinding window because resolution and fetch aren't atomic (redirects are disabled to reduce that), and PHP's
filter_varIPv6 reserved handling has known edges. Happy to iterate on the approach - e.g. centralizing this into a helper or using a stricter resolver - if you'd prefer.