Skip to content

Commit

Permalink
Fix adming regex search handling and run brakeman and bundle audit.
Browse files Browse the repository at this point in the history
The various admin searches were passed to mongo as a regex search.
However, we were only wanting to perform a "contains" search, and not
actually expose the raw regexs. Exposing the raw regexes caused searches
with various special characters to fail (since they weren't escaped).

Allowing raw regexes also allowed for potential regex-based denial of
services by untrusted admins. Brakeman pointed this out, so we're also
adding brakeman and bundle-audit to our CI runs for better security
checks.
  • Loading branch information
GUI committed Apr 28, 2016
1 parent 7ff8625 commit a528416
Show file tree
Hide file tree
Showing 11 changed files with 94 additions and 29 deletions.
23 changes: 21 additions & 2 deletions build/cmake/test/test-web-app.cmake
Original file line number Diff line number Diff line change
@@ -1,7 +1,26 @@
add_custom_target(
test-web-app-target
env PATH=${STAGE_EMBEDDED_DIR}/bin:$ENV{PATH} INTEGRATION_TEST_SUITE=true bundle exec rake
test-web-app-bundle-audit
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/src/api-umbrella/web-app
COMMAND env PATH=${STAGE_EMBEDDED_DIR}/bin:$ENV{PATH} bundle exec bundle-audit check --update
)

add_custom_target(
test-web-app-brakeman
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/src/api-umbrella/web-app
COMMAND env PATH=${STAGE_EMBEDDED_DIR}/bin:$ENV{PATH} bundle exec brakeman . --format html --output brakeman.html --exit-on-warn
)

add_custom_target(
test-web-app-rake
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/src/api-umbrella/web-app
COMMAND env PATH=${STAGE_EMBEDDED_DIR}/bin:$ENV{PATH} INTEGRATION_TEST_SUITE=true bundle exec rake
)

add_custom_target(
test-web-app-target
COMMAND ${CMAKE_BUILD_TOOL} test-web-app-bundle-audit
COMMAND ${CMAKE_BUILD_TOOL} test-web-app-brakeman
COMMAND ${CMAKE_BUILD_TOOL} test-web-app-rake
)

add_custom_target(
Expand Down
1 change: 1 addition & 0 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ general:
# Keep logs after running to help debug if errors do crop up.
- /tmp/api-umbrella-test/var/log
- src/api-umbrella/web-app/log
- src/api-umbrella/web-app/brakeman.html
# Keep screenshots of capybara failures for easier debugging.
- src/api-umbrella/web-app/tmp/capybara
machine:
Expand Down
3 changes: 2 additions & 1 deletion src/api-umbrella/web-app/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ config/settings.local.yml
config/settings/*.local.yml
config/environments/*.local.yml

# Test coverage reports
# Test reports
/spec/reports
/coverage
/brakeman.html

# Generated assets
/public/test-assets
Expand Down
9 changes: 9 additions & 0 deletions src/api-umbrella/web-app/Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,15 @@ group :development, :test do

# For running the api-umbrella process as a background process during tests.
gem "childprocess", "~> 0.5.9"

# Check for security issues on gem dependencies.
#
# Use this fork for .bundlerauditignore support:
# https://github.com/rubysec/bundler-audit/pull/122
gem "bundler-audit", :git => "https://github.com/rickypai/bundler-audit.git", :branch => "rpai/ignorefile"

# Check for application security issues.
gem "brakeman"
end

group :development do
Expand Down
35 changes: 35 additions & 0 deletions src/api-umbrella/web-app/Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ GIT
pyu-ruby-sasl (~> 0.0.3.1)
rubyntlm (~> 0.3)

GIT
remote: https://github.com/rickypai/bundler-audit.git
revision: 1781f4944f2e2f11358a05240091cb29ed70f906
branch: rpai/ignorefile
specs:
bundler-audit (0.4.0)
bundler (~> 1.2)
thor (~> 0.18)

GIT
remote: https://github.com/riking/omniauth-github.git
revision: 8d1f8b665501dd27bae941ea8e128597b7e8edea
Expand Down Expand Up @@ -109,6 +118,16 @@ GEM
blankslate (3.1.3)
bootstrap-sass (2.3.2.2)
sass (~> 3.2)
brakeman (3.2.1)
erubis (~> 2.6)
haml (>= 3.0, < 5.0)
highline (>= 1.6.20, < 2.0)
ruby2ruby (~> 2.3.0)
ruby_parser (~> 3.8.1)
safe_yaml (>= 1.0)
sass (~> 3.0)
slim (>= 1.3.6, < 4.0)
terminal-table (~> 1.4)
builder (3.0.4)
capistrano (3.3.5)
capistrano-stats (~> 1.1.0)
Expand Down Expand Up @@ -227,8 +246,11 @@ GEM
fuubar (1.3.3)
rspec (>= 2.14.0, < 3.1.0)
ruby-progressbar (~> 1.4)
haml (4.0.7)
tilt
handlebars-source (1.3.0)
hashie (3.4.3)
highline (1.7.8)
hike (1.2.3)
htmlentities (4.3.3)
http_accept_language (2.0.5)
Expand Down Expand Up @@ -459,6 +481,11 @@ GEM
rainbow (>= 1.99.1, < 3.0)
ruby-progressbar (~> 1.4)
ruby-progressbar (1.7.5)
ruby2ruby (2.3.0)
ruby_parser (~> 3.1)
sexp_processor (~> 4.0)
ruby_parser (3.8.1)
sexp_processor (~> 4.1)
rubyntlm (0.5.2)
safe_yaml (1.0.4)
sass (3.2.19)
Expand All @@ -467,6 +494,10 @@ GEM
sass (>= 3.1.10)
tilt (~> 1.3)
sequel (4.31.0)
sexp_processor (4.7.0)
slim (3.0.6)
temple (~> 0.7.3)
tilt (>= 1.3.3, < 2.1)
sprockets (2.2.3)
hike (~> 1.2)
multi_json (~> 1.0)
Expand All @@ -479,6 +510,8 @@ GEM
net-scp (>= 1.1.2)
net-ssh (>= 2.8.0)
tabs_on_rails (2.2.0)
temple (0.7.6)
terminal-table (1.5.2)
test-unit (3.1.3)
power_assert
therubyracer (0.12.2)
Expand Down Expand Up @@ -523,6 +556,8 @@ DEPENDENCIES
addressable (~> 2.3.8)
awesome_print (~> 1.6.1)
bootstrap-sass (~> 2.3.2.2)
brakeman
bundler-audit!
capistrano (~> 3.3.5)
capistrano-rails (~> 1.1.2)
capybara (~> 2.5.0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def index

if(params[:search] && params[:search][:value].present?)
@admin_groups = @admin_groups.or([
{ :name => /#{params[:search][:value]}/i },
{ :name => /#{Regexp.escape(params[:search][:value])}/i },
])
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ def index

if(params["search"] && params["search"]["value"].present?)
@admins = @admins.or([
{ :first_name => /#{params["search"]["value"]}/i },
{ :last_name => /#{params["search"]["value"]}/i },
{ :email => /#{params["search"]["value"]}/i },
{ :username => /#{params["search"]["value"]}/i },
{ :authentication_token => /#{params["search"]["value"]}/i },
{ :_id => /#{params["search"]["value"]}/i },
{ :first_name => /#{Regexp.escape(params["search"]["value"])}/i },
{ :last_name => /#{Regexp.escape(params["search"]["value"])}/i },
{ :email => /#{Regexp.escape(params["search"]["value"])}/i },
{ :username => /#{Regexp.escape(params["search"]["value"])}/i },
{ :authentication_token => /#{Regexp.escape(params["search"]["value"])}/i },
{ :_id => /#{Regexp.escape(params["search"]["value"])}/i },
])
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ def index

if(params[:search] && params[:search][:value].present?)
@api_scopes = @api_scopes.or([
{ :name => /#{params[:search][:value]}/i },
{ :host => /#{params[:search][:value]}/i },
{ :path_prefix => /#{params[:search][:value]}/i },
{ :name => /#{Regexp.escape(params[:search][:value])}/i },
{ :host => /#{Regexp.escape(params[:search][:value])}/i },
{ :path_prefix => /#{Regexp.escape(params[:search][:value])}/i },
])
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ def index

if(params["search"] && params["search"]["value"].present?)
@apis = @apis.or([
{ :name => /#{params["search"]["value"]}/i },
{ :frontend_host => /#{params["search"]["value"]}/i },
{ :backend_host => /#{params["search"]["value"]}/i },
{ :"url_matches.backend_prefix" => /#{params["search"]["value"]}/i },
{ :"url_matches.frontend_prefix" => /#{params["search"]["value"]}/i },
{ :"servers.host" => /#{params["search"]["value"]}/i },
{ :_id => /#{params["search"]["value"]}/i },
{ :name => /#{Regexp.escape(params["search"]["value"])}/i },
{ :frontend_host => /#{Regexp.escape(params["search"]["value"])}/i },
{ :backend_host => /#{Regexp.escape(params["search"]["value"])}/i },
{ :"url_matches.backend_prefix" => /#{Regexp.escape(params["search"]["value"])}/i },
{ :"url_matches.frontend_prefix" => /#{Regexp.escape(params["search"]["value"])}/i },
{ :"servers.host" => /#{Regexp.escape(params["search"]["value"])}/i },
{ :_id => /#{Regexp.escape(params["search"]["value"])}/i },
])
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ def index

if(params["search"] && params["search"]["value"].present?)
@api_users = @api_users.or([
{ :first_name => /#{params["search"]["value"]}/i },
{ :last_name => /#{params["search"]["value"]}/i },
{ :email => /#{params["search"]["value"]}/i },
{ :api_key => /#{params["search"]["value"]}/i },
{ :registration_source => /#{params["search"]["value"]}/i },
{ :roles => /#{params["search"]["value"]}/i },
{ :_id => /#{params["search"]["value"]}/i },
{ :first_name => /#{Regexp.escape(params["search"]["value"])}/i },
{ :last_name => /#{Regexp.escape(params["search"]["value"])}/i },
{ :email => /#{Regexp.escape(params["search"]["value"])}/i },
{ :api_key => /#{Regexp.escape(params["search"]["value"])}/i },
{ :registration_source => /#{Regexp.escape(params["search"]["value"])}/i },
{ :roles => /#{Regexp.escape(params["search"]["value"])}/i },
{ :_id => /#{Regexp.escape(params["search"]["value"])}/i },
])
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ def index

if(params[:search] && params[:search][:value].present?)
@website_backends = @website_backends.or([
{ :frontend_host => /#{params[:search][:value]}/i },
{ :server_host => /#{params[:search][:value]}/i },
{ :frontend_host => /#{Regexp.escape(params[:search][:value])}/i },
{ :server_host => /#{Regexp.escape(params[:search][:value])}/i },
])
end
end
Expand Down

0 comments on commit a528416

Please sign in to comment.