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

[Bug]: Two useless authtoken database queries for every anonymous request #42589

Closed
4 of 8 tasks
ChristophWurst opened this issue Jan 4, 2024 · 4 comments · Fixed by #42607
Closed
4 of 8 tasks

[Bug]: Two useless authtoken database queries for every anonymous request #42589

ChristophWurst opened this issue Jan 4, 2024 · 4 comments · Fixed by #42607

Comments

@ChristophWurst
Copy link
Member

⚠️ This issue respects the following points: ⚠️

Bug description

Reading query logs I noticed

 primary SELECT * FROM `oc_appconfig`
 replica SELECT * FROM `oc_authtoken` WHERE (`token` = :dcValue1) AND (`version` = :dcValue2)
 replica SELECT * FROM `oc_authtoken` WHERE (`token` = :dcValue1) AND (`version` = :dcValue2)

for every anonymous request.

The problem is that \OC\User\Session::tryTokenLogin tries to find a token for the current PHP session. \OC\Authentication\Token\PublicKeyTokenProvider::getToken does up to two lookups when the instance has an instance secret set. There will never be a hit. The only exception would be a hash collision of the new session ID and a previous one.

The solution would be to check if the request had sent a cookie with the instance id as name. Those are used for the PHP session. If there is no cookie, this is a new session, and there won't be a token.

Steps to reproduce

  1. curl https://localhost/login

Expected behavior

 primary SELECT * FROM `oc_appconfig`

Installation method

None

Nextcloud Server version

26

Operating system

None

PHP engine version

None

Web server

None

Database engine version

None

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other

Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

@solracsf
Copy link
Member

solracsf commented Jan 5, 2024

Could this fix it?

try {
$dbToken = $this->tokenProvider->getToken($token);
} catch (InvalidTokenException $e) {
// Can't really happen but better save than sorry
return true;
}

// Check if the request had sent a cookie with the instance id as name
// If there is no cookie, this is a new session

$instanceId = \OC::$server->getConfig()->getSystemValueString('instanceid');
if (!is_null($request->getCookie($instanceId))) {
	try {
		$dbToken = $this->tokenProvider->getToken($token);
	} catch (InvalidTokenException $e) {
		// Can't really happen but better save than sorry
		return true;
	}
}

@ChristophWurst
Copy link
Member Author

Yes, something like that should do the trick

@solracsf
Copy link
Member

solracsf commented Jan 5, 2024

Should i PR it? :)

@ChristophWurst
Copy link
Member Author

Yes please :)

Two suggestions:

  1. You have access to IConfig in Session. Use it instead of the service locator.
  2. Don't wrap the try-catch with an if but exit early with false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants