nextcloud / nextcloud/notify_push

trusted proxies validation is invalid and allows nonsense values

Open
#544 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Rust
Stars
344
Forks
64
Avg merge
2d 10h
Merged PRs (30d)
5

Description

How to use GitHub
  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Steps to reproduce
  1. add trusted_proxies with IPv6 syntax
  2. create invalid notify_push config e.g. wrong trusted_proxies or missing headers
  3. perform either occ notify_push:setup ... or occ notify_push:self-test
  4. a warning about as invalid IPv6 proxy is shown
Expected behaviour

according to docs https://docs.nextcloud.com/server/stable/admin_manual/configuration_server/reverse_proxy_configuration.html reverse proxy could be IPv4 addresses and ranges in CIDR notation, IPv6 addresses and ranges in CIDR notation. the check should reflect this fact.

  • IPv6 syntax should be valid
  • IPv4 CIDR syntax should be verified properly (current regex allows nonsense values like 333.444.555.666/77)
Actual behaviour
🗴 push server is not a trusted proxy by Nextcloud or another proxy in the chain.
  Nextcloud resolved the following client address for the test request: "Invalid response when testing if the push server is a trusted proxy: invalid IP address syntax" instead of the expected "1.2.3.4" test value.
  The following trusted proxies are currently configured: "172.16.0.0/12", "192.168.0.0/16", "10.0.0.0/8", "fc00::/7", "fe80::/10", "2001:db8::/32"
    of which the following seem to be invalid: "fc00::/7", "fe80::/10", "2001:db8::/32"
  The following x-forwarded-for header was received by Nextcloud: ""
    from the following remote:

the problem results from $cidrre = '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/'; regex used in SelfTest.php which doesn't match IPv6 (and is not strict enough to match only valid IPv4 syntax)

https://github.com/nextcloud/notify_push/blob/f71fcbc1a0b348d7b4417bb65cb624bd79eb22bc/lib/SelfTest.php#L240C1-L248C3

private function isValidProxyConfig(string $proxyConfig): bool {
	$cidrre = '/^([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3})\/([0-9]{1,2})$/';

	if (filter_var($proxyConfig, FILTER_VALIDATE_IP) !== false) {
		return true;
	} else {
		return (bool)preg_match($cidrre, $proxyConfig);
	}
}

there are better regex like https://uibakery.io/regex-library/ip-address-regex-php or better check functions like https://gist.github.com/pavinjosdev/cb1d636ea9dc2bd201d54107d10650c5

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

The validation is in lib/SelfTest.php, in isValidProxyConfig; start there and reproduce the issue with occ notify_push:setup or occ notify_push:self-test using the trusted proxy examples. Done means valid IPv6 and IPv4 CIDR values are accepted while malformed IPv4 addresses and prefixes are rejected.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.