Skip to content

feat(http)!: automatically encrypt cookies#1447

Merged
innocenzi merged 3 commits into2.xfrom
feat/encrypt-cookies
Aug 6, 2025
Merged

feat(http)!: automatically encrypt cookies#1447
innocenzi merged 3 commits into2.xfrom
feat/encrypt-cookies

Conversation

@innocenzi
Copy link
Member

@innocenzi innocenzi commented Aug 1, 2025

This PR builds on #1346 to automatically encrypt cookies. This PR does not include an option to have plain-text cookies.

What required the most attention with this PR was the internal tests—we now really need to have a SIGNING_KEY. For this reason, the tests' bootstrap calls key:generate under the hood.

An alternative could have been to add hardcoded configs (like tests/Integration/encryption.config.testing.php and tests/Integration/signing.config.testing.php), but the HTTP client tests start an actual server outside of the testing environment, to which I couldn't pass through the SIGNING_KEY environment variable.

@brendt
Copy link
Member

brendt commented Aug 4, 2025

Shouldn't we just force the SIGNING_KEY to be present at all times by checking it in the kernel and throwing an exception if it's not present.

Maybe even better, why not generate it automatically (as in: write it once to .env) if it's not set? Is that too much magic?

@innocenzi
Copy link
Member Author

Shouldn't we just force the SIGNING_KEY to be present at all times by checking it in the kernel and throwing an exception if it's not present.

Yeah, probably! I'll explore this.

Maybe even better, why not generate it automatically (as in: write it once to .env) if it's not set? Is that too much magic?

I kinda like this. The main issue I can see happening is that if we generate a signing key because it's missing in .env, things can get corrupted really quick if the missing key in .env was a mistake and there was already existing encrypted data

@innocenzi
Copy link
Member Author

Yeah, probably! I'll explore this.

Went to implement this and realized it doesn't make too much sense. Only tempest/http requires it right now for the cookies, so even though technically SIGNING_KEY would be always needed, I prefer having the logic of throwing at initialization time in the encrypter component

@brendt
Copy link
Member

brendt commented Aug 6, 2025

Ok, I'm good to merge if you are happy with it

@innocenzi innocenzi merged commit 039c38f into 2.x Aug 6, 2025
87 of 88 checks passed
@innocenzi innocenzi deleted the feat/encrypt-cookies branch August 6, 2025 11:45
@innocenzi
Copy link
Member Author

Nice!

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.

2 participants