Skip to content

fix: Starcounter-Jack JSON-Patch Prototype Pollution vulnerability#708

Merged
QuantumExplorer merged 1 commit intov0.24-devfrom
fix/json-schema-diff-validator
Jan 5, 2023
Merged

fix: Starcounter-Jack JSON-Patch Prototype Pollution vulnerability#708
QuantumExplorer merged 1 commit intov0.24-devfrom
fix/json-schema-diff-validator

Conversation

@strophy
Copy link
Copy Markdown
Collaborator

@strophy strophy commented Jan 4, 2023

Issue being fixed or feature implemented

CI was failing with:

Run yarn npm audit --environment production --all --recursive --ignore 1080920
└─ fast-json-patch: 2.2.1
   ├─ ID: 1085296
   ├─ Issue: Starcounter-Jack JSON-Patch Prototype Pollution vulnerability
   ├─ URL: https://github.com/advisories/GHSA-8gh8-hqwg-xf34
   ├─ Severity: moderate
   ├─ Vulnerable Versions: <3.1.1
   ├─ Patched Versions: >=3.1.1
   ├─ Via: fast-json-patch, json-schema-diff-validator
   └─ Recommendation: Upgrade to version 3.1.1 or later

What was done?

Force abandoned fast-json-patch dep to use patched json-schema-diff-validator.

How Has This Been Tested?

In CI

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@strophy strophy added this to the v0.24.0 milestone Jan 4, 2023
Comment thread package.json
"elliptic": "^6.5.4",
"bn.js": "4.12.0",
"ejs": "^3.1.7",
"fast-json-patch": "^3.1.1",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Only updated dependency version is not enough? Resolution usually needed if you have deep dependency problem which you can't control

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a resolution

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, but just updating version in DPP packages not enough?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand what you are asking or what change you are proposing. The change is in the monorepo root package.json because this is the only legal location for resolutions.

@QuantumExplorer QuantumExplorer merged commit 610cf93 into v0.24-dev Jan 5, 2023
@QuantumExplorer QuantumExplorer deleted the fix/json-schema-diff-validator branch January 5, 2023 23:05
shumkov added a commit that referenced this pull request May 4, 2026
Per the wasm-dpp2 CONVENTIONS.md "Versioning" section: every versioned
protocol enum should use `#[serde(tag = "$formatVersion")]`. `Validator`
and `ValidatorSet` were the last two top-level versioned enums still
defaulting to externally-tagged `{"V0": {...}}` form.

Wire shape changes from:

    {"V0": {"pro_tx_hash": "...", ...}}

to the canonical:

    {"$formatVersion": "0", "pro_tx_hash": "...", ...}

JSON-side tests pass — dashcore hash newtypes (`ProTxHash`, `PubkeyHash`,
`QuorumHash`) deserialize cleanly from hex strings on the HR path.

Value-side tests are `#[ignore]`'d pending dashcore PR #708
(dashpay/rust-dashcore#708) — the dashcore hash
newtypes need dual-shape visitors so they round-trip through serde's
`ContentDeserializer`, which always reports `is_human_readable: true`
even when wrapping bytes from a non-HR source like `platform_value::Value`.
This is the same root cause as the OutPoint/Txid bug fixed locally in
commit 09c0a2b; ProTxHash/PubkeyHash trip the same wire on
`tag = "$formatVersion"` deserialization through ContentDeserializer.

Once that PR lands and we bump the dashcore dependency, drop the
`#[ignore]`s on the two value tests.

Note: `ValidatorSetV0::members` is `BTreeMap<ProTxHash, ValidatorV0>`
(not `BTreeMap<ProTxHash, Validator>`), so members are the bare V0
struct on the wire without their own `$formatVersion` tag — the test
documents this inline.

dpp lib: 3639 passing, 10 ignored (+2 from the new value-path
`#[ignore]`s, otherwise unchanged).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shumkov added a commit that referenced this pull request May 5, 2026
…or ignores

PR #708 only fixed the `serde_struct_human_string_impl!` macro (used by
`OutPoint`), not the `hashes::serde_macros::SerdeHash` macro family used
by `ProTxHash`/`PubkeyHash`/`QuorumHash`. They have the same kind of bug
(string-only HR visitor → fails through `ContentDeserializer`) but live
in a different macro and need their own fix.

Update the `#[ignore]` notes on the two value-side tests to:
- Remove the misleading PR #708 link.
- Spell out the actual error (`HexVisitor::visit_str` sees the 32-byte
  buffer interpreted as 32 chars instead of the expected 64-char hex
  form, hence "bad hex string length 32 (expected 64)").
- Frame as a "follow-up dashcore PR" pending in the same family.

No code changes — just clarifies what we're actually waiting on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shumkov added a commit that referenced this pull request May 5, 2026
…e PR

dashcore PR #729 (dashpay/rust-dashcore#729) is
the companion to #708 — same `ContentDeserializer` HR-quirk root cause,
but for the separate `hashes::serde_macros::SerdeHash` macro family
that generates `Txid` / `BlockHash` / `ProTxHash` / `PubkeyHash` /
`QuorumHash` etc. (vs. #708 which fixed `OutPoint` via
`serde_struct_human_string_impl!`).

Update the two `#[ignore]` notes on `Validator::value_round_trip` and
`ValidatorSet::value_round_trip` to reference #729 instead of the vague
"follow-up PR" phrasing. When #729 lands and we bump dashcore, drop the
`#[ignore]`s.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

3 participants