Skip to content

fix(SaveInterface): add hasOwnProperty checks to prevent object injection#5215

Open
WillyEverGreen wants to merge 1 commit intosugarlabs:masterfrom
WillyEverGreen:fix/saveinterface-object-injection
Open

fix(SaveInterface): add hasOwnProperty checks to prevent object injection#5215
WillyEverGreen wants to merge 1 commit intosugarlabs:masterfrom
WillyEverGreen:fix/saveinterface-object-injection

Conversation

@WillyEverGreen
Copy link
Contributor

PR Description

Overview

This PR addresses security warnings (security/detect-object-injection) identified by ESLint in js/SaveInterface.js. It adds proper property existence checks to prevent potential object injection vulnerabilities.

Related

Changes Made

Added Object.prototype.hasOwnProperty.call() checks before accessing object/array properties with variable keys:

  • MIDI Mapping Lookups: Validates drumMIDI[drum] and instrumentMIDI[instrument] with fallbacks to default values.
  • Array Access in saveAbc(): Added property existence validation for notationStaging[t] and notationDrumStaging[t].
  • Array Access in saveLilypond(): Implemented consistent validation patterns for Lilypond export staging.

Benefits

  • Security: Prevents potential object injection attacks where malicious input could access unexpected object properties.
  • Code Quality: Eliminates all 26 ESLint security/detect-object-injection warnings for this file.
  • Robustness: Adds defensive checks that gracefully handle edge cases and missing keys.

Verification

  • Ran npx eslint js/SaveInterface.js and confirmed 0 errors and 0 warnings (previously had 26 warnings).
  • Ran npx prettier --check to ensure all files pass formatting checks.
  • Manual testing: Confirmed MIDI generation and notation export still work correctly.

@github-actions
Copy link
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

themebox.test.js

@vanshika2720
Copy link
Contributor

@WillyEverGreen Jest cases are failing.Please resolve

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.

2 participants