Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elaborate registry names to index URLs when publishing #4957

Merged
merged 3 commits into from
Jan 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ impl Package {
}
}

pub fn to_registry_toml(&self) -> CargoResult<String> {
let manifest = self.manifest().original().prepare_for_publish();
pub fn to_registry_toml(&self, config: &Config) -> CargoResult<String> {
let manifest = self.manifest().original().prepare_for_publish(config)?;
let toml = toml::to_string(&manifest)?;
Ok(format!("\
# THIS FILE IS AUTOMATICALLY GENERATED BY CARGO\n\
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ fn tar(ws: &Workspace,
})?;

let mut header = Header::new_ustar();
let toml = pkg.to_registry_toml()?;
let toml = pkg.to_registry_toml(ws.config())?;
header.set_path(&path)?;
header.set_entry_type(EntryType::file());
header.set_mode(0o644);
Expand Down
8 changes: 7 additions & 1 deletion src/cargo/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,13 @@ impl Config {
/// Gets the index for a registry.
pub fn get_registry_index(&self, registry: &str) -> CargoResult<Url> {
Ok(match self.get_string(&format!("registries.{}.index", registry))? {
Some(index) => index.val.to_url()?,
Some(index) => {
let url = index.val.to_url()?;
if url.username() != "" || url.password().is_some() {
bail!("Registry URLs may not contain credentials");
}
url
}
None => bail!("No index found for registry: `{}`", registry),
})
}
Expand Down
93 changes: 58 additions & 35 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,25 +178,26 @@ impl<'de> de::Deserialize<'de> for TomlDependency {
}

#[derive(Deserialize, Serialize, Clone, Debug, Default)]
#[serde(rename_all = "kebab-case")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah unfortuantely I don't think we can do this as it breaks manifests that use default_features = false, for example.

When we first moved to serde we ran into that (as the old rustc-serialize toml-rs accepted both) and that's why there's two entries below :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #[serde(rename = "default_features")] down on default_features2 will handle that: https://play.rust-lang.org/?gist=db44b7c463a8b4ac43c0b67e2b36e5d0&version=stable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I missed that, nice!

pub struct DetailedTomlDependency {
version: Option<String>,
registry: Option<String>,
registry_index: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this use #[serde(rename)] to use registry-index instead?

path: Option<String>,
git: Option<String>,
branch: Option<String>,
tag: Option<String>,
rev: Option<String>,
features: Option<Vec<String>>,
optional: Option<bool>,
#[serde(rename = "default-features")]
default_features: Option<bool>,
#[serde(rename = "default_features")]
default_features2: Option<bool>,
}

#[derive(Debug, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub struct TomlManifest {
#[serde(rename = "cargo-features")]
cargo_features: Option<Vec<String>>,
package: Option<Box<TomlProject>>,
project: Option<Box<TomlProject>>,
Expand All @@ -207,11 +208,9 @@ pub struct TomlManifest {
test: Option<Vec<TomlTestTarget>>,
bench: Option<Vec<TomlTestTarget>>,
dependencies: Option<BTreeMap<String, TomlDependency>>,
#[serde(rename = "dev-dependencies")]
dev_dependencies: Option<BTreeMap<String, TomlDependency>>,
#[serde(rename = "dev_dependencies")]
dev_dependencies2: Option<BTreeMap<String, TomlDependency>>,
#[serde(rename = "build-dependencies")]
build_dependencies: Option<BTreeMap<String, TomlDependency>>,
#[serde(rename = "build_dependencies")]
build_dependencies2: Option<BTreeMap<String, TomlDependency>>,
Expand Down Expand Up @@ -471,13 +470,13 @@ struct Context<'a, 'b> {
}

impl TomlManifest {
pub fn prepare_for_publish(&self) -> TomlManifest {
pub fn prepare_for_publish(&self, config: &Config) -> CargoResult<TomlManifest> {
let mut package = self.package.as_ref()
.or_else(|| self.project.as_ref())
.unwrap()
.clone();
package.workspace = None;
return TomlManifest {
return Ok(TomlManifest {
package: Some(package),
project: None,
profile: self.profile.clone(),
Expand All @@ -486,56 +485,68 @@ impl TomlManifest {
example: self.example.clone(),
test: self.test.clone(),
bench: self.bench.clone(),
dependencies: map_deps(self.dependencies.as_ref()),
dev_dependencies: map_deps(self.dev_dependencies.as_ref()
.or_else(|| self.dev_dependencies2.as_ref())),
dependencies: map_deps(config, self.dependencies.as_ref())?,
dev_dependencies: map_deps(config, self.dev_dependencies.as_ref()
.or_else(|| self.dev_dependencies2.as_ref()))?,
dev_dependencies2: None,
build_dependencies: map_deps(self.build_dependencies.as_ref()
.or_else(|| self.build_dependencies2.as_ref())),
build_dependencies: map_deps(config, self.build_dependencies.as_ref()
.or_else(|| self.build_dependencies2.as_ref()))?,
build_dependencies2: None,
features: self.features.clone(),
target: self.target.as_ref().map(|target_map| {
target: match self.target.as_ref().map(|target_map| {
target_map.iter().map(|(k, v)| {
(k.clone(), TomlPlatform {
dependencies: map_deps(v.dependencies.as_ref()),
dev_dependencies: map_deps(v.dev_dependencies.as_ref()
.or_else(|| v.dev_dependencies2.as_ref())),
Ok((k.clone(), TomlPlatform {
dependencies: map_deps(config, v.dependencies.as_ref())?,
dev_dependencies: map_deps(config, v.dev_dependencies.as_ref()
.or_else(|| v.dev_dependencies2.as_ref()))?,
dev_dependencies2: None,
build_dependencies: map_deps(v.build_dependencies.as_ref()
.or_else(|| v.build_dependencies2.as_ref())),
build_dependencies: map_deps(config, v.build_dependencies.as_ref()
.or_else(|| v.build_dependencies2.as_ref()))?,
build_dependencies2: None,
})
}))
}).collect()
}),
}) {
Some(Ok(v)) => Some(v),
Some(Err(e)) => return Err(e),
None => None,
},
replace: None,
patch: None,
workspace: None,
badges: self.badges.clone(),
cargo_features: self.cargo_features.clone(),
};
});

fn map_deps(deps: Option<&BTreeMap<String, TomlDependency>>)
-> Option<BTreeMap<String, TomlDependency>>
fn map_deps(config: &Config, deps: Option<&BTreeMap<String, TomlDependency>>)
-> CargoResult<Option<BTreeMap<String, TomlDependency>>>
{
let deps = match deps {
Some(deps) => deps,
None => return None
None => return Ok(None),
};
Some(deps.iter().map(|(k, v)| (k.clone(), map_dependency(v))).collect())
let deps = deps.iter()
.map(|(k, v)| Ok((k.clone(), map_dependency(config, v)?)))
.collect::<CargoResult<BTreeMap<_, _>>>()?;
Ok(Some(deps))
}

fn map_dependency(dep: &TomlDependency) -> TomlDependency {
fn map_dependency(config: &Config, dep: &TomlDependency) -> CargoResult<TomlDependency> {
match *dep {
TomlDependency::Detailed(ref d) => {
let mut d = d.clone();
d.path.take(); // path dependencies become crates.io deps
TomlDependency::Detailed(d)
// registry specifications are elaborated to the index URL
if let Some(registry) = d.registry.take() {
let src = SourceId::alt_registry(config, &registry)?;
d.registry_index = Some(src.url().to_string());
}
Ok(TomlDependency::Detailed(d))
}
TomlDependency::Simple(ref s) => {
TomlDependency::Detailed(DetailedTomlDependency {
Ok(TomlDependency::Detailed(DetailedTomlDependency {
version: Some(s.clone()),
..Default::default()
})
}))
}
}
}
Expand Down Expand Up @@ -933,10 +944,18 @@ impl TomlDependency {
None => SourceId::crates_io(cx.config)?
};

let new_source_id = match (details.git.as_ref(), details.path.as_ref(), details.registry.as_ref()) {
(Some(_), _, Some(_)) => bail!("dependency ({}) specification is ambiguous. \
let new_source_id = match (
details.git.as_ref(),
details.path.as_ref(),
details.registry.as_ref(),
details.registry_index.as_ref(),
) {
(Some(_), _, Some(_), _) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day we will make this better. It is not this day.

(Some(_), _, _, Some(_))=> bail!("dependency ({}) specification is ambiguous. \
Only one of `git` or `registry` is allowed.", name),
(Some(git), maybe_path, _) => {
(_, _, Some(_), Some(_)) => bail!("dependency ({}) specification is ambiguous. \
Only one of `registry` or `registry-index` is allowed.", name),
(Some(git), maybe_path, _, _) => {
if maybe_path.is_some() {
let msg = format!("dependency ({}) specification is ambiguous. \
Only one of `git` or `path` is allowed. \
Expand All @@ -963,7 +982,7 @@ impl TomlDependency {
let loc = git.to_url()?;
SourceId::for_git(&loc, reference)?
},
(None, Some(path), _) => {
(None, Some(path), _, _) => {
cx.nested_paths.push(PathBuf::from(path));
// If the source id for the package we're parsing is a path
// source, then we normalize the path here to get rid of
Expand All @@ -981,8 +1000,12 @@ impl TomlDependency {
cx.source_id.clone()
}
},
(None, None, Some(registry)) => SourceId::alt_registry(cx.config, registry)?,
(None, None, None) => SourceId::crates_io(cx.config)?,
(None, None, Some(registry), None) => SourceId::alt_registry(cx.config, registry)?,
(None, None, None, Some(registry_index)) => {
let url = registry_index.to_url()?;
SourceId::for_registry(&url)?
}
(None, None, None, None) => SourceId::crates_io(cx.config)?,
};

let version = details.version.as_ref().map(|v| &v[..]);
Expand Down
36 changes: 35 additions & 1 deletion tests/alt-registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ extern crate hamcrest;

use cargotest::ChannelChanger;
use cargotest::support::registry::{self, Package, alt_api_path};
use cargotest::support::{project, execs};
use cargotest::support::{paths, project, execs};
use hamcrest::assert_that;
use std::fs::File;
use std::io::Write;

#[test]
fn is_feature_gated() {
Expand Down Expand Up @@ -423,3 +425,35 @@ fn publish_with_crates_io_dep() {
.arg("--registry").arg("alternative").arg("-Zunstable-options"),
execs().with_status(0));
}

#[test]
fn credentials_in_url_forbidden() {
registry::init();

let config = paths::home().join(".cargo/config");

File::create(config)
.unwrap()
.write_all(br#"
[registries.alternative]
index = "ssh://git:secret@foobar.com"
"#)
.unwrap();

let p = project("foo")
.file("Cargo.toml", r#"
cargo-features = ["alternative-registries"]

[project]
name = "foo"
version = "0.0.1"
authors = []
"#)
.file("src/main.rs", "fn main() {}")
.build();

assert_that(p.cargo("publish").masquerade_as_nightly_cargo()
.arg("--registry").arg("alternative").arg("-Zunstable-options"),
execs().with_status(101)
.with_stderr_contains("error: Registry URLs may not contain credentials"));
}
19 changes: 14 additions & 5 deletions tests/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use std::fs::File;
use std::io::prelude::*;
use std::path::{Path, PathBuf};

use cargotest::{cargo_process, process};
use cargotest::support::{project, execs, paths, git, path2url, cargo_exe};
use cargotest::{cargo_process, process, ChannelChanger};
use cargotest::support::{project, execs, paths, git, path2url, cargo_exe, registry};
use cargotest::support::registry::Package;
use flate2::read::GzDecoder;
use hamcrest::{assert_that, existing_file, contains, equal_to};
Expand Down Expand Up @@ -715,6 +715,8 @@ fn generated_manifest() {
Package::new("ghi", "1.0.0").publish();
let p = project("foo")
.file("Cargo.toml", r#"
cargo-features = ["alternative-registries"]

[project]
name = "foo"
version = "0.0.1"
Expand All @@ -730,7 +732,7 @@ fn generated_manifest() {

[dependencies]
bar = { path = "bar", version = "0.1" }
def = "1.0"
def = { version = "1.0", registry = "alternative" }
ghi = "1.0"
abc = "1.0"
"#)
Expand All @@ -744,7 +746,9 @@ fn generated_manifest() {
.file("bar/src/lib.rs", "")
.build();

assert_that(p.cargo("package").arg("--no-verify"),
assert_that(p.cargo("package")
.masquerade_as_nightly_cargo()
.arg("--no-verify"),
execs().with_status(0));

let f = File::open(&p.root().join("target/package/foo-0.0.1.crate")).unwrap();
Expand All @@ -761,6 +765,7 @@ fn generated_manifest() {
// BTreeMap makes the order of dependencies in the generated file deterministic
// by sorting alphabetically
assert_that(&contents[..], equal_to(
&*format!(
r#"# THIS FILE IS AUTOMATICALLY GENERATED BY CARGO
#
# When uploading crates to the registry Cargo will automatically
Expand All @@ -773,6 +778,8 @@ r#"# THIS FILE IS AUTOMATICALLY GENERATED BY CARGO
# editing this file be aware that the upstream Cargo.toml
# will likely look very different (and much more reasonable)

cargo-features = ["alternative-registries"]

[package]
name = "foo"
version = "0.0.1"
Expand All @@ -791,10 +798,12 @@ version = "0.1"

[dependencies.def]
version = "1.0"
registry-index = "{}"

[dependencies.ghi]
version = "1.0"
"#));
"#,
registry::alt_registry())));
}

#[test]
Expand Down