[Bug] For a query param initialized as an array, visiting a URL with an unexpected value results in a crash

[lolmaus]

🐞 Describe the Bug

When a property configured to back a query param is initialized as an array, Ember crashes when the query param exists and is not a JSON representation of an array.

This should not be the case. As the URL field is accessible for the user to type anything into it, no input should cause the app to crash gracelessly.

To make it worse, it seems to be impossible to work around it with public API. I had to resort to private API like this:

  // This method is private API
  deserializeQueryParam(value, urlKey, defaultValueType) {
    try {
      return super.deserializeQueryParam(value, urlKey, defaultValueType);
    } catch (error) {
      if (error instanceof SyntaxError && urlKey === 'items' && value.slice(0, 2) !== '["') {
        // Silence cases where `items` are:
        // * an empty string
        // * a single quoteless id
        let newValue = value === '' ? '[]' : `["${value}"]`;
        return super.deserializeQueryParam(newValue, urlKey, defaultValueType);
      } else {
        throw error;
      }
    }
  }

On top of that, I found it impossible to update the URL with the expected query param! This is due to a combination of reasons:

• The crash happens before the beforeModel hook, so I still need to override the private deserializeQueryParam.
• I cannot do a redirect in deserializeQueryParam because it does not have access to transition.to.queryParams.
• In beforeModel, I can do a replaceWith() and I do have access to transition.to.queryParams, but those are raw strings, whereas replaceWith expects them to be values of properties on controllers.
• I cannot convert string query params to controller values, because deserializeQueryParam expects a defaultValueType argument which can only be accessed through _queryParamsFor which is also private! 😡

🔬 Minimal Reproduction

1. Define a query param like this:

   export default class IndexController extends Controller {
     queryParams = ['items'];
     items = [];
   }

2. Visit any of these URLs:

   • http://localhost:4200/?items
   • http://localhost:4200/?items=
   • http://localhost:4200/?items=foo
   • http://localhost:4200/?items=42

🚀 Demo

I have discovered that the exact error is different depending on the case and assembled a demo that covers all the differences:

Demo: https://main--splendid-twilight-1e584c.netlify.app/

Source : https://github.com/lolmaus/ember-app-query-param-array-error

🌍 Environment

• Ember: 4.11.0

[lolmaus]

I tried fixing the query param in the URL but it requires a horrible amount of private APIs:

import { STATE_SYMBOL } from 'router_js';

export default class TransactionsIndexRoute extends Route {
  beforeModel(transition) {
    super.beforeModel(...arguments);

    let { highlight, items: itemsSerialized, showSuggestedAttachments } = transition.to.queryParams;

    // ToDo: remove private API usage when this issue is fixed:
    // https://github.com/emberjs/ember.js/issues/20421
    if (itemsSerialized !== null && !Array.isArray(itemsSerialized)) {
      // prettier-ignore
      let state = transition[STATE_SYMBOL];
      let qpCache = this._router._queryParamsFor(state.routeInfos);

      let queryParamsSerialized = transition.to.queryParams;
      let queryParamsDeserialized = {};

      // Source: https://github.com/emberjs/ember.js/blob/v4.11.0/packages/%40ember/routing/router.ts#L1848-L1865
      for (let key in queryParamsSerialized) {
        if (key === 'items') {
          // prettier-ignore
          queryParamsDeserialized.items =
            itemsSerialized === 'true' || itemsSerialized === 'false' || itemsSerialized === ''
              ? []
              : [itemsSerialized];

          continue;
        }

        if (!Object.prototype.hasOwnProperty.call(queryParamsSerialized, key)) {
          continue;
        }
        let value = queryParamsSerialized[key];
        let qp = qpCache.map[key];

        queryParamsDeserialized[key] = qp
          ? this.deserializeQueryParam(value, qp.urlKey, qp.type)
          : null;
      }
      console.log('queryParamsDeserialized', queryParamsDeserialized);

This does not work yet but I gave up because it's clear at this point that this PR will not be accepted. 🙈

[lolmaus]

deserializeQueryParam runs before beforeModel, yet deserialized query param values are inaccessible inside beforeModel. 😭

[lolmaus]

The error has started to appear in Sentry only since certain moment, but the app URLs where it happens seem to be ancient, bookmarked long ago.

I've dug through our commit history and noticed that Ember has been upgraded ember-source from 4.5.1 to 4.11.0 just days before the error started to be logged.

Not sure if it's related, I have found no relevant release notes after 4.5.1.

[lolmaus]

@wakooka found out why the problem started in our Sentry at a certain date: we have removed custom serialization/deserialization from query params. 🙇‍♂️

Which explains the source of faulty URLs, and also indicates this is not a regression in Ember. Ember query params handling has been faulty before that. 😓

[sebastianhelbig]

Simple fix (for Ember 5.3), in router.js:

  _deserializeQueryParam(value, defaultType) {
    if (value === null || value === undefined) {
      return value;
    } else if (defaultType === 'boolean') {
      return value === 'true';
    } else if (defaultType === 'number') {
      return Number(value).valueOf();
    } else if (defaultType === 'array') {
      try {
        return A(JSON.parse(value));
      } catch (e) {
        return A([]);
      }
    }
    return value;
  }