Adjusted Minimongo to prohibit dot based field name inserts, to fix issue #3786#8169
Adjusted Minimongo to prohibit dot based field name inserts, to fix issue #3786#8169benjamn merged 5 commits intometeor:develfrom
Conversation
…dots, to address issue meteor#3786.
|
Hmm - Travis failed but from the error log the failures don't look like anything caused by this PR? |
|
Okay, it is my fault - |
| // field name restrictions: | ||
| // https://docs.mongodb.com/manual/reference/limits/#Restrictions-on-Field-Names | ||
| if (doc) { | ||
| const invalidFieldMatches = JSON.stringify(doc).match(/"([^"]*\.[^"]*)":/); |
There was a problem hiding this comment.
I believe this RegEx might be susceptible to match inadvertently on the value side.
Consider the JSON:
{"value":"silly quoted JSON in here \"︵ (°.°)/ ︵\": inadvertently matches! eeps"}...which I believe would throw the error incorrectly.
As an alternative, thoughts on using the replacer function of JSON.stringify?
JSON.stringify(doc, (key) => {
if (key.indexOf(".") > -1) {
throw MinimongoError(`Field ${key} must not contain '.'`);
}
return value;
});We could fix the regex, but I'm just thinking that the built-in function might be faster anyway and more able to handle whatever is thrown at it.
There was a problem hiding this comment.
Awesome idea! I'll make that change - thanks!
|
Does this check for Nice effort 👍 |
|
I'm assuming dots are the most commonly encountered reason, but it's reasonable to say that this could/should be expanded to prevent the other disallowed configurations: null characters or starting with a dollar sign. I'm not sure if anything outside of those three things would be a problem though. |
|
Hi guys - yes, this PR was just addressing the original issue of |
|
Whoa - @abernix, check out the "All checks have passed" section of this PR. My last commit just completely by-passed CircleCI:
I know why - I originally started this PR with my own instance of CircleCI running checks, then disabled my CircleCI instance just before the last commit, to speed up the CI run using Meteor's CI instances. The act of disabling it then committing means I've just been able to by-pass the CircleCI tests completely, and we're all green! Looks like yet another security hole caused by Circle allowing developer CI instances to override project CI instances 😞 ... |
|
I have not checked code, but |
|
Hi @mitar - good point; I just ran some tests with |
|
Hm, maybe then also looking into $set would be beneficial. BTW, I opened many tickets about difference between client and server side in the past: https://github.com/meteor/meteor/issues?q=is%3Aissue+is%3Aopen+minimongo+author%3Amitar You could look into them. How I caught them: I made tests run on client and server. So Meteor tests on the client, and Minimongo tests against the server. I think this should be done and would catch many many differences already. See also closed issues which were not all addressed: https://github.com/meteor/meteor/issues?q=is%3Aissue+minimongo+author%3Amitar+is%3Aclosed |
|
Hi all - I've adjusted With regards to other differences between Minimongo/Mongo, I know - I've been watching several of your issues. I believe that's a can of worms for another day/PR though ... Thanks again all! |
|
You are amazing! Thanks so much! |
|
And for other issues, I think the best first step would be to start running tests on both client and server side for Mongo related stuff. |
|
This is a little late, but there was an unintended consequence of this fix. When doing an upsert where the query/selector has dotted keys, the upsert fails with the "MinimongoError: Key must not contain '.'" error.
The selector is a valid query and the update code will add the selector to the doc to ensure a second upsert actually finds the new document. But it just adds the We are still stuck on a fairly old meteor (1.4.2) which actually works with the above code because the minimongo version is 1.0.19. However, something in our full unit test suite is pulling in a slightly newer version (1.0.21) and the unit tests fail. Makes it a little tough to deliver code to master when the tests fail. When we upgrade to a newer version, this issue might get worse. |
|
@dreich-nucleus in the meantime you could use |
|
The unit tests are mocking the real collections as in memory collections - I did not think rawCollection was available on those. |
|
@dreich-nucleus I think you should open a new issue about this. |
|
I could do that, but what if it is fixed in 1.7? It would be my first issue here and I don't want to make a silly one. Advice is welcome. |
|
I think better to comment in already closed PRs is to open an issue. Report in which version you are finding this problem (this is part of good issue) and yes, maybe it will be closed if it is fixed in newer version. But it will definitely get more eyes if it is a new issue and not discussion inside something already closed. |
|
Sounds good. I figured the context would be helpful, but I will link this discussion to the new issue. Thanks |
6 participants
Hi all - this PR is intended to help address issue #3786. Right now Minimongo allows inserts with dot (".") containing field names, which is against Mongo's field name restrictions. The changes in this PR will cause a new exception to be thrown if dot containing field inserts are attempted by Minimongo, with the exception thrown matching the exception thrown by Mongo on the server side. Thanks!