From dc1301e9e3d461a849eb46f6922b52619dd203a5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Thu, 23 Apr 2026 18:21:05 -0400 Subject: [PATCH 01/14] WIP: Do not expand errorpage %codes injected by X509 certificates This WIP commit records a failed attempt to solve the problem by exposing ErrorState::compileLeadingCode() code to Security::ErrorDetail. This code does not compile, but that is easy to fix. The primary changes committed here already show many XXXs that would be difficult to address. I hope to find a better solution. The problem description below should still apply, and even the high-level solution analysis below may apply. ---- When available, Security::ErrorDetail is added to Squid-generated errors that use customizable errorpage formats containing `%D` errorpage `%code`. Security::ErrorDetail itself supports customizable verbose reporting format (see `detail` in `errors/templates/error-details.txt`). ErrorDetail format may contain both ErrorDetail-specific %codes and generic legacy errorpage %codes handled by ErrorState. To support the above "nested" formats when processing `%D`, ErrorState::compileLegacyCode() compiled ErrorDetail output, substituting any legacy errorpage %codes outputted by ErrorDetail. That re-compilation could not distinguish a `%code` sequence configured by `error-details.txt` from a `%code` sequence that came from, say, a received X509 certificate field. Both would be expanded! This bug applies to both legacy errorpage `%code` sequences like `%R` and modern `@Squid{logformat %code}` errorpage sequences. X509 certificate fields are the only known injection vector, but it is conceivable that some other error details may contain character sequences that match errorpage legacy %codes (e.g., SysErrorDetail returns strerror(3) output that Squid does not control and did not escape. This bug is similar to a bug fixed in 2018 commit 6feeb15f, but this one deals with injected errorpage %codes rather than HTML tags. Two primary solution candidates were considered: A) Escape-then-compile-to-unescape: Security::ErrorDetail could escape `%code` sequences received from "external" sources, so that ErrorState::compile() would see `%%X` and replace that with `%X`, correctly relaying received `%X`. This solution was rejected because we risk forgetting to escape some input, now or during code refactoring, especially since the risk applies to all ErrorDetail classes. Also, escaping what we know we will immediately un-escape is wasteful. B) Never compile "external" input. This implemented solution required giving ErrorDetail::verbose() access to the `%code` compiling code in ErrorState, so that ErrorDetail can decide what to compile and what not to. Now, Security::ErrorDetail only compiles `%code` sequences found in `detail`; ErrorState does not compile ErrorDetail output. Side effects ------------ The `details` field in `error-details.txt` now supports modern `@Squid{logformat %code}` errorpage sequences, addressing an old TODO. The `descr` field in `error-details.txt` no longer supports `%code` sequences. It was never documented to support them. It is not meant to contain such sequences -- they belong to the `details` field. Hopefully, no deployed configurations use `%code` sequences in that field. --- src/errorpage.cc | 44 ++++++++++++++++++++++---------- src/security/ErrorDetail.cc | 51 +++++++++++++++++++++++++++++++++---- src/servers/FtpServer.cc | 2 +- 3 files changed, 78 insertions(+), 19 deletions(-) diff --git a/src/errorpage.cc b/src/errorpage.cc index 2569d3b3ca5..aa3435cae79 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -1002,9 +1002,7 @@ ErrorState::compileLegacyCode(Build &build) if (!build.allowRecursion) p = "%D"; // if recursion is not allowed, do not convert else if (detail) { - auto rawDetail = detail->verbose(request); - // XXX: Performance regression. c_str() reallocates - const auto compiledDetail = compileBody(rawDetail.c_str(), false); + const auto compiledDetail = detail->verbose(request, this); mb.append(compiledDetail.rawContent(), compiledDetail.length()); do_quote = 0; } @@ -1457,24 +1455,44 @@ ErrorState::compile(const char *input, bool building_deny_info_url, bool allowRe build.input = input; auto blockStart = build.input; - while (const auto letter = *build.input) { - if (letter == '%') { + while (*build.input) { + // here, "code" and %code are used in compileLeadingCode() sense + auto oneCode = build; + oneCode.output.clear(); // to capture just one compiled leading %code below + if (compileLeadingCode(oneCode)) { + // %code-less input block before the compiled %code build.output.append(blockStart, build.input - blockStart); - compileLegacyCode(build); + // compiled %code itself + build.output.append(oneCode.output); + Assure(build.input < oneCode.input); // we are making progress + build.input = oneCode.input; // after the compiled %code blockStart = build.input; - } - else if (letter == '@' && LogformatMagic.cmp(build.input, LogformatMagic.length()) == 0) { - build.output.append(blockStart, build.input - blockStart); - compileLogformatCode(build); - blockStart = build.input; - } else { + } else ++build.input; - } } build.output.append(blockStart, build.input - blockStart); return build.output; } +/// Here, "code" means legacy %code or modern @Squid{logformat %code) sequence +SBuf +ErrorState::compileLeadingCode(Build &build) +{ + const auto letter = *build.input; // may be the terminating NUL + + if (letter == '%') { + compileLegacyCode(build); + return true; + } + + if (letter == '@' && LogformatMagic.cmp(build.input, LogformatMagic.length()) == 0) { + compileLogformatCode(build); + return true; + } + + return false; +} + /// react to a compile() error /// \param msg description of what went wrong /// \param errorLocation approximate start of the problematic input diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index afe9fd2e0eb..7b19300fff3 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -530,8 +530,23 @@ Security::ErrorDetail::brief() const return os.buf(); } +// XXX: Duplicates errorpage.cc code. +namespace ErrorPage { + +/// state and parameters shared by several ErrorState::compile*() methods +class Build +{ +public: + SBuf output; ///< compilation result + const char *input = nullptr; ///< template bytes that need to be compiled + bool building_deny_info_url = false; ///< whether we compile deny_info URI + bool allowRecursion = false; ///< whether top-level compile() calls are OK +}; + +} // namespace ErrorPage + SBuf -Security::ErrorDetail::verbose(const HttpRequestPointer &request) const +Security::ErrorDetail::verbose(const HttpRequestPointer &request, ErrorDetailCompiler * const compiler) const { std::optional customFormat; #if USE_OPENSSL @@ -549,12 +564,38 @@ Security::ErrorDetail::verbose(const HttpRequestPointer &request) const SBufStream os; assert(format); auto remainder = format; - while (auto p = strchr(remainder, '%')) { + // TODO: This loop condition mishandles modern @Squid{logformat %code} + // elements that compileLeadingCode() called below does support. We do not + // have to support them (in this branch), but we should not mishandle them! + while (const auto p = strchr(remainder, '%')) { os.write(remainder, p - remainder); - const auto formattingCodeLen = convertErrorCodeToDescription(++p, os); - if (!formattingCodeLen) + remainder = p; + + // XXX: convertErrorCodeToDescription() does not need this. + Build build; + build.building_deny_info_url = false; // XXX: Assumes the caller does not support this. Correct for now. + build.allowRecursion = false; + build.input = remainder; + + // When compiler is nil, we leave behind both legacy errorpage %codes and modern @Squid{} codes. + // The former is a regression because those codes used to be compiled by `%D` code in errorpage.cc. + + // XXX: convertErrorCodeToDescription() does not want to see the leading + // '%' character, but compileLeadingCode() does need it, causing +1s. + if (const auto formattingCodeLen = convertErrorCodeToDescription(remainder+1, os)) { + remainder += formattingCodeLen + 1; + } else if (compiler && compiler->compileLeadingCode(oneCode)) { + os << oneCode.output; + Assure(remainder < oneCode.input); // we are making progress + remainder = oneCode.input; + } else { + // TODO: Perhaps compiler->compileLeadingCode() should always + // "succeed" if given a %code? We do not really support incremental + // parsing anyway: compileLogformatCode() rejects missing '}', and + // compileLegacyCode() in master/v8 now barf at "%\0", at least. os << '%'; - remainder = p + formattingCodeLen; + ++remainder; + } } os << remainder; return os.buf(); diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index bb236a3bc6e..d2c0f4d615f 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1105,7 +1105,7 @@ Ftp::Server::writeErrorReply(const HttpReply *reply, const int scode) for (const auto &detail: request->error.details) { mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief())); - mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->verbose(request))); + mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->verbose(request, nullptr))); } #if USE_ADAPTATION From 1dab51220c3bce403831add7a06c12665d922f72 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 24 Apr 2026 16:38:06 -0400 Subject: [PATCH 02/14] WIP: Addressed most XXXs/TODOs in the previous branch commit Removed the compilation loop from Security::ErrorDetail::verbose(). The old ErrorState::compile() loop is now responsible for calling Security::ErrorDetail for parsing custom %codes. Besides out-of-scope legacy const-correctness problems and basic build fixes, we need to resolve one new API problem: An ErrorDetail::verbose() caller supplies build.output, but the method returns its output instead of updating the supplied field. The solution may affect verbose() API, so it blocks those "basic build fixes". --- src/error/Detail.h | 3 +- src/error/forward.h | 5 +++ src/errorpage.cc | 89 ++++++++++++++++++------------------- src/errorpage.h | 67 ++++++++++++++++++++++++---- src/security/ErrorDetail.cc | 81 ++++++++++----------------------- src/security/ErrorDetail.h | 8 +++- src/servers/FtpServer.cc | 17 +++++-- 7 files changed, 153 insertions(+), 117 deletions(-) diff --git a/src/error/Detail.h b/src/error/Detail.h index 9a6c3b23fed..fdd65f2a780 100644 --- a/src/error/Detail.h +++ b/src/error/Detail.h @@ -21,6 +21,7 @@ class ErrorDetail: public RefCountable { public: using Pointer = ErrorDetailPointer; + using Build = ErrorPage::Build; ~ErrorDetail() override {} @@ -30,7 +31,7 @@ class ErrorDetail: public RefCountable /// \returns all available details; may be customized for the given request /// suitable for error pages and other output meant for human consumption - virtual SBuf verbose(const HttpRequestPointer &) const = 0; + virtual SBuf verbose2(const Build &) const = 0; // Duplicate details for the same error typically happen when we update some // error storage (e.g., ALE) twice from the same detail source (e.g., the diff --git a/src/error/forward.h b/src/error/forward.h index e66bd492dfe..1741dff9a5d 100644 --- a/src/error/forward.h +++ b/src/error/forward.h @@ -92,6 +92,11 @@ class Error; class ErrorDetail; class ErrorState; +namespace ErrorPage { + class PercentCodeCompiler; + class Build; +} // namespace ErrorPage + typedef RefCount ErrorDetailPointer; #endif /* SQUID_SRC_ERROR_FORWARD_H */ diff --git a/src/errorpage.cc b/src/errorpage.cc index aa3435cae79..50f54e20540 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -102,16 +102,6 @@ class ErrorDynamicPageInfo { namespace ErrorPage { -/// state and parameters shared by several ErrorState::compile*() methods -class Build -{ -public: - SBuf output; ///< compilation result - const char *input = nullptr; ///< template bytes that need to be compiled - bool building_deny_info_url = false; ///< whether we compile deny_info URI - bool allowRecursion = false; ///< whether top-level compile() calls are OK -}; - /// pretty-prints error page/deny_info building error class BuildErrorPrinter { @@ -1002,7 +992,7 @@ ErrorState::compileLegacyCode(Build &build) if (!build.allowRecursion) p = "%D"; // if recursion is not allowed, do not convert else if (detail) { - const auto compiledDetail = detail->verbose(request, this); + const auto compiledDetail = detail->verbose2(build); mb.append(compiledDetail.rawContent(), compiledDetail.length()); do_quote = 0; } @@ -1447,50 +1437,38 @@ ErrorState::compileBody(const char *input, bool allowRecursion) SBuf ErrorState::compile(const char *input, bool building_deny_info_url, bool allowRecursion) { - assert(input); - - Build build; + Build build(*request, input, *this); build.building_deny_info_url = building_deny_info_url; build.allowRecursion = allowRecursion; - build.input = input; - - auto blockStart = build.input; - while (*build.input) { - // here, "code" and %code are used in compileLeadingCode() sense - auto oneCode = build; - oneCode.output.clear(); // to capture just one compiled leading %code below - if (compileLeadingCode(oneCode)) { - // %code-less input block before the compiled %code - build.output.append(blockStart, build.input - blockStart); - // compiled %code itself - build.output.append(oneCode.output); - Assure(build.input < oneCode.input); // we are making progress - build.input = oneCode.input; // after the compiled %code - blockStart = build.input; - } else - ++build.input; - } - build.output.append(blockStart, build.input - blockStart); + build.compile(); return build.output; } -/// Here, "code" means legacy %code or modern @Squid{logformat %code) sequence -SBuf -ErrorState::compileLeadingCode(Build &build) +void +ErrorState::compile(Build &build) const { - const auto letter = *build.input; // may be the terminating NUL + assert(build.input); - if (letter == '%') { - compileLegacyCode(build); - return true; - } + // TODO: Instead of violating const-correctness with const_cast + // below, adjust compile*() methods to avoid ErrorState modifications. - if (letter == '@' && LogformatMagic.cmp(build.input, LogformatMagic.length()) == 0) { - compileLogformatCode(build); - return true; + auto blockStart = build.input; + while (const auto letter = *build.input) { + if (letter == '%') { + build.output.append(blockStart, build.input - blockStart); + if (build.secondaryCompiler && !build.secondaryCompiler->compilePercentCode(build)) + const_cast(this)->compileLegacyCode(build); + blockStart = build.input; + } + else if (letter == '@' && LogformatMagic.cmp(build.input, LogformatMagic.length()) == 0) { + build.output.append(blockStart, build.input - blockStart); + const_cast(this)->compileLogformatCode(build); + blockStart = build.input; + } else { + ++build.input; + } } - - return false; + build.output.append(blockStart, build.input - blockStart); } /// react to a compile() error @@ -1528,6 +1506,25 @@ ErrorState::noteBuildError_(const char *const msg, const char * const errorLocat } } +/* ErrorPage::Build */ + +ErrorPage::Build::Build(const HttpRequest &aRequest, const char * const templateFragment, const PrimaryCompiler &aPrimaryCompiler): + request(aRequest), + primaryCompiler(aPrimaryCompiler), + input(templateFragment) +{ +} + +ErrorPage::Build +ErrorPage::Build::subtask(const char * const templateFragment, const PercentCodeCompiler &aSecondaryCompiler) const +{ + Build subBuild(request, templateFragment, primaryCompiler); + subBuild.secondaryCompiler = &aSecondaryCompiler; + subBuild.building_deny_info_url = building_deny_info_url; + subBuild.allowRecursion = allowRecursion; + return subBuild; +} + /* ErrorPage::BuildErrorPrinter */ std::ostream & diff --git a/src/errorpage.h b/src/errorpage.h index 350484fe504..92f9c94769c 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -78,18 +78,14 @@ class MemBuf; class StoreEntry; class wordlist; -namespace ErrorPage { - -class Build; - -} // namespace ErrorPage - /// \ingroup ErrorPageAPI class ErrorState { CBDATA_CLASS(ErrorState); public: + using Build = ErrorPage::Build; + /// creates an error of type other than ERR_RELAY_REMOTE ErrorState(err_type type, Http::StatusCode, HttpRequest * request, const AccessLogEntryPointer &al); ErrorState() = delete; // not implemented. @@ -113,12 +109,13 @@ class ErrorState /// ensures that a future BuildHttpReply() is likely to succeed void validate(); + /// appends build.input to build.output after replacing all %codes + void compile(Build &) const; + /// the source of the error template (for reporting purposes) SBuf inputLocation; private: - typedef ErrorPage::Build Build; - /// initializations shared by public constructors ErrorState(err_type, const AccessLogEntryPointer &); @@ -136,6 +133,7 @@ class ErrorState /// compile @Squid{%code} sequence containing a single logformat %code void compileLogformatCode(Build &build); + /// compile(Build&) convenience wrapper without a custom compiler support; /// replaces all legacy and logformat %codes in the given input /// \param input the template text to be converted /// \param building_deny_info_url whether input is a deny_info URL parameter @@ -336,6 +334,59 @@ class TemplateFile err_type templateCode; ///< The internal code for this template. }; +namespace ErrorPage { + +/// An API for recognizing and replaces a %code sequence. +class PercentCodeCompiler: public Interface { +public: + /// Either recognizes and replaces a single %code sequence at the beginning + /// of `build.input`, appending the replacement to `build.output`, OR does + /// not modify `build`. + /// \returns false when the leading %code was not recognized + /// \prec `build.input` starts with a `%` character. + virtual bool compilePercentCode(Build &) const = 0; +}; + +/// Manages conversion of an errorpage template fragment (or equivalent) into an +/// error response body fragment. This conversion replaces legacy errorpage +/// %code sequences, logformat %code sequences, and, in some cases, +/// context-dependent %code sequences. Related ErrorState methods refer to this +/// substitution process as errorpage "compilation". +class Build +{ +public: + using PrimaryCompiler = ErrorState; + + /// Creates a build step that focuses on the given template fragment, using + /// the given primary compiler. The build object is expected to be allocated + /// on stack and have shorter lifetime than constructor parameters lifetime. + explicit Build(const HttpRequest &, const char *templateFragment, const PrimaryCompiler &); + + /// Creates a build step that focuses on the given template fragment, using + /// the given "secondary" context-dependent compiler. The build object + /// lifetime is expected to be shorter than that of function parameters. + Build subtask(const char *templateFragment, const PercentCodeCompiler &secondary) const; + + /// converts `input` into `output`, replacing all %codes as needed + void compile() { primaryCompiler.compile(*this); } + + /// transaction that triggered this build + const HttpRequest &request; + + /// handles legacy errorpage %code sequences and logformat %code sequences + const PrimaryCompiler &primaryCompiler; + + /// handles context-dependent %code sequences + const PercentCodeCompiler *secondaryCompiler = nullptr; + + SBuf output; ///< compilation result + const char *input; ///< template bytes that need to be compiled; never nil + bool building_deny_info_url = false; ///< whether we compile deny_info URI + bool allowRecursion = false; ///< whether top-level compile() calls are OK +}; + +} // namespace ErrorPage + /** * Parses the Accept-Language header value and return one language item on * each call. diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 7b19300fff3..77a818f8986 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -530,75 +530,24 @@ Security::ErrorDetail::brief() const return os.buf(); } -// XXX: Duplicates errorpage.cc code. -namespace ErrorPage { - -/// state and parameters shared by several ErrorState::compile*() methods -class Build -{ -public: - SBuf output; ///< compilation result - const char *input = nullptr; ///< template bytes that need to be compiled - bool building_deny_info_url = false; ///< whether we compile deny_info URI - bool allowRecursion = false; ///< whether top-level compile() calls are OK -}; - -} // namespace ErrorPage - +// XXX: API contradiction: Caller supplies build.output but we return new output instead of using that SBuf -Security::ErrorDetail::verbose(const HttpRequestPointer &request, ErrorDetailCompiler * const compiler) const +Security::ErrorDetail::verbose2(const ErrorPage::Build &build) const { std::optional customFormat; #if USE_OPENSSL - if (const auto errorDetail = Ssl::ErrorDetailsManager::GetInstance().findDetail(error_no, request)) { + if (const auto errorDetail = Ssl::ErrorDetailsManager::GetInstance().findDetail(error_no, &build.request)) { detailEntry = *errorDetail; customFormat = detailEntry->detail; } else { detailEntry.reset(); } -#else - (void)request; #endif auto format = customFormat ? customFormat->c_str() : "SSL handshake error (%err_name)"; - SBufStream os; - assert(format); - auto remainder = format; - // TODO: This loop condition mishandles modern @Squid{logformat %code} - // elements that compileLeadingCode() called below does support. We do not - // have to support them (in this branch), but we should not mishandle them! - while (const auto p = strchr(remainder, '%')) { - os.write(remainder, p - remainder); - remainder = p; - - // XXX: convertErrorCodeToDescription() does not need this. - Build build; - build.building_deny_info_url = false; // XXX: Assumes the caller does not support this. Correct for now. - build.allowRecursion = false; - build.input = remainder; - - // When compiler is nil, we leave behind both legacy errorpage %codes and modern @Squid{} codes. - // The former is a regression because those codes used to be compiled by `%D` code in errorpage.cc. - - // XXX: convertErrorCodeToDescription() does not want to see the leading - // '%' character, but compileLeadingCode() does need it, causing +1s. - if (const auto formattingCodeLen = convertErrorCodeToDescription(remainder+1, os)) { - remainder += formattingCodeLen + 1; - } else if (compiler && compiler->compileLeadingCode(oneCode)) { - os << oneCode.output; - Assure(remainder < oneCode.input); // we are making progress - remainder = oneCode.input; - } else { - // TODO: Perhaps compiler->compileLeadingCode() should always - // "succeed" if given a %code? We do not really support incremental - // parsing anyway: compileLogformatCode() rejects missing '}', and - // compileLegacyCode() in master/v8 now barf at "%\0", at least. - os << '%'; - ++remainder; - } - } - os << remainder; - return os.buf(); + auto myBuild = build.subtask(format, *this); + myBuild.compile(); // calls our compilePercentCode() as needed + return myBuild.output; } /// textual representation of the subject of the broken certificate @@ -780,6 +729,24 @@ Security::ErrorDetail::printErrorLibError(std::ostream &os) const os << "[No Error]"; } +bool +Security::ErrorDetail::compilePercentCode(ErrorPage::Build &build) const +{ + Assure(build.input); + Assure(*build.input == '%'); + const auto codeNameStart = build.input + 1; + SBufStream os; + if (const auto codeNameLength = convertErrorCodeToDescription(codeNameStart, os)) { + Assure(build.input < codeNameStart + codeNameLength); // making parsing progress + Assure(codeNameStart + codeNameLength <= build.input + strlen(build.input)); // still within bounds + build.input = codeNameStart + codeNameLength; + build.output.append(os.buf()); + return true; + } + + return false; // including bare/trailing '%' cases +} + /** * Converts the code to a string value. Supported formatting codes are: * diff --git a/src/security/ErrorDetail.h b/src/security/ErrorDetail.h index b027ca80b34..bbff47c75dc 100644 --- a/src/security/ErrorDetail.h +++ b/src/security/ErrorDetail.h @@ -11,6 +11,7 @@ #include "base/RefCount.h" #include "error/Detail.h" +#include "errorpage.h" #include "http/forward.h" #include "security/forward.h" #include "SquidString.h" @@ -36,7 +37,7 @@ namespace Security { /// * for certificate validation errors: validation failure reason /// * for non-validation errors: TLS library-reported error(s) /// * for non-validation errors: system call errno(3) -class ErrorDetail: public ::ErrorDetail +class ErrorDetail: public ::ErrorDetail, public ErrorPage::PercentCodeCompiler { MEMPROXY_CLASS(Security::ErrorDetail); @@ -64,7 +65,7 @@ class ErrorDetail: public ::ErrorDetail /* ErrorDetail API */ SBuf brief() const override; - SBuf verbose(const HttpRequestPointer &) const override; + SBuf verbose2(const Build &) const override; /// \returns error category; \see ErrorCode ErrorCode errorNo() const { return error_no; } @@ -87,6 +88,9 @@ class ErrorDetail: public ::ErrorDetail private: ErrorDetail(ErrorCode err, int aSysErrorNo); + /* ErrorPage::PercentCodeCompiler API */ + bool compilePercentCode(ErrorPage::Build &) const override; + /* methods for formatting error details using admin-configurable %codes */ void printSubject(std::ostream &os) const; void printCaName(std::ostream &os) const; diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index d2c0f4d615f..ba4333b90af 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1103,9 +1103,20 @@ Ftp::Server::writeErrorReply(const HttpReply *reply, const int scode) if (request->error) mb.appendf("%i-%s\r\n", scode, errorPageName(request->error.category)); - for (const auto &detail: request->error.details) { - mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief())); - mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->verbose(request, nullptr))); + if (!request->error.details.empty()) { + // Addressing TODO at the end of this method should remove this temporary hack. + // XXX: ErrorState currently requires non-constant HttpRequest, HttpReply. + const auto err = std::make_unique( + const_cast(request), + const_cast(reply), + pipeline.front()->http->al); + + for (const auto &detail: request->error.details) { + ErrorPage::Build build(*request, "", *err); + detail->verbose(build); + mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief())); + mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(build.output)); + } } #if USE_ADAPTATION From 17b5b30eff732b51dba1872e7c89458432274573 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Mon, 27 Apr 2026 16:46:52 -0400 Subject: [PATCH 03/14] fixup: Addressed API XXX marked in the previous branch commit ErrorDetail::verbose() does not need access to Build. Unlike internal ErrorPage methods, current ErrorDetail objects do not need to parse portions of error templates, engaging multiple portion-specific parsers. ErrorDetail::verbose() API is much simpler -- "dump details" (into the returned SBuf value). The only wrinkle here is that Security::ErrorDetail needs ErrorState compilation abilities to do that, but that can be handled by adding an ErrorState parameter. And since ErrorState objects already have `request` data members, we can drop the existing HttpRequest parameter. TODO: ErrorDetail::verbose() may benefit from switching to an std::ostream-based API, but doing so now would increase out-of-scope noise. It is probably best done together with upgrading ErrorState "compilation" methods to use std::ostream. --- src/error/Detail.h | 7 +++--- src/errorpage.cc | 49 ++++++++++++++++++++----------------- src/errorpage.h | 47 ++++++++++++----------------------- src/security/ErrorDetail.cc | 12 +++------ src/security/ErrorDetail.h | 2 +- src/servers/FtpServer.cc | 6 ++--- 6 files changed, 54 insertions(+), 69 deletions(-) diff --git a/src/error/Detail.h b/src/error/Detail.h index fdd65f2a780..4caaf106129 100644 --- a/src/error/Detail.h +++ b/src/error/Detail.h @@ -21,7 +21,7 @@ class ErrorDetail: public RefCountable { public: using Pointer = ErrorDetailPointer; - using Build = ErrorPage::Build; + using ErrorTemplateCompiler = ErrorState; ~ErrorDetail() override {} @@ -29,9 +29,10 @@ class ErrorDetail: public RefCountable /// suitable as an access.log field and similar output processed by programs virtual SBuf brief() const = 0; - /// \returns all available details; may be customized for the given request + /// \returns all available details /// suitable for error pages and other output meant for human consumption - virtual SBuf verbose2(const Build &) const = 0; + /// Supports configurable templates populated by the given compiler. + virtual SBuf verbose(const ErrorTemplateCompiler &) const = 0; // Duplicate details for the same error typically happen when we update some // error storage (e.g., ALE) twice from the same detail source (e.g., the diff --git a/src/errorpage.cc b/src/errorpage.cc index 50f54e20540..d120c12a9be 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -992,7 +992,7 @@ ErrorState::compileLegacyCode(Build &build) if (!build.allowRecursion) p = "%D"; // if recursion is not allowed, do not convert else if (detail) { - const auto compiledDetail = detail->verbose2(build); + const auto compiledDetail = detail->verbose(*this); mb.append(compiledDetail.rawContent(), compiledDetail.length()); do_quote = 0; } @@ -1437,13 +1437,37 @@ ErrorState::compileBody(const char *input, bool allowRecursion) SBuf ErrorState::compile(const char *input, bool building_deny_info_url, bool allowRecursion) { - Build build(*request, input, *this); + assert(input); + + Build build; build.building_deny_info_url = building_deny_info_url; build.allowRecursion = allowRecursion; - build.compile(); + build.input = input; + + compile(build); + Assure(!*build.input); // compiled the whole template + + return build.output; +} + +SBuf +ErrorState::compileDetail(const char * const format, const ErrorPage::PercentCodeCompiler * const secondaryCompiler) const +{ + Assure(format); + + Build build; + build.input = format; + build.secondaryCompiler = secondaryCompiler; // may be nil + + compile(build); + Assure(!*build.input); // compiled the whole template + return build.output; } +/// Replaces all %code sequences found in build.input. +/// Appends processed input to build.output. +/// Consumes processed build.input. void ErrorState::compile(Build &build) const { @@ -1506,25 +1530,6 @@ ErrorState::noteBuildError_(const char *const msg, const char * const errorLocat } } -/* ErrorPage::Build */ - -ErrorPage::Build::Build(const HttpRequest &aRequest, const char * const templateFragment, const PrimaryCompiler &aPrimaryCompiler): - request(aRequest), - primaryCompiler(aPrimaryCompiler), - input(templateFragment) -{ -} - -ErrorPage::Build -ErrorPage::Build::subtask(const char * const templateFragment, const PercentCodeCompiler &aSecondaryCompiler) const -{ - Build subBuild(request, templateFragment, primaryCompiler); - subBuild.secondaryCompiler = &aSecondaryCompiler; - subBuild.building_deny_info_url = building_deny_info_url; - subBuild.allowRecursion = allowRecursion; - return subBuild; -} - /* ErrorPage::BuildErrorPrinter */ std::ostream & diff --git a/src/errorpage.h b/src/errorpage.h index 92f9c94769c..336a77d8f9a 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -109,8 +109,10 @@ class ErrorState /// ensures that a future BuildHttpReply() is likely to succeed void validate(); - /// appends build.input to build.output after replacing all %codes - void compile(Build &) const; + /// Replaces all %codes in the given ErrorDetail `format` template. + /// \param compiler is an optional handler for caller-specific template %code sequences + /// \sa compile() + SBuf compileDetail(const char *format, const ErrorPage::PercentCodeCompiler *compiler) const; /// the source of the error template (for reporting purposes) SBuf inputLocation; @@ -133,14 +135,16 @@ class ErrorState /// compile @Squid{%code} sequence containing a single logformat %code void compileLogformatCode(Build &build); - /// compile(Build&) convenience wrapper without a custom compiler support; /// replaces all legacy and logformat %codes in the given input /// \param input the template text to be converted /// \param building_deny_info_url whether input is a deny_info URL parameter /// \param allowRecursion whether to compile %codes which produce %codes /// \returns the given input with all %codes substituted + /// \sa compileDetail() SBuf compile(const char *input, bool building_deny_info_url, bool allowRecursion); + void compile(Build &build) const; + /// React to a compile() error, throwing if buildContext allows. /// \param msg description of what went wrong /// \param errorLocation approximate start of the problematic input @@ -347,40 +351,21 @@ class PercentCodeCompiler: public Interface { virtual bool compilePercentCode(Build &) const = 0; }; -/// Manages conversion of an errorpage template fragment (or equivalent) into an -/// error response body fragment. This conversion replaces legacy errorpage -/// %code sequences, logformat %code sequences, and, in some cases, -/// context-dependent %code sequences. Related ErrorState methods refer to this -/// substitution process as errorpage "compilation". +/// State and parameters shared by several +/// PercentCodeCompiler::compilePercentCode() and ErrorState::compile*() methods +/// that convert an errorpage template fragment (or equivalent) into an error +/// response body fragment. This conversion replaces legacy errorpage %code +/// sequences, logformat %code sequences, and, in some cases, context-dependent +/// %code sequences. class Build { public: - using PrimaryCompiler = ErrorState; - - /// Creates a build step that focuses on the given template fragment, using - /// the given primary compiler. The build object is expected to be allocated - /// on stack and have shorter lifetime than constructor parameters lifetime. - explicit Build(const HttpRequest &, const char *templateFragment, const PrimaryCompiler &); - - /// Creates a build step that focuses on the given template fragment, using - /// the given "secondary" context-dependent compiler. The build object - /// lifetime is expected to be shorter than that of function parameters. - Build subtask(const char *templateFragment, const PercentCodeCompiler &secondary) const; - - /// converts `input` into `output`, replacing all %codes as needed - void compile() { primaryCompiler.compile(*this); } - - /// transaction that triggered this build - const HttpRequest &request; - - /// handles legacy errorpage %code sequences and logformat %code sequences - const PrimaryCompiler &primaryCompiler; + SBuf output; ///< compilation result + const char *input; ///< template bytes that need to be compiled; never nil - /// handles context-dependent %code sequences + /// handles context-dependent %code sequences unsupported by ErrorState const PercentCodeCompiler *secondaryCompiler = nullptr; - SBuf output; ///< compilation result - const char *input; ///< template bytes that need to be compiled; never nil bool building_deny_info_url = false; ///< whether we compile deny_info URI bool allowRecursion = false; ///< whether top-level compile() calls are OK }; diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 77a818f8986..9e3250b0a15 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -530,13 +530,12 @@ Security::ErrorDetail::brief() const return os.buf(); } -// XXX: API contradiction: Caller supplies build.output but we return new output instead of using that SBuf -Security::ErrorDetail::verbose2(const ErrorPage::Build &build) const +Security::ErrorDetail::verbose(const ErrorTemplateCompiler &compiler) const { std::optional customFormat; #if USE_OPENSSL - if (const auto errorDetail = Ssl::ErrorDetailsManager::GetInstance().findDetail(error_no, &build.request)) { + if (const auto errorDetail = Ssl::ErrorDetailsManager::GetInstance().findDetail(error_no, compiler.request)) { detailEntry = *errorDetail; customFormat = detailEntry->detail; } else { @@ -544,10 +543,7 @@ Security::ErrorDetail::verbose2(const ErrorPage::Build &build) const } #endif auto format = customFormat ? customFormat->c_str() : "SSL handshake error (%err_name)"; - - auto myBuild = build.subtask(format, *this); - myBuild.compile(); // calls our compilePercentCode() as needed - return myBuild.output; + return compiler.compileDetail(format, this); // calls our compilePercentCode() as needed } /// textual representation of the subject of the broken certificate @@ -709,7 +705,7 @@ Security::ErrorDetail::printErrorDescription(std::ostream &os) const #if USE_OPENSSL if (detailEntry) { - os << detailEntry->descr; + os << detailEntry->descr; // quote for HTML? return; } #endif diff --git a/src/security/ErrorDetail.h b/src/security/ErrorDetail.h index bbff47c75dc..a690689a2e4 100644 --- a/src/security/ErrorDetail.h +++ b/src/security/ErrorDetail.h @@ -65,7 +65,7 @@ class ErrorDetail: public ::ErrorDetail, public ErrorPage::PercentCodeCompiler /* ErrorDetail API */ SBuf brief() const override; - SBuf verbose2(const Build &) const override; + SBuf verbose(const ErrorTemplateCompiler &) const override; /// \returns error category; \see ErrorCode ErrorCode errorNo() const { return error_no; } diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index ba4333b90af..093e2f26ee5 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1105,17 +1105,15 @@ Ftp::Server::writeErrorReply(const HttpReply *reply, const int scode) if (!request->error.details.empty()) { // Addressing TODO at the end of this method should remove this temporary hack. - // XXX: ErrorState currently requires non-constant HttpRequest, HttpReply. + // XXX: ErrorState currently requires non-constant HttpRequest and HttpReply. const auto err = std::make_unique( const_cast(request), const_cast(reply), pipeline.front()->http->al); for (const auto &detail: request->error.details) { - ErrorPage::Build build(*request, "", *err); - detail->verbose(build); mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief())); - mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(build.output)); + mb.appendf("%i-Error-Detail-Verbose: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->verbose(*err))); } } From 48a03a67e67048c1f4866dc86a98664f7a576b22 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 09:41:20 -0400 Subject: [PATCH 04/14] Generate src/error/categories.cc without wrong ErrorPage namespace ld: errorpage.o: warning: relocation against `err_type_str' in read-only section `.text' src/errorpage.cc:276: undefined reference to `err_type_str' src/errorpage.cc:631: undefined reference to `err_type_str' ... Branch changes added forward declarations for classes declared inside an ErrorPage namespace. The AWK script incorrectly applied that namespace to the generated err_type_str definition. This surgical fix does not cover all possible namespace-related variations. The correct fix is to stop parsing C++ using AWK, at least as far as namespaces are concerned: The script caller knows what namespace(s) must be used for the container definition. --- src/mk-string-arrays.awk | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/mk-string-arrays.awk b/src/mk-string-arrays.awk index c9695b1683a..6bc1fa9ccfd 100644 --- a/src/mk-string-arrays.awk +++ b/src/mk-string-arrays.awk @@ -25,8 +25,11 @@ BEGIN { nspath = "" } -# when namespace is encountered store it +# Remember the name of the last namespace encountered before the enum. +# XXX: We should remember the name(s) of the namespace(s) surrounding the enum +# instead. TODO: Replace this C++ parsing hack with a command-line parameter. /^namespace *[a-zA-Z]+/ { + if (type) next nspath = tolower($2) "/" # nested folder namespace = $2 # code namespace reconstruct next From b7c4596abc241c58684ffc16563410f148a4bdd8 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 10:06:12 -0400 Subject: [PATCH 05/14] fixup: Apply ErrorDetail::verbose() API changes ... to verbose() methods that did not use HttpRequest. --- src/clients/FtpClient.cc | 2 +- src/clients/FtpClient.h | 2 +- src/error/Detail.cc | 2 +- src/error/ExceptionErrorDetail.h | 2 +- src/error/SysErrorDetail.cc | 2 +- src/error/SysErrorDetail.h | 2 +- src/tests/stub_liberror.cc | 2 +- src/tests/stub_libsecurity.cc | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index c9db1ea7e61..6243a7071a8 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -78,7 +78,7 @@ Ftp::ErrorDetail::brief() const } SBuf -Ftp::ErrorDetail::verbose(const HttpRequest::Pointer &) const +Ftp::ErrorDetail::verbose(const ErrorTemplateCompiler &) const { return ToSBuf("FTP reply with completion code ", completionCode); } diff --git a/src/clients/FtpClient.h b/src/clients/FtpClient.h index 600247fb7d0..8f8ffc4e7c2 100644 --- a/src/clients/FtpClient.h +++ b/src/clients/FtpClient.h @@ -32,7 +32,7 @@ class ErrorDetail: public ::ErrorDetail { /* ErrorDetail API */ SBuf brief() const override; - SBuf verbose(const HttpRequestPointer &) const override; + SBuf verbose(const ErrorTemplateCompiler &) const override; private: int completionCode; ///< FTP reply completion code diff --git a/src/error/Detail.cc b/src/error/Detail.cc index 17e0879b3b2..1cd8fcbfdae 100644 --- a/src/error/Detail.cc +++ b/src/error/Detail.cc @@ -22,7 +22,7 @@ class NamedErrorDetail: public ErrorDetail /* ErrorDetail API */ SBuf brief() const override { return name; } - SBuf verbose(const HttpRequestPointer &) const override { return name; } + SBuf verbose(const ErrorTemplateCompiler &) const override { return name; } private: /// distinguishes us from all other NamedErrorDetail objects diff --git a/src/error/ExceptionErrorDetail.h b/src/error/ExceptionErrorDetail.h index eb105f59838..1763a8abb75 100644 --- a/src/error/ExceptionErrorDetail.h +++ b/src/error/ExceptionErrorDetail.h @@ -31,7 +31,7 @@ class ExceptionErrorDetail: public ErrorDetail return ToSBuf("exception=", asHex(exceptionId)); } - SBuf verbose(const HttpRequestPointer &) const override { + SBuf verbose(const ErrorTemplateCompiler &) const override { return ToSBuf("Exception (ID=", asHex(exceptionId), ')'); } diff --git a/src/error/SysErrorDetail.cc b/src/error/SysErrorDetail.cc index 260f521552a..a39fb85c0fe 100644 --- a/src/error/SysErrorDetail.cc +++ b/src/error/SysErrorDetail.cc @@ -24,7 +24,7 @@ SysErrorDetail::brief() const } SBuf -SysErrorDetail::verbose(const HttpRequestPointer &) const +SysErrorDetail::verbose(const ErrorTemplateCompiler &) const { return SBuf(xstrerr(errorNo)); } diff --git a/src/error/SysErrorDetail.h b/src/error/SysErrorDetail.h index 829b1bac601..3c94f572e21 100644 --- a/src/error/SysErrorDetail.h +++ b/src/error/SysErrorDetail.h @@ -30,7 +30,7 @@ class SysErrorDetail: public ErrorDetail /* ErrorDetail API */ SBuf brief() const override; - SBuf verbose(const HttpRequestPointer &) const override; + SBuf verbose(const ErrorTemplateCompiler &) const override; private: // hidden by NewIfAny() to avoid creating SysErrorDetail from zero errno diff --git a/src/tests/stub_liberror.cc b/src/tests/stub_liberror.cc index 2d7bd98bc0a..d2caf9204df 100644 --- a/src/tests/stub_liberror.cc +++ b/src/tests/stub_liberror.cc @@ -29,6 +29,6 @@ ErrorDetail::Pointer MakeNamedErrorDetail(const char *) STUB_RETVAL(ErrorDetail: #include "error/SysErrorDetail.h" SBuf SysErrorDetail::Brief(int) STUB_RETVAL(SBuf()) SBuf SysErrorDetail::brief() const STUB_RETVAL(SBuf()) -SBuf SysErrorDetail::verbose(const HttpRequestPointer &) const STUB_RETVAL(SBuf()) +SBuf SysErrorDetail::verbose(const ErrorTemplateCompiler &) const STUB_RETVAL(SBuf()) std::ostream &operator <<(std::ostream &os, ReportSysError) STUB_RETVAL(os) diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 86989daab7f..2b9ddd706f4 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -70,7 +70,7 @@ Security::ErrorDetail::ErrorDetail(ErrorCode, int, int) STUB Security::ErrorDetail::ErrorDetail(ErrorCode, LibErrorCode, int) STUB #endif void Security::ErrorDetail::setPeerCertificate(const CertPointer &) STUB -SBuf Security::ErrorDetail::verbose(const HttpRequestPointer &) const STUB_RETVAL(SBuf()) +SBuf Security::ErrorDetail::verbose(const ErrorTemplateCompiler &) const STUB_RETVAL(SBuf()) SBuf Security::ErrorDetail::brief() const STUB_RETVAL(SBuf()) Security::ErrorCode Security::ErrorCodeFromName(const char *) STUB_RETVAL(0) const char *Security::ErrorNameFromCode(ErrorCode, bool) STUB_RETVAL("") From 9e3285bb1d203dc2bbb1f2bf3a7f2cfec678d19d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 10:09:26 -0400 Subject: [PATCH 06/14] fixup: Fix "make check" --- src/tests/stub_libsecurity.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 2b9ddd706f4..cfd2eaee76e 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -72,6 +72,7 @@ Security::ErrorDetail::ErrorDetail(ErrorCode, LibErrorCode, int) STUB void Security::ErrorDetail::setPeerCertificate(const CertPointer &) STUB SBuf Security::ErrorDetail::verbose(const ErrorTemplateCompiler &) const STUB_RETVAL(SBuf()) SBuf Security::ErrorDetail::brief() const STUB_RETVAL(SBuf()) +bool Security::ErrorDetail::compilePercentCode(ErrorPage::Build &) const STUB_RETVAL(false) Security::ErrorCode Security::ErrorCodeFromName(const char *) STUB_RETVAL(0) const char *Security::ErrorNameFromCode(ErrorCode, bool) STUB_RETVAL("") From fda31c06e73cc7ea159de7238b9d94e99239ad7c Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 10:15:45 -0400 Subject: [PATCH 07/14] fixup: Apply compileLegacyCode() when there is no secondaryCompiler --- src/errorpage.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/errorpage.cc b/src/errorpage.cc index d120c12a9be..6dc1ad96fe8 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -1480,7 +1480,7 @@ ErrorState::compile(Build &build) const while (const auto letter = *build.input) { if (letter == '%') { build.output.append(blockStart, build.input - blockStart); - if (build.secondaryCompiler && !build.secondaryCompiler->compilePercentCode(build)) + if (!build.secondaryCompiler || !build.secondaryCompiler->compilePercentCode(build)) const_cast(this)->compileLegacyCode(build); blockStart = build.input; } From f81c9ad5e57e327cff170c3d44beec71caf2ee0a Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 11:02:41 -0400 Subject: [PATCH 08/14] fixup: Formatted modifies sources --- src/error/forward.h | 4 ++-- src/servers/FtpServer.cc | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/error/forward.h b/src/error/forward.h index 1741dff9a5d..0769a4d328e 100644 --- a/src/error/forward.h +++ b/src/error/forward.h @@ -93,8 +93,8 @@ class ErrorDetail; class ErrorState; namespace ErrorPage { - class PercentCodeCompiler; - class Build; +class PercentCodeCompiler; +class Build; } // namespace ErrorPage typedef RefCount ErrorDetailPointer; diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index 093e2f26ee5..f7d57d8b144 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -1107,9 +1107,9 @@ Ftp::Server::writeErrorReply(const HttpReply *reply, const int scode) // Addressing TODO at the end of this method should remove this temporary hack. // XXX: ErrorState currently requires non-constant HttpRequest and HttpReply. const auto err = std::make_unique( - const_cast(request), - const_cast(reply), - pipeline.front()->http->al); + const_cast(request), + const_cast(reply), + pipeline.front()->http->al); for (const auto &detail: request->error.details) { mb.appendf("%i-Error-Detail-Brief: " SQUIDSBUFPH "\r\n", scode, SQUIDSBUFPRINT(detail->brief())); From eacceb37f4df5241556d8055ed6520eb7ab3f475 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 11:18:36 -0400 Subject: [PATCH 09/14] fixup: This new code meets Assure() requirements --- src/errorpage.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/errorpage.cc b/src/errorpage.cc index 6dc1ad96fe8..352d83d515f 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -1471,7 +1471,7 @@ ErrorState::compileDetail(const char * const format, const ErrorPage::PercentCod void ErrorState::compile(Build &build) const { - assert(build.input); + Assure(build.input); // TODO: Instead of violating const-correctness with const_cast // below, adjust compile*() methods to avoid ErrorState modifications. From 96a7d12db485c344fd8b68e310c6c5aae55a481e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 11:19:23 -0400 Subject: [PATCH 10/14] fixup: Polished comments describing new code --- src/errorpage.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/errorpage.h b/src/errorpage.h index 336a77d8f9a..e306ae784ad 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -340,13 +340,13 @@ class TemplateFile namespace ErrorPage { -/// An API for recognizing and replaces a %code sequence. +/// An API for recognizing and replacing a single %code sequence. class PercentCodeCompiler: public Interface { public: - /// Either recognizes and replaces a single %code sequence at the beginning - /// of `build.input`, appending the replacement to `build.output`, OR does - /// not modify `build`. - /// \returns false when the leading %code was not recognized + /// Either recognizes a single %code sequence at the beginning of + /// `build.input` and appends its replacement to `build.output` OR does not + /// modify `build`. + /// \returns true when the leading %code was recognized /// \prec `build.input` starts with a `%` character. virtual bool compilePercentCode(Build &) const = 0; }; @@ -355,15 +355,16 @@ class PercentCodeCompiler: public Interface { /// PercentCodeCompiler::compilePercentCode() and ErrorState::compile*() methods /// that convert an errorpage template fragment (or equivalent) into an error /// response body fragment. This conversion replaces legacy errorpage %code -/// sequences, logformat %code sequences, and, in some cases, context-dependent -/// %code sequences. +/// sequences, logformat %code sequences, and, in some cases, +/// ErrorDetail-dependent %code sequences. class Build { public: SBuf output; ///< compilation result const char *input; ///< template bytes that need to be compiled; never nil - /// handles context-dependent %code sequences unsupported by ErrorState + /// Handles context-dependent %code sequences unsupported by ErrorState. + /// Compiler object lifetime must exceed this Build object lifetime. const PercentCodeCompiler *secondaryCompiler = nullptr; bool building_deny_info_url = false; ///< whether we compile deny_info URI From 7838ebc418a61f5726a34cd5dc3c12ccb700e371 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 11:20:53 -0400 Subject: [PATCH 11/14] fixup: Removed out-of-scope comment/to-do --- src/security/ErrorDetail.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index 9e3250b0a15..da6c935bda3 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -705,7 +705,7 @@ Security::ErrorDetail::printErrorDescription(std::ostream &os) const #if USE_OPENSSL if (detailEntry) { - os << detailEntry->descr; // quote for HTML? + os << detailEntry->descr; return; } #endif From 679ac7914cffcc4e6d9d0300c7bd30a023dc0810 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 11:43:39 -0400 Subject: [PATCH 12/14] fixup: Removed an old TODO effectively addressed by this branch --- src/security/ErrorDetail.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/security/ErrorDetail.cc b/src/security/ErrorDetail.cc index da6c935bda3..4a67763bb77 100644 --- a/src/security/ErrorDetail.cc +++ b/src/security/ErrorDetail.cc @@ -787,7 +787,6 @@ Security::ErrorDetail::convertErrorCodeToDescription(const char * const code, st } } - // TODO: Support logformat %codes. return 0; } From 95408d6b2b601b2ff293e95df4e89704b52f4599 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 13:34:17 -0400 Subject: [PATCH 13/14] fixup: Improve buildtests/layer-01-minimal build --- src/errorpage.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/errorpage.h b/src/errorpage.h index e306ae784ad..a20e9e8cacd 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -12,6 +12,7 @@ #define SQUID_SRC_ERRORPAGE_H #include "cbdata.h" +#include "base/TypeTraits.h" #include "comm/forward.h" #include "error/Detail.h" #include "error/forward.h" From d05744ab0bbd2d60105dc1fb97a990e22272861e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 28 Apr 2026 13:56:27 -0400 Subject: [PATCH 14/14] fixup: Fix include order broken by previous fixup --- src/errorpage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/errorpage.h b/src/errorpage.h index a20e9e8cacd..90983c82db9 100644 --- a/src/errorpage.h +++ b/src/errorpage.h @@ -11,8 +11,8 @@ #ifndef SQUID_SRC_ERRORPAGE_H #define SQUID_SRC_ERRORPAGE_H -#include "cbdata.h" #include "base/TypeTraits.h" +#include "cbdata.h" #include "comm/forward.h" #include "error/Detail.h" #include "error/forward.h"