Skip to content

Allow subpath in ntfy host configuration#29319

Draft
sschritt wants to merge 1 commit intoevcc-io:masterfrom
sschritt:master
Draft

Allow subpath in ntfy host configuration#29319
sschritt wants to merge 1 commit intoevcc-io:masterfrom
sschritt:master

Conversation

@sschritt
Copy link
Copy Markdown
Contributor

Fix #29010

The ntfy host parameter inherited the global default validation pattern, which rejects forward slashes, making it impossible to configure hosts with a subpath (e.g. myserver.com/ntfy). This PR adds a custom validation pattern to fix this.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • The new host validation regex does not support IPv6 literals (e.g. https://[2001:db8::1]/ntfy), which ntfy can handle; consider extending the pattern or documenting that IPv6 hosts are unsupported here.
  • Since the regex allows a path with a port (e.g. myserver.com:8080/ntfy), consider adding such a case to the pattern.examples list to clarify that this format is intentionally supported.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The new host validation regex does not support IPv6 literals (e.g. `https://[2001:db8::1]/ntfy`), which ntfy can handle; consider extending the pattern or documenting that IPv6 hosts are unsupported here.
- Since the regex allows a path with a port (e.g. `myserver.com:8080/ntfy`), consider adding such a case to the `pattern.examples` list to clarify that this format is intentionally supported.

## Individual Comments

### Comment 1
<location path="templates/definition/messenger/ntfy.yaml" line_range="9-14" />
<code_context>
+    description:
+      de: IP-Adresse oder Hostname (mit optionalem Pfad)
+      en: IP address or hostname (with optional path)
+    pattern:
+      regex: "^[^\\/\\s]+(:[0-9]{1,5})?(\\/\\S*)?$"
+      examples: ["ntfy.sh", "192.168.1.100", "myserver.com:8080", "myserver.com/ntfy"]
   - name: topics
</code_context>
<issue_to_address>
**suggestion:** Current host regex does not support IPv6 literal addresses or bracketed forms.

This pattern works for hostnames and IPv4, but rejects valid IPv6 literals such as `[2001:db8::1]` and `[2001:db8::1]:8080`. If IPv6 addresses should be accepted, update the regex or add a separate validation path that handles bracketed IPv6 with optional ports and paths.

```suggestion
    description:
      de: IP-Adresse, Hostname oder IPv6-Literal (mit optionalem Pfad)
      en: IP address, hostname or IPv6 literal (with optional path)
    pattern:
      regex: "^(?:\\[[0-9a-fA-F:]+](?::[0-9]{1,5})?|[^\\/\\s]+(?::[0-9]{1,5})?)(?:\\/\\S*)?$"
      examples: ["ntfy.sh", "192.168.1.100", "myserver.com:8080", "myserver.com/ntfy", "[2001:db8::1]", "[2001:db8::1]:8080", "[2001:db8::1]/ntfy"]
```
</issue_to_address>

### Comment 2
<location path="templates/definition/messenger/ntfy.yaml" line_range="66" />
<code_context>
 render: |
   type: ntfy
-  uri: https://{{ .host }}/{{ .topics | join "," }}
+  uri: https://{{ .host | trimSuffix "/" }}/{{ .topics | join "," }}
   priority: {{ .priority }}
   tags: {{ .tags | join "," }}
</code_context>
<issue_to_address>
**suggestion:** Use a more robust approach if multiple trailing slashes on `host` should be normalized.

`trimSuffix "/"` removes only one trailing slash, so `"ntfy.sh///"` becomes `"ntfy.sh//"`. If you need full normalization, use a helper (or chain calls) that strips all trailing `/` characters before concatenation.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread templates/definition/messenger/ntfy.yaml
Comment thread templates/definition/messenger/ntfy.yaml
@andig
Copy link
Copy Markdown
Member

andig commented Apr 23, 2026

I don't think this is a good way to fix it. The issue of allowing path is common to any http integration using host instead of uri. That said: we should either fix it in the general case or leave as-is. Note that #29340 will allow you to configure any device directly instead of using templates (same as you can in yaml) so this might already be suitable workaround.

/cc @naltatis

@andig andig marked this pull request as draft April 23, 2026 12:09
@naltatis
Copy link
Copy Markdown
Member

don't think this is a good way to fix it.

Agreed.

Why not simply add an advanced path parameter?

@andig
Copy link
Copy Markdown
Member

andig commented Apr 23, 2026

Grrrh :/. Make it an uri?

@naltatis
Copy link
Copy Markdown
Member

Uri might be tricky since it strictly also may include query params which would break the topics append (/{{ .topics | join "," }}).
However, we dont need strict guards here, so this might be fine.

@andig whats the argument agains path? I think it would be cleaner than deprecating host and introducing uri (backwards compatibility).

@sschritt
Copy link
Copy Markdown
Contributor Author

Makes sense. Fixing this only for ntfy would be inconsistent if the same issue exists across all HTTP integrations using host.

@andig
Copy link
Copy Markdown
Member

andig commented Apr 27, 2026

whats the argument agains path?

We'd need that for every single integration using HTTP. Bummer. The root cause of this issue is that we've tightened host to just that while still using host for http.

Either we:

  • relax host (not good)
  • use uri everywhere http is in place (may have query)
  • add an already-relaxed httphost (why would you)
  • add path (cumbersome)

Choose one, fix everywhere.

@naltatis
Copy link
Copy Markdown
Member

We'd need that for every single integration using HTTP.

Why is that? Dont see why most other templates (go-e, shelly, ...) would even need a user defined path section.

The requirement in #29010 is only there because of self hosting. This is not applicable for most of our integrations.

@andig
Copy link
Copy Markdown
Member

andig commented Apr 27, 2026

Good point. We've definitely seen this pattern before though. I don't have a clear view which services require this then and which don't. Anyway, with #29340 merged you can already do that through a custom device. Unless we have a clear global story how to fix this I'd rather not do it at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Konfiguration von NTFY nicht möglich

3 participants