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

fix: add x-forwarded-host to proxyHeaderIgnore defaults #462

Merged
merged 1 commit into from
Jan 4, 2021
Merged

fix: add x-forwarded-host to proxyHeaderIgnore defaults #462

merged 1 commit into from
Jan 4, 2021

Conversation

hassanselim0
Copy link
Contributor

As mentioned in #456, some reverse proxies get confused when they get an x-forwarded-host and cause loops or incorrectly rewriting the host header. I assume the proxying of this header might also cause issues with CloudFlare.
After adding this header to the ignored list my problem was fixed and I have tested the new defaults with CloudFlare reverse proxy and it's working fine.

Note: I've also updated docs to reflect new defaults and explain the purpose of that new default, and removed the note about disabling header proxying altogether when using CloudFlare since this is no longer needed thanks to this commit and previous ones that added the cf-ray and cf-connecting-ip headers to that same ignore list.

This it to avoid reverse-proxy loops.
Updated docs to reflect new defaults and explain the purpose of that new default.
@codecov
Copy link

codecov bot commented Jan 1, 2021

Codecov Report

Merging #462 (e43b1f7) into master (030b6b7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #462   +/-   ##
=======================================
  Coverage   95.55%   95.55%           
=======================================
  Files           1        1           
  Lines          45       45           
  Branches       25       25           
=======================================
  Hits           43       43           
  Misses          2        2           
Impacted Files Coverage Δ
lib/module.js 95.55% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 030b6b7...e43b1f7. Read the comment docs.

@pi0 pi0 changed the title Added x-forwarded-host to proxyHeaderIgnore defaults fix: add x-forwarded-host to proxyHeaderIgnore defaults Jan 4, 2021
@pi0 pi0 merged commit 433548b into nuxt-community:master Jan 4, 2021
@pi0
Copy link
Member

pi0 commented Jan 4, 2021

Thanks!

@bblanchon
Copy link
Contributor

Hi,

Thank you for this change, @hassanselim0; it's a good improvement, but I think it's incomplete.

If you ignore X-Forwarded-Host, you should also ignore X-Forwarded-Proto.
When Axios forwards this header, Django returns URLs like https://127.0.0.1:8000, i.e., HTTPS instead of HTTP for localhost.

I didn't have any problem with X-Forwarded-Port, but it makes sense to exclude it as well.

@pi0, I'll be glad to open a PR if you want.

Best regards,
Benoit

@hassanselim0
Copy link
Contributor Author

@bblanchon can you elaborate more on that specific case and how it caused problems?

I might have missed it as I've only tested with https on a staging environment on kubernetes (so no localhost), because that's where I have an Nginx reverse proxy (actually two proxies if you count Cloudflare too).

The scenario I'm imagining here is as follows:

Browser > Public Internet > Nginx > Nuxt Server
Nuxt Server > Public Internet > Nginx > Backend Server

This assumes Nuxt Axios mimics everything and doesn't just go from Nuxt to Backend through internal network (I don't know if such a think is possible TBH and it's probably a bad idea to skip the reverse proxy).
So let's say Browser to Nginx is HTTPS and Nginx to Nuxt is HTTP, so you get the header X-Forwarded-Proto: https, so far so good. Then Nuxt send request to Nginx via HTTPS (because it mimics client-side call), then Nginx to Backend Server via HTTP and the X-Forwarded-Proto: https is still there.

Without Nuxt the flow is like this:

Browser > Public Internet > Nginx > Static HTML
Browser > Public Internet > Nginx > Backend Server

In this case the Backend Server will still get X-Forwarded-Proto: https just like in the previous flow.
This makes me feel like the problem you're getting might have a different cause and/or should be fixed regardless of ignoring that header. Or is your flow quite different?

That said, I do see the value of ignoring all X-Forwarded-* headers since proxying them provides no value to anybody as far as I know, and could cause undefined behavior in some niche cases. I just wanted to make this PR limited to the scope of the exact issue I reported which for me happened due to the X-Forwarded-Host header.

@bblanchon
Copy link
Contributor

My scenario is pretty basic:

                                   > Nuxt
                                  /   |
Browser > Public Internet > Nginx     | (ssr)
                                  \   V
                                   > Django

Both servers run on the same machine, so during SSR, the Nuxt server communicates with the backend server via HTTP.
Unfortunately, since Axios forwards X-Forwarded-Proto, the Django server believes the page is served via HTTPS (see SECURE_PROXY_SSL_HEADER).
As a consequence, the Django server returns absolute URLs (see build_absolute_uri) that start with https://127.0.0.1:8000, which is incorrect.

@pi0
Copy link
Member

pi0 commented Jan 11, 2021

Ignoring both X-Forwarded-* headers (X-Forwarded-Proto, X-Forwarded-Port and X-Forwarded-For) makes sense since they are all meant for Client <--> SSR communication rather than SSR <--> API.

@hassanselim0 @bblanchon do you mind making a PR?

(also in foot note, i think going to replace proxied headers with allowlist of headers in next major versions...)

bblanchon added a commit to bblanchon/axios-module that referenced this pull request Jan 11, 2021
bblanchon added a commit to bblanchon/axios-module that referenced this pull request Jan 11, 2021
bblanchon added a commit to bblanchon/axios-module that referenced this pull request Jan 11, 2021
bblanchon added a commit to bblanchon/axios-module that referenced this pull request Jan 11, 2021
bblanchon added a commit to bblanchon/axios-module that referenced this pull request Jan 11, 2021
@bblanchon
Copy link
Contributor

I just opened a PR.
@pi0, I think a whitelist is a good idea.

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