You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "villebro (via GitHub)" <gi...@apache.org> on 2023/05/01 10:21:11 UTC
[GitHub] [superset] villebro opened a new pull request, #23888: chore(key-value): use json serialization for main resources
villebro opened a new pull request, #23888:
URL: https://github.com/apache/superset/pull/23888
### SUMMARY
To simplify the main key-value resources, replace pickle with json.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dpgaspar commented on a diff in pull request #23888: chore(key-value): use json serialization for main resources
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #23888:
URL: https://github.com/apache/superset/pull/23888#discussion_r1182229236
##########
superset/migrations/versions/2023-05-01_12-03_9c2a5681ddfd_convert_key_value_entries_to_json.py:
##########
@@ -0,0 +1,73 @@
+# 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.
+"""convert key-value entries to json
+
+Revision ID: 9c2a5681ddfd
+Revises: 7e67aecbf3f1
+Create Date: 2023-05-01 12:03:17.079862
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "9c2a5681ddfd"
+down_revision = "7e67aecbf3f1"
+
+import json
+import pickle
+
+from alembic import op
+from sqlalchemy import Column, Integer, LargeBinary, String
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy.orm import Session
+
+from superset import db
+from superset.migrations.shared.utils import paginated_update
+
+Base = declarative_base()
+VALUE_MAX_SIZE = 2**24 - 1
+RESOURCES_TO_MIGRATE = ("app", "dashboard_permalink", "explore_permalink")
+
+
+class KeyValueEntry(Base):
+ __tablename__ = "key_value"
+ id = Column(Integer, primary_key=True)
+ resource = Column(String(32), nullable=False)
+ value = Column(LargeBinary(length=VALUE_MAX_SIZE), nullable=False)
+
+
+def upgrade():
+ bind = op.get_bind()
+ session: Session = db.Session(bind=bind)
+ for entry in paginated_update(
+ session.query(KeyValueEntry).filter(
+ KeyValueEntry.resource.in_(RESOURCES_TO_MIGRATE)
+ )
+ ):
+ value = pickle.loads(entry.value) or {}
Review Comment:
Can we identify the classes that we allow for here? and use Unpickler and find_class
##########
superset/key_value/types.py:
##########
@@ -42,3 +47,27 @@ class KeyValueResource(str, Enum):
class SharedKey(str, Enum):
DASHBOARD_PERMALINK_SALT = "dashboard_permalink_salt"
EXPLORE_PERMALINK_SALT = "explore_permalink_salt"
+
+
+class KeyValueCodec(ABC):
+ def encode(self, value: Any) -> bytes:
Review Comment:
nit: how about:
``` python
class KeyValueCodec(ABC):
@abstractmethod
def encode(self, value: Any) -> bytes:
...
@abstractmethod
def decode(self, value: bytes) -> Any:
...
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23888: chore(key-value): use json serialization for main resources
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23888:
URL: https://github.com/apache/superset/pull/23888#discussion_r1183375448
##########
superset/migrations/versions/2023-05-01_12-03_9c2a5681ddfd_convert_key_value_entries_to_json.py:
##########
@@ -0,0 +1,73 @@
+# 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.
+"""convert key-value entries to json
+
+Revision ID: 9c2a5681ddfd
+Revises: 7e67aecbf3f1
+Create Date: 2023-05-01 12:03:17.079862
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "9c2a5681ddfd"
+down_revision = "7e67aecbf3f1"
+
+import json
+import pickle
+
+from alembic import op
+from sqlalchemy import Column, Integer, LargeBinary, String
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy.orm import Session
+
+from superset import db
+from superset.migrations.shared.utils import paginated_update
+
+Base = declarative_base()
+VALUE_MAX_SIZE = 2**24 - 1
+RESOURCES_TO_MIGRATE = ("app", "dashboard_permalink", "explore_permalink")
+
+
+class KeyValueEntry(Base):
+ __tablename__ = "key_value"
+ id = Column(Integer, primary_key=True)
+ resource = Column(String(32), nullable=False)
+ value = Column(LargeBinary(length=VALUE_MAX_SIZE), nullable=False)
+
+
+def upgrade():
+ bind = op.get_bind()
+ session: Session = db.Session(bind=bind)
+ for entry in paginated_update(
+ session.query(KeyValueEntry).filter(
+ KeyValueEntry.resource.in_(RESOURCES_TO_MIGRATE)
+ )
+ ):
+ value = pickle.loads(entry.value) or {}
Review Comment:
Great idea - I added a `RestrictedUnpickler` that fails for all classes (primitive types don't invoke `find_class`, so `dict` and `list` which we need are ok)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] vin01 commented on pull request #23888: chore(key-value): use json serialization for main resources
Posted by "vin01 (via GitHub)" <gi...@apache.org>.
vin01 commented on PR #23888:
URL: https://github.com/apache/superset/pull/23888#issuecomment-1712192516
It makes me sad that people are getting CVEs for stuff like this and even trying to fly it as a "RCE". 😞
- https://www.horizon3.ai/apache-superset-part-ii-rce-credential-harvesting-and-more/
> An attacker with write access to the metadata database can insert an arbitrary pickle payload into the store, and then trigger deserialization of it, leading to remote code execution.
With write access to the metadata database, I don't think any further steps are needed to compromise anything here. Such CVEs should be outright rejected.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #23888: chore(key-value): use json serialization for main resources
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #23888:
URL: https://github.com/apache/superset/pull/23888
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23888: chore(key-value): use json serialization for main resources
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #23888:
URL: https://github.com/apache/superset/pull/23888#discussion_r1183780595
##########
tests/integration_tests/key_value/commands/create_test.py:
##########
@@ -53,11 +60,43 @@ def test_create_uuid_entry(app_context: AppContext, admin: User) -> None:
from superset.key_value.models import KeyValueEntry
with override_user(admin):
- key = CreateKeyValueCommand(resource=RESOURCE, value=VALUE).run()
+ key = CreateKeyValueCommand(
+ resource=RESOURCE, value=JSON_VALUE, codec=JSON_CODEC
+ ).run()
entry = (
db.session.query(KeyValueEntry).filter_by(uuid=key.uuid).autoflush(False).one()
)
- assert pickle.loads(entry.value) == VALUE
+ assert json.loads(entry.value) == JSON_VALUE
assert entry.created_by_fk == admin.id
db.session.delete(entry)
db.session.commit()
+
+
+def test_create_fail_json_entry(app_context: AppContext, admin: User) -> None:
Review Comment:
Nice tests
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23888: chore(key-value): use json serialization for main resources
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23888:
URL: https://github.com/apache/superset/pull/23888#issuecomment-1529580236
## [Codecov](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23888](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d283de0) into [master](https://codecov.io/gh/apache/superset/commit/07381834f885d5813862d33b2870ad7be5cfdfca?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0738183) will **decrease** coverage by `11.39%`.
> The diff coverage is `70.14%`.
> :exclamation: Current head d283de0 differs from pull request most recent head f8af7e3. Consider uploading reports for the commit f8af7e3 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23888 +/- ##
===========================================
- Coverage 68.11% 56.72% -11.39%
===========================================
Files 1938 1938
Lines 74958 75004 +46
Branches 8141 8141
===========================================
- Hits 51055 42549 -8506
- Misses 21824 30376 +8552
Partials 2079 2079
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.94% <67.16%> (+0.01%)` | :arrow_up: |
| python | `59.13% <70.14%> (-23.55%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `52.83% <70.14%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...tend/src/components/Chart/DrillBy/DrillByChart.tsx](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5Q2hhcnQudHN4) | `100.00% <ø> (ø)` | |
| [.../components/Chart/DrillBy/useDisplayModeToggle.tsx](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS91c2VEaXNwbGF5TW9kZVRvZ2dsZS50c3g=) | `100.00% <ø> (ø)` | |
| [...c/components/Chart/DrillBy/useResultsTableView.tsx](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS91c2VSZXN1bHRzVGFibGVWaWV3LnRzeA==) | `100.00% <ø> (ø)` | |
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `55.96% <ø> (ø)` | |
| [superset/dashboards/permalink/commands/create.py](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9wZXJtYWxpbmsvY29tbWFuZHMvY3JlYXRlLnB5) | `50.00% <ø> (-42.86%)` | :arrow_down: |
| [superset/dashboards/permalink/commands/get.py](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9wZXJtYWxpbmsvY29tbWFuZHMvZ2V0LnB5) | `50.00% <0.00%> (-31.25%)` | :arrow_down: |
| [superset/examples/supported\_charts\_dashboard.py](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvc3VwcG9ydGVkX2NoYXJ0c19kYXNoYm9hcmQucHk=) | `0.00% <ø> (ø)` | |
| [superset/explore/permalink/commands/create.py](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhwbG9yZS9wZXJtYWxpbmsvY29tbWFuZHMvY3JlYXRlLnB5) | `42.42% <ø> (-48.49%)` | :arrow_down: |
| [superset/explore/permalink/commands/get.py](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhwbG9yZS9wZXJtYWxpbmsvY29tbWFuZHMvZ2V0LnB5) | `48.57% <ø> (-42.86%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| ... and [14 more](https://codecov.io/gh/apache/superset/pull/23888?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [293 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23888/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org