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.

Sign up for GitHub

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

Jump to bottom

Validate local file ingestions against a whitelist of directories #3180

Merged
merged 1 commit into from
Oct 10, 2017
Merged

Validate local file ingestions against a whitelist of directories #3180

merged 1 commit into from
Oct 10, 2017

Conversation

mjgiarlo
Copy link
Member

@mjgiarlo mjgiarlo commented Oct 10, 2017 •
edited
Loading

Ports samvera/hyrax#1789 to Sufia for inclusion in 7.4.1 release

From @ojlyytinen:

Make CreateWithRemoteFilesActor to validate the list of file urls given. To pass the validation, each file path must start with a whitelisted directory path from which ingestions are allowed to happen. Without this check, users could ingest any files from the file system (as long as the server process can read them).

The default whitelist will be either empty or contain the Browse Everything file system mount point if it is defined. As far as I can tell, nothing else should need to ingest files directly from the local file system. However, people may have implemented custom ingestion mechanisms in which case they would need to add their ingestion directories in Hyrax.config.ingest_dirs.

The validation method currently allows all urls that use something else than the file: scheme. It would probably be a good idea to implement some kind of validation here as well. It is quite likely that on production systems the server can access network resources that the user shouldn't be able to access.

@samvera/sufia-code-reviewers

jcoyne
jcoyne previously requested changes Oct 10, 2017
View reviewed changes
spec/actors/sufia/create_with_remote_files_actor_spec.rb Outdated

describe "#validate_remote_url" do
before do
allow(Hyrax.config).to receive(:whitelisted_ingest_dirs).and_return(['/test/', '/local/file/'])
Copy link
Contributor

Choose a reason for hiding this comment

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

What's a "Hyrax"?

Copy link
Member Author

Choose a reason for hiding this comment

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

AGH. I thought I got them all.

@mjgiarlo mjgiarlo force-pushed the port_of_hyrax_1789 branch 2 times, most recently from 7a3f6cd to fa2a515 Compare October 10, 2017 21:04
@mjgiarlo mjgiarlo dismissed jcoyne’s stale review October 10, 2017 21:04

Addressed yr review, @jcoyne.

@samvera-deprecated samvera-deprecated deleted a comment from coveralls Oct 10, 2017
@samvera-deprecated samvera-deprecated deleted a comment from coveralls Oct 10, 2017
@samvera-deprecated samvera-deprecated deleted a comment from coveralls Oct 10, 2017
@mjgiarlo mjgiarlo merged commit ef4aa38 into master Oct 10, 2017
@mjgiarlo mjgiarlo deleted the port_of_hyrax_1789 branch October 10, 2017 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcoyne jcoyne jcoyne left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mjgiarlo @jcoyne @carolyncole