Impossible to use `tag` as GET parameter

[franga2000]

Bug Report

NOTE: This might be a Mithril 2 problem, but I haven't been able to trace the issue so I'd rather not send this to them without being sure where it's coming from.

Current Behavior Using tag as a GET parameter breaks the page.

Steps to Reproduce

1. Open browser console
2. Go to any Flarum page
3. Add a GET parameter named tag (like https://flarum.localhost/?tag=hello)
4. See error

Expected Behavior All GET parameters being available. I know tag is a reserved name in Mithril components, but why that would apply to GET parameters is really beyond me - m.route.param() returns a plain object, so the query parameters must be getting dragged through some component's attr at some other point in the stack.

Error log

Error: [e] You cannot use the "tag" attribute name with Mithril 2.
    setAttrs Component.ts:122
    oninit Component.ts:57
    oninit Page.js:11
    oninit IndexPage.js:23

flarum info

Flarum core 0.1.0-beta.14.1
PHP version: 7.4.13
Loaded extensions: Core, date, libxml, openssl, pcre, sqlite3, zlib, ctype, curl, dom, fileinfo, filter, ftp, hash, iconv, json, mbstring, SPL, PDO, pdo_sqlite, session, posix, readline, Reflection, standard, SimpleXML, Phar, tokenizer, xml, xmlreader, xmlwriter, mysqlnd, exif, gd, pdo_mysql, sodium, Zend OPcache
+---------------------+------------------+--------+
| Flarum Extensions   |                  |        |
+---------------------+------------------+--------+
| ID                  | Version          | Commit |
+---------------------+------------------+--------+
| flarum-lang-english | v0.1.0-beta.14.1 |        |
+---------------------+------------------+--------+
Base URL: https://flarum.localhost
Installation path: /www/flarum
Debug mode: ON

[clarkwinkelmann]

I'm not actually sure whether that's a Mithril or Flarum issue. But it's certainly code in Flarum that's blocking the ability to use those names right now.

In our RouteResolver, we pass down all vnode.attrs to the underlying component

https://github.com/flarum/core/blob/46c3124b0b0cc80b4d8f3ff73ba9cf706ccebd92/js/src/common/resolvers/DefaultResolver.ts#L29

This behavior is documented in Mithril, and does not come with any warning https://mithril.js.org/route.html#routeresolverrender

The code triggering the issue seen is in Component, where we added an explicit error for having properties named tag or children:

https://github.com/flarum/core/blob/46c3124b0b0cc80b4d8f3ff73ba9cf706ccebd92/js/src/common/Component.ts#L121-L123

The children test makes sense, because in Mithril <2 you could have a children property in attrs, and this code detects old code. However in Mithril >=2, the children is moved to the vnode object, so technically it can now be used as an attrs (I think), it just wouldn't do the same as in Mithril <2.

I have no idea why there is a check for tag in there however. I don't think it was possible to pass the vnode tag in attrs, either before Mithril 2 nor after. Unfortunately when digging in the history all I can find is the PR flarum/framework#2255 and there's no comment regarding the reason for that addition.

There might be more in the detailed commits on the older Mithril rewrite branch. Paging @datitisev who might know more.

Looking at the older Mithril documentation and older code from Flarum, I don't even think tag could ever have been passed as an attribute, that seems to simply have never existed in Mithril. The only example I have is this older code, and it's not modifying props.tag, it's directly using vnode.tag https://github.com/flarum/core/blob/v0.1.0-beta.13/js/src/common/components/LinkButton.js#L24

I have tested removing the 'tag' in attrs check locally and adding a ?tag= querystring and I don't see any Mithril error. Same with children.

If there was a Mithril reason for those explicit errors, we should probably check whether Mithril still disallows those values. If there's none, we should probably remove these errors to increase flexibility and avoid this very issue, which can also happen with user-entered parameters for whatever reason.

If we really do need to keep the errors, then we should probably filter out those property names at the RouteResolver level as to not cause a routing error when they are used.

We could probably also filter all of the values out except key at the Resolver level, since we're not actually using those parameters via attrs anyway (?) I think we always access them with m.route.params. So simply just removing like 29 shown above in the DefaultResolver code.

Similar but not the same https://github.com/MithrilJS/mithril.js/issues/1271

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!