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

Read in static files into `collection.files` as `StaticFile`s. #2737

Merged
merged 6 commits into from Aug 13, 2014

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Aug 12, 2014

Fixes #2555.

@@ -214,7 +206,7 @@ def read(opts = {})
unless defaults.empty?
@data = defaults
end
@content = File.open(path, "rb", merged_file_read_opts(opts)) { |f| f.read }
@content = File.read(path, merged_file_read_opts(opts))

This comment has been minimized.

@parkr

parkr Aug 12, 2014

Member

@alfredxing This reverts your change from before. Why did you go with one and not the other? Aren't they the same?

@parkr

parkr Aug 12, 2014

Member

@alfredxing This reverts your change from before. Why did you go with one and not the other? Aren't they the same?

This comment has been minimized.

@alfredxing

alfredxing Aug 12, 2014

Member

I'm fine with both implementations. I was actually working on this one (where static files were StaticFiles instead of Documents), but switched over to the Document one as per your comment in the PR (it was a bit simpler to implement it with Documents as well).

@alfredxing

alfredxing Aug 12, 2014

Member

I'm fine with both implementations. I was actually working on this one (where static files were StaticFiles instead of Documents), but switched over to the Document one as per your comment in the PR (it was a bit simpler to implement it with Documents as well).

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 12, 2014

Member

@mattr- I keep getting RedCarpet errors in Ruby 2.1 on Travis:

  1) Failure:
TestRedcarpet#test_: redcarpet with pygments enabled should render fenced code blocks with syntax highlighting.  [/home/travis/build/jekyll/jekyll/test/test_redcarpet.rb:35]:
<"<div class=\"highlight\"><pre><code class=\"language-ruby\" data-lang=\"ruby\"><span class=\"nb\">puts</span> <span class=\"s2\">&quot;Hello world&quot;</span>\n</code></pre></div>"> expected but was
<"">.
Member

parkr commented Aug 12, 2014

@mattr- I keep getting RedCarpet errors in Ruby 2.1 on Travis:

  1) Failure:
TestRedcarpet#test_: redcarpet with pygments enabled should render fenced code blocks with syntax highlighting.  [/home/travis/build/jekyll/jekyll/test/test_redcarpet.rb:35]:
<"<div class=\"highlight\"><pre><code class=\"language-ruby\" data-lang=\"ruby\"><span class=\"nb\">puts</span> <span class=\"s2\">&quot;Hello world&quot;</span>\n</code></pre></div>"> expected but was
<"">.

@parkr parkr merged commit 315cc38 into master Aug 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@parkr parkr deleted the fix-reading-imgs branch Aug 13, 2014

parkr added a commit that referenced this pull request Aug 13, 2014

doc.read
docs << doc
else
relative_dir = File.join(relative_directory, File.dirname(file_path)).chomp("/.")

This comment has been minimized.

@benbalter

benbalter Aug 26, 2014

Contributor

@parkr Shouldn't this be full_path? Where is this path sanitized?

@benbalter

benbalter Aug 26, 2014

Contributor

@parkr Shouldn't this be full_path? Where is this path sanitized?

This comment has been minimized.

@parkr

parkr Aug 26, 2014

Member

Would you feel better if we did Jekyll.sanitized_path(relative_directory, File.dirname(file_path)).chomp("/.")? Can you create a PoC that exploits this line? The StaticFile class should handle the reading and writing path sanitation, but I can change this if you want.

@parkr

parkr Aug 26, 2014

Member

Would you feel better if we did Jekyll.sanitized_path(relative_directory, File.dirname(file_path)).chomp("/.")? Can you create a PoC that exploits this line? The StaticFile class should handle the reading and writing path sanitation, but I can change this if you want.

This comment has been minimized.

@benbalter

benbalter Aug 26, 2014

Contributor

The StaticFile class should handle the reading and writing path sanitation, but I can change this if you want.

I thought that too, but looked, and couldn't find a sanitize_path call (or am I missing something)?

@benbalter

benbalter Aug 26, 2014

Contributor

The StaticFile class should handle the reading and writing path sanitation, but I can change this if you want.

I thought that too, but looked, and couldn't find a sanitize_path call (or am I missing something)?

#
# Returns a new hash with symbolized keys
def symbolize_hash_keys(hash)
transform_keys(hash) { |key| key.to_sym rescue key }

This comment has been minimized.

@benbalter

benbalter Aug 26, 2014

Contributor

@parkr am I remembering correctly, something about not calling .to_sym on user-supplied values?

@benbalter

benbalter Aug 26, 2014

Contributor

@parkr am I remembering correctly, something about not calling .to_sym on user-supplied values?

This comment has been minimized.

@benbalter

benbalter Aug 26, 2014

Contributor

In Ruby, symbols are never garbage collected after they are created. A symbol always points to the same memory address — that’s what makes them so good for repeated hash keys. They don’t use new memory or have to be garbage collected!.

But that’s a double-edged sword. If too many symbols are created they may exahust physical memory.

http://www.sitepoint.com/rails-security-pitfalls/

@benbalter

benbalter Aug 26, 2014

Contributor

In Ruby, symbols are never garbage collected after they are created. A symbol always points to the same memory address — that’s what makes them so good for repeated hash keys. They don’t use new memory or have to be garbage collected!.

But that’s a double-edged sword. If too many symbols are created they may exahust physical memory.

http://www.sitepoint.com/rails-security-pitfalls/

This comment has been minimized.

@parkr

parkr Aug 26, 2014

Member

For a program like rails, which is long-running, it's very important to manage your memory usage. For a program like Jekyll, which runs and exits completely, all memory that was previously allocated by the Ruby Virtual Machine will be deallocated upon exit by the kernel. This is not a concern for Jekyll.

@parkr

parkr Aug 26, 2014

Member

For a program like rails, which is long-running, it's very important to manage your memory usage. For a program like Jekyll, which runs and exits completely, all memory that was previously allocated by the Ruby Virtual Machine will be deallocated upon exit by the kernel. This is not a concern for Jekyll.

parkr added a commit that referenced this pull request Aug 29, 2014

Use Jekyll.sanitized_path when adding static files to Collections.
h/t @benbalter #2737 (comment)
Not sure if the previous code can be exploited, but being super safe is never a bad thing.

parkr added a commit that referenced this pull request Aug 31, 2014

Use Jekyll.sanitized_path when adding static files to Collections.
h/t @benbalter #2737 (comment)
Not sure if the previous code can be exploited, but being super safe is never a bad thing.

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.