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

Support disabling default Server and Date headers #818

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Support disabling default Server and Date headers #818

merged 2 commits into from
Jun 11, 2021

Conversation

stiiin
Copy link
Contributor

@stiiin stiiin commented Oct 14, 2020

As described in the individual commit messages, this pull request adds configuration flags to control the default Server and Date headers. With these flags, the Server header can be entirely removed rather than overwritten, and the Date header can be suppressed in (the extremely rare) case that HTTP/1.1 requires an origin server to do so.

Fixes #688

@euri10
Copy link
Member

euri10 commented Jun 7, 2021

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.

Hi @stiiin sorry for getting back this late,
could you please if you still fancy doing it rebase this PR and adapt your tests to the new "layout" we setup for tests, reading current tests this should be fairly easy, if you dont have time let me know I'll pursue this PR.

@euri10 euri10 self-assigned this Jun 7, 2021
@euri10
Copy link
Member

euri10 commented Jun 7, 2021

also if you can find again the relevant part of the RFC that would be awesome to keep it written here for future reference, thanks !

@stiiin
Copy link
Contributor Author

stiiin commented Jun 7, 2021

Hi @euri10! I rebased my changes, but without running tests. Let's see if the workflow likes it.

also if you can find again the relevant part of the RFC that would be awesome to keep it written here for future reference, thanks !

I already mentioned a section of the RFC in the commit message for the Date header change. Is that what you were looking for, or was it something else?

@euri10
Copy link
Member

euri10 commented Jun 8, 2021

the CI fails, you can use ./scripts/lint

you also can use ./scripts/check and ./scripts/test locally also, it would be preferable as now Github forces me to approve workflow runs for 1st time contributor which is a pain in the **** :)

and thanks for the rfc, but I cant find the link to the server one

date: https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.1.2
server: https://datatracker.ietf.org/doc/html/rfc7231#section-7.4.2

@stiiin
Copy link
Contributor Author

stiiin commented Jun 8, 2021

Sorry for the failed run, I should've just looked around a bit more to find the linter and test run script.

After fixing the linter issues, it seems I messed up the rebase in a way that removed the behaviour for the Date header, so I re-implemented that. Please review those changes, maybe the naming or the style could be better.

I've changed the commit message about the Server option so that it refers to 7.4.2 from the same RFC, where the wording implies that the Server header is optional.

docs/deployment.md Outdated Show resolved Hide resolved
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.

approved with the last naming agreed on ! I'll do the merge once done
thanks a lot for this @stiiin !

@Kludex Kludex added the hold Don't merge yet label Jun 9, 2021
uvicorn/main.py Outdated Show resolved Hide resolved
uvicorn/main.py Outdated Show resolved Hide resolved
uvicorn/main.py Outdated Show resolved Hide resolved
uvicorn/main.py Outdated Show resolved Hide resolved
uvicorn/main.py Outdated Show resolved Hide resolved
uvicorn/main.py Outdated Show resolved Hide resolved
@Kludex Kludex removed the hold Don't merge yet label Jun 10, 2021
@stiiin
Copy link
Contributor Author

stiiin commented Jun 10, 2021

You're right, I should've acted on my other gut feeling ;)

If you don't mind, I'll amend my changes rather than using Github's commit feature. At least I know what's going to happen if I do it through git's CLI.

Section 7.4.2 of RFC 7231 states that "an origin server MAY generate
a Server field in its responses." The "MAY" means that sending this
header is entirely optional.

While the implementation of #321 allowed applications to override
the Server header, there was no way to disable the Server header
altogether.

By default, uvicorn adds a Server header to each response that doesn't
have one through other means. This change adds the server_header flag
to Config, as well as the commandline option --no-server-header,
through which this behaviour can be disabled.
When an origin server does not have a sufficiently reliable clock,
section 7.1.1.2 of RFC 7231 stipulates that it "MUST NOT" send
a Date header field.

By default, uvicorn adds a Date header to each response. This change
adds the date_header flag to Config, as well as the commandline
option --no-date-header, through which this behaviour can be disabled.
@euri10 euri10 merged commit df23249 into encode:master Jun 11, 2021
@euri10
Copy link
Member

euri10 commented Jun 11, 2021

thanks a lot @stiiin !

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

I forgot to press enter on the review. 😅

I'm going to create a small PR later.

@@ -36,6 +36,20 @@ async def test_override_server_header():
)


@pytest.mark.asyncio
async def test_disable_default_server_header():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def test_disable_default_server_header():
async def test_disable_server_header():



@pytest.mark.asyncio
async def test_disable_default_date_header():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def test_disable_default_date_header():
async def test_disable_date_header():

"--server-header/--no-server-header",
is_flag=True,
default=True,
help="Enable/Disable default Server header.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help="Enable/Disable default Server header.",
help="Enable/Disable server header.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the distinction in the help text would still be somewhat helpful. You can always issue a Server header from the application code, right?

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. 😗 👍

"--date-header/--no-date-header",
is_flag=True,
default=True,
help="Enable/Disable default Date header.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help="Enable/Disable default Date header.",
help="Enable/Disable date header.",

Kludex pushed a commit that referenced this pull request Nov 17, 2021
* Support disabling default Server header (#688)

Section 7.4.2 of RFC 7231 states that "an origin server MAY generate
a Server field in its responses." The "MAY" means that sending this
header is entirely optional.

While the implementation of #321 allowed applications to override
the Server header, there was no way to disable the Server header
altogether.

By default, uvicorn adds a Server header to each response that doesn't
have one through other means. This change adds the server_header flag
to Config, as well as the commandline option --no-server-header,
through which this behaviour can be disabled.

* Support disabling default Date header

When an origin server does not have a sufficiently reliable clock,
section 7.1.1.2 of RFC 7231 stipulates that it "MUST NOT" send
a Date header field.

By default, uvicorn adds a Date header to each response. This change
adds the date_header flag to Config, as well as the commandline
option --no-date-header, through which this behaviour can be disabled.
Kludex pushed a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* Support disabling default Server header (encode#688)

Section 7.4.2 of RFC 7231 states that "an origin server MAY generate
a Server field in its responses." The "MAY" means that sending this
header is entirely optional.

While the implementation of encode#321 allowed applications to override
the Server header, there was no way to disable the Server header
altogether.

By default, uvicorn adds a Server header to each response that doesn't
have one through other means. This change adds the server_header flag
to Config, as well as the commandline option --no-server-header,
through which this behaviour can be disabled.

* Support disabling default Date header

When an origin server does not have a sufficiently reliable clock,
section 7.1.1.2 of RFC 7231 stipulates that it "MUST NOT" send
a Date header field.

By default, uvicorn adds a Date header to each response. This change
adds the date_header flag to Config, as well as the commandline
option --no-date-header, through which this behaviour can be disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Server Header
3 participants