From 80827f076ce02b662de92739bef2907b6fc55da5 Mon Sep 17 00:00:00 2001
From: Paul Holden <paulh@moodle.com>
Date: Tue, 31 Dec 2019 10:45:56 +0000
Subject: [PATCH] MDL-62853 privacy: sanitize subcontext paths.

---
 .../local/request/moodle_content_writer.php   |  8 ++-
 privacy/tests/moodle_content_writer_test.php  | 52 +++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/privacy/classes/local/request/moodle_content_writer.php b/privacy/classes/local/request/moodle_content_writer.php
index 5f367cd257e..fd0340d2b2a 100644
--- a/privacy/classes/local/request/moodle_content_writer.php
+++ b/privacy/classes/local/request/moodle_content_writer.php
@@ -139,11 +139,8 @@ class moodle_content_writer implements content_writer {
      * @return  content_writer
      */
     public function export_related_data(array $subcontext, $name, $data) : content_writer {
-        $path = $this->get_path($subcontext, "{$name}.json");
-
-        $this->write_data($path, json_encode($data, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT));
-
-        return $this;
+        return $this->export_custom_file($subcontext, "{$name}.json",
+            json_encode($data, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT));
     }
 
     /**
@@ -289,6 +286,7 @@ class moodle_content_writer implements content_writer {
         // This weird code is to look for a subcontext that contains a number and append an '_' to the front.
         // This is because there seems to be some weird problem with array_merge_recursive used in finalise_content().
         $subcontext = array_map(function($data) {
+            $data = clean_param($data, PARAM_PATH);
             if (stripos($data, DIRECTORY_SEPARATOR) !== false) {
                 $newpath = explode(DIRECTORY_SEPARATOR, $data);
                 $newpath = array_map(function($value) {
diff --git a/privacy/tests/moodle_content_writer_test.php b/privacy/tests/moodle_content_writer_test.php
index 57ada559950..c3ebc3c41e8 100644
--- a/privacy/tests/moodle_content_writer_test.php
+++ b/privacy/tests/moodle_content_writer_test.php
@@ -963,6 +963,34 @@ class moodle_content_writer_test extends advanced_testcase {
         $this->assertEquals($data, $expanded);
     }
 
+    /**
+     * Test that exported related data name is properly cleaned
+     *
+     * @covers ::export_related_data
+     */
+    public function test_export_related_data_clean_name() {
+        $context = \context_system::instance();
+        $subcontext = [];
+        $data = (object) ['foo' => 'bar'];
+
+        $name = 'Bad/chars:>';
+
+        $writer = $this->get_writer_instance()
+            ->set_context($context)
+            ->export_related_data($subcontext, $name, $data);
+
+        $nameclean = clean_param($name, PARAM_FILE);
+
+        $contextpath = $this->get_context_path($context, $subcontext, "{$nameclean}.json");
+        $expectedpath = "System _.{$context->id}/Badchars.json";
+        $this->assertEquals($expectedpath, $contextpath);
+
+        $fileroot = $this->fetch_exported_content($writer);
+        $json = $fileroot->getChild($contextpath)->getContent();
+
+        $this->assertEquals($data, json_decode($json));
+    }
+
     /**
      * Test that exported user preference is human readable.
      *
@@ -1002,6 +1030,30 @@ class moodle_content_writer_test extends advanced_testcase {
         ];
     }
 
+    /**
+     * Test that exported data subcontext is properly cleaned
+     *
+     * @covers ::export_data
+     */
+    public function test_export_data_clean_subcontext() {
+        $context = \context_system::instance();
+        $subcontext = ['Something/weird', 'More/bad:>', 'Bad&chars:>'];
+        $data = (object) ['foo' => 'bar'];
+
+        $writer = $this->get_writer_instance()
+            ->set_context($context)
+            ->export_data($subcontext, $data);
+
+        $contextpath = $this->get_context_path($context, $subcontext, 'data.json');
+        $expectedpath = "System _.{$context->id}/Something/weird/More/bad/Badchars/data.json";
+        $this->assertEquals($expectedpath, $contextpath);
+
+        $fileroot = $this->fetch_exported_content($writer);
+        $json = $fileroot->getChild($contextpath)->getContent();
+
+        $this->assertEquals($data, json_decode($json));
+    }
+
     /**
      * Test that exported data is shortened when exceeds the limit.
      *