fix: avoid Object.prototype collisions in remark headings map to fix …#1676
fix: avoid Object.prototype collisions in remark headings map to fix …#1676h30s wants to merge 3 commits intojson-schema-org:mainfrom
Conversation
|
@jdesrosiers PTAL |
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
As much as it pains me to say it, you can remove the tests. Adding tests in a repo that isn't setup to run tests are just a waste. They won't get run just become stale.
|
@jdesrosiers I have removed the tests as requested. Please let me know if any further changes are needed. |
remark/remark-headings.js
Outdated
| file.data.headings = {}; | ||
| file.data.headings = Object.create(null); | ||
|
|
||
| visit(tree, "heading", (headingNode) => { | ||
| if (headingNode.data?.id) { | ||
| if (headingNode.data.id in file.data.headings) { | ||
| if (Object.prototype.hasOwnProperty.call(file.data.headings, headingNode.data.id)) { |
There was a problem hiding this comment.
You've fixed this in two different ways when it only needs one, I think. If you use Object.create(null), then you shouldn't need hasOwnProperty. But, if you just define it as {}, then hasOwnProperty is necessary. So, pick one solution.
And if you pick the hasOwnProperty approach, use Object.hasOwn instead.
remark/remark-reference-links.js
Outdated
| const headerText = nodeToString(file.data.headings[id]); | ||
| const linkText = text(file.data.headings[id].data.section); | ||
| const headingNode = file.data.headings[id]; | ||
| const headerText = nodeToString(headingNode); | ||
| const linkText = text(headingNode.data.section); |
There was a problem hiding this comment.
This doesn't have anything to do with what you're fixing. Please revert it.
|
@jdesrosiers PTAL |
| findAndReplace(tree, [referenceLink, (value, id) => { | ||
| // file.data.headings comes from ./remark-headings.js | ||
| if (!(id in file.data.headings)) { | ||
| if (!Object.prototype.hasOwnProperty.call(file.data.headings, id)) { |
There was a problem hiding this comment.
You're still using hasOwnProperty here.
2 participants
What kind of change does this PR introduce?
Bugfix
Issue & Discussion References
Summary
This PR addresses a build crash caused by heading IDs colliding with
Object.prototypebuilt-in properties likeconstructor.When an author uses
{{constructor}}, the(id in file.data.headings)check natively evaluates totrue, causing the plugin to crash because it tries to map a native JS function as a remark AST Node.The problem is resolved by explicitly using a null-prototype map via
Object.create(null)inremark-headings.jsand changing presence checks inremark-reference-links.jsto strictly useObject.prototype.hasOwnProperty.call. I have also added testing to confirm standard headers and colliding headers (likeconstructor) map cleanly without triggering ReferenceLinkErrors.Does this PR introduce a breaking change?
No breaking change.