Fixes an issue with x-forwarded headers and checkOrigin'

[qzio]

Changes

Before this change, the validateForwardedHeaders only used the matchPattern with a hardcoded hostname of example.com when validating the X-Forwarded-Proto header.

This meant that NOT having example.com in the security.allowedDomains would always result in an empty result.proto.

Which meant that when having security.checkOrigin to true, it would make it impossible to get POST's through unless the host was localhost, example.com, or by adding example.com into the security.allowedDomains list.

The middleware in packages/astro/src/core/app/middlewares.ts checks the origin header against the parsed request, and includes the protocol in that check.
So, unless the protocol are parsed correctly, the check will fail.

This PR changes the validateForwardedHeaders to require allowedDomains up front (since it was already semi-required).

It also changes to check the entry in allowedDomains (not against the hard coded example.com value).

It also expects only one entry in security.allowedDomains per hostname.

However there's a bit of a chicken and the egg problem here since the host check would benifit (I guess) from the protocol and port check to come before, but it's tricky to get those unless we have the host beforehand...

Testing

The relevant tests was updated to always include a x-forwarded-host header (otherwise we just return early).

I also had to add findRequestPort to node.ts to get the test prefers port from host test to pass. However - this was a hack to get the test to pass, and I'm reluctant to the solution in this pr. I did not found a way to reliable get the port out of the request object...

Docs

The docs are a bit confusing for checkOrigin.

It states that checkOrigin checks for pathname but I would expect, and it does, check origin.
And if the astro server is behind a reverse proxy, security.allowedDomains would be required to have a functional checkOrigin.

NOTE

A smaller PR would be to change the matchPattern into matchProtocol in the previous validate-headers.ts but this is something I realised after I refactored the function and made this bigger PR...

Here's the smaller PR: #15544

If this PR is considered to big/scary/buggy/bad/etc I can make a small pr with just that funcion call changed, and it would solve the "bug" for now.

However, I do think it's a bit scary to allow multiple entries on security.allowedDomains for a single hostname, and to check all of them for any matching host, protocol or port.
-> I think this PR has some value.

[changeset-bot]

🦋 Changeset detected

Latest commit: c4153ca

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codspeed-hq]

Merging this PR will not alter performance

✅ 9 untouched benchmarks

---

_{Comparing qzio:fix-issue-with-x-forwarded-header-and-checkorigin (978e6d3) with main (e959698)}

[matthewp]

We're talking about this on Discord, not ready yet for review.

[matthewp]

I do think I understand the issue you're wanting to fix better now. Here's my feedback:

1. Why did you have to add example.com to that many tests? Is it because previously we didn't require it but now we do? Just checking my expectations here.
2. Is there a new test that can be added that shows what happens when you don't have an X-Forwarded-Host? I assume it will just reject?
3. Can you add a test specifically for the checkOrigin condition?