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

refactor(query): deprecate explain without master key #7520

Closed
wants to merge 1 commit into from

Conversation

mstniy
Copy link
Contributor

@mstniy mstniy commented Aug 24, 2021

New Pull Request Checklist

Issue Description

Currently, any user is able to run a query with the explain parameter and obtain the raw result returned by MongoDB. This discloses too much information to the clients, nor is it of great utility to them.

Related issue: #7519

Approach

rest.js now checks whether the user is allowed to use the explain option.
The PR uses the depreactor to deprecate the current default, which is to allow even non-master users to use explain.

TODOs before merging

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

  Manages whether non-master users are allowed to use the "explain" parameter on queries
  Currently defaults to true, for backwards compatibility. However, this behaviour is deprecated, and at some point will default to false.
Added tests for it
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #7520 (00363b4) into master (2f557f8) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7520      +/-   ##
==========================================
- Coverage   93.92%   93.92%   -0.01%     
==========================================
  Files         181      181              
  Lines       13273    13277       +4     
==========================================
+ Hits        12467    12470       +3     
- Misses        806      807       +1     
Impacted Files Coverage Δ
src/Deprecator/Deprecations.js 100.00% <ø> (ø)
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/rest.js 97.82% <75.00%> (-1.04%) ⬇️
src/ParseServerRESTController.js 97.01% <0.00%> (-1.50%) ⬇️
src/RestWrite.js 94.10% <0.00%> (+0.15%) ⬆️

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 2f557f8...00363b4. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Aug 24, 2021

Thanks for opening this PR!

Apologies, I should have explained better; this should use the Runtime Deprecator without requiring the introduction of a new config option. It's actually a much smaller PR.

@mtrezza mtrezza changed the title Added a new server option, nonMasterExplain improvement(query): restrict explain to master key Aug 24, 2021
@mtrezza mtrezza changed the title improvement(query): restrict explain to master key security(query): restrict explain to master key Aug 24, 2021
@mtrezza mtrezza changed the title security(query): restrict explain to master key security(query): deprecate explain without master key Aug 24, 2021
@mtrezza
Copy link
Member

mtrezza commented Aug 24, 2021

You can just add a condition to where the query is executed and add a runtime deprecation. This is an example of how to use it, specifically the Deprecator.logRuntimeDeprecation

@mstniy
Copy link
Contributor Author

mstniy commented Aug 24, 2021

What are the advantages of having runtime deprecation vs having a config parameter?

@mtrezza
Copy link
Member

mtrezza commented Aug 24, 2021

The main advantages is that otherwise, every deployment gets a deprecation warning on start up, even if they don't ever use explain. The other advantage is it's less code intrusive, so we don't have to add a temporary new config parameter, just to later remove it again. A config parameter needs to be validated, tested, documented, etc.

There are cases where we may introduce a config parameter to deprecate something, for example if there is a complex change in code that we don't want to introduce all of a sudden, but phase-in to gather feedback and mature the change. In this case however, it's just a master key enforcement, which doesn't require phasing-in.

@mstniy
Copy link
Contributor Author

mstniy commented Aug 24, 2021

Closing this PR in favor of #7521

@mstniy mstniy closed this Aug 24, 2021
@mtrezza mtrezza changed the title security(query): deprecate explain without master key refactor(query): deprecate explain without master key Aug 25, 2021
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.

2 participants