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

Added cli suport for headers containing colon #813

Merged
merged 1 commit into from
Oct 12, 2020
Merged

Added cli suport for headers containing colon #813

merged 1 commit into from
Oct 12, 2020

Conversation

romulocollopy
Copy link
Contributor

Some http headers need to have colon in its values, such as:

Content-Security-Policy: default-src 'self'; script-src https://example.com;

Passing this value though the client used to raise an error, because this
header would be parsed as:

["Content-Security-Policy", "default-src 'self'; script-src https", "//example.com;"]

And could no be unpached as key, value pair.

This commit limits the spliting on colon to its first occuence in the string,
enabling us to pass a value containing semicolon in the cli:

$ uvicorn main:app --header Content-Security-Policy:default-src *:8000

Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

Good catch !
Can you please also run the scripts/lint so that it formats everything well ?
thanks

CHANGELOG.md Outdated Show resolved Hide resolved
Some http headers need to have colon in its values, such as:
```
Content-Security-Policy: default-src 'self'; script-src https://example.com;
```
Passing this value though the client used to raise an error, because this
header would be parsed as:
```
["Content-Security-Policy", "default-src 'self'; script-src https", "//example.com;"]
```
And could no be unpached as key, value pair.

This commit limits the spliting on colon to its first occuence in the string,
enabling us to pass a value containing semicolon in the cli:

```
$ uvicorn main:app --header 'Content-Security-Policy:default-src *:8000'
```
@romulocollopy
Copy link
Contributor Author

@euri10 changes applied!
I was considering to put a .strip() here: https://github.com/encode/uvicorn/blob/master/uvicorn/config.py#L278

Tell me what you think and I can open another PR by this week (I didn't find where to test it yet)

@euri10
Copy link
Member

euri10 commented Oct 12, 2020

@euri10 changes applied!
I was considering to put a .strip() here: https://github.com/encode/uvicorn/blob/master/uvicorn/config.py#L278

Tell me what you think and I can open another PR by this week (I didn't find where to test it yet)

I dont think that's necessary, I just looked briefly but it seems to me we get key / value in Config from maiin so the strip is not necessary since it will have happened already with this fix, but I can mix up things .)

If that's not the case then writing a failing test will make it obvious.

@euri10 euri10 merged commit 6873289 into encode:master Oct 12, 2020
@euri10
Copy link
Member

euri10 commented Oct 12, 2020

thanks @romulocollopy !

@romulocollopy romulocollopy deleted the fix/header-with-colon branch October 12, 2020 07:59
@romulocollopy
Copy link
Contributor Author

thanks @romulocollopy !

my pleasure =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants