Skip to content

Change the working directory if we can't stat it #6559

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

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Apr 9, 2019

Attempt to change the working directory if we can't stat it. This hopefully runs before the macaron init but may need to be moved to a file called a.go

Fix #4634

@zeripath
Copy link
Contributor Author

OK this is now working but it's not excellent as it doesn't inform us that we are moving a temporary directory. This needs to be logged somehow but will require thought - for example - we can't log from gitea serv as this would break git.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 10, 2019
@codecov-io
Copy link

codecov-io commented Apr 10, 2019

Codecov Report

Merging #6559 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6559   +/-   ##
=======================================
  Coverage   41.49%   41.49%           
=======================================
  Files         440      440           
  Lines       59453    59453           
=======================================
  Hits        24671    24671           
  Misses      31564    31564           
  Partials     3218     3218

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 1f84970...1f84970. Read the comment docs.

@mrsdizzie
Copy link
Member

Does it make sense for this code to run all of the time? If somebody runs gitea web in a folder they don't own, it wouldn't make sense to move to a /tmp folder and create files/run from there would it? Or to create a dump file in a /tmp folder and then delete that /tmp folder after run

If not, could it just be run based on os.Args and only run for certain commands where using a temporary work dir makes sense, like gitea admin, and avoiding other issues like the one you mention?

I assume it isn't reasonable to have gitea configured to run from a place without proper permissions and have things like web and srv working, where as the initial bug report case is more reasonable to expect

@zeripath
Copy link
Contributor Author

@mrsdizzie You don't get that choice - this has to run at package init() time, and has to run before macaron.v1's package init runs.

That means when it runs you don't know what command you're going to run.

@zeripath
Copy link
Contributor Author

We could however, detect that we're running from the temporary directory for some commands and fail because we can't run from there with a different Before for those that need it.

@zeripath
Copy link
Contributor Author

zeripath commented Apr 11, 2019

An alternative option is to delete the working directory at the start of main. Then if anything tries to write or read from the working directory it will fail - albeit with the incorrect error: "no such file or directory" instead of "permission denied"

@mrsdizzie @lunny Would that be preferable?

@codecov-io
Copy link

Codecov Report

Merging #6559 into master will decrease coverage by 0.02%.
The diff coverage is 31.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6559      +/-   ##
==========================================
- Coverage   40.37%   40.34%   -0.03%     
==========================================
  Files         405      406       +1     
  Lines       54260    54289      +29     
==========================================
- Hits        21907    21905       -2     
- Misses      29337    29366      +29     
- Partials     3016     3018       +2
Impacted Files Coverage Δ
routers/init.go 63.29% <0%> (-5.21%) ⬇️
aainit/init.go 39.13% <39.13%> (ø)
models/unit.go 0% <0%> (-14.29%) ⬇️
modules/log/event.go 64.46% <0%> (-1.53%) ⬇️
models/repo_list.go 66.84% <0%> (-1.06%) ⬇️
routers/repo/view.go 41.08% <0%> (-1%) ⬇️

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 c02c6a1...95430ca. Read the comment docs.

@codecov-io
Copy link

Codecov Report

Merging #6559 into master will decrease coverage by 0.01%.
The diff coverage is 31.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6559      +/-   ##
==========================================
- Coverage   40.37%   40.35%   -0.02%     
==========================================
  Files         405      406       +1     
  Lines       54260    54289      +29     
==========================================
+ Hits        21907    21910       +3     
- Misses      29337    29361      +24     
- Partials     3016     3018       +2
Impacted Files Coverage Δ
routers/init.go 63.29% <0%> (-5.21%) ⬇️
aainit/init.go 39.13% <39.13%> (ø)
models/unit.go 0% <0%> (-14.29%) ⬇️
routers/repo/view.go 41.08% <0%> (-1%) ⬇️

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 c02c6a1...95430ca. Read the comment docs.

@zeripath zeripath closed this May 21, 2019
@zeripath zeripath force-pushed the fix-4634-change-the-working-directory-if-necessary branch from 95430ca to 1f84970 Compare May 21, 2019 18:54
@zeripath zeripath deleted the fix-4634-change-the-working-directory-if-necessary branch October 16, 2019 15:16
@zeripath zeripath restored the fix-4634-change-the-working-directory-if-necessary branch April 28, 2020 14:26
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@zeripath zeripath deleted the fix-4634-change-the-working-directory-if-necessary branch March 4, 2021 09:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot create a new Gitea user without Gitea system user having read/execute access to current directory
5 participants