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

A gem-based plugin whitelist for `safe` mode #1657

Merged
merged 12 commits into from Dec 8, 2013

Conversation

Projects
None yet
10 participants
@parkr
Member

parkr commented Oct 23, 2013

I proposed a little while ago to @benbalter, @mojombo, @qrush and @mattr- the possibility of a whitelist of plugins. My thought was to bundle them all as one gem for use with Pages. It would be required in the Gemfile the Pages servers use so there wouldn't need to be any change within Jekyll.

Thinking further, and with the merge of #1557, it makes a lot of sense to allow each of these to be controlled as separate gems, with GitHub's pages-gem gem controlling the versions -- that the pages-gem would be, itself, the combining element of the whitelist.

(previous derp redacted)

@kelvinst

This comment has been minimized.

kelvinst commented Oct 23, 2013

🤘 FINALLY! PLUGINS AT PAGES! ❤️ 💙 😍

@kevingimbel

This comment has been minimized.

kevingimbel commented Oct 23, 2013

Yes! That'd make Jekyll on Github so much more comfortable.

@rigelstpierre

This comment has been minimized.

rigelstpierre commented Oct 23, 2013

👍

@benbalter

This comment has been minimized.

Contributor

benbalter commented Oct 23, 2013

So your suggestion would be monkey patch over adding some sort of "whitelist" or similar config or command line var?

Also, at least for a 0.1, having a public list of whitelisted, community-contributed plugins would be a HUGE undertaking, especially around conducing a security review of each version bump, not to mention, decision around inclusion.

Believe the feature can have uses beyond pages, but yes, the goal would be able to take the most popular X plugins and enable them, or have some functionality that better integrates Pages with the repository itself.

@smithtimmytim

This comment has been minimized.

smithtimmytim commented Oct 23, 2013

+1

@mklarmann

This comment has been minimized.

mklarmann commented Oct 24, 2013

Thats a good way to go. +1

@primalivet

This comment has been minimized.

primalivet commented Oct 25, 2013

+1

@parkr

This comment has been minimized.

Member

parkr commented Nov 5, 2013

@benbalter Any more thoughts? @mattr-?

@mattr-

This comment has been minimized.

Member

mattr- commented Nov 5, 2013

I'm on a mission to keep from adding more stuff to Site. I think this is one of those things that might be better served in a separate class and would love to hear some thoughts about that.

Other than that, I haven't given this much thought. My initial impression is that it seems a bit weird to have something in Jekyll core that's specifically directed towards a single hosting provider. What's the difference between this and only using the gems key (IIRC) in _config.yml to specify the allowed list?

@parkr

This comment has been minimized.

Member

parkr commented Nov 5, 2013

It can be extracted from Site, no prob.

The difference here is that this is a list of items that aren't restricted by safe. Using gems: in _config.yml will still be blocked on GH Pages or my company's Jekyll build server (Pages is a big one, but not the only one).

@@ -97,6 +96,22 @@ def ensure_not_in_dest
end
end
def require_gems
self.gems.each do |gem|
if whitelist.include?(gem) || !self.safe

This comment has been minimized.

@parkr

parkr Nov 16, 2013

Member

if the site is safe, then just iterate over whitelist & gems (the intersection of the two arrays), otherwise just iterate over gems. Might be wise to make a Site#allowed_gems array that handles the aforementioned condition.

@benbalter

This comment has been minimized.

Contributor

benbalter commented Nov 16, 2013

This is looking awesome. Question: I have gem X in my _config.yml gems list. Gems X,Y, and Z are whitelisted. In a second config, I have gem Y listed. I I pass jekyll build --config=_config.yml,_config2.yml is Jekyll smart enough to merge the Gem lists (resulting in X,Y) or would Y overwrite X in the current behavior?

@parkr

This comment has been minimized.

Member

parkr commented Nov 16, 2013

@benbalter So that's where it gets tricky. The best option I can think of is, in safe mode, override (i.e. Y overwrites X), and in unsafe mode, merge them (i.e. X | Y).

@benbalter

This comment has been minimized.

Contributor

benbalter commented Nov 17, 2013

Wouldn't you want them merged in both cases? If not whitelisted, they'd just be ignored, no?

@parkr

This comment has been minimized.

Member

parkr commented Nov 17, 2013

@benbalter In the case of GitHub Pages, if your overriding configuration file has whitelist: [jekyll-json, jekyll-feed] and my repo has whitelist: [i-will-hack-your-system], then I'd say you want the former, not a merge of both, no? This is the only configuration which should be overwritten (as the present implementation does) with each successive config file.

@benbalter

This comment has been minimized.

Contributor

benbalter commented Nov 17, 2013

Based on this pull, I thought whitelist was a command line arg to a YAML file. My question was about gems.

@parkr

This comment has been minimized.

Member

parkr commented Nov 17, 2013

@benbalter Whoops, sorry about that, my b. My eye skipped over the gems in "in my _config.yml gems list." – teaches me to reply to issues late at night. Yeah, we'd merge the gems but override the values of the whitelist. :)

@parkr

This comment has been minimized.

Member

parkr commented Dec 6, 2013

@mattr- let's get this merged! any comments?

@parkr

This comment has been minimized.

Member

parkr commented Dec 6, 2013

@mattr- Refactored and added tests last night. Will likely want to add a new class for plugin management but this works :)

parkr added a commit that referenced this pull request Dec 8, 2013

Merge pull request #1657 from mojombo/safe-whitelist
A gem-based plugin whitelist for `safe` mode

@parkr parkr merged commit b58cd5c into master Dec 8, 2013

@parkr parkr deleted the safe-whitelist branch Dec 8, 2013

parkr added a commit that referenced this pull request Dec 8, 2013

@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.