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

Improving default File Security #6995

Closed
dblythy opened this issue Nov 8, 2020 · 11 comments
Closed

Improving default File Security #6995

dblythy opened this issue Nov 8, 2020 · 11 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@dblythy
Copy link
Member

dblythy commented Nov 8, 2020

Is your feature request related to a problem? Please describe.
This FR is in relation to the default security around files, specifically around deployments that never intend to save files.

As discussed in this PR, personally I think it feels bit clunky to recommend a fileTrigger for users that never intend to have any files uploaded to their servers. Also, considering anonymous users are disabled by default, I would expect that file uploads require a logged in user.

Describe the solution you'd like
I was thinking of possibly baking these into the server config:

-allowAnonymousFileUploads: would be a breaking change as would prevent file uploads for no logged in users (unless set to true)
-allowFileUploads: defaults to true. Can set to false for deployments that never want file uploads.

Describe alternatives you've considered
As said, you can achieve this functionality with a file trigger. I just think you should be able to build a secure server without requiring cloud code. If you're building a Parse Server and only reading the Object docs, you won't ever realise that your deployment can accept files, which could be problematic.

Additional context
Made this an issue instead of a PR for @mtrezza :)

@mtrezza
Copy link
Member

mtrezza commented Nov 8, 2020

Thanks for suggesting (and for opening an issue to discuss this 😉).

This seems to be an elegant solution to the file upload issue. The breaking change seems acceptable because it is a security improvement (the PR requires an warning note in the Changelog).

Note that the file upload issue describes that not even an anonymous user is required to upload a file, just the endpoint and appId. Since "Anonymous User" in Parse Server is a feature that describes a "provisional, internal user", this is different from the file upload endpoint being open to the "public". So we need a 3rd configuration option for that case.

Grouping the parameters could make for better overview in the configuration, what do you think about something like:

fileUpload: {
  enabled: true, // Is true if files can be uploaded with Parse Server. Default is true.
  enabledForAnonymousUser: false, // Is true if file upload is enabled for Anonymous Users. Default is false.
  enabledForPublic: false // If true if file upload is enabled for anyone with access to the Parse Server file upload endpoint, regardless of user authentication. Default is false.
}

Please feel free to refine the wording. Maybe you can look into how the Parse docs refer to "public" vs. "Anonymous User", so that we use the same terminology here.

@dblythy
Copy link
Member Author

dblythy commented Nov 8, 2020

That's a good point. I completely forgot about anonymous users, I was referring to "public" users, not "anonymous". I like your suggestion. Wording looks simple enough - I'll look into it.

I'll work on a PR, this should be a really simple addition so should have something shortly.

@mtrezza
Copy link
Member

mtrezza commented Nov 8, 2020

Now that I think about it again, isn't the fileUpload.enabled parameter implicitly a fileUpload.enabledForAuthenticatedUser parameter? And if so, should we change that logic, to allow for more granular control?

E.g. with the previous suggestion, it is not possible to allow file uploads only for anonymous users, but not for authenticated users or public. I wouldn't have a use case for that at hand, but it would give more granular control.

What is your opinion on that?

@dblythy
Copy link
Member Author

dblythy commented Nov 9, 2020

It's definitely an interesting use case. I see the fileUpload.enabled as a master kill switch. But I agree it should be as granular as possible

I've added enabledForAuthenticatedUser to the latest commit.

@mtrezza
Copy link
Member

mtrezza commented Nov 9, 2020

I don't think fileUpload.enabled is still needed when one can disable the 3 other options and achieve the same result.

@dblythy
Copy link
Member Author

dblythy commented Nov 9, 2020

Yeah, good point.

@dblythy
Copy link
Member Author

dblythy commented Jan 31, 2021

Closed via #7071

@dblythy dblythy closed this as completed Jan 31, 2021
@tehsunnliu
Copy link

tehsunnliu commented Jun 30, 2021

Hi, does this prevent useMasterKey to save files too? Because from CloudCode when I try to save the file with useMasterKey set to true, I get "File upload by public is disabled" error message. Even from the dashboard I get the same error.
Setting enableForPublic works fine.

@mtrezza
Copy link
Member

mtrezza commented Jun 30, 2021

@tehsunnliu If you need help or have a question please post in the community forum. If you discovered a bug, please open a new issue in this repo. This PR is closed.

@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
@hamzabinamin
Copy link

hamzabinamin commented May 25, 2022

Hi, does this prevent useMasterKey to save files too? Because from CloudCode when I try to save the file with useMasterKey set to true, I get "File upload by public is disabled" error message. Even from the dashboard I get the same error. Setting enableForPublic works fine.

@tehsunnliu Can you show me your configuration file? I have added 'enableForPublic', yet I still receive the 'File upload by public is disabled' message.

@c0sm1cdus7
Copy link

@hamzabinamin did you manage to fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

5 participants