Skip to content

docs: add explanation about redirect and Cookies/Headers #8165

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

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 7, 2023

Description

  • add explanation about redirect and Cookies/Headers

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the documentation Pull requests for documentation only label Nov 7, 2023
@kenjis
Copy link
Member Author

kenjis commented Nov 7, 2023

This is the current behavior,
but what would be the issues if we automatically copy them to RedirectResponse?

@MGatner
Copy link
Member

MGatner commented Nov 7, 2023

It seems like a possible security concern to pass forward all cookies & headers blindly. Because any library can force a redirect with RedirectException I think it is better to require explicit forwarding of those attributes.

@michalsn
Copy link
Member

michalsn commented Nov 7, 2023

This is the current behavior,
but what would be the issues if we automatically copy them to RedirectResponse?

There might be even simple issues with the flow. User can set a cookie but then decide not to include it when some sort of error occur and a redirect + error message is needed.

@kenjis kenjis merged commit 97e0292 into codeigniter4:develop Nov 7, 2023
@kenjis kenjis deleted the docs-redirect-with-cookie-header branch November 7, 2023 21:54
@kenjis
Copy link
Member Author

kenjis commented Nov 7, 2023

Yes, there is a valid point in explicit forwarding.
But it seems not easy to understand the behavior like #8163
In CI3 or pure PHP, setting cookies is global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests for documentation only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants