View Issue Details

IDProjectCategoryView StatusLast Update
0034595FPCPackagespublic2018-12-13 14:47
ReporterMichal Gawrycki Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product Version3.3.1 
Target Version3.2.0Fixed in Version3.3.1 
Summary0034595: fphttpclient - improved redirections
DescriptionCurrently, fphttpclient does not support redirection to a relative path. Cookies also should not be cleaned when redirecting within the same domain.
Patch attached.
TagsNo tags attached.
Fixed in Revision40395
FPCOldBugId
FPCTarget
Attached Files

Activities

Michal Gawrycki

2018-11-23 20:44

reporter  

fphttpclient-redir.patch (1,460 bytes)   
Index: packages/fcl-web/src/base/fphttpclient.pp
===================================================================
--- packages/fcl-web/src/base/fphttpclient.pp	(revision 39795)
+++ packages/fcl-web/src/base/fphttpclient.pp	(working copy)
@@ -1376,7 +1376,7 @@
   Stream: TStream; const AllowedResponseCodes: array of Integer);
 
 Var
-  M,L,NL : String;
+  M,L,NL,RNL : String;
   RC : Integer;
   RR : Boolean; // Repeat request ?
 
@@ -1399,17 +1399,22 @@
         if (RC>MaxRedirects) then
           Raise EHTTPClient.CreateFmt(SErrMaxRedirectsReached,[RC]);
         NL:=GetHeader(FResponseHeaders,'Location');
-        if Not Assigned(FOnRedirect) then
-          L:=NL
-        else
+        if Assigned(FOnRedirect) then
           FOnRedirect(Self,L,NL);
+        if (not IsAbsoluteURI(NL)) and ResolveRelativeURI(L,NL,RNL) then
+          NL:=RNL;
         if (RedirectForcesGET(FResponseStatusCode)) then
           M:='GET';
+        // Request has saved cookies in sentcookies.
+        if ParseURI(L).Host=ParseURI(NL).Host then
+          FreeAndNil(FSentCookies)
+        else
+          begin
+          FreeAndNil(FCookies);
+          FCookies:=FSentCookies;
+          FSentCookies:=Nil;
+          end;
         L:=NL;
-        // Request has saved cookies in sentcookies.
-        FreeAndNil(FCookies);
-        FCookies:=FSentCookies;
-        FSentCookies:=Nil;
         end;
       end;
     if (FResponseStatusCode=401) then
fphttpclient-redir.patch (1,460 bytes)   

Thaddy de Koning

2018-11-25 11:39

reporter   ~0112180

Last edited: 2018-11-25 11:51

View 3 revisions

"Cookies also should not be cleaned when redirecting within the same domain"
Yes they should. Always. Basic.
I am curious that you write a decent patch and introduce a vulnerability at the same time? Plz explain...
Introducing such code into public domain open source is a very (not!) inventive way to compromise any re-compiles.

You also made the point "and also" where I can only see it as the core of the patch.... Plz explain....
Such things should be properly evaluated on the mailing list.

Thaddy de Koning

2018-11-25 11:58

reporter   ~0112184

The core team should discuss this.

Michael Van Canneyt

2018-11-29 15:38

administrator   ~0112263

Checked and applied patch, thanks; this is a useful improvement !

Michal Gawrycki

2018-12-13 14:47

reporter   ~0112536

Thanks! Closed.

Issue History

Date Modified Username Field Change
2018-11-23 20:44 Michal Gawrycki New Issue
2018-11-23 20:44 Michal Gawrycki File Added: fphttpclient-redir.patch
2018-11-25 11:39 Thaddy de Koning Note Added: 0112180
2018-11-25 11:44 Thaddy de Koning Note Edited: 0112180 View Revisions
2018-11-25 11:51 Thaddy de Koning Note Edited: 0112180 View Revisions
2018-11-25 11:58 Thaddy de Koning Note Added: 0112184
2018-11-29 15:34 Michael Van Canneyt Assigned To => Michael Van Canneyt
2018-11-29 15:34 Michael Van Canneyt Status new => assigned
2018-11-29 15:38 Michael Van Canneyt Fixed in Revision => 40395
2018-11-29 15:38 Michael Van Canneyt Note Added: 0112263
2018-11-29 15:38 Michael Van Canneyt Status assigned => resolved
2018-11-29 15:38 Michael Van Canneyt Fixed in Version => 3.3.1
2018-11-29 15:38 Michael Van Canneyt Resolution open => fixed
2018-11-29 15:38 Michael Van Canneyt Target Version => 3.2.0
2018-12-13 14:47 Michal Gawrycki Note Added: 0112536
2018-12-13 14:47 Michal Gawrycki Status resolved => closed