fix: protect read file from path traversal

[ndom91]

☕️ Reasoning

• Ensure that the new get_pr_template_contents fn can't read anything outside of the project's root directory

🧢 Changes

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
gitbutler-web	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 18, 2024 2:33pm

[ndom91]

@mtsgrd some small rust question:

• There's probably no need for .context() on each method that potentially fails, right?
• Is it okay to add the entire git2 crate to gitbutler-fs's Cargo.toml? Can I add something like feature = { Repository } to only add what I need here?

[Byron]

• Is it okay to add the entire git2 crate to gitbutler-fs's Cargo.toml? Can I add something like feature = { Repository } to only add what I need here?

Features are predefined in the crate and they usually only provide features if it grates off other dependencies, not just code. When I see the example above, it reminds me of typescript imports :).

[Caleb-T-Owens]

Small nit, the path() function may return the repository path rather than the .git folder if the repository is bare. If we had a repository that was bare, that would mean that it could access files outside the repository.

Suggested change

	fn read_file_from_workspace(&self, relative_path: &Path) -> Result<String> {
	let ctx = CommandContext::open(self)?;
	let base_path = ctx
	.repository()
	.path()
	.parent()
	.ok_or(anyhow::anyhow!("Could not find repository base path"))?;
	let canonicalized_file_path = base_path.join(relative_path).canonicalize()?;
	if canonicalized_file_path.as_path().starts_with(base_path) {
	let tree = ctx.repository().head()?.peel_to_tree()?;
	let entry = tree.get_path(relative_path)?;
	let blob = ctx.repository().find_blob(entry.id())?;
	let content = std::str::from_utf8(blob.content())?;
	Ok(content.to_string())
	} else {
	anyhow::bail!("Invalid workspace file");
	}
	}
	fn read_file_from_workspace(&self, relative_path: &Path) -> Result<String> {
	let ctx = CommandContext::open(self)?;
	let repository = ctx.repository();
	// If a repostiory is bare, repository.path will return the repository
	// directory rather than the `.git` repository.
	// Ref: https://docs.rs/git2/latest/git2/struct.Repository.html#method.path
	let base_path = if repository.is_bare() {
	repository.path()
	} else {
	repository
	.path()
	.parent()
	.ok_or(anyhow::anyhow!("Could not find repository base path"))?
	};
	let canonicalized_file_path = base_path.join(relative_path).canonicalize()?;
	if canonicalized_file_path.as_path().starts_with(base_path) {
	let tree = repository.head()?.peel_to_tree()?;
	let entry = tree.get_path(relative_path)?;
	let blob = repository.find_blob(entry.id())?;
	let content = std::str::from_utf8(blob.content())?;
	Ok(content.to_string())
	} else {
	anyhow::bail!("Invalid workspace file");
	}
	}

I wonder if we should name the function something like read_commited_file or something like that, to make it clear that its not going to look at uncommitted changes.

[Caleb-T-Owens]

https://docs.rs/git2/latest/git2/struct.Repository.html#method.path

[Caleb-T-Owens]

Actually, I've overcomplicating it, we have the project, so we should be able to just use self.path with no faffing about 😆