Custom feed path setting

[orderedlist]

Many sites already have feeds setup at a path other than feed.xml. If their host e.g., GitHub pages, doesn’t allow for redirects, changing to a new feed path isn’t really an option.

This PR adds a _config.yml option of feed_path, which defaults to feed.xml but can be set to a custom path like atom.xml.

[benbalter]

We'll need to use Jekyll.sanitized_path(@site.source, (config["feed_path"] || "feed.xml")) here. A malicious user could provide a relative path outside the Jekyll site source to expose the contents of files they otherwise wouldn't have access to. It may also be worth adding a test for this (e.g., verify a path of ../../foo becomes /foo).

[orderedlist]

Is this actually the case? This path is only used as a string in the link tag rendered into the pages, it doesn't read the file at all.

[benbalter]

Ahh. I was thinking the source template. We sanitize prior to the output destination (to prevent overwriting a file outside the site source).

@parkr is there any problem with it being passed into keep_files? Should we sanitize there to be safe?

[benbalter]

Thanks for the pull request @orderedlist. Small doc suggestion, and small security fix, otherwise, this 👀 good.

[parkr]

:)

[aroben]

Will this blow up if someone does feed: foo in their config file? In that case config["feed"] won't be a dict.

[benbalter]

Goot catch. Here's how we did it for jemoji:

 def src
      @src ||=
        if config.key?("emoji") && config["emoji"].key?("src")
          config["emoji"]["src"]
        else
          "https://assets.github.com/images/icons/"
        end
    end

[aroben]

Won't key? have the same problem? There's no String#key? method.

[parkr]

perhaps

def path
  if config.key?("feed")
    if config["feed"].is_a? Hash
      config["feed"]["path"]
    else
      config["feed"].to_s
    end
  end
end

[envygeeks]

The only thing that matters here is if this is supposed to be a hash, if it is then let it blow up because that's your fault and it's the right thing to do, if accepts any value then this matters.

[aroben]

Yeah, if the goal is only to support feed as a hash then blowing up might be a good thing. I don't know the conventions around this kind of thing in Jekyll-land which is why I raised it.

[envygeeks]

It's not up to Jekyll-Core it's up to the author of the plugin or in this case @orderedlist, if he wants a hash then that's on him tbh, Jekyll doesn't enforce like that (not that I've seen at least,) so it's entirely up to the author if he wants to blow up because he wants a hash or not, it's not up to Jekyll-Core whether he accepts both gracefully.

[orderedlist]

So the way it is now:

if config["feed"] && config["feed"]["path"]

...won't blow up unless feed.path is set and incorrect (e.g. a hash instead of a string). I'm personally fine with this, as it's well documented how it should properly be configured.

[envygeeks]

This is broken on 3.0, is this going to be fixed because we have a lot of users on 3.0 and this is a defacto-plugin since it sits under the Jekyll umbrella.

[benbalter]

This is broken on 3.0

Can you explain what you mean by that?

[envygeeks]

@benbalter https://travis-ci.org/jekyll/jekyll-feed/jobs/65270484

[envygeeks]

Actually, this failing test confuses me based on the way it's failing, I'm going to patch a local version and drop myself into a debugger and check out the state machine across all Jekyll versions because logically if it failed on one it should fail on all no?

[envygeeks]

I finally had a second to look at the tests to figure out what was going on and it turns out that the site wasn't regenerating on Jekyll 3 so it wasn't picking up what @orderedlist wanted but I also had to make another small change to the source to get the test to pass:

    let(:config) do
      Jekyll.configuration(Jekyll::Utils.deep_merge_hashes(
        overrides, {
          "feed" => {
            "path" => "atom.xml"
          }
        }
      ))
    end

That way the new config would pick up the original overrides with regeneration.

[orderedlist]

I think I've addressed all the feedback. 👾

[benbalter]

Is there any issue with adding an unsanitized, potentially relative or synmlink path to keep files? @parkr?

[parkr]

The path is sanitized later, I thought?

[benbalter]

The path is sanitized later, I thought?

keep_files can already accept arbitrary input via _config.yml, so there's no additional risk of arbitrary input here.

@orderedlist thanks for submitting the PR and for working through the feedback. WIll try to get a new release out so you can test it out in the wild. 😸

[orderedlist]

[envygeeks]

❤️

[parkr]

@orderedlist that gif tho. 😂

👍