Fix for unhandled undefined config #4334

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.

Sign up for GitHub

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

Jump to bottom

Merged

flovilmart merged 7 commits into parse-community:master from bryandel:master

Nov 11, 2017

Contributor

bryandel commented Nov 9, 2017

When an invalid application id is passed either for reset/change password or email verification, config.get returns undefined. This causes internal server.


          Fix for unhandled undefined config

a877c85

When an invalid application id is passed either for reset/change password or email verification, config.get returns undefined. This causes internal server.

codecov bot commented Nov 9, 2017 •

edited

Loading

Codecov Report

Merging #4334 into master will increase coverage by 0.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #4334      +/-   ##
==========================================
+ Coverage   92.52%   92.54%   +0.01%     
==========================================
  Files         118      118              
  Lines        8258     8273      +15     
==========================================
+ Hits         7641     7656      +15     
  Misses        617      617

Impacted Files	Coverage Δ
src/Routers/PublicAPIRouter.js	`91.58% <93.33%> (+0.28%)`	⬆️
src/RestWrite.js	`93.39% <0%> (ø)`	⬆️
src/Adapters/Auth/meetup.js	`89.47% <0%> (+5.26%)`	⬆️

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 932a474...9ce201c. Read the comment docs.

Contributor

montymxb commented Nov 10, 2017 •

edited

Loading

Nice catch. The standard 'Not found.' & '404' response seems ok, @flovilmart think we're good using the existing missingPublicServerURL for this as well?

montymxb self-assigned this

Contributor

flovilmart commented Nov 10, 2017

We should probably throw if Config.get(appId) don’t return anything no?

Contributor

montymxb commented Nov 10, 2017

Something like this?

throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'appId not found');

or maybe OBJECT_NOT_FOUND? Not too many codes we could match to this unless we wanted to use INTERNAL_SERVER_ERROR.

Contributor

flovilmart commented Nov 10, 2017

What do we usually throw when making a request with an invalid appId? Unauthorized I guess .)

Contributor

montymxb commented Nov 10, 2017 •

edited

Loading

No specific cases come to mind actually, just You must provide an appId! in ParseServer.js and ...appIds or appSecret is incorrect. in vkontakte.js. What if we made the message just

appId is incorrect

Simple and to the point. Both of those other refs just throw errors, the latter uses OBJECT_NOT_FOUND.

montymxb suggested changes

View reviewed changes

Contributor

montymxb left a comment

@bryandel if you can split up the checking on the config and throw instead that would be fantastic 👍 .

src/Routers/PublicAPIRouter.js Outdated

@@ @@ -15,7 +15,7 @@ export class PublicAPIRouter extends PromiseRouter { @@
                   const appId = req.params.appId;
                   const config = Config.get(appId);
-                  if (!config.publicServerURL) {
+                  if (!config || !config.publicServerURL) {

Contributor

montymxb Nov 10, 2017

Would you be willing to update this to handle !config separately? We would like to throw an exception rather than return a standard 404. Something like this should do

if (!config) {
    throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'appId does not exist in config.');
}

Contributor

flovilmart Nov 10, 2017 •

edited

Loading

following in middleware.js if appId can't be found, this should be

if (!config)  {
    const error = new Error();
    error.status = 403;
    error.message = "unauthorized";
    throw error;
}

The error will be handled by the PromiseRouter and transformed in to the proper response.

Contributor

montymxb Nov 11, 2017

Ahh, didn't see that initially. @bryandel take flovilmart's suggestion for the proper error to report.

src/Routers/PublicAPIRouter.js Outdated

@@ @@ -40,7 +40,7 @@ export class PublicAPIRouter extends PromiseRouter { @@
                   const appId = req.params.appId;
                   const config = Config.get(appId);
-                  if (!config.publicServerURL) {
+                  if (!config || !config.publicServerURL) {

Contributor

montymxb Nov 10, 2017

Same here

src/Routers/PublicAPIRouter.js Outdated

@@ @@ -66,7 +66,7 @@ export class PublicAPIRouter extends PromiseRouter { @@
                 changePassword(req) {
                   return new Promise((resolve, reject) => {
                     const config = Config.get(req.query.id);
-                    if (!config.publicServerURL) {
+                    if (!config || !config.publicServerURL) {

Contributor

montymxb Nov 10, 2017

and here

src/Routers/PublicAPIRouter.js Outdated

@@ @@ -89,7 +89,7 @@ export class PublicAPIRouter extends PromiseRouter { @@
                   const config = req.config;
-                  if (!config.publicServerURL) {
+                  if (!config || !config.publicServerURL) {

Contributor

montymxb Nov 10, 2017

same

src/Routers/PublicAPIRouter.js Outdated

@@ @@ -114,7 +114,7 @@ export class PublicAPIRouter extends PromiseRouter { @@
                   const config = req.config;
-                  if (!config.publicServerURL) {
+                  if (!config || !config.publicServerURL) {

Contributor

montymxb Nov 10, 2017

same

bryandel added 6 commits

November 11, 2017 13:27


          Throwing a 403 exception instead of returning a 404 for an invalid ap…

94eb920

…p id

Also, added a missing semicolon


          Fix indent issues

6db01b7


          Fix invalid colon to semicolon

e62b88a


          Fix space and indent issues

d245d0f


          Merge branch 'master' into master

b64beae


          Tests for the fix for unhandled undefined config

9ce201c

Contributor Author

bryandel commented Nov 11, 2017

Thank you for the suggestions. I have changed the implementation and also added tests for the code coverage. But it seems in the last build a different test has failed

update android device token with duplicate device token
- Expected 1 to equal 0.

flovilmart approved these changes

View reviewed changes

flovilmart merged commit 4e207d3 into parse-community:master

Contributor

montymxb commented Nov 11, 2017

@bryandel thanks again for the PR!

UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request


          Fix for unhandled undefined config in reset password pages (parse-com…

6d3d66f

…munity#4334)

* Fix for unhandled undefined config

When an invalid application id is passed either for reset/change password or email verification, config.get returns undefined. This causes internal server.

* Throwing a 403 exception instead of returning a 404 for an invalid app id

Also, added a missing semicolon

* Fix indent issues

* Fix invalid colon to semicolon

* Fix space and indent issues

* Tests for the fix for unhandled undefined config

dplewis mentioned this pull request

refactor: upgrade @graphql-tools/links from 8.2.6 to 8.2.7 #7941

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet