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

nginx preserve upstream header values, fix ip #2847

Merged
merged 1 commit into from
Apr 13, 2023
Merged

nginx preserve upstream header values, fix ip #2847

merged 1 commit into from
Apr 13, 2023

Conversation

kspearrin
Copy link
Member

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Resolves #2500

Fix improper protocol and host header from persisting across proxy jumps.

Also, remove X-Real-IP header from being evaluated for GetIpAddress since it will always be the value of the last connecting client, which could be a proxy whenever there is a chain of multiple proxies. X-Forwarded-For will properly chain proxy IP addresses in a CSV string, and ASP.NET Core will load the correct value in httpContext.Connection.RemoteIpAddress.

Code changes

  • proxy.conf: Null coalesce previously set header values and default back to standard values.
  • CoreHelpers.cs: Remove X-Real-IP from being evaluated from self hosted servers for client ip address.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@kspearrin kspearrin requested review from a team and MGibson1 and removed request for a team April 12, 2023 01:11
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

C# looks good, but I'd like @bitwarden/dept-devops to get involved on nginx config. That is beyond my experience, but looks right 🤷

@kspearrin kspearrin merged commit 4673e3b into master Apr 13, 2023
36 checks passed
@kspearrin kspearrin deleted the headerfix branch April 13, 2023 14:58
@devmsv
Copy link

devmsv commented Mar 18, 2024

Hi I,m still facing issue #2500 with 2024.2.3
is this merged or should I use :dev?

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.

SSO: Invalid redirect URI when using Azure OIDC or SAML 2.0
4 participants