Copilot commented on code in PR #13390:
URL: https://github.com/apache/trafficserver/pull/13390#discussion_r3590312897
##########
plugins/webp_transform/ImageTransform.cc:
##########
@@ -105,8 +125,20 @@ class ImageTransform : public TransformationPlugin
handleInputComplete() override
{
std::string input_data = _img.str();
- Blob input_blob(input_data.data(), input_data.length());
- Image image;
+
+ if (!has_signature_for(input_data, _input_image_type)) {
+ TSError("[webp_transform] input body does not match its declared image
encoding: %d, length: %zu",
+ static_cast<int>(_input_image_type), input_data.length());
+ if (!input_data.empty()) {
+ produce(input_data);
+ }
+ _transform_image_type = _input_image_type;
+ setOutputComplete();
Review Comment:
In the signature-mismatch path, `_transform_image_type` is reverted to
`_input_image_type`, but the `Content-Type` header was already rewritten in
`handleReadResponseHeaders()` based on the original `_transform_image_type`.
Since updating `_transform_image_type` here does not update the already-mutated
headers, clients can still receive (for example) `Content-Type: image/jpeg`
while the body is passed through as WebP/invalid data, which contradicts the
“pass through unchanged” behavior.
Consider deferring `Content-Type` mutation until it’s guaranteed the
transform will be applied (e.g., set it in a later hook such as
`HOOK_SEND_RESPONSE_HEADERS` based on the final decision), or otherwise
ensuring the header is reverted when bypassing the transform. Adding replay
assertions for the `Content-Type` on invalid/empty inputs will help prevent
regressions.
##########
tests/gold_tests/pluginTest/webp_transform/webp_transform_invalid_input.replay.yaml:
##########
@@ -0,0 +1,186 @@
+# 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.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Verify webp_transform safely passes through invalid image
bodies'
+
+ server:
+ name: 'webp-transform-server'
+
+ client:
+ name: 'webp-transform-client'
+
+ ats:
+ name: 'webp-transform-ts'
+ process_config:
+ enable_cache: false
+ disable_log_checks: true
+
+ plugin_config:
+ - 'webp_transform.so convert_to_jpeg,convert_to_webp'
+
+ records_config:
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: 'webp_transform'
+
+ remap_config:
+ - from: 'http://www.example.com/'
+ to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/'
+
+ log_validation:
+ diags_log:
+ contains:
+ - expression: 'input body does not match its declared image encoding'
+ description: 'Verify invalid input is rejected before conversion'
+ excludes:
+ - expression: 'zero-length blob not permitted|no decode delegate for
this image format'
+ description: 'Verify ImageMagick never receives the invalid input'
+
+sessions:
+ - transactions:
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /empty.webp
+ headers:
+ fields:
+ - [Host, www.example.com]
+ - [Accept, 'image/jpeg']
+ - [uuid, empty-webp]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Type, image/webp]
+ - [Content-Length, 0]
+ content:
+ size: 0
+
+ proxy-response:
+ status: 200
+ content:
+ size: 0
+
Review Comment:
This transaction verifies the empty body is passed through, but it does not
assert that the proxy preserves the original `Content-Type` (image/webp). Since
the PR goal is “pass through unchanged” for empty/mislabeled bodies, the replay
should validate headers as well as the body.
##########
tests/gold_tests/pluginTest/webp_transform/webp_transform_invalid_input.replay.yaml:
##########
@@ -0,0 +1,186 @@
+# 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.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Verify webp_transform safely passes through invalid image
bodies'
+
+ server:
+ name: 'webp-transform-server'
+
+ client:
+ name: 'webp-transform-client'
+
+ ats:
+ name: 'webp-transform-ts'
+ process_config:
+ enable_cache: false
+ disable_log_checks: true
+
+ plugin_config:
+ - 'webp_transform.so convert_to_jpeg,convert_to_webp'
+
+ records_config:
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: 'webp_transform'
+
+ remap_config:
+ - from: 'http://www.example.com/'
+ to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/'
+
+ log_validation:
+ diags_log:
+ contains:
+ - expression: 'input body does not match its declared image encoding'
+ description: 'Verify invalid input is rejected before conversion'
+ excludes:
+ - expression: 'zero-length blob not permitted|no decode delegate for
this image format'
+ description: 'Verify ImageMagick never receives the invalid input'
+
+sessions:
+ - transactions:
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /empty.webp
+ headers:
+ fields:
+ - [Host, www.example.com]
+ - [Accept, 'image/jpeg']
+ - [uuid, empty-webp]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Type, image/webp]
+ - [Content-Length, 0]
+ content:
+ size: 0
+
+ proxy-response:
+ status: 200
+ content:
+ size: 0
+
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /invalid.webp
+ headers:
+ fields:
+ - [Host, www.example.com]
+ - [Accept, 'image/jpeg']
+ - [uuid, invalid-webp]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Type, image/webp]
+ - [Content-Length, 1]
+ content:
+ data: x
+
+ proxy-response:
+ status: 200
+ content:
+ verify: {value: x, as: equal}
+
Review Comment:
This transaction verifies the invalid WebP body is passed through, but it
does not assert that the proxy preserves the original `Content-Type`
(image/webp). Without this, the test won’t catch cases where the body is passed
through but headers still indicate a converted encoding.
##########
tests/gold_tests/pluginTest/webp_transform/webp_transform_invalid_input.replay.yaml:
##########
@@ -0,0 +1,186 @@
+# 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.
+
+meta:
+ version: "1.0"
+
+autest:
+ description: 'Verify webp_transform safely passes through invalid image
bodies'
+
+ server:
+ name: 'webp-transform-server'
+
+ client:
+ name: 'webp-transform-client'
+
+ ats:
+ name: 'webp-transform-ts'
+ process_config:
+ enable_cache: false
+ disable_log_checks: true
+
+ plugin_config:
+ - 'webp_transform.so convert_to_jpeg,convert_to_webp'
+
+ records_config:
+ proxy.config.diags.debug.enabled: 1
+ proxy.config.diags.debug.tags: 'webp_transform'
+
+ remap_config:
+ - from: 'http://www.example.com/'
+ to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/'
+
+ log_validation:
+ diags_log:
+ contains:
+ - expression: 'input body does not match its declared image encoding'
+ description: 'Verify invalid input is rejected before conversion'
+ excludes:
+ - expression: 'zero-length blob not permitted|no decode delegate for
this image format'
+ description: 'Verify ImageMagick never receives the invalid input'
+
+sessions:
+ - transactions:
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /empty.webp
+ headers:
+ fields:
+ - [Host, www.example.com]
+ - [Accept, 'image/jpeg']
+ - [uuid, empty-webp]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Type, image/webp]
+ - [Content-Length, 0]
+ content:
+ size: 0
+
+ proxy-response:
+ status: 200
+ content:
+ size: 0
+
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /invalid.webp
+ headers:
+ fields:
+ - [Host, www.example.com]
+ - [Accept, 'image/jpeg']
+ - [uuid, invalid-webp]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Type, image/webp]
+ - [Content-Length, 1]
+ content:
+ data: x
+
+ proxy-response:
+ status: 200
+ content:
+ verify: {value: x, as: equal}
+
+ - client-request:
+ method: GET
+ version: '1.1'
+ url: /invalid.jpg
+ headers:
+ fields:
+ - [Host, www.example.com]
+ - [Accept, 'image/webp']
+ - [uuid, invalid-jpeg]
+
+ server-response:
+ status: 200
+ reason: OK
+ headers:
+ fields:
+ - [Content-Type, image/jpeg]
+ - [Content-Length, 8]
+ content:
+ data: not-jpeg
+
+ proxy-response:
+ status: 200
+ content:
+ verify: {value: not-jpeg, as: equal}
+
Review Comment:
This transaction verifies the invalid JPEG body is passed through, but it
does not assert that the proxy preserves the original `Content-Type`
(image/jpeg). Adding a header assertion makes sure the pass-through behavior is
truly “unchanged” from the client’s perspective.
--
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]
