Structured /health Response

[montymxb]

The current response from /health is a simple OK. This is pretty straight forward, and is handy for quickly checking if a server is up. However the response from /parse will also do the same, leading the health check to appear rather extraneous.

This PR proposes to return structured data instead of a simple string from /health, sending a response that can be parsed by any of the sdks with additional details.

{
    "status": "ok",
    "version": "2.6.5"
}

The endpoint remains simple, but through this we can not only tell if the server appears to be running but which version of a server is running. I've also observed that certain providers of parse-server (sashido in this case) have already made a similar modification to their /health response.

{
    "version":"X.Y.Z",
    "status":"ok",
    "database":"ok",
    "region":"2-us",
    "state":"finished",
    "build":"1",
    "deployment":"2"
}

Although what they are doing is not expected, I feel we could move to make this standard. Being able to assess a server by /health and knowing that you can expect a status and version as JSON is both handy and usable by all the sdks (if needed). With the server moving along, and no guarantee that a provider will be keeping up, this can provide an easier way to check for compatibility without needing the masterKey to check /serverInfo.

As stated above, I should note we already have /serverInfo which provides the version in addition to some mostly static configs. In depth details may be better suited there, but I believe adding in the version to health makes sense as well.

[codecov]

Codecov Report

Merging #4305 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4305      +/-   ##
==========================================
+ Coverage   92.51%   92.51%   +<.01%     
==========================================
  Files         118      118              
  Lines        8251     8256       +5     
==========================================
+ Hits         7633     7638       +5     
  Misses        618      618

Impacted Files	Coverage Δ
src/ParseServer.js	94.57% <100%> (+0.21%)	⬆️

---

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 46af1b6...3002b25. Read the comment docs.

[montymxb]

Bleh, finally CI completes....

I want to note that it looks like (beyond seeing a repeat of #4294) I'm seeing some additional arbitrary test failures as follows.

Parse.User testing should not retrieve hidden fields

  - Failed: There were open connections to the server left after the test finished

Parse.User testing should not allow updates to hidden fields

  - Failed: {"name":"StatusCodeError","statusCode":404,"message":"404 - {\"code\":101,\"error\":\"Object not found.\"}","error":{"code":101,"error":"Object not found."},"options":{"method":"GET","url":"http://localhost:8378/1/users/GueK4gdPqb","json":true,"headers":{"X-Parse-Application-Id":"test","X-Parse-REST-API-Key":"rest"},"simple":true,"resolveWithFullResponse":false,"transform2xxOnly":false},"response":{"statusCode":404,"body":{"code":101,"error":"Object not found."},"headers":{"x-powered-by":"Express","access-control-allow-origin":"*","access-control-allow-methods":"GET,PUT,POST,DELETE,OPTIONS","access-control-allow-headers":"X-Parse-Master-Key, X-Parse-REST-API-Key, X-Parse-Javascript-Key, X-Parse-Application-Id, X-Parse-Client-Version, X-Parse-Session-Token, X-Requested-With, X-Parse-Revocable-Session, Content-Type","content-type":"application/json; charset=utf-8","content-length":"40","etag":"W/\"28-+5YirrDDOrWkX+4BB7U4IKOPrA0\"","date":"Tue, 31 Oct 2017 18:53:40 GMT","connection":"close"},"request":{"uri":{"protocol":"http:","slashes":true,"auth":null,"host":"localhost:8378","port":"8378","hostname":"localhost","hash":null,"search":null,"query":null,"pathname":"/1/users/GueK4gdPqb","path":"/1/users/GueK4gdPqb","href":"http://localhost:8378/1/users/GueK4gdPqb"},"method":"GET","headers":{"X-Parse-Application-Id":"test","X-Parse-REST-API-Key":"rest","accept":"application/json"}}}}

Installations update android device token with duplicate device token

  - Expected 1 to equal 0.

I've been making a personal note of these. At some point I'll start going after them.

[nbering]

From a security perspective, doesn't this make foot-printing easier?

Returning information about the server version on a public unauthenticated request makes it really easy to develop bots that check for a version of Parse Server that is vulnerable to an attack, lowering the cost of effort for a random attacker to locate vulnerable servers.

If the Health Check is going to return structured data, I think that's a feature that should be possible to disable for security hardening. I'd prefer to see the health check do more - but still just return OK.

Specifically, it would be nice if it did a simple round-trip to the database that does nothing but confirm the database server is up. A more complex implementation could provide an API for plugins to register health check operations so that - for example - the s3 file adapter could check that it's configured s3 bucket exists and that it can write a file and read it back.

[flovilmart]

I Like your thinking @nbering

[nbering]

@flovilmart Thanks. People pay me to think about these things and I can never shut it off. 😊

[montymxb]

@nbering yes you are correct, it would make it easier for others to run automated scans/attacks. The thought for having the version here is in open consideration that feature detection based on version would be handy later on for the sdks. It could allow variable adjustment of internal functionality, such as whether we can POST vs. GET to login (was added in #4268). As it stands, the only other way to get the version is via the masterKey as mentioned above. Most of the client sdks have no capacity to check this, unless they have the masterKey, and if it's running on their own device that's a problem on it's own...

Allowing the health check to incorporate additional information is definitely idea here. Making it structured we would be free to add/remove information from /health from this point on. The server version is a non-essential component, although I should admit I'm a bit biased towards it myself. Considering the potential detriment of version we can drop that, we just need status to get started.

I really like the idea of being able to variably register additional information for the health check. I'm going to go back and see if I can't put something together to move in that direction. Additionally, considering that some of this information may just be too sensitive, perhaps a similar registering system could be implemented over at /serverInfo? Assuming this works out fine.

[flovilmart]

I’m ok to add all of that on serverInfo, we could have a health panel also on the dashboard. Also, it’s quite biased as only 1 server is hit by the dashboard.

[montymxb]

Alright, so going along with what was being discussed I figured this should introduce a basic Health checking system that can have additional data & callbacks registered into what is ultimately reported at /health. I decided to round this all up inside the new Health.js.

Now the implementation isn't really there yet for how the data for the Health instance should be stored (perhaps using AppCache internally?) but this first bit of commits is focused on putting together what is in the core.

In essence, you can work on it like a ParseObject (roughly). You can interact with it like so.

// set one thing
Health.set('report_this', 'running');

// set a bunch of things
Health.setAll({
    item1: 'check',
    item2: 1
});

// set a callback (can do this in 'setAll' as well)
Health.set('callback', function() {
    return 'functional';
});

// get a value from our current health report
Health.get('item1');

// get the result of the callback (not the callback itself)
Health.get('callback');

// get everything! This is what's used to generate the final report as well
Health.getAll();

// alias for above
Health.getReport();

// get the raw value, like the actual Function that is our callback
Health.getRaw('callback');

// clear a value
Health.clear('item1');

// clear all values
Health.clearAll();

// reset to what we started with
Health.reset();

I figure setting it up like this it would not be difficult to register and add additional reporting data directly, or via a callback. If we wanted to add something for health, like an API or exposing a config, we wouldn't have to worry about handling the actual incorporation of the data into the response. All we would have to focus on is providing the data to Health.set('new_data', ...) and we're good. That's the ideal scenario in my mind, just need to adjust this as needed so it can be usable in such an instance.

@flovilmart @nbering if you have a moment let me know what your thoughts are on this. I'm not married to anything in here yet, but I think it's a step in the right direction. May still change the naming of this as well, not sure if Health is too vague and if HealthReport would be more accurate for one.

[montymxb]

As an additional note if this seems good I would be willing to work towards applying the same thing for /serverInfo. That would give us the capacity to dynamically adjust reports for both, giving us the choice whether we want the reported information to be public or private (assuming the masterKey is a reasonable indicator of trust for private).

[flovilmart]

ServerInfo is solely used by the dashboard to determine which features it should enable, I don’t believe we should change it. If anything, it should be static, not dynamic.

[montymxb]

Ahh, I didn't realize that...scratch that last note then.

[flovilmart]

@montymxb, I believe it’s overkill, and the endpoint being public, overall, probably insecure. Also, how would one configure the Health exernally? The module is not available.

Also in a distributed environment, I’m not sure how I would call this to monitor the status of my servers, unless i’m At loadbalancer level and have access to all IP’s of each server.

[montymxb]

Alright, I'll just scrap this and roll back to the original structured approach. That was the original intent, minus exploring a bit more.

[montymxb]

@flovilmart rolled back the last commit, back to structured 'status' of 'ok' now 👍 . Doesn't really indicate it very well (doesn't like hard resets I presume), but the change has been made.

[flovilmart]

Alright let's go with it! As for health of the dashboard, this goes with particular instrumentation of the different components, perhaps through a health adapter, that would report some values in statsd / prometheus format etc... This would be interesting to explore to provide insights without the usual monkey patching.

[montymxb]

Yeah, I would like to see something like this at some point. Adding reporting into the dashboard would be quite handy.