Check symlink outside site_source without Pathutil #9015

ashmaroli · 2022-03-28T12:07:55Z

This is a 🔨 code refactoring.

Summary

pathutil gem appears to be unmaintained.
Therefore, emulate its existing use in lib/jekyll/entry_filter.rb using Ruby's built-in methods.

Test Coverage

Relies on existing test defined as:

jekyll/test/test_entry_filter.rb

Lines 89 to 93 in e695c1e

    
           should "filter symlink pointing outside site source" do 
        
             ent1 = %w(_includes/tmp) 
        
             entries = EntryFilter.new(@site).filter(ent1) 
        
             assert_equal %w(), entries 
        
           end

parkr

LGTM, I thought of an alternative for you to consider but your original solution is likely still best.

parkr · 2022-03-28T20:56:45Z

lib/jekyll/entry_filter.rb

-      !Pathutil.new(entry).in_path?(
-        site.in_source_dir
-      )
+      !File.realpath(entry).start_with?(site.in_source_dir)


One alternative here might be to check if a constructed path and the real path are not equal?

site.in_source_dir(entry) != File.realpath(entry)

The downside to this is that it results in consumption of additional resources.
site.in_source_dir(entry) is more expensive (both in terms of allocations and cpu) than site.in_source_dir.

A better alternative to my original proposal here would be to check directly against site.source instead:

Suggested change

!File.realpath(entry).start_with?(site.in_source_dir)

!File.realpath(entry).start_with?(site.source)

But since EntryFilter#symlink_outside_site_source? method is mostly invoked only if site.safe && File.symlink?(entry) returns true, the difference between site.source and site.in_source_dir is negligible.

Sounds good.

ashmaroli · 2022-04-03T16:58:56Z

@jekyllbot: merge +dev

Ashwin Maroli: Check symlink outside site_source without Pathutil (#9015) Merge pull request 9015

Check symlink outside site_source without Pathutil

1ed146a

ashmaroli requested review from mattr- and parkr March 28, 2022 12:18

parkr approved these changes Mar 28, 2022

View reviewed changes

parkr reviewed Mar 28, 2022

View reviewed changes

jekyllbot merged commit 9c7972d into jekyll:master Apr 3, 2022

jekyllbot added fix internal labels Apr 3, 2022

jekyllbot added a commit that referenced this pull request Apr 3, 2022

Update history to reflect merge of #9015 [ci skip]

354110d

ashmaroli deleted the replace-pathutil-entry-filter branch April 3, 2022 16:59

github-actions bot pushed a commit that referenced this pull request Apr 3, 2022

Deploy docs from 9c7972d

4118af2

Ashwin Maroli: Check symlink outside site_source without Pathutil (#9015) Merge pull request 9015

jekyll locked and limited conversation to collaborators Apr 3, 2023

jekyllbot added the frozen-due-to-age label Apr 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check symlink outside site_source without Pathutil #9015

Check symlink outside site_source without Pathutil #9015

	should "filter symlink pointing outside site source" do
	ent1 = %w(_includes/tmp)
	entries = EntryFilter.new(@site).filter(ent1)
	assert_equal %w(), entries
	end

	!File.realpath(entry).start_with?(site.in_source_dir)
	!File.realpath(entry).start_with?(site.source)

Check symlink outside site_source without Pathutil #9015

Check symlink outside site_source without Pathutil #9015

Conversation

Summary

Test Coverage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment