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: use request.protocol to check for HTTPS #282

Merged
merged 1 commit into from
Mar 21, 2024
Merged

fix: use request.protocol to check for HTTPS #282

merged 1 commit into from
Mar 21, 2024

Conversation

mohd-akram
Copy link
Contributor

The header shouldn't be read unless trustProxy is true.

Checklist

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

Is there a problem with trusting the header here? We'd at worst be sending secure=true, and if that's not really the case, the browser would ignore it

But I agree with you in principle that proxies should not be trusted when trustProxy is disabled

@fastify/plugins is this breaking?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

I don't think so.

@gurgunday gurgunday merged commit cb3346f into fastify:master Mar 21, 2024
19 checks passed
@mohd-akram mohd-akram deleted the fix-protocol-check branch March 21, 2024 15:26
@climba03003
Copy link
Member

climba03003 commented Mar 21, 2024

I don't think always sent secure cookies to the client that pretend to be https is a wrong idea.

Even if the next hop is not a trust proxy, sending secure cookies should not harm. Secure cookies will not set when the end client (broswer) is using http.

@gurgunday
Copy link
Member

gurgunday commented Mar 21, 2024

That was what I thought... but the reason I approved anyway is trustProxy wasn't respected at the end of the day...

I could accept the following:

if (opts.secure === 'auto') {
    if (reply.request.protocol === 'https' || (fastify.trustProxy === true && request.headers['x-forwarded-proto'] === 'https')) {
      opts.secure = true
    } else {
      opts.sameSite = 'lax'
    }
}

However, should be noted that x-forwarded-proto is not the standard, Forwarded is

@mohd-akram
Copy link
Contributor Author

fastify.trustProxy === true && request.headers['x-forwarded-proto'] === 'https')

This is the same as reply.request.protocol === 'https' AFAICT since it check trustProxy and uses the header if it's available.

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.

5 participants