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.

Sign up for GitHub

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

Jump to bottom

Add repository login component username validation #3166

Merged
merged 1 commit into from
Jul 14, 2024
Merged

Add repository login component username validation #3166

merged 1 commit into from
Jul 14, 2024

Conversation

Shikachuu
Copy link
Contributor

Validate username in login.go command

Fixes #3069

@apostasie
Copy link
Contributor

Thanks @Shikachuu !

Left a couple of comments.
Looks to me like the CI failure is unrelated, but let's see.

@Shikachuu
Copy link
Contributor Author

Thanks @Shikachuu !

Left a couple of comments. Looks to me like the CI failure is unrelated, but let's see.

Ngl I don't see any comments but this one, where should I find those?

Checked the CI too, at first glance it doesn't seem login related.

apostasie
apostasie suggested changes Jul 4, 2024
View reviewed changes
cmd/nerdctl/login.go Outdated Show resolved Hide resolved
cmd/nerdctl/login.go Outdated Show resolved Hide resolved
@apostasie
Copy link
Contributor

Thanks @Shikachuu !
Left a couple of comments. Looks to me like the CI failure is unrelated, but let's see.

Ngl I don't see any comments but this one, where should I find those?

Checked the CI too, at first glance it doesn't seem login related.

My bad - forgot to press the big green button to send these comments :p

You should see them now.

@Shikachuu Shikachuu force-pushed the fix-login-username branch 3 times, most recently from 74e46ef to 547df82 Compare July 5, 2024 15:43
@apostasie
Copy link
Contributor

@Shikachuu can you rebase on the latest main? There has been some kerfuffle earlier on project checks (my fault). Hopefully rebasing should fix it.

@Shikachuu
Copy link
Contributor Author

@Shikachuu can you rebase on the latest main? There has been some kerfuffle earlier on project checks (my fault). Hopefully rebasing should fix it.

✔️ done, hopefully fixes it!

@fahedouch fahedouch added this to the v2.0.0 milestone Jul 6, 2024
@fahedouch fahedouch added the area/login authentification/ login label Jul 6, 2024
fahedouch
fahedouch reviewed Jul 6, 2024
View reviewed changes
cmd/nerdctl/login.go Outdated Show resolved Hide resolved
@apostasie
Copy link
Contributor

@Shikachuu can you rebase on the latest main? There has been some kerfuffle earlier on project checks (my fault). Hopefully rebasing should fix it.

✔️ done, hopefully fixes it!

It does.
Thanks a lot @Shikachuu !

LGTM as far as I am concerned (I am not a maintainer here, so, this is not formal approval).

AkihiroSuda
AkihiroSuda reviewed Jul 7, 2024
View reviewed changes
cmd/nerdctl/login.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda approved these changes Jul 14, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit d9a9a83 into containerd:main Jul 14, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fahedouch fahedouch fahedouch left review comments

@apostasie apostasie apostasie requested changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
area/login authentification/ login
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

nerdctl login allows passing a username with a colon, which is forbidden by RFC 7617
4 participants
@Shikachuu @apostasie @AkihiroSuda @fahedouch