Skip to content

Bug 5523: Do not percent-encode URI suffixes #2289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rousskov wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Bug 5523: Do not percent-encode URI suffixes #2289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
857dc4a
Bug 5523: Do not encode URI query
rousskov Oct 29, 2025
36d46f5
fixup: Remove unused PathChars() [-Wunused-function]
rousskov Oct 29, 2025
f47c9e6
fixup: Restore rfc1738_unescape() calls
rousskov Oct 29, 2025
fd45a1a
Remove Ftp::Gateway::decodedRequestUriPath()
rousskov Oct 29, 2025
cd53bfb
fixup: Remove a comment made stale by other branch changes
rousskov Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/acl/UrlPath.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,19 @@
#include "acl/FilledChecklist.h"
#include "acl/UrlPath.h"
#include "HttpRequest.h"
#include "rfc1738.h"

int
Acl::UrlPathCheck::match(ACLChecklist * const ch)
{
const auto checklist = Filled(ch);
auto urlPath = checklist->request->url.path();

if (urlPath.isEmpty())
if (checklist->request->url.path().isEmpty())
return -1;

return data->match(urlPath.c_str());
const auto esc_buf = SBufToCstring(checklist->request->url.path());
rfc1738_unescape(esc_buf);
const auto result = data->match(esc_buf);
xfree(esc_buf);
return result;
Copy link
Contributor Author

@rousskov rousskov Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const and AAA aside, these changes restore pre ff60b10 code. Polishing that code is out of this PR scope, and I will remove the remaining cosmetic improvements if requested.

}
21 changes: 1 addition & 20 deletions src/anyp/Uri.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,25 +50,6 @@ UserInfoChars()
return userInfoValid;
}

/// Characters which are valid within a URI path section
static const CharacterSet &
PathChars()
{
/*
* RFC 3986 section 3.3
*
* path = path-abempty ; begins with "/" or is empty
* ...
* path-abempty = *( "/" segment )
* segment = *pchar
* pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
*/
static const auto pathValid = CharacterSet("path", "/:@-._~%!$&'()*+,;=") +
CharacterSet::ALPHA +
CharacterSet::DIGIT;
return pathValid;
}

/**
* Governed by RFC 3986 section 2.1
*/
Expand Down Expand Up @@ -776,7 +757,7 @@ AnyP::Uri::absolutePath() const
{
if (absolutePath_.isEmpty()) {
// TODO: Encode each URI subcomponent in path_ as needed.
absolutePath_ = Encode(path(), PathChars());
absolutePath_ = path();
}

return absolutePath_;
Expand Down
18 changes: 4 additions & 14 deletions src/clients/FtpGateway.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ class Gateway : public Ftp::Client

int checkAuth(const HttpHeader * req_hdr);
void checkUrlpath();
std::optional<SBuf> decodedRequestUriPath() const;
void buildTitleUrl();
void writeReplyBody(const char *, size_t len);
void completeForwarding() override;
Expand Down Expand Up @@ -2304,15 +2303,7 @@ ftpReadQuit(Ftp::Gateway * ftpState)
ftpState->serverComplete();
}

/// absolute request URI path after successful decoding of all pct-encoding sequences
std::optional<SBuf>
Ftp::Gateway::decodedRequestUriPath() const
{
return AnyP::Uri::Decode(request->url.absolutePath());
}

/// \prec !ftpState->flags.try_slash_hack
/// \prec ftpState->decodedRequestUriPath()
static void
ftpTrySlashHack(Ftp::Gateway * ftpState)
{
Expand All @@ -2325,9 +2316,10 @@ ftpTrySlashHack(Ftp::Gateway * ftpState)
wordlistDestroy(&ftpState->pathcomps);

/* Build the new path */
// XXX: Conversion to c-string effectively truncates where %00 was decoded
safe_free(ftpState->filepath);
ftpState->filepath = SBufToCstring(ftpState->decodedRequestUriPath().value());
const auto path = SBufToCstring(ftpState->request->url.absolutePath());
rfc1738_unescape(path);
ftpState->filepath = path;
Copy link
Contributor Author

@rousskov rousskov Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty lines removal, const/AAA in path declaration, and some comment movements aside, these changes restore pre ff60b10 code. Polishing this code is out of this PR scope, and I will remove the remaining cosmetic improvements if requested.


/* And off we go */
ftpGetFile(ftpState);
Expand Down Expand Up @@ -2382,15 +2374,13 @@ ftpFail(Ftp::Gateway *ftpState)
" reply code " << code << "flags(" <<
(ftpState->flags.isdir?"IS_DIR,":"") <<
(ftpState->flags.try_slash_hack?"TRY_SLASH_HACK":"") << "), " <<
"decodable_filepath=" << bool(ftpState->decodedRequestUriPath()) << ' ' <<
"mdtm=" << ftpState->mdtm << ", size=" << ftpState->theSize <<
"slashhack=" << (slashHack? "T":"F"));

/* Try the / hack to support "Netscape" FTP URL's for retrieving files */
if (!ftpState->flags.isdir && /* Not a directory */
!ftpState->flags.try_slash_hack && !slashHack && /* Not doing slash hack */
ftpState->mdtm <= 0 && ftpState->theSize < 0 && /* Not known as a file */
ftpState->decodedRequestUriPath()) {
ftpState->mdtm <= 0 && ftpState->theSize < 0) { /* Not known as a file */

switch (ftpState->state) {

Expand Down