Conversation
Before: A placeholder in `host`/`hosts` was expanded and passed to the connection options as is. So a tag or a record field could add a host with ",", userinfo with "@" or a path with "/". The configured user, password and custom_headers were then sent to that host. After: Each placeholder value is checked before the whole setting is expanded, so a value can only fill in a host name and a port. The rest of the setting, such as the "," between hosts or a scheme, userinfo and path the operator wrote, is left as it is. A placeholder used together with credentials logs a warning at startup. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
kenhys
marked this pull request as ready for review
September 17, 2026 01:35
Watson1978
requested changes
Sep 17, 2026
| # without this a log sender could add a host with ",", userinfo with "@" or a path with "/", | ||
| # and receive the configured user/password and custom_headers. A port is | ||
| # allowed because it can only be chosen on a host the value already picked. | ||
| HOST_PLACEHOLDER_VALUE_PATTERN = /\A[0-9A-Za-z_\-.]*(:[0-9]+)?\z/ |
Contributor
There was a problem hiding this comment.
This plugin supports IPv6 address https://github.com/fluent/fluent-plugin-opensearch#ipv6-addresses
However, it seems this regexp does not support ipv6.
Contributor
There was a problem hiding this comment.
Other cases.
-
Domain escape
.is allowed without limit, so a value can still move the request to another domain. Withhost myhost-${pipeline_id}plususer/password, a record field ofevil.example.netpasses this check andget_connection_optionsreturnshost: "myhost-evil.example.net"with the configured credentials attached.custom_headersfollows the same path, so the leak this PR describes is still open for a template with no fixed suffix.
-
:port accepted anywhere
(:[0-9]+)?is accepted wherever the placeholder sits, but a port can only follow the end of a host.hosts https://logs-${pipeline_id}.example.com/oswith the value1:9999passes and becomeshttps://logs-1:9999.example.com/os, whichURI()rejects.connection_options_descriptionin therescueofsend_bulkthen raises the sameURI::InvalidURIError, soignore_exceptionsandexception_backupnever apply.
-
Empty value
*lets an empty value pass. Withhosts ${pipeline_id}and an empty value,get_connection_options("")returns{hosts: []}, so the destination is silently gone instead of rejected.+would reject that, and also:9999, which currently reachesURI()and raises.
Before: a value could hold a host name and a port wherever the
placeholder was.
* `host myhost-${pipeline_id}` let a record field move the request to
another domain
* `hosts https://logs-${pipeline_id}.example.com/os` let a value put a
port in the middle of a name, which `URI()` cannot read
* An empty value dropped the host without an error
* An IPv6 address was rejected everywhere
After: what a value may hold depends on where the operator put the
placeholder.
* A value that continues a name the operator wrote fills in one label,
so it can add neither a "." nor a port
* A value that stands for the whole host name may hold a name and a port
* An IPv6 address is allowed where the operator wrote the "[]" or the
scheme that `URI()` needs to read it
* An empty value is rejected everywhere
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
Watson1978
reviewed
Sep 18, 2026
| # could add a host with ",", userinfo with "@" or a path with "/", and | ||
| # the configured user/password and custom_headers would go to that host. | ||
| HOST_PLACEHOLDER_PATTERN = /\$\{[^}]*\}/ | ||
| HOST_LABEL_PATTERN = /\A[0-9A-Za-z_\-]+\z/ |
Contributor
There was a problem hiding this comment.
Seems that host logs-${tag}.google.com is rejected once the tag contains a dot.
host logs-${tag}.google.com- tag
"test"-> ACCEPT with "logs-test.google.com" - tag
"test.template"-> REJECT
- tag
So, it needs to contain \. like HOST_NAME_PATTERN
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before: A placeholder in
host/hostswas expanded and passed to the connection options as is. So a tag or a record field could add a host with ",", userinfo with "@" or a path with "/". The configured user, password and custom_headers were then sent to that host.After: Each placeholder value is checked before the whole setting is expanded, so a value can only fill in a host name and a port. The rest of the setting, such as the "," between hosts or a scheme, userinfo and path the operator wrote, is left as it is. A placeholder used together with credentials logs a warning at startup.