Do not coerce layout paths in theme-gem to the source directory #6603
Conversation
features/incremental_rebuild.feature
Outdated
| Scenario: A themed-site and incremental regeneration | ||
| Given I have a configuration file with "theme" set to "test-theme" | ||
| And I have an "index.md" page that contains "Themed site" | ||
| When I run jekyll build -IV |
There was a problem hiding this comment.
Can you use the full flag names here for readability, --incremental, --verbose?
lib/jekyll/regenerator.rb
Outdated
| @@ -26,6 +26,7 @@ def regenerate?(document) | |||
| when Document | |||
| regenerate_document?(document) | |||
| else | |||
| return false if document.is_a?(String) && file_in_theme_dir?(document) | |||
There was a problem hiding this comment.
Where do we call regenerate? with a String?
There was a problem hiding this comment.
If you debug right here using puts document.inspect calls, you'll see that some of the dependencies (on the first build with -I flag) are just regular paths to files in the theme-gem
|
I tried this branch with @mmistakes theme and the build time is way faster: BeforeRegeneration time is almost the same as the initial build time: After |
|
CI Tests are passing, a quick benchmark shows a significant improvement (should benefit all /cc @benbalter |
lib/jekyll/renderer.rb
Outdated
| site.in_source_dir(document.path), | ||
| site.in_source_dir(layout.path) | ||
| ) | ||
| site.regenerator.add_dependency(document.path, layout.path) |
There was a problem hiding this comment.
I'm worried that this change could introduce security issues because we're not being explicit about files being in the site's source directory. Do we have tests that indicate that document and layout objects can only exist within the sandbox that is a jekyll site?
There was a problem hiding this comment.
I agree with your concerns regarding security since this change removes the existing sanitization..
While document here refers to any renderable object (Jekyll::Document, Jekyll::Excerpt, Jekyll::Page, Jekyll::PageWithoutAFile) which can be manually created by a plugin and pushed into the site payload via the :pre_render hooks, this method here simply considers adding the concerned paths to the metadata file when required..
Layouts can exist both in the site.source and in site.theme.root elsewhere. So coercing a layout.path to be strictly within the site.source just here is not right.
Lines 38 to 44 in 999151d
I can coerce the document.path to be within the site.source and not affect this patch at all since the document.path is simply a key in the metadata hash..
while a layout object (a dependency) can point to a file within the source directory or within the theme-gem, other renderable objects can be coerced to within the source directory.
9274ed7 to
98993f9
Compare
The patch changed considerably since last review
|
one more review for this, and we can ship it in 3.7.0! |
|
@jekyllbot: merge +minor |
Resolves #6241
Resolves #6332
Do not coerce layout paths in theme-gem to the source directory.
/cc @jekyll/build