Deleting more then 14 items do not work

[bunyevacz]

Deleting 1,2,3...14 items works. But 15,16, 17, 18, 19, 20 items do not.

Deleting 15 items (FAILS):

client -> server

_csrf | TwgHXamd-w2yzxQUkNTe0ztCMoTGiEcCIBTw
mailbox | 2b184657c4acb0049beafec6
message | 4436,4434,4433,4432,4431,4430,4429,4428,4427,4426,4425,4424,4423,4422,4421

Response (server -> client):

"/users/2b184657c4acb0049beafec5/mailboxes/2b184657c4acb0049beafdc6/messages/4436%2C4434%2C4433%2C4432%2C4431%2C4430%2C4429%2C4428%2C4427%2C4426%2C4425%2C4424%2C4423%2C4422%2C4421 does not exist"

Deleting 14 items (SUCCESS):

client -> server

_csrf | TwgHXamd-w2yzxQUkNTe0ztCMoTGiEcCIBTw
-- | --
mailbox | 2b184657c4acb0049beafec6
message | 4470,4468,4467,4466,4465,4461,4445,4444,4443,4442,4440,4439,4437,4435

server->client

success | true
mailbox | 5b184657c4acb0049beafdc8
id | […]
action | move

I'll try to debug this issue more this evening.

ps: deleting a single message now works. Thank you. (other report)

[bunyevacz]

Ok, this is the root cause: lib/api-client.js->_render method somehow manage to replace , with %2C.

This is the offending code:

 let url = path.replace(/\{([^}]+)\}/g, (match, key) => {
        if (key in args) {
            found.add(key);
            return encodeURIComponent(args[key]);
        }
        return match;
});

After that, url becomes:

url: /users/2b184657c4acb0049beaeec5/mailboxes/2b184657c4acb0049beaeec6/messages/4475%2C4474%2C4473%2C4472%2C4471%2C4460%2C4459%2C4446%2C4441%2C4438%2C4436%2C4434%2C4433%2C4432%2C4431%2C4430%2C4429%2C4428%2C4427%2C4426

I need to strenghten my regexp-fu.

I'll be back:)

[andris9]

Oh, that explains a lot - if I test with small UIDs then I never get this as the resulting list would be too short

[bunyevacz]

Hmm, it is not the regexp neither encodeURIComponent fault. The url is in both case , -> %2C (which is urlencoded comma).

But it fails somewhere there. Maybe it is restify-clients, which can not handle too long URIs... I stuck a bit, so if you can figure out, I'll be more then happy to look at the commit message, where was the error:) Otherwise I will try to trace each function call in the coming days...

Update: So when deleting an email, the website (wildduck-webmail) acts like a gateway, the browser calls are translated to actual wildduck calls. Browser->wildduck-webmail:

POST mywebsite.com/api/delete

_csrf | l7jxHGFL-FYirk8lpCQxLfzgHmu2sIQG_2Ik
-- | --
mailbox | 2b184657c4acb0049beafec6
message | 4683,4682

The website makes several REST api calls (as client) to the wildduck server: wildduck-webmail -> wildduck

1.
lib/api-client.js->users.get()
GET 127.0.0.1:8080
/users/2b184657c4acb0049beafec5'

2. 
lib/api-client.js->mailboxes.get()
GET 127.0.0.1:8080
/users/2b184657c4acb0049beafec5/mailboxes/resolve?path=INBOX

3. 
lib/api-client.js->mailboxes.list()
GET 127.0.0.1:8080
/users/2b184657c4acb0049beafec5/mailboxes?counters=true

4. 
lib/api-client.js->messages.update() 4673,4672
PUT 127.0.0.1:8080
/users/2b184657c4acb0049beaedc5/mailboxes/2b184657c4acb0049beaedc6/messages/4673%2C4672

5. 
lib/api-client.js->users.get()
GET 127.0.0.1:8080
/users/2b184657c4acb0049beafec5

6. 
lib/api-client.js->mailboxes.get()
GET 127.0.0.1:8080
/users/2b184657c4acb0049beafec5/mailboxes/resolve?path=INBOX

7. 
 lib/api-client.js->messages.list()
GET 127.0.0.1:8080
/users/2b184657c4acb0049beafec5/mailboxes/2b184657c4acb0049beafec6/messages?next=&previous=&page=

Then after a successful delete, the webpage immediately request /api/list (step 7?)

An unsuccessful delete:

1. users.get()
2. mailboxes.get()
3. mailboxes.list()
4. message.update()
/users/2b184657c4acb0049beafec5/mailboxes/2b184657c4acb0049beafde6/messages/4685%2C4684%2C4671%2C4670%2C4669%2C4665%2C4618%2C4608%2C4556%2C4544%2C4543%2C4539%2C4490%2C4483%2C4477%2C4438%2C4432%2C4426%2C4368%2C4361

and the response for this call is ResourceNotFound (404). Which is handed back to the webbrowser if I'm right.

The actual call hits wildduck, so the bug should be somewhere inside wildduck.

Another day passed, another update: In case of successful delete, this codepath is called:

 wduck/lib/api/messages.js->.put(../:message), req.params.message: 5031,5030

When error happens (20 delete at once), the above codepath is not called at all.

Breakfast thought:)

I think there is a design flaw right here. Reading the restify library documentation, in put, post, delete request, you do have an object argument alongside of path argument:

put(path, object, callback)

You are encoding everything inside the path. Every single call could made with a simple GET request. All the req.body parameters gets encoded (via wildduck webmail) into the request url (path).

The problem with this approach ( /users/:user/mailboxes/:mailbox/messages/:message), that you can have arbitrary long request url, ie. without limit. The maximum url length is about 2000 characters, you will hit this limit in some edge cases.

In our current bug, we hit about 200 char request limit:

https://127.0.0.1:8080/api/users/2b184657c4acb0049beafec5/mailboxes/2b184657c4acb0049beafdc6/messages/4436%2C4434%2C4433%2C4432%2C4431%2C4430%2C4429%2C4428%2C4427%2C4426%2C4425%2C4424%2C4423%2C4422%2C4421

(205 chars)

Still far away from the 2000 char limits (which is implementation limit, so may not apply in our case (node client <-> node server (restify on both side)).

I'll continue my journey this evening:)

[bunyevacz]

@andris9 Found the bug. It is in find-my-way library, which gives 100 chars as default param length.

The code fails at

 // parametric route
    if (kind === NODE_TYPES.PARAM) {
      currentNode = node
      i = path.indexOf('/')
      if (i === -1) i = pathLen
      if (i > maxParamLength) return null //<--- HERE!
      decoded = fastDecode(path.slice(0, i))
      if (decoded === null) return null
      params[pindex++] = decoded
      path = path.slice(i)
      continue
}

And the maxParamLength is 100 by default:

  this.maxParamLength = opts.maxParamLength || 100

Here is a link to the current version (although we are using 1.13.0).

The fix is rather straightforward: In wildduck/api.js we should add a maxParamLength option, like this:

const serverOptions = {
    maxParamLength: 196,
    name: 'WildDuck API',
    strictRouting: true,
    formatters: {
        'application/json; q=0.4': (req, res, body) => {
            let data = body ? JSON.stringify(body, false, 2) + '\n' : 'null';
            res.setHeader('Content-Length', Buffer.byteLength(data));
            return data;
        }
    }
};

Thus overwriting the default value of 100.

While the fix may seem obvious, it is not a real fix in my opinion. It is a major design flaw in the api. You are converting a variable length option (HTTP PUT body) into url (HTTP PUT path). I think the real fix would be to have the message parameter inside the body, as was between the browser<->wildduck-webmail.

The find-my-way algorithm is a recursive tree finding algoritm. It will be exponentially slower as the parameter gets longer and longer. If you kept messages parameter in the body, it would be a simple object key.

What is your opinion?

[andris9]

Yes, it seems like a bad design. I updated both wildduck and wildduck-webmail to send message list in PUT body, instead of an URL param, so the issue should be fixed in latest version.

[bunyevacz]

@andris9: I think it would also make sense to explicitly specify the max request param (as I wrote):

const serverOptions = {
    maxParamLength: 196,

Not because the find-my-way's default 100 is too short, but because we should be aware of this settings, and adjust if needed. And also to not get changed upstream, without our knowledge.

Thoughts?

[andris9]

Yeah, good idea https://github.com/nodemailer/wildduck/commit/50eac429658bb24d9bc61db192138cb76221f720