Skip to content

Protected fields fix #5463

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

Merged
merged 6 commits into from
Mar 30, 2019
Merged

Conversation

acinader
Copy link
Contributor

We launch the parse server using the cli.

I discovered after releasing parse-server@3.2.1 last night that our userSensitiveFields were leaking.

I tracked it down to the fact that the cli merges in the defaults before instantiating ParseServer.

My fix is to always process and merge userSensitiveFields if they exist.

I'd like to get this in and publish 3.2.2.

Add start of some more tests for protectedFields
which i need to do to document the feature.
@acinader acinader requested a review from dplewis March 30, 2019 19:20
@acinader
Copy link
Contributor Author

@awgeorge review please?

Copy link
Contributor

@mrmarcsmith mrmarcsmith left a comment

Choose a reason for hiding this comment

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

Looks good!

@codecov
Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #5463 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5463      +/-   ##
==========================================
+ Coverage   93.91%   93.92%   +0.01%     
==========================================
  Files         123      123              
  Lines        9024     9025       +1     
==========================================
+ Hits         8475     8477       +2     
+ Misses        549      548       -1
Impacted Files Coverage Δ
src/ParseServer.js 96.29% <100%> (+0.02%) ⬆️
src/RestWrite.js 93.26% <0%> (+0.18%) ⬆️

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 11976b8...edf7fd6. Read the comment docs.

Copy link
Contributor

@awgeorge awgeorge left a comment

Choose a reason for hiding this comment

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

Some initial thoughts - will run it against my server now.

@acinader
Copy link
Contributor Author

ok.

  1. I removed the initialization code in question. It wasn't getting covered so I was trying to test it (and remember why I put it in in the first place).

  2. I also notice that some of the unit test in UserPII.spec.js were failing, but the failure was getting swallowed. Here is what I noticed:

✓ should not get PII via REST (0.29 sec)
error undefined UserPII.spec.js:183

when I dug into it, I discovered that it was a false fail because some of the tests were running before the beforeEach that sets up the user. I returned all promises to jasmine and made sure that we weren't calling done after an error occurs and they all appear to be working as expected.

@awgeorge I think we're good now to push this to clean up leaking userSensitiveFields (but not email) for users like me who use the cli to launch.

url: 'http://localhost:8378/1/classes/_User',
headers: {
'X-Parse-Application-Id': 'test',
'X-Parse-Javascript-Key': 'test',
},
})
.then(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

an error with the request gets swallowed here.

expect(fetchedUser.zip).toBe(ZIP);
expect(fetchedUser.email).toBe(undefined);
},
e => console.error('error', e.message)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cause this resolves the error and we go into the then.

@acinader
Copy link
Contributor Author

AH! forgot to run build locally. I see the failing test.

is set without a _User permission.
@acinader
Copy link
Contributor Author

third times a charm?

I think I fully reasoned through the various states that options.protectedFields will be.

when we take out sensitiveFields in the next minor release, we can get rid of all this logic....

@acinader acinader merged commit edf5b51 into parse-community:master Mar 30, 2019
@acinader acinader deleted the protected-fields-fix branch March 30, 2019 22:38
@awgeorge
Copy link
Contributor

Cheers for the patch - good to know about the CLI mode.

UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* fix minor spelling mistake

* Always process userSensitiveFields if they exist

* Cover change to protectedFields
Add start of some more tests for protectedFields
which i need to do to document the feature.

* re-arrange promise deck chairs to not
swallow errors.

* remove noop code

* protect agains the case where options.protectedFields
is set without a _User permission.
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