Skip to content

Fix user-linking, XSS, and port allocation bugs; complete translations - #58

Open
RATR2 wants to merge 1 commit into
wohahobg:mainfrom
RATR2:fix/user-linking-xss-port-allocation
Open

Fix user-linking, XSS, and port allocation bugs; complete translations#58
RATR2 wants to merge 1 commit into
wohahobg:mainfrom
RATR2:fix/user-linking-xss-port-allocation

Conversation

@RATR2

@RATR2 RATR2 commented Aug 13, 2026

Copy link
Copy Markdown
  • getPterodactylUser() trusted a colliding external_id with no verification, which could hand a customer's new server to an unrelated existing panel account; now requires the email to match before trusting the match
  • Fixed reflected XSS in the client area server-action links - unescaped $_SERVER host/URI was reflected straight into inline onclick JS
  • Scoped AdminAreaHeadOutput/AdminAreaFooterOutput hooks to only load the module's assets on relevant admin pages instead of every admin page
  • Reworked CreateAccount to resolve the exact node/ports up front and fail fast if unavailable, instead of letting Pterodactyl auto-deploy and then reconciling/swapping allocations afterward; this also removes a bug where the swap step sent the same allocation id as both primary and additional
  • Added the missing direct-access guard to helper.php, matching hooks.php
  • PreDeleteClient/ClientClose compared an API response array against the literal int 404, which is always true; now checks status_code correctly
  • Filled in translation keys missing from several language files (German, Danish, Dutch, French, Polish, Portuguese, Traditional Chinese) that were never updated after newer strings were added, and fixed a handful of typos (German "EZWINGEN", Polish "ZRESERTUJ"/"napewno"/"istneje")

- getPterodactylUser() trusted a colliding external_id with no verification,
  which could hand a customer's new server to an unrelated existing panel
  account; now requires the email to match before trusting the match
- Fixed reflected XSS in the client area server-action links - unescaped
  $_SERVER host/URI was reflected straight into inline onclick JS
- Scoped AdminAreaHeadOutput/AdminAreaFooterOutput hooks to only load the
  module's assets on relevant admin pages instead of every admin page
- Reworked CreateAccount to resolve the exact node/ports up front and fail
  fast if unavailable, instead of letting Pterodactyl auto-deploy and then
  reconciling/swapping allocations afterward; this also removes a bug where
  the swap step sent the same allocation id as both primary and additional
- Added the missing direct-access guard to helper.php, matching hooks.php
- PreDeleteClient/ClientClose compared an API response array against the
  literal int 404, which is always true; now checks status_code correctly
- Filled in translation keys missing from several language files (German,
  Danish, Dutch, French, Polish, Portuguese, Traditional Chinese) that were
  never updated after newer strings were added, and fixed a handful of
  typos (German "EZWINGEN", Polish "ZRESERTUJ"/"napewno"/"istneje")
@RATR2

RATR2 commented Aug 13, 2026

Copy link
Copy Markdown
Author

My PR overlaps with AjdinDev's PR

i recomend choosing mine over theirs, because it is more extensive.

@RATR2

RATR2 commented Aug 13, 2026

Copy link
Copy Markdown
Author

To expand on why I'd suggest taking this one over #57 rather than both: the admin asset-loading fix in this PR covers the same ground as #57 (unconditional AdminAreaHeadOutput/AdminAreaFooterOutput injection on every admin page - not just PteroSync's own pages), but is one part of a larger set of fixes found while auditing the module end-to-end. Merging this would cover #57's fix too, so you wouldn't need both.

What else is in here, and why:

Security/correctness

  • getPterodactylUser() trusted a matching external_id completely blindly, with no check that it actually belonged to the WHMCS client it was being used for. If any pre-existing Pterodactyl account (staff, a manually-created account, leftover data) happened to have an external_id that coincidentally matched a WHMCS client's id, the module would silently treat it as that client's account and hand it a brand-new server - without ever touching or verifying the account. Now it checks the email matches before trusting the link.
  • The client area's server action buttons (start/stop/restart/kill) build their URLs from raw $_SERVER['HTTP_HOST']/REQUEST_URI, which get reflected unescaped into inline onclick JS in overview.tpl. That's a reflected XSS - a crafted link to a customer's own service page could execute arbitrary script in their WHMCS session.

Reliability

  • CreateAccount used to let Pterodactyl auto-deploy a port, then search for the "real" port afterward and swap allocations if the auto-picked one didn't match - which happens on nearly every creation, since the deploy step is told to pick from the same range the swap logic then searches. The swap itself had a bug: it sent the same allocation id as both the primary allocation and in add_allocations in the same request. I reworked it to resolve the exact node and every required port up front, and cancel the order before anything is created if the ranges are full - so you get a clear error instead of a half-configured server sitting on the panel that needs manual cleanup.
  • PreDeleteClient/ClientClose compared the API response array against the literal int 404 ($userResult !== 404), which is always true since the value is never actually an int. Currently harmless since the delete call underneath is commented out, but the guard itself was dead.
  • helper.php was missing the defined("WHMCS") direct-access guard that hooks.php has.

Translations

  • Several language files (German, Danish, Dutch, French, Polish, Portuguese, Traditional Chinese) were missing keys added after their initial translation and never updated - SERVER_SETTINGS, SERVER_STARTUP, SERVER_OVERVIEW, and the two "editing not allowed" messages render as blank text for anyone on those languages. Filled those in, plus fixed a few real typos I noticed along the way (German "EZWINGEN" → "ERZWINGEN", Polish "ZRESERTUJ" → "ZRESTARTUJ", etc).

Happy to split any of this into separate PRs if that's easier to review in pieces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant