X Tutup
Skip to content

Fix invalid foreach in SharingBlacklist.php#36038

Merged
sharidas merged 1 commit intomasterfrom
invalid-foreach-SharingBlacklist
Sep 10, 2019
Merged

Fix invalid foreach in SharingBlacklist.php#36038
sharidas merged 1 commit intomasterfrom
invalid-foreach-SharingBlacklist

Conversation

@phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Aug 15, 2019 •
edited
Loading

Description

The "string" value that it is set to is supposed to be valid JSON representing an array of group ids.

blacklisted_receiver_groups can be set "manually" by the administrator using an occ command, or by some automated test or... So it can end up being an empty string, or some string that is not valid JSON, or some string that represents a multi-level nested array-of-arrays, or some other weird structure.

  1. If the JSON decode does not produce an array, then return an empty list. (JSON decode might have failed, or returned a plain string/int etc)

  2. Check the elements of the decoded array. Keep elements that are "string-like" (string, int and float) and cast them to string. Discard other elements (e.g. an array, or boolean). This allows for group names like "0", "1", "1.23", "1.23e-4" that can look like both numbers and strings, while discarding other crud.

Related Issue

How Has This Been Tested?

Local run of acceptance test and observe that the log message no longer happens.

make test-acceptance-webui BEHAT_FEATURE=tests/acceptance/features/webUISharingInternalGroups/shareWithGroups.feature:239

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@phil-davis phil-davis self-assigned this Aug 15, 2019
phil-davis
phil-davis commented Aug 15, 2019
View reviewed changes
@codecov
Copy link

codecov bot commented Aug 15, 2019 •
edited
Loading

Codecov Report

Merging #36038 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #36038   +/-   ##
=======================================
  Coverage   54.01%   54.01%           
=======================================
  Files          63       63           
  Lines        7404     7404           
  Branches     1309     1309           
=======================================
  Hits         3999     3999           
  Misses       3019     3019           
  Partials      386      386
Flag Coverage Δ
#javascript 54.01% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39a6ce8...36d978c. Read the comment docs.

jvillafanez
jvillafanez reviewed Aug 16, 2019
View reviewed changes
@PVince81
Copy link
Contributor

PVince81 commented Aug 23, 2019

@phil-davis any update ?

@phil-davis
Copy link
Contributor Author

phil-davis commented Aug 23, 2019

@phil-davis any update ?

I thought it was going to be an easy/obvious fix - silly me. Now I need to spend some time "hardening" against this setting being potentially any invalid JSON or weird JSON.

@phil-davis phil-davis force-pushed the invalid-foreach-SharingBlacklist branch from c0d3e4b to 0bf22e0 Compare August 23, 2019 09:13
@phil-davis
Copy link
Contributor Author

phil-davis commented Aug 23, 2019

@jvillafanez @PVince81 I switched around the logic so that the code is in the private method fetchBlacklistedReceiverGroupIds rather than previously, where I had the private method calling the public method.

I added a more general check using json_last_error and report the problem as an error log. But I go on by setting the group ids empty.

Please review.

jvillafanez
jvillafanez reviewed Aug 23, 2019
View reviewed changes
@jvillafanez
Copy link
Member

jvillafanez commented Aug 23, 2019

Note that this needs unittests to cover this new case.

@phil-davis phil-davis force-pushed the invalid-foreach-SharingBlacklist branch 3 times, most recently from ab44629 to e0154c1 Compare September 4, 2019 06:41
@phil-davis phil-davis requested a review from PVince81 September 4, 2019 06:42
@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 4, 2019

@jvillafanez or anybody - ready for review again.
I went crazy and covered whatever crappy things I thought someone might be able to set this to.

jvillafanez
jvillafanez reviewed Sep 4, 2019
View reviewed changes
@phil-davis phil-davis force-pushed the invalid-foreach-SharingBlacklist branch 2 times, most recently from d7c5ea7 to 8f1df37 Compare September 6, 2019 10:15
phil-davis
phil-davis commented Sep 6, 2019
View reviewed changes
phil-davis
phil-davis commented Sep 6, 2019
View reviewed changes
@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 6, 2019

@jvillafanez I refactored stuff - please review again.

jvillafanez
jvillafanez reviewed Sep 6, 2019
View reviewed changes
Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

one last thing and we're done

@phil-davis phil-davis force-pushed the invalid-foreach-SharingBlacklist branch from 8f1df37 to 36d978c Compare September 6, 2019 14:22
@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 6, 2019

@jvillafanez ready for review again

@sharidas
Copy link
Contributor

sharidas commented Sep 10, 2019

@phil-davis I am merging this change.

@sharidas sharidas merged commit fab02c4 into master Sep 10, 2019
@delete-merged-branch delete-merged-branch bot deleted the invalid-foreach-SharingBlacklist branch September 10, 2019 11:13
@micbar micbar mentioned this pull request Sep 16, 2019
Closed
13 tasks
@HanaGemela HanaGemela mentioned this pull request Oct 1, 2019
Closed
@HanaGemela
Copy link
Contributor

HanaGemela commented Oct 2, 2019

Cannot share to blacklisted group with group names like "0", "1", "1.23", "1.23e-4" on the desktop sync client 2.6.0 RC1, macOS 10.14.6 and testpilot 2.5.4

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

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@PVince81 PVince81 Awaiting requested review from PVince81

Assignees

@phil-davis phil-davis

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Invalid argument supplied for foreach SharingBlacklist.php

5 participants

@phil-davis @PVince81 @jvillafanez @sharidas @HanaGemela
X Tutup