er314 Posted December 20, 2015 Share Posted December 20, 2015 Hi, I'm happily contributing to populating the security section . First, let me say that I highly regard Processwire for its security framework and for its security-conscious core implementation. Now, my issue : the fact that get() requests are not subject to access control, because, as stated in the selectors documentation, "if you are asking for a single specific page, we assume you mean it". -> I must say that, everytime I read this sentence, my jaw drops ... what ? PW does never make any asumption about our frontend code, but *here*, on a very sentitive *security* subject, PW does the *asumption* that my code does not need the security framework and as a result it is bypassed ?!.... the reality is, that yes I can easily imagine situations where I would not be sure that the request would target an allowed page, especially if the workflow would involve prior input from the frontend users. Ok, you can always circumvent this, by enforcing your own controls, and/or you can write your code differently in order to forbid any absolute reference to pages to reach the frontend users, but, .1. first, it's actually the aim of the CMF security framework, to give you tools which make your life easier (and less risky) when dealing with security aspects. Personnally I like to use PW access control in my design of the site resources access policy, but because of this get() behaviour I now have become very cautious EACH time I write a get(....) , thinking twice about "could it be subject to an breach scenario ?" ... and for safety, I sometimes switch to using a find() instead. .2. even in PW core, there's frontend code which makes use of absolute reference to pages : in comments workflow -> Personally, I would feel much better if, for instance, there was a "$config->accesscontrol_get = true" setting which would allow to enforce access control also on get() requests ; plus the ability to override this behaviour for a given request, just like find(). As a side note, related to absolute references to pages ; there is inherently another risk, because everything in PW is a page, so, when the frontend user has access to a single occurrence of this, it allows him to try to grab the whole site data (then comes the access control into play). For security in depth, I really don't like this, and, in my code, everywhere this would happen, I have done things differently (I even applied my method & patched the standard comments workflow . This was subject to following thread : https://processwire.com/talk/topic/10624-more-security-in-regards-to-page-id/ thanks Guillaume 2 Link to comment Share on other sites More sharing options...
LostKobrakai Posted December 20, 2015 Share Posted December 20, 2015 Having access control even for single page retrieval is quite simple. You'll need this only if there's user input in the selector anyway, which at least in my case is the exception. $page = $pages->find("…, limit=1")->first(); 1 Link to comment Share on other sites More sharing options...
WillyC Posted December 20, 2015 Share Posted December 20, 2015 alwayses.use $page->viewable() befour u starts outputtin datas from.it no matters where u.got it from. pw access checks page.hit by urls and u access checks.pages u get on yours own befores u starts throwing datas from.it. 5 Link to comment Share on other sites More sharing options...
teppo Posted December 21, 2015 Share Posted December 21, 2015 Even if I generally speaking agree with WillyC there, I must also admit that get() not checking permissions does feel a bit off to me too. Additionally it's one of the most common mistakes I've had to correct in my code reviews so far: since find() by default skips over content that the user doesn't have permission to see, developers tend to assume that get() works in the same way. I wouldn't consider it a vulnerability, but I don't think it's entirely in line with some other features either, and in my experience it's proven out to be a common source of confusion and potentially harmful mistakes. These days whenever I explain ProcessWire's API to others, I tend to include a special note about get() vs. find() from a permissions point of view Link to comment Share on other sites More sharing options...
Martijn Geerts Posted December 21, 2015 Share Posted December 21, 2015 Get behaves as a get, you must know that it is there and you know it contents. When you're not sure about content or it needs access checking, you need to search for it. You could always do a findOne() but I don't think it's entirely in line with some other features either, and in my experience it's proven out to be a common source of confusion and potentially harmful mistakes. These days whenever I explain ProcessWire's API to others, I tend to include a special note about get() vs. find() from a permissions point of view You should note about get() vs find(), but it has nothing to do with 'weird' behaviors . The whole concept makes perfectly sense and every one I explained it did understand it. Link to comment Share on other sites More sharing options...
teppo Posted December 21, 2015 Share Posted December 21, 2015 You should note about get() vs find(), but it has nothing to do with 'weird' behaviors . The whole concept makes perfectly sense and every one I explained it did understand it. Just sharing real-world observations. This is a point of confusion for some devs, including me, and thus I believe it makes sense to ask whether it's working as expected. As always, if the general consensus is that it works exactly as it should, then it probably does, and the disagreeing minority should just suck it up. Either way, when it comes to questions that involve security, I tend to gravitate towards the most obvious and fool-proof approach possible 2 Link to comment Share on other sites More sharing options...
LostKobrakai Posted December 21, 2015 Share Posted December 21, 2015 You could always do a findOne(). get() is just another name for findOne(). findOne() is the actual implementation. If access control is needed one does need to use find() or override the "FindOne" option to false. Link to comment Share on other sites More sharing options...
Martijn Geerts Posted December 21, 2015 Share Posted December 21, 2015 @LostKobrakai: findOne is returning the first of the found in the wire array it's derived from a find. https://github.com/ryancramerdesign/ProcessWire/blob/8eb350b0362c1c70003d3895c6961b3a1655ffc5/wire/core/WireArray.php#L1215 Link to comment Share on other sites More sharing options...
LostKobrakai Posted December 21, 2015 Share Posted December 21, 2015 That's the wrong file. WireArray is just about runtime selection: https://github.com/ryancramerdesign/ProcessWire/blob/master/wire/core/Pages.php#L310-L336 Link to comment Share on other sites More sharing options...
ryan Posted December 21, 2015 Share Posted December 21, 2015 When using PW's API, you should always consider your code to be operating as a kind of superuser, just like in PHP. In find() operations where you are retrieving multiple pages that you don't know exactly what they will all be, it's a sensible default to exclude "hidden", "unpublished" and no access pages, unless you request otherwise. This is there as a matter of convenience and pagination, not security. Of course you can override that (when you want to) with the "include=hidden" or "include=unpublished" or "include=all" options. Whereas with get() you are being very specific about what you want–a single specific page. Otherwise why would you get() it? Neither find() or get() are intended as a primary means of access control–we have other methods for that (see below). They are methods setup to operate in the most common use cases and keep your API calls as short and simple as possible. Just because find() filters out hidden, unpublished and no-access pages by default does not mean you should count on it for access control. That's because the pages excluded from a find() don't include runtime access control, just that which is stored in the DB. Further, the behavior of whether or not a no-access page shows up in a find() result (that isn't overridden with an include= directive) is configurable with template access settings. Granted, in most cases find()'s defaults are giving you exactly what you want in terms of excluding pages the user can't view (for your convenience), but that's not the entirely of PW's access control, which supports runtime hooks. PW's job is to provide access control for the request, but not for your own code, that's the developers job. PW intends to give you, the developer, access to everything (like you would with jQuery and the DOM), and you decide whether or not it should be available for output. PW provides you with some handy access control methods to help with this: $page->viewable(); // is the page viewable? true or false $page->viewable('field_name'); // is the field on page viewable? true or false $page->editable(); // is the page editable? true or false $page->editable('field_name'); // is the field on page editable? true or false If you want to have you own get() and find() that operate under different defaults, this is easy to do with hooks in your /site/ready.php file. For instance, this would add a $pages->one("selector"); method that automatically does a viewable() check for you and returns a NullPage() if the page is not viewable: $pages->addHook('one', function($event) { $page = $event->object->get($event->arguments(0)); if($page->id && !$page->viewable()) $page = new NullPage(); $event->return = $page; }); Would there be value in adding a method like the above to our $pages API by default? Maybe. It wouldn't benefit my own API use, because the only time I use $pages->get() is because I want to get something without conditions. But if it would be super handy for others I'm happy to add it. Here's another example, lets say you want a method in $pages, like $pages->all("selector"); that always returns all pages with "include=all" implied: $pages->addHook('all', function($event) { $event->return = $event->object->find($event->arguments[0], array('include' => 'all')); }); 4 Link to comment Share on other sites More sharing options...
teppo Posted December 21, 2015 Share Posted December 21, 2015 Whereas with get() you are being very specific about what you want–a single specific page. Otherwise why would you get() it? Use cases I've seen in the wild: Get a page at "/news/". Site admins have access to this page, and thus someone could replace a page in that URL with something entirely different, or unpublish it expecting it to disappear completely. Get a page matching user-defined search query etc. from URL-segment, GET param, or something similar. Selector is nicely sanitized, but that doesn't do much unless permissions are checked separately. The answer to "otherwise why would you get() it" is often as simple as "because I want to find one page." In other words it's used as a syntactic sugar for something like $pages->find("some-stuff, limit=1")->first(). To my best knowledge findOne() is rarely used, but there the potential for confusion is actually even bigger, just like we can see from Martijns reply above. You're absolutely right that there are no problems as long as the dev checks permissions manually. What I'm saying here is that in my experience so far this is surprisingly often unexpected behaviour. Some devs will assume that the system automatically skips over pages current user doesn't have access to. Whether or not that's a reasonable assumption is another question entirely. In a way this might be similar to "should content be escaped automatically": in my opinion it shouldn't, but numerous times I've had to remind others about properly escaping content before output – or in ProcessWire's case enabling certain field settings. Twig and other templating systems seem to gravitate towards autoescape, making output of raw content a conscious decision, which is actually a great design decision in that context. The whole get() vs. find() thing is probably just something to be more adamant about when instructing new users, but when I see a mistake like this made often enough, I tend to get worried. Obviously you can't avoid all such cases with a system as flexible and unobtrusive as ProcessWire. Link to comment Share on other sites More sharing options...
er314 Posted December 21, 2015 Author Share Posted December 21, 2015 Another example in the wild is PW's Comment submission workflow : In CommentForm.php, after retrieving the just POSTed "page_id" input parameter, you perform a get() which uses this id, then you check that the resulting page is viewable before going further. So this is correct ; during the dev, you thought about "what if this parameter is tampered by a malicious user?", you arrived to the conclusion that access control must be enforced, so you have to check that the page is viewable, check added, perfect. We are not worried about YOUR devs obviously it's more about the devs made by the rest of the world So here on this case, would this reasoning, and this additional check, had not been made by the developer -> bam, security vulnerability in the web app. To summarize, I think that what teppo & me are expecting, (well, at least me haha), in terms of developing practices & workflow/procedure, is : for finding a set of pages-> use find() unless you want to bypass the access control policy which is in place, and retrieve all results disregarding the access rights-> then use findNoSec() instead (which would be a hook, or a wrapper, or the developer having to add check_access=0 so that he's well aware that he's purposedly bypassing access control) for retrieving one single page-> use get() unless you want to bypass the access control policy which is in place, and retrieve the page disregarding the access rights-> then use getNoSec() instead (which would be a hook, or a wrapper, or the developer having to add check_access=0 so that he's well aware that he's purposedly bypassing access control) This would be possible if we had something like $config->get_is_like_find = true; But, it seems that we won't have it So, based on the previous replies, the developing practices & worklow that we should put in place, will be more like : for finding a set of pagesuse find() unless you want to bypass the access control policy which is in place, and retrieve all results disregarding the access rights-> then use findNoSec() instead (which would be a hook, or a wrapper, or the developer having to add check_access=0 so that he's well aware that he purposedly bypasses security) for retrieving one single page-> use getSec() (which would best be a hook as proposed by Ryan above, because "security by default" should be easier to do, or at least not more complex, than "security bypassed") unless you want to bypass the access control policy which is in place, and retrieve the page disregarding the access rights-> then use get() instead So ok, this will do the job, but, I hope you better understand why it still does not fully map with our approach, it's still somewhat "upside down", and prone to unwanted mistakes. Link to comment Share on other sites More sharing options...
ryan Posted December 21, 2015 Share Posted December 21, 2015 PW provides access control for the request. Once that request is successful, it's between the developer and the API what they want to do. The API is not jailed and not intended to be. If one is dealing with user input, they must sanitize and validate that input. PW works like PHP does in this respect. That means if you are going to start grabbing pages that are influenced by variables from user input (like in Teppo's scenario), then you have to sanitize that user input and validate the result, regardless of what method you've retrieved the page(s). The purpose of get() is to get a specific page that you asked for. The purpose of find() is to find pages that you don't necessarily know what they will be. With find(), since you don't know exactly what pages you'll be retrieving, filtering that result makes a lot of sense as a default behavior. But with get() you are asking for something specific that you know about ahead of time, not some unknown result as with find(). So it makes little sense to apply the same logic to get(), and it would run counter to the purpose of it. I'm not interested in changing the purpose of get(), which works exactly how it's supposed to and intended. I also don't want for there to be an implication that the API is doing everything for you in terms of access control, when that is not the intention or design of it. Where would it end? Blocking $pages->save() API calls and the like? That is not PW. The API does not run in a jailed environment by design, instead it provides dedicated methods for access control. The API is there to give you great power, and yes we expect the developer to use it responsibly. We expect that when a developer makes an API call, they know what the method is for. The get() and find() are entirely different methods with different purposes that are clearly documented. What I'm gathering from er314 and Teppo is that the concern is people using the get() method without a proper understanding of what it's for, or applying what they know about find() to the get() method, even though they are entirely different animals. I don't believe we should intentionally handicap methods to gear them towards people using them improperly. But if it's a concern or enough demand for it, I'm happy to add another method (like the "one" method I used as an example above) for that alternate purpose. Perhaps it'd be useful in a couple of the scenarios Teppo described above if the developer wants to get a page and check for access in one method call, saving a line of code. 2 Link to comment Share on other sites More sharing options...
ryan Posted December 21, 2015 Share Posted December 21, 2015 So here on this case, would this reasoning, and this additional check, had not been made by the developer -> bam, security vulnerability in the web app. What you are describing is a coding error. Any time someone uses function/method calls improperly, especially when user input is involved, they are creating a vulnerability. This has nothing to do with PW. This is the world we live in. I fully support trying to anticipate mistakes a developer might make and saving them from themselves. But a $pages->get() method with a stated purpose of retrieving a specific page unconditionally is not the place for it. I am however open to adding a different method with a different purpose. This would be possible is we had something like $config->get_is_like_find = true; But, it seems that we won't have it If we did have it, we could expect a whole lot of things to break when it is enabled. The behavior of get() is fundamental to the application logic of PW and sites/apps built upon it. Because a get_is_like_find option would make the get() method perform as something entirely different from what it is, we could expect much of the API to be broken. The intended consumer of a get() result (and most API calls for that matter) is the developer. The get() method has nothing to do with users or access, and everything to do with retrieving the specific page you asked for. You are operating on the assumption that get() is there for a different purpose, like specifically for output. Yes there may be cases where one may want to retrieve a single page and also have it bundle in some access checks, but that would be a different purpose and a different method. 2 Link to comment Share on other sites More sharing options...
er314 Posted December 21, 2015 Author Share Posted December 21, 2015 Ok, I get the point that allowing to change get() behaviour for frontend purposes would completely break PW . And I understand that get() is not the method I need for my purpose. My purpose is to give to frontend developers the ability to fully benefit from a strong underlying security feature of PW (let's call it "implicit viewable access control enforcement"), and currently by default it's not as straightforward as it could be. So while this strong underlying security feature of PW could offer by itself implicit prevention from a number of coding errors and security impacts, which would always be very welcome, currently by default it's not the case. So in a sense, the strong security framework provided by PW is not fully realized in terms on API. And I agree that my mind is not formatted the way it is expected to be (upside down), ever since the 1st time I read "if you are asking for a single specific page, we assume you mean it" , which means check_access is forced to 0 on the most often used API call. And finally I agree that the API, and your previous suggestions (thanks for the hook method, it looks really nifty & optimized), provide all the tools needed to quite efficiently make things as straightforward and realized as can be thanks Link to comment Share on other sites More sharing options...
LostKobrakai Posted December 21, 2015 Share Posted December 21, 2015 I'm not sure what exactly you mean by frontend developers, but everyone, that does have access to your php files, does essentially have full superuser access over the whole application. E.g. everyone of those can just append ", include=all" to any selector and call it a day. For anyone else it's not about how secure pw's api is, but how much access you as a developer are providing the potential user via your own interfaces, may that be an api or a website or whatever. 3 Link to comment Share on other sites More sharing options...
er314 Posted December 21, 2015 Author Share Posted December 21, 2015 I don't get your point, except that I see that you don't get mine , so I'll try to be as clear as I can : Frontend developer, is you and me, as opposed to PW core developer. I was making this distinction, as per Ryan explanation about get() having to remain as it is, unless would break PW core. So, what every frontend developer (i.e. web app developer), using PW, 1. who wants to work in "security by default" mode, 2. and who wants to take benefit of PW underlying security framework at its best, can do, is, as suggested by Ryan, * for retrieving one single page -> use getSec() (which would best be a hook as proposed by Ryan above, because "security by default" should be easier to do, or at least not more complex, than "security bypassed") : $pages->addHook('getSec', function($event) { $page = $event->object->get($event->arguments(0)); if($page->id && !$page->viewable()) $page = new NullPage(); $event->return = $page; }); - unless you want to bypass the access control policy which is in place, and retrieve the page disregarding the access rights -> then use get() instead Why bother with all of this ? -> again : - to take full advantage of the underlying security features of PW framework - to apply as many security measures as possible "by default" ; disabling a security control should be explicit, not implicit - to make use of the framework, as much as permitted, for avoiding coding errors / mistakes / unanticipated use cases & attacks scenarios This is the rationale. In a nutshell : for me, it's never use get(), unless explicitly justified ; use getSec() instead. This is what I call security by default, making full use of the security framework potential. This is how I aim to use any "software stack", and PW CMF in particular, and PW is rather well suited for this. Digression below ; feel free to tl;dr So, while I of course fully agree with "[it's about] how much access you as a developer are providing the potential user" , I don't agree at all with "it's not about how secure pw's api is" -> all the contrary. One of the main "selling point" for me, for choosing PW, is exactly because of its strong security foundations. In the current discussion thread, we are nitpicking about one particular PW API call, but PW security foundations are strong, and this is what counts, and this is what permits to very easily adapt the usage of the API to (my view of) what is the full potential of the PW security framework. Without a strong security foundation, it would mean that it's all up to the developer to 1. learn then 2. imagine and 3. handle all possible attack scenarios. This is simply *impossible*. In real life, this means "oh no we have been hacked ! it's the fault of this guy, the developer". It may be, but at some point, no, it's the responsability of the designer / decision maker. I think we can be glad that nowadays with current technologies web developers don't have anymore to handle buffer overflow issues ; nor cookie generation & handling issues, etc... And I am glad that PW provides the foundation and API for handling fine grained role-based access control (RBAC), for me it's a MUST, and PW implementation rocks. And now that, after some practice with PW, I have fully understood my need for the getSec() hook, I have nearly everything to feel confident and comfortable. Unless I'm mistaken of course. Or unless my English is too bad. Or unless both Well at least this ends my arguments on this topic cheers Link to comment Share on other sites More sharing options...
ryan Posted December 21, 2015 Share Posted December 21, 2015 My purpose is to give to frontend developers the ability to fully benefit from a strong underlying security feature of PW (let's call it "implicit viewable access control enforcement"), and currently by default it's not as straightforward as it could be. What is front-end and what is back-end? That term is just for our point of view. The code powering the rest of PW doesn't know about this, nor does it know if you are using it for an access controlled website, or some command line API tool, or something else. While your front-end page is rendered, there might be several other core and/or 3rd party modules also doing other things (because they auto loaded, or they were requested by the page), and those modules in turn might be doing several of their own $pages API calls that have absolutely nothing to do with your output, user or access control. There is a much bigger picture than just what you are doing in your template file. The built-in filtering that you have on top of $pages->find() is actually a pretty unique case in the overall API, and the phpdoc for that method outlines it as a unique case. After all, find() is primarily used for lists and pagination. You have access to everything with your API code, but your code is not the user, and the user is not your code, so be careful not to blur those lines. The API is there to assist you in getting any content you need as quickly and easily as possible, but you are the gatekeeper. It's important to keep this in mind anywhere in the API. So while this strong underlying security feature of PW could offer by itself implicit prevention from a number of coding errors and security impacts, which would always be very welcome, currently by default it's not the case. So in a sense, the strong security framework provided by PW is not fully realized in terms on API. In the more than 5 years of this open source project, I'm not aware of any instance where someone has misused a get() method and caused a security problem. So we're talking about a hypothetical scenario of someone making a code error, that to my knowledge has never occurred before, though who knows. We could construct all kinds of scenarios where a developer could misuse the power provided and get themselves in trouble. PW is a tool that when used correctly, provides very strong security and its been my primary focus in this project. But I also don't believe in magic_quotes, safe_mode or trying to save developers from themselves by introducing obstacles. These things might seem to provide more security initially, but actually create a perception that one is no longer responsible for their own security and thus become security problems themselves. I have no qualms about an access controlled "one" function per an earlier post, which could be a useful shortcut for some developers and scenarios. But I think it's important that such a method is completely distinct and also outlined as a unique case, because PW's API context is one where the site/app developer using it carries that power and responsibility. We like to access control the user, not the developer. It's good to see this Security board finally getting some discussion. 3 Link to comment Share on other sites More sharing options...
apeisa Posted December 21, 2015 Share Posted December 21, 2015 Gotta agree with Ryan here. Most of problems we have faced has been opposite: building different (admin) views for otherwise denied pages and forgetting that find filters results by default. 1 Link to comment Share on other sites More sharing options...
er314 Posted December 21, 2015 Author Share Posted December 21, 2015 mmm... again, we cross-posted. I guess my post will clear up most things, as when reading yours, I agree with everything * , so I hope my explanations about my objectives & my strategy are clear ; I can't do better. * I disagree (heavily on one point though : " In the more than 5 years of this open source project, I'm not aware of any instance where someone has misused a get() method and caused a security problem " Actually I don't understand why you say this, it mostly contradicts the rest of your purpose Anyway, let's go : without speaking specifically about PW, I can see the following main reasons for not being aware of vulnerabilities is a given web app : 1. the web app has no flaw (because very good design & development processes + developer awareness + reviews + not too big & too complex + etc... + point .2.) 2. the underlying CMF does a good job at preventing and/or assisting the designer & developer against many kinds of flaws (mostly for the syntaxic ones, less for the semantic ones) 3. the web app has not a huge traffic / not a huge potential "value" for compromise ; so apart from script kiddies -which would be stopped by .2.-, no talented attacker has the motive to try and break it 4. the web app has not been subject to a thorough security audit 5. compromise occured, but as not been detected hihi Our current discussion sits mostly in point .1. , where I'm trying to get the best out of .2. , in terms of how to adapt my processes. The fact that PW core has a strong reputation in terms of security (strong sec. framework + no known vulns) is also a strong point for another reason : it keeps script-kiddies and general lurker away from the web sites built on this platform. That being said, to make the asumption that there's no flaw, because we are not aware of any exploited in the wild... ! Well, this may be said for www.cnn.com , for your online banking app, at&t selfprovisioning, ... ok, they are constantly probed and shaked. You guess mainly from where in the planet, and it's confirmed by real world statistics. Look, if we take as example PW CommentForm.php as above. If you had forgotten the check $page->viewable() , there would be a security flaw. Detectable and exploitable, just by testing from the internet for data inputs, even without code review. I'm just taking this as a "pseudo"-real case. So, now : - Do you think, that all PW applications in the wild have all, and always, this kind of check in place where needed ? my experience (in webapps security in general, not PW related) tells me no. It's human. Some rare people are geniuses (I have the chance to tchat with one right now , but most are human, with sometimes not the right background, sometimes not being given the right priorities, sometimes make mistakes, etc... And if you don't know a single case, I cant tell you that yes, you have one : me ! Until I did a full review, I had a few issues of this kind. - Do you think this would be detected in the wild ? clearly, to detect this, it means that it's not a script-kiddie, it means that it's a real person, with his brain, whose aim it to find a flaw in the logic of this particular application. Frankly, even for processwire itself, I'm not sure that, would this check be forgotten, someone would have discovered it as of today. So, for a given custom web app made with PW, well... it depend on the traffic. If more traffic than PW, then more chances. In can happen. or not. So basically, I'm sure that yes, some web apps built with PW have security flaws. Semantic flaws, in their logic, in corner cases. Of course ! And it's not PW fault. And there would be even more flaws if PW security foundation was not what it is. I have the impression that I'm saying obvious things, no ? Actually I don't understand why we don't agree. And I don't even criticize PW in any way ; there I would have understood 1 Link to comment Share on other sites More sharing options...
ryan Posted December 22, 2015 Share Posted December 22, 2015 er314, I think your intentions are in the right place and I appreciate your determination. You are still asking for an established method with a specific purpose to have a different purpose and an access controlled front-end context, when that is not the purpose or full context of the method. So I hope you can understand why it's not realistic to just change the definition and implementation of an established method. But I get where you are coming from and think you've identified a potential helpful addition in the API, so I go ahead and add a "one" method or change the "findOne" method (in PW3) to behave as a "find just one" method with regard to access control, hidden/unpublished visibility settings, etc. While I don't personally think I will ever use this method, I think that you and Teppo have identified a couple of situations where some might find it handy. Even if one doesn't need it, perhaps just the presence of it in the API will further clarify the intentions of the methods as a whole, for folks that may have thought that the existing find() and get() were the same. I still worry a little bit about people forming an impression that PW's API is doing access control for them, when our entire API is based around providing methods for the developer to control access the way they see fit. The viewable() method is the basis of that. But we've already dipped into that territory with the find() method, and by adding the proposed method, we're not removing any control, just adding more options. So I think it's an okay. * I disagree (heavily on one point though : " In the more than 5 years of this open source project, I'm not aware of any instance where someone has misused a get() method and caused a security problem " Actually I don't understand why you say this, it mostly contradicts the rest of your purpose What I was stating is that I'm not aware of an instance where anyone has introduced security problems into their site as a result of using the get() method. Though you stated above that you did just that, so now I'm aware of an instance. You are correct that if a developer uses a $pages->get() method to retrieve a page that came from user input, and neglects to validate that the page is one you allow, then that could be a problem. But the same problem could surface anytime a developer neglects to properly sanitize or validate something–anything–that originated from user input. And this is not something you can skip regardless of what method you are using to retrieve a page or group of pages. Simply validating that a page is viewable does not mean it's valid for whatever operation you may be performing upon it. You would need to validate that it uses the expected template, comes from the expected parent, etc. So while we are adding a PW3 method to support your request, be careful not to get the impression that you no longer need to validate a page that originated from user input. Also keep in mind that a $pages->find() or proposed $pages->findOne() method that filters results is based on database-filtering, not runtime filtering. Part of PW's access control model supports runtime hooks to Page::viewable. If your situation includes any runtime access control options and pages you are loading as a result of user input, you shouldn't skip a $page->viewable() call regardless of what method you used to retrieve the page. 4 Link to comment Share on other sites More sharing options...
er314 Posted December 22, 2015 Author Share Posted December 22, 2015 hello, Thanks for your lastest explanations. To be very clear, I think our discussion has derived a little, from the original question about get() behaviour, to more general approaches about security. Personally, I don't mean to force you, or the whole PW community for that matter, to create and make use of a new API method ; as I said before, regarding the original question, I reiterate that I'm very fine with having my own "getSec" method which I would build via a hook thanks to the code your suggested at reply #10. Thanks again. I'm marking this thread as solved. 1 Link to comment Share on other sites More sharing options...
ryan Posted December 23, 2015 Share Posted December 23, 2015 Personally, I don't mean to force you, or the whole PW community for that matter, to create and make use of a new API method ; as I said before, regarding the original question, I reiterate that I'm very fine with having my own "getSec" method which I would build via a hook thanks to the code your suggested at reply #10. er314 sorry I think I misunderstood, as I thought you were suggesting we change the behavior of the existing $pages->get() method, or make it have front-end vs. back-end state (which isn't really possible since PW's API has no front-end/back-end knowledge, and the context of the API is neither). But I'm glad you started this topic because, like I said, I think you've identified a reasonable addition to the API here that should help to further clarify the purpose of the methods for any that may have confusion. And if you and Teppo had confusion about it, then certainly others will too. We've had a $pages->findOne() method in our API since the beginning, but it's just been there as an alias to $pages->get() and just for backwards compatibility. I have dropped the purpose of that method in PW3, since we no longer need that backwards compatibility, and I very much doubt anyone uses it other than the core in a few spots (which I've updated to use the get method instead). I have changed $pages->findOne() to be an alternative to $pages->get() in PW3, that by default filters in the same way as $pages->find(). You'll see this in today's commits. I think this adds more clarity because one can now assume that any $pages method that starts with "find" is also "filtered" by default. 3 Link to comment Share on other sites More sharing options...
er314 Posted December 23, 2015 Author Share Posted December 23, 2015 er314 sorry I think I misunderstood, as I thought you were suggesting we change the behavior of the existing $pages->get() method Yes, this was precisely my original intent. But once you explained that it was not possible due to the big impacts on the Core (and there is no front-end vs. back-end state as you point out) , then your hook suggestion was a good solution for my usage, and I'm happy with this. And I know that any addition to the API have to be done very cautiously, so that it remains focused and consistent. Now, having this findOne() API method seems actually excellent, because, even in terms of naming, it makes perfect sense of what it does, and it nicely complements the API. And yes I think it will really add clarity, especially for newcomers. 1 Link to comment Share on other sites More sharing options...
Recommended Posts
Create an account or sign in to comment
You need to be a member in order to leave a comment
Create an account
Sign up for a new account in our community. It's easy!
Register a new accountSign in
Already have an account? Sign in here.
Sign In Now