Do not allow empty name in package ID spec #13152

hi-rustin · 2023-12-11T01:54:04Z

What does this PR try to resolve?

Do not allow empty names in package ID spec.

See: hi-rustin/cargo-information#63

How should we test and review this PR?

Check unit tests.

Additional information

rustbot · 2023-12-11T01:54:09Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

src/cargo/core/package_id_spec.rs

Signed-off-by: hi-rustin <[email protected]>

hi-rustin

🔢 Self-check

hi-rustin · 2023-12-11T02:15:03Z

src/cargo/util_schemas/core/package_id_spec.rs

@@ -98,6 +98,9 @@ impl PackageIdSpec {
            Some(version) => Some(version.parse::<PartialVersion>()?),
            None => None,
        };
+        if name.is_empty() {
+            bail!("package ID specification must have a name: `{spec}`");
+        }
        validate_package_name(name, "pkgid", "")?;


Maybe we should put the empty check into this function?

That seems like where it should belong but we already have that check be separate for manifests

let package_name = package.name.trim(); if package_name.is_empty() { bail!("package name cannot be an empty string") } validate_package_name(package_name, "package name", "")?;

This makes me think some more investigation is needed.

Note: empty feature names were disallowed as of #12928 but that was because we relied on the behavior of our TOML parser to disallow it which wasn't conformant behavior.

I have checked all the references for this function and it seems okay to include the check in the validate_package_name. However, I have also discovered some bugs in those places. For example, if you attempt to type cargo add @1.2.3, you will receive a panic instead of an error. Therefore, I suggest we move forward with this pull request first. Once that's done, I can work on moving it into the validate_package_name and also add tests to cover these problematic cases. This PR is more focused on fixing bugs from package ID spec.

As a heads up, I'm looking at touching the name validation code as well, so it'll be good to coordinate so we don't cause each other a lot of pain

Got it. I will prioritize ensuring that empty names are not allowed. I will make sure I only change one thing each time and do not bring any off-topic changes. Hope that can help.

epage · 2023-12-12T02:22:20Z

@bors r+

bors · 2023-12-12T02:22:24Z

📌 Commit 1248522 has been approved by epage

It is now in the queue for this repository.

bors · 2023-12-12T02:23:31Z

⌛ Testing commit 1248522 with merge f2b2438...

Do not allow empty name in package ID spec

bors · 2023-12-12T02:33:18Z

💔 Test failed - checks-actions

hi-rustin · 2023-12-12T02:42:02Z

The CI is broken by rust-lang/rust@f481596

hi-rustin · 2023-12-12T02:42:38Z

I am working on fixing it.

hi-rustin · 2023-12-13T00:10:48Z

@epage Could you please merge it again? Thanks!

epage · 2023-12-13T01:21:29Z

@bors r+

bors · 2023-12-13T01:21:32Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2023-12-13T01:21:32Z

📌 Commit 1248522 has been approved by epage

It is now in the queue for this repository.

bors · 2023-12-13T01:22:39Z

⌛ Testing commit 1248522 with merge 8412d30...

bors · 2023-12-13T02:05:21Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 8412d30 to master...

fix: Fill in more empty name holes ### What does this PR try to resolve? This is a follow up to #13152 and expands on work done in #12928. This is also part of #12801 as I am refactoring how we do validation so that it parts of it can move into the schema, removing the last dependency the schema has on the rest of cargo. ### How should we test and review this PR? This prevents empty registry names which should be fine for the same reason as preventing empty feature names in #12928, that this was a regression due to changes in toml parsers ### Additional information

Update cargo 10 commits in 1aa9df1a5be205cce621f0bc0ea6062a5e22a98c..37bc5f0232a0bb72dedd2c14149614fd8cdae649 2023-12-12 14:52:31 +0000 to 2023-12-15 18:33:31 +0000 - docs(home): prepare the changelog for 0.5.9 (rust-lang/cargo#13177) - refactor: Pull name validation into `util_schemas` (rust-lang/cargo#13166) - chore(deps): bump zerocopy from 0.7.29 to 0.7.31 (rust-lang/cargo#13174) - Replace SHGetFolderPathW with SHGetKnownFolderPath (rust-lang/cargo#13173) - chore(bump-check): dont check `home` against beta/stable branches (rust-lang/cargo#13167) - fix: Fill in more empty name holes (rust-lang/cargo#13164) - Do not allow empty name in package ID spec (rust-lang/cargo#13152) - chore(deps): update rust crate openssl to 0.10.61 (rust-lang/cargo#13159) - `all-static` feature should include `vendored-libgit2` (rust-lang/cargo#13134) - doc/registry-web-api: Adjust success response code documentation (rust-lang/cargo#13160) r? ghost

Update cargo 11 commits in 1aa9df1a5be205cce621f0bc0ea6062a5e22a98c..1a2666ddd14cf0a255d4ddb61c63531c259a7b39 2023-12-12 14:52:31 +0000 to 2023-12-17 17:53:53 +0000 - chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13179) - docs(home): prepare the changelog for 0.5.9 (rust-lang/cargo#13177) - refactor: Pull name validation into `util_schemas` (rust-lang/cargo#13166) - chore(deps): bump zerocopy from 0.7.29 to 0.7.31 (rust-lang/cargo#13174) - Replace SHGetFolderPathW with SHGetKnownFolderPath (rust-lang/cargo#13173) - chore(bump-check): dont check `home` against beta/stable branches (rust-lang/cargo#13167) - fix: Fill in more empty name holes (rust-lang/cargo#13164) - Do not allow empty name in package ID spec (rust-lang/cargo#13152) - chore(deps): update rust crate openssl to 0.10.61 (rust-lang/cargo#13159) - `all-static` feature should include `vendored-libgit2` (rust-lang/cargo#13134) - doc/registry-web-api: Adjust success response code documentation (rust-lang/cargo#13160) r? ghost

rustbot assigned weihanglo Dec 11, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2023

weihanglo reviewed Dec 11, 2023

View reviewed changes

src/cargo/core/package_id_spec.rs Outdated Show resolved Hide resolved

hi-rustin added 5 commits December 11, 2023 10:08

test: add a case for parsing empty names

a7ff31f

Signed-off-by: hi-rustin <[email protected]>

fix: validate if name is empty

b1642b5

Signed-off-by: hi-rustin <[email protected]>

test: add a case for parsing URL spec without name

442636b

Signed-off-by: hi-rustin <[email protected]>

test: add a case for bad package URLs starting with numbers

4dc1178

Signed-off-by: hi-rustin <[email protected]>

fix: validate package name from URL

1248522

Signed-off-by: hi-rustin <[email protected]>

hi-rustin force-pushed the rustin-patch-empty-name branch from bdfa10e to 1248522 Compare December 11, 2023 02:11

hi-rustin commented Dec 11, 2023

View reviewed changes

hi-rustin marked this pull request as ready for review December 11, 2023 02:15

hi-rustin mentioned this pull request Dec 11, 2023

fix: do now allow empty spec name hi-rustin/cargo-information#63

Merged

hi-rustin changed the title ~~test: add an invalid parsing case~~ Do not allow empty name in package ID spec Dec 11, 2023

hi-rustin requested review from weihanglo and epage December 11, 2023 02:28

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2023

bors added a commit that referenced this pull request Dec 12, 2023

Auto merge of #13152 - hi-rustin:rustin-patch-empty-name, r=epage

f2b2438

Do not allow empty name in package ID spec

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 12, 2023

This was referenced Dec 12, 2023

Remove edition umbrella features. rust-lang/rust#118802

Merged

Remove the deleted feature test_2018_feature from the test #13156

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2023

bors merged commit 8412d30 into rust-lang:master Dec 13, 2023
20 checks passed

epage mentioned this pull request Dec 13, 2023

fix: Fill in more empty name holes #13164

Merged

weihanglo mentioned this pull request Dec 16, 2023

Update cargo rust-lang/rust#119007

Merged

epage mentioned this pull request Jan 17, 2024

Better error report when crate name is empty #13314

Closed

ehuss added this to the 1.76.0 milestone Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not allow empty name in package ID spec #13152

Do not allow empty name in package ID spec #13152

hi-rustin commented Dec 11, 2023

rustbot commented Dec 11, 2023

hi-rustin left a comment

hi-rustin Dec 11, 2023

epage Dec 11, 2023

hi-rustin Dec 12, 2023

epage Dec 12, 2023

hi-rustin Dec 12, 2023

epage commented Dec 12, 2023

bors commented Dec 12, 2023

bors commented Dec 12, 2023

bors commented Dec 12, 2023

hi-rustin commented Dec 12, 2023

hi-rustin commented Dec 12, 2023

hi-rustin commented Dec 13, 2023

epage commented Dec 13, 2023

bors commented Dec 13, 2023

bors commented Dec 13, 2023

bors commented Dec 13, 2023

bors commented Dec 13, 2023

Do not allow empty name in package ID spec #13152

Do not allow empty name in package ID spec #13152

Conversation

hi-rustin commented Dec 11, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Dec 11, 2023

hi-rustin left a comment

Choose a reason for hiding this comment

hi-rustin Dec 11, 2023

Choose a reason for hiding this comment

epage Dec 11, 2023

Choose a reason for hiding this comment

hi-rustin Dec 12, 2023

Choose a reason for hiding this comment

epage Dec 12, 2023

Choose a reason for hiding this comment

hi-rustin Dec 12, 2023

Choose a reason for hiding this comment

epage commented Dec 12, 2023

bors commented Dec 12, 2023

bors commented Dec 12, 2023

bors commented Dec 12, 2023

hi-rustin commented Dec 12, 2023

hi-rustin commented Dec 12, 2023

hi-rustin commented Dec 13, 2023

epage commented Dec 13, 2023

bors commented Dec 13, 2023

bors commented Dec 13, 2023

bors commented Dec 13, 2023

bors commented Dec 13, 2023