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

internal/ssh: ignore env command totally #6825

Merged
merged 4 commits into from May 9, 2019
Merged

internal/ssh: ignore env command totally #6825

merged 4 commits into from May 9, 2019

Conversation

sapk
Copy link
Member

@sapk sapk commented May 2, 2019

Looking at #6085, I discover that we do nothing with the env variable send via SSH. The env command only set the variable for the command in args so calling it without a command is useless.

Proof:

env TEST=1
env | grep TEST
-> The variable TEST is not the set for the second call of env
 -- vs --
env TEST=1 env | grep TEST
-> The variable is TEST is set for the env command in arg of the env command :-)

The best solution would be to list the variable each time the env is called and add them to the array cmd.Env when calling exec but I think we would need a bit of filtering and I don't have enough background to know what to authorize or not.
So I suggest to simply ignore them (like we did previously by calling the env lonely) this will fix #6085 and windows user without the env command.

@codecov-io
Copy link

codecov-io commented May 2, 2019 •
edited

Codecov Report

Merging #6825 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6825      +/-   ##
==========================================
- Coverage   41.41%   41.41%   -0.01%     
==========================================
  Files         432      432              
  Lines       59552    59541      -11     
==========================================
- Hits        24661    24656       -5     
+ Misses      31652    31646       -6     
  Partials     3239     3239
Impacted Files Coverage Δ
modules/ssh/ssh.go 59.47% <ø> (+3.98%) ⬆️
modules/log/event.go 64.46% <0%> (-1.53%) ⬇️
models/repo_list.go 66.84% <0%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f21bc3...03038d0. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 2, 2019
@techknowlogick techknowlogick added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label May 3, 2019
@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 May 8, 2019
@techknowlogick
Copy link
Member

Looks good. I think the code can be entirely removed instead of just being commented out, but I'm ok either way.

@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 May 8, 2019
@lunny
Copy link
Member

lunny commented May 9, 2019

Why not remove them but commented?

@sapk
Copy link
Member Author

sapk commented May 9, 2019

I kept it because in the future we should implement it and it explain why we have a switch with only one match left. But I can remove it if needed.
In the meantime, I will create an issue related to the need to parse env command.

@sapk sapk mentioned this pull request May 9, 2019
Open
@lafriks
Copy link
Member

lafriks commented May 9, 2019 •
edited

I prefer that unneeded code is deleted :) if needed it can be found in git history

@sapk
Remove commented code
5824a5a 
Needed fix described in issue go-gitea#6889
@sapk
Copy link
Member Author

sapk commented May 9, 2019

@lunny @lafriks done

@techknowlogick techknowlogick merged commit 10ff527 into go-gitea:master May 9, 2019
@sapk sapk deleted the internal-ssh-disable-env branch May 9, 2019 23:06
@sapk sapk mentioned this pull request May 9, 2019
Closed
7 tasks
@sapk
Copy link
Member Author

sapk commented May 9, 2019

Should I backport this PR ?

@lunny lunny added this to the 1.9.0 milestone May 10, 2019
@lunny
Copy link
Member

lunny commented May 10, 2019

@sapk I don't think this need be back port since it's not a serious one.

zeripath pushed a commit to zeripath/gitea that referenced this pull request May 11, 2019
@sapk @zeripath
internal/ssh: ignore env command totally (go-gitea#6825)
3c8b4fc 
* ssh: ignore env command totally

* Remove commented code 

Needed fix described in issue go-gitea#6889
techknowlogick pushed a commit that referenced this pull request May 11, 2019
@zeripath @techknowlogick
Remove local clones & make hooks run on merge/edit/upload (#6672)
ce8de35 
* Add options to git.Clone to make it more capable

* Begin the process of removing the local copy and tidy up

* Remove Wiki LocalCopy Checkouts

* Remove the last LocalRepo helpers

* Remove WithTemporaryFile

* Enable push-hooks for these routes

* Ensure tests cope with hooks

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Remove Repository.LocalCopyPath()

* Move temporary repo to use the standard temporary path

* Fix the tests

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Remove LocalWikiPath

* Fix missing remove

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Use AppURL for Oauth user link (#6894)

* Use AppURL for Oauth user link

Fix #6843

* Update oauth.go

* Update oauth.go

* internal/ssh: ignore env command totally (#6825)

* ssh: ignore env command totally

* Remove commented code 

Needed fix described in issue #6889

* Escape the commit message on issues update and title in telegram hook (#6901)

* update sdk to latest (#6903)

* improve description of branch protection (fix #6886) (#6906)

The branch protection description text were not quite accurate.

* Fix logging documentation (#6904)

* ENABLE_MACARON_REDIRECT should be REDIRECT_MACARON_LOG

* Allow DISABLE_ROUTER_LOG to be set in the [log] section

* [skip ci] Updated translations via Crowdin

* Move sdk structs to modules/structs (#6905)

* move sdk structs to moduels/structs

* fix tests

* fix fmt

* fix swagger

* fix vendor
@cybe cybe mentioned this pull request Aug 2, 2019
Closed
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@techknowlogick techknowlogick techknowlogick approved these changes

@kolaente kolaente kolaente approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.

Builtin SSH server bug when client sends environment variables
7 participants
@sapk @codecov-io @techknowlogick @lunny @lafriks @kolaente @GiteaBot