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

Update reserved usernames list #18438

Merged
merged 19 commits into from
Mar 31, 2022
Merged

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Jan 28, 2022

  • Adding additional usernames which are already routes, it's safe to add them as any existing users should already have been "broken"(shouldn't be able to access profile etc.)

- Adding additional usernames which are already routes, it's safe to add
them as any existing users should already have been "broken"(shouldn't
be able to access profile etc.)
@Gusted Gusted added the type/enhancement An improvement of existing functionality label Jan 28, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jan 28, 2022
@wxiaoguang
Copy link
Contributor

It's better to use the char ~ for all new paths in future, then we do not need to reserve anything more.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 28, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 28, 2022

Why serviceworker.js was reserved, it's under /assets/

(I mean the old code, since we are improving the code, maybe we could improve the legacy together)

@Gusted
Copy link
Contributor Author

Gusted commented Jan 28, 2022

Why serviceworker.js was reserved, it's under /assets/

Was introduced in #15834 #15834 (comment)

@wxiaoguang
Copy link
Contributor

Why serviceworker.js was reserved, it's under /assets/

Was introduced in #15834 #15834 (comment)

I can not get the point why should we keep it in the latest code base, maybe it's time to remove it?

And "less" (maybe more) could also be removed.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 28, 2022
@Gusted
Copy link
Contributor Author

Gusted commented Jan 28, 2022

I can not get the point why should we keep it in the latest code base, maybe it's time to remove it?

🤷🏽‍♀️ I'm not sure, better to leave it in then or silverwind gives the green light.

@lunny
Copy link
Member

lunny commented Jan 28, 2022

help and plugins should be kept for future usage.

@wxiaoguang
Copy link
Contributor

help and plugins should be kept for future usage.

What about using the char ~ for all new paths in future, then we do not need to reserve anything more.

For example: /~plugins/, no conflict anymore, no one would be hurt.

@lunny
Copy link
Member

lunny commented Jan 28, 2022

Or /-/ :)

@wxiaoguang
Copy link
Contributor

Yep, /-/ is also widely used in many systems.

@Gusted
Copy link
Contributor Author

Gusted commented Jan 28, 2022

So the removal of those "futured reserved names" is good?

@lunny
Copy link
Member

lunny commented Jan 28, 2022

So the removal of those "futured reserved names" is good?

After we have an agreement of /-/.

@wxiaoguang
Copy link
Contributor

From my experience, if we say about "agreement", most people just keep silence, a few people just disagree for no reason explained, then many useful ideas cannot be made to an agreement.

@Gusted
Copy link
Contributor Author

Gusted commented Jan 28, 2022

From my experience, if we say about "agreement", most people just keep silence, a few people just disagree for no reason explained, then many useful ideas cannot be made to an agreement.

Well, what about documenting a similar idea/process as Go has? https://github.com/golang/proposal#the-proposal-process whereby it's a "simple" agreed if nobody has a objection to it. Such that certain changes can be discussed and actually being worked on instead of being stalled because nobody wants to speak up.

@wxiaoguang
Copy link
Contributor

Most communities have similar processes. besides Golang, there are Python PEP https://www.python.org/dev/peps/#meta-peps-peps-about-peps-or-processes , PHP RFC https://wiki.php.net/rfc , etc.

If Gitea can have such documents, it would help, however, such documents need an agreement again. I hope we can get right things done and move on.

@wxiaoguang
Copy link
Contributor

Well, some comments are a liitle off-topic. Let back to this PR.

I think @lunny @Gusted and I all agree we can use /-/ in future and do not reserve any name, and remove the unused reserved names. Am I right?

And @go-gitea/maintainers , does any one have objection and new suggestions?

@KN4CK3R
Copy link
Member

KN4CK3R commented Jan 29, 2022

For the Docker/Open Container api I will need /v2/ (https://docs.docker.com/registry/spec/api/) and you can't change that to something else (like /<user>/v2).

@zeripath
Copy link
Contributor

not even /api/v2?

@KN4CK3R
Copy link
Member

KN4CK3R commented Jan 29, 2022

The last time I checked this I did not find a way. Here is the Gitlab change for example: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/10422 But I will test it again tomorrow.

@KN4CK3R
Copy link
Member

KN4CK3R commented Jan 31, 2022

Tested it again. The official Docker server has support for a different endpoint but the Docker CLI does not support it.
The spec does not allow a prefix too: https://github.com/opencontainers/distribution-spec/blob/v1.0.0/spec.md#api

Compose file:

version: '3'

services:
  docker-registry:
    image: registry:2
    environment:
      - REGISTRY_HTTP_PREFIX=/test/sub/

Opening <domain>/test/sub/v2 in a browser yields the expected JSON response.

docker tag ubuntu:latest <domain>/test/sub/ubuntu:latest
docker push <domain>/test/sub/ubuntu:latest

results in

"GET /v2/ HTTP/1.1" 404 19 "" "docker/20.10.12 go/go1.16.12 git-commit/459d0df kernel/5.10.60.1-microsoft-standard-WSL2 os/linux arch/amd64 UpstreamClient(Docker-Client/20.10.12 \\(windows\\))"
"HEAD /v2/test/sub/ubuntu/blobs/sha256:f3ef4ff62e0da0ef761ec1c8a578f3035bef51043e53ae1b13a20b3e03726d17 HTTP/1.1" 404 19 "" "docker/20.10.12 go/go1.16.12 git-commit/459d0df kernel/5.10.60.1-microsoft-standard-WSL2 os/linux arch/amd64 UpstreamClient(Docker-Client/20.10.12 \\(windows\\))"
"POST /v2/test/sub/ubuntu/blobs/uploads/ HTTP/1.1" 404 19 "" "docker/20.10.12 go/go1.16.12 git-commit/459d0df kernel/5.10.60.1-microsoft-standard-WSL2 os/linux arch/amd64 UpstreamClient(Docker-Client/20.10.12 \\(windows\\))"

The CLI does not know about the prefix and prepends the v2.

References:
kubernetes/ingress-nginx#4027
https://stackoverflow.com/questions/65888391/docker-registry-with-prefix-has-problem-with-docker-login
docker/cli#3255

@lunny
Copy link
Member

lunny commented Jan 31, 2022

Tested it again. The official Docker server has support for a different endpoint but the Docker CLI does not support it. The spec does not allow a prefix too: https://github.com/opencontainers/distribution-spec/blob/v1.0.0/spec.md#api

Compose file:

version: '3'

services:
  docker-registry:
    image: registry:2
    environment:
      - REGISTRY_HTTP_PREFIX=/test/sub/

Opening <domain>/test/sub/v2 in a browser yields the expected JSON response.

docker tag ubuntu:latest <domain>/test/sub/ubuntu:latest
docker push <domain>/test/sub/ubuntu:latest

results in

"GET /v2/ HTTP/1.1" 404 19 "" "docker/20.10.12 go/go1.16.12 git-commit/459d0df kernel/5.10.60.1-microsoft-standard-WSL2 os/linux arch/amd64 UpstreamClient(Docker-Client/20.10.12 \\(windows\\))"
"HEAD /v2/test/sub/ubuntu/blobs/sha256:f3ef4ff62e0da0ef761ec1c8a578f3035bef51043e53ae1b13a20b3e03726d17 HTTP/1.1" 404 19 "" "docker/20.10.12 go/go1.16.12 git-commit/459d0df kernel/5.10.60.1-microsoft-standard-WSL2 os/linux arch/amd64 UpstreamClient(Docker-Client/20.10.12 \\(windows\\))"
"POST /v2/test/sub/ubuntu/blobs/uploads/ HTTP/1.1" 404 19 "" "docker/20.10.12 go/go1.16.12 git-commit/459d0df kernel/5.10.60.1-microsoft-standard-WSL2 os/linux arch/amd64 UpstreamClient(Docker-Client/20.10.12 \\(windows\\))"

The CLI does not know about the prefix and prepends the v2.

References: kubernetes/ingress-nginx#4027 https://stackoverflow.com/questions/65888391/docker-registry-with-prefix-has-problem-with-docker-login docker/cli#3255

So we need to add v2 as reserved name.

@fnetX
Copy link
Contributor

fnetX commented Mar 4, 2022

Regarding "-", I think it's fine for all technical routes (like assets, avatars etc), but for everything visible to the end user, I personally prefer fancy URLs (/user, /settings, /admin etc). They are more intuitive to type and share (I often type URLs myself, and would probably forget about the dash easily).

@Gusted
Copy link
Contributor Author

Gusted commented Mar 4, 2022

Regarding "-", I think it's fine for all technical routes (like assets, avatars etc), but for everything visible to the end user, I personally prefer fancy URLs (/user, /settings, /admin etc). They are more intuitive to type and share (I often type URLs myself, and would probably forget about the dash easily).

The - would probably only be used for new routes(which currently is none in the planning IIRC)

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 27, 2022
@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Mar 28, 2022
@lunny
Copy link
Member

lunny commented Mar 28, 2022

OK. Some instances may want to customize their reserved names list. So we may could also add a new list in ini configuration.

@Gusted
Copy link
Contributor Author

Gusted commented Mar 28, 2022

OK. Some instances may want to customize their reserved names list. So we may could also add a new list in ini configuration.

Should that be added within this PR? Seems like a good idea.

@lunny
Copy link
Member

lunny commented Mar 28, 2022

OK. Some instances may want to customize their reserved names list. So we may could also add a new list in ini configuration.

Should that be added within this PR? Seems like a good idea.

A standalone PR maybe reviewed faster.

@wxiaoguang wxiaoguang merged commit 43332a4 into go-gitea:main Mar 31, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 31, 2022
* giteaofficial/main:
  Update reserved usernames list (go-gitea#18438)
  Configure OpenSSH log level via Environment in Docker (go-gitea#19274)
  Use a more general (and faster) method to sanitize URLs with credentials (go-gitea#19239)
  [skip ci] Updated translations via Crowdin
  fix link to package registry docs (go-gitea#19268)
  Add Redis Sentinel Authentication Support (go-gitea#19213)
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
@Gusted Gusted deleted the update-reserved-usernames branch July 30, 2022 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants