Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Correcting validation error #231

Merged
merged 3 commits into from
Oct 12, 2019
Merged

Conversation

adamoki
Copy link
Contributor

@adamoki adamoki commented May 29, 2018

Sometimes there is a problem with validation - when generated hash is valid integer (includes only digits - it rarely happens) - php converts that string to integer - then you get integer in array of hashes ($hashes). Comparing that integer to returned string from hash_file function results in false value while hash is the same but differs in variable type.

Sometimes there is a problem with validation - when generated hash is valid integer (includes only digits - it rarely happens) - php converts that string to integer - then you get integer in array of hashes ($hashes). Comparing that integer to returned string from hash_file function results in false value while hash is the same but differs in variable type.
Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@adamoki Please see my comment below.

Definitely we need add test case for it, and then provide a proper fix.

@@ -163,7 +163,7 @@ public function isValid($value, $file = null)
}

foreach ($hashes as $hash) {
if ($filehash === $hash) {
if ($filehash == $hash) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave it as it is and in the other place assure that $hash is always string.

@weierophinney weierophinney modified the milestone: 2.10.3 Dec 13, 2018
@weierophinney
Copy link
Member

We definitely need a unit test for this before we can merge. The unit test should fail under current master, and pass after the patch is applied.

I tend to agree with @webimpress - we should be casting hashes to strings when calling setHash() and/or in the constructor.

We were checking it when we providing one hash, but there
was no check for array:
```
$options = [
   'hash1',
   'hash2',
   // ...
   'algorithm' => '...',
];
```
@michalbundyra
Copy link
Member

@weierophinney I've pushed changes to this branch. I am checking now if input hash is string, and if not I am throwing an exception. We do have that check when we provide just a hash, without algorithm, but with algorithm we haven't had this check. Please see test I've added.

@adamoki Are you satisfied with the solution I've provided?

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Changes make sense to me. 🚢

@michalbundyra michalbundyra added this to the 2.12.1 milestone Oct 11, 2019
@michalbundyra
Copy link
Member

Thanks, @adamoki!

michalbundyra added a commit that referenced this pull request Oct 12, 2019
Correcting validation error
michalbundyra added a commit that referenced this pull request Oct 12, 2019
@michalbundyra michalbundyra merged commit 5f28b5a into zendframework:master Oct 12, 2019
michalbundyra added a commit that referenced this pull request Oct 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants