Skip to content

fix(cbor): stop leaking inherited keys into encoded objects - #7250

Merged
bartlomieju merged 5 commits into
denoland:mainfrom
tomas-zijdemans:fix-cbor-for-in
Aug 2, 2026
Merged

fix(cbor): stop leaking inherited keys into encoded objects#7250
bartlomieju merged 5 commits into
denoland:mainfrom
tomas-zijdemans:fix-cbor-for-in

Conversation

@tomas-zijdemans

Copy link
Copy Markdown
Contributor

encodeObject() writes the CBOR map header from Object.keys(input).length but iterates entries with for-in, which also visits inherited enumerable keys. When the two disagree, the declared pair count no longer matches the pairs written. The header lies.

Concrete failure:

const obj = Object.assign(Object.create({ inherited: "boom" }), { own: 1 });
encodeCbor(obj);

On main this returns 21 bytes: a valid 6-byte map declaring one pair, then "inherited": "boom" appended as trailing garbage. The inherited value leaks into the wire bytes, and strict decoders reject the trailing data. Same story if something pollutes Object.prototype with an enumerable property: every encoded object grows a stowaway pair.

The fix makes calcObjectEncodingSize() and encodeObject() iterate the same Object.keys() array the header count comes from, so count, buffer size, and written pairs can't drift apart.

Review focus: the two loops in cbor/_common_encode.ts. Output for plain objects without inherited enumerables is byte-for-byte unchanged.

Tested with the existing cbor suite (83 pass) plus the repro above, which returns the correct 6 bytes on this branch. Happy to add it as a regression test in encode_cbor_test.ts if wanted.

@github-actions github-actions Bot added the cbor label Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.03%. Comparing base (df4d14d) to head (eb6bb60).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7250      +/-   ##
==========================================
- Coverage   95.03%   95.03%   -0.01%     
==========================================
  Files         619      618       -1     
  Lines       51499    51496       -3     
  Branches     9301     9300       -1     
==========================================
- Hits        48940    48937       -3     
  Misses       2021     2021              
  Partials      538      538              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bartlomieju

Copy link
Copy Markdown
Member

Nice catch — the Object.keys().length header vs. for…in iteration mismatch is a real correctness bug, and this also brings the sync encoder in line with CborSequenceEncoderStream, which already iterated Object.keys/Object.entries. Two small things:

  1. The comment on cbor/_common_encode.ts:53 refers to calcObjectEncodingSize(), but there's no such symbol — the function is calcEncodingSize(). Could you fix the name?

  2. Could you add the regression test you offered? Something like encodeCbor() ignores inherited enumerable properties in cbor/encode_cbor_test.ts. Without it the fix is only covered incidentally by existing tests, and this is exactly the kind of thing that regresses quietly.

Separately, and out of scope for this PR: _common_encode.ts:58 sizes object keys as y.length bytes while encodeString() writes UTF-8, so non-ASCII keys under-allocate the buffer — encodeCbor({ "é☃é☃é☃é☃": 1 }) throws today. The value branch on line 34 correctly uses length * 3. Worth a separate issue if you'd like to file one.

@tomas-zijdemans

Copy link
Copy Markdown
Contributor Author

Small twist on the comment fix: line 53 already said calcEncodingSize(). The stale name was hiding in the mirror comment inside encodeObject() (line 220), so that's the one I fixed.

Regression test added as suggested. To make sure it earns its keep I ran it against the encoder from main before the fix: fails there, passes here.

Good catch on the key sizing. Filing it as a separate issue .

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for addressing the feedback

@bartlomieju
bartlomieju merged commit 80a56c0 into denoland:main Aug 2, 2026
17 checks passed
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.

2 participants