Skip to content
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.

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

if the device custom protocol contains '&','<','>'characters ,the devsdk_add_device_callback,devsdk_update_device_callback,media_server_remove_device parses protocol incorrectly #465

Closed
yeehecan opened this issue Jul 4, 2023 · 11 comments
Labels
bug Something isn't working
Projects

Comments

@yeehecan
Copy link

yeehecan commented Jul 4, 2023

Hello there! 😄

🐞 Bug Report

The issue is located in devsdk_add_device_callback,devsdk_update_device_callback,devsdk_emove_device function.These callback functions are triggered normally。
but when using edgex-ui-go add/update/delete device from the metadata service ,
if the device protocol value contains & < > ,
then the protocols param of devsdk_add_device_callback,devsdk_update_device_callback,devsdk_emove_device function
got the Incorrect escape character.
\026 for &
\036 for >
\034 for <
it shoud be
\x26 or \046 for &
\x3e or \076 for >
\x3c or \074 for <

1
2

Environment
ubuntu22.04,amd64

**EdgeX Version [REQUIRED]: 3.0.0

device-sdk-c version:3.0.0

i subscribe to the MessageBus. the Redis pub/sub
the & < > character in the message payload is formatted as \u0026 ,\u003c,\u003e

Currently ,
i roughly replace \026 \036 \034 with \x26 \x3e \x3c to solve the problem.

@yeehecan yeehecan added the bug Something isn't working label Jul 4, 2023
@yeehecan yeehecan changed the title if the device custom protocol contains '&','<','>'characters ,the devsdk_add_device_callback,devsdk_update_device_callback,media_server_remove_device parses incorrectly if the device custom protocol contains '&','<','>'characters ,the devsdk_add_device_callback,devsdk_update_device_callback,media_server_remove_device parses protocol incorrectly Jul 4, 2023
@FelixTing FelixTing added this to New Issues in Device WG via automation Aug 30, 2023
@FelixTing FelixTing self-assigned this Aug 30, 2023
@FelixTing FelixTing moved this from New Issues to In progress in Device WG Aug 30, 2023
@FelixTing
Copy link
Member

FelixTing commented Sep 5, 2023

The JSON data sent from core-metadata to the Device Service callback function enforces the conversion of string values to valid UTF-8, replacing "<", ">", "&", U+2028, and U+2029 with Unicode escape sequences because they can lead to security holes.
However, the JSON parser in iotech-c-utils used by the C Device SDK is unable to accurately parse Unicode escape sequences. This will result in the C Device Service being unable to correctly read these characters.

For now, the workaround is to perform URL encoding on the mentioned characters before using them in device profile or device configuration and decode them in the device driver.

@FelixTing
Copy link
Member

@lenny-intel @jpwhitemn
I would like to make a revision to my proposal. The modification might be made in core-metadata publishSystemEvent(https://github.com/edgexfoundry/edgex-go/blob/2b0aa32e305f4468c05d36fa7488a318f523b7ee/internal/core/metadata/application/notify.go#L144), not in go-mod-messaging.

The issue arises from the use of json.Marshal, which performs HTML escaping on string values. See https://pkg.go.dev/encoding/json#Marshal.

String values encode as JSON strings coerced to valid UTF-8, replacing invalid bytes with the Unicode replacement rune. So that the JSON will be safe to embed inside HTML <script> tags, the string is encoded using HTMLEscape, which replaces "<", ">", "&", U+2028, and U+2029 are escaped to "\u003c","\u003e", "\u0026", "\u2028", and "\u2029". This replacement can be disabled when using an Encoder, by calling SetEscapeHTML(false).

However, in the context of EdgeX, when system event data is received from the message bus, it is decoded using json.Unmarshal (for those Go services), which includes restoring those escape sequences. In this case, HTML escaping appears to be unnecessary. According to Golang standard library documentation, we can marshal system events to JSON without escaping certain characters. For example:

payload := new(bytes.Buffer)
encoder := json.NewEncoder(payload)
encoder.SetEscapeHTML(false)
_ = encoder.Encode(systemEvent)
envelope := types.NewMessageEnvelope(payload.Bytes(), ctx)

Another solution is to enhance the JSON parser in iotech-c-utils (https://github.com/IOTechSystems/iotech-c-utils/blob/master/src/c/data.c#L2657-L2664) to handle the Unicode replacement rune properly.

@cloudxxx8 for awareness

@cloudxxx8
Copy link
Member

it's a kind of breaking change in System Event, so we will not make this change in core-metadata.
before iotech-c-utils handles the Unicode replacement rune, we should list this as a known issue.
https://wiki.edgexfoundry.org/display/FA/Minnesota

Please also mention the URL encoding workaround for reference.
https://www.w3schools.com/tags/ref_urlencode.ASP

@FelixTing
Copy link
Member

Added this as a known bug on https://wiki.edgexfoundry.org/display/FA/Minnesota

@cloudxxx8 cloudxxx8 moved this from In progress to Icebox in Device WG Oct 6, 2023
@cloudxxx8
Copy link
Member

thanks, @FelixTing , moving this issue to icebox

@FelixTing FelixTing removed their assignment Oct 6, 2023
@iain-anderson
Copy link
Member

Created issue in the utils : IOTechSystems/iotech-c-utils#314

@FelixTing
Copy link
Member

This issue has been resolved in iotech-c-utils by @iain-anderson.
@yeehecan Could you please use the latest https://github.com/IOTechSystems/iotech-c-utils/tree/v1.5-branch and try again?

@lenny-goodell
Copy link
Member

@FelixTing , @cloudxxx8 , is this issue now resolved for 3.1 Napa release?

@FelixTing
Copy link
Member

@lenny-intel Yes, this issue is now resolved for the Napa release.

@cloudxxx8
Copy link
Member

fixed by IOTechSystems/iotech-c-utils#315

@FelixTing
Copy link
Member

This issue has been resolved in iotech-c-utils by @iain-anderson. @yeehecan Could you please use the latest https://github.com/IOTechSystems/iotech-c-utils/tree/v1.5-branch and try again?

Closed due to no response.

Device WG automation moved this from Icebox to Done Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Device WG
  
Done
Development

No branches or pull requests

5 participants