enforce configuration of Safe Browsing API key (#104) #108

stklcode · 2021-04-11T10:27:06Z

Second stage of #104 and follow-up to #105.

The warning that will be introduces in 1.4.3 (#105) is now an error. On plugin activation Safe Browsing will be deactivated, if no key is provided. In the settings UI, the field is not "required" (should be no problem, because is it also "disabled" if the checkbox is unchecked) and if for whatever reason the configuration is saved without a key, an error will be displayed.

If again for any reason the check is triggered without a key, the fallback is no longer be used and the check is skipped.

patrickrobrecht · 2021-05-28T11:08:21Z

inc/class-antivirus.php

+		if ( self::_get_option( 'safe_browsing' ) && empty( $safe_browsing_key ) ) {
+			self::_update_option( 'safe_browsing', 0 );
+			set_transient( 'antivirus-activation-notice', true, 2592000 );
+		}


In my manual tests the admin notice was never shown. When (re)activating AntiVirus, all options are cleared in ll. 138-143. Therefore the condition in l. 152 can never be true, right?

This check be run independent of activation, shouldn't it? I. e. after the update or in the check_safe_browsing() method itself? Then we even cover cases such as updating incl. the correction + warning and importing a DB backup afterwards - check is repeated and results in showing the warning again.

I'll take a look at the implementation again, possibly something got lost or is still incorrect here.

This check be run independent of activation, shouldn't it? I. e. after the update

The check should be run in several places. The one here in the activation hook should be triggered right after plugin activation and as plugins are reactivated after an update, at least once after the update and should notify the administrator that the check has been disabled (if it was enabled without API key before). In this place we set a transient object because the update might be triggered asynchronously so the admin will be informed on the next visit.

Check is also performed when saving settings and right before the check itself, if for any reason the options are stored incorrectly.

When (re)activating AntiVirus, all options are cleared in ll. 138-143.

That's not correct. Options should and must not be cleared on plugin activation, otherwise everything would be reset on updates or reactivation for whatever reason.

From the add_option() reference:

Existing options will not be updated and checks are performed to ensure that you aren’t adding a protected WordPress option.

Works for me in the intended way:

save settings with SB enabled and no API key in v1.4.3 (raises a warning)

update plugin to 1.5.0

deactivate and activate the plugin (done automatically when updating the regular way)

warning is shown once that SB has been disabled

screenshot (animated)

warning is shown once that SB has been disabled

Could this be displayed until it is actively dismissed? Otherwise it could easily be missed ...

codecov-commenter · 2021-07-21T08:36:11Z

Codecov Report

Patch coverage: 10.71% and project coverage change: -0.66% ⚠️

Comparison is base (e55943c) 35.93% compared to head (4d7e3be) 35.27%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #108      +/-   ##
=============================================
- Coverage      35.93%   35.27%   -0.66%     
+ Complexity       151      146       -5     
=============================================
  Files              5        5              
  Lines            782      788       +6     
=============================================
- Hits             281      278       -3     
- Misses           501      510       +9

Files Changed	Coverage Δ
antivirus.php	`0.00% <ø> (ø)`
inc/class-antivirus.php	`17.00% <0.00%> (-0.46%)`	⬇️
inc/class-antivirus-safebrowsing.php	`98.61% <100.00%> (+4.94%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2023-09-17T09:59:28Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

Zodiac1978

Looks okay to me, but needs testing aftwerwards.

Zodiac1978 · 2023-10-31T14:30:15Z

Merging is blocked because @patrickrobrecht requested changes. Maybe he can look at it again. Otherwise, I can merge without those changes if necessary.

Zodiac1978 · 2023-11-03T07:44:02Z

Looks like $screen = get_current_screen(); can be removed, because $screen is not used anymore with this PR.

And additionally the comment seems confusing:

Add admin notice to users who can manage options, Safe Browsing has been disabled without custom API key.

Maybe changing it to makes it clearer:

Show admin notice to users who can manage options that Safe Browsing has been disabled because custom API key is missing.

The built-in fallback API key must not be used anymore. Enforce configuration of a custom API key and deactivate the feature, if none is provided.

sonarqubecloud · 2023-11-03T08:35:31Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

3.8% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

stklcode added this to the 1.5.0 milestone Apr 11, 2021

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch 2 times, most recently from 46caab0 to e12e543 Compare April 11, 2021 10:40

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from e12e543 to 27cc83a Compare May 15, 2021 08:01

stklcode marked this pull request as ready for review May 26, 2021 07:39

patrickrobrecht requested changes May 28, 2021

View reviewed changes

Zodiac1978 linked an issue Jul 20, 2021 that may be closed by this pull request

Remove fallback API key for Google SafeBrowsing API #104

Closed

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from b5c312f to baf3941 Compare May 11, 2022 18:48

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from baf3941 to 329d258 Compare September 10, 2022 08:18

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from 329d258 to aad7d2c Compare February 9, 2023 11:56

stklcode added the enhancement label Mar 27, 2023

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from aad7d2c to 4d7e3be Compare September 17, 2023 09:48

pluginkollektiv deleted a comment from codecov-io Sep 17, 2023

Zodiac1978 approved these changes Oct 31, 2023

View reviewed changes

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from 4d7e3be to 3a059a8 Compare October 31, 2023 15:04

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from 0cbc229 to 043408b Compare November 3, 2023 08:32

enforce configuration of Safe Browsing API key

ef265c9

The built-in fallback API key must not be used anymore. Enforce configuration of a custom API key and deactivate the feature, if none is provided.

stklcode force-pushed the feature/104-enforce-safebrowsing-key branch from 043408b to ef265c9 Compare November 3, 2023 08:34

Zodiac1978 merged commit 2b26f66 into develop Nov 3, 2023

stklcode deleted the feature/104-enforce-safebrowsing-key branch November 3, 2023 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enforce configuration of Safe Browsing API key (#104) #108

enforce configuration of Safe Browsing API key (#104) #108

stklcode commented Apr 11, 2021 •

edited

Loading

patrickrobrecht May 28, 2021

stklcode May 28, 2021 •

edited

Loading

stklcode May 28, 2021

Zodiac1978 Jul 21, 2021

codecov-commenter commented Jul 21, 2021 •

edited

Loading

sonarqubecloud bot commented Sep 17, 2023

Zodiac1978 left a comment

Zodiac1978 commented Oct 31, 2023

Zodiac1978 commented Nov 3, 2023

sonarqubecloud bot commented Nov 3, 2023

enforce configuration of Safe Browsing API key (#104) #108

enforce configuration of Safe Browsing API key (#104) #108

Conversation

stklcode commented Apr 11, 2021 • edited Loading

patrickrobrecht May 28, 2021

Choose a reason for hiding this comment

stklcode May 28, 2021 • edited Loading

Choose a reason for hiding this comment

stklcode May 28, 2021

Choose a reason for hiding this comment

Zodiac1978 Jul 21, 2021

Choose a reason for hiding this comment

codecov-commenter commented Jul 21, 2021 • edited Loading

Codecov Report

sonarqubecloud bot commented Sep 17, 2023

Zodiac1978 left a comment

Choose a reason for hiding this comment

Zodiac1978 commented Oct 31, 2023

Zodiac1978 commented Nov 3, 2023

sonarqubecloud bot commented Nov 3, 2023

stklcode commented Apr 11, 2021 •

edited

Loading

stklcode May 28, 2021 •

edited

Loading

codecov-commenter commented Jul 21, 2021 •

edited

Loading