[SERVER-26431] Validate the collection name for commands that read documents from a collection Created: 03/Oct/16 Updated: 08/Jan/24 Resolved: 02/Aug/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Querying |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Jeffrey Yemin | Assignee: | Backlog - Query Team (Inactive) |
| Resolution: | Duplicate | Votes: | 0 |
| Labels: | query-44-grooming | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||
| Assigned Teams: |
Query
|
||||||||||||||||||||
| Participants: | |||||||||||||||||||||
| Description |
|
When the server receives a command sent to a database with an illegal name (e.g., one that contains a '.' character), it returns an OP_REPLY with zero documents. For example, a createIndexes command sent to the 'dbname-v1.1' database:
returns a reply with zero documents. Expected results: reply with a document indicating command failure. |
| Comments |
| Comment by David Storch [ 02/Aug/19 ] | |||||||||||||||
|
It appears that we have an older improvement request which suggests that read commands should validate the collection name. Therefore, I'm closing this ticket as a duplicate of SERVER-12817. | |||||||||||||||
| Comment by David Storch [ 26/Jul/19 ] | |||||||||||||||
|
The $ character is most certainly not legal in a collection name, which is enforced on code paths that create collections: Therefore, I believe this ticket tracks an improvement request to validate the collection name for OP_QUERY reads and read commands (find, aggregate, etc.). This change would cause the server to return an error rather than an empty result set for this example. Updating the title of the ticket and the issue type accordingly. | |||||||||||||||
| Comment by Matt Cotter [ 12/Oct/16 ] | |||||||||||||||
|
So digging into this more, the db for the namespace string is definitely separated by the first period, so the db name here is "dbname-v1" and the collection is "1.$cmd"
So the db name is totally valid. The collection would fail the validCollectionName check (because it has a '$'), but it turns out we don't validate the collection name in isValid check for namespaces:
sketchy. Maybe we should validate the collection there... o.o looks like the empty() check was the easy way of allowing "Special namespaces", but "1.$cmd" should fail. But that might not matter anyway – I did a quick read of instance.cpp's assembleResponse, and it seems not to call isValid() at all on the path that this code is executing. I think we should fix that (uassert isValid?), and then it would be more clear what's going wrong. On the other hand, if '$' was a valid collection name character though, this is totally "closed works as designed", because this is definitely a query and not a command. | |||||||||||||||
| Comment by Matt Cotter [ 12/Oct/16 ] | |||||||||||||||
|
david.storch here's the reply: | |||||||||||||||
| Comment by David Storch [ 12/Oct/16 ] | |||||||||||||||
|
matt.cotter, gotcha, that makes sense. My bad. It's definitely running as a query rather than as a command based on the log output. Assuming the shell is sending a proper OP_QUERY command but with an invalid database name, perhaps the absence of a valid database name is causing us to fall into the query path? Can you tell what the OP_REPLY received by the shell looks like? | |||||||||||||||
| Comment by Matt Cotter [ 12/Oct/16 ] | |||||||||||||||
|
My point with the original shell code is that, using legacy find, that and my second code example would be the exact same – i.e. I think what's happening is instead of a command being run, it's just a query on the collection "1.$cmd". | |||||||||||||||
| Comment by Matt Cotter [ 12/Oct/16 ] | |||||||||||||||
|
charlie.swanson, david.storch that code I put was just to illustrate, sorry for confusion.
| |||||||||||||||
| Comment by Jeffrey Yemin [ 12/Oct/16 ] | |||||||||||||||
|
I'm not sure how to reproduce this in the shell because it doesn't let you 'use' an invalid database name:
I can confirm that the Java driver is using OP_QUERY to send the command, not OP_COMMAND, which is what I believe the 3.4 shell would be using. | |||||||||||||||
| Comment by David Storch [ 12/Oct/16 ] | |||||||||||||||
|
charlie.swanson, as far as I understand this problem does not have to do with the legacy query path or the find command. Rather, it is a problem with our generic handling of the OP_QUERY command protocol. Note that the repro uses a createIndexes command, not a query. matt.cotter, your script above is using the find command, as opposed to issuing a createIndexes command. | |||||||||||||||
| Comment by Charlie Swanson [ 12/Oct/16 ] | |||||||||||||||
|
matt.cotter I only vaguely understand this stuff, but it might be that your shell is using the find command instead of the legacy OP_QUERY, which will execute under a different path. I think you can do something like ./mongo --shellReadMode=legacy --shellWriteMode=legacy to get a runCommand to turn into an OP_QUERY on the db.$cmd namespace. david.storch is that right? | |||||||||||||||
| Comment by Matt Cotter [ 12/Oct/16 ] | |||||||||||||||
|
I'm trying to reproduce this, and I can't see how this is possible. jeff.yemin do you have a script that reproduces this? Perhaps in the shell?
|