Skip to content

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
@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