Do not allow empty name in package ID spec by 0xPoe · Pull Request #13152 · rust-lang/cargo

0xPoe · 2023-12-11T01:54:04Z

What does this PR try to resolve?

Do not allow empty names in package ID spec.

See: 0xPoe/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 <rustin.liu@gmail.com>

0xPoe

🔢 Self-check

0xPoe · 2023-12-11T02:15:03Z

src/cargo/util_schemas/core/package_id_spec.rs

+        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

0xPoe · 2023-12-12T02:42:02Z

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

0xPoe · 2023-12-12T02:42:38Z

I am working on fixing it.

0xPoe · 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

0xPoe added 5 commits December 11, 2023 10:08

test: add a case for parsing empty names

a7ff31f

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

fix: validate if name is empty

b1642b5

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

test: add a case for parsing URL spec without name

442636b

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

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

4dc1178

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

fix: validate package name from URL

1248522

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

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

0xPoe commented Dec 11, 2023

View reviewed changes

0xPoe marked this pull request as ready for review December 11, 2023 02:15

0xPoe mentioned this pull request Dec 11, 2023

fix: do now allow empty spec name 0xPoe/cargo-information#63

Merged

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

0xPoe requested review from epage and weihanglo 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

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

Conversation

0xPoe commented Dec 11, 2023

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Dec 11, 2023

Uh oh!

Uh oh!

0xPoe left a comment

Choose a reason for hiding this comment

Uh oh!

0xPoe Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

epage Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

0xPoe Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

epage Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

0xPoe Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

epage commented Dec 12, 2023

Uh oh!

bors commented Dec 12, 2023

Uh oh!

bors commented Dec 12, 2023

Uh oh!

bors commented Dec 12, 2023

Uh oh!

0xPoe commented Dec 12, 2023

Uh oh!

0xPoe commented Dec 12, 2023

Uh oh!

0xPoe commented Dec 13, 2023

Uh oh!

epage commented Dec 13, 2023

Uh oh!

bors commented Dec 13, 2023

Uh oh!

bors commented Dec 13, 2023

Uh oh!

bors commented Dec 13, 2023

Uh oh!

bors commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants