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

internal/ssh: ignore env command totally #6825

Merged
merged 4 commits into from May 9, 2019

Conversation

7 participants
@sapk
Copy link
Member

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

This comment has been minimized.

Copy link

commented May 2, 2019

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 lgtm/need 1 and removed lgtm/need 2 labels May 8, 2019

@techknowlogick

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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 and removed lgtm/need 1 labels May 8, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Why not remove them but commented?

@sapk

This comment has been minimized.

Copy link
Member Author

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.

@lafriks

This comment has been minimized.

Copy link
Member

commented May 9, 2019

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

Remove commented code
Needed fix described in issue #6889
@sapk

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@techknowlogick techknowlogick merged commit 10ff527 into go-gitea:master May 9, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details

@sapk sapk deleted the sapk-fork:internal-ssh-disable-env branch May 9, 2019

@sapk sapk referenced this pull request May 9, 2019

Closed

Builtin SSH server bug when client sends environment variables #6085

2 of 7 tasks complete
@sapk

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Should I backport this PR ?

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

@lunny

This comment has been minimized.

Copy link
Member

commented May 10, 2019

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

zeripath added a commit to zeripath/gitea that referenced this pull request May 11, 2019

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

* Remove commented code 

Needed fix described in issue go-gitea#6889

techknowlogick added a commit that referenced this pull request May 11, 2019

Remove local clones & make hooks run on merge/edit/upload (#6672)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.