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

Coding standards: Use WordPress-Extra ruleset to prevent potential security issues #18502

Open
mmtr opened this issue Nov 14, 2019 · 6 comments
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.

Comments

@mmtr
Copy link
Contributor

mmtr commented Nov 14, 2019

Is there any reason why Gutenberg is not following the recommended best practices provided by the WordPress-Extra ruleset for phpcs?

I enabled it on my local dev environment to check how many linting issues would it raise, and it seems it has a few, including some security issues like missing escaping and nonces.

See full log
FILE: /Users/mmtr/dev/gutenberg/bin/generate-gutenberg-php.php
----------------------------------------------------------------------
FOUND 5 ERRORS AND 3 WARNINGS AFFECTING 8 LINES
----------------------------------------------------------------------
 10 | WARNING | File operations should use WP_Filesystem methods
    |         | instead of direct PHP filesystem calls. Found:
    |         | fopen()
    |         | (WordPress.WP.AlternativeFunctions.file_system_read_fopen)
 22 | ERROR   | All output should be run through an escaping function
    |         | (see the Security sections in the WordPress Developer
    |         | Handbooks), found '"define( 'GUTENBERG_VERSION',
    |         | '$plugin_version' );\n"'.
    |         | (WordPress.Security.EscapeOutput.OutputNotEscaped)
 24 | WARNING | shell_exec() found. PHP system calls are often
    |         | disabled by server admins.
    |         | (WordPress.PHP.DiscouragedPHPFunctions.system_calls_shell_exec)
 26 | ERROR   | All output should be run through an escaping function
    |         | (see the Security sections in the WordPress Developer
    |         | Handbooks), found '"define( 'GUTENBERG_GIT_COMMIT',
    |         | '$git_commit' );\n"'.
    |         | (WordPress.Security.EscapeOutput.OutputNotEscaped)
 45 | ERROR   | All output should be run through an escaping function
    |         | (see the Security sections in the WordPress Developer
    |         | Handbooks), found '$line'.
    |         | (WordPress.Security.EscapeOutput.OutputNotEscaped)
 51 | ERROR   | All output should be run through an escaping function
    |         | (see the Security sections in the WordPress Developer
    |         | Handbooks), found '$line'.
    |         | (WordPress.Security.EscapeOutput.OutputNotEscaped)
 56 | ERROR   | All output should be run through an escaping function
    |         | (see the Security sections in the WordPress Developer
    |         | Handbooks), found '$line'.
    |         | (WordPress.Security.EscapeOutput.OutputNotEscaped)
 62 | WARNING | File operations should use WP_Filesystem methods
    |         | instead of direct PHP filesystem calls. Found:
    |         | fclose()
    |         | (WordPress.WP.AlternativeFunctions.file_system_read_fclose)
----------------------------------------------------------------------

FILE: /Users/mmtr/dev/gutenberg/bin/get-server-blocks.php

FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
13 | WARNING | error_reporting() can lead to full path
| | disclosure.
| | (WordPress.PHP.DevelopmentFunctions.prevent_path_disclosure_error_reporting)
13 | WARNING | error_reporting() found. Changing configuration
| | values at runtime is strongly discouraged.
| | (WordPress.PHP.DiscouragedPHPFunctions.runtime_configuration_error_reporting)
39 | WARNING | json_encode() is discouraged. Use wp_json_encode()
| | instead.
| | (WordPress.WP.AlternativeFunctions.json_encode_json_encode)

FILE: /Users/mmtr/dev/gutenberg/gutenberg.php

FOUND 6 ERRORS AFFECTING 6 LINES

14 | ERROR | Logical operator "or" is prohibited; use "||"
| | instead
| | (Squiz.Operators.ValidLogicalOperators.NotAllowed)
58 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $submenu
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)
64 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $submenu
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)
90 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '__'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
103 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
137 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found 'wp_create_nonce'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)

FILE: /Users/mmtr/dev/gutenberg/lib/widgets.php

FOUND 1 ERROR AFFECTING 1 LINE

71 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found 'wp_nonce_field'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)

FILE: /Users/mmtr/dev/gutenberg/lib/experiments-page.php

FOUND 6 ERRORS AFFECTING 5 LINES

19 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found ''.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
106 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$args'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
107 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$args'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
107 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$args'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
108 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$args'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
120 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '
'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)

FILE: /Users/mmtr/dev/gutenberg/lib/rest-api.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

33 | WARNING | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Recommended)

FILE: /Users/mmtr/dev/gutenberg/lib/client-assets.php

FOUND 2 ERRORS AND 7 WARNINGS AFFECTING 8 LINES

241 | WARNING | [x] "require_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
443 | WARNING | [ ] In footer ($in_footer) is not set explicitly
| | wp_enqueue_script; It is recommended to load
| | scripts in the footer. Please set this value to
| | true to load it in the footer, or explicitly
| | false if it should be loaded in the
| | header.
| | (WordPress.WP.EnqueuedResourceParameters.NotInFooter)
443 | ERROR | [ ] Resource version not set in call to
| | wp_enqueue_script(). This means new versions of
| | the script will not always be loaded due to
| | browser caching.
| | (WordPress.WP.EnqueuedResourceParameters.MissingVersion)
509 | ERROR | [ ] All output should be run through an escaping
| | function (see the Security sections in the
| | WordPress Developer Handbooks), found
| | '"$src|$filename\n"'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
534 | WARNING | [ ] File operations should use WP_Filesystem methods
| | instead of direct PHP filesystem calls. Found:
| | fclose()
| | (WordPress.WP.AlternativeFunctions.file_system_read_fclose)
537 | WARNING | [ ] File operations should use WP_Filesystem methods
| | instead of direct PHP filesystem calls. Found:
| | fopen()
| | (WordPress.WP.AlternativeFunctions.file_system_read_fopen)
538 | WARNING | [ ] File operations should use WP_Filesystem methods
| | instead of direct PHP filesystem calls. Found:
| | fwrite()
| | (WordPress.WP.AlternativeFunctions.file_system_read_fwrite)
539 | WARNING | [ ] File operations should use WP_Filesystem methods
| | instead of direct PHP filesystem calls. Found:
| | fclose()
| | (WordPress.WP.AlternativeFunctions.file_system_read_fclose)
606 | WARNING | [ ] file_get_contents() is discouraged. Use
| | wp_remote_get() for remote URLs instead.
| | (WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents)

PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

FILE: /Users/mmtr/dev/gutenberg/lib/demo.php

FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 3 LINES

18 | WARNING | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Recommended)
18 | WARNING | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Recommended)
34 | WARNING | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Recommended)
56 | WARNING | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Recommended)

FILE: ...ev/gutenberg/lib/class-experimental-wp-widget-blocks-manager.php

FOUND 4 ERRORS AND 1 WARNING AFFECTING 5 LINES

287 | WARNING | json_encode() is discouraged. Use wp_json_encode()
| | instead.
| | (WordPress.WP.AlternativeFunctions.json_encode_json_encode)
313 | ERROR | All output should be run through an escaping
| | function (see the Security sections in the WordPress
| | Developer Handbooks), found '$options'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
315 | ERROR | All output should be run through an escaping
| | function (see the Security sections in the WordPress
| | Developer Handbooks), found 'render_block'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
317 | ERROR | All output should be run through an escaping
| | function (see the Security sections in the WordPress
| | Developer Handbooks), found '$options'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
416 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $sidebars_widgets
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)

FILE: /Users/mmtr/dev/gutenberg/lib/customizer.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

35 | WARNING | json_encode() is discouraged. Use wp_json_encode()
| | instead.
| | (WordPress.WP.AlternativeFunctions.json_encode_json_encode)

FILE: /Users/mmtr/dev/gutenberg/lib/class-wp-block-styles-registry.php

FOUND 3 ERRORS AFFECTING 3 LINES

45 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$message'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
51 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$message'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
77 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '$message'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)

FILE: /Users/mmtr/dev/gutenberg/lib/template-loader.php

FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES

137 | WARNING | file_get_contents() is discouraged. Use
| | wp_remote_get() for remote URLs instead.
| | (WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents)
175 | ERROR | All output should be run through an escaping
| | function (see the Security sections in the WordPress
| | Developer Handbooks), found
| | 'wp_get_document_title'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)

FILE: ...r/dev/gutenberg/lib/class-wp-rest-block-directory-controller.php

FOUND 0 ERRORS AND 9 WARNINGS AFFECTING 9 LINES

97 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
98 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
99 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
100 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
169 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
170 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
171 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
172 | WARNING | [x] "include_once" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)
221 | WARNING | [x] "include" is a statement not a function; no
| | parentheses are required
| | (PEAR.Files.IncludingFile.BracketsNotRequired)

PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY

FILE: /Users/mmtr/dev/gutenberg/lib/compat.php

FOUND 1 ERROR AFFECTING 1 LINE

70 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $post
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)

FILE: ...gutenberg/packages/block-serialization-default-parser/parser.php

FOUND 12 ERRORS AND 2 WARNINGS AFFECTING 14 LINES

83 | ERROR | Visibility must be declared on method
| | "__construct" (Squiz.Scope.MethodScope.Missing)
100 | WARNING | Only one object structure is allowed in a file
| | (Generic.Files.OneObjectStructurePerFile.MultipleFound)
155 | ERROR | Visibility must be declared on method
| | "__construct" (Squiz.Scope.MethodScope.Missing)
172 | WARNING | Only one object structure is allowed in a file
| | (Generic.Files.OneObjectStructurePerFile.MultipleFound)
227 | ERROR | Visibility must be declared on method "parse"
| | (Squiz.Scope.MethodScope.Missing)
234 | ERROR | Empty DO statement detected
| | (Generic.CodeAnalysis.EmptyStatement.DetectedDo)
255 | ERROR | Visibility must be declared on method "proceed"
| | (Squiz.Scope.MethodScope.Missing)
291 | ERROR | The use of count() inside a loop condition is not
| | allowed; assign the return value to a variable and
| | use the variable in the loop condition instead
| | (Squiz.PHP.DisallowSizeFunctionsInLoops.Found)
401 | ERROR | Visibility must be declared on method
| | "next_token" (Squiz.Scope.MethodScope.Missing)
452 | ERROR | Empty IF statement detected
| | (Generic.CodeAnalysis.EmptyStatement.DetectedIf)
476 | ERROR | Visibility must be declared on method "freeform"
| | (Squiz.Scope.MethodScope.Missing)
488 | ERROR | Visibility must be declared on method
| | "add_freeform" (Squiz.Scope.MethodScope.Missing)
509 | ERROR | Visibility must be declared on method
| | "add_inner_block"
| | (Squiz.Scope.MethodScope.Missing)
530 | ERROR | Visibility must be declared on method
| | "add_block_from_stack"
| | (Squiz.Scope.MethodScope.Missing)

FILE: ...packages/block-serialization-default-parser/test/test-parser.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

15 | WARNING | json_encode() is discouraged. Use wp_json_encode()
| | instead.
| | (WordPress.WP.AlternativeFunctions.json_encode_json_encode)

FILE: ...mmtr/dev/gutenberg/packages/block-library/src/calendar/index.php

FOUND 4 ERRORS AFFECTING 4 LINES

28 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $monthnum
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)
30 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $year
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)
44 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $monthnum
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)
46 | ERROR | Overriding WordPress globals is prohibited. Found
| | assignment to $year
| | (WordPress.WP.GlobalVariablesOverride.Prohibited)

FILE: ...sers/mmtr/dev/gutenberg/packages/block-library/src/rss/index.php

FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES

33 | WARNING | strip_tags() is discouraged. Use the more
| | comprehensive wp_strip_all_tags() instead.
| | (WordPress.WP.AlternativeFunctions.strip_tags_strip_tags)
62 | WARNING | strip_tags() is discouraged. Use the more
| | comprehensive wp_strip_all_tags() instead.
| | (WordPress.WP.AlternativeFunctions.strip_tags_strip_tags)
71 | WARNING | This comment is 44% valid code; is this commented out
| | code? (Squiz.PHP.CommentedOutCode.Found)

FILE: ...s/mmtr/dev/gutenberg/packages/block-library/src/search/index.php

FOUND 1 ERROR AFFECTING 1 LINE

18 | ERROR | Increment and decrement operators must be bracketed
| | when used in string concatenation
| | (Squiz.Operators.IncrementDecrementUsage.NoBrackets)

FILE: ...tr/dev/gutenberg/packages/block-library/src/categories/index.php

FOUND 1 ERROR AFFECTING 1 LINE

77 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found 'home_url'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)

FILE: ...r/dev/gutenberg/packages/e2e-tests/plugins/wp-editor-metabox.php

FOUND 2 ERRORS AFFECTING 2 LINES

51 | ERROR | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Missing)
54 | ERROR | Processing form data without nonce verification.
| | (WordPress.Security.NonceVerification.Missing)

FILE: ...rg/packages/block-serialization-spec-parser/test/test-parser.php

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE

15 | WARNING | json_encode() is discouraged. Use wp_json_encode()
| | instead.
| | (WordPress.WP.AlternativeFunctions.json_encode_json_encode)

FILE: ...s/mmtr/dev/gutenberg/phpunit/class-extend-preload-paths-test.php

FOUND 2 ERRORS AFFECTING 2 LINES

23 | ERROR | Visibility must be declared on method
| | "test_localizes_script"
| | (Squiz.Scope.MethodScope.Missing)
35 | ERROR | Visibility must be declared on method
| | "test_replaces_registered_properties"
| | (Squiz.Scope.MethodScope.Missing)

FILE: ...mmtr/dev/gutenberg/phpunit/class-vendor-script-filename-test.php

FOUND 2 ERRORS AFFECTING 2 LINES

9 | ERROR | Visibility must be declared on method
| | "vendor_script_filename_cases"
| | (Squiz.Scope.MethodScope.Missing)
55 | ERROR | Visibility must be declared on method
| | "test_gutenberg_vendor_script_filename"
| | (Squiz.Scope.MethodScope.Missing)

FILE: /Users/mmtr/dev/gutenberg/phpunit/class-extend-styles-test.php

FOUND 5 ERRORS AND 2 WARNINGS AFFECTING 7 LINES

61 | WARNING | file_get_contents() is discouraged. Use
| | wp_remote_get() for remote URLs instead.
| | (WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents)
69 | WARNING | File operations should use WP_Filesystem methods
| | instead of direct PHP filesystem calls. Found:
| | file_put_contents()
| | (WordPress.WP.AlternativeFunctions.file_system_read_file_put_contents)
77 | ERROR | Visibility must be declared on method
| | "test_without_built_styles"
| | (Squiz.Scope.MethodScope.Missing)
95 | ERROR | Visibility must be declared on method
| | "test_unset_editor_settings_style"
| | (Squiz.Scope.MethodScope.Missing)
111 | ERROR | Visibility must be declared on method
| | "test_replace_default_editor_styles"
| | (Squiz.Scope.MethodScope.Missing)
138 | ERROR | Visibility must be declared on method
| | "test_replace_rearranged_default_editor_styles"
| | (Squiz.Scope.MethodScope.Missing)
165 | ERROR | Visibility must be declared on method
| | "test_without_default_editor_styles"
| | (Squiz.Scope.MethodScope.Missing)

FILE: /Users/mmtr/dev/gutenberg/phpunit/class-override-script-test.php

FOUND 5 ERRORS AFFECTING 5 LINES

9 | ERROR | Visibility must be declared on method "setUp"
| | (Squiz.Scope.MethodScope.Missing)
21 | ERROR | Visibility must be declared on method "tearDown"
| | (Squiz.Scope.MethodScope.Missing)
30 | ERROR | Visibility must be declared on method
| | "test_localizes_script"
| | (Squiz.Scope.MethodScope.Missing)
49 | ERROR | Visibility must be declared on method
| | "test_replaces_registered_properties"
| | (Squiz.Scope.MethodScope.Missing)
71 | ERROR | Visibility must be declared on method
| | "test_registers_new_script"
| | (Squiz.Scope.MethodScope.Missing)

FILE: /Users/mmtr/dev/gutenberg/post-content.php

FOUND 37 ERRORS AFFECTING 36 LINES

11 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
16 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
20 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
24 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
28 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
32 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
36 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
42 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
48 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
52 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
56 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
60 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
65 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
66 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
67 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
68 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
69 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
70 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
79 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
83 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
88 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
89 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
94 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
98 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
112 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
116 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
120 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
124 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
128 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
141 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
145 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
155 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
159 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
159 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
168 | ERROR | All output should be run through an escaping function
| | (see the Security sections in the WordPress Developer
| | Handbooks), found '__'.
| | (WordPress.Security.EscapeOutput.OutputNotEscaped)
177 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)
185 | ERROR | All output should be run through an escaping function
| | (like esc_html_e() or esc_attr_e()), found '_e'.
| | (WordPress.Security.EscapeOutput.UnsafePrintingFunction)

@mmtr mmtr added [Type] Enhancement A suggestion for improvement. [Type] Question Questions about the design or development of the editor. labels Nov 14, 2019
@mmtr mmtr changed the title Coding standards: Use WordPress-Extra ruleset Coding standards: Use WordPress-Extra ruleset to fix security issues Nov 14, 2019
@aduth
Copy link
Member

aduth commented Nov 14, 2019

cc @ntwb , @westonruter

Not really sure the history here as to whether it was an explicit choice. Based on some of the early pull requests (#617, #2914), I think there's been a pattern of gradual adoption of stricter rulesets.

I personally don't see any reason why we shouldn't consider adopting these additional rules.

@aduth aduth added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Nov 14, 2019
@westonruter
Copy link
Member

Yeah, the additional sniffs in WordPress-Extra help catch potential security issues. If the sniff marks code as a false positive, it can be suppressed with the phpcs:ignore comment, naturally.

@ntwb
Copy link
Member

ntwb commented Nov 17, 2019

What Weston said 👆🏼

@mmtr mmtr changed the title Coding standards: Use WordPress-Extra ruleset to fix security issues Coding standards: Use WordPress-Extra ruleset to prevent potential security issues Nov 18, 2019
@gziolo gziolo added Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts and removed [Type] Question Questions about the design or development of the editor. labels Dec 9, 2019
@momotofu momotofu self-assigned this Jan 22, 2020
@jrfnl
Copy link
Member

jrfnl commented Sep 28, 2022

After a discussion with @anton-vlasenko regarding issue #44151, I suggested the exact same thing: start using the WordPress-Extra ruleset, and in particular, make sure the PrefixAllGlobals sniff is activated.

I've done an initial pass at updating the ruleset and fixing up issues which would start to get flagged - either straight away or in the near future via WPCS 3.0.0 -.
The PRs I've pulled today are the result of that effort.

Even after those 11 PRs would be merged, there are still over 500 issues left to be fixed.

Some of the fixes may be simply a case of adding select excludes to the updated ruleset, either for a complete sniff or for a sniff in combination with certain files/directories.

In other cases, actual code improvements will need to be made and people more familiar with the Gutenberg code base than me, are better suited to make those fixes.

Please regard this as a call to action for people more familiar with the code base to start fixing these issues and/or to let me know about excludes which should be made.

If it helps, I can pull a draft PR with the proposed ruleset update already.

The below is a list of issues by error code which still need to be fixed/reviewed before the ruleset update is viable.

To see the underlying issues for any one of the sniffs, change <rule ref="WordPress-Core"/> in the phpcs.xml.dist file to <rule ref="WordPress-Extra"/>, and then run PHPCS like so:

vendor/bin/phpcs --sniffs=Standard.Category.SniffName

... where Standard.Category.SniffName is replaced with one of the sniffs mentioned in the below list.

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
--------------------------------------------------------------------------------------
SOURCE                                                                           COUNT
--------------------------------------------------------------------------------------
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound            260
WordPress.Security.EscapeOutput.OutputNotEscaped                                 53
WordPress.Security.EscapeOutput.UnsafePrintingFunction                           38
WordPress.Security.NonceVerification.Recommended                                 30
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound               26
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound            26
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound            20
WordPress.WP.GlobalVariablesOverride.Prohibited                                  11
WordPress.PHP.DevelopmentFunctions.error_log_trigger_error                       9
WordPress.WP.EnqueuedResourceParameters.NotInFooter                              9
WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedConstantFound            8
WordPress.Security.NonceVerification.Missing                                     6
WordPress.WP.AlternativeFunctions.json_encode_json_encode                        6
Squiz.PHP.CommentedOutCode.Found                                                 5
Generic.Files.OneObjectStructurePerFile.MultipleFound                            4
PHPCompatibility.Syntax.RemovedPartiallySupportedCallables.DeprecatedStatic      4
WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize                        4
WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents            4
WordPress.WP.AlternativeFunctions.strip_tags_strip_tags                          4
WordPress.WP.EnqueuedResourceParameters.MissingVersion                           3
Generic.CodeAnalysis.UnconditionalIfStatement.Found                              2
Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed                      1
Squiz.Operators.ValidLogicalOperators.NotAllowed                                 1
Squiz.PHP.DisallowSizeFunctionsInLoops.Found                                     1
Universal.Operators.DisallowLogicalAndOr.LogicalOr                               1
WordPress.NamingConventions.PrefixAllGlobals.DynamicHooknameFound                1
WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode                  1
WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize                      1
WordPress.PHP.DiscouragedPHPFunctions.system_calls_shell_exec                    1
WordPress.PHP.DiscouragedPHPFunctions.urlencode_urlencode                        1
WordPress.WP.AlternativeFunctions.file_system_read_fclose                        1
WordPress.WP.AlternativeFunctions.file_system_read_fopen                         1
WordPress.WP.AlternativeFunctions.file_system_read_readfile                      1
WordPress.WP.AlternativeFunctions.parse_url_parse_url                            1
WordPress.WP.AlternativeFunctions.rand_mt_rand                                   1
--------------------------------------------------------------------------------------
A TOTAL OF 546 SNIFF VIOLATIONS WERE FOUND IN 35 SOURCES
--------------------------------------------------------------------------------------

@anton-vlasenko
Copy link
Contributor

I've worked on fixing WordPress.NamingConventions.PrefixAllGlobals linter errors.
I submitted a PR that fixes lots of the issues related to that.
There should be 0 WordPress.NamingConventions.PrefixAllGlobals.* linter errors after adding the changes I'm listing below and merging my PR.
I propose to make the following changes to phpcs.xml.dist:

  1. Let's add the _gutenberg prefix to the list of allowed prefixes:

    gutenberg/phpcs.xml.dist

    Lines 83 to 90 in 4f350dc

    <rule ref="WordPress.NamingConventions.PrefixAllGlobals">
    <properties>
    <property name="prefixes" type="array">
    <element value="gutenberg"/>
    <element value="wp"/>
    </property>
    </properties>
    </rule>
  2. Let's add these rules to phpcs.xml.dist:
<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound">
		<exclude-pattern>lib/compat/*</exclude-pattern>
		<exclude-pattern>lib/experimental/*</exclude-pattern>
		<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
		<exclude-pattern>packages/*</exclude-pattern>
	</rule>

	<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound">
		<exclude-pattern>lib/compat/*</exclude-pattern>
		<exclude-pattern>lib/experimental/*</exclude-pattern>
		<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
		<exclude-pattern>packages/*</exclude-pattern>
	</rule>

	<rule ref="WordPress.NamingConventions.PrefixAllGlobals.ForbiddenPrefixPassed">
		<exclude-pattern>lib/compat/*</exclude-pattern>
		<exclude-pattern>lib/experimental/*</exclude-pattern>
		<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
		<exclude-pattern>packages/*</exclude-pattern>
	</rule>

	<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound">
		<exclude-pattern>lib/compat/*</exclude-pattern>
		<exclude-pattern>lib/experimental/*</exclude-pattern>
		<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
		<exclude-pattern>packages/*</exclude-pattern>
	</rule>

	<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound">
		<exclude-pattern>lib/compat/*</exclude-pattern>
		<exclude-pattern>lib/experimental/*</exclude-pattern>
		<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
		<exclude-pattern>packages/*</exclude-pattern>
	</rule>

	<rule ref="WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedConstantFound">
		<exclude-pattern>lib/compat/*</exclude-pattern>
		<exclude-pattern>lib/experimental/*</exclude-pattern>
		<exclude-pattern>bin/generate-gutenberg-php.php</exclude-pattern>
		<exclude-pattern>packages/*</exclude-pattern>
	</rule>

Let me explain the changes a bit.
lib/compat and lib/experimental folders should be excluded as they contain Core-ready code. It should be allowed to have any prefixes/hook names there. The code in these folders is going to be merged to Core anyway.

However, functions/classes in these folders should be guarded. There must be another sniff in place to check that since WordPress.NamingConventions.PrefixAllGlobals is not designed to do it. Please see #44151 for more details.

Let's exclude the packages/ folder for now. This code is being used under specific circumstances. I feel like a broader discussion is needed to decide the correct approach here.

bin/generate-gutenberg-php.php - this one should be refactored into a PHP class. Please see #46016.

@jrfnl Would it be possible to add these phpcs.xml.dist changes to your branch?

@azaozz
Copy link
Contributor

azaozz commented Nov 28, 2022

start using the WordPress-Extra ruleset

Not sure this is a good idea. Why should core code (Gutenberg is core) be using non-core rules/sniffs? This may even increase the complexity when merging the PHP code from GitHub to Trac.

make sure the PrefixAllGlobals sniff is activated.

Enabling only this rule seems sufficient to fix the problem. Still there seem to be false positives (that need exceptions that later need to be removed before merging to Trac).

Frankly I'm 50/50 on how useful PrefixAllGlobals is at this point, but perhaps would be nice to have yet another safeguard that is applied to patches/PRs early.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants