fix: validate module, class, and filename during MediaUpload deserialization - #2796
Conversation
There was a problem hiding this comment.
Code Review
This pull request mitigates a deserialization vulnerability (CWE-502) in googleapiclient/http.py by replacing dynamic imports and reflection in MediaUpload.new_from_json with an explicit allowlist of supported subclasses. It also adds input validation to MediaFileUpload.from_json to prevent null-byte injection and malformed filenames. Feedback on the changes highlights two issues: first, the new test class uses pytest features and does not inherit from unittest.TestCase, which will cause them to be skipped or fail under the existing unittest runner; second, using .get() without fallback values for _chunksize and _resumable can pass None to the constructor, resulting in a TypeError.
d6c4c26 to
0a4ce39
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces security mitigations for CWE-502 (unsafe dynamic deserialization) in MediaUpload.new_from_json by restricting deserialization to an explicit allowlist of classes and validating that the module is googleapiclient.http. It also hardens MediaFileUpload.from_json by validating the filename and providing safe fallbacks for chunk size and resumability, accompanied by comprehensive unit tests. The review feedback suggests further hardening the deserialization process by ensuring that the parsed JSON is indeed a dictionary before calling .get() on it, and validating the types of _chunksize, _resumable, and _mimetype to prevent unexpected runtime errors.
0a4ce39 to
b1ba5d5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces safe deserialization mitigations for CWE-502 in googleapiclient/http.py by replacing dynamic imports and reflection with an explicit class allowlist. It also adds strict type and structure validation for deserialized JSON payloads in MediaUpload.new_from_json and MediaFileUpload.from_json, along with comprehensive unit tests. The review feedback suggests defining the class allowlist dictionary after the MediaFileUpload class definition to avoid forward-reference lambdas, and replacing an f-string with standard % formatting to maintain consistency with the rest of the codebase.
Fixes unsafe reflection in
MediaUpload.new_from_json()where_moduleand_classwere passed directly to__import__()andgetattr().Key changes:
MediaUpload.new_from_json()with an explicit lookup mapping limited togoogleapiclient.http.MediaFileUpload.dicttype check on parsed JSON in bothMediaUpload.new_from_json()andMediaFileUpload.from_json()to avoidAttributeErroron primitive/array JSON inputs.MediaFileUpload.from_json():_filenamemust be a non-empty string without null bytes._chunksizemust be an integer, defaulting toDEFAULT_CHUNK_SIZEifNone._resumablemust be a boolean, defaulting toFalseifNone._mimetypemust be a string orNone.Fixes b/531771000