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.

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

Fixes full path leak to source directory when using include tag #1951

Merged
merged 2 commits into from Jan 22, 2014

Conversation

Projects
None yet
5 participants
@jens-na
Copy link

jens-na commented Jan 14, 2014

Fixes #1950.
Uses the base name for the error output.

/cc @benbalter

@benbalter

This comment has been minimized.

Copy link
Contributor

benbalter commented Jan 14, 2014

To stay consistent with other errors, shouldn't it be the full path relative to the site source?

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 14, 2014

To stay consistent with other errors, shouldn't it be the full path relative to the site source?

👍 to that. @jens-na, would you please update your pull request?

@jens-na

This comment has been minimized.

Copy link
Author

jens-na commented Jan 14, 2014

Sure @parkr. this will output something like this:

Liquid Exception: Included file '_includes/foo' not found in _layouts/post.html
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 14, 2014

@jens-na Could probably also remove _includes. What do you think?

@jens-na

This comment has been minimized.

Copy link
Author

jens-na commented Jan 15, 2014

Makes sense. The error message says Inlucded file [...]. All files which are included somewhere resides in the directory _includes/.

On the other hand if all the other errors are relative to the source directory it should be _includes/foo to be consistent.

I don't know all error messages of Jekyll but I have found another one which outputs the filename relative to the source directory:

Liquid Exception: Unknown tag 'bar' in _posts/2013-12-07-test-file.html/#excerpt

Personally I like the output of _includes/foo.html or _posts/bar.html (relative to the source dir) because it gives the user an additional hint where to find the file - this can be very useful for unexperienced users that don't know these directories (_includes/, _posts) yet.

@benbalter

This comment has been minimized.

Copy link
Contributor

benbalter commented Jan 15, 2014

Personally I like the output of _includes/foo.html or _posts/bar.html (relative to the source dir) because it gives the user an additional hint where to find the file - this can be very useful for unexperienced users that don't know these directories (_includes/, _posts) yet.

Makes sense to me. Thanks for taking the time to put this patch together.

@@ -122,11 +122,12 @@ def validate_dir(dir, safe)
end
end

def validate_file(file, safe)
def validate_file(sourcedir, file, safe)
relative_file = file.sub(sourcedir, '')[1..-1]

This comment has been minimized.

@parkr

parkr Jan 15, 2014

Member

Any reason you chose not to go with Pathname#relative_path_from here? This feels a bit like a hack and like we're relying on a heuristic. If it's possible to avoid string manipulation like this, I think it's worth a try. :)

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 15, 2014

@jens-na Great work! Thanks for doing this so quickly, too. Would you mind writing tests for this error message?

@jens-na

This comment has been minimized.

Copy link
Author

jens-na commented Jan 15, 2014

Updated.
These tests passes on my machine. Have I done something wrong here?

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 15, 2014

@jens-na You might have an old version of master. Can you merge/rebase on jekyll:master?

@jens-na

This comment has been minimized.

Copy link
Author

jens-na commented Jan 15, 2014

I thought I have done this before:

$ git remote -v
origin  git@github.com:jens-na/jekyll.git (fetch)
origin  git@github.com:jens-na/jekyll.git (push)
upstream    https://github.com/jekyll/jekyll.git (fetch)
upstream    https://github.com/jekyll/jekyll.git (push)

and

$ git merge upstream/master
Already up-to-date.

Another pull request fails with the same errors.

@jens-na

This comment has been minimized.

Copy link
Author

jens-na commented Jan 16, 2014

I reran the test!

<-- resolved the issue (#1958)

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jan 16, 2014

👍 LGTM! Thanks for your hard work, @jens-na.

@mattr-, thoughts?

@ghost ghost assigned mattr- Jan 16, 2014

parkr added a commit that referenced this pull request Jan 22, 2014

@parkr parkr merged commit 05df50f into jekyll:master Jan 22, 2014

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Jan 22, 2014

@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.
You can’t perform that action at this time.