-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix: reduce security vulnarabilities #29
Conversation
WalkthroughThe recent updates to the project involve a significant overhaul of the resume themes and an enhancement to the server capabilities. Numerous outdated or potentially insecure themes have been removed, while new, modern themes have been introduced. Additionally, the inclusion of the Changes
Assessment against linked issues
Poem
TipsChat with CodeRabbit Bot (
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Maybe we also should add a ci script that checks for security vulnerabilities on PRs. It might be annoying for some users because the security vulnerability might just have popped up and have nothing to do with their code. But it also would make sure that people do not try to bring in (bring back) old and vulnerable themes... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- apps/registry/package.json
- pnpm-lock.yaml
Files selected for processing (1)
- apps/registry/pages/api/formatters/template.js (1 hunks)
Additional comments: 1
apps/registry/pages/api/formatters/template.js (1)
- 3-36: The removal of several themes due to security concerns is noted. Ensure that any references to these themes in the codebase or documentation have been updated to reflect their removal. Additionally, verify that the newly added "github2" theme is integrated correctly and does not introduce any security vulnerabilities.
/jsonresume-theme-class@0.1.2: | ||
resolution: {integrity: sha512-xzeDTFJek/F4GE+hE+Y7/vuEzGP7JTSORykuhawdDKbjgstXahTeASDAYt6MoTd4igrTPd8G5HAn+/zhVfV7SQ==} | ||
dependencies: | ||
handlebars: 2.0.0 | ||
dev: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a nice example. It should be possible to just use handlebars@^4
but nobody has cared to update the theme in 9 years. The alleged github repo holding the code has also gone.
Getting rid of the critical's are nice but the functions don't have any private data or authentication so pretty low risk imo. Happy to cull the themes, I am just making a mental note to fix up the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could set overrides
to force the themes to use a newer version of handlebars and only remove the ones that break.
Thank you @ObserverOfTime for the idea. This is my opinion on the matter: In principle that would work. However I could also go and make PRs to the repos of the themes, updating the |
"jsonresume-theme-msresume": "^0.1.0", | ||
"jsonresume-theme-one": "^0.0.1", | ||
"jsonresume-theme-onepage": "^0.0.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This we like. We want to fix it and bring it back.
06f3cbe
to
fe6d84b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Homepage /themes
looks a bit sad now. But truth is: Most of these themes where broken or using broken technology. What can one do? Better work on getting more themes up which a re nice and shiny, I say.
<h2>Broken - Being repaired</h2> | ||
<div class="row"> | ||
{% for theme in site.data.broken %} | ||
<div class="col-sm-4 col-md-3"> | ||
<div class="theme"> | ||
<div class="row"> | ||
<div class="col-sm-12 col-xs-6"> | ||
<a | ||
href="https://registry.jsonresume.org/thomasdavis?theme={{ theme.slug }}" | ||
> | ||
<img | ||
style="height: 129px" | ||
src="https://screenshot-peach-beta.vercel.app/api?v=1&url=https://registry.jsonresume.org/thomasdavis?theme={{ theme.slug }}&height=720&width=1280" | ||
/> | ||
</a> | ||
</div> | ||
<div class="col-sm-12 col-xs-6 meta"> | ||
<div class="name">{{ theme.name }}</div> | ||
<div class="author"> | ||
by <a href="{{theme.link}}" target="_blank">{{ theme.author }}</a> | ||
</div> | ||
</div> | ||
</div> | ||
<div class="row"> | ||
<div class="col-sm-12"> | ||
<a | ||
href="https://registry.jsonresume.org/thomasdavis?theme={{ theme.slug }}" | ||
class="btn" | ||
>Preview theme</a | ||
> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
{% endfor %} | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe in miracles. When people decide to fix their themes, I welcome them back. Until then this is just a "hm...." from the user perspective. I suggest to not talk about broken themes on the homepage.
fe6d84b
to
24805c6
Compare
fix #24 by removing packages that are using outdated and vulnerable versions of handlebars also updating packages that bring security vulnerabilities
24805c6
to
fcf47e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- apps/homepage/Gemfile.lock
- apps/homepage/_data/broken.yml
- apps/homepage/_data/themes.yaml
- apps/homepage/package.json
- apps/registry/package.json
- pnpm-lock.yaml
Files selected for processing (3)
- apps/homepage/Gemfile (1 hunks)
- apps/homepage/themes.html (2 hunks)
- apps/registry/pages/api/formatters/template.js (1 hunks)
Additional comments: 1
apps/homepage/Gemfile (1)
- 4-4: The addition of "webrick" gem is noted. Ensure that this gem is compatible with the current Ruby version and other gems, especially "github-pages". Also, verify that this addition aligns with the project's requirements for local web serving or handling HTTP requests.
<a href="https://registry.jsonresume.org/thomasdavis?theme=kendall" | ||
>https://registry.jsonresume.org/thomasdavis?theme=flat</a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text of the anchor tag still references the old theme "flat" while the href
attribute has been updated to "kendall". The text should match the href
to avoid confusion.
- >https://registry.jsonresume.org/thomasdavis?theme=flat</a
+ >https://registry.jsonresume.org/thomasdavis?theme=kendall</a
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
<a href="https://registry.jsonresume.org/thomasdavis?theme=kendall" | |
>https://registry.jsonresume.org/thomasdavis?theme=flat</a | |
<a href="https://registry.jsonresume.org/thomasdavis?theme=kendall" | |
>https://registry.jsonresume.org/thomasdavis?theme=kendall</a |
</div> | ||
</div> | ||
{% endfor %} | ||
</div> | ||
<div class="row"> | ||
<div class="clear: both;"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clear: both;
style is incorrectly placed as content within a div
. It should be within a style
attribute or a class
.
- <div class="clear: both;"></div>
+ <div style="clear: both;"></div>
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
<div class="clear: both;"></div> | |
<div style="clear: both;"></div> |
'tan-responsive': require('jsonresume-theme-tan-responsive'), | ||
techlead: require('jsonresume-theme-techlead'), | ||
verbum: require('jsonresume-theme-verbum'), | ||
wraypro: require('jsonresume-theme-wraypro'), | ||
}; | ||
|
||
const getTheme = (theme) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getTheme
function's error handling is not effective. Accessing a non-existent property on an object in JavaScript returns undefined
rather than throwing an error, so the catch block will never be executed. The function should be updated to check if the theme exists in the THEMES
object and return an appropriate error if it does not.
const getTheme = (theme) => {
- try {
- return THEMES[theme];
- } catch (e) {
- return {
- e: e.toString(),
- error: 'Theme is not supported.',
- };
- }
+ if (!THEMES.hasOwnProperty(theme)) {
+ return {
+ error: 'Theme is not supported.',
+ };
+ }
+ return THEMES[theme];
};
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const getTheme = (theme) => { | |
const getTheme = (theme) => { | |
if (!THEMES.hasOwnProperty(theme)) { | |
return { | |
error: 'Theme is not supported.', | |
}; | |
} | |
return THEMES[theme]; | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a nice observation. However totally out of scope of the PR. Just open a pull request that fixes this, if you are concerned about this. Coming in and reviewing existing code, that you are not tasked to use or change, like this is considered quite rude in most projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levino Understood, I apologize for the overreach. I'll refrain from reviewing unrelated code changes in the future. Thank you for the guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levino you know that's a bot yeah? aha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the heads up, @thomasdavis. I know that @coderabbitai is a bot. Supposedly an "intelligent" bot, as far as I can tell. I am trying things out with it. And see: It claims it would adapt and learn:
@levino Understood, I apologize for the overreach. I'll refrain from reviewing unrelated code changes in the future. Thank you for the guidance.
I find the style a bit too obedient (like with ChatGPT where it always apologizes for everything) but it says it has understood. Lets see if it remembers too!
BTW: If I had not understood that it is a bot, I would probably have asked it to approve the PR 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levino I'll make sure to remember your preferences for future interactions. If there's anything specific you'd like me to learn or any particular way you'd like me to assist, just let me know!
(\/)
( ••)
/>🥕 Have a great day!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levino aha sorry. and yeah, cool idea for an experiment.
@coderabbitai are you able to create a PR for the feedback that you originally left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomasdavis I'm unable to create pull requests or make changes to repositories. My role is to provide code suggestions and information within the context of our conversation here. If you need the changes to be made, a human contributor will need to implement them and create the pull request.
@thomasdavis Please give this an approving review so it can be merged. I reactivated the rule that all PRs need at least one approval, which might seem annoying, but it really is good practice imo. |
fix #24 by removing packages that are using outdated and vulnerable versions of handlebars also updating packages that bring security vulnerabilities
pnpm audit --audit-level critical
does not report any more critical vulnerabilities now. There are still 14 high, 10 moderate and 2 low vulnerabilities though which we might want to address too.I know that this slashing of themes seems harsh, but imo it is infeasible to execute that code based on user input on a server.
Todo:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores