Skip to content

fix: add authorization checks on management outfit endpoints + widen SQL columns#2

Open
sudorest wants to merge 1 commit intoBluecoastroleplay:mainfrom
sudorest:fix/security-auth-checks-sql-truncation
Open

fix: add authorization checks on management outfit endpoints + widen SQL columns#2
sudorest wants to merge 1 commit intoBluecoastroleplay:mainfrom
sudorest:fix/security-auth-checks-sql-truncation

Conversation

@sudorest
Copy link
Copy Markdown

Summary

Fixes #1 — addresses the two critical authorization bypasses and the SQL truncation risk reported in the security issue.

Changes

1. deleteManagementOutfit — missing authorization (Critical)

Before: Any connected player who knows a management outfit ID could delete any job/gang outfit from the database. The handler only checked that id was a number.

After: The handler now:

  • Validates the mType parameter ('Job' or 'Gang')
  • Looks up the outfit by ID via a new Database.ManagementOutfits.GetByID() helper
  • Verifies the player belongs to the same job/gang as the outfit (job.name == outfit.job_name)
  • Checks that the player has boss-level rank (job.isboss)

2. saveManagementOutfit — insufficient authorization (Critical)

Before: Any employee of a job (even the lowest rank) could create management outfits for their organization. The handler only checked that the player's job name matched outfitData.JobName.

After: Added job.isboss check so only boss/manager-rank players can create management outfits.

3. SQL VARCHAR columns silently truncating DLC outfit data

Before: props VARCHAR(1000) and components VARCHAR(1500) in both player_outfits and management_outfits tables. DLC collection metadata (e.g. collectionHash strings) can push JSON payloads beyond these limits, causing silent truncation and corrupted outfit saves.

After: Both columns changed to TEXT in the schema definitions, matching how the playerskins table already handles variable-length JSON (skin TEXT).

Note for existing deployments: The SQL files define CREATE TABLE IF NOT EXISTS, so existing tables won't be automatically altered. Servers with existing data should run:

ALTER TABLE player_outfits MODIFY COLUMN props TEXT DEFAULT NULL;
ALTER TABLE player_outfits MODIFY COLUMN components TEXT DEFAULT NULL;
ALTER TABLE management_outfits MODIFY COLUMN props TEXT DEFAULT NULL;
ALTER TABLE management_outfits MODIFY COLUMN components TEXT DEFAULT NULL;

4. New Database.ManagementOutfits.GetByID() helper

Added to server/database/managementoutfits.lua to support the delete authorization logic — fetches a single management outfit row by its primary key.

Files Changed

File Change
server/main.lua Added job.isboss check to saveManagementOutfit; rewrote deleteManagementOutfit with full job membership + boss rank validation
server/database/managementoutfits.lua Added GetByID(id) function
sql/player_outfits.sql props and components columns: VARCHARTEXT
sql/management_outfits.sql props and components columns: VARCHARTEXT

…SQL columns

Addresses three issues reported in Bluecoastroleplay#1:

1. deleteManagementOutfit now verifies the player belongs to the outfit's
   job/gang AND holds a boss-level rank before allowing deletion.
   Previously any connected player could delete any management outfit by ID.

2. saveManagementOutfit now checks job.isboss in addition to the existing
   job name check, ensuring only boss/manager-rank players can create
   management outfits for their organization.

3. props and components columns in both player_outfits and management_outfits
   SQL schemas changed from VARCHAR(1000)/VARCHAR(1500) to TEXT, preventing
   silent truncation of DLC collection data that can exceed the old limits.

Also adds Database.ManagementOutfits.GetByID helper used by the delete
authorization logic.
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.

[Security] Missing authorization checks on management outfit endpoints + SQL truncation risk

1 participant