cors
cors copied to clipboard
expressjs
isRegExp: instanceof + check for properties
In a project I am involved in, an Express application is under quite high load. What we discovered, is the app sporadically does not return CORS headers.
If it was misconfigured, I guess, that misbehaviour would be ever present. We use list of RegExps for allowed origins. The only suspicious thing I can potentially blame is instanceof RegExp check. This PR changes it to a slightly more complicated property-based check.
Background for why instanceof RegExp could be flaky:
- https://github.com/micromatch/anymatch/issues/48
- https://github.com/jonschlinkert/kind-of/blob/master/index.js#L88
Oh, if that would be easy, I would do it. As I told in the description, missed CORS headers happened sporadically under high load. This instanceof RegExp replacement is a guess. It must not hurt, but I have no proof that it solves the problem, as I can not reproduce the failing scenario.
Ah, thanks. After I posted that, a thought crossed my mind: this bug should be reported to Node.js or V8 as many things test for RegExp and if it randomly will fail, this would create unknown code paths thoughout your app. There are many implications if this is the case and really needs to get reported to the platform and resolved there. Trying to patch every module on npm is going to be a never-ending task. If at leaat the Node.js doc are updated to state do not use this check, we can follow their guidelines.
I mean, did you deply this change at all to your production and at least verify that it resolves your problem? We'd want to at the very least do that. But ultimately without a test for this code path, the change ia unlikely to be accepted as there is no way to prevent regression of behavior that someone may now be reliant on.
Okay, I see your point. I have prepared a local modified version of the package. When we hit the high enough load again, and experience the bug, we'll deploy the change. If it actually resolves the issue, IMO, it might be a good enough signal to convince you, that the patch here is important, even if as you noticed, it is an underlying issue lurking beneath the surface. For now, please, let the PR hang for a couple of weeks.
No problem, I would definitely ask that you at least also file an issue with the platform so they are aware to look into the issue. If not, please add code to your reploy that logs what the regular expression is that fails the instanceof check so I can try to get enough information to raise the issue with the platform.
This is an excellent change. Not sure why the library owner's wouldn't just approve.
It was the first comment: need tests added that fail without this change otherwise it is just regress. No tests have surfaced in all this time, I think the PR may be abandoned. I'm going to close this, but anyone, including OP can always open a new one with tests included.
I have tried and tried but never have been able to reproduce the reported issue and the OP didn't provide any instructions, either. It could very well have been a bug in the platform that isn't even a bug any longer.
Sorry I added a comment from the code view without looking at the comments here. @dougwilson the repro for me is very simple, and honestly not sure if this PR actually fixes it or not, which is why I asked for test.
// This does not work
const corsOptions: CorsOptions = {
origin: [/vercel\.app$/],
}
// This works
const corsOptions: CorsOptions = {
origin: [new RegExp('vercel.app$')],
}
Something in the recursive call is switching the type of the regex.
There might be more at play here like Node version because I can't repro locally, but had to redeploy many times to land on this. It's not flaky, it's consistent.
