rfc:phar_stop_autoloading_metadata

Differences

This shows you the differences between two versions of the page.

Link to this comparison view

Both sides previous revisionPrevious revision
Next revision
Previous revision
rfc:phar_stop_autoloading_metadata [2020/07/07 23:58] tandrerfc:phar_stop_autoloading_metadata [2020/08/04 13:43] (current) – close the vote tandre
Line 1: Line 1:
 ====== PHP RFC: Don't automatically unserialize Phar metadata outside getMetadata() ====== ====== PHP RFC: Don't automatically unserialize Phar metadata outside getMetadata() ======
-  * Version: 0.1 +  * Version: 0.4 
-  * Date: 2020-07-07 (use today's date here)+  * Date: 2020-07-07
   * Author: Tyson Andre <tandre@php.net>   * Author: Tyson Andre <tandre@php.net>
-  * Status: Draft +  * Status: Implemented 
-  * First Published at: https://wiki.php.net/rfc/phar.sanitize_object_metadata+  * First Published at: https://wiki.php.net/rfc/phar_stop_autoloading_metadata 
 +  * Implementation: https://github.com/php/php-src/pull/5855
  
 ===== Introduction ===== ===== Introduction =====
Line 15: Line 16:
 ===== Proposal ===== ===== Proposal =====
  
-Don't unserialize the metadata automatically when a phar is opened by php. Only unserialize the metadata if ''%%Phar->getMetadata()%%'' or ''%%PharFile->getMetadata()%%'' is called directly.+Don't unserialize the metadata automatically when a phar file is opened by php. Make PHP unserialize the metadata **only** if ''%%Phar->getMetadata()%%'' or ''%%PharFile->getMetadata()%%'' is called directly.
  
-Additionally, add an ''$allowed_classes'' parameter to both getMetadata() implementations, defaulting to the current behavior of allowing any classes (true)This will be passed to the call to ''unserialize()'' performed internally.+Additionally, add an ''array $unserialize_options = []'' parameter to both getMetadata() implementations, defaulting to the current default ''unserialize()'' behavior such as allowing any classes. (As an implementation detail, if ''$unserialize_options'' is set to anything other than the default, the resulting metadata won't be cached and this won't return the value from the cache. E.g. ''%%getMetaData(['allowed_classes' => []])%%'' after ''setMetadata(new stdClass())'' will likely trigger a ''%%unserialize(['allowed_classes' => []])%%'' call internally.)
  
 This implements one possible solution for https://bugs.php.net/bug.php?id=76774 This implements one possible solution for https://bugs.php.net/bug.php?id=76774
Line 33: Line 34:
 ===== RFC Impact ===== ===== RFC Impact =====
 ==== To SAPIs ==== ==== To SAPIs ====
-This affects stream wrapper calls such as file_exists(), as well as direct calls to ''%%Phar->getMetadata()%%'' +This affects stream wrapper calls such as ''file_exists()'' (they will no longer call ''unserialize()'' if there is metadata), as well as direct calls to ''%%Phar->getMetadata()%%'' (they will call ''unserialize()'' if there is metadatainstead of using the data from the time the phar file was loaded)
-Describe the impact to CLIDevelopment web server, embedded PHP etc.+
  
-==== php.ini Defaults ==== +===== Vote =====
-If there are any php.ini settings then list: +
-  * hardcoded default values: On +
-  * php.ini-development values: On +
-  * php.ini-production values: On +
- +
-===== Proposed Voting Choices =====+
  
 Yes/No, requiring 2/3 majority to stop automatically unserializing metadata. Yes/No, requiring 2/3 majority to stop automatically unserializing metadata.
 +Voting started on 2020-07-21 and ended 2020-08-04.
 +
 +<doodle title="Stop automatically unserializing Phar metadata outside direct getMetadata() calls" auth="tandre" voteType="single" closed="true">
 +   * Yes
 +   * No
 +</doodle>
  
 ===== References ===== ===== References =====
 +
 +https://externals.io/message/110871 '[RFC] Don't automatically unserialize Phar metadata outside getMetadata()'
  
 https://externals.io/message/110856 'Including "Disable the ability to use concrete types in PHAR metadata" in PHP 8.0?' https://externals.io/message/110856 'Including "Disable the ability to use concrete types in PHAR metadata" in PHP 8.0?'
Line 52: Line 54:
 https://externals.io/message/105271 'PHP deserialization techniques offer rich pickings for security researchers' https://externals.io/message/105271 'PHP deserialization techniques offer rich pickings for security researchers'
  
-https://bugs.php.net/bug.php?id=76774+https://bugs.php.net/bug.php?id=76774 'Request #76774 Disable the ability to use concrete types in PHAR metadata'
  
 ===== Rejected Features ===== ===== Rejected Features =====
  
-I'd considered continuing to call unserialize() all of the time to validate phars earlier, but with ''allowed_classes=[]'' instead (controlled by an ini setting). This seemed like an unnecessary ini setting that could be avoided by just not automatically loading the metadata.+I'd considered continuing to call ''unserialize()'' all of the time to validate phars earlier, but with ''allowed_classes=[]'' instead (controlled by an ini setting). This seemed like an unnecessary ini setting that could be avoided by just not automatically loading the metadata
 + 
 +===== Changelog ===== 
 + 
 +0.4: Change from ''getMetadata($allowed_classes = ...)'' to ''getMetadata(array $unserialize_options = [])'' in this document. I forgot about max_depth being added in php 8.0 and the usefulness of being able to support future options added to unserialize() without changing the signature of getMetadata. Elaborate on implementation details ''$unserialize_options'' would lead to when setMetaData is called before ''%%$pharFileOrEntry->getMetadata(['allowed_classes' => $classes])%%'' 
 + 
 +0.3: Clarify wording, add link to RFC announcement thread. Remove inapplicable ini defaults section. 
 + 
 +0.2: Link to implementation.
rfc/phar_stop_autoloading_metadata.1594166301.txt.gz · Last modified: 2020/07/07 23:58 by tandre