Accept context via header X-Parse-Cloud-Context #7437

cbaker6 · 2021-06-20T14:51:37Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to Use context header to accept context instead of modifying body #7438.

Issue Description

Related issue: Currently context is setup to accept an object in the header, but no header is available. This allows client SDK's to add context via a header instead of injecting _context in the body of JSON object, forcing the parse-server to have to delete it in middleware. Close #7438

Approach

Add a new header for content called X-Parse-Cloud-Context.

TODOs before merging

Add test cases
Add entry to changelog

codecov · 2021-06-20T15:02:11Z

Codecov Report

Merging #7437 (a3e6e66) into master (c3b71ba) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7437      +/-   ##
==========================================
+ Coverage   93.92%   93.95%   +0.02%     
==========================================
  Files         181      181              
  Lines       13252    13267      +15     
==========================================
+ Hits        12447    12465      +18     
+ Misses        805      802       -3

Impacted Files	Coverage Δ
src/middlewares.js	`97.36% <100.00%> (+0.18%)`	⬆️
src/RestWrite.js	`93.92% <0.00%> (-0.16%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`93.02% <0.00%> (+0.65%)`	⬆️
src/batch.js	`93.10% <0.00%> (+1.72%)`	⬆️

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 c3b71ba...a3e6e66. Read the comment docs.

mtrezza

One argument for transmitting context in the HTTP body (as it is currently done) instead of header is the size limitation of headers. RFC does not impose a size limitation, but practically the limit in most implementations is much lower for header than for body.~~

~~At this point I don't think this PR is beneficial. Let's discuss in #7438 (comment) but I wanted to make a note here so the PR doesn't get merged by mistake before the discussion in concluded.~~

As discussed in #7438 (comment), context transmission via header and body are both valid approaches. It can be considered a feature or at least expected behavior that header values can be overridden with body values as described in #7438 (comment).

mtrezza · 2021-06-20T17:49:35Z

Since we allow to override header values with body values, should we add a separate test case for that? I don't know whether other parameters have a test case for override, but it would at least document that behavior if we add a test case for context override. And it would probably fix the coverage fail.

cbaker6 · 2021-06-20T18:16:20Z

Since we allow to override header values with body values, should we add a separate test case for that? I don't know whether other parameters have a test case for override, but it would at least document that behavior if we add a test case for context override. And it would probably fix the coverage fail.

I can add this later today. I think the coverage fail is codecov has some probability of error in its line of code detection. The commit before this was +2/-2 for lines and passed.

cbaker6 · 2021-06-20T22:32:26Z

These get hit when you don't provide provide an X-Parse-Application-Id header and are using _ApplicationId in the body:

parse-server/src/middlewares.js

Lines 60 to 115 in 9ea355b

    
           if (!info.appId || !AppCache.get(info.appId)) { 
        
             // See if we can find the app id on the body. 
        
             if (req.body instanceof Buffer) { 
        
               // The only chance to find the app id is if this is a file 
        
               // upload that actually is a JSON body. So try to parse it. 
        
               // https://github.com/parse-community/parse-server/issues/6589 
        
               // It is also possible that the client is trying to upload a file but forgot 
        
               // to provide x-parse-app-id in header and parse a binary file will fail 
        
               try { 
        
                 req.body = JSON.parse(req.body); 
        
               } catch (e) { 
        
                 return invalidRequest(req, res); 
        
               } 
        
               fileViaJSON = true; 
        
             } 
        
             if (req.body) { 
        
               delete req.body._RevocableSession; 
        
             } 
        
             if ( 
        
               req.body && 
        
               req.body._ApplicationId && 
        
               AppCache.get(req.body._ApplicationId) && 
        
               (!info.masterKey || AppCache.get(req.body._ApplicationId).masterKey === info.masterKey) 
        
             ) { 
        
               info.appId = req.body._ApplicationId; 
        
               info.javascriptKey = req.body._JavaScriptKey || ''; 
        
               delete req.body._ApplicationId; 
        
               delete req.body._JavaScriptKey; 
        
               // TODO: test that the REST API formats generated by the other 
        
               // SDKs are handled ok 
        
               if (req.body._ClientVersion) { 
        
                 info.clientVersion = req.body._ClientVersion; 
        
                 delete req.body._ClientVersion; 
        
               } 
        
               if (req.body._InstallationId) { 
        
                 info.installationId = req.body._InstallationId; 
        
                 delete req.body._InstallationId; 
        
               } 
        
               if (req.body._SessionToken) { 
        
                 info.sessionToken = req.body._SessionToken; 
        
                 delete req.body._SessionToken; 
        
               } 
        
               if (req.body._MasterKey) { 
        
                 info.masterKey = req.body._MasterKey; 
        
                 delete req.body._MasterKey; 
        
               } 
        
               if (req.body._context && req.body._context instanceof Object) { 
        
                 info.context = req.body._context; 
        
                 delete req.body._context; 
        
               } 
        
               if (req.body._ContentType) { 
        
                 req.headers['content-type'] = req.body._ContentType; 
        
                 delete req.body._ContentType; 
        
               }

the request library expects objects to be JSON strings, so the SDKs using body modification must be sending objects directly instead of JSON strings (not the correct way), similar to the fix I made in #7242. Because of this, I can't create the test to override the header with the body without doing a similar fix which is outside the scope of this PR.

I assume those SDKs (only the JS SDK) using context as private body property already work as I don't see reported issues about them
This PR enables context to also be sent via a header and doesn't get in the way of previous implementations

Because of what I mentioned above, I believe this PR can be merged without the additional test.

cbaker6 · 2021-06-20T22:59:53Z

My comment about codecov can be seen by observing the actual codecov: https://app.codecov.io/gh/parse-community/parse-server/compare/7437/diff#diff-c3JjL21pZGRsZXdhcmVzLmpz

There's no delta...

mtrezza · 2021-06-21T05:15:56Z

the SDKs using body modification must be sending objects directly instead of JSON strings

Are you referring to a test using the REST API? Couldn't you write a test using the JS SDK, saving a Parse.Object and setting a context in the save options and override that by setting _context on the Parse.Object?

cbaker6 · 2021-06-21T12:29:18Z

Are you referring to a test using the REST API? Couldn't you write a test using the JS SDK, saving a Parse.Object and setting a context in the save options and override that by setting _context on the Parse.Object?

I'm not sure what you mean by this. Isn't this already done in this test and the tests after it?: https://github.com/cbaker6/parse-server/blob/a1be895cc55519586d2dab9f25a0b7616c3bb54c/spec/CloudCode.spec.js#L2839-L2847

Which ends up passing _context in the JS SDK:
https://github.com/parse-community/Parse-SDK-JS/blob/e6f15fdc09429a2c456ec6387d365914af8a8988/src/RESTController.js#L230-L234

I can add another test that checks if options overrides the _context property set on an object, but this wouldn't have anything to do with the header:

it('context options should override _context object property when saving a new object', async () => {
    Parse.Cloud.beforeSave('TestObject', req => {
      expect(req.context.a).toEqual('a');
      expect(req.context.hello).not.toBeDefined();
      expect(req._context).not.toBeDefined();
    });
    Parse.Cloud.afterSave('TestObject', req => {
      expect(req.context.a).toEqual('a');
      expect(req.context.hello).not.toBeDefined();
      expect(req._context).not.toBeDefined();
    });
    const obj = new TestObject();
    obj.set('_context', { hello: 'world' });
    await obj.save(null, { context: { a: 'a' } });
  });

mtrezza · 2021-06-21T14:12:44Z

Isn't this already done in this test and the tests after it?:

No, because it doesn't test overriding.

I can add another test that checks if options overrides the _context property set on an object

This is the test I was thinking about. It would test that the context in body overrides the context in header. Since this seems to be expected behavior, I think it would be good to test it.

but this wouldn't have anything to do with the header

Wouldn't obj.save(null, { context: { a: 'a' } }) set the context in the header and obj.set('_context', { hello: 'world' }); set the context in the body for override?

mtrezza · 2021-06-21T14:25:23Z

src/middlewares.js

@@ -35,7 +35,7 @@ export function handleParseHeaders(req, res, next) {
    dotNetKey: req.get('X-Parse-Windows-Key'),
    restAPIKey: req.get('X-Parse-REST-API-Key'),
    clientVersion: req.get('X-Parse-Client-Version'),
-    context: {},
+    context: req.get('X-Parse-Cloud-Context') == null ? {} : JSON.parse(req.get('X-Parse-Cloud-Context')),


Should we wrap JSON parsing into a try catch and throw an error if necessary? A malformed JSON string would throw an exception.

cbaker6 · 2021-06-21T14:52:55Z

Wouldn't obj.save(null, { context: { a: 'a' } }) set the context in the header and obj.set('_context', { hello: 'world' }); set the context in the body for override?

This wouldn't set context in the header and therefore wouldn't override it, this is because JS uses this to set _context during a save here: https://github.com/parse-community/Parse-SDK-JS/blob/e6f15fdc09429a2c456ec6387d365914af8a8988/src/RESTController.js#L230-L234. From what I can see, JS doesn't seem to use headers at all and instead modifies the body for appId, context, etc.

But I think I have a simple mod that will improve the code and test the overriding.

mtrezza · 2021-06-21T20:00:58Z

This wouldn't set context in the header and therefore wouldn't override it, but I think I have a simple mod that will improve the code and test the overriding.

I assume this would be a separate PR in the JS SDK. But wouldn't that be the behavior we aim for? The obj.save should set the header, instead of a hidden field in the parse object, which you said seems "hacky". And if someone indeed feels "hacky", they can use obj.set, for whatever reason.

…ing body with json string

cbaker6 · 2021-06-21T20:15:18Z

This wouldn't set context in the header and therefore wouldn't override it, but I think I have a simple mod that will improve the code and test the overriding.

I assume this would be a separate PR in the JS SDK. But wouldn't that be the behavior we aim for? The obj.save should set the header, instead of a hidden field in the parse object, which you said seems "hacky". And if someone indeed feels "hacky", they can use obj.set, for whatever reason.

I didn't intend to create another PR for the JS SDK and not sure if that behavior should be changed since I'm not one of the maintainers of JS nor do I use it directly. Instead, on the server-side, _context only supports objects, so I made it support JSON strings as well to test overriding along with allowing any other SDK in the future that needs to use _contect but can only send JSON strings.

I've added two tests which I xited, these are meant to catch when _context can't make an object. The reason why I xited the tests is because using the JS SDK, I don't know how to make a valid object that would generate incorrect JSON, so the error isn't throwing in those tests. If you know how to make this happen in JS, feel free to suggest.

mtrezza · 2021-06-21T20:38:45Z

src/middlewares.js

@@ -454,3 +469,8 @@ function invalidRequest(req, res) {
  res.status(403);
  res.end('{"error":"unauthorized"}');
 }
+
+function malformedContext(req, res) {
+  res.status(500);


This error is caused by a developer mistake, so it should be a 4xx response code.

I suggest 400 Bad Request:

The request could not be understood by the server due to malformed syntax. The client SHOULD NOT repeat the request without modifications.

Switched to 400. I'm assuming you only meant the status change?

spec/CloudCode.spec.js

src/middlewares.js

mtrezza · 2021-07-23T18:15:49Z

@cbaker6 Is this ready for review / merge? I've lost track of this. Looking over it, it looks good to merge to me, just maybe add a few words to the change log entry so readers understand what this change is?

CHANGELOG.md

cbaker6 · 2021-07-23T22:05:54Z

@cbaker6 Is this ready for review / merge? I've lost track of this. Looking over it, it looks good to merge to me, just maybe add a few words to the change log entry so readers understand what this change is?

Yes, this is ready to go

mtrezza

Looks good to me, thanks for this PR! Let's wait for a 2nd review approval...

davimacedo

It looks good to me

* master: (55 commits) Accept context via header X-Parse-Cloud-Context (parse-community#7437) [Snyk] Upgrade ws from 7.4.6 to 7.5.3 (parse-community#7457) fix: upgrade @apollographql/graphql-playground-html from 1.6.28 to 1.6.29 (parse-community#7473) fix: upgrade @apollographql/graphql-playground-html from 1.6.27 to 1.6.28 (parse-community#7411) fix: upgrade graphql from 15.5.0 to 15.5.1 (parse-community#7462) [Snyk] Security upgrade parse from 3.2.0 to 3.3.0 (parse-community#7464) fix: upgrade apollo-server-express from 2.25.1 to 2.25.2 (parse-community#7465) fix: upgrade graphql-tag from 2.12.4 to 2.12.5 (parse-community#7466) fix: upgrade graphql-relay from 0.7.0 to 0.8.0 (parse-community#7467) Add MongoDB 5.0 support + bump CI env (parse-community#7469) changed twitter API endpoint for oauth test (parse-community#7472) add runtime deprecation warning (parse-community#7451) bumped node (parse-community#7452) fix: upgrade apollo-server-express from 2.25.0 to 2.25.1 (parse-community#7449) fix: upgrade subscriptions-transport-ws from 0.9.19 to 0.10.0 (parse-community#7450) fix: upgrade mongodb from 3.6.8 to 3.6.9 (parse-community#7445) fix: upgrade mongodb from 3.6.7 to 3.6.8 (parse-community#7430) fix: upgrade apollo-server-express from 2.24.1 to 2.25.0 (parse-community#7435) fix: upgrade ldapjs from 2.2.4 to 2.3.0 (parse-community#7436) fix: upgrade graphql-relay from 0.6.0 to 0.7.0 (parse-community#7443) ...

parse-github-assistant · 2021-10-17T11:17:57Z

Thanks for opening this pull request!

❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Related issue: #123 in the PR description, so I can recognize it.

parseplatformorg · 2021-11-01T16:27:27Z

🎉 This change has been released in version 5.0.0-beta.1

parseplatformorg · 2022-03-14T12:59:38Z

🎉 This change has been released in version 5.0.0

cbaker6 added 2 commits June 20, 2021 10:41

failing testcase

6e75987

add header

0c23bfd

mtrezza requested changes Jun 20, 2021

View reviewed changes

switch to X-Parse-Cloud-Context header

9e24f48

cbaker6 changed the title ~~Accept context via header X-Parse-Context~~ Accept context via header X-Parse-Cloud-Context Jun 20, 2021

Merge remote-tracking branch 'upstream/master' into context

119aee4

add back blank line that lint removed

a1be895

mtrezza reviewed Jun 21, 2021

View reviewed changes

test replacing context header with body context. Add support for sett…

3158916

…ing body with json string

cbaker6 added 2 commits June 21, 2021 16:18

add back blank line

0fa1d3a

cover error when _context body is wrong

bb972b8

mtrezza requested changes Jun 21, 2021

View reviewed changes

Update middlewares.js

a551957

mtrezza requested changes Jun 21, 2021

View reviewed changes

src/middlewares.js Outdated Show resolved Hide resolved

cbaker6 added 2 commits June 21, 2021 17:36

revert accidental status change

8503ce6

make sure context always decodes to an object else throw error

3d4a4dd

cbaker6 mentioned this pull request Jun 21, 2021

Switched context header X-Parse-Context to X-Parse-Cloud-Context parse-community/Parse-Swift#170

Merged

7 tasks

cbaker6 added 2 commits June 21, 2021 20:13

improve context object check

33087a0

Merge branch 'master' into context

9aedefb

mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed and removed type:bug Impaired feature or lacking behavior that is likely assumed labels Jul 11, 2021

mtrezza reviewed Jul 23, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Merge branch 'master' into context

6808c05

mtrezza approved these changes Jul 23, 2021

View reviewed changes

mtrezza requested a review from davimacedo July 23, 2021 22:47

Merge branch 'master' into context

8da1e4c

mtrezza requested a review from dplewis July 24, 2021 11:51

davimacedo approved these changes Jul 26, 2021

View reviewed changes

Merge branch 'master' into context

a3e6e66

davimacedo merged commit c8e822b into parse-community:master Jul 26, 2021

parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021

cbaker6 deleted the context branch November 22, 2021 18:43

mtrezza mentioned this pull request Mar 12, 2022

build: release 5.0 #7851

Merged

parseplatformorg added the state:released Released as stable version label Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept context via header X-Parse-Cloud-Context #7437

Accept context via header X-Parse-Cloud-Context #7437

cbaker6 commented Jun 20, 2021 •

edited

Loading

codecov bot commented Jun 20, 2021 •

edited

Loading

mtrezza left a comment •

edited

Loading

mtrezza commented Jun 20, 2021 •

edited

Loading

cbaker6 commented Jun 20, 2021

cbaker6 commented Jun 20, 2021 •

edited

Loading

cbaker6 commented Jun 20, 2021

mtrezza commented Jun 21, 2021

cbaker6 commented Jun 21, 2021

mtrezza commented Jun 21, 2021 •

edited

Loading

mtrezza Jun 21, 2021 •

edited

Loading

cbaker6 Jun 21, 2021

cbaker6 commented Jun 21, 2021 •

edited

Loading

mtrezza commented Jun 21, 2021 •

edited

Loading

cbaker6 commented Jun 21, 2021 •

edited

Loading

mtrezza Jun 21, 2021

cbaker6 Jun 21, 2021

mtrezza commented Jul 23, 2021 •

edited

Loading

cbaker6 commented Jul 23, 2021

mtrezza left a comment

davimacedo left a comment

parse-github-assistant bot commented Oct 17, 2021 •

edited

Loading

parseplatformorg commented Nov 1, 2021

parseplatformorg commented Mar 14, 2022

Accept context via header X-Parse-Cloud-Context #7437

Accept context via header X-Parse-Cloud-Context #7437

Conversation

cbaker6 commented Jun 20, 2021 • edited Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

codecov bot commented Jun 20, 2021 • edited Loading

Codecov Report

mtrezza left a comment • edited Loading

Choose a reason for hiding this comment

mtrezza commented Jun 20, 2021 • edited Loading

cbaker6 commented Jun 20, 2021

cbaker6 commented Jun 20, 2021 • edited Loading

cbaker6 commented Jun 20, 2021

mtrezza commented Jun 21, 2021

cbaker6 commented Jun 21, 2021

mtrezza commented Jun 21, 2021 • edited Loading

mtrezza Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

cbaker6 Jun 21, 2021

Choose a reason for hiding this comment

cbaker6 commented Jun 21, 2021 • edited Loading

mtrezza commented Jun 21, 2021 • edited Loading

cbaker6 commented Jun 21, 2021 • edited Loading

mtrezza Jun 21, 2021

Choose a reason for hiding this comment

cbaker6 Jun 21, 2021

Choose a reason for hiding this comment

mtrezza commented Jul 23, 2021 • edited Loading

cbaker6 commented Jul 23, 2021

mtrezza left a comment

Choose a reason for hiding this comment

davimacedo left a comment

Choose a reason for hiding this comment

parse-github-assistant bot commented Oct 17, 2021 • edited Loading

Thanks for opening this pull request!

parseplatformorg commented Nov 1, 2021

parseplatformorg commented Mar 14, 2022

cbaker6 commented Jun 20, 2021 •

edited

Loading

codecov bot commented Jun 20, 2021 •

edited

Loading

mtrezza left a comment •

edited

Loading

mtrezza commented Jun 20, 2021 •

edited

Loading

cbaker6 commented Jun 20, 2021 •

edited

Loading

mtrezza commented Jun 21, 2021 •

edited

Loading

mtrezza Jun 21, 2021 •

edited

Loading

cbaker6 commented Jun 21, 2021 •

edited

Loading

mtrezza commented Jun 21, 2021 •

edited

Loading

cbaker6 commented Jun 21, 2021 •

edited

Loading

mtrezza commented Jul 23, 2021 •

edited

Loading

parse-github-assistant bot commented Oct 17, 2021 •

edited

Loading