Skip to content

fix(security): harden dynamic object maps against prototype pollution - #1283

Open
ghiscoding wants to merge 2 commits into
masterfrom
bugfix/security-audit
Open

fix(security): harden dynamic object maps against prototype pollution#1283
ghiscoding wants to merge 2 commits into
masterfrom
bugfix/security-audit

Conversation

@ghiscoding

@ghiscoding ghiscoding commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

replicate Slickgrid-Universal PR 2742 and PR 2743 into SlickGrid by doing a security audit using AI

Summary

Harden SlickGrid's dynamic object maps against prototype pollution risks.

Changes

  • Use prototype-free maps for DataView, Grid, aggregators, CSS overlays, copy managers, and composite editors.
  • Safely handle dynamic __proto__, constructor, and toString keys.
  • Replace unsafe hasOwnProperty() calls with Object.prototype.hasOwnProperty.call().
  • Protect dynamically assigned Row Detail properties.
  • Preserve generated dist/ files as build artifacts; they are not intended for inclusion in the PR.

Validation

  • TypeScript check passed.
  • ESLint passed.
  • git diff --check passed.

LLM

Implemented and reviewed with OpenAI GPT-5.6 Luna.

@ghiscoding
ghiscoding requested a review from 6pac August 22, 2026 04:51
@6pac

6pac commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Honestly, I feel like this is a bit over the top. The risk of prototype pollution of, for example, the object constructor is pretty low. If the attacker can do that, then it seems likely there are a lot of other attack paths that could be open as well.

Ultimately, nothing on the client side is ever to be considered secure, and the only way to enforce security correctly is to check permissions related business logic at the point of entry at the server.
I suspect there are a high percentage of programs that don't do this out there in the wild.

But in the end, I have no objection. It doesn't hurt, and if this is recommended, then perhaps it is the response to a particular automated attack vector that is getting traction.

@ghiscoding

Copy link
Copy Markdown
Collaborator Author

I'll let you decide if you want to merge or not, the reason why this came up was because a researcher team found similar entry in another repo of mine (excel-builder-vanilla) and I went ahead and scanned all my repos for the same thing and that is what came up... so feel free to close the PR if you wish. Cheers

@6pac

6pac commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Could you give me more info about the researcher team?

@ghiscoding

ghiscoding commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Could you give me more info about the researcher team?

I forwarded you the email I originally received, it's probably a scanning system that detected it (not the actual researchers but their scanning system)

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