Gi60s / openapi-enforcer

Apache License 2.0
94 stars 22 forks source link

request validator shouldn't expect request object hasOwnProperty #65

Closed hankconn closed 4 years ago

hankconn commented 4 years ago

This requires a workaround in our code (using NestJS), otherwise it throws an error:

    /*
     * Enforcer checks that the object.hasOwnProperty("keys"), which fails for the
     * original request object. So this is a workaround to get it to work.
     */
    const reqObj: object = {};
    for (const k in req) {
      reqObj[k] = req[k];
    }

    const result = validator.request(reqObj);
Gi60s commented 4 years ago

Thanks for the issue. I've made an adjustment that should fix this. Please let me know if you find anything else. I've published this to NPM as version 1.8.4.

hankconn commented 4 years ago

@Gi60s unfortunately this doesn't work. It's still only getting "own" properties -- "The Object.assign() method copies all enumerable own properties from one or more source objects to a target object"

Gi60s commented 4 years ago

I think I've misunderstood. You're not saying that req is an object with a null prototype

Are you saying that the req is an object who's prototype defines the required properties, like below?

const prototype = Object.create(null)
prototype.method = 'get'
prototype.path = '/foo'

const req = Object.create(prototype)
Gi60s commented 4 years ago

I've upgraded the way this is working. Please let me know if it is working for your case. If it is not then I'll need more details about what this generated req object looks like.

Published to npm as version 1.8.7.

Thank you.

hankconn commented 4 years ago

@Gi60s the fix does not work, because Object.assign only copies "own" properties. So the Object.assign code is redundant. In this case the request object extends the base request object, so properties like path, etc., are properties of the object but not "own" properties. The code I mentioned in the original bug report is the only way I'm aware of to ensure the object has all properties as "own" properties. The other way to support this would be to simply check whether the property is defined (e.g. req.path, rather than req.hasOwnProperty("path")).

Gi60s commented 4 years ago

I believe the last code fix on NPM version 1.8.7 and newer fixes this. It no longer uses the Object.assign and instead calls the internal toRequestObject function.

const requestKeys = ['body', 'headers', 'method', 'path', 'query'];

Operation.prototype.toRequestObject: function (req) {
  const result = {};
  requestKeys.forEach(key => {
    if (key in req) result[key] = req[key]
  });
  return result;
}

Have you tested 1.8.7 or newer and found it not to be working?

hankconn commented 4 years ago

Sorry I missed that! Yes the new fix works, thanks!

Gi60s commented 4 years ago

Whew. :) Glad to hear it. Thanks again for helping me to fix this.