Skip to content
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

ValueError is raised instead of InvalidHeader #6083

Closed
arossert opened this issue Mar 11, 2022 · 6 comments
Closed

ValueError is raised instead of InvalidHeader #6083

arossert opened this issue Mar 11, 2022 · 6 comments
Milestone

Comments

@arossert
Copy link

arossert commented Mar 11, 2022

When using an invalid header name it is not checked in the requests.utils.check_header_validity function, only the value is checked.

This causes the underline HTTPConnection to fail when trying to write the header on ValueError, seems like that it is checked against:
_is_legal_header_name = re.compile(rb'[^:\s][^:\r\n]*').fullmatch

The expected exception, in this case, will be InvalidHeader

import requests
requests.get("https://www.google.com", headers={":bad": "header"})

System Information

$ python -m requests.help
{
  "chardet": {
    "version": null
  },
  "charset_normalizer": {
    "version": "2.0.12"
  },
  "cryptography": {
    "version": ""
  },
  "idna": {
    "version": "3.3"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.9.10"
  },
  "platform": {
    "release": "5.4.0-1068-aws",
    "system": "Linux"
  },
  "pyOpenSSL": {
    "openssl_version": "",
    "version": null
  },
  "requests": {
    "version": "2.27.1"
  },
  "system_ssl": {
    "version": "1010100f"
  },
  "urllib3": {
    "version": "1.26.8"
  },
  "using_charset_normalizer": true,
  "using_pyopenssl": false
}
@Khasim481
Copy link

Khasim481 commented Apr 6, 2022

In requests.utils.check_header_validity there is a validation only for value, I think we have to validate both name and value of the header.

As per the IMF (refer https://www.rfc-editor.org/rfc/rfc2822#section-2.2)

A field name MUST be composed of printable US-ASCII characters (i.e., characters that have values between 33 and 126, inclusive), except colon.

I figured the fix for this one (the regex for validating header name goes like this '^[!-9\[;-~]*$'), I am a first timer, but seasoned developer. Please assign it to me, I would like to contribute.

@nateprewitt
Copy link
Member

Thanks for volunteering @Khasim481. The RFC linked is quite old and has been obsolete for almost a decade. I believe you're looking for the definition in the current RFC 7230. You'll find the exact ABNF is 1 or more tchar as defined in section 3.2.6.

We strictly adhere to the spec for field-values but it's correct we omitted the field-name when this was first done. The change was explicitly to solve security risks with header values. This issue is more cosmetic, but I do agree we should have consistency with the error type. We actually already have a branch in place with most of the fix for this but it's still needs some final refactoring.

@Khasim481
Copy link

The changes in the branch looks perfect, If you mention those cosmetic changes broadly here, I can be of help.

@nateprewitt nateprewitt added this to the 2.28.0 milestone Apr 7, 2022
@arossert
Copy link
Author

arossert commented Jun 3, 2022

@Khasim481 Any estimation when this will be fixed?

@nateprewitt
Copy link
Member

It's currently a candidate fix for the next release, 2.28.0.

@nateprewitt
Copy link
Member

This has been merged in #6154 and will be available in the next release. Thanks everyone!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 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

No branches or pull requests

3 participants