Uploaded image for project: 'Undertow'
  1. Undertow
  2. UNDERTOW-2566

HttpRequestParser.handleQueryParameters can set an encoded query string

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Done
    • Icon: Major Major
    • 2.3.19.Final, 2.2.38.Final
    • None
    • None
    • None

      While parsing each query string, the method resets the urlDecodeRequired value to false. 

      However, this variable is used for two different goals: defining if the current query param being parsed needs decoding, and defining if the whole query string being parsed needs decoding. The variable can be set to true only if ALLOW_UNENCODED_CHARACTERS_IN_URL is set to true. Hence, this bug happens only with this specific config.

      If the query string contains multiple parameters, where some of which need encoding while others don't, this can malfunction. The method can end up not decoding a queryString that needs decoding if the last query param did not require decoding.

      Here follows the corresponding code snippet:

      while (buffer.hasRemaining()) {
          final char next = (char) (buffer.get() & 0xFF);
          if(!allowUnescapedCharactersInUrl && !ALLOWED_TARGET_CHARACTER[next]) {
              throw new BadRequestException(UndertowMessages.MESSAGES.invalidCharacterInRequestTarget(next));
          }
          if (next == ' ' || next == '\t') {
              String queryString = stringBuilder.toString();
              if(urlDecodeRequired && this.allowUnescapedCharactersInUrl) {
                  queryString = decode(queryString, urlDecodeRequired, state, slashDecodingFlag, false);
              }
              exchange.setQueryString(queryString);
              (...)
              return;
          } else if (next == '\r' || next == '\n') {
              throw UndertowMessages.MESSAGES.failedToParsePath();
          } else {
              if (decode && (next == '+' || next == '%' || next > 127)) { //+ is only a whitespace substitute in the query part of the URL
                  urlDecodeRequired = true;
              } else if (next == '=' && nextQueryParam == null) {
                (...)
              } else if (next == '&' && nextQueryParam == null) {
                (...)
                  exchange.addQueryParam(decode(stringBuilder.substring(queryParamPos), urlDecodeRequired, state, true, true), "");
                  }
                  urlDecodeRequired = false;
                  queryParamPos = stringBuilder.length() + 1;
              } else if (next == '&') {
                  (...)
                  exchange.addQueryParam(nextQueryParam, decode(stringBuilder.substring(queryParamPos), urlDecodeRequired, state, true, true));
                  urlDecodeRequired = false;
                  queryParamPos = stringBuilder.length() + 1;
                  nextQueryParam = null;
              }
              stringBuilder.append(next);
          } 

              flaviarnn Flavia Rainone
              flaviarnn Flavia Rainone
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

                Created:
                Updated:
                Resolved: