configs: Improve security of in-repo configuration.

cli/src/commands/git/mod.rs

@@ -21,7 +21,6 @@ mod push;
 mod remote;
 mod root;
 
-use std::io::Write as _;
 use std::path::Path;
 
 use clap::Subcommand;
@@ -30,9 +29,11 @@ use jj_lib::config::ConfigFile;
 use jj_lib::config::ConfigSource;
 use jj_lib::git;
 use jj_lib::git::UnexpectedGitBackendError;
+use jj_lib::protos::user_config::RepoConfig;
 use jj_lib::ref_name::RemoteNameBuf;
 use jj_lib::ref_name::RemoteRefSymbol;
 use jj_lib::store::Store;
+use jj_lib::user_config::write_user_config;
 
 use self::clone::GitCloneArgs;
 use self::clone::cmd_git_clone;
@@ -53,6 +54,7 @@ use self::root::cmd_git_root;
 use crate::cli_util::CommandHelper;
 use crate::cli_util::WorkspaceCommandHelper;
 use crate::command_error::CommandError;
+use crate::command_error::internal_error;
 use crate::command_error::user_error_with_message;
 use crate::ui::Ui;
 
@@ -127,7 +129,13 @@ fn write_repository_level_trunk_alias(
     let mut file = ConfigFile::load_or_empty(ConfigSource::Repo, repo_path.join("config.toml"))?;
     file.set_value(["revset-aliases", "trunk()"], symbol.to_string())
         .expect("initial repo config shouldn't have invalid values");
-    file.save()?;
+    write_user_config(
+        repo_path,
+        &RepoConfig {
+            config: file.text(),
+        },
+    )
+    .map_err(internal_error)?;
     writeln!(
         ui.status(),
         "Setting the revset alias `trunk()` to `{symbol}`",

cli/src/config.rs

@@ -552,13 +552,33 @@ impl ConfigEnv {
         // same as InvalidSignature or RepoMovedError .
         let legacy_config = path.join("config.toml");
         match std::fs::read_to_string(&legacy_config).context(&legacy_config) {
-            Ok(content) => {
-                self.set_repo_config(&content, true)?;
-                std::fs::remove_file(&legacy_config).context(&legacy_config)?;
-            }
+            Ok(content) => match read_user_config::<RepoConfig>(path) {
+                Ok(_) => {
+                    return Err(user_error_with_hint(
+                        "Both new and legacy repo config were found. You need to delete .jj/repo/config.toml.",
+                        "You probably want to copy the contents of .jj/repo/config.toml and run `jj config edit --repo` to edit your repo config instead",
+                    ));
+                }
+                Err(UserConfigError::LegacyRepo) => {
+                    writeln!(
+                        ui.warning_default(),
+                        "Migrating from legacy repo config file (`$REPO_DIR/config.toml`) to \
+                         secure configuration",
+                    )?;
+                    writeln!(
+                        ui.hint_default(),
+                        "In the future, use `jj config edit --repo` to edit your config files \
+                         instead"
+                    )?;
+                    self.set_repo_config(&content, true)?;
+                    std::fs::remove_file(&legacy_config).context(&legacy_config)?;
+                }
+                Err(e) => return Err(internal_error(e)),
+            },
             Err(e) if e.source.kind() == std::io::ErrorKind::NotFound => {}
             Err(e) => return Err(e.into()),
         }
+
         self.repo_config = self.load_user_config::<RepoConfig>(ui, path)?;
         Ok(())
     }

cli/tests/test_alias.rs

@@ -308,10 +308,15 @@ fn test_alias_in_repo_config() {
     work_dir2.create_dir("sub");
 
     test_env.add_config(r#"aliases.l = ['log', '-r@', '--no-graph', '-T"user alias\n"']"#);
-    work_dir1.write_file(
-        ".jj/repo/config.toml",
-        r#"aliases.l = ['log', '-r@', '--no-graph', '-T"repo1 alias\n"']"#,
-    );
+    work_dir1
+        .run_jj([
+            "config",
+            "set",
+            "--repo",
+            "aliases.l",
+            r#"['log', '-r@', '--no-graph', '-T"repo1 alias\n"']"#,
+        ])
+        .success();
 
     // In repo1 sub directory, aliases can be loaded from the repo1 config.
     let output = test_env.run_jj_in(work_dir1.root().join("sub"), ["l"]);

cli/tests/test_config_command.rs

@@ -408,10 +408,15 @@ fn test_config_layer_override_default() {
     "#);
 
     // Repo
-    work_dir.write_file(
-        ".jj/repo/config.toml",
-        format!("{config_key} = {value}\n", value = to_toml_value("repo")),
-    );
+    work_dir
+        .run_jj([
+            "config",
+            "set",
+            "--repo",
+            config_key,
+            &format!("{}", to_toml_value("repo")),
+        ])
+        .success();
     let output = work_dir.run_jj(["config", "list", config_key]);
     insta::assert_snapshot!(output, @r#"
     merge-tools.vimdiff.program = "repo"
@@ -488,10 +493,15 @@ fn test_config_layer_override_env() {
     "#);
 
     // Repo
-    work_dir.write_file(
-        ".jj/repo/config.toml",
-        format!("{config_key} = {value}\n", value = to_toml_value("repo")),
-    );
+    work_dir
+        .run_jj([
+            "config",
+            "set",
+            "--repo",
+            config_key,
+            &format!("{}", to_toml_value("repo")),
+        ])
+        .success();
     let output = work_dir.run_jj(["config", "list", config_key]);
     insta::assert_snapshot!(output, @r#"
     ui.editor = "repo"
@@ -554,13 +564,15 @@ fn test_config_layer_workspace() {
         .success();
 
     // Repo
-    main_dir.write_file(
-        ".jj/repo/config.toml",
-        format!(
-            "{config_key} = {value}\n",
-            value = to_toml_value("main-repo")
-        ),
-    );
+    main_dir
+        .run_jj([
+            "config",
+            "set",
+            "--repo",
+            config_key,
+            &format!("{}", to_toml_value("main-repo")),
+        ])
+        .success();
     let output = main_dir.run_jj(["config", "list", config_key]);
     insta::assert_snapshot!(output, @r#"
     ui.editor = "main-repo"

cli/tests/test_config_security.rs

@@ -197,8 +197,22 @@ fn test_legacy_config_migration() {
     insta::assert_snapshot!(output, @r###"
     bar
     [EOF]
+    ------- stderr -------
+    Warning: Migrating from legacy repo config file (`$REPO_DIR/config.toml`) to secure configuration
+    Hint: In the future, use `jj config edit --repo` to edit your config files instead
+    [EOF]
     "###);
 
     assert!(!legacy_config.exists());
     assert!(new_config.exists());
+
+    std::fs::write(&legacy_config, "foo = \"baz\"").unwrap();
+    let output = work_dir.run_jj(["config", "get", "foo"]);
+    insta::assert_snapshot!(output, @r###"
+    ------- stderr -------
+    Error: Both new and legacy repo config were found. You need to delete .jj/repo/config.toml.
+    Hint: You probably want to copy the contents of .jj/repo/config.toml and run `jj config edit --repo` to edit your repo config instead
+    [EOF]
+    [exit status: 1]
+    "###);
 }

cli/tests/test_global_opts.rs

@@ -492,7 +492,9 @@ fn test_color_config() {
     ");
 
     // Test that per-repo config overrides the user config.
-    work_dir.write_file(".jj/repo/config.toml", r#"ui.color = "never""#);
+    work_dir
+        .run_jj(["config", "set", "--repo", "ui.color", r#""never""#])
+        .success();
     let output = work_dir.run_jj(["log", "-T", "commit_id"]);
     insta::assert_snapshot!(output, @r"
     @  e8849ae12c709f2321908879bc724fdb2ab8a781

cli/tests/test_log_command.rs

@@ -1112,7 +1112,9 @@ fn test_default_revset_per_repo() {
     work_dir.run_jj(["describe", "-m", "add a file"]).success();
 
     // Set configuration to only show the root commit.
-    work_dir.write_file(".jj/repo/config.toml", r#"revsets.log = "root()""#);
+    work_dir
+        .run_jj(["config", "set", "--repo", "revsets.log", r#""root()""#])
+        .success();
 
     // Log should only contain one line (for the root commit), and not show the
     // commit created above.