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

chore: introduce a new document root #3432

Closed
wants to merge 2 commits into from
Closed

Conversation

dvikan
Copy link
Contributor

@dvikan dvikan commented Jun 11, 2023

Add a new document root at public.

This change opens up some possibilities and increases security.

Preserving the old way of deploying.

See #3341

@em92

index.php Outdated Show resolved Hide resolved
@dvikan
Copy link
Contributor Author

dvikan commented Jun 11, 2023

i added your suggestion @em92

as written in prev thread. we could theoretically repurpose the cache folder as a "data" folder since it's no longer in the document root accessible via web server.

examples:

  • cache/app.sqlite (main application database)
  • cache/log/application.log (application log file)
  • cache/secrets.json (api keys for thirdparty services)
  • cache/files (folder for temporary files)

@em92
Copy link
Contributor

em92 commented Jun 14, 2023

we could theoretically repurpose the cache folder as a "data" folder since

In general "cache" is the thing, that can be cleaned up in application and it will work fine. In our case it won't.

I suggest to leave "cache" directory as it is now, for secrets and main db use "data" directory, for logs use "log" directory.

@dvikan
Copy link
Contributor Author

dvikan commented Jun 14, 2023

Yes we could introduce new folder data. But it would leave it open via http server direct url access so we cannot put secrets in there.

Well, I guess in shared hosting scenarios both alternatives leave the folder open for direct access. Yeah I think a new folder is best actually. I want only one "data"(data or var) folder and I want var/app.sqlite and var/log and var/temporary_files etc.

@Mynacol
Copy link
Contributor

Mynacol commented Jun 14, 2023

I'm in favor of this change. Restricting access to only the needed files is always a good improvement.

Regarding backwards compatibility but also new folders/structure I had the idea to look for the cache folder if already there and use it as previously. But for new installs, we should be able to create better suited folder names, properly separating cache from data.
I'm explicitly against saving persistent data (e.g. sqlite db) in the existing "cache" folder just in the name of backwards compatibility. That would break with user expectations in a hard way.

One more idea: To avoid complicating the webserver config, we could move the static/ folder inside of the public/ folder. I don't see why this would be a major bad style given the options.

@somini
Copy link
Contributor

somini commented Jun 17, 2023

Preserving the old way of deploying.

Thanks for this.

@dvikan
Copy link
Contributor Author

dvikan commented Jul 4, 2023

there might be an nginx path traversal bug in this pr. in the alias directive

@dvikan
Copy link
Contributor Author

dvikan commented Aug 1, 2023

well as long as we want to preserve old-style deploying to shared hosting there is not much wiggle room here.

  • we can use unpredictable name for the sqlite db
  • we can use htaccess/nginx rules to prevent direct access to e.g. var/application.sqlite

@somini
Copy link
Contributor

somini commented Aug 1, 2023

Usually shared hosting includes some kind of "private" folder specifically for this purpose. So making the data folder location configurable should be enough.

@dvikan
Copy link
Contributor Author

dvikan commented Aug 22, 2023

there is not much immediate pressure for this pr except for some sensitive data being exposed in cache folder if an attacker know the file to access which could be e.g. 7971b570e8143e6aaf0d9e6065edf8f0.cache or simply cache.sqlite in the case of sqlite.

@dvikan
Copy link
Contributor Author

dvikan commented Sep 13, 2023

this can be revisited later when the need is stronger

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants