Keep URIs encoded in the acquire system

We do not deal a lot with URIs which need encoding, but then we do it is
a pain that we store it decoded in the acquire system as it means we
have to decode and reencode URIs eventually which is potentially giving
us slightly different URIs.

We see that in our own testing framework while setting up redirects as
the config options are effectively double-encoded and decoded to pass
them around successfully as otherwise %2f and / in an URI are treated
the same.

This commit adds the infrastructure for methods to opt into getting URIs
send in encoded form (and returning them to us in encoded form, too) so
that we eventually do not have to touch the URIs which is how it should
be. This means though that we have to deal with methods who do not
support this yet (aka: all at the moment) for which we decode and encode
while communicating with them.

9 files changed, 140 insertions, 43 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 820dc7b59..34528f2ec 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -1380,7 +1380,7 @@ string pkgAcqMetaBase::Custom600Headers() const
 void pkgAcqMetaBase::QueueForSignatureVerify(pkgAcqTransactionItem * const I, std::string const &File, std::string const &Signature)
 {
    AuthPass = true;
-   I->Desc.URI = "gpgv:" + Signature;
+   I->Desc.URI = "gpgv:" + pkgAcquire::URIEncode(Signature);
    I->DestFile = File;
    QueueURI(I->Desc);
    I->SetActiveSubprocess("gpgv");
@@ -1415,7 +1415,7 @@ bool pkgAcqMetaBase::CheckDownloadDone(pkgAcqTransactionItem * const I, const st
    if (FileName != I->DestFile && RealFileExists(I->DestFile) == false)
    {
       I->Local = true;
-      I->Desc.URI = "copy:" + FileName;
+      I->Desc.URI = "copy:" + pkgAcquire::URIEncode(FileName);
       I->QueueURI(I->Desc);
       return false;
    }
@@ -2888,9 +2888,10 @@ bool pkgAcqIndexDiffs::QueueNextDiff()					/*{{{*/
    }
 
    // queue the right diff
-   Desc.URI = Target.URI + ".diff/" + available_patches[0].file + ".gz";
+   auto const BaseFileURI = Target.URI + ".diff/" + pkgAcquire::URIEncode(available_patches[0].file);
+   Desc.URI = BaseFileURI + ".gz";
    Desc.Description = Target.Description + " " + available_patches[0].file + string(".pdiff");
-   DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(Target.URI + ".diff/" + available_patches[0].file), Target);
+   DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(BaseFileURI), Target);
 
    if(Debug)
       std::clog << "pkgAcqIndexDiffs::QueueNextDiff(): " << Desc.URI << std::endl;
@@ -2923,7 +2924,7 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes,
 	    std::clog << "Sending to rred method: " << UnpatchedFile << std::endl;
 	 State = StateApplyDiff;
 	 Local = true;
-	 Desc.URI = "rred:" + UnpatchedFile;
+	 Desc.URI = "rred:" + pkgAcquire::URIEncode(UnpatchedFile);
 	 QueueURI(Desc);
 	 SetActiveSubprocess("rred");
 	 return;
@@ -2979,9 +2980,9 @@ pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire *const Owner,
 
    Desc.Owner = this;
    Desc.ShortDesc = Target.ShortDesc;
-   Desc.URI = Target.URI + ".diff/" + patch.file + ".gz";
+   Desc.URI = Target.URI + ".diff/" + pkgAcquire::URIEncode(patch.file) + ".gz";
    Desc.Description = Target.Description + " " + patch.file + ".pdiff";
-   DestFile = GetPartialFileNameFromURI(Target.URI + ".diff/" + patch.file + ".gz");
+   DestFile = GetPartialFileNameFromURI(Desc.URI);
 
    if(Debug)
       std::clog << "pkgAcqIndexMergeDiffs: " << Desc.URI << std::endl;
@@ -3068,7 +3069,7 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 	    std::clog << "Sending to rred method: " << UnpatchedFile << std::endl;
 	 State = StateApplyDiff;
 	 Local = true;
-	 Desc.URI = "rred:" + UnpatchedFile;
+	 Desc.URI = "rred:" + pkgAcquire::URIEncode(UnpatchedFile);
 	 QueueURI(Desc);
 	 SetActiveSubprocess("rred");
 	 return;
@@ -3270,7 +3271,7 @@ void pkgAcqIndex::StageDownloadDone(string const &Message)
       if (symlink(Filename.c_str(), DestFile.c_str()) != 0)
 	 _error->WarningE("pkgAcqIndex::StageDownloadDone", "Symlinking file %s to %s failed", Filename.c_str(), DestFile.c_str());
       Stage = STAGE_DECOMPRESS_AND_VERIFY;
-      Desc.URI = "store:" + DestFile;
+      Desc.URI = "store:" + pkgAcquire::URIEncode(DestFile);
       QueueURI(Desc);
       SetActiveSubprocess(::URI(Desc.URI).Access);
       return;
@@ -3299,9 +3300,9 @@ void pkgAcqIndex::StageDownloadDone(string const &Message)
    Stage = STAGE_DECOMPRESS_AND_VERIFY;
    DestFile = GetKeepCompressedFileName(GetPartialFileNameFromURI(Target.URI), Target);
    if (Filename != DestFile && flExtension(Filename) == flExtension(DestFile))
-      Desc.URI = "copy:" + Filename;
+      Desc.URI = "copy:" + pkgAcquire::URIEncode(Filename);
    else
-      Desc.URI = "store:" + Filename;
+      Desc.URI = "store:" + pkgAcquire::URIEncode(Filename);
    if (DestFile == Filename)
    {
       if (CurrentCompressionExtension == "uncompressed")
@@ -3627,7 +3628,7 @@ void pkgAcqChangelog::Init(std::string const &DestDir, std::string const &DestFi
 	 DestFile = SrcName + ".changelog";
       else
 	 DestFile = DestFilename;
-      Desc.URI = "changelog:/" + DestFile;
+      Desc.URI = "changelog:/" + pkgAcquire::URIEncode(DestFile);
       return;
    }
 
@@ -3792,13 +3793,13 @@ std::string pkgAcqChangelog::URI(std::string const &Template,
       return "";
 
    // the path is: COMPONENT/SRC/SRCNAME/SRCNAME_SRCVER, e.g. main/a/apt/apt_1.1 or contrib/liba/libapt/libapt_2.0
-   std::string Src = SrcName;
-   std::string path = APT::String::Startswith(SrcName, "lib") ? Src.substr(0, 4) : Src.substr(0,1);
-   path.append("/").append(Src).append("/");
-   path.append(Src).append("_").append(StripEpoch(SrcVersion));
+   std::string const Src{SrcName};
+   std::string path = pkgAcquire::URIEncode(APT::String::Startswith(SrcName, "lib") ? Src.substr(0, 4) : Src.substr(0,1));
+   path.append("/").append(pkgAcquire::URIEncode(Src)).append("/");
+   path.append(pkgAcquire::URIEncode(Src)).append("_").append(pkgAcquire::URIEncode(StripEpoch(SrcVersion)));
    // we omit component for releases without one (= flat-style repositories)
    if (Component != NULL && strlen(Component) != 0)
-      path = std::string(Component) + "/" + path;
+      path = pkgAcquire::URIEncode(Component) + "/" + path;
 
    return SubstVar(Template, "@CHANGEPATH@", path);
 }
@@ -3858,8 +3859,11 @@ pkgAcqFile::pkgAcqFile(pkgAcquire *const Owner, string const &URI, HashStringLis
    else
       DestFile = flNotDir(URI);
 
+   ::URI url{URI};
+   url.Path = pkgAcquire::URIEncode(url.Path);
+
    // Create the item
-   Desc.URI = URI;
+   Desc.URI = std::string(url);
    Desc.Description = Dsc;
    Desc.Owner = this;
 
@@ -3901,7 +3905,7 @@ void pkgAcqFile::Done(string const &Message,HashStringList const &CalcHashes,
       if (_config->FindB("Acquire::Source-Symlinks",true) == false ||
 	  Cnf->Removable == true)
       {
-	 Desc.URI = "copy:" + FileName;
+	 Desc.URI = "copy:" + pkgAcquire::URIEncode(FileName);
 	 QueueURI(Desc);
 	 return;
       }
diff --git a/apt-pkg/acquire-method.cc b/apt-pkg/acquire-method.cc
index 486098c77..6e1674f7f 100644
--- a/apt-pkg/acquire-method.cc
+++ b/apt-pkg/acquire-method.cc
@@ -75,6 +75,9 @@ pkgAcqMethod::pkgAcqMethod(const char *Ver,unsigned long Flags)
    if ((Flags & AuxRequests) == AuxRequests)
       try_emplace(fields, "AuxRequests", "true");
 
+   if ((Flags & SendURIEncoded) == SendURIEncoded)
+      try_emplace(fields, "Send-URI-Encoded", "true");
+
    SendMessage("100 Capabilities", std::move(fields));
 
    SetNonBlock(STDIN_FILENO,true);
diff --git a/apt-pkg/acquire-method.h b/apt-pkg/acquire-method.h
index dde28a6ff..e854f5ff1 100644
--- a/apt-pkg/acquire-method.h
+++ b/apt-pkg/acquire-method.h
@@ -112,7 +112,8 @@ class APT_PUBLIC pkgAcqMethod
       LocalOnly = (1 << 3),
       NeedsCleanup = (1 << 4),
       Removable = (1 << 5),
-      AuxRequests = (1 << 6)
+      AuxRequests = (1 << 6),
+      SendURIEncoded = (1 << 7),
    };
 
    void Log(const char *Format,...);
diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index eff79215d..3fbc5c09f 100644
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -307,8 +307,17 @@ bool pkgAcquire::Worker::RunMessages()
                break;
             }
 
-	    std::string const NewURI = LookupTag(Message,"New-URI",URI.c_str());
-            Itm->URI = NewURI;
+	    std::string const GotNewURI = LookupTag(Message,"New-URI",URI.c_str());
+	    if (Config->GetSendURIEncoded())
+	       Itm->URI = GotNewURI;
+	    else
+	    {
+	       ::URI tmpuri{GotNewURI};
+	       tmpuri.Path = pkgAcquire::URIEncode(tmpuri.Path);
+	       Itm->URI = tmpuri;
+	    }
+	    auto NewURI = Itm->URI;
+
 	    auto const AltUris = VectorizeString(LookupTag(Message, "Alternate-URIs"), '\n');
 
 	    ItemDone();
@@ -323,18 +332,37 @@ bool pkgAcquire::Worker::RunMessages()
 	    Itm = nullptr;
 	    for (auto const &Owner: ItmOwners)
 	    {
-	       for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt)
-		  Owner->PushAlternativeURI(std::string(*alt), {}, false);
-
 	       pkgAcquire::ItemDesc &desc = Owner->GetItemDesc();
+
 	       // for a simplified retry a method might redirect without URI change
 	       // see also IsRedirectionLoop implementation
-	       if (desc.URI != NewURI)
+	       bool simpleRetry = false;
+	       if (Config->GetSendURIEncoded())
 	       {
-		  auto newuri = NewURI;
-		  if (Owner->IsGoodAlternativeURI(newuri) == false && Owner->PopAlternativeURI(newuri) == false)
-		     newuri.clear();
-		  if (newuri.empty() || Owner->IsRedirectionLoop(newuri))
+		  for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt)
+		     Owner->PushAlternativeURI(std::string(*alt), {}, false);
+		  if (desc.URI == GotNewURI)
+		     simpleRetry = true;
+	       }
+	       else
+	       {
+		  for (auto alt = AltUris.crbegin(); alt != AltUris.crend(); ++alt)
+		  {
+		     ::URI tmpuri{*alt};
+		     tmpuri.Path = pkgAcquire::URIEncode(tmpuri.Path);
+		     Owner->PushAlternativeURI(std::string(tmpuri), {}, false);
+		  }
+		  ::URI tmpuri{desc.URI};
+		  tmpuri.Path = DeQuoteString(tmpuri.Path);
+		  if (GotNewURI == std::string(tmpuri))
+		     simpleRetry = true;
+	       }
+
+	       if (not simpleRetry)
+	       {
+		  if (Owner->IsGoodAlternativeURI(NewURI) == false && Owner->PopAlternativeURI(NewURI) == false)
+		     NewURI.clear();
+		  if (NewURI.empty() || Owner->IsRedirectionLoop(NewURI))
 		  {
 		     std::string msg = Message;
 		     msg.append("\nFailReason: RedirectionLoop");
@@ -665,12 +693,18 @@ bool pkgAcquire::Worker::Capabilities(string Message)
    Config->NeedsCleanup = StringToBool(LookupTag(Message,"Needs-Cleanup"),false);
    Config->Removable = StringToBool(LookupTag(Message,"Removable"),false);
    Config->SetAuxRequests(StringToBool(LookupTag(Message, "AuxRequests"), false));
+   if (_config->FindB("Acquire::Send-URI-Encoded", true))
+      Config->SetSendURIEncoded(StringToBool(LookupTag(Message, "Send-URI-Encoded"), false));
 
    // Some debug text
    if (Debug == true)
    {
       clog << "Configured access method " << Config->Access << endl;
-      clog << "Version:" << Config->Version << " SingleInstance:" << Config->SingleInstance << " Pipeline:" << Config->Pipeline << " SendConfig:" << Config->SendConfig << " LocalOnly: " << Config->LocalOnly << " NeedsCleanup: " << Config->NeedsCleanup << " Removable: " << Config->Removable << " AuxRequests: " << Config->GetAuxRequests() << endl;
+      clog << "Version:" << Config->Version << " SingleInstance:" << Config->SingleInstance
+	   << " Pipeline:" << Config->Pipeline << " SendConfig:" << Config->SendConfig
+	   << " LocalOnly: " << Config->LocalOnly << " NeedsCleanup: " << Config->NeedsCleanup
+	   << " Removable: " << Config->Removable << " AuxRequests: " << Config->GetAuxRequests()
+	   << " SendURIEncoded: " << Config->GetSendURIEncoded() << '\n';
    }
 
    return true;
@@ -737,6 +771,8 @@ bool pkgAcquire::Worker::SendConfiguration()
       configuration tree */
    std::ostringstream Message;
    Message << "601 Configuration\n";
+   if (not _config->Exists("Acquire::Send-URI-Encoded"))
+      Message << "Config-Item: Acquire::Send-URI-Encoded=1\n";
    _config->Dump(Message, NULL, "Config-Item: %F=%V\n", false);
    Message << '\n';
 
@@ -763,10 +799,16 @@ bool pkgAcquire::Worker::QueueItem(pkgAcquire::Queue::QItem *Item)
 
    string Message = "600 URI Acquire\n";
    Message.reserve(300);
-   Message += "URI: " + Item->URI;
+   URI URL(Item->URI);
+   if (Config->GetSendURIEncoded())
+      Message += "URI: " + Item->URI;
+   else
+   {
+      URL.Path = DeQuoteString(URL.Path);
+      Message += "URI: " + std::string(URL);
+   }
    Message += "\nFilename: " + Item->Owner->DestFile;
 
-   URI URL(Item->URI);
    // FIXME: We should not hard code proxy protocols here.
    if (URL.Access == "http" || URL.Access == "https")
    {
diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc
index 1cffc0463..8693de930 100644
--- a/apt-pkg/acquire.cc
+++ b/apt-pkg/acquire.cc
@@ -51,6 +51,12 @@
 
 using namespace std;
 
+std::string pkgAcquire::URIEncode(std::string const &part)		/*{{{*/
+{
+   // The "+" is encoded as a workaround for an S3 bug (LP#1003633 and LP#1086997)
+   return QuoteString(part, _config->Find("Acquire::URIEncode", "+~ ").c_str());
+}
+									/*}}}*/
 // Acquire::pkgAcquire - Constructor					/*{{{*/
 // ---------------------------------------------------------------------
 /* We grab some runtime state from the configuration space */
@@ -662,10 +668,11 @@ static void CheckDropPrivsMustBeDisabled(pkgAcquire const &Fetcher)
 	 if (_config->Exists(conf) == true)
 	    continue;
 
-	 if (IsAccessibleBySandboxUser(source.Path, false) == false)
+	 auto const filepath = DeQuoteString(source.Path);
+	 if (not IsAccessibleBySandboxUser(filepath, false))
 	 {
 	    _error->NoticeE("pkgAcquire::Run", _("Download is performed unsandboxed as root as file '%s' couldn't be accessed by user '%s'."),
-		  source.Path.c_str(), SandboxUser.c_str());
+			    filepath.c_str(), SandboxUser.c_str());
 	    _config->CndSet("Binary::file::APT::Sandbox::User", "root");
 	    _config->CndSet("Binary::copy::APT::Sandbox::User", "root");
 	 }
@@ -880,6 +887,7 @@ class pkgAcquire::MethodConfig::Private
 {
    public:
    bool AuxRequests = false;
+   bool SendURIEncoded = false;
 };
 pkgAcquire::MethodConfig::MethodConfig() : d(new Private()), Next(0), SingleInstance(false),
 					   Pipeline(false), SendConfig(false), LocalOnly(false), NeedsCleanup(false),
@@ -897,6 +905,17 @@ void pkgAcquire::MethodConfig::SetAuxRequests(bool const value) /*{{{*/
    d->AuxRequests = value;
 }
 									/*}}}*/
+bool pkgAcquire::MethodConfig::GetSendURIEncoded() const		/*{{{*/
+{
+   return d->SendURIEncoded;
+}
+									/*}}}*/
+void pkgAcquire::MethodConfig::SetSendURIEncoded(bool const value)	/*{{{*/
+{
+   d->SendURIEncoded = value;
+}
+									/*}}}*/
+
 // Queue::Queue - Constructor						/*{{{*/
 // ---------------------------------------------------------------------
 /* */
@@ -1073,10 +1092,25 @@ bool pkgAcquire::Queue::Shutdown(bool Final)
 /* */
 pkgAcquire::Queue::QItem *pkgAcquire::Queue::FindItem(string URI,pkgAcquire::Worker *Owner)
 {
-   for (QItem *I = Items; I != 0; I = I->Next)
-      if (I->URI == URI && I->Worker == Owner)
-	 return I;
-   return 0;
+   if (Owner->Config->GetSendURIEncoded())
+   {
+      for (QItem *I = Items; I != nullptr; I = I->Next)
+	 if (I->URI == URI && I->Worker == Owner)
+	    return I;
+   }
+   else
+   {
+      for (QItem *I = Items; I != nullptr; I = I->Next)
+      {
+	 if (I->Worker != Owner)
+	    continue;
+	 ::URI tmpuri{I->URI};
+	 tmpuri.Path = DeQuoteString(tmpuri.Path);
+	 if (URI == std::string(tmpuri))
+	    return I;
+      }
+   }
+   return nullptr;
 }
 									/*}}}*/
 // Queue::ItemDone - Item has been completed				/*{{{*/
diff --git a/apt-pkg/acquire.h b/apt-pkg/acquire.h
index 8cb4d2532..a2c4fbc67 100644
--- a/apt-pkg/acquire.h
+++ b/apt-pkg/acquire.h
@@ -363,6 +363,8 @@ class APT_PUBLIC pkgAcquire
     */
    virtual ~pkgAcquire();
 
+   APT_HIDDEN static std::string URIEncode(std::string const &part);
+
    private:
    APT_HIDDEN void Initialize();
 };
@@ -680,6 +682,8 @@ struct APT_PUBLIC pkgAcquire::MethodConfig
 
    APT_HIDDEN bool GetAuxRequests() const;
    APT_HIDDEN void SetAuxRequests(bool const value);
+   APT_HIDDEN bool GetSendURIEncoded() const;
+   APT_HIDDEN void SetSendURIEncoded(bool const value);
 
    virtual ~MethodConfig();
 };
diff --git a/apt-pkg/deb/debmetaindex.cc b/apt-pkg/deb/debmetaindex.cc
index a72f6d055..f24a5e79e 100644
--- a/apt-pkg/deb/debmetaindex.cc
+++ b/apt-pkg/deb/debmetaindex.cc
@@ -142,10 +142,10 @@ static std::string constructMetaIndexURI(std::string URI, std::string const &Dis
    if (Dist == "/")
       ;
    else if (Dist[Dist.size()-1] == '/')
-      URI += Dist;
+      URI += pkgAcquire::URIEncode(Dist);
    else
-      URI += "dists/" + Dist + "/";
-   return URI + Type;
+      URI += "dists/" + pkgAcquire::URIEncode(Dist) + "/";
+   return URI + pkgAcquire::URIEncode(Type);
 }
 std::string debReleaseIndex::MetaIndexURI(const char *Type) const
 {
@@ -420,6 +420,13 @@ void debReleaseIndex::AddComponent(std::string const &sourcesEntry,	/*{{{*/
       d->DebEntries.push_back(entry);
 }
 									/*}}}*/
+std::string debReleaseIndex::ArchiveURI(std::string const &File) const	/*{{{*/
+{
+   if (File.empty())
+      return URI;
+   return URI + pkgAcquire::URIEncode(File);
+}
+									/*}}}*/
 
 bool debReleaseIndex::Load(std::string const &Filename, std::string * const ErrorText)/*{{{*/
 {
diff --git a/apt-pkg/deb/debmetaindex.h b/apt-pkg/deb/debmetaindex.h
index 5576ff809..717f08e2b 100644
--- a/apt-pkg/deb/debmetaindex.h
+++ b/apt-pkg/deb/debmetaindex.h
@@ -32,7 +32,7 @@ class APT_HIDDEN debReleaseIndex : public metaIndex
    debReleaseIndex(std::string const &URI, std::string const &Dist, bool const Trusted, std::map<std::string,std::string> const &Options);
    virtual ~debReleaseIndex();
 
-   virtual std::string ArchiveURI(std::string const &File) const APT_OVERRIDE {return URI + File;};
+   virtual std::string ArchiveURI(std::string const &File) const APT_OVERRIDE;
    virtual bool GetIndexes(pkgAcquire *Owner, bool const &GetAll=false) APT_OVERRIDE;
    virtual std::vector<IndexTarget> GetIndexTargets() const APT_OVERRIDE;
 
diff --git a/apt-pkg/update.cc b/apt-pkg/update.cc
index 4c64eeb5d..1b25bafd6 100644
--- a/apt-pkg/update.cc
+++ b/apt-pkg/update.cc
@@ -87,6 +87,8 @@ bool AcquireUpdate(pkgAcquire &Fetcher, int const PulseInterval,
       ::URI uri((*I)->DescURI());
       uri.User.clear();
       uri.Password.clear();
+      if ((*I)->Local)
+	 uri.Path = DeQuoteString(uri.Path);
       std::string const descUri = std::string(uri);
       // Show an error for non-transient failures, otherwise only warn
       if ((*I)->Status == pkgAcquire::Item::StatTransientNetworkError)