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

Improve output escaping, input sanitizing and logging #252

Draft
wants to merge 49 commits into
base: trunk
Choose a base branch
from

Conversation

gudmdharalds
Copy link
Contributor

@gudmdharalds gudmdharalds commented Feb 28, 2022

This pull request aims at improving output escaping, input sanitizing and logging.

TODO:

  • Escape inauto-approval.php:
    • Escape/sanitize labels
  • Escape messages submitted to GitHub via functions in reports.php, such as:
    • Line numbers
    • Commit-IDs
    • Text messages
  • Escape parameters in functions in github-api.php:
    • commit-ID in vipgoci_github_pr_comments_generic_submit()
    • $dismiss_message in vipgoci_github_pr_review_dismiss()
    • $label_name in vipgoci_github_label_add_to_pr()
    • $description in vipgoci_github_status_create()
  • Sanitize input
    • Sanitize $option parameters, removing unexpected values
      • Possibly via the functions that process them during startup
    • In wpscan-scan.php, only keep fields from WPScan API results that are needed. This includes both the general result, such as latest version, and also vulnerability results (i.e. id, title, cvss fields). This logic could be implemented here.
  • Sanitize parameters in functions
    • In github-api.php
      • $state and $context in vipgoci_github_status_create()
    • Sanitize HTTP header values sent in requests -- remove any ASCII characters that are outside of 32 to 127 values (decimal). cURL will not escape by default.
  • Escape output:
    • Refine vipgoci_output_markdown_escape() so to escape >, < and & correctly. Current escaping will not result in user-friendly output, though it is safe.
      • Apply function everywhere where Markdown is outputted.
  • Escape logged messages
  • Can some functions from WordPress be re-used for sanitizing/escaping?
  • Improve logged messages
    • Add space to URL logged in main.php (in Starting scanning PRs;)
    • Add URL to vip-go-ci in reports.php
  • Add or update PHPDoc comments for new or updated functions (main code only; for VIP)
  • Document escaping and sanitizing strategy in CONTRIBUTING.md
  • Changelog entry (for VIP)
  • Add/update unit-tests (if needed)
  • Check automated unit-tests

Copy link
Collaborator

@wpcomvip-vipgoci-bot wpcomvip-vipgoci-bot left a comment

Choose a reason for hiding this comment

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

phpcs scanning turned up:

🚫 5 errors


This bot provides automated PHP linting and PHPCS scanning. For more information about the bot and available customizations, see our documentation.


Scan run detail

Software versions

  • vip-go-ci version: 1.2.0
  • PHP runtime version for vip-go-ci: 8.1.3
  • PHP runtime version for PHP linting: 7.4.28
  • PHP runtime version for PHPCS: 7.4.28
  • PHPCS version: 3.6.2

Options file (.vipgoci_options)

Options file enabled: true

Configurable options:

  • skip-execution
  • skip-draft-prs
  • lint-modified-files-only
  • phpcs
  • phpcs-severity
  • phpcs-sniffs-include
  • phpcs-sniffs-exclude
  • report-no-issues-found
  • review-comments-sort
  • review-comments-include-severity
  • post-generic-pr-support-comments
  • review-comments-sort
  • scan-details-msg-include
  • svg-checks
  • autoapprove
  • autoapprove-php-nonfunctional-changes
  • hashes-api

Options altered:

  • phpcs-severityset to1
  • phpcs-sniffs-includeset toGeneric.PHP.DisallowShortOpenTag, Squiz.PHP.CommentedOutCode
  • phpcs-sniffs-excludeset toWordPress.Security.EscapeOutput, WordPress.PHP.DevelopmentFunctions, WordPress.WP.AlternativeFunctions, WordPress.PHP.DiscouragedPHPFunctions, WordPress.Files.FileName, Squiz.Commenting.FileComment, Generic.PHP.Syntax
  • skip-draft-prsset to

PHP lint options

PHP lint files enabled: true

Lint modified files only: true

Directories not PHP linted:

  • None

SVG configuration

SVG scanning enabled: true

Auto-approval configuration

Auto-approvals enabled: true

Non-functional changes auto-approved: true

Auto-approval DB enabled: true

Auto-approved file-types:

  • css
  • csv
  • eot
  • gif
  • gz
  • ico
  • ini
  • jpeg
  • jpg
  • json
  • less
  • map
  • md
  • mdown
  • mo
  • mp4
  • otf
  • pcss
  • pdf
  • po
  • pot
  • png
  • sass
  • scss
  • styl
  • ttf
  • txt
  • woff
  • woff2
  • yml

PHPCS configuration

PHPCS scanning enabled: true

PHPCS severity level: 1

Standard(s) used:

  • PHPCompatibility
  • PHPCompatibilityParagonieRandomCompat
  • PHPCompatibilityParagonieSodiumCompat
  • VariableAnalysis
  • WordPress

Runtime set:

  • testVersion 7.4-

Custom sniffs included:

  • Generic.PHP.DisallowShortOpenTag
  • Squiz.PHP.CommentedOutCode

Custom sniffs excluded:

  • WordPress.Security.EscapeOutput
  • WordPress.PHP.DevelopmentFunctions
  • WordPress.WP.AlternativeFunctions
  • WordPress.PHP.DiscouragedPHPFunctions
  • WordPress.Files.FileName
  • Squiz.Commenting.FileComment
  • Generic.PHP.Syntax

Directories not PHPCS scanned:

  • tests/unit

auto-approval.php Outdated Show resolved Hide resolved
auto-approval.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
@gudmdharalds gudmdharalds added this to the 1.2.1 milestone Feb 28, 2022
@gudmdharalds gudmdharalds changed the title Improve output escaping. Improve output escaping Feb 28, 2022
@wpcomvip-vipgoci-bot wpcomvip-vipgoci-bot dismissed their stale review February 28, 2022 20:50

Dismissing review as all inline comments are obsolete by now

@gudmdharalds gudmdharalds modified the milestones: 1.2.1, 1.2.2, 1.2.3 Mar 7, 2022
@gudmdharalds gudmdharalds removed this from the 1.2.3 milestone Mar 21, 2022
@gudmdharalds gudmdharalds changed the title Improve output escaping Improve output escaping and logging Mar 23, 2022
@gudmdharalds gudmdharalds marked this pull request as draft April 4, 2022 12:18
@gudmdharalds gudmdharalds changed the title Improve output escaping and logging Improve output escaping, input sanitizing and logging Jun 7, 2022
@wpcomvip-vipgoci-bot
Copy link
Collaborator

No issues were found to report when scanning latest commit (commit-ID: 9c379f4)


This bot provides automated PHP linting, PHPCS scanning and Vulnerability and Update Scan. For more information about the bot and available customizations, see our documentation.



Scan run detail

Software versions

  • vip-go-ci version: 1.3.9
  • PHP runtime version for vip-go-ci: 8.2.11
  • PHP runtime for linting:
    • PHP 8.1: 8.1.24
  • PHP runtime version for PHPCS: 8.2.11
  • PHPCS version: 3.7.2
  • PHP runtime version for SVG scanner: 7.4.33

Options file (.vipgoci_options)

Options file enabled: true

Configurable options:

  • skip-execution
  • skip-draft-prs
  • lint-modified-files-only
  • phpcs
  • phpcs-severity
  • phpcs-sniffs-include
  • phpcs-sniffs-exclude
  • report-no-issues-found
  • review-comments-sort
  • review-comments-include-severity
  • post-generic-pr-support-comments
  • review-comments-sort
  • scan-details-msg-include
  • svg-checks
  • autoapprove
  • autoapprove-php-nonfunctional-changes

Options altered:

  • phpcs-severityset to1
  • phpcs-sniffs-includeset toGeneric.PHP.DisallowShortOpenTag, Squiz.PHP.CommentedOutCode
  • phpcs-sniffs-excludeset toWordPress.Security.EscapeOutput, WordPress.PHP.DevelopmentFunctions, WordPress.WP.AlternativeFunctions, WordPress.PHP.DiscouragedPHPFunctions, WordPress.Files.FileName, Squiz.Commenting.FileComment, Generic.PHP.Syntax
  • skip-draft-prsset to

PHP lint options

PHP lint files enabled: true

Lint modified files only: true

Lint files with file extensions:

  • php

Directories not PHP linted:

  • None

SVG configuration

SVG scanning enabled: true

Scan added/modified files with file extensions:

  • svg

Auto-approval configuration

Auto-approvals enabled: true

Non-functional changes auto-approved: true

Files with file extensions to consider for non-functional change auto-approval: php

Auto-approved file-types:

  • css
  • csv
  • eot
  • gif
  • gz
  • ico
  • ini
  • jpeg
  • jpg
  • json
  • less
  • map
  • md
  • mdown
  • mo
  • mp4
  • otf
  • pcss
  • pdf
  • po
  • pot
  • png
  • sass
  • scss
  • styl
  • ttf
  • txt
  • woff
  • woff2
  • yml

PHPCS configuration

PHPCS scanning enabled: true

PHPCS severity level: 1

Standard(s) used:

  • PHPCompatibility
  • PHPCompatibilityParagonieRandomCompat
  • PHPCompatibilityParagonieSodiumCompat
  • VariableAnalysis
  • WordPress

Runtime set:

  • testVersion 8.1-

Scan added/modified files with file extensions:

  • php
  • js
  • twig

Custom sniffs included:

  • Generic.PHP.DisallowShortOpenTag
  • Squiz.PHP.CommentedOutCode

Custom sniffs excluded:

  • WordPress.Security.EscapeOutput
  • WordPress.PHP.DevelopmentFunctions
  • WordPress.WP.AlternativeFunctions
  • WordPress.PHP.DiscouragedPHPFunctions
  • WordPress.Files.FileName
  • Squiz.Commenting.FileComment
  • Generic.PHP.Syntax

Directories not PHPCS scanned:

  • None

WPScan API configuration

WPScan API scanning enabled: true

WPScan API URL: https://wpscan.com/api/v3

Directories scanned:

  • plugins
  • client-mu-plugins
  • themes

Directories not scanned:

  • None

Scan added/modified plugins based on headers present in files with file extensions:

  • php

Scan added/modified themes based on headers present in files with file extensions:

  • css

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

Successfully merging this pull request may close these issues.

2 participants