Copilot commented on code in PR #12947:
URL: https://github.com/apache/trafficserver/pull/12947#discussion_r2906200112
##########
src/proxy/http/HttpBodyFactory.cc:
##########
@@ -181,7 +182,8 @@ HttpBodyFactory::fabricate_with_old_api(const char *type,
HttpTransact::State *c
if (buffer) { // got an instantiated template
if (!plain_flag) {
snprintf(content_language_out_buf, content_language_buf_size, "%s",
lang_ptr);
- snprintf(content_type_out_buf, content_type_buf_size, "text/html;
charset=%s", charset_ptr);
+ const char *mime_type = content_type_ptr ? content_type_ptr :
"text/html";
+ snprintf(content_type_out_buf, content_type_buf_size, "%s; charset=%s",
mime_type, charset_ptr);
Review Comment:
`Content-Type` from `.body_factory_info` is appended with `; charset=...`
unconditionally. If an operator configures `Content-Type` with parameters (e.g.
`application/json; charset=utf-16`), this will generate an invalid header with
duplicate charset parameters. Consider either (a) treating
`.body_factory_info`'s `Content-Type` as the full header value and not
appending charset, or (b) parsing/stripping parameters (or at least detecting
an existing `charset=`) before appending.
##########
doc/admin-guide/monitoring/error-messages.en.rst:
##########
@@ -108,6 +108,45 @@ it would be used instead of ``cache#read_error`` if there
is no ``apache_cache#r
The text for an error message is processed as if it were a
:ref:`admin-logging-fields` which
enables customization by values present in the transaction for which the error
occurred.
+.. _body-factory-info:
+
+Template Set Metadata
+---------------------
+
+Each template set directory must contain a ``.body_factory_info`` file for the
template set to be
+loaded. This file controls the ``Content-Type``, ``Content-Language``, and
character set of the
+HTTP response headers sent with error pages.
+
+The following directives are supported:
+
+``Content-Language``
+ The natural language of the error pages. This value is sent in the
``Content-Language`` HTTP
+ response header. Default: ``en``.
+
+``Content-Charset``
+ The character encoding of the error pages. This value is appended to the
``Content-Type`` header
+ as a ``charset`` parameter. Default: ``utf-8``.
+
+``Content-Type``
+ The MIME type for the error response. This controls the media type portion
of the ``Content-Type``
+ HTTP response header. Default: ``text/html``.
+
+For example, to serve plain text error pages in English::
+
+ Content-Language: en
+ Content-Charset: utf-8
+ Content-Type: text/plain
+
+This would produce the response header ``Content-Type: text/plain;
charset=utf-8``.
+
+To describe Korean error pages encoded in the ``iso-2022-kr`` character set::
+
+ Content-Language: kr
Review Comment:
`Content-Language: kr` is not a standard language tag for Korean (commonly
`ko` / `ko-KR` per BCP 47). Since this section is newly documenting metadata
directives, consider updating the example to use a standard tag to avoid
propagating incorrect configuration patterns.
```suggestion
Content-Language: ko-KR
```
##########
tests/gold_tests/body_factory/body_factory_content_type.test.py:
##########
@@ -0,0 +1,104 @@
+'''
+Tests that the Content-Type directive in .body_factory_info is honored
+for body factory error responses.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+
+Test.Summary = 'Verify Content-Type directive in .body_factory_info controls
error response MIME type'
+Test.ContinueOnFail = True
+
+
+class BodyFactoryContentTypeTest:
+ """
+ Test that the Content-Type directive in .body_factory_info is used for
+ body factory error responses instead of the hardcoded text/html default.
+
+ Two scenarios:
+ 1. Default: no Content-Type directive -> text/html; charset=utf-8
+ 2. Custom: Content-Type: text/plain -> text/plain; charset=utf-8
+ """
+
+ def __init__(self):
+ self._setupDefaultTS()
+ self._setupCustomTS()
+
+ def _setupDefaultTS(self):
+ """ATS instance with default body factory (no Content-Type
directive)."""
+ self._ts_default = Test.MakeATSProcess("ts_default")
+ self._ts_default.Disk.records_config.update(
+ {
+ 'proxy.config.body_factory.enable_customizations': 1,
+ 'proxy.config.url_remap.remap_required': 1,
+ })
+ self._ts_default.Disk.remap_config.AddLine('map
http://mapped.example.com http://127.0.0.1:65535')
+
+ body_factory_dir = self._ts_default.Variables.BODY_FACTORY_TEMPLATE_DIR
+ info_path = os.path.join(body_factory_dir, 'default',
'.body_factory_info')
+ self._ts_default.Disk.File(info_path).WriteOn("Content-Language:
en\nContent-Charset: utf-8\n")
+
+ def _setupCustomTS(self):
+ """ATS instance with Content-Type: text/plain in .body_factory_info."""
+ self._ts_custom = Test.MakeATSProcess("ts_custom")
+ self._ts_custom.Disk.records_config.update(
+ {
+ 'proxy.config.body_factory.enable_customizations': 1,
+ 'proxy.config.url_remap.remap_required': 1,
+ })
+ self._ts_custom.Disk.remap_config.AddLine('map
http://mapped.example.com http://127.0.0.1:65535')
+
+ body_factory_dir = self._ts_custom.Variables.BODY_FACTORY_TEMPLATE_DIR
+ info_path = os.path.join(body_factory_dir, 'default',
'.body_factory_info')
+ self._ts_custom.Disk.File(info_path).WriteOn("Content-Type:
text/plain\n")
+
+ def run(self):
+ self._testDefaultContentType()
+ self._testCustomContentType()
+
+ def _testDefaultContentType(self):
+ """Without Content-Type directive, error responses should use
text/html."""
+ tr = Test.AddTestRun('Default body factory Content-Type is text/html')
+ tr.Processes.Default.StartBefore(self._ts_default)
+ tr.Processes.Default.Command = (
+ f'curl -s -D- -o /dev/null'
+ f' -H "Host: unmapped.example.com"'
+ f' http://127.0.0.1:{self._ts_default.Variables.port}/')
+ tr.Processes.Default.ReturnCode = 0
+ tr.Processes.Default.TimeOut = 5
+ tr.Processes.Default.Streams.stdout += Testers.ContainsExpression(
+ 'Content-Type: text/html; charset=utf-8', 'Default body factory
should produce text/html with charset')
Review Comment:
This assertion is potentially brittle if the server emits a different
charset casing (e.g. `UTF-8`) or slightly different formatting/spacing. If
`ContainsExpression` is regex-based, consider using a case-insensitive pattern
and/or a more flexible match for the charset portion to reduce test flakiness
across platforms/builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
