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.

Sign up for GitHub

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

Disable watch to prevent race condition between threads calling site.process #320

Merged
merged 23 commits into from
Mar 15, 2017
Merged

Disable watch to prevent race condition between threads calling site.process #320

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fd83a67
When updating a document or page, read item by itself
parkr Mar 9, 2017
1203e14
Use ensure_* helper methods and use this more robust method as a fall…
parkr Mar 9, 2017
9e7b78d
Use accurate method names; add the || for the page server back.
parkr Mar 11, 2017
efd6ede
Merge branch 'master' into prevent-race-condition-on-change
mertkahyaoglu Mar 11, 2017
4ce5082
centralize path manipulation methods in server.rb
benbalter Mar 13, 2017
c43c34e
slightly less abstraction
benbalter Mar 13, 2017
0c170bd
DRY up removing the site source
benbalter Mar 13, 2017
82aba50
move path helper methods to its own module
benbalter Mar 13, 2017
a573bb5
correct rubocop offenses;
benbalter Mar 13, 2017
87f8e2b
centralize file manipulation methods in file helper
benbalter Mar 14, 2017
9514901
autoload ALL THE THINGS!
benbalter Mar 14, 2017
2bf85ae
add some missing tests
benbalter Mar 14, 2017
3652c41
use FileHelper and PathHelper for DataFile methods
benbalter Mar 14, 2017
f4d94ab
add entries url to collection hash
benbalter Mar 14, 2017
1d5a49b
directory_path should be private
benbalter Mar 14, 2017
5a87555
actually, actually disable watch
benbalter Mar 14, 2017
e26934f
Use @ashmaroli's approach to monkey patching the build command to dis…
benbalter Mar 15, 2017
7d086f2
stop server before starting just in case
benbalter Mar 15, 2017
c8fab6c
-f, not -9
benbalter Mar 15, 2017
e1af381
Merge branch 'master' into prevent-race-condition-on-change
benbalter Mar 15, 2017
28c4f9c
revert changes to lib/jekyll/commands/serve.rb
benbalter Mar 15, 2017
1f8c8c2
test --watch with this plugin
ashmaroli Mar 15, 2017
bce59f7
Merge pull request #324 from ashmaroli/test-watch-admin
benbalter Mar 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/jekyll-admin/server/collection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ class Server < Sinatra::Base
end

write_file(write_path, document_body)
updated_document = collection.docs.find { |d| d.path == write_path }
updated_document = Jekyll::Document.new(write_path, {
:collection => collection, :site => site,
}).tap(&:read)
render_404 if updated_document.nil?
Copy link
Member

Choose a reason for hiding this comment

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

If we rename updated_document to document, we can use ensure_document which does the same thing. Same for ensure_page.

Copy link
Member Author

Choose a reason for hiding this comment

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

We won't be able to do this, because ensure_document calls document, which uses the buggy way of reading:

 80       def document
 81         collection.docs.find { |d| d.path == document_path }
 82       end

Copy link
Member

Choose a reason for hiding this comment

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

Isn't document overwritten by Jekyll::Document.new ? So ensure_document uses newly created document object instead of the result object of document method.

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing methods' priority is higher over objects in ruby so it will use document method anyway?

json updated_document.to_api(:include_content => true)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be document now instead of updated_document.

end
Expand Down
4 changes: 3 additions & 1 deletion lib/jekyll-admin/server/page.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ class Server < Sinatra::Base
end

write_file(write_path, page_body)
updated_page = pages.find { |p| p.path == write_path }
updated_page = Jekyll::Page.new(
site, site.source, File.dirname(write_path), File.basename(write_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the path sanitized prior to being read? (to prevent directory traversal from the user-supplied write_path?

Copy link
Member Author

@parkr parkr Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@benbalter The write_path comes from either document_path or request_path. In my opinion this should most certainly be sanitized before it gets here as it's used to interact with the filesystem before this line.

)
render_404 if updated_page.nil?
json updated_page.to_api(:include_content => true)
Copy link
Member

@mertkahyaoglu mertkahyaoglu Mar 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This needs to be page.

end
Expand Down