IdHTTP corrections when proxy is used

[NevTon]

Hi, when using TIdHTTP with proxy there is exception generated when authorization was needed.
Exception EIdHTTPProtocolException is fired with some params, but I think that this procedure contains some bugs:
1. you can not have exception and not discard content from response at the same time
2. it fills this exception with empty information for response text and third parameter as well

So my code fix proposition is as follows:

Code:

function TIdHTTPProtocol.ProcessResponse(AIgnoreReplies: array of Int16): TIdHTTPWhatsNext;
var
  LResponseCode, LResponseDigit: Integer;

  procedure CheckException;
  var
    i: Integer;
    LTempStream: TMemoryStream;
    LOrigStream: TStream;
    LRaiseException: Boolean;
    LDiscardContent: Boolean;
    LTempString: String;
  begin
    LRaiseException := True;
    LDiscardContent := not (hoWantProtocolErrorContent in FHTTP.HTTPOptions);

    if hoNoProtocolErrorException in FHTTP.HTTPOptions then begin
      LRaiseException := False;
    end
    else if High(AIgnoreReplies) > -1 then begin
      for i := Low(AIgnoreReplies) to High(AIgnoreReplies) do begin
        if LResponseCode = AIgnoreReplies[i] then begin
          LRaiseException := False;
          LDiscardContent := not (hoWantProtocolErrorContent in FHTTP.HTTPOptions);
          Break;
        end;
      end;
    end;

    LTempString := '';
    LTempStream := Nil;
    if {LRaiseException or} LDiscardContent then begin
      LTempStream := TMemoryStream.Create;
      LOrigStream := Response.ContentStream;
      Response.ContentStream := LTempStream;
    end else begin
      LOrigStream := Nil;
    end;
    try
      try
        try
          FHTTP.ReadResult(Request, Response);
        except
          on E: Exception do begin
            FHTTP.Disconnect;
            if not (E is EIdConnClosedGracefully) then begin
              raise;
            end;
          end;
        end;
        if LRaiseException then begin
          if LTempStream <> Nil then begin
            LTempStream.Position := 0;
            LTempString := ReadStringAsCharset(LTempStream, Response.CharSet);
          end;
          //
          raise EIdHTTPProtocolException.CreateError(LResponseCode, Response.ResponseText, LTempString);
        end;
      finally
        if {LRaiseException or} LDiscardContent then begin
          Response.ContentStream := LOrigStream;
        end;
      end;
    finally
      if LDiscardContent then begin
        LTempStream.Free;
      end;
    end;
  end;
...

Thanks, NevTon.

[rlebeau]

Hi, when using TIdHTTP with proxy there is exception generated when authorization was needed.
Exception EIdHTTPProtocolException is fired with some params, but I think that this procedure contains some bugs:
1. you can not have exception and not discard content from response at the same time
2. it fills this exception with empty information for response text and third parameter as well

I think you might have a misunderstanding of how hoNoProtocolErrorException and hoWantProtocolErrorContent are meant to work together (see New TIdHTTP flags and OnChunkReceived event).

If hoNoProtocolErrorException IS NOT set, and an HTTP error response is received, EIdHTTPProtocolException is meant to be raised (unless AIgnoreReplies includes the HTTP response code) and ALWAYS contain the response's body data in its ErrorMessage property.  This way, the caller ALWAYS has a chance of finding out what actually went wrong if needed, as the body data usually contains more detailed information than what the HTTP status line alone provides.

The purpose of hoWantProtocolErrorContent is to allow the caller to disable EIdHTTPProtocolException but still receive the body data of an error response.  When hoWantProtocolErrorContent IS set, but ONLY WHEN hoNoProtocolErrorException is ALSO set, then the body data is saved to the caller's target TStream (think when a server always sends a JSON document containing success/error info).  The body data is NOT saved to the caller's target TStream unless the caller EXPLICITLY asks for it, to avoid corrupting any data that may already be in the TStream (think when the caller is downloading a file to disk via a TFileStream).

hoWantProtocolErrorContent was originally intended to take effect ONLY when hoNoProtocolErrorException IS set. THIS IS BY DESIGN, it is not a bug.

Here the behavior before and after your proposed changes (click on the image to enlarge):

   

Is that what you really want?  What would be the actual use-case for this change?

[NevTon]

Here the behavior before and after your proposed changes (click on the image to enlarge):



Is that what you really want?  What would be the actual use-case for this change?

Exactly, exception with content is buggy, there is no content inside exception object (it seems that it is disregarded or something like it), and since there is exception, the easy way to check for "why", is to check response object, for example:

1. I do: http.Post(uri, input, output);
2. input contains some XML to be sended,
3. output is a TStringStream getting post return value,
4. Post is sended through PROXY,
5. If proxy needs authentication, it responds with error 407 - authentication required and some HTML page describing the reason,
(this is special situation - because this is exception and valid response at the same time, it should be fully parsed)
6. I need to catch exception and to be able to check: http.Response.ContentType = "text/html",
7. in fact, I need all http.Response object filelds to be filled with what was the response from proxy as it was normal (good) response.

Remy, can You see what is the problem here?

My personal opinion is that: "by design" You and other authors of Indy components forced yourselfs and other programmers to "do bad things".
Let me explain what I think is "bad thing":

1. If I catch exception, there is two things that interest me: exception code (407) and reason (authentication required message) - it is the first line of every response raw header,
2. this and other details should be also contained in http.Response object fields - where is its place.

This is more programmer friendly practise and helps alot with writing better code, and not always thinking where is the problem while debugging.

Thank You for hard work, commitment and understanding.
Do not take "constructive criticism" to Yourself, its nothing personal.
Belive me, I know how hard it is to make something that complex as Indy - better.
But You constantly saying that Indy 11 will be rebuild and better, why not start to getting there already?

[rlebeau]

Exactly, exception with content is buggy, there is no content inside exception object

It should be storing content in the exception object by default.  What SPECIFIC configuration are you using where the content is missing?  I need a CONCRETE EXAMPLE.

(it seems that it is disregarded or something like it), and since there is exception, the easy way to check for "why", is to check response object, for example:

1. I do: http.Post(uri, input, output);
2. input contains some XML to be sended,
3. output is a TStringStream getting post return value,
4. Post is sended through PROXY,
5. If proxy needs authentication, it responds with error 407 - authentication required and some HTML page describing the reason,
(this is special situation - because this is exception and valid response at the same time, it should be fully parsed)
6. I need to catch exception and to be able to check: http.Response.ContentType = "text/html",
7. in fact, I need all http.Response object filelds to be filled with what was the response from proxy as it was normal (good) response.

The TIdHTTP.Response object is ALWAYS populated with data from the response headers before EIdHTTPProtocolException is raised.  The only thing that is handled separately is where the response body data, if anywhere.

TIdHTTP raises EIdHTTPProtocolException on a 407 reply from a proxy (subject to hoNoProtocolErrorException and AIgnoreReplies) only if either:

• no TIdAuthentication class type is selected based on the proxy's 'Proxy-Authenticate' response header, because the appropriate TIdAuthentication classes are not registered (you need to add the relevant units to your uses clause), and/or the user's TIdHTTP.OnSelectProxyAuthorization event handler (if assigned) returns nil.
  
• the user's TIdHTTP.OnProxyAuthorization event handler (if assigned) returns False.
  
• authorization fails too many times in a row (TIdHTTP.AuthProxyRetries exceeds TIdHTTP.MaxAuthRetries).
  

If EIdHTTPProtocolException is raised, the response body data SHOULD be stored in the exception object by default.  So, what are you doing SPECIFICALLY to cause it NOT to be stored in the exception object?

Remy, can You see what is the problem here?

No, I don't. You need to walk me through a SPECIFIC scenario, with SPECIFIC examples.

1. If I catch exception, there is two things that interest me: exception code (407) and reason (authentication required message) - it is the first line of every response raw header,

Those 2 values are ALWAYS stored in the EIdHTTPProtocolException object, in its ErrorCode and Message properties, respectively.

Those same 2 values are ALWAYS available in the TIdHTTP.Response object as well, in its ResponseCode and ResponseText properties, respectively.

2. this and other details should be also contained in http.Response object fields - where is its place.

ALL of the HTTP response headers are ALWAYS stored in the TIdHTTP.Response object, in its RawHeaders property, as well as individual sub-properties for individual headers that TIdHTTP parses for you.

There is nowhere available in the TIdHTTP.Response object for the HTTP response body data to be stored.  So, it MAY be stored in an EIdHTTPProtocolException object (in its ErrorMessage property), or it MAY be stored in the user's target TStream, or it MAY be discarded, depending on the HTTP error state and how the TIdHTTP.HTTPOptions flags are configured.  This is BY DESIGN, whether you agree with the design or not.

But You constantly saying that Indy 11 will be rebuild and better, why not start to getting there already?

Everything that I have previously mentioned that I wanted for Indy 11 has been pushed back to Indy 12.  Indy 11 (if it ever gets released at this point) has been decided to be just a maintenance release to clean up the existing codebase (drop support for old compilers, cleanup IFDEFs, tweak the packages to utilize {$LIBSUFFIX}, streamline installation steps, etc).  No new component features, no code redesigns, just cleanup (and MAYBE get it into Embarcadero's GetIt component manager, finally).  Work has already been done on this effort (see the "Restructure" branch in Indy's SVN and GitHub repositories), but it is not quite finished yet (the packages still need to be updated and tested).

[NevTon]

I've tested oryginal Indy code once more.
This line is not working:

Code:

raise EIdHTTPProtocolException.CreateError(LResponseCode, FHTTP.ResponseText, ReadStringAsCharset(LTempStream, FHTTP.Response.CharSet));

LResponseCode is 407 - it is OK,
FHTTP.ResponseText - it is EMPTY (should be "Authentication Required" message or similar),

ReadStringAsCharset(LTempStream, FHTTP.Response.CharSet) - resturns empty string ALWAYS,
FHTTP.Response.CharSet - returns EMPTY charset ALWAYS.


I think that bug reported by me earlier was this one, sorry for making noise.


If You correct the line, to this:

Code:

raise EIdHTTPProtocolException.CreateError(LResponseCode, Response.ResponseText, ReadStringAsCharset(LTempStream, Response.CharSet));

than Exception.Message is set, but still no content is available in EIdHTTPProtocolException(E).ErrorMessage.

It seems that I was confused and was getting flag LDiscardContent as responsible for emptying ErrorMessage field, my bad.
Seems that, to fill ErrorMessage field, it needs more digging to discover the reason, why it is not set at this time.

[rlebeau]

If You correct the line, to this:

Code:

raise EIdHTTPProtocolException.CreateError(LResponseCode, Response.ResponseText, ReadStringAsCharset(LTempStream, Response.CharSet));

than Exception.Message is set

Done and checked in.

but still no content is available in EIdHTTPProtocolException(E).ErrorMessage.

Did you check whether the LTempStream is empty or not? It makes sense for a 407 response to not carry any body data, since a web browser would not normally display such content to the user, since it would be busy prompting the user for credentials instead.

[NevTon]

but still no content is available in EIdHTTPProtocolException(E).ErrorMessage.

Did you check whether the LTempStream is empty or not?  It makes sense for a 407 response to not carry any body data, since a web browser would not normally display such content to the user, since it would be busy prompting the user for credentials instead.

Yes, and You'r right, my proxy is responding only with headers and no body data (tested with Microsoft Network Monitor 3.4).
I do not have a way to test, if this will be filled with some data, as I do not have administrative privilege to the proxy.

[rlebeau]

I do not have a way to test, if this will be filled with some data, as I do not have administrative privilege to the proxy.

You do not actually need a live proxy or a live webserver in order to test how TIdHTTP parses response data.  You can make use of Indy's TIdIOHandlerStream component to feed in your own test data directly into TIdHTTP without using a TCP socket at all.  For example:

Code:

var
 ServerData: TStringStream;
 //ClientData: TStreamStream;
 IO: TIdIOHandlerStream;
begin
 ServerData := TStringStream.Create(
   'HTTP/1.1 407 Proxy Authentication Required' + EOL +
   'Proxy-Authenticate: Basic realm="test"' + EOL +
   'Content-Length: 28' + EOL +
   EOL +
   'Proxy credentials are needed' +
   'HTTP/1.1 200 OK' + EOL +
   'Content-Type: text/plain' + EOL +
   'Content-Length: 8' + EOL +
   EOL +
   'Success!'
   , TEncoding.UTF8);
 try
   // optional: if you want to see what TIdHTTP "sends", you can
   // also assign a separate TStream to receive the sent data...
   {ClientData := TStreamStream.Create('', TEncoding.UTF8);
   try}

   IO := TIdIOHandlerStream.Create(IdHTTP, ServerData{, ClientData});
   IO.FreeStreams := False;
   IdHTTP1.IOHandler := IO;

   IdHTTP1.ProxyParams.ProxyServer := 'testproxy';
   IdHTTP1.ProxyParams.ProxyPort := 8080;
   ShowMessage(IdHTTP1.Get('http://dummy.com'));

   {finally
     //ShowMessage(ClientData.DataString);
     ClientData.Free;
   end;}
 finally
   ServerData.Free;
 end;
end;