Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Fix security problems in mapper-attachments. #185

Merged
merged 3 commits into from
Nov 7, 2015

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Nov 7, 2015

Many of the tests were not running, or did not check the exceptions.
I renamed all tests to meet Tests so they run, and assert exception messages.

Also because we must (currently) invoke tika with additional privileges, I added
the security logic, and fixed unit testing to call our static method directly.

This must be package private for security reasons, i simply put everything in
org.elasticsearch.mapper.attachments package.

I upgraded tika to the latest, so we are up to date, and removed logic around
tika == null and old locale issues.

Many of the tests were not running, or did not check the exceptions.
I renamed all tests to meet *Tests* so they run, and assert exception messages.

Also because we must (currently) invoke tika with additional privileges, I added
the security logic, and fixed unit testing to call our static method directly.

This must be package private for security reasons, i simply put everything in
org.elasticsearch.mapper.attachments package.

I upgraded tika to the latest, so we are up to date, and removed logic around
tika == null and old locale issues.
@@ -10,5 +10,5 @@
- do:
nodes.info: {}

- match: { nodes.$master.plugins.0.name: mapper-attachments }
- match: { nodes.$master.plugins.0.name: elasticsearch-mapper-attachments }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be necessary. Esplugin defaults to the project name, and gradle defaults the project name to the directory name. Instead of changing this, if you add the name explicitly in build.gradle under the esplugin block, then it wont matter what directory the repo is checked out into.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is really trappy? We should just fail if this is not set.

@rjernst
Copy link
Member

rjernst commented Nov 7, 2015

Lgtm, one small comment about forcing the plugin name.

rmuir added a commit that referenced this pull request Nov 7, 2015
Fix security problems in mapper-attachments.
@rmuir rmuir merged commit f2ee1d7 into elastic:master Nov 7, 2015
@dadoonet
Copy link
Member

dadoonet commented Nov 7, 2015

Is it possible to backport most of those changes in other branches so the next releases will have those enhancements?

@rmuir
Copy link
Contributor Author

rmuir commented Nov 8, 2015

We can backport all the commits once its stable? I want to get some good tests for each document we support. That means for complex formats like PDF, good mix with embedded/embedded-subset/external fonts, the whole shebang. I think we can get these test docs from tika itself or other apache licensed projects and then we know stuff works.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants