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

Warn on SSH connection for incorrect configuration #19317

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Apr 3, 2022

  • When setting.RepoRootPath cannot be found(most likely due to incorrect configuration) show "Gitea: Incorrect configuration" on the client-side to help easier with debugging the problem.

- When `setting.RepoRootPath` cannot be found(most likely due to
incorrect configuration) show "Gitea: Incorrect configuration" on the
client-side to help easier with debugging the problem.
@Gusted Gusted added this to the 1.17.0 milestone Apr 3, 2022
@Gusted Gusted added type/enhancement An improvement of existing functionality backport/v1.16 labels Apr 3, 2022
cmd/serv.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 4, 2022
Co-authored-by: delvh <dev.lh@web.de>
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

fail(RepoRootPath) would leak some server information to end users.

Since it's a rare case and shouldn't occur when Gitea works correctly, I can accept it.

A better approach is only showing the RepoRootPath in logs.


Update: only the first arg of fail (aka userMessage) will be returned to end user, while the second arg won't and it's for server log. Sorry for the misleading.

@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 Apr 5, 2022
@lunny
Copy link
Member

lunny commented Apr 5, 2022

setting.RepoRootPath should not be leaked to client users.

cmd/serv.go Outdated Show resolved Hide resolved
@Gusted Gusted requested review from delvh and wxiaoguang April 5, 2022 15:44
@Gusted
Copy link
Contributor Author

Gusted commented Apr 5, 2022

A better approach is only showing the RepoRootPath in logs.

That's a problem, those kinds of logs would only be show when ENABLE_SSH_LOG is enabled(which currently isn't mentioned in the docs, see #19316).

cmd/serv.go Outdated Show resolved Hide resolved
@codecov-commenter

This comment was marked as outdated.

@techknowlogick techknowlogick merged commit 606e33d into go-gitea:main Apr 5, 2022
@Gusted Gusted deleted the warn-incorrect-configuration branch April 5, 2022 20:32
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 11, 2022
* giteaofficial/main: (22 commits)
  Add logic to switch between source/rendered on Markdown (go-gitea#19356)
  Fixed registry host value. (go-gitea#19363)
  [skip ci] Updated translations via Crowdin
  Allow package linking to private repository (go-gitea#19348)
  Use "main" as default branch name (go-gitea#19354)
  Move milestone to models/issues/ (go-gitea#19278)
  Refactor CSRF protection modules, make sure CSRF tokens can be up-to-date. (go-gitea#19337)
  Remove dependent on session auth for api/v1 routers (go-gitea#19321)
  API: Search Issues, dont show 500 if filter result in empty list (go-gitea#19244)
  [skip ci] Updated translations via Crowdin
  Never use /api/v1 from Gitea UI Pages (go-gitea#19318)
  [skip ci] Updated translations via Crowdin
  Show ssh command directly in template instead of i18n translation (go-gitea#19335)
  Package registry changes (go-gitea#19305)
  [skip ci] Updated translations via Crowdin
  Add `ENABLE_SSH_LOG` to debugging problems (go-gitea#19316)
  Warn on SSH connection for incorrect configuration (go-gitea#19317)
  escape fake link
  Allow custom redirect for landing page (go-gitea#19324)
  [skip ci] Updated translations via Crowdin
  ...
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Apr 20, 2022
* Warn on SSH connection for incorrect configuration

- When `setting.RepoRootPath` cannot be found(most likely due to
incorrect configuration) show "Gitea: Incorrect configuration" on the
client-side to help easier with debugging the problem.

* Update cmd/serv.go

Co-authored-by: delvh <dev.lh@web.de>

* Don't leak configuration

* Update cmd/serv.go

Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@6543
Copy link
Member

6543 commented Apr 20, 2022

-> #19437

@6543 6543 added the backport/done All backports for this PR have been created label Apr 20, 2022
6543 added a commit that referenced this pull request Apr 20, 2022
Backport #19317

- Warn on SSH connection for incorrect configuration
- When `setting.RepoRootPath` cannot be found(most likely due to
incorrect configuration) show "Gitea: Incorrect configuration" on the
client-side to help easier with debugging the problem.

Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@wxiaoguang
Copy link
Contributor

To correct the review before (which was incorrect):

fail(RepoRootPath) would leak some server information to end users.

Since it's a rare case and shouldn't occur when Gitea works correctly, I can accept it.

A better approach is only showing the RepoRootPath in logs.

Only the first arg of fail (aka userMessage) will be returned to end user, while the second arg won't and it's for server log. Sorry for the misleading.

AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* Warn on SSH connection for incorrect configuration

- When `setting.RepoRootPath` cannot be found(most likely due to
incorrect configuration) show "Gitea: Incorrect configuration" on the
client-side to help easier with debugging the problem.

* Update cmd/serv.go

Co-authored-by: delvh <dev.lh@web.de>

* Don't leak configuration

* Update cmd/serv.go

Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Gusted added a commit to Gusted/gitea that referenced this pull request Feb 4, 2023
- This should prevent SSH failures from happening as described in
go-gitea#19317
Gusted added a commit to Gusted/gitea that referenced this pull request Feb 4, 2023
- Backport go-gitea#22754
  - This should prevent SSH failures from happening as described in go-gitea#19317
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 4, 2023
If a user's `app.ini` contains a `APP_DATA_PATH` which refers to a
non-absolute path then `gitea serv` etc. become dependent on the
`AppWorkPath`.

`gitea serv` may then require `--work-path` to be set in the
`authorized_keys` if the `AppWorkPath` determined by `gitea web` and
`gitea serv` are different.

This would occur if `GITEA_WORK_DIR` is set, `--work-path` is used to
run `gitea web` or if the AppPath cannot be determined at start-up.

This PR adds some code to attempt to automatically determine if this is
necessary and adds some documentation to suggest adding `--work-path` to
the template.

This should prevent SSH failures from happening as described in go-gitea#19317

Replace go-gitea#22754

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants