Skip to content
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

Exception thrown if Application does not have a password field #50497

Closed
Thiritin opened this issue Mar 13, 2024 · 8 comments · Fixed by #50507
Closed

Exception thrown if Application does not have a password field #50497

Thiritin opened this issue Mar 13, 2024 · 8 comments · Fixed by #50507
Assignees
Labels

Comments

@Thiritin
Copy link

Thiritin commented Mar 13, 2024

Laravel Version

11

PHP Version

8.3

Database Driver & Version

No response

Description

Referenced Code: ceb8ed2#r139707506

The previous code snippet makes a hash check, problem is my app does not have a password field this causes the previous method storePasswordHashInSession to set an empty password hash in the session, causing the hash_equals to throw the following exception.

hash_equals(): Argument #1 ($known_string) must be of type string, null given

Stackstace via Flare https://flareapp.io/share/J7oDeQZ5#context-request-browser

Steps To Reproduce

Create a new application and remove the password field via the migration

@nalewis75
Copy link

I have the same problem as I don't use a password for authentication. Overrode the getAuthPasswordName method in my User model seemed to work if I returned email from it.

public function getAuthPasswordName()
{
    return "email";
}

Not sure if this is a good approach or not but work for now.

@Thiritin
Copy link
Author

Thiritin commented Mar 13, 2024

I have the same problem as I don't use a password for authentication. Overrode the getAuthPasswordName method in my User model seemed to work if I returned email from it.

public function getAuthPasswordName()
{
    return "email";
}

Not sure if this is a good approach or not but work for now.

image
On the user Model also works in my case, but it will just treat the new id as the password which technically should not be needed at all.

@driesvints
Copy link
Member

cc @valorin

@driesvints driesvints added the bug label Mar 13, 2024
@driesvints
Copy link
Member

@valorin I'm not sure what the behaviour should be if there's an empty password. Should we logout the user in that case?

@valorin
Copy link
Contributor

valorin commented Mar 13, 2024

The problem is the password_hash_* session key is being set, even without a password field on the record. There is no point setting this at all if there is no password, so I think we need a way to disable that entirely - and detect when there is no password for backwards compatibility. At the moment the code just assumes there is a password and sets the key - it hashes an empty string fine, but cannot compare an empty string.

I'll work on a possible fix.

@driesvints
Copy link
Member

Thanks @valorin

@valorin
Copy link
Contributor

valorin commented Mar 13, 2024

Alright, I've dived deeper into this and it seems to be caused by the \Illuminate\Session\Middleware\AuthenticateSession middleware, whose sole purpose is to log out any other sessions when the user changes their password. It's also optional and disabled by default.

I'm having trouble replicating the issue in 11 after enabling it, but I'll keep trying. However, I suspect the issue is actually that this middleware shouldn't be used when you're not using a password.

Is there a reason why you're using this middleware? Or can you replicate this issue without this middleware enabled?

@valorin
Copy link
Contributor

valorin commented Mar 13, 2024

Alright, I replicated the issue and it's definitely that middleware. I've made a PR to silence the issue: #50507

The other fix is to disable the middleware. That could go in the upgrade guide - this middleware is not useful without a password on the user record.

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 a pull request may close this issue.

4 participants