-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore(key-value): use json serialization for main resources #23888
Conversation
c8756cc
to
37b8a32
Compare
Codecov Report
@@ Coverage Diff @@
## master #23888 +/- ##
==========================================
- Coverage 68.10% 66.35% -1.76%
==========================================
Files 1940 1940
Lines 75016 75057 +41
Branches 8154 8155 +1
==========================================
- Hits 51092 49803 -1289
- Misses 21841 23171 +1330
Partials 2083 2083
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 90 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
567d754
to
721534c
Compare
superset/migrations/versions/2023-05-01_12-03_9c2a5681ddfd_convert_key_value_entries_to_json.py
Outdated
Show resolved
Hide resolved
721534c
to
7aa52f5
Compare
819307c
to
fe91c72
Compare
fe91c72
to
5543be7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really cool solution!
(cherry picked from commit f1fa1a7)
(cherry picked from commit f1fa1a7)
It makes me sad that people are getting CVEs for stuff like this and even trying to fly it as a "RCE". 😞
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. |
SUMMARY
Since the permalink and app resources were already using simple primitive types that serialize well with
json.dumps
, we don't really need to usepickle
to serialize them. Therefore, to simplify the main key-value resources, we replace pickle with json where applicable. However, the metastore cache will still usepickle
, as it needs to be able to handle arbitrary binary types. This should be ok, as it's opt-in, so it won't be enabled by default.To test this permalinks were created on master branch, the branch checked out, the database migrated, and the permalinks validated to work as before.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION