swagger-api / swagger-api/swagger-codegen
python codegen __deseralize_file() is not safe
Nobody has claimed this yet.
- Dominant language
- Mustache
- Stars
- 17.8k
- Forks
- 6k
- PR merge metrics
- No merged PRs in 30d
Description
Description
In api_client.py generated by swagger-codegen the function __deserialize_file calls tempfile.mkstemp(dir=self.configuration.temp_folder_path). This returns a unique temporary path. However, the uniqueness is only guaranteed if the full path including the filename is sued. I tested this on macOS 12.1/python 3.8.2 (not sure other os/python combos). A few lines later the code throws away the file name with a call to os.path.dirname(path) and append the filename retrieved from the Content-Disposition header.
This is flawed in two ways:
- As I mentioned earlier, if multiple files are downloaded where the filename is the same in the
Content-Dispositionheader then the temp file created will be overwritten, and - It is not a good security practice to trust the filename provided by the server.
The fix is to simply use the full path returned by mkstemp.
Swagger-codegen version
the latest version downloaded now at the time of writing which is 3.0.31
Swagger declaration file content or url
N/A
Command line used for generation
N/A
Steps to reproduce
N/A
Related issues/PRs
N/A
Suggest a fix/enhancement
suggested change to modules/swagger-codegen/src/main/resources/python/api_client.mustache
def __deserialize_file(self, response):
fd, path = tempfile.mkstemp(dir=self.configuration.temp_folder_path)
os.close(fd)
with open(path, {{^writeBinary}}"w"{{/writeBinary}}{{#writeBinary}}"wb"{{/writeBinary}}) as f:
f.write(response.data)
return path
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in modules/swagger-codegen/src/main/resources/python/api_client.mustache at the __deserialize_file template referenced by the issue. Read how tempfile.mkstemp, the Content-Disposition filename, and the returned path are used. Done means generated Python clients retain the unique temporary path without trusting or replacing its filename.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100