picocms / Pico

Pico is a stupidly simple, blazing fast, flat file CMS.
http://picocms.org/
MIT License
3.82k stars 616 forks source link

[Feature Idea] Pass $_GET to Twig template #305

Closed mayamcdougall closed 7 years ago

mayamcdougall commented 8 years ago

This seems like something that could be immensely powerful in templates. I've already got two features for NotePaper that are planned around using GET variables in URLs. This is however something that Pico doesn't do at the moment.

I was able to code this with one line in the DummyPlugin, by adding $twigVariables['TwigGetUrl'] = $_GET; to the onPageRendering() function.

While I could include a very basic plugin like this in my theme, it seems like kind of a waste, especially for something that could benefit others.

Best of all, since it's just passing a variable, it doesn't interfere with or change anything in Pico. It does harmlessly pass along the page id used by the routing system, but that doesn't seem like it would really cause a problem (since most use cases would be searching for a specific item by name).

The only other issue I could think of would be that a theme developer would need to use the rewrite_url variable to determine whether to use a ? or an & in their code (if they want to support both Rewriting and the Routing System anyway).

What do you guys think about this idea? I'd love to see this as a feature, and it seems simple enough to do.

mayamcdougall commented 8 years ago

https://github.com/picocms/Pico/commit/0c85d708204c72fc80dcfc07344036eaf10e4d08 Well that's certainly more than one line. I take it you like the idea? Yours is certainly more elegant than the ? and '?' ~ current_page.id ~ '&' I'm using right now. :wink:

Not sure how you plan to handle the reading of query data (most of your code is over my head), but it'd be neat to keep it in array format. With the example above that I'm using, I have access to various bits of the query using TwigGetUrl.queryitem. ?page=5 is accessable as TwigGetUrl.page.

Anyway, looking forward to seeing how this comes out. :smile:

PhrozenByte commented 8 years ago

It wasn't intended that adding custom query data to an URL wasn't supported, so I've added a sane way to do this in 0c85d70. We probably will never support reading query data generically and without plugins, as this would introduce serious security issues ($twigVariables[...] = $_GET; unfortunately is no good idea). Input validation is hard - even for experienced programmers. It's nearly impossible with Twig. I'll reason this in detail when I have time for it, we can discuss the advantages and disadvantages after that. Sorry :unamused:

mayamcdougall commented 8 years ago

:disappointed:

I look forward to discussing it more. Just gonna dump some thoughts here while I'm thinking of them. I know you're busy, so don't feel like you have to read or respond to this today, I'm not expecting you to. :wink:

I'm no security expert, or even a php programmer, but here's how I see it:

1) Pico is already using query strings for its routing system, meaning any way a malformed or exploited url could break Pico's PHP already exists (and is presumably already protected against).

2) Since this variable would pass through Pico without being modified, there is no way for it to interact with PHP at this stage, therefore no PHP security concerns (for the moment).

3) My assumption about your security concerns is that they therefore must occur in the processing of Twig. A malformed or exploited url could break Twig. At this point though, it seems like any breaking would be occurring outside of Pico, and therefore not really Pico's concern. Not a great way to look at it, but I wanted to throw that out there.

4) So at this stage, responsibility would seem to fall on the theme developer and/or Twig itself. I'd argue that any input validation would be the responsibility of the theme developer, since only they would know what kind of value to check for. If their theme breaks because they didn't validate it, then that was their fault and nothing Pico did wrong.

5) Any breaking that occurs because of a malformed or exploited url within Twig should be extremely minor. This would most likely just cause a conditional statement to return incorrectly or something. Depends on how the theme developer was using the variable in the first place. I just dumped a whole bunch of crap into a query string and couldn't manage to break my own implementation.

?page=3*2"5(%00g77+3 config.NotePaper.toc.text config.site_title site_title {{ site_title }}

Using this value, not only did Twig print the value on the page properly (minus the %00, which got correctly interpreted as null and thus didn't display), but it didn't even break my math code. Twig's math apparently ignored everything after the initial "3" and all of my links and page rendering continued to work as if it read ?page=3. Apparently, Twig is already really good at handling and escaping information.

6) Finally, if there is a way to severely break Twig using a malformed or exploited url (maybe one containing PHP code), this would indicate a huge security issue in Twig that should be fixed. Again though, not an issue with Pico.

Anyway, didn't mean to rant. Apparently I had a lot more to say than I thought. Again, this is just my perspective on the matter, and as my knowledge of how PHP, Pico, and Twig work internally is quite limited, I fully expect to be wrong on a few of those points.

I do at least understand why non-validated user input could be a problem, and that it's usually how most exploits occur to begin with. :wink:

PhrozenByte commented 8 years ago

Just a note/hint to myself for later elaboration of a possible solution: Allow registration of scalar query string parameters.

mayamcdougall commented 8 years ago

So in relation to this, I now have a third feature I've written for NotePaper that utilizes this, a Tag Cloud.

I'll get back to that in a moment though. The first two features are a custom implementation of pagination (which excludes any "Widget" pages), and a rudimentary search feature (which searches page titles, descriptions, and content for a query). Both pretty basic tasks that don't involve any complex data.

Anyway, so the newest feature I created is a Tag system, which can parse a meta.tags variable and create both a list and "cloud" of tags for your page. The list is mostly uninteresting, but the cloud is pretty cool. It makes the tags different sizes based on the number of occurrences. It's my attempt at recreating the common "Tag Cloud" widgets that different blogging platforms usually offer.

This feature on its own works fine without reading query data, but it loses its interactiveness. When it prints the tags on the page, they are links ending with "?tag=tagname". Clicking them basically mimics the search feature but checking against meta.tags instead of the name/description/content.

Now that I've rambled on about that... I did actually have a point. :unamused:

I wanted to offer the Tag list/cloud code for the cookbook if you think others would find it interesting. It's somewhat long however (about 100 lines), and still has some limitations. For instance, tags can't yet contain spaces, because it breaks the query string and fails html validation, but it should be easy enough to substitute the spaces when I get around to it.

So anyway, if you think it's interesting (despite being non-interactive without a query plugin), I'd be happy to clean it up and document it for the cookbook. If you're interested, you can have a look at the code here.

I'd still like to have that conversation about reading query data if you have time.

If you have any other suggestions for making things interactable, I'd love to hear them. I just want to be able to click on a link and have it affect the rendering of the next page using Twig.

If I were using PHP, I feel the answer would obviously be using a GET request and query data. Not having that capability is limiting my creativity. :laughing:

(Well, I guess it hasn't been that limiting, since I've gone ahead and written features around query data anyway... still, I'd like to have a "proper" way for them to work.)

PhrozenByte commented 8 years ago

I'll have a solution or at least a detailed explanation in the next few weeks, together with Pico 1.0.1 and first dev versions of the promised plugins for Pico 1.1 (see #270) :smiley: NotePaper looks great btw! :+1:

mayamcdougall commented 8 years ago

lol, I completely forgot you were making Blogging plugins. Hope I didn't write that tag cloud for nothing. :worried:

Oh well, it was fun to do, and a good practice. I think Twig's a lot more powerful than you give it credit for. :wink:

I've always enjoyed writing web pages, but I don't often have the content to fill them with. Writing a theme for Pico provides a fun solution for that problem.

In the next version, I've expanded the idea of css overrides to being outright "themes" for NotePaper. NotePaper would be more of the structure, with the appearance being flexible (since wood grain and sticky notes may not be the look most users are going for exactly).

It's grown a lot since I started it. I wish I'd had it on GitHub since version 1.0, because that file is long gone now. :disappointed:

PhrozenByte commented 8 years ago

The blogging plugins will not make your work obsolete (the plugins will be very minimalistic, "stupidly simple" applies to official plugins, too :smile:), just a small portion of it and even this doesn't mean, that you have to remove it from NotePaper - you can keep it as fallback. :smiley:

Additionally, there's no decision made whether official plugins are distributed together with Pico or not. I'll start a discussion about this in an new issue when I've formed a initial opinion about it.

mayamcdougall commented 8 years ago

Thanks for the reassurance, but I was mostly kidding. :sweat_smile:

I'm really not upset about it. I look forward to seeing what those plugins look like. :smile:

My vote would be to call them "Optional Extras". A separate download, but front and center. Also, make sure they're clearly labeled as "Official" that way users would feel confident about using them.

I forgot to mention earlier, but my "Productivity Time" should be coming back later this week so expect more activity from me. Long story short, my job usually involves a decent amount of "on call" time where my only responsibility is to be ready if I'm needed. It gives me a nice mostly-quiet productivity environment to get stuff done with only occasional interruptions. Dec - Feb has been an exception to this, but soon it should be business as usual. What better way to use free time than work on Open Source though. :wink:

PhrozenByte commented 7 years ago

This feature will be shipped with Pico 1.1, see a74db1d for implementation details. This feature has furthermore been backported to Pico 1.0 using a (official) plugin, see PhrozenByte/pico-http-params for details. For feedback about this feature, please refer to #334. Thanks @smcdougall for bringing this up!