Issues in Manager.php - All Issues - Inspection of "Implemented a secure metabox saving process" - amarkal/amarkal-metabox - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — master ( 6f8a68...2f55bf )

by Askupa

created 2017-03-27 18:50 UTC

Manager.php (8 issues)

Labels

Severity

Upgrade to new PHP Analysis Engine

These results are based on our legacy PHP analysis, consider migrating to our new PHP analysis engine instead. Learn more

<?php

namespace Amarkal\Metabox;

/**
 * Metabox Manager adds metaboxes to WordPress posts
 */
class Manager
{
    /**
     * @var Singleton The reference to *Singleton* instance of this class
     */
    private static $instance;
    
    /**
     * @var Array Stores all the registered metaboxes
     */
    private $metaboxes = array();
    
    /**
     * Security nonce action
     */
    const NONCE_ACTION = 'amarkal_metabox';
    
    /**
     * Returns the *Singleton* instance of this class.
     *
     * @return Singleton The *Singleton* instance.
     */
    public static function get_instance()
    {
        if( null === static::$instance ) 
class YourClass
{
    private static $someVariable;

    public static function getSomeVariable()
    {
        return static::$someVariable;
    }
}
        {
            static::$instance = new static();
class YourClass
{
    private static $someVariable;

    public static function getSomeVariable()
    {
        return static::$someVariable;
    }
}
            static::$instance->init();
class YourClass
{
    private static $someVariable;

    public static function getSomeVariable()
    {
        return static::$someVariable;
    }
}
        }
        return static::$instance;
class YourClass
{
    private static $someVariable;

    public static function getSomeVariable()
    {
        return static::$someVariable;
    }
}
    }
    
    /**
     * Add a metabox.
     * 
     * @param string $id
     * @param array $args
     * @throws \RuntimeException if the given metabox id has already been registered
     */
    public function add( $id, array $args )
    {
        if( !in_array($id, $this->metaboxes) )
        {
            $this->metaboxes[$id] = array_merge($this->default_args(), $args);
        }
        else throw new \RuntimeException("A metabox with id '$id' has already been registered.");
    }
    
    /**
     * Render a metabox.
     * 
     * @param WP_Post $post
     * @param array $args
     */
    public function render( $post, $args )
    {
        $metabox = $this->metaboxes[$args['id']];
        wp_nonce_field(self::NONCE_ACTION, $args['id'].'_nonce');
        foreach( $metabox['fields'] as $field )
        {
            $field['post_id'] = $post->ID;
            $field_template = new Field($field);
            echo $field_template->render();
        }
    }
    
    /**
     * Internally used to register metaboxes.
     */
    public function add_meta_boxes()
    {
        foreach( $this->metaboxes as $id => $args )
        {
            \add_meta_box(
                $id,
                $args['title'],
                array($this, 'render'),
                $args['screen'],
                $args['context'],
                $args['priority']
            );
        }
    }
    
    /**
     * Save metaboxes data for a given page.
     * 
     * @param number $post_id
     */
    public function save_meta_boxes( $post_id )
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
    {
        /**
         * A note on security:
         * 
         * We need to verify this came from the our screen and with proper authorization,
         * because save_post can be triggered at other times. since metaboxes can 
         * be removed - by having a nonce field in only one metabox there is no 
         * guarantee the nonce will be there. By placing a nonce field in each 
         * metabox you can check if data from that metabox has been sent 
         * (and is actually from where you think it is) prior to processing any data.
         * @see http://wordpress.stackexchange.com/a/49460/25959
         */
 
        /*
         * If this is an autosave, our form has not been submitted,
         * so we don't want to do anything.
         */
        if( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE ) 
        {
            return $post_id;
        }
 
        // Check the user's permissions.
        if( isset($_POST['post_type']) && 'page' == $_POST['post_type'] ) 
        {
            if( !current_user_can('edit_page', $post_id) ) return $post_id;
        } 
        else 
        {
            if( !current_user_can('edit_post', $post_id) ) return $post_id;
        }

        // Update the meta fields.
        foreach( $this->metaboxes as $id => $metabox )
        {
            $this->save_meta_box( $post_id, $id, $metabox );
        }
    }
    
    /**
     * Save the data of a single metabox.
     * 
     * @param number $post_id
     * @param string $id
     * @param array $metabox
     */
    public function save_meta_box( $post_id, $id, $metabox )
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
    {
        $nonce_name = $id.'_nonce';
        // Check if our nonce is set.
        if ( !isset($_POST[$nonce_name]) ) {
            return $post_id;
        }

        $nonce = $_POST[$nonce_name];

        // Verify that the nonce is valid.
        if ( !wp_verify_nonce( $nonce, self::NONCE_ACTION ) ) {
            return $post_id;
        }
        
        foreach( $metabox['fields'] as $field )
        {
            $data = filter_input( INPUT_POST, $field['name'] );
            \update_post_meta( $post_id, $field['name'], $data );
        }
    }
    
    /**
     * Print custom metabox style.
     */
    public function print_style() 
    {
        $cs = get_current_screen();
        
        foreach( $this->metaboxes as $metabox )
        {
            if( $metabox['screen'] === $cs->id )
            {
                echo '<style>';
                include 'metabox.css';
                echo '</style>';
                return;
            }
        }
    }
    
    /**
     * Initiate the metaboxes by adding action hooks for printing and saving.
     */
    private function init()
    {
        \add_action( 'add_meta_boxes', array( $this, 'add_meta_boxes' ) );
        \add_action( 'save_post', array( $this, 'save_meta_boxes' ) );
        \add_action( 'admin_footer', array( $this, 'print_style' ) );
    }
    
    /**
     * Default arguments for the add() method.
     * 
     * @return array
     */
    private function default_args()
    {
        return array(
            'id'       => null,
            'title'    => null,
            'screen'   => null,
            'context'  => 'advanced',
            'priority' => 'default',
            'fields'   => array()
        );
    }
}

1			<?php
2
3			namespace Amarkal\Metabox;
4
5			/**
6			* Metabox Manager adds metaboxes to WordPress posts
7			*/
8			class Manager
9			{
10			/**
11			* @var Singleton The reference to Singleton instance of this class
12			*/
13			private static $instance;
14
15			/**
16			* @var Array Stores all the registered metaboxes
17			*/
18			private $metaboxes = array();
19
20			/**
21			* Security nonce action
22			*/
23			const NONCE_ACTION = 'amarkal_metabox';
24
25			/**
26			* Returns the Singleton instance of this class.
27			*
28			* @return Singleton The Singleton instance.
29			*/
30			public static function get_instance()
31			{
32			if( null === static::$instance )
			0 ignored issues – show Bug introduced 2017-03-27 05:44 UTC by Report Bug Copy Issue Report Show Similar Issues like this Since `$instance` is declared private, accessing it with `static` will lead to errors in possible sub-classes; consider using `self`, or increasing the visibility of `$instance` to at least protected. Let’s assume you have a class which uses late-static binding: class YourClass { private static $someVariable; public static function getSomeVariable() { return static::$someVariable; } } The code above will run fine in your PHP runtime. However, if you now create a sub-class and call the `getSomeVariable()` on that sub-class, you will receive a runtime error: class YourSubClass extends YourClass { } YourSubClass::getSomeVariable(); // Will cause an access error. In the case above, it makes sense to update `SomeClass` to use `self` instead: class SomeClass { private static $someVariable; public static function getSomeVariable() { return self::$someVariable; // self works fine with private. } } Loading history...
33			{
34			static::$instance = new static();
			0 ignored issues – show Bug introduced 2017-03-27 05:44 UTC by Report Bug Copy Issue Report Show Similar Issues like this Since `$instance` is declared private, accessing it with `static` will lead to errors in possible sub-classes; consider using `self`, or increasing the visibility of `$instance` to at least protected. Let’s assume you have a class which uses late-static binding: class YourClass { private static $someVariable; public static function getSomeVariable() { return static::$someVariable; } } The code above will run fine in your PHP runtime. However, if you now create a sub-class and call the `getSomeVariable()` on that sub-class, you will receive a runtime error: class YourSubClass extends YourClass { } YourSubClass::getSomeVariable(); // Will cause an access error. In the case above, it makes sense to update `SomeClass` to use `self` instead: class SomeClass { private static $someVariable; public static function getSomeVariable() { return self::$someVariable; // self works fine with private. } } Loading history... Documentation Bug introduced 2017-03-27 05:44 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `new static()` of type `this<Amarkal\Metabox\Manager>` is incompatible with the declared type `object<Amarkal\Metabox\Singleton>` of property `$instance`. Our type inference engine has found an assignment to a property that is incompatible with the declared type of that property. Either this assignment is in error or the assigned type should be added to the documentation/type hint for that property.. Loading history...
35			static::$instance->init();
			0 ignored issues – show Bug introduced 2017-03-27 05:44 UTC by Report Bug Copy Issue Report Show Similar Issues like this Since `$instance` is declared private, accessing it with `static` will lead to errors in possible sub-classes; consider using `self`, or increasing the visibility of `$instance` to at least protected. Let’s assume you have a class which uses late-static binding: class YourClass { private static $someVariable; public static function getSomeVariable() { return static::$someVariable; } } The code above will run fine in your PHP runtime. However, if you now create a sub-class and call the `getSomeVariable()` on that sub-class, you will receive a runtime error: class YourSubClass extends YourClass { } YourSubClass::getSomeVariable(); // Will cause an access error. In the case above, it makes sense to update `SomeClass` to use `self` instead: class SomeClass { private static $someVariable; public static function getSomeVariable() { return self::$someVariable; // self works fine with private. } } Loading history...
36			}
37			return static::$instance;
			0 ignored issues – show Bug introduced 2017-03-27 05:44 UTC by Report Bug Copy Issue Report Show Similar Issues like this Since `$instance` is declared private, accessing it with `static` will lead to errors in possible sub-classes; consider using `self`, or increasing the visibility of `$instance` to at least protected. Let’s assume you have a class which uses late-static binding: class YourClass { private static $someVariable; public static function getSomeVariable() { return static::$someVariable; } } The code above will run fine in your PHP runtime. However, if you now create a sub-class and call the `getSomeVariable()` on that sub-class, you will receive a runtime error: class YourSubClass extends YourClass { } YourSubClass::getSomeVariable(); // Will cause an access error. In the case above, it makes sense to update `SomeClass` to use `self` instead: class SomeClass { private static $someVariable; public static function getSomeVariable() { return self::$someVariable; // self works fine with private. } } Loading history... Bug Compatibility introduced 2017-03-27 05:44 UTC by Report Bug Copy Issue Report Show Similar Issues like this The expression `static::$instance;` of type `Amarkal\Metabox\Manager\|Amarkal\Metabox\Singleton` adds the type `Amarkal\Metabox\Manager` to the return on line 37 which is incompatible with the return type documented by `Amarkal\Metabox\Manager::get_instance` of type `Amarkal\Metabox\Singleton`. Loading history...
38			}
39
40			/**
41			* Add a metabox.
42			*
43			* @param string $id
44			* @param array $args
45			* @throws \RuntimeException if the given metabox id has already been registered
46			*/
47			public function add( $id, array $args )
48			{
49			if( !in_array($id, $this->metaboxes) )
50			{
51			$this->metaboxes[$id] = array_merge($this->default_args(), $args);
52			}
53			else throw new \RuntimeException("A metabox with id '$id' has already been registered.");
54			}
55
56			/**
57			* Render a metabox.
58			*
59			* @param WP_Post $post
60			* @param array $args
61			*/
62			public function render( $post, $args )
63			{
64			$metabox = $this->metaboxes[$args['id']];
65			wp_nonce_field(self::NONCE_ACTION, $args['id'].'_nonce');
66			foreach( $metabox['fields'] as $field )
67			{
68			$field['post_id'] = $post->ID;
69			$field_template = new Field($field);
70			echo $field_template->render();
71			}
72			}
73
74			/**
75			* Internally used to register metaboxes.
76			*/
77			public function add_meta_boxes()
78			{
79			foreach( $this->metaboxes as $id => $args )
80			{
81			\add_meta_box(
82			$id,
83			$args['title'],
84			array($this, 'render'),
85			$args['screen'],
86			$args['context'],
87			$args['priority']
88			);
89			}
90			}
91
92			/**
93			* Save metaboxes data for a given page.
94			*
95			* @param number $post_id
96			*/
97			public function save_meta_boxes( $post_id )
			0 ignored issues – show Coding Style introduced 2017-03-27 18:52 UTC by Report Bug Copy Issue Report Show Similar Issues like this `save_meta_boxes` uses the super-global variable `$_POST` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
98			{
99			/**
100			* A note on security:
101			*
102			* We need to verify this came from the our screen and with proper authorization,
103			* because save_post can be triggered at other times. since metaboxes can
104			* be removed - by having a nonce field in only one metabox there is no
105			* guarantee the nonce will be there. By placing a nonce field in each
106			* metabox you can check if data from that metabox has been sent
107			* (and is actually from where you think it is) prior to processing any data.
108			* @see http://wordpress.stackexchange.com/a/49460/25959
109			*/
110
111			/*
112			* If this is an autosave, our form has not been submitted,
113			* so we don't want to do anything.
114			*/
115			if( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE )
116			{
117			return $post_id;
118			}
119
120			// Check the user's permissions.
121			if( isset($_POST['post_type']) && 'page' == $_POST['post_type'] )
122			{
123			if( !current_user_can('edit_page', $post_id) ) return $post_id;
124			}
125			else
126			{
127			if( !current_user_can('edit_post', $post_id) ) return $post_id;
128			}
129
130			// Update the meta fields.
131			foreach( $this->metaboxes as $id => $metabox )
132			{
133			$this->save_meta_box( $post_id, $id, $metabox );
134			}
135			}
136
137			/**
138			* Save the data of a single metabox.
139			*
140			* @param number $post_id
141			* @param string $id
142			* @param array $metabox
143			*/
144			public function save_meta_box( $post_id, $id, $metabox )
			0 ignored issues – show Coding Style introduced 2017-03-27 18:52 UTC by Report Bug Copy Issue Report Show Similar Issues like this `save_meta_box` uses the super-global variable `$_POST` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
145			{
146			$nonce_name = $id.'_nonce';
147			// Check if our nonce is set.
148			if ( !isset($_POST[$nonce_name]) ) {
149			return $post_id;
150			}
151
152			$nonce = $_POST[$nonce_name];
153
154			// Verify that the nonce is valid.
155			if ( !wp_verify_nonce( $nonce, self::NONCE_ACTION ) ) {
156			return $post_id;
157			}
158
159			foreach( $metabox['fields'] as $field )
160			{
161			$data = filter_input( INPUT_POST, $field['name'] );
162			\update_post_meta( $post_id, $field['name'], $data );
163			}
164			}
165
166			/**
167			* Print custom metabox style.
168			*/
169			public function print_style()
170			{
171			$cs = get_current_screen();
172
173			foreach( $this->metaboxes as $metabox )
174			{
175			if( $metabox['screen'] === $cs->id )
176			{
177			echo '<style>';
178			include 'metabox.css';
179			echo '</style>';
180			return;
181			}
182			}
183			}
184
185			/**
186			* Initiate the metaboxes by adding action hooks for printing and saving.
187			*/
188			private function init()
189			{
190			\add_action( 'add_meta_boxes', array( $this, 'add_meta_boxes' ) );
191			\add_action( 'save_post', array( $this, 'save_meta_boxes' ) );
192			\add_action( 'admin_footer', array( $this, 'print_style' ) );
193			}
194
195			/**
196			* Default arguments for the add() method.
197			*
198			* @return array
199			*/
200			private function default_args()
201			{
202			return array(
203			'id' => null,
204			'title' => null,
205			'screen' => null,
206			'context' => 'advanced',
207			'priority' => 'default',
208			'fields' => array()
209			);
210			}
211			}

amarkal / amarkal-metabox

Push — master ( 6f8a68...2f55bf )

Manager.php (8 issues)

Labels

Severity

Introduced By

Upgrade to new PHP Analysis Engine

Duplication Side-by-Side

Filter issues like