-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Remove XCom API endpoint full deserialization option #27740
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
Conversation
It is possible to fully deserialize XCom values in the webserver by specifying a `deserialize` parameter. This creates a potential security issue as deserialization instantiates arbitrary objects. Custom XCom backends should implement `orm_deserialize_value` themselves if they want to retain the behavior. However, it is strongly suggested to deserialize on the client side for the above mentioned reasons.
e9b6f89 to
06fc728
Compare
|
The changes we are introducing in #27540 are what makes this behaviour a potential security risk now (though tbf this never "worked" well if you stored xcom as pickle as it wouldn't be able to send the "inflated" value over an HTTP transport anyway. Trying to decide if we just remove that feature entirely, or if we need to keep it for non-pickle+plain JSON cases (which does sound complex, just talking out loud about options) |
|
You could DOS the webserver with a large pickle without #27540 which is also a security issue :-). As I mentioned I think the proper way (i.e. architecturally correct) to deserialize over-the-wire is to do this on the client side. After all if using a transport like HTTP we are serializing again. |
That implies the client has the credentials to perform that, and doing it on the server almost seems like it was explicitly the reason the OP asked for this feature in #26174 |
|
That's not neccessarily true. With JSON serialization all the information to deserialize is available to you. With |
|
Sounds to me the most reasonable approach here would be to add a config to allow this feature. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
It is possible to fully deserialize XCom values in the webserver by specifying a
deserializeparameter. This creates a potential security issue as deserialization instantiates arbitrary objects.Custom XCom backends should implement
orm_deserialize_valuethemselves if they want to retain the behavior. However, it is strongly suggested to deserialize on the client side for the above mentioned reasons.See also: #26343
cc @uranusjr @ashb @herunyu
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.