Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs missing from latest releases #919

Closed
orlitzky opened this issue Dec 26, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@orlitzky
Copy link
Contributor

commented Dec 26, 2016

The last few versions (5.6.17, 5.2.18, and 5.2.19) are missing some documentation that used to be part of the tarball:

  • README.md
  • changelog.md
  • The docs/ directory

The SECURITY.md file is new, but it would also be nice to have. We install all of these as part of the Gentoo package. I'll drop them for now so that we can get the CVE fix, but can you please bring them back? =)

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Dec 26, 2016

dev-php/PHPMailer: new version to fix CVE-2016-10033.
The new version fixes a critical security vulnerability, and so it
replaces the existing (unstable) version. The release tarball for the
latest version is missing some documentation. I reported that
upstream, and commented those docs out of the ebuild for now so that
we can get the CVE fix as soon as possible.

Gentoo-Bug: 603750
PHPMailer-Bug: PHPMailer/PHPMailer#919

Package-Manager: portage-2.3.0
@SDKiller

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2016

@orlitzky

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2016

It looks like that's the commit that removed them. They should be fixing composer bugs in composer, not in every upstream package =P

@Synchro

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

This is slightly awkward - in the past we've had complaints about the install bundle including too many files, plus it's considered an information leak during pentests, so we added that .gitattributes file to skip those files when PHPMailer is deployed via composer (though this change isn't specific to composer). Unfortunately, GitHub uses these same properties when building a tarball. None of those files are necessary for a functional deployment, and you can always get them via a git clone or by browsing the site. There was a later change that brought back the examples folder, but that will be going again in 6.0. The docs folder is effectively going away altogether in 6.0 too. The docs and examples could be moved to a separate project which would make this "missing files" problem more transparent.

@Synchro

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

I wouldn't call this a composer bug.

@orlitzky

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2016

Just one more thing broken for everyone, for the sake of composer, then. Ain't nobody got time for this.

@orlitzky orlitzky closed this Dec 26, 2016

@Synchro

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

Not at all. We had complaints about this long before composer existed; composer is merely making use of a standard git feature - the very same one that github uses. Are you arguing that a functional deployment should include all documentation and example code, even if it represents a security risk? Case in point - the omission of these unnecessary files in deployments may help to mitigate the nasty RCE that cropped up this weekend.

@orlitzky

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2016

Are you arguing that a functional deployment should include all documentation and example code, even if it represents a security risk?

Of course not. Composer is the only tool that's stupid enough to put the documentation along with all the other source code in the public document root of your project. It doesn't belong there in the first place. We install it to /usr/share/doc, because our package manager isn't stupid. We also install the source code in /usr/share/php (outside of the public web root), because our package manager isn't stupid. There are many other files in many other packages that pose a far greater risk than a README, and none of them belong in the document root.

If you're worried about security and you use Composer, you should probably be grepping your hard drives for vulnerable copies of PHPMailer right now, because you've got bigger problems than a README if you think any "production" deployment using Composer is secure.

The docs are part of the project, and end users need them. I can't work on a train or a plane or in a coffee-shop with a Windows/Mac-only web proxy if I don't have the docs. They accompany the source code, which is why you've got them in the repo in the first place. The only argument for removing them is that "Composer is going to do something asinine with them."

@Synchro

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

This is a bizarre line of reasoning. Most people don't use composer and are still vulnerable to exactly what you say; composer provides a reasonable fix for this problem, and is absolutely not the cause. You're describing a system-level package manager, not an application-level, OS-independent one. We'd be very happy to accept a .deb/rpm/brew etc bundler script, or use another OS-independent PHP dependency manager that provides a better implementation if you'd be so kind as to provide one.

@orlitzky

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2016

If you download a release tarball, extract it, and then move the documentation files into the web root of your site, then you've got me -- you're "vulnerable" to having the documentation right where you put it.

I'm not sure what you want me to provide. Any package script or whatever you had in mind is going to have to grab the release tarball at some point, and the release tarball is going to be missing the documentation, which is exactly the problem that I reported.

@SDKiller

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2016

I'd agree with @orlitzky - if someone is able to open files in your vendor directory in browser - you're most probably doing something wrong - all that should be available from webroot is css, js, images and index.php as entry point.

Abovementioned "pentesting" says exactly that - some developers are doing it wrong and it is not phpmailer fault.
Anyway talking about docs as a point of version disclosure we forget about X-Mailer header which not every webmaster removes.

Of course tests, travis config, etc. shoud not be included in tarball.

@Synchro

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

The original discussion about this was here: https://www.reddit.com/r/PHP/comments/2jzp6k/i_dont_need_your_tests_in_my_production/

The docs folder really isn't worth keeping - as I said, it's gone in 6.0 because everything it covered in 5.2 is either obsolete, has been reimplemented as an example, or moved to the project wiki. phpdoc docs can be generated from the source locally, so they don't need to be included anyway.

I know that access to the vendor (or whatever your manually-maintained equivalent is) folder should be made impossible, however it often isn't - for example Wordpress provides no such protection - often because many users don't have the luxury of a web root in a subfolder. Overall this policy is extremely common across many PHP packages (why it was done in the first place), so I don't think we're doing anything unusual here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.