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

only add Content-Type header when respnose body is present #82

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

markdboyd
Copy link
Contributor

Changes Proposed

Related to #81

The discussion in #81 highlights a potential bug in this proxy: it always adds a default Content-Type header to the response, even in cases like HTTP 204/304 responses which have no response body and thus for which a Content-Type header is inappropriate.

This PR updates the Nginx configuration to only set a default Content-Type header when the response body length is not 0, which should prevent undesirable behavior on 204/304 responses.

Security Considerations

It seems like adding a Content-Type header was done to resolve a POAM: cloud-gov/product#540

At the same time, it seems like the Content-Type header itself may have been an afterthought: #6

But for responses where the response body length is 0, I don't see how adding a Content-Type header is ever appropriate.

@markdboyd markdboyd requested review from wz-gsa and a team June 17, 2024 20:09

# Don't add a `Content-Type` header if the response length is 0,
# e.g. 204/304 status codes
if ( $upstream_response_length != 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is making me think of all the if is evil posts about nginx. I think using a map is safer here (but I'm still pondering how exactly to write it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I couldn't find the original "if is evil" post for context. All I found was people saying not to use if in location blocks

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the not in location blocks thing is what I mostly found while digging. I think this is the original "If is evil" post: https://github.com/nginxinc/nginx-wiki/blob/master/source/start/topics/depth/ifisevil.rst

(but note that we are in a location block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the original post: https://web.archive.org/web/20150924145945/https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/

It seems like it is saying not to use if inside a location block, but we're using it inside a server block.

All that being said, if there's a safer way to do it, I'm all for it. It seems like the only way to do it would probably be some kind of Lua block, but that felt beyond me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bengerman13 What do you think about 099ce01 ?

Inspired by:

https://stackoverflow.com/questions/33660493/nginx-lua-set-proxy-headers
https://www.reddit.com/r/nginx/comments/4clta3/using_lua_to_set_headers/

I could also use this to set the headers: https://github.com/openresty/headers-more-nginx-module?tab=readme-ov-file#more_clear_headers, which even allows you to specifically enumerate the response codes for which the header should be added. But it seems to me like we would rather enumerate response codes where the header should not be added. And basing the condition on the response length is probably a better way regardless.

@markdboyd markdboyd requested a review from bengerman13 June 18, 2024 13:37
@markdboyd markdboyd merged commit b43867b into main Jun 20, 2024
@markdboyd markdboyd deleted the remove-content-type-no-response branch June 20, 2024 17:57
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