Prevent database connections to sqlite

[suddjian]

CATEGORY

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

SQLite allows users to create DBs locally on the machine running Superset. This is dangerous because it allows mapping the local filesystem and can also lead to DoS attacks. There is no good reason to be using SQLite as an analytics DB, so we've opted to prevent it from being used.

This change introduces a new flag PREVENT_UNSAFE_DB_CONNECTIONS which is true by default. Any other future unsafe db connections can be added to the same logic I've written here.

TEST PLAN

Unit tested, smoke tested locally

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

REVIEWERS

@willbarrett @craig-rueda @nytai

[willbarrett]

It would be good to add tests for the endpoints to ensure that SQLite connection strings are rejected at the API layer. I think this deserves a partial integration test or two.

[dpgaspar]

I think this is a case to write a note on UPDATING.md

[nytai]

Looks like there's an api test failing. That should be enough to ensure the api rejects sqlite connection strings

[craig-rueda]

LGTM, with a few nits. Would be good to add an integration test or two

[codecov-io]

Codecov Report

Merging #9218 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9218      +/-   ##
==========================================
+ Coverage   58.91%   58.92%   +0.01%     
==========================================
  Files         372      372              
  Lines       11996    11999       +3     
  Branches     2937     2940       +3     
==========================================
+ Hits         7068     7071       +3     
  Misses       4750     4750              
  Partials      178      178

Impacted Files	Coverage Δ
...frontend/src/views/dashboardList/DashboardList.tsx	59.34% <0%> (ø)	⬆️
...uperset-frontend/src/views/chartList/ChartList.tsx	63.39% <0%> (ø)	⬆️
...ontend/src/components/ListView/TableCollection.tsx	90% <0%> (+1.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f73f8a...3ef1a0f. Read the comment docs.

[john-bodley]

@suddjian should this be if uri.drivername == "sqlite":? Also could you add typing to this method so it's apparent the type of the uri method.

[suddjian]

This PR is merged but I can add typing in a new PR.

uri is a string so I assume you're referring to the output of make_url from sqlalchemy. make_url(uri).drivername == "sqlite" won't quite work in all cases because there are actually multiple drivers available for sqlite, each with their own protocol portion of the URI. We would need multiple checks, or make_url(uri).drivername.startswith("sqlite"). Any sqlite URI will start with "sqlite", however, so I think this way is simpler.