Skip to content

fix: Setting Parse Server option masterKeyIps: [] doesn't disable all IP addresses #8339

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

Open
wants to merge 5 commits into
base: alpha
Choose a base branch
from

Conversation

stewones
Copy link
Contributor

@stewones stewones commented Nov 27, 2022

New Pull Request Checklist

Issue Description

it's not quite working as expected, default ips are being concatenated to the user option. this PR aims to fix it.

Approach

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: disable master key ips fix: Disable master key ips Nov 27, 2022
@parse-github-assistant
Copy link

parse-github-assistant bot commented Nov 27, 2022

Thanks for opening this pull request!

  • ❌ Please check all required checkboxes at the top, otherwise your pull request will be closed.

  • ⚠️ Remember that a security vulnerability must only be reported confidentially, see our Security Policy. If you are not sure whether the issue is a security vulnerability, the safest way is to treat it as such and submit it confidentially to us for evaluation.

@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

Base: 94.24% // Head: 94.25% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (f6d35fe) compared to base (b2761fb).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8339      +/-   ##
==========================================
+ Coverage   94.24%   94.25%   +0.01%     
==========================================
  Files         180      180              
  Lines       13977    13976       -1     
==========================================
+ Hits        13172    13173       +1     
+ Misses        805      803       -2     
Impacted Files Coverage Δ
src/ParseServer.js 91.09% <ø> (-0.05%) ⬇️
src/RestWrite.js 94.91% <0.00%> (+0.14%) ⬆️
src/Adapters/Files/GridFSBucketAdapter.js 94.16% <0.00%> (+0.72%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@dblythy dblythy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic here is:

masterKeyIps: []: disable masterKey from anywhere
masterKeyIps: ["0.0.0.0/0"]: allow masterKey from anywhere

Comment on lines 471 to 476
options.masterKeyIps =
options.masterKeyIps?.length > 0
? Array.from(
new Set(options.masterKeyIps.concat(defaults.masterKeyIps, options.masterKeyIps))
)
: [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options.masterKeyIps =
options.masterKeyIps?.length > 0
? Array.from(
new Set(options.masterKeyIps.concat(defaults.masterKeyIps, options.masterKeyIps))
)
: [];

I think these lines can be removed completely

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah looking closer, fully agreed 😄

I was actually confused initially and didn't want to make some shit, especially because I'm not being able to run tests locally due to this error. any ideas?

~/dev/parse-server fix/masterKeyIps
❯ npm test

> [email protected] pretest /Users/stewan/dev/parse-server
> cross-env MONGODB_VERSION=${MONGODB_VERSION:=5.3.2} MONGODB_TOPOLOGY=${MONGODB_TOPOLOGY:=standalone} mongodb-runner start

  ◝ Starting a MongoDB deployment to test against...Error: Could not find download URL for version 5.3.2 {
  version: '5.3.2',
  arch: [ 'arm64', 'aarch64' ],
  platform: 'darwin',
  target: [
    { value: 'osx', priority: 1 },
    { value: 'osx-ssl', priority: 10 },
    { value: 'darwin', priority: 1 },
    { value: 'macos', priority: 1 }
  ],
  enterprise: false,
  cryptd: false
}
    at resolve (/Users/stewan/dev/parse-server/node_modules/mongodb-download-url/lib/index.js:177:15)
    at async getDownloadURL (/Users/stewan/dev/parse-server/node_modules/mongodb-download-url/lib/index.js:242:12)
npm ERR! Test failed.  See above for more details.

stewones and others added 2 commits November 27, 2022 21:02
masterKeyIps: []: disable masterKey from anywhere
masterKeyIps: ["0.0.0.0/0"]: allow masterKey from anywhere

Co-authored-by: Daniel <[email protected]>
@mtrezza
Copy link
Member

mtrezza commented Nov 28, 2022

I'm closing this PR since the way to "disable" or allow all IPs is to set masterKeyIps: ['0.0.0.0/0']. That is also documented in the option:

  • Setting ['0.0.0.0/0'] means disabling the filter

The reason you probably haven't seen this in the option docs is that the docs are only created for stable releases, so once Parse Sever 6.0.0 is released, the docs will also be updated.

@mtrezza mtrezza closed this Nov 28, 2022
@mtrezza mtrezza reopened this Nov 28, 2022
@stewones stewones changed the title fix: Disable master key ips fix: masterKeyIps Nov 28, 2022
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: masterKeyIps fix: MasterKeyIps Nov 28, 2022
@mtrezza mtrezza changed the title fix: MasterKeyIps fix: Setting Parse Server option masterKeyIps to an empty array doesn't disable all IP addresses Nov 28, 2022
@mtrezza mtrezza changed the title fix: Setting Parse Server option masterKeyIps to an empty array doesn't disable all IP addresses fix: Setting Parse Server option masterKeyIps: [] doesn't disable all IP addresses Nov 28, 2022
@@ -467,10 +467,6 @@ function injectDefaults(options: ParseServerOptions) {
});
}
});

options.masterKeyIps = Array.from(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this really just be removed and the issue is fixed?

cc @dblythy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. I will test and report back

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.

3 participants