Skip to content
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

security: SecureSerializer: support generic low-level serializers #8982

Merged
merged 2 commits into from May 5, 2024

Conversation

shirsa
Copy link
Contributor

@shirsa shirsa commented Apr 25, 2024

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

Fixes #8981

This PR adds 'data_type' as a new field to the metadata that is saved as part of SecuredSerializer serialization.
This info enable us to encode/decode the original data, in order to be fit to the expected input of our serializer, without the need to assume its type when decoding (for example, today there is assumption that if the decided value is bytes, then the original data was of type string. This assumption also breaks other serializers that outputs 'binary' format, such as pickle.

Copy link

codecov bot commented Apr 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.81%. Comparing base (e9ebd65) to head (c254589).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8982   +/-   ##
=======================================
  Coverage   77.80%   77.81%           
=======================================
  Files         150      150           
  Lines       18686    18686           
  Branches     3193     3193           
=======================================
+ Hits        14539    14540    +1     
  Misses       3854     3854           
+ Partials      293      292    -1     
Flag Coverage Δ
unittests 77.79% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shirsa
Copy link
Contributor Author

shirsa commented Apr 30, 2024

@auvipy can you check when you have time, please?
Thanks!

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be better to have more test coverage and if possible some integration test.

@shirsa shirsa force-pushed the main branch 4 times, most recently from a5874b1 to 96a766d Compare May 1, 2024 16:52
@shirsa
Copy link
Contributor Author

shirsa commented May 1, 2024

it would be better to have more test coverage and if possible some integration test.

Thanks @auvipy
I realized that the fix is much more simple, and I added more coverage to the unit-tests then there is today (so the current bug will be checked as well).
Regarding the integration tests, there is currently an issue (#5269) that prevents the ability to run the security integration tests

@shirsa shirsa force-pushed the main branch 2 times, most recently from 96a766d to 9679a41 Compare May 1, 2024 18:10
@auvipy
Copy link
Member

auvipy commented May 2, 2024

I restarted the build

@shirsa
Copy link
Contributor Author

shirsa commented May 2, 2024

I restarted the build

Thanks @auvipy .
The smoke-tests failed on timeout, but I don't think its related to my changes..

@shirsa
Copy link
Contributor Author

shirsa commented May 5, 2024

@auvipy Is there anything else you would like me to change/add here?
Thanks

@auvipy auvipy merged commit 91c5b90 into celery:main May 5, 2024
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SecureSerializer fails on certain types and binary serializers
2 participants