Uploaded image for project: 'SonarQube'
  1. SonarQube
  2. SONAR-12245

Bad 401 error status handling

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 7.8
    • Fix Version/s: 8.0
    • Component/s: Web
    • Labels:
      None
    • Edition:
      Community
    • Production Notes:
      None

      Description

      We have a potential bug for anonymous users. It breaks the app, throwing an exception:

      The root cause is that throwGlobalError() will receive an undefined value if the WS returns a 401 status, which throws the error.

      The reason is, that getJSON() calls checkStatus(), which, if the status is 401, redirects to the login page, and then rejects the promise without any parameters.:

      // src/main/js/helpers/request.ts:162
      
      export function checkStatus(response: Response): Promise<Response> {
        return new Promise((resolve, reject) => {
          if (checkApplicationVersion(response)) {
            if (response.status === 401) {
              import('../app/utils/handleRequiredAuthentication')
                .then(i => i.default())
                .then(reject, reject); // <-- Here
            } else if (response.status >= 200 && response.status < 300) {
              resolve(response);
            } else {
              reject({ response });
            }
          }
        });
      }
      

      Apparently, in the current state checkStatus() and throwGlobalError() can *never* coexist, and yet, *they are used together all the time!!*

      One way could be to do the following and stop reject when we anyway redirect to the authentication page (letting the promise hang):

      export function checkStatus(response: Response): Promise<Response> {
        return new Promise((resolve, reject) => {
          if (checkApplicationVersion(response)) {
            if (response.status === 401) {
              import('../app/utils/handleRequiredAuthentication')
                .then(i => i.default());
            } else if (response.status >= 200 && response.status < 300) {
              resolve(response);
            } else {
              reject({ response });
            }
          }
        });
      }
      

      It's really not clean, but it should at least solve the issue. Note that there aren't many WS that return a 401 (403, yes; 401 is rare). It's probable we never have a real use case for this. But a bad implementation in a PR simply exposed the problem

      At the same time we could also make the reject of checkStatus cleaner and only reject the response and not wrap it inside an object.

        Attachments

          Activity

            People

            Assignee:
            jeremy.davis Jeremy Davis
            Reporter:
            gregoire.aubert Gregoire Aubert
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Dates

              Due:
              Created:
              Updated:
              Resolved: