You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/28 00:23:07 UTC

[GitHub] [incubator-superset] suddjian opened a new pull request #9218: Prevent database connections to sqlite

suddjian opened a new pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218
 
 
   ### CATEGORY
   
   - [x] Bug Fix
   - [ ] Enhancement (new features, refinement)
   - [ ] Refactor
   - [x] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   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
   <!--- What steps should be taken to verify the changes -->
   
   Unit tested, smoke tested locally
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] 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 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] suddjian commented on a change in pull request #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#discussion_r387395285
 
 

 ##########
 File path: superset/security/analytics_db_safety.py
 ##########
 @@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+class DBSecurityException(Exception):
+    """ Exception to prevent a security issue with connecting a DB """
+
+    status = 400
+
+
+def check_sqlalchemy_uri(uri):
+    if uri.startswith("sqlite"):
 
 Review comment:
   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](https://docs.sqlalchemy.org/en/13/dialects/sqlite.html#dialect-sqlite-pysqlite-connect) 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.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] craig-rueda merged pull request #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
craig-rueda merged pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] john-bodley commented on a change in pull request #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#discussion_r387375975
 
 

 ##########
 File path: superset/security/analytics_db_safety.py
 ##########
 @@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+class DBSecurityException(Exception):
+    """ Exception to prevent a security issue with connecting a DB """
+
+    status = 400
+
+
+def check_sqlalchemy_uri(uri):
+    if uri.startswith("sqlite"):
 
 Review comment:
    @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.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-io commented on issue #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#issuecomment-592781079
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=h1) Report
   > Merging [#9218](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4f73f8a1f9fec7f15ec760d6d98617bbe04f4023?src=pr&el=desc) will **increase** coverage by `0.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9218/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=tree)
   
   ```diff
   @@            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](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9218/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `59.34% <0%> (ø)` | :arrow_up: |
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9218/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `63.39% <0%> (ø)` | :arrow_up: |
   | [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9218/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `90% <0%> (+1.11%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=footer). Last update [4f73f8a...3ef1a0f](https://codecov.io/gh/apache/incubator-superset/pull/9218?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] craig-rueda commented on a change in pull request #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#discussion_r385916646
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1314,6 +1318,8 @@ def testconn(self):
         db_name = request.json.get("name")
         uri = request.json.get("uri")
         try:
+            if app.config.get("PREVENT_UNSAFE_DB_CONNECTIONS"):
 
 Review comment:
   Change `app.config.get("PREVENT_UNSAFE_DB_CONNECTIONS")` -> `app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]` as all configs are required to have defaults.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] willbarrett commented on issue #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#issuecomment-592605599
 
 
   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.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] craig-rueda commented on a change in pull request #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#discussion_r385916714
 
 

 ##########
 File path: superset/views/database/mixins.py
 ##########
 @@ -191,6 +192,8 @@ class DatabaseMixin:
     }
 
     def _pre_add_update(self, database):
+        if app.config.get("PREVENT_UNSAFE_DB_CONNECTIONS"):
 
 Review comment:
   Same comment as above

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] dpgaspar commented on issue #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#issuecomment-592631772
 
 
   I think this is a case to write a note on UPDATING.md

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] suddjian commented on a change in pull request #9218: Prevent database connections to sqlite

Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9218: Prevent database connections to sqlite
URL: https://github.com/apache/incubator-superset/pull/9218#discussion_r387395285
 
 

 ##########
 File path: superset/security/analytics_db_safety.py
 ##########
 @@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+class DBSecurityException(Exception):
+    """ Exception to prevent a security issue with connecting a DB """
+
+    status = 400
+
+
+def check_sqlalchemy_uri(uri):
+    if uri.startswith("sqlite"):
 
 Review comment:
   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](https://docs.sqlalchemy.org/en/13/dialects/sqlite.html#dialect-sqlite-pysqlite-connect) 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.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org