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

Changed X-Frappe-Site-Name header to use value from $host instead o… #184

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

girip11
Copy link
Contributor

@girip11 girip11 commented Apr 9, 2020

…f $http_host in nginx configuration

ISSUE

$http_host is used for setting header 'X-Frappe-Site-Name'
which adds port number to the header along with the host value. Frappe source app.py expects the header
value to contain only the host name and not the port number. So $host should be used instead of
$http_host to set the 'X-Frappe-Site-Name' header

$http_host vs $host in nginx

$http_host contains the host name along with port number whereas $host contains only the host name in lowercase without the port number.

$host - This variable is equal to line Host in the header of request or
name of the server processing the request if the Host header is not available.
This variable may have a different value from $http_host in such cases:

  • when the Host input header is absent or has an empty value,
    $host equals to the value of server_name directive;
  • when the value of Host contains port number, $host doesn't include
    that port number. $host's value is always lowercase since 0.8.17.

From the frappe source file app.py, X-Frappe-Site-Name is used if its set.

site = _site or request.headers.get('X-Frappe-Site-Name') or get_site_name(request.host)

Since $host variable will never contain port number which is not the case with $http_host,
$host should be used for setting the header 'X-Frappe-Site-Name'. Otherwise we have issues with site serving.

Tested the above changes in compose as well as in swarm environment.
In compose, tested the site with host mapping of 80 and 8000. Works with both the host port mapping.

Tested with erpnext version - v12.5.2

Changes to be committed:
modified: build/common/nginx-default.conf.template

Please provide enough information so that others can review your pull request:

Explain the details for making this change. What existing problem does the pull request solve?

…f `$http_host` in nginx configuration

ISSUE
-----
`$http_host` is used for setting header 'X-Frappe-Site-Name'
which adds port number to the header along with the host value. Frappe source app.py expects the header
value to contain only the host name and not the port number. So `$host` should be used instead of
`$http_host` to set the 'X-Frappe-Site-Name' header

`$http_host` vs `$host` in nginx
--------------------------------
`$http_host` contains the host name along with port number whereas `$host` contains only the host name in lowercase without the port number.

> `$host` - This variable is equal to line Host in the header of request or
> name of the server processing the request if the Host header is not available.
> This variable may have a different value from $http_host in such cases:
> * when the Host input header is absent or has an empty value,
> `$host` equals to the value of server_name directive;
> * when the value of Host contains port number, `$host` doesn't include
> that port number. $host's value is always lowercase since 0.8.17.
> - [$host vs $http_host stackoverflow](https://stackoverflow.com/questions/15414810/whats-the-difference-of-host-and-http-host-in-nginx)

From the frappe source file [app.py](https://github.com/frappe/frappe/blob/develop/frappe/app.py#L107), X-Frappe-Site-Name is used if its set.

```Python
site = _site or request.headers.get('X-Frappe-Site-Name') or get_site_name(request.host)
```

Since `$host` variable will never contain port number which is not the case with `$http_host`,
`$host` should be used for setting the header 'X-Frappe-Site-Name'. Otherwise we have issues with site serving.

Tested the above changes in compose as well as in swarm environment.
In compose, tested the site with host mapping of 80 and 8000. Works with both the host port mapping.

Tested with erpnext version - v12.5.2

Changes to be committed:
	modified:   build/common/nginx-default.conf.template
@revant revant merged commit 1b6ede5 into frappe:develop Apr 10, 2020
TheRealJim1 pushed a commit to TheRealJim1/frappe_docker that referenced this pull request Jul 26, 2024
Changed X-Frappe-Site-Name header to use value from `$host` instead o…
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.

3 participants