Bug 481536 - Review Request: enano - Enano CMS, a php-based modular content management system
Review Request: enano - Enano CMS, a php-based modular content management system
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: David Nalley
Fedora Extras Quality Assurance
:
Depends On: tinymce 608575
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2009-01-25 23:55 EST by Neal Gompa
Modified: 2013-02-19 05:58 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-19 05:58:06 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Neal Gompa 2009-01-25 23:55:26 EST
Spec URL: http://kinginuyasha.enanocms.org/downloads/enano.spec
SRPM URL: http://kinginuyasha.enanocms.org/downloads/enano-1.1.6-1.20090125hg2c20563245b2.fc10.src.rpm

Description: 

Enano CMS is a content management system that strives to
have "less bloat and more float" than other solutions.
It includes powerful administrative capabilities and a
variety of plugins to add portals, forums, blogs, and
other functionalities.
Comment 1 Neal Gompa 2009-03-04 19:21:49 EST
Spec URL: http://kinginuyasha.enanocms.org/downloads/enano.spec
SRPM URL: http://kinginuyasha.enanocms.org/downloads/enano-1.1.6-2.20090301hge1ce6a91469b.fc10.src.rpm


I updated the package to the 20090301 Mercurial snapshot of Enano CMS 1.1.x, mainly because there was a huge breakage in passwords and a bunch of other changes that were made.
Comment 2 David Nalley 2009-04-23 07:35:57 EDT
Enano Review

FIX: rpmlint must be run on every package. The output should be posted in the review.
[ke4qqq@nalleyt61 rpmbuild]$ rpmlint SPECS/enano.spec 
SPECS/enano.spec: W: no-%build-section
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
[ke4qqq@nalleyt61 rpmbuild]$ rpmlint SRPMS/enano-1.1.6-2.20090301hge1ce6a91469b.fc10.src.rpm 
enano.src: W: name-repeated-in-summary Enano
enano.src: W: no-%build-section
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
[ke4qqq@nalleyt61 rpmbuild]$ rpmlint RPMS/noarch/enano-1.1.6-2.20090301hge1ce6a91469b.fc10.noarch.rpm 
enano.noarch: W: name-repeated-in-summary Enano
enano.noarch: W: incoherent-version-in-changelog 1.1.6-2 ['1.1.6-2.20090301hge1ce6a91469b.fc10', '1.1.6-2.20090301hge1ce6a91469b']
enano.noarch: E: htaccess-file /usr/share/enano/cache/.htaccess
enano.noarch: W: hidden-file-or-dir /usr/share/enano/.htaccess.new
enano.noarch: E: htaccess-file /usr/share/enano/themes/.htaccess
enano.noarch: E: non-executable-script /usr/share/enano/images/smilies/convert.sh 0644
enano.noarch: E: htaccess-file /usr/share/enano/install/.htaccess
enano.noarch: E: script-without-shebang /usr/share/enano/install/includes/cli-core.php
enano.noarch: E: htaccess-file /usr/share/enano/includes/clientside/.htaccess
enano.noarch: E: htaccess-file /usr/share/enano/files/.htaccess
enano.noarch: E: htaccess-file /usr/share/enano/images/.htaccess
enano.noarch: E: htaccess-file /usr/share/enano/files/avatars/.htaccess
1 packages and 0 specfiles checked; 9 errors, 3 warnings.


So a few comments from rpmlint:
Since you a providing a conf file for /etc/httpd/ you should push all of the .htaccess content to that and purge the .htaccess files. 
Put a comment in the build section and rpmlint will quit complaining about that. 
The rest should be relatively obvious. 

OK: The package must be named according to the Package Naming Guidelines .
Though you may find it easier to use YYYYMMDDhg or something similar and note the change number in a comment. 

OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. 
OK: The package must meet the Packaging Guidelines . 
OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
FIX: The License field in the package spec file must match the actual license.  - 
	code and readme indicate this is GPLv2+ not GPLv2
OK: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.

OK: The spec file must be written in American English. 
OK: The spec file for the package MUST be legible. 
FIX: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

Using method in spec to grab source doesn't agree with what's packaged in the SRPM
[ke4qqq@nalleyt61 tmp]$ md5sum ~/rpmbuild/SOURCES/enano-1.1.6-20090301.tar.gz 
52cdd1476c9d63238e487d05d437b724  /home/ke4qqq/rpmbuild/SOURCES/enano-1.1.6-20090301.tar.gz
[ke4qqq@nalleyt61 tmp]$ md5sum ./enano-1.1.6-20090301.tar.gz 
6450ad3e971d54680a07f69eb15ea7c6  ./enano-1.1.6-20090301.tar.gz



OK: The package MUST successfully compile and build into binary rpms on at least one primary architecture. 
NA: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. 
OK: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
NA: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
NA: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
NA: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 
OK: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
OK: A Fedora package must not list a file more than once in the spec file's %files listings. 

Though I will note that you can likely remove a number of the listings in %file by just claiming the entire directory. 

OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
OK: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK: Each package must consistently use macros. 
OK: The package must contain code, or permissable content. 
NA: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]
OK: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
NA: Header files must be in a -devel package. 
NA: Static libraries must be in a -static package. 
NA: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). 
NA: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
NA: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
NA: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
NA: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. 
NA: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK: All filenames in rpm packages must be valid UTF-8. 


I am also concerned about some of the bundled libraries in includes which are actually separate programs, and may need to be packaged separately. For instance the captcha stuff which is actually freecap. 

While this currently builds, I get a 403 (Forbidden) when I try and go to http://localhost/enanocms/ after installing.
Comment 3 Neal Gompa 2009-04-26 07:44:14 EDT
I'm looking into the issues brought up by rpmlinit, but a few things to note.

.htaccess.new and config.new.php absolutely must be there because Enano autogenerates .htaccess and config.php during secondary install (either commandline installation or web based installation)

I don't know about removing the .htaccess files, since I don't really know how to delete them as part of the build process of the package, but I have moved the data to enano.conf


The 403 error was from an accidental redundancy in the enano.conf, which is fixed in my sources now.

To fix it locally, just remove the second line in enano.conf

AFAIK, the extra libraries bundled are modified a bit to work within Enano I believe. In any case, I don't really think there will be problems keeping it all in one package.
Comment 4 Neal Gompa 2009-04-26 09:24:37 EDT
Spec URL: http://kinginuyasha.enanocms.org/downloads/enano.spec
SRPM URL:
http://kinginuyasha.enanocms.org/downloads/enano-1.1.6-4.20090415hg4babf8545826.fc10.src.rpm


I updated the package to the 20090415 Mercurial snapshot of Enano CMS 1.1.x, and I fixed a whole bunch of issues that rpmlint spewed out.
Comment 5 David Nalley 2009-04-29 22:14:17 EDT
[ke4qqq@nalleyt61 SPECS]$ rpmlint ./enano.spec 
./enano.spec: W: no-%build-section
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

You must at least have the %build section and then put a comment in that section to make rpmlint shut up. 
I know there is no real building going on - but it will make things cleaner. 

[ke4qqq@nalleyt61 SPECS]$ rpmlint ../SRPMS/enano-1.1.6-4.20090415hg4babf8545826.fc10.src.rpm 
enano.src: W: name-repeated-in-summary Enano
enano.src: W: no-%build-section
1 packages and 0 specfiles checked; 0 errors, 2 warnings.



You repeated the name of the package in the summary (and of course the no build section) Please clean that up - the description generally doesn't need to refer to the package name itself. 

[ke4qqq@nalleyt61 SPECS]$ rpmlint ../RPMS/noarch/enano-1.1.6-4.20090415hg4babf8545826.fc10.noarch.rpm 
enano.noarch: W: name-repeated-in-summary Enano
enano.noarch: W: incoherent-version-in-changelog 1.1.6-4 ['1.1.6-4.20090415hg4babf8545826.fc10', '1.1.6-4.20090415hg4babf8545826']
enano.noarch: W: hidden-file-or-dir /usr/share/enano/.htaccess.new
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

If we can fix the description and the build section I think it's good to go from that particular perspective. 

WRT to the other packages being included: 
http://fedoraproject.org/wiki/Packaging:Guidelines#Duplication_of_system_libraries

And while your package isn't Java take a look at:
https://fedoraproject.org/wiki/Packaging/Java#Pre-built_JAR_files_.2F_Other_bundled_software

I tell you this because I also have a web app that I packaged that essentially had the same problems, and I had to package up the included software that's separate. 

From my brief analysis there are at least two packages there that appear to be directly verbatim from another package.
Comment 6 Neal Gompa 2009-04-30 11:38:25 EDT
Which packages in the Enano package are verbatim from another package?

Give me a list, and I can check with upstream. If they are truly verbatim, I will start stripping them out and doing to work necessary to symlink in the required parts.

However, if they are modified, can I leave them in, or will I have to split them out into an enano-* package?
Comment 7 David Nalley 2009-05-10 13:35:53 EDT
One such package is:
http://pear.php.net/package/Text_Wiki/

Which is in the wiki engine. While you are correct in that it's not verbatim. 
They have at least deleted some files from source, but it appears that the files that came with the original package. 

For those packages with changes, the proper course of action I think is to ask upstream to submit their patches upstream. If it's more of a complete rewrite, that may be a problem. 

I have not gone through and checked each separate package for changes, but it's probably worth talking to upstream about. 

The above package would probably need to be packaged separately.
Comment 8 Neal Gompa 2009-05-25 20:15:35 EDT
Ok, I talked to upstream of Enano about the included parts, and he has told me that the components in Enano are significantly modified from the original sources.

Specifically in the case of Text_Wiki, there is a huge boatload of patches and modifications to make it work properly in Enano. Additionally, Text_Wiki has not seen a release since 2007, and upstream (Enano) is planning on replacing it (Text_Wiki) in the near future.

For now, Enano 1.1.6 final has been released, so a new spec file and SRPM are made available:

Spec URL: http://kinginuyasha.enanocms.org/downloads/enano.spec
SRPM URL: http://kinginuyasha.enanocms.org/downloads/enano-1.1.6-5.fc10.src.rpm

This one should basically fix up all the rpmlint complaints.
Comment 9 David Nalley 2009-05-26 11:47:41 EDT
So I talked to a FPC member and the gist of the conversation is: 

These libraries have to go.  In order of preference:

1) Upstream the patches, eg have upstream (enano) commit their changes to the library project in question. 
2) Fork from upstream. eg, have enano essentially fork the packages and begin maintaining a fork of each of those libraries (which will then need to be packaged)
3) Other solution....  I don't know what other solution exists to be honest, though I imagine that there exists some novel solution. 

Unfortunately a lot of this seems very much out of your hands and in the hands of upstream. Let me know how I can help you proceed.
Comment 10 Dan Fuhry 2009-05-27 00:05:14 EDT
Hi. I'm the project manager, and your contact for Enano upstream. Here is the official word on these packages and how Enano uses them.

Text_Wiki stopped being developed in 2007. Enano forked the last version and customized it heavily. It's now an integral part of Enano; the engine will not work outside of an Enano environment. For now, I honestly think it would be best to include our modified Text_Wiki with the Enano package.

We are doing some research on cutting Text_Wiki out of the picture entirely. It was really only a band-aid solution until I could get good enough with PCRE to hack up an engine myself, and now I have that level of knowledge, it's just a matter of finding the time to go through it and build a list of things to parse, design the new engine, and implement it. Patches welcome.

TinyMCE is not forked. We actually do not modify the code in their core package at all. But we do throw a few extra files, tinymce_gzip*, into the TinyMCE root. Those ARE modified, because the gzip script's caching feature has to integrate with Enano's. Otherwise, you would have cached files in two locations. From the webmaster's perspective, that is a disaster waiting to happen. In any case, tinymce_gzip.php seems to depend on being in the TinyMCE root. I can fork the hell out of that script for the sake of leaving the rest of TinyMCE alone, or we can distribute TinyMCE with the Enano package, which is probably safer anyway seeing as MoxieCode has historically been very good at breaking their API on us.

freeCap is another thing we were forced to fork. The upstream package includes a lot of overhead for deciding on a word to obfuscate into the image, but Enano already took care of that so we had to remove that. Like the TinyMCE caching, Enano has its own way of storing the captcha code and does not and cannot use what freeCap provides.

Most other third-party components are single files - small PHP classes I found on the net or borrowed from other projects because I didn't have the knowledge to write them myself. This is the case with MediaWiki's table engine, phpWiki's diff engine, etc. I'm not going to write out and get approval for packages for a couple hundred KB of code from 10+ different sources. Reusing code != one copy on the entire system.

That's the gist of things. Please take these into consideration and re-evaluate.
Comment 11 Toshio Ernie Kuratomi 2009-05-27 12:16:04 EDT
Your comments explain why you've decided to bundle the libraries in with enano.  What they do not do is explain how you've mitigated the problems with bundling libraries:

1) Bundling libraries means that whenever a security issue comes up in a library we have to first find the applications that bundle those libraries, then fix the version in that application (which could be an older version or forked from mainline and so not just a matter of applying an update).

2) We have to audit the code to find out if there are licensing issues.  With just a quick look at the code, I've found that:

* includes/captcha/engine_failsafe.php: is GPL (v2 only) (so Enano as a whole would need to be GPLv2 only, not GPLv2 or later.)

* includes/clientside/admin-menu.js: the Tigra Tree Menu should be looked at by spot/FSF.  The term "header" needs to be clarified and we need to know if this usage is in compliance.

* includes/wikiengine/Render/Plain/Prefilter.php: is licensed under the PHP license v2.0 which is GPL incompatible.  So it's use with Enano might not be okay.

* includes/graphs.php: is licensed under the PHP license v3.01 whichisGPL incompatible.  Once again, this might not be okay.

* includes/graphs.php also has this sketchy bit of text:
"""
// Graph Generator for PHP
// Originally located at http://szewo.com/php/graph, but link was broken, so this file was retrieved from:
// http://web.archive.org/web/20030130065944/szewo.com/php/graph/graph.class.php3.txt
// License unknown, however sources on the web have shown this to be either GPL or public domain.
"""
If the link is broken and the license is unknown, what leads you to think that the code is public domain?  At best some citation is missing.  At worst, this has to be removed/remaining pieces will need to be rewritten for Enano.

* There are many files that reference external files for the details of their license information, for example GPL-LICENSE.txt

Remember, this wasn't an exhaustive search -- it was only a series of quick greps to look for especially problematic files.
Comment 12 Dan Fuhry 2009-05-29 14:04:22 EDT
1. The way Enano bundles libraries, we only use parts that are very, very unlikely to result in security vulnerabilities because they are only ever given sanitized input.

2. I've checked the validity of these.

    * engine_failsafe: from the phpBB project (v2.0.21), and is GPLv2+. Clarified in comments in upstream.
    * Tigra: we got them to license it under the GPL. I will be fixing the comments in upstream shortly.
    * Prefilter: this is an upstream licensing issue for us. Prefilter is from Text_Wiki. The file is a skeleton anyway. I suspect the author labeled it wrongly. I'll just rewrite this file for Enano. See: http://hg.enanocms.org/repos/enano-1.1/file/tip/includes/wikiengine/Render/Plain/Prefilter.php
    * I'm considering removing the graph stuff. It wasn't used for very long and I was afraid of API breakage which is the only reason it's still in there. I don't know about stuff under the PHP license; from what I know (and I could very likely be wrong), it should be legal to link code under the PHP License with GPL code.
    * The failsafe graph code (with that sketchy comment) is GPL. Citation: http://google.com/codesearch/p?hl=en#bvpP-RfBwPE/sb_statsbar.php&q=%22function%20BarGraphHoriz%22
      Going by the fact that this file was released in 2006, and it points to the URL which always shows the latest version of the license, it would be very safe to assume this means GPLv2 or later. I still might remove it because it's a stale and unmaintained part of the API.

One quick question. Have you looked at licenses/index.html?
Do that and *then* tell us what our problems are. Every third party component that's been added to Enano with the exception of public domain code has been documented in that file, with copies of all relevant licenses included.
Comment 14 Toshio Ernie Kuratomi 2009-05-29 14:33:10 EDT
For security, that's not really good enough.  Sanitising input is necessary whether you bundle your libraries or not.  Bundling means that if foo.php v1.1 has an unintended flaw that allows users to access resources they shouldn't (or DOS the server or...) even without injection, and upstream foo fixes that by releasing foo-2.0 we will upgrade the foo package ASAP.  But we don't know that enano is carrying an old, insecure foo-1.1 because you didn't notice the security announcement or didn't immediately release a new enano version.  System administrators rely on us to keep their software free of security vulnerabilities.  Not bundling libraries is one way that we ensure that.

For the PHP license:
http://www.fsf.org/licensing/licenses/index_html#GPLIncompatibleLicenses
Comment 15 Dan Fuhry 2009-05-29 14:56:20 EDT
I don't think you really understood what I was communicating. What I mean by that statement is as follows.

User input is, of course, always sanitized. I'm not an idiot. What I mean is that Enano checks values that are already "trusted" (pulled from the database, or sent through validation already) a second time before sending them to third party libraries, so that any potentially unsafe input is eliminated before it could possibly touch 3rd-party code.

If you have, for example, an XSS vulnerability that can be exploited through MediaWiki's table code, we fix Enano's preprocessor to filter it out, instead of fixing MediaWiki's table code. To the outside world at large, the vulnerability is still closed; we just are extra careful to make sure trouble doesn't ever reach the vulnerable code.

I know this is controversial because it seems like a band-aid or hackish solution instead of fixing the underlying problem. That's true in a sense. Anybody that writes Enano plugins is going to read the API documentation and see that the correct way to sanitize text is with RenderMan::preprocess_text(), and the correct way to render it is RenderMan::render(), and that stuff like MediaWiki's table code is considered a private API. If developers avoid using private APIs, there won't ever be a security concern with regards to 3rd-party libraries.
Comment 16 Toshio Ernie Kuratomi 2009-05-29 15:39:13 EDT
What I'm communicating is that sanitising user input is only one part of security and it doesn't form a complete argument for why bundling libraries would be okay.  If you didn't bundle your libraries, you'd still pass the data through the Enano preprocessor before handing off to the third party libraries, right?  So there's no security advantage to bundling.  But the security advantage to unbundled libraries is that if there's a security flaw in an unbundled library, we can address that by updating a single package.  If there's a security flaw and enano, drupal, phpnuke, and wordpress all have that library bundled, then we have to find that the library exists in each of those packages, backport the fix to each of the versions each of those apps is bundling, make it work with the local modifications that you may have applied, rebuild all of those packages, and release new versions of all of those packages with a security announcement for each of those packages which our users then have to download and install on their machines.
Comment 17 Dan Fuhry 2009-05-31 12:13:11 EDT
Please excuse my confrontational disposition.

I'd like to know how MediaWiki made it into Fedora repositories, seeing as much of the code it uses is also borrowed without being separately packaged. An example of this would be phpWiki's diff engine - both Enano and MediaWiki use this same code from phpWiki 1.3.3. By the standard you are holding us to, MediaWiki would have had to split its diff engine out before it was accepted into Fedora repositories. They don't even HAVE a centralized list of 3rd-party code distributed with their package.

Like other web apps, Enano is not very much like a desktop application in the way it is designed. It is a web application. We can't just distribute it with a configure script that screams at you about dependent libraries until everything is satisfied. On the Web, libraries are so small and APIs change so quickly that bundling libraries is the only thing that makes sense. MediaWiki does it like this, so why can't Enano?

We take security problems as seriously as anyone. Our turnaround time for security releases is typically 4 hours from the time I'm alerted to it to the time the tarballs are on the Web server and announcements are circulating. Neal Gompa (King InuYasha), who built and plans to maintain the RPM, is the second-highest person in the Enano project. If there's going to be a security release, he knows about it as soon as I become aware of the vulnerability, and sometimes sooner as he's our official QA contact.

Now on to API stabilization. Web developers love to break APIs. (Yes, I'm guilty of this too.) By controlling the code ourselves, we ensure that there are not any API changes that could cause Enano to break or somehow introduce a security problem. When done properly - with proper porting and removal of unneeded components, such as is done in Enano and MediaWiki - bundling can mean that security is in fact INCREASED because only the core components involving the heavy number crunching really stay; everything else is done by Enano. Almost all GPL'ed PHP scripts are distributed as applications, not individual libraries and toolkits. This is in contrast to Java applications, which you are equating with PHP applications, but really are different in the sense that Java components are separated into libraries rather than bundled and integrated tightly with the core. This is an industry trend and not something the Enano project really has control over. We have to go with the flow in this case.

Integration is crucial too: we also have our own API that bundled libraries need to use, such as the code required to parse wikilinks and route CAPTCHA images through Enano's URL and page management code. Proper integration doesn't happen without this.

Another fun statistic: while Enano has a bunch of bundled libraries, none of them ever make SQL queries or directly parse any sort of user input. So the two biggest sources of security vulnerabilities - SQL injection and XSS - should NEVER be present in any 3rd party code. The 80:20 rule applies to security problems too: roughly 80% of security holes are caused by 20% of vulnerability types.

In summary: we bundle for two reasons:
  1) We don't want API breakage, and
  2) Web apps are different from traditional ones; libraries often do too much and need things to be stripped out in order to work correctly. We allow this because the overhead, not the core functionality, is almost always where security problems are.

If you're going to have someone review this package, it should be an experienced web developer who understands this stuff. Web apps just are not traditional desktop apps, and many web application security practices are difficult to grasp from the perspective of someone who handles security of desktop apps. They're not less secure, they're just different.
Comment 18 Toshio Ernie Kuratomi 2009-05-31 14:10:46 EDT
(In reply to comment #17)
> Please excuse my confrontational disposition.
> 
The problem with confrontational dispositions are that they leave the impression that the party being confrontational is unwilling to learn and adapt.  At that point, it's often not worthwhile to continue a conversation because no progress will be made.

> I'd like to know how MediaWiki made it into Fedora repositories, seeing as much
> of the code it uses is also borrowed without being separately packaged. An
> example of this would be phpWiki's diff engine - both Enano and MediaWiki use
> this same code from phpWiki 1.3.3. By the standard you are holding us to,
> MediaWiki would have had to split its diff engine out before it was accepted
> into Fedora repositories. They don't even HAVE a centralized list of 3rd-party
> code distributed with their package.
> 
Sorry if my quick grep for license incompatibilities confused you.  Your example falls outside the scope of the no bundled library requirement because the file is not a library.  Even though no duplication of code would lead to more security in an ideal world, there are a number of issues that prevent us from doing this reasonably.  The requirement that libraries not be bundled is a good compromise since libraries are packaged separately, released on their own timelines, and exist to be used in this way.

Since the code you point out has been copy and pasted between three applications, I'd say that it's a great candidate for spinning off into its own library so all three projects can share the maintainance burden and addition of new features.

If you can point out a place where Mediawiki is bundling a library I'll be happy to open a bug report and pursue the mediawiki packager to get that fixed.

Incompatible licensing or even problems with people including unlicensed code is a separate issue from bundled libraries as they can exist independent of each other.  However, projects that bundle libraries often have license issues as well as they stem from the same root cause: upstream not understanding all the implications of including code that they do not originate.

> Like other web apps, Enano is not very much like a desktop application in the
> way it is designed. It is a web application. We can't just distribute it with a
> configure script that screams at you about dependent libraries until everything
> is satisfied. On the Web, libraries are so small and APIs change so quickly
> that bundling libraries is the only thing that makes sense.

This is certainly a developer way of looking at things.  However, the claim that web applications are special is false.  There are web applications that distribute with scripts that check that necessary functionality is available.  There are desktop applications that have to deal with small libraries and quickly changing APIs.  We've made the decision in Fedora that convenience in this area does not trump security.  Instead we evolve solutions to the problems you face.

Sometimes we port code to newer versions of libraries for you and send the patches back to you.  In other instances we create parallel installable versions of the libraries your application depends on.  Compromises that address both API instability and security exist.

> MediaWiki does it
> like this, so why can't Enano?
> 
If mediawiki bundles libraries, I'll file a bug and either the maintainer will fix it or I'll talk with the PHP SIG about the best way to fix the issues.

> We take security problems as seriously as anyone. Our turnaround time for
> security releases is typically 4 hours from the time I'm alerted to it to the
> time the tarballs are on the Web server and announcements are circulating. Neal
> Gompa (King InuYasha), who built and plans to maintain the RPM, is the
> second-highest person in the Enano project. If there's going to be a security
> release, he knows about it as soon as I become aware of the vulnerability, and
> sometimes sooner as he's our official QA contact.
> 
This shows that you are good at reacting to security issues which is certainly good.  It doesn't show that you are good at proactively designing secure systems.  It doesn't show that you are paying attention to the security issues that are popping up in the third-party code you are bundling.  It doesn't show that you are evaluating the third party code you incorporate for security issues.

> Now on to API stabilization. Web developers love to break APIs. (Yes, I'm
> guilty of this too.) By controlling the code ourselves, we ensure that there
> are not any API changes that could cause Enano to break or somehow introduce a
> security problem. When done properly - with proper porting and removal of
> unneeded components, such as is done in Enano and MediaWiki - bundling can mean
> that security is in fact INCREASED because only the core components involving
> the heavy number crunching really stay; everything else is done by Enano.

This is only partially true but has offsetting costs as well.  The partially true part is that if you are using function A but not B from a library, it doesn't matter if you strip function B from the library or use the system version, a security flaw in function A will affect you in either case and a security flaw in function B will not affect you.

If you are removing functionality from function A to do your work, then yes, you can increase security.  But the cost is that you have to make invasive local changes to the code which takes time, could possibly introduce other security flaws into the code, and which makes backporting new fixes from the upstream for that code harder.

> Almost all GPL'ed PHP scripts are distributed as applications, not individual
> libraries and toolkits. This is in contrast to Java applications, which you are
> equating with PHP applications, but really are different in the sense that Java
> components are separated into libraries rather than bundled and integrated
> tightly with the core. This is an industry trend and not something the Enano
> project really has control over. We have to go with the flow in this case.
> 
Recently there's been a lot more dependencies on http://pear.php.net/ modules cropping up in php applications so I'd dispute your idea that this is a trend.  Perhaps, "the way applications have been coded in the past" would be more accurate.  Contributing the common code you rely on to PEAR and maintaining them there is a way that Enano can go against hte flow and help the greater PHP community as well.  Regardless, the blocker is on things that are separately bundled libraries, not things that you've copy-and-pasted out of random other applications (despite the problems that can crop up here as well).

> Integration is crucial too: we also have our own API that bundled libraries
> need to use, such as the code required to parse wikilinks and route CAPTCHA
> images through Enano's URL and page management code. Proper integration doesn't
> happen without this.
> 
> Another fun statistic: while Enano has a bunch of bundled libraries, none of
> them ever make SQL queries or directly parse any sort of user input. So the two
> biggest sources of security vulnerabilities - SQL injection and XSS - should
> NEVER be present in any 3rd party code. The 80:20 rule applies to security
> problems too: roughly 80% of security holes are caused by 20% of vulnerability
> types.
> 
> In summary: we bundle for two reasons:
>   1) We don't want API breakage, and
>   2) Web apps are different from traditional ones; libraries often do too much
> and need things to be stripped out in order to work correctly. We allow this
> because the overhead, not the core functionality, is almost always where
> security problems are.
> 
> If you're going to have someone review this package, it should be an
> experienced web developer who understands this stuff. Web apps just are not
> traditional desktop apps, and many web application security practices are
> difficult to grasp from the perspective of someone who handles security of
> desktop apps. They're not less secure, they're just different.  

If you want an experienced web application developer to review this package, I can take over the review.  But seeing as I've been explaining the problem with bundling libraries, that particular blocker would remain.
Comment 19 Dan Fuhry 2010-06-24 22:38:24 EDT
Update:
Version 1.1.7 of Enano was released in December, resolving our use of Text_Wiki. We now have our own parser code that integrates much better with the Enano.

There are other pieces of third party code, but they're not really libraries - more like a single file, or in cases like phpWiki's diff code <10 files, worth of source code. It's not that much. Have a look at:

  http://enanocms.org/licenses/

phpWiki's diff engine is 8 files, some modified some not IIRC.

MediaWiki's table engine is one file with a few helper functions that were cherry picked out of its parser code.

phpBB's Advanced Visual Confirmation Mod is one file + a couple of fonts. Modified from original version, removed all phpBB-specific code and kept only the image generation features.

Jim Tucek's e-mail encryptor is half of a file (includes/email.php). Ported to PHP from Javascript.

Tigra Tree Menu, 1 file. Modified from original version.

DOM-drag: removed in 1.1.8, 1 file, modified.

freeCap: 1 file, modified.

jQuery: 2 files, one is jquery.js, the other is jquery UI, the latter of which was built with the tool on their website (limited feature set for smaller file size)

TinyMCE: Neal is working on a package for this. The core package is unmodified for easy upgrades, but we use a modified version of tinymce_gzip to integrate with Enano's shared cache directory, which makes webmasters' (and SELinux policy writers') lives easier by not requiring a separate apache-writable directory for TinyMCE.

MD5/SHA1 in Javascript: 20K of code in 1 file.

FastJSON: 1 file out of Zend Framework. It would be absolutely ridiculous to bundle all of the Zend Framework for this one file, which we've modified to fix bugs with Unicode.

famfamfam Silk icons: bits and pieces used, compiled into sprites, resized, custom versions made, etc. Again, impractical to use the whole package (~1000 images) for ~30 icons.

As you can see, most of these are just very small pieces of bigger projects, or have been modified. Hope this clears up the issue.

-Dan
Comment 20 Neal Gompa 2010-06-28 04:01:02 EDT
I've packaged up TinyMCE[1] and TinyMCE's spellchecker plugin[2] as separate packages. Once they are accepted into the repository, I'll work with upstream to make a new package that can depend on those.

So, it is a matter of playing the waiting game right now...

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=608574
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=608575
Comment 21 David Nalley 2010-09-05 00:30:41 EDT
Neal: Sorry for my delayed responses. 
I'll try and take a look at the other packages. 
Thanks for working on this. 

Dan: I don't have the authority to change the package review guidelines, or ignore them. That said, there is an exception process that one can apply to the Fedora Enginneering Steering Committee for. I maintain packages for another CMS and recently had to request such an exception. If you and Neal want to do so, I'll be happy to help you submit the request.
Comment 22 Dan Fuhry 2011-06-02 06:47:04 EDT
As of revision 1de01205143b (http://hg.enanocms.org/repos/enano-1.1/rev/1de01205143b), TinyMCE has been completely removed from Enano.

Documentation of remaining 3rd party code (excluding public domain resources): http://hg.enanocms.org/repos/enano-1.1/raw-file/tip/licenses/index.html

As TinyMCE is the last significant piece of third-party code (see my comment above), I think we can finally go ahead and move forward with the packaging process. We can either release 1.1.8 as an RPM with TinyMCE bundled and the understanding that its removal has been committed for the next release, or release an RPM straight from trunk.
Comment 23 Miroslav Suchý 2012-12-16 08:09:57 EST
Ping? Any progress here? Or we can close this review?
Comment 24 Miroslav Suchý 2013-02-19 05:58:06 EST
Stalled Review. Closing per:
https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
If you ever want to continue with this review, please reopen or
submit new review.

Note You need to log in before you can comment on or make changes to this bug.