Skip to content

Adjust handling of (unquoted) extended parameters in headers#2536

Closed
runderwood wants to merge 4 commits intopallets:mainfrom
runderwood:main
Closed

Adjust handling of (unquoted) extended parameters in headers#2536
runderwood wants to merge 4 commits intopallets:mainfrom
runderwood:main

Conversation

@runderwood
Copy link
Copy Markdown

Adjust handling of (unquoted) extended parameters in headers to conform with RFCs and browser implementations.

Addresses #2529 by matching these implementations, namely not quoting extended parameter values with charset/language syntax (see RFC 8187/5987 -- also Chromium's handling of extended parameter values).

Also moves mypy comment (type: ignore) so as to no longer be unused.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

…age identifiers) to conform with standards and browser implementations
@davidism davidism added this to the 2.2.3 milestone Oct 25, 2022
Comment thread tests/test_send_file.py
"""
if utf8:
assert f"filename*=UTF-8''{utf8}" in content_disposition
else:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure why this condition has been removed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think I thought it was moot at the time, but you're absolutely right - - I'll put it back.

Comment thread tests/test_send_file.py Outdated
content_disposition = rv.headers["Content-Disposition"]
assert f"filename={ascii}" in content_disposition

"""Per RFC 5987/8187, "extended" values may *not* be quoted.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this would be better as a comment #, although I don't think it is appropriate here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Would you prefer I remove the comment altogether or reformat it with single-line comment(s)? It was largely a reminder to myself about which standards docs provided the basis for the changes. If it doesn't seem helpful, I'm happy to pull it out entirely.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you pull it out entirely. The comment is in the commit and the docstring above where it is more relevant in my view.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done!

@pgjones
Copy link
Copy Markdown
Member

pgjones commented Jan 8, 2023

This makes sense to me, bar a couple of questions

@pgjones
Copy link
Copy Markdown
Member

pgjones commented Jan 9, 2023

Thanks, I've merged this manually into the 2.2.x branch (as to me this is a bug fix). See 97547cc

@pgjones pgjones closed this Jan 9, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

send_file's Content-Disposition filename* is ignored in Chrome and Safari, when download_name is unicode with a brace

3 participants