Skip to content

fix: block prototype-polluting set paths#186

Closed
shepherdwind wants to merge 4 commits into
masterfrom
codex/fix-prototype-pollution-issue-in-velocityjs
Closed

fix: block prototype-polluting set paths#186
shepherdwind wants to merge 4 commits into
masterfrom
codex/fix-prototype-pollution-issue-in-velocityjs

Conversation

@shepherdwind
Copy link
Copy Markdown
Owner

Motivation

  • Prevent prototype pollution when rendering attacker-controlled templates that use #set to write properties like __proto__, constructor, or prototype into object paths.
  • The #set implementation resolved and wrote path segments without guarding prototype-polluting keys, allowing modifications to Object.prototype in the running process.

Description

  • Add a denylist POLLUTING_KEYS = new Set(['__proto__','constructor','prototype']) and helper isPollutingKey in src/compile/set.ts to detect dangerous keys.
  • Short-circuit #set writes when the root reference (ref.id) or any resolved path segment matches a polluting key to avoid writing into prototype properties.
  • Add regression tests in test/set.test.ts to cover __proto__, constructor.prototype, and index-based "__proto__" pollution attempts.
  • Modified files: src/compile/set.ts and test/set.test.ts.

Testing

  • Ran linting with npm run lint and it completed successfully.
  • Ran unit tests with npm test -- --runInBand and all test suites passed (175 passed, 175 total), including the new prototype protection tests.
  • Ran the build-and-test flow with npm run test:build and the build + build-tests succeeded.

Codex Task

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9bf03f1370

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/compile/set.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7be7881df4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/compile/set.ts
Comment on lines +52 to +53
if (isPollutingKey(ref.id) || this.hasPollutingPropertyPath(ref)) {
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve safe own constructor/prototype fields

When a template writes through an ordinary data object that has an own field named constructor or prototype (for example #set($model.constructor.name = 'Car') with { model: { constructor: {} } }), this early check skips the whole assignment even though it never traverses Object.prototype; the dangerous case is the inherited constructor.prototype chain, not every data property with these names. This silently drops valid template updates for contexts that legitimately expose those keys.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

val = this.config.valueMapper?.(this.getLiteral(ast.equal[1]));

P2 Badge Defer RHS evaluation for blocked dynamic keys

For dynamic index paths such as #set($key = "__proto__") #set($target[$key].polluted = $markEvaluated()), the new static guard does not run because the path segment is an index, so the RHS is evaluated here before the traversal reaches the isPollutingKey(key) check and skips the write. That means application callbacks or other RHS side effects still run for prototype-polluting paths, unlike the static __proto__ case covered by the new test; validate/resolve the dynamic path before evaluating the assigned value or defer RHS evaluation until after the key check.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Owner Author

Closed in favor of #188, which reworks the fix from a path-resolution model instead of the earlier broad key denylist.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant