Skip to content

Fix design flaw in auth process.#703

Closed
dereuromark wants to merge 1 commit into3.xfrom
3.x-fix-design-flaw
Closed

Fix design flaw in auth process.#703
dereuromark wants to merge 1 commit into3.xfrom
3.x-fix-design-flaw

Conversation

@dereuromark
Copy link
Member

This seems to resolve #701

But I am not sure if thats the correct approach.
Either way, the rest of the stack must only be gone through once the persisting has happened again, otherwise the following code can be in a 5xx state.


if ($authenticator !== null && !$authenticator instanceof StatelessInterface) {
assert($result->getData() !== null);
$service->persistIdentity($request, new Response(), $result->getData());
Copy link
Member

@ADmad ADmad Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are ignoring the return value of persistIdentity(). When CookieAuthenticator is used the return value will contain the updated response instance with the header set for the cookie. So with your patch the cookie will never be set on the client.

Copy link
Member

@ADmad ADmad Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nor you can return the response instance at this point as returning a response from the middleware will short circuit the response handling by the framework.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, how can we fix this up to adhere to that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing that can be done here. PSR-15's middleware implementation doesn't support passing an updated response instance down the queue, it can only pass down the request.

@dereuromark dereuromark deleted the 3.x-fix-design-flaw branch April 18, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Design flaw - can 5xx the application

2 participants