feat(multipart): add max_length to secure_filename()

[Krishn1412]

1. adding max_length parameter which truncates the file but includes the ext.
2. This is presently set to None but can be set 255 characters.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7621942) to head (a323dea).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2421   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         7728      7737    +9     
  Branches      1071      1073    +2     
=========================================
+ Hits          7728      7737    +9     

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

[vytas7]

Thanks for this PR @Krishn1412!

In order to speed up the review process though, it is usually more efficient if you make sure that at least tox passes locally for you first. Thanks!

[vytas7]

Provided implementation suggestions in-line.

Also, you still need to make all the tox tests pass. To that end, you'll also need to add new unit tests verifying the proposed behaviour.

[vytas7]

I suggest a smaller default value. On some systems, e.g., Windows, the max possible path length is around 255-260, so likely we will want to make the file name part of the path shorter than that.

Maybe 64 could be a sane default?

[CaselIT]

64 may be a bit small as default, but 96/128 is likely ok.

[vytas7]

Personally I think 64 ought to be enough for the basename, since secure_filename() is mangling the name in any case, so there is no guarantee to get exactly the same string as submitted in the form anyway. But 96 works for me as well.

[CaselIT]

other than the max length for v5 the rest looks fine, thanks!

[vytas7]

Thanks, the algorithm itself looks good now!

It seems that you forgot to actually forgot to wire the new option into the actual implementation of the BodyPart.secure_filename property though.

See also other comments inline.

[vytas7]

This new length is not passed to the actual invocation of secure_filename() when referencing the corresponding part property.

Also add tests (make sure to parametrize on both WSGI/ASGI) for actually modifying this parsing option, and verifying that it is respected when referencing the said property.

[vytas7]

We need to add a newsfragment for this improvement.

[vytas7]

This (int, optional) looks a bit unconventional. I think you can simply remove this type from here altogether, and let Sphinx pick it up from the type annotation.

[Krishn1412]

Do you want to pass the max_length as a parameter in the BodyPart.secure_filename function or set it as an instance or class variable and reference it from there?

[vytas7]

I want us to actually use the new option in BodyPart.secure_filename(), i.e.:

    @property
    def secure_filename(self) -> str:
        """The sanitized version of `filename` using only the most common ASCII
        <...>
        """  # noqa: D205
        try:
            return misc.secure_filename(self.filename or '')
        except ValueError as ex:
            raise MultipartParseError(description=str(ex)) from ex

⬇️

    @property
    def secure_filename(self) -> str:
        """The sanitized version of `filename` using only the most common ASCII
        <...>
        """  # noqa: D205
        try:
            return misc.secure_filename(self.filename or '', max_length=self._parse_options.max_secure_filename_length)
        except ValueError as ex:
            raise MultipartParseError(description=str(ex)) from ex

(untested)