Migration causing upgrade to fail (Specified key was too long)

[KDederichs]

Just FYI:

The migration that's part of 0.6.0 is causing updates to fail with:

Exception: Database error when running migration 000000Date20220923134100 for app solid An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 3072 bytes

on MySql 10 databases with default config.

[JurgenG]

I can confirm this. Having the same issue on our installation.

System info:

$ mysql -V
mysql  Ver 15.1 Distrib 10.5.15-MariaDB, for debian-linux-gnu (x86_64) using  EditLine wrapper
$ php -v
PHP 8.1.11 (cli) (built: Sep 29 2022 22:17:15) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.11, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.11, Copyright (c), by Zend Technologies
    with Xdebug v3.1.5, Copyright (c) 2002-2022, by Derick Rethans

It looks like this has to do with the InnoDB limits: https://dev.mysql.com/doc/refman/5.7/en/innodb-limits.html https://mariadb.com/kb/en/innodb-system-variables/#innodb_large_prefix

[Potherca]

Hi @KDederichs! Thank you for reporting (and @JurgenG for confirming) this issue.

A quick search on StackOverflow gives similar reasons (InnoDB limits) as root cause. A lot of folks suggest using setting a string limit in the migration file using Schema::defaultStringLength(191);, but I am not yet fully convinced this is the best solution...

I'll try to reprocude this locally and see which fix is most effective.

[Potherca]

^{(Oops, wrong milestone)}

[JurgenG]

Is there a reason we need key lengths of more than 3072 bytes?

[Potherca]

The max length we need for the value is 2048 (for the URL / URI field[^1]) but depending on charset collation (for instance UTF8 or UTF8-MB4) that can mean 8192 bytes are actually needed in order to store 2048 characters.

I think the error is caused by the index on the uri filed: $table->addIndex(['jti', 'uri']); for which the length is longer than accepted for the index key[^2].

I'm still thinking how much of the URL we actually need for the index... :thinking:

[^1]: Although the RFC/Spec does not define a maximum length, for historical reasons (namely Internet Explorer and the XML Sitemap Protocol) 2048 characters has become the convention.

[^2]: But until I can trace where this is trigger by the DB Migration code I can't make an informed decision if the best solution is to set the collation in the PHP code, or to force the default string length, or something else entirely.

[KDederichs]

What's it used for? Is it maybe enough to just index the host part and not the query parameters?

[poef]

According to the spec, the JTI claim must be unique for each request, except if a different URI is called. This means that we need to efficiently lookup if the given JTI + URI combination has already been used. In that case we must assume that someone is trying a replay attack. I'll try to find the exact quote in the spec.

[poef]

https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop-11#section-11.1

"To prevent multiple uses of the same DPoP proof servers can store, in the context of the target URI, the jti value of each DPoP proof for the time window in which the respective DPoP proof JWT would be accepted and decline HTTP requests to the same URI for which the jti value has been seen before. In order to guard against memory exhaustion attacks a server that is tracking jti values should reject DPoP proof JWTs with unnecessarily large jti values or store only a hash thereof."

[Potherca]

As we need both the domain and the path, what would work is to hash the input and store that, rather than storing the actual URL. For instance using MD5 or SHA1, so we would only need 32 or 40 chars, respectively.

This is something I have been thinking about anyway, for other reasons (security/privacy but also to have a fixed length db field).

@poef Any opinions regarding that approach?

Basically this: https://github.com/pdsinterop/solid-nextcloud/pull/108/files

[poef]

@potherca Yes, that is an excellent solution!

[Potherca]

Then I'll do some local testing and prepare a v0.7.1 release.

[VestigeJ]

Just an additional note this is true on Snap installed Nextcloud trying to install 6.0 from the App store (Nextcloud Version 24.0.6)

[Potherca]

This issue should be resolved with release v0.7.1. Feedback welcome!

[KDederichs]

I'll give it a try when it evenutally hits the nextcloud app store