Closed bricebou closed 6 years ago
bricebou
commented
6 years ago Hi,
I've worked again on my plugins and splitted the PicoSimpleStats plugin into two plugins:
PicoSimpleStats depends on PicoCookieBAR.
@PhrozenByte I was wondering if I did that right:
and then
Thanks in advance for your comments :)
tumapav
commented
6 years ago Hi, I've tried all of your plugins and checked their source code, here's my feedback:
/tag/<script>alert(0)</script> will execute the script. I always thought Twig does auto-escaping by default whenever {{ }} is used to print out variables. However, autoescaping is by default disabled in Pico CMS (see the $defaultTwigConfig in Pico source). Now the question is if it should be escaped with htmlspecialchars() in the plugin code when assigning the user input into the $twigVariables or in the template using a Twig filter. I think it's better in the template, because if someone sets autoescape: true in twig configuration in config.yml, the output of the plugin would be double-escaped if the first variant was used. Therefore I suggest you to update just the readme of your plugin and output the user input like this: {{ tag|e }}.{{ base_url }} missing in a few links in the example usages in the readme.--- delimiter for page meta sections in the example.$this->cookiebar_url will be undefined (php notice) if configuration property cookiebar_theme is missing. Would be nice if cookiebar_url had a default value so the bar will appear as soon as the plugin is installed (and will not require to specify a cookiebar_theme in the config, even as an empty value).```yaml which will then highlight the individual options, it's much easier to read and easy to distinguish configuration options from comments.I wrote everything I could think of while trying these plugins out, hope it helps. I think all of these plugins are useful, thanks for the work you put into creating them.
tumapav
commented
6 years ago As this is a thread about plugins for Pico 2.0, I'd like to share my new plugin PicoAuth (GitHub: picoauth/picoauth), which provides extensive authentication and authorization solutions for Pico CMS. A simple demo is available at https://picoauth.xyz/.
bricebou
commented
6 years ago Thanks a lot @tumapav !
PicoTags
I'll update the README with your recomandations (missing {{ base_url }} Markdown headers...)
About the XSS vulnerability risk, maybe I could add a test inside the onRequestUrl function, before checking if it starts with the tag string ?
it took me some time, and a lot of failed attempts... but here is where I've come so far:
strip_tags function within the onRequestUrl function:
https://github.com/bricebou/PicoTags/commit/49fae65481794507e782cd2a20559160c41f77f7|e filter inside Twig template this way:
<a href="{{ base_url }}/tag/{{ tag|e('url') }}">#{{ tag|e }}</a>What do you think ?
What do you think ?
PicoSimpleStats : I haven't worked yet on the composer way of doing things :-/ First, I wanted to add the cookieBAR script as a submodule or a subtree of my own plugin but when you use the zip download on GitHub, it doesn't download any of the depedencies... :-/ I'll look to composer once I've finished my theme upgrade (and it's quite long to find the right color palette)
PicoXMLSitemap and PicoSyntaxHighlighter : I've updated the README files with your suggestions.
Thanks again !
tumapav
commented
6 years ago PicoTags: The changes you made are good. Having strip_tags() in the plugin is a good addition in case someone forgets to use the |e filter in their template. I checked the README of PicoTags and it seems you forget to add it on one place, check this line:
<h2>Posts tagged <a href="{{ page.url }}">#{{ current_tag }}</a></h2>I consider this one as the most important that should be escaped, because current_tag is the only place in the plugin where a user input gets reflected in the response. On all other places the tags are written by page authors and we don't generally expect them to be malicious (but it's still good to escape it).
Also on this line page.url is undefined so the href will always be blank. Maybe you meant {{ current_page.url }}, but that wouldn't work, because tag page is not a real page in Pico. Since it's a link to the current page, you can use something like href="#" or remove the link completely.
Updates in the other plugins are good :+1:
bricebou
commented
6 years ago Thanks a lot @tumapav :-))) I've modified the README, removed the link within the h2...
I haven't look at your plugin, but I will :-) I'm working on a new plugin that will replace SyntaxHighlighter with Prism (https://prismjs.com/index.html) : https://github.com/bricebou/PicoPrism
bricebou
commented
6 years ago Hi @PhrozenByte and @tumapav !
I am thinking about creating a LinkChecker plugin for Pico, but it implies that I "create" a page only registered users would see.
So, I thought to some plugins of yours: PicoAuth and Pico Admin ; it seems to me that each plugin implements it's own authentification process.
Wouldn't it be better to have one plugin for authentification on which other plugins could depend on ? Unfortunately, I am far from being able to do that :-/
PhrozenByte
commented
6 years ago Wouldn't it be better to have one plugin for authentification on which other plugins could depend on ?
Definitly! We don't want to invent the wheel multiple times. However, we can't just use PicoAuth for this purpose. We always try to be as "stupidly simple" as possible - and this doesn't just embrace the UX, but also Pico's source code. Official plugins always just incorporate the "bare minimum" to do a job.
PicoAuth is the "definitive Pico plugin for auth" right now. It does a really great job. But it doesn't include just the "bare minimum", it includes some rather "advanced features" like OAuth, a registration form and password reset/change forms. Implementing these "advanced features" again require even more things "under the hood" - like mailing, caching and storage.
For official plugins we always take the perspective of the "most basic user" - and for editing pages online, you don't need to distinguish multiple users. Thus our goal should be to secure compatibility. For example, we might create some common API (class interface and events) and config for auth, and some sort of a PicoBasicAuth plugin implementing the "bare minimum". However, that's not all. We furthermore have to think about how to safely disable and replace other plugins (i.e. PicoAuth needs a way to "overrule"/replace PicoBasicAuth).
bricebou
commented
6 years ago Great ! :-) But I'll let @tumapav react and the two of you discuss about that (it's way above my level) :)
I've worked on PicoLeaflet : https://github.com/bricebou/PicoLeaflet. It adds Leaflet interactive maps to Pico pages ; you can see it live here: http://insta.momh.fr/
I was wondering if I can help with Pico website, updating the plugin page ? But I don't think it's a great idea to simply add files to this folder https://github.com/picocms/picocms.github.io/tree/master/_plugins Do you want to keep only Pico 2 native plugins ?
tumapav
commented
6 years ago I agree that PicoAuth does not fit the "stupidly simple" requirement when it comes to choosing a universal Pico authentication plugin. I don't mean that it isn't simple to use - all the advanced features are either pre-configured or disabled by default, but as you say, it doesn't include just the bare minimum.
However, when it comes to authentication, it could be difficult to balance the simplicity and a good authentication security. That's why I like the idea of giving a user an option to switch the authentication dependency of a plugin to an alternative to get more advanced options (or even to use other authenticators like SSO in case of PicoAuth).
I can imagine that there would be an official User interface similar to PicoAuth\User and every plugin that requires authentication would after successful authentication receive an instance of it without a need of knowing how it was obtained. This would be done either by PicoBasicAuth directly or by another official plugin that would only serve for interfacing authentication plugins. Right now I am not sure how to achieve the plugin interchangeability considering that the $dependsOn cannot be changed by the plugin configuration, because Pico sorts the plugins before the configuration is even read. (I wanted to add a similar plugin selection to PicoAuth by giving a user a configuration option to choose if PicoSession should be used for session management. I couldn't implement this because of above.) No matter how it will be actually implemented, I am interested in modifying PicoAuth to make it compliant with this Pico authentication interface.
I'd like to also ask if the structure of the plugin page on the Pico website is going to change now when Pico 2.0 is released. I would be glad if PicoAuth would be included on this list, but it requires Pico 2.0 so I'm not sure if it can just be added next to the plugins that work with 1.0. Similar question about the Pico Plugins page on the Pico wiki. Will there be a new heading just for Pico 2.0 plugins?
PhrozenByte
commented
6 years ago @tumapav
I was rather thinking about passing responsibility than replacing PicoBasicAuth by a more advanced plugin like PicoAuth. Just imagine PicoBasicAuth as some sort of "container" for auth plugins: Its main purpose is to trigger the onAuthInit event that carries $this (i.e. a instance of PicoBasicAuth). The event serves the purpose to allow other plugins to discover PicoBasicAuth. Apart from that it implements (just) the getAuthenticator() and setAuthenticator(AuthenticatorInterface $authenticator) methods. getAuthenticator() returns PicoBasicAuth's basic implementation of an authenticator by default, whereas setAuthenticator() allows advanced plugins like PicoAuth to set their own authenticator. PicoBasicAuth then simply passes responsibility to this authenticator.
Most of the auth process should be implemented using events, just like PicoSession for session handling. There should be a bunch of events in PicoBasicAuth's authenticator that must be implemented by authenticators of more advanced plugins like PicoAuth, too. Anyway, it doesn't mean that more advanced plugins can't add more events. The important thing is, that it's not the respective authenticator that receives Pico's core events, but PicoBasicAuth - and it's always PicoBasicAuth (not PicoAuth!) that passes events to the authenticator.
If a plugin just needs auth (like PicoAdmin), it must simply depend on PicoBasicAuth. If the user installs PicoAuth, it silently replaces the authenticator by a more advanced one and PicoAdmin won't ever notice any difference. Anyway, if a plugin specifically requires more advanced auth, it must simply depend on both PicoAuth and PicoBasicAuth.
As far as I remember you chose to implement a own session handler because you were worried about PicoSession not being stable (what is a good reason!). Anway, since session handling is a rather "self-contained subject", I don't think there's a reason for plugins to implement their own session handling as soon as PicoSession is final. What I'm trying to say is: IMO we don't need a general approach to replace arbitrary plugins by other plugins, auth is rather a exception here. However, maybe I just can't think of good reasons. Do I miss something?
@bricebou @tumapav
Regarding the plugins page on Pico's website:
I'd like to also ask if the structure of the plugin page on the Pico website is going to change now when Pico 2.0 is released.
I don't think that it's reasonable to remove plugins for Pico 1.0 from Pico's website as long as they work with Pico 2.0 (they should; if a plugin doesn't work with Pico 2.0, it should be removed). We might simply distinguish plugins developed for Pico 1.0 and Pico 2.0 by tags.
Same for the wiki: Plugins that aren't compatible with Pico 2.0 should be moved to a separate "Pico 1.0 only" section, everything else should be in some sort of a "current" section. The wiki is world-editable, help is appreciated! :smiley:
I was wondering if I can help with Pico website, updating the plugin page ?
Yes! :+1: You're right that we shouldn't add plugins untested, however, the approval process consists of two more or less independent tasks: (1) whether the plugin actually works and its docs are coherent, and (2) whether the source code looks fine. Help is highly appreciated! It would be really great if you find some time to check all promoted plugins whether they still work with Pico 2.0. Same for all open PRs in the picocms/picocms.github.io repo. :smiley:
stale[bot]
commented
6 years ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:
PhrozenByte
commented
6 years ago @bricebou
I finally found some time to check the plugins. Unfortunately they aren't really ready to be promoted on Pico's website:
PicoTags::tags_sorting(): Remove both array_flip() and use usort() instead of uksort(). However, I'm not sure why the tags array needs to be sorted!? :confused: I guess if one wants tag B to be listed before tag A, it means that tag B is more important. This is different for the tags overview page, the tags should be sorted there, but just there.PicoTags: Not sure what the nbcol feature is for... Doesn't make much sense to me. How about the batch Twig filter?PicoTags: Not sure what the excluded_templates is for... If a page shouldn't have tags, the tags array should be empty.PicoTags::onRequestUrl(): It should be rawurldecode() instead of urldecode(). Furthermore the strip_tags() call has no effect (must be called with the result of rawurldecode()). However, strip_tags() doesn't work proberly anyway, you must call Twig's escape filter nevertheless. Otherwise special characters like < (without >, i.e. no tag) and & will still cause problems. If forcing the escape Twig filter isn't desirable, simply use a character whitelist for tag names, like [a-zA-Z0-9_\.\-].PicoTags: There are various calls to explode(), even though the contents are YAML. If the plugin expects an array, the user should declare an array. Don't use separators like | or ,.PicoTags::onPagesLoaded(): The $pages array looses its keys. This is a absolute no-go!PicoTags::onPagesLoaded(): You might want to implement $this->ptags_delunique using array_filter()PicoTags::onPagesLoaded(): Not sure what the else clause labelled as "Workaround" is for...PicoSimpleStats: This really isn't something for a plugin... This should be implemented in the theme. The valid use cases for modifying $output in onPageRendered() are very, very limited.PicoXMLSitemap: See Pico's official https://github.com/PhrozenByte/pico-robotsPicoSyntaxHighlighter: Same as with PicoSimpleStatsPicoCookieBAR: Same as with PicoSimpleStatsPicoLeaflet: The source code is very confusing... Not sure whether this is like PicoSimpleStats and should be implemented using pure Twig.stale[bot]
commented
6 years ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:
tumapav
commented
6 years ago We don't need a general approach to replace arbitrary plugins by other plugins, auth is rather a exception here. However, maybe I just can't think of good reasons. Do I miss something?
That's right. The method of passing responsibility to auth plugins by using events is good and probably even better than the one with replacing plugins I was thinking about, because in your solution the change to a different plugin happens automatically as soon the plugin is installed, which makes it easier and doesn't require an additional configuration. I'll be happy to update PicoAuth to use the mentioned interface in the future when the specification of AuthenticatorInterface and the required events are determined.
I also agree with you that there is no need for plugins to implement their own session handling when PicoSession is released. I already have the implementation of PicoAuth\Session\SessionInterface which uses the current PicoSession ready, so I will be able to make this change very quickly as soon as PicoSession is ready.
Now I've added PicoAuth plugin to the list of plugins in the wiki. I assumed the "current" section you mentioned will be the one currently labeled "Plugins for Pico 1.0+". I couldn't figure out how is the list ordered, so I've put it next to Pico Users.
bricebou
commented
6 years ago @PhrozenByte : thanks a lot for your review ! First, I'm not a developer and I've worked on this plugin essentially for fun. I read the docs, try a lot of things, test a lot what I get, but I don't understand everytime anything...
PicoTags::tags_sorting(): Remove both array_flip() and use usort() instead of uksort(). However, I'm not sure why the tags array needs to be sorted!? confused I guess if one wants tag B to be listed before tag A, it means that tag B is more important. This is different for the tags overview page, the tags should be sorted there, but just there.
Hum... I made tag sorting an option so you can disable it. When I add tags to a blog post, I write them as they come in mind, and I'm not sorting them based on their importance...
I've used arry_flip() and uksort() because I'm french and I need to sort tags with accents, diacritics... When I first work on this plugin, three years ago, that was the only way to make the sort work.
PicoTags: Not sure what the nbcol feature is for... Doesn't make much sense to me. How about the batch Twig filter?
I didn't know about that Twig filter. And you are right, it wasn't useful, it was more like a test I've made.
PicoTags: Not sure what the excluded_templates is for... If a page shouldn't have tags, the tags array should be empty.
You are right ! I'm still thinking as if Pico was in it's 0.9 version, when you have to declare the meta to use in your content headers.
PicoTags::onRequestUrl(): It should be rawurldecode() instead of urldecode(). Furthermore the strip_tags() call has no effect (must be called with the result of rawurldecode()). However, striptags() doesn't work proberly anyway, you must call Twig's escape filter nevertheless. Otherwise special characters like < (without >, i.e. no tag) and & will still cause problems. If forcing the escape Twig filter isn't desirable, simply use a character whitelist for tag names, like [a-zA-Z0-9.-].
I'm not sure how to handle that : I've made several tests with rawurldecode() but it wasn't working well with spaces. The better result was achieved with urldecode()... I like the character whitelist idea but I'm far from understanding how to implement that...
PicoTags: There are various calls to explode(), even though the contents are YAML. If the plugin expects an array, the user should declare an array. Don't use separators like | or ,.
Of course... Still using Pico 0.9 way of thinking... :-(
PicoTags::onPagesLoaded(): The $pages array looses its keys. This is a absolute no-go!
I've encountered an issue with that : https://github.com/picocms/Pico/issues/444#issuecomment-401586254
But I don't really know how to handle that... Can you point me to the right way to solve that issue ?
PicoTags::onPagesLoaded(): You might want to implement $this->ptags_delunique using array_filter()
I'll check that.
PicoTags::onPagesLoaded(): Not sure what the else clause labelled as "Workaround" is for...It's from the Dan Reeves' first version of the plugin. As I've said I'm far from being a coder... so I don't always understand what I'm doing :-/
PicoSimpleStats: This really isn't something for a plugin... This should be implemented in the theme. The valid use cases for modifying $output in onPageRendered() are very, very limited. PicoXMLSitemap: See Pico's official https://github.com/PhrozenByte/pico-robots PicoSyntaxHighlighter: Same as with PicoSimpleStats PicoCookieBAR: Same as with PicoSimpleStats PicoLeaflet: The source code is very confusing... Not sure whether this is like PicoSimpleStats and should be implemented using pure Twig.
All the other plugins were more like experiments. I'll check your pico-robots and I'll work much more with Twig.
Thanks again.
stale[bot]
commented
6 years ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:
PhrozenByte
commented
6 years ago @bricebou
I've used arry_flip() and uksort() because I'm french and I need to sort tags with accents, diacritics...
PicoTags::tags_sorting(): That's not the problem. Simply replace the whole method by usort($array, 'PicoTags::wd_unaccent_compare_ci'); (i.e. remove both array_flip() calls). It does absolutely the same as uksort() and a respective array_flip() call just before and after it, and saves the array_flip() calls.
I like the character whitelist idea but I'm far from understanding how to implement that...
PicoTags::onRequestUrl(): Try something like
if ($this->is_tag) {
$this->current_tag = preg_replace('/[^a-zA-Z0-9_\.\-]/', '', substr($url, 4));
}I've encountered an issue with that : https://github.com/picocms/Pico/issues/444#issuecomment-401586254
PicoTags::onPagesLoaded(): You must use foreach ($pages as $id => $page) { } and add items to your $new_pages array by using $new_pages[$id] = $page;
It's from the Dan Reeves' first version of the plugin. As I've said I'm far from being a coder... so I don't always understand what I'm doing :-/
PicoTags::onPagesLoaded(): So, you should still look into it and if you can't see how it makes any sense, remove it :wink:
stale[bot]
commented
6 years ago This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:
Hi,
As @PhrozenByte has seen it (and even suffers from it... ^^), I've worked on several plugins, existing or not, for the version 2 of Pico. They are available on GitHub but before declaring them in Pico's documentation, I think it would be great if they could be reviewed by someone who isn't a hobbyist coder as I am...
Here are the plugins:
Thanks in advance for your comments!