Repository navigation
Remove the github-pages gem dependency - #572
Conversation
This gem has a path traversal vulnerability in a transitive dependency and appears to be abandoned, so it's unlikely that it'll get fixed. So instead, we'll just take it out of our dependency graph. This requires a few other changes: - Explicitly add frontmatter to md files that should get rendered - (thus edit existing files, templates, and the crosswalk generator) - Moving the scss file (easier than fighting the theme) As an added bonus, it makes some of the implicit behavior more explicit and we don't need to handle certain links differently. Signed-off-by: Ben Cotton <bcotton@linuxfoundation.org>
lehors
left a comment
lehors
left a comment
There was a problem hiding this comment.
Nice! I wish I knew you were working on this. I just made a similar change to the AI Security Baseline and was going to submit one here.
For what it's worth I was told directly by GitHub support that they wouldn't change the github-pages gem because it would break too many sites so this is definitely the way to go.
There was a problem hiding this comment.
🤖 Multi-model AI review.
Verdict: needs changes. The site is not built from this Gemfile, so the vulnerable gem stays in production, and the theme switch drops the footer social links.
Do next
- Make
docs/Gemfilethe file that builds the site, or scope the description to the lockfile alert (inline,docs/Gemfile:6). - Restore the footer social links (inline,
docs/_config.yml:25). - Re-add the plugins the site relied on (inline,
docs/Gemfile:12). - Loosen the
jekyll-feedandjekyll-seo-tagconstraints, or every build warns because the action'sbundle checkcannot satisfy them. - Keep the
<a href>checklist links (inline,docs/index.md:19).
Questions for the author
- Is the path traversal in the description Dependabot alert #26? Naming it there helps the next reader.
- Is the goal the lockfile alert, or changing what builds baseline.openssf.org? Item 1 depends on the answer.
|
Oh, GitHub support recommended to me not using the jekyll-build-pages github action anymore and instead build the site directly using jekyll instead. It actually took me a few iterations to get it working but finally got it. You can see my updated version of web-publish.yml in the AI Security Baseline repo and if that helps I can submit the changes. |
|
Ugh, this LLM feedback is so damn verbose that it's going to take me longer to think through what it's saying than to fix the issues. |
|
@funnelfiasco sorry for the misfire there. I had it set to give all of the relevant information required for a fix, which ended up being a lot here. Trying to tidy it up now. |
- Footer - FAQ rendering - Other themey things Note: I explicitly set the URL instead of deriving it from the GitHub environment since that's the only place we'd use it and it doesn't seem worth adding another dependency just for that. Signed-off-by: Ben Cotton <bcotton@linuxfoundation.org>
Signed-off-by: Ben Cotton <bcotton@linuxfoundation.org>
d409e0c to
2ca280e
Compare
|
Okay, I think it's updated enough now. Thanks for the pointer @lehors! |
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
Trying to parse this without understanding of ruby, but a few comments about the removed dependencies.
My $0.02 is that the path traversal risk is silly for a static site generator, but I can understand not wanting to stand up a VEX pipeline to cancel this one complaint out.
| with: | ||
| source: ./docs | ||
| destination: ./_site | ||
| run: cd docs && bundle exec jekyll build |
There was a problem hiding this comment.
Do we want to use cd docs or --source ./docs? I feel like putting the built code inside the docs dir could cause problems on a (local) rebuild. (I know that I'd be likely to copy the command from here when building locally is why I ask.)
There was a problem hiding this comment.
I think it's fine? It matches what the Makefile already does and we have docs/_site in .gitignore so I don't see any issues.
There was a problem hiding this comment.
Oh, interesting! The docs/_site entry in gitignore wouldn't have been used previously.
| jekyll-optional-front-matter (0.3.2) | ||
| jekyll (>= 3.0, < 5.0) |
There was a problem hiding this comment.
If we include jekyll-optional-front-matter could we avoid needing to add a bunch of empty frontmatter to the .md files?
There was a problem hiding this comment.
We could. However, I think this simplifies things for us by clearly distinguishing between "should be rendered as HTML" and "should be kept as a markdown file".
This gem has a path traversal vulnerability in a transitive dependency and appears to be abandoned, so it's unlikely that it'll get fixed. So instead, we'll just take it out of our dependency graph. This requires a few other changes:
As an added bonus, it makes some of the implicit behavior more explicit and we don't need to handle certain links differently.