Ensure that the hash method does not return null by artonge · Pull Request #46218 · nextcloud/server

artonge · 2024-07-01T12:27:08Z

To match:

server/lib/private/Files/View.php

Line 1050 in beececf

public function hash($type, $path, $raw = false): string|bool {

artonge · 2024-07-01T12:28:38Z

/backport to stable29

artonge · 2024-07-01T12:28:40Z

/backport to stable28

artonge · 2024-07-01T12:29:18Z

/backport to stable27

lib/private/Files/Storage/Local.php

come-nc · 2024-07-01T14:45:44Z

lib/private/Files/Storage/Local.php

-	public function hash($type, $path, $raw = false) {
-		return hash_file($type, $this->getSourcePath($path), $raw);
+	public function hash($type, $path, $raw = false): string|false {
+		return hash_file($type, $this->getSourcePath($path), $raw) ?? false;


https://www.php.net/manual/en/function.hash-file.php says that the method never returns null, is that wrong?

lib/private/Files/Storage/Wrapper/Availability.php may return null from hash().
lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php also but I am not sure it’s the same hash.

lib/private/Files/Storage/Wrapper/Availability.php may return null from hash().

Where do you see that?

server/lib/private/Files/Storage/Wrapper/Availability.php

Lines 331 to 339 in 8dcf933

public function hash($type, $path, $raw = false) {

$this->checkAvailability();

try {

return parent::hash($type, $path, $raw);

} catch (StorageNotAvailableException $e) {

$this->setUnavailable($e);

}

}

I am also surprised that hash_file returns null even though the doc says otherwise.

lib/private/Security/Bruteforce/Backend/MemoryCacheBackend.php also but I am not sure it’s the same hash.

Indeed, I don't think this is the same hash.

In the catch branch there is no return

Do you have evidence that hash_file returns null apart from this stacktrace? If this is true it should be reported to the PHP project.

In the catch branch there is no return

Right, missed that 🙈. I pushed a fixed to add a proper return value.
I will also investigate where does the null value come from.

solracsf · 2024-07-01T16:03:58Z

/backport to stable27

Should this be backported to (eol) 27?

come-nc · 2024-07-02T14:48:48Z

lib/private/Files/Storage/Local.php

+		if ($hash === null) {
+			return false;
+		}


I read the C code of the PHP function and I really see no possibility for hash_file to return null.
https://github.com/php/php-src/blob/master/ext/hash/hash.c#L460
points to
https://github.com/php/php-src/blob/master/ext/hash/hash.c#L358
It returns false, string or throws.

I would suggest to remove this null check, only keep the fix in lib/private/Files/Storage/Wrapper/Availability.php and the strong return type here, and merge that.

lib/private/Files/Storage/Local.php

To match https://github.com/nextcloud/server/blob/beececf66068f57c416225efcde9b44ce5c2e835/lib/private/Files/View.php#L1050 - Fix #44110 Signed-off-by: Louis Chemineau <[email protected]>

Co-authored-by: Ferdinand Thiessen <[email protected]> Signed-off-by: John Molakvoæ <[email protected]>

solracsf · 2024-09-16T17:07:25Z

/backport to stable30

artonge requested review from come-nc and icewind1991 July 1, 2024 12:28

artonge self-assigned this Jul 1, 2024

artonge added bug 3. to review Waiting for reviews feature: files php Pull requests that update Php code labels Jul 1, 2024

artonge added this to the Nextcloud 30 milestone Jul 1, 2024

backportbot bot added the backport-request label Jul 1, 2024

artonge force-pushed the artonge/fix/hash_return_type branch from 4896b63 to 4269166 Compare July 1, 2024 12:30

github-advanced-security bot found potential problems Jul 1, 2024

View reviewed changes

icewind1991 approved these changes Jul 1, 2024

View reviewed changes

artonge force-pushed the artonge/fix/hash_return_type branch from 4269166 to ae8c728 Compare July 1, 2024 14:52

solracsf approved these changes Jul 1, 2024

View reviewed changes

artonge force-pushed the artonge/fix/hash_return_type branch from ae8c728 to 3a61867 Compare July 2, 2024 09:39

come-nc reviewed Jul 2, 2024

View reviewed changes

solracsf mentioned this pull request Jul 29, 2024

fix(files): Don't try to hash invalid paths #46845

Closed

4 tasks

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024

susnux reviewed Sep 6, 2024

View reviewed changes

lib/private/Files/Storage/Local.php Outdated Show resolved Hide resolved

skjnldsv approved these changes Sep 16, 2024

View reviewed changes

skjnldsv enabled auto-merge September 16, 2024 09:32

susnux approved these changes Sep 16, 2024

View reviewed changes

artonge and others added 2 commits September 16, 2024 15:07

fix(files): Ensure that the hash method does not return null

6674f79

To match https://github.com/nextcloud/server/blob/beececf66068f57c416225efcde9b44ce5c2e835/lib/private/Files/View.php#L1050 - Fix #44110 Signed-off-by: Louis Chemineau <[email protected]>

chore: improve hash_file php usage in Local Storage

9acaf07

Co-authored-by: Ferdinand Thiessen <[email protected]> Signed-off-by: John Molakvoæ <[email protected]>

skjnldsv force-pushed the artonge/fix/hash_return_type branch from b392f5d to 9acaf07 Compare September 16, 2024 13:07

skjnldsv merged commit dde0b48 into master Sep 16, 2024

skjnldsv deleted the artonge/fix/hash_return_type branch September 16, 2024 15:15

This was referenced Sep 16, 2024

[stable27] Ensure that the hash method does not return null #48102

Merged

[stable28] Ensure that the hash method does not return null #48103

Merged

[stable29] Ensure that the hash method does not return null #48104

Merged

backportbot bot removed the backport-request label Sep 16, 2024

backportbot bot added the backport-request label Sep 16, 2024

backportbot bot mentioned this pull request Sep 16, 2024

[stable30] Ensure that the hash method does not return null #48109

Merged

backportbot bot removed the backport-request label Sep 16, 2024

icewind1991 mentioned this pull request Sep 25, 2024

Ensure that the hash method doesnt return null #46249

Closed

skjnldsv mentioned this pull request Jan 7, 2025

31.0.0 beta 1 #50066

Merged

	public function hash($type, $path, $raw = false) {
	$this->checkAvailability();
	try {
	return parent::hash($type, $path, $raw);
	} catch (StorageNotAvailableException $e) {
	$this->setUnavailable($e);
	}
	}

Uh oh!

Conversation

artonge commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artonge commented Jul 1, 2024

Uh oh!

artonge commented Jul 1, 2024

Uh oh!

artonge commented Jul 1, 2024

Uh oh!

Uh oh!

Check failure

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solracsf commented Jul 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

solracsf commented Sep 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

artonge commented Jul 1, 2024 •

edited

Loading