Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated test and moved from remarkHtml to rehype #51

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Conversation

mgc1194
Copy link
Contributor

@mgc1194 mgc1194 commented Apr 3, 2024

Story

DependaBot marked a few dependencies in this repo as high or critical.

This PR does:

  • Updates the node version to 18.16.0
  • Upgrades dependencies: remark-parse and unified.
  • Removes dependencies: remark-html
  • Adds the following dependencies: rehype-raw, rehype-sanitize, rehype-stringify and remark-rehype. (Note that more recent versions of the unified ecosystem are only ESM compatible)
  • Renames imported libraries to match the naming pattern used in our main repo.
  • Refactores the method markdownToHtml and markdownToSyntaxTree using rehype libraries instead of remark-html as we do in our main repo.
  • Updates tests removing a \n introduced by remak-html (We no longer use remark-html)
  • Deprecates codestudio plugin as code studio pull-through was deprecated along with Curriculum Builder.

After the update only two high-severity dependencies are left:

# npm audit report

trim  <0.0.3
Severity: high
Regular Expression Denial of Service in trim - https://github.com/advisories/GHSA-w5p7-h5w8-2hfq
fix available via `npm audit fix --force`
Will install remark-parse@11.0.0, which is a breaking change
node_modules/trim
  remark-parse  <=8.0.3
  Depends on vulnerable versions of trim
  node_modules/remark-parse

2 high severity vulnerabilities

However we don't use this library on our server side, therefore this vulnerability can't be exploited.

'xml',
];
schema.tagNames = schema.tagNames.concat(blocklyTags);

module.exports.markdownToSyntaxTree = (source, plugin = null) =>
Copy link
Member

Choose a reason for hiding this comment

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

It would be helpful to add a comment as to why we use this particular chain and that order matters (as we struggled a lot with this!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I found out how we manage the schema in our main repo and copied the same pattern.

@@ -23,12 +23,15 @@
"homepage": "https://github.com/code-dot-org/remark-plugins#readme",
Copy link
Member

Choose a reason for hiding this comment

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

Should we also bump the version, perhaps to a new major since we're making significant updates to the underlying transitives? 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not opposed to that at all.

package.json Outdated
"unist-builder": "^2.0.3"
},
"dependencies": {
"rehype-sanitize": "^4.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this was moved to dependencies as I see its only usage in test/utils.js

Copy link
Contributor Author

@mgc1194 mgc1194 Apr 4, 2024

Choose a reason for hiding this comment

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

I am not 100% sure of the difference between dependencies and devDependencies, but having rehype-sanitize in dependencies also includes the hast-util-sanitize dependency in our node-modules. I was reading this discussion

Copy link
Member

Choose a reason for hiding this comment

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

devDependencies means that we won't bundle it in our tree (only used for development). For example, prettier is not needed for the usage of this package but it is useful for the development of this package (to make it look prettier).

In this case, do we need hast-util-sanitize somewhere in this package? We can probably just add that as a dependency rather than rehype-sanitize

@mgc1194 mgc1194 merged commit 1371659 into master Apr 4, 2024
1 check passed
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.

None yet

2 participants