search

bakerkretzmar / laravel-deploy-preview

A GitHub Action to deploy PR preview sites for Laravel apps.
MIT License
19 stars 6 forks source link

Set site names to be lowercase #32

Closed GlitchWitch closed 10 months ago

GlitchWitch commented 10 months ago

When normalising site names as discussed in #22, they should also be set to lowercase. This will prevent mismatches between the actual domain name and site name.

bakerkretzmar commented 10 months ago

mismatches between the actual domain name and site name

Can you be more specific about when and how this happens? I just tried this and it looks like the site name in Forge can include uppercase letters, they persist into the directory name and Nginx config and everything. Is that bad?

GlitchWitch commented 10 months ago

I'd argue that uppercase letters should not be used here at all.

Specifically domain names are case insensitive, and thus default to lowercase for all other systems (browsers, cookies, headers, dns, etc).

Since the deploy preview uses the domain name for the forge site name, it should also ensure it's lower case to make sure both match.

As an example edge case where this becomes a problem is if a user has the following config:

## Session config
SESSION_DRIVER="database"
SESSION_LIFETIME="120"
SESSION_SECURE_COOKIE="true"
SESSION_DOMAIN="${APP_DOMAIN}"
SANCTUM_STATEFUL_DOMAINS="${APP_DOMAIN}"

If APP_DOMAIN includes uppercase characters, this will cause breakage due to cookies storing the domain in lowercase.

We ran into a few other edge cases like this, and the easiest solution is to just use lowercase for the forge site name to prevent issues like these and align with all external systems and how they typically handle domain names.

bakerkretzmar commented 10 months ago

Gotcha, thanks for that context 👍🏻 will do!