From 00c92b3d71cbc6bd7ff3f2a56209aa65680296cf Mon Sep 17 00:00:00 2001
From: Nicholas Nethercote <nnethercote@mozilla.com>
Date: Wed, 29 Apr 2020 21:47:12 +0000
Subject: [PATCH] Bug 1632279 - Disallow default pref definitions in user pref
 files. r=KrisWright

We distinguish between two kinds of pref syntax.
- "Default pref files" are the ones that come with Firefox, constructed from
  `all.js` and similar files.
- "User pref files" are the ones that get created in the user's profile.
  `prefs.js` is the one that Firefox creates and overwrites every time a pref
  changes. `user.js` is the one that users can create themselves.

We also have two basic kinds of pref.
- Default: `pref(...)` and the unfortunate `sticky_pref(...)`.
- User: `user_pref(...)`, which override but don't replace the default.

It only makes sense for user pref files to contain user prefs; users shouldn't
be able to create default prefs or change default pref values.

But it turns out that user pref files have been able to define default prefs
pretty much forever. This appears to be an oversight, and this commit restricts
things so that user pref files cannot contain default prefs.

The commit also fixes an incorrect comment in testParser.js.

Differential Revision: https://phabricator.services.mozilla.com/D73003
---
 modules/libpref/docs/index.md                |  5 +++++
 modules/libpref/parser/src/lib.rs            | 17 +++++++++++++----
 modules/libpref/test/gtest/Parser.cpp        | 15 ++++++++++++---
 modules/libpref/test/unit/data/testParser.js |  4 ++--
 4 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/modules/libpref/docs/index.md b/modules/libpref/docs/index.md
index 9a4ce1de13fc..03b9ac44b7aa 100644
--- a/modules/libpref/docs/index.md
+++ b/modules/libpref/docs/index.md
@@ -181,6 +181,11 @@ reads it.
 These files are not JavaScript; the `.js` suffix is present for historical
 reasons. They are read by a custom parser within libpref.
 
+User pref file syntax is slightly more restrictive than default pref file
+syntax. In user pref files `user_pref` definitions are allowed but `pref` and
+`sticky_pref` definitions are not, and attributes (such as `locked`) are not
+allowed.
+
 **Problem:** geckodriver has a separate prefs parser in the mozprofile crate.
 
 **Problem:** there is no versioning of these files, for either the syntax or
diff --git a/modules/libpref/parser/src/lib.rs b/modules/libpref/parser/src/lib.rs
index 7265498bcae8..a8c5b6b317a6 100644
--- a/modules/libpref/parser/src/lib.rs
+++ b/modules/libpref/parser/src/lib.rs
@@ -10,7 +10,8 @@
 //!
 //! <pref-file>   = <pref>*
 //! <pref>        = <pref-spec> "(" <pref-name> "," <pref-value> <pref-attrs> ")" ";"
-//! <pref-spec>   = "user_pref" | "pref" | "sticky_pref"
+//! <pref-spec>   = "user_pref" | "pref" | "sticky_pref" // in default pref files
+//! <pref-spec>   = "user_pref"                          // in user pref files
 //! <pref-name>   = <string-literal>
 //! <pref-value>  = <string-literal> | "true" | "false" | <int-value>
 //! <int-value>   = <sign>? <int-literal>
@@ -373,14 +374,22 @@ impl<'t> Parser<'t> {
         loop {
             // <pref-spec>
             let (pref_value_kind, mut is_sticky) = match token {
-                Token::Pref => (PrefValueKind::Default, false),
-                Token::StickyPref => (PrefValueKind::Default, true),
+                Token::Pref if self.kind == PrefValueKind::Default => {
+                    (PrefValueKind::Default, false)
+                }
+                Token::StickyPref if self.kind == PrefValueKind::Default => {
+                    (PrefValueKind::Default, true)
+                }
                 Token::UserPref => (PrefValueKind::User, false),
                 Token::SingleChar(EOF) => return !self.has_errors,
                 _ => {
                     token = self.error_and_recover(
                         token,
-                        "expected pref specifier at start of pref definition",
+                        if self.kind == PrefValueKind::Default {
+                            "expected pref specifier at start of pref definition"
+                        } else {
+                            "expected 'user_pref' at start of pref definition"
+                        },
                     );
                     continue;
                 }
diff --git a/modules/libpref/test/gtest/Parser.cpp b/modules/libpref/test/gtest/Parser.cpp
index 1738c2b3ae8c..972d32cfcadd 100644
--- a/modules/libpref/test/gtest/Parser.cpp
+++ b/modules/libpref/test/gtest/Parser.cpp
@@ -374,8 +374,17 @@ pref("parse.error", true))",
   );
 
   USER(R"(
-pref("parse.error", true;
-pref("int.ok", 1);
+pref("parse.error", true);
+sticky_pref("parse.error", true);
+user_pref("int.ok", 1);
+    )",
+    "test:2: prefs parse error: expected 'user_pref' at start of pref definition\n"
+    "test:3: prefs parse error: expected 'user_pref' at start of pref definition\n"
+  );
+
+  USER(R"(
+user_pref("parse.error", true;
+user_pref("int.ok", 1);
     )",
     "test:2: prefs parse error: expected ')' after pref value\n"
   );
@@ -418,7 +427,7 @@ pref("parse.error", true)",
   );
 
   USER(R"(
-pref("parse.error", true)",
+user_pref("parse.error", true)",
     "test:2: prefs parse error: expected ')' after pref value\n"
   );
 
diff --git a/modules/libpref/test/unit/data/testParser.js b/modules/libpref/test/unit/data/testParser.js
index f59567c0334c..be29430b2aff 100644
--- a/modules/libpref/test/unit/data/testParser.js
+++ b/modules/libpref/test/unit/data/testParser.js
@@ -1,5 +1,5 @@
-// Note: this file tests only valid syntax (of user pref files, not default
-// pref files). See modules/libpref/test/gtest/Parser.cpp for tests if invalid
+// Note: this file tests only valid syntax (of default pref files, not user
+// pref files). See modules/libpref/test/gtest/Parser.cpp for tests of invalid
 // syntax.
 
 #