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

restrict allowable paths for bundle dir input #399

Merged
merged 1 commit into from Oct 3, 2018

Conversation

SaravShah
Copy link
Contributor

@SaravShah SaravShah commented Oct 3, 2018

Hopefully i spelled all of the mounts here correctly

closes #387

@coveralls
Copy link

coveralls commented Oct 3, 2018

Pull Request Test Coverage Report for Build 1302

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 88.103%

Totals Coverage Status
Change from base Build 1294: 0.3%
Covered Lines: 511
Relevant Lines: 580

💛 - Coveralls

@atz
Copy link
Contributor

atz commented Oct 3, 2018

List looks correct.

Copy link
Member

@jmartin-sul jmartin-sul left a comment

Choose a reason for hiding this comment

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

not sure whether we want to treat the dir listing as sensitive info. guessing probably not.

still, i'd be more in favor of putting the development and test listing as a single entry in the base settings.yml (since it's the same for both envs), and then putting the production settings in production.yml in shared configs. the separate yml with env specific sections loaded by an initializer seems a little non-standard/circuitous, and i'm not sure i see the benefit.

retracting the above comment, as @atz and @SaravShah pointed out that there's some template rendering in order to get the paths set up for dev and test (i didn't notice that the config file is actually a .yml.erb, not a plain .yml).

end

it 'not a sub dir of parent directories' do
expect { bc.bundle_dir = 'spec/../../' }.to change(bc, :valid?).to(false)
Copy link
Member

Choose a reason for hiding this comment

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

what about using 'spec/test_data/../../' instead? that way, the test path's first few segments would be valid were it not for the .. (and thus more plausible as an attack if we were being more naive about our path normalization and comparison).

@jmartin-sul jmartin-sul merged commit dfb049b into master Oct 3, 2018
@jmartin-sul jmartin-sul deleted the path_validation_bundle_dir branch October 3, 2018 21:50
@jmartin-sul jmartin-sul removed the review label Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restrict allowable paths for bundle input dir
4 participants