feat: Allow multiple options in request_ip_source #1985

Merged
Aranjedeath merged 2 commits from Omar007/continuwuity:feature/ip-src-fallback into main 2026-07-21 14:35:55 +00:00
Contributor

The current request_ip_source setting only allows a single option.
While it does fall back to the peer IP if the header is missing as of !2003,
which likely covers a lot of regular use, only allowing a single option limits
the deployment options available to more advanced deployments.
Setups where internal and external traffic use different reverse proxies will
end up with the wrong IP and the implicitness of the fallback allows for
situations where the used IP is not the IP expected.

By introducing a setting that allows multiple options to be set,
this limitation is resolved and it becomes possible to have client IP resolution
behind different reverse proxies and also making it possible to decide if and/or
what the fallback should be.

If set, options are evaluated in order. If all fail, the request fails.

Pull request checklist:

  • This pull request targets the main branch, and the branch is named something other than
    main.
  • I have written an appropriate pull request title and my description is clear.
  • I understand I am responsible for the contents of this pull request.
  • I have followed the contributing guidelines:
The current `request_ip_source` setting only allows a single option. While it does fall back to the peer IP if the header is missing as of !2003, which likely covers a lot of regular use, only allowing a single option limits the deployment options available to more advanced deployments. Setups where internal and external traffic use different reverse proxies will end up with the wrong IP and the implicitness of the fallback allows for situations where the used IP is not the IP expected. By introducing a setting that allows multiple options to be set, this limitation is resolved and it becomes possible to have client IP resolution behind different reverse proxies and also making it possible to decide if and/or what the fallback should be. If set, options are evaluated in order. If all fail, the request fails. **Pull request checklist:** <!-- You need to complete these before your PR can be considered. If you aren't sure about some, feel free to ask for clarification in #dev:continuwuity.org. --> - [x] This pull request targets the `main` branch, and the branch is named something other than `main`. - [x] I have written an appropriate pull request title and my description is clear. - [x] I understand I am responsible for the contents of this pull request. - I have followed the [contributing guidelines][c1]: - [x] My contribution follows the [code style][c2], if applicable. - [x] I ran [pre-commit checks][c1pc] before opening/drafting this pull request. - [x] I have [tested my contribution][c1t] (or proof-read it for documentation-only changes) myself, if applicable. This includes ensuring code compiles. - [x] My commit messages follow the [commit message format][c1cm] and are descriptive. [c1]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md [c2]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/docs/development/code_style.mdx [c1pc]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#pre-commit-checks [c1t]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#running-tests-locally [c1cm]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#commit-messages
feat: Allow multiple options in request_ip_source
Some checks failed
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 3s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 5s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m9s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m19s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
4f0a8febf9
Only allowing a single option prevents results in requests being denied if
the singular selected source option isn't used for every single request flow.
This means you need to choose between selecting the right IP or properly
separating your infrastructure traffic. You can't have both;
running edge traffic through a reverse proxy with proper IP resolution,
as well as appservices isolated internally is not possible at the same time.

By allowing multiple options to be set, this limitation is resolved and
it becomes possible to have client IP resolution behind reverse proxies,
as well as allow for direct internal connections for appservices.

Options are evaluated in order. If all fail, the request still fails.
@ -21,0 +89,4 @@
.iter()
.filter_map(|s| ClientIp::for_source(s))
.map(|f| f(&parts))
.reduce(|a, b| a.or_else(|_| b))
Author
Contributor

I thought about maybe doing the mapping early and recording it as an extension, then only grab the extension and execute it here but placing the type in the top-level src/router crate would not allow it to be used in here and I don't know if some other common location would make sense 🤔

I thought about maybe doing the mapping early and recording it as an extension, then only grab the extension and execute it here but placing the type in the top-level src/router crate would not allow it to be used in here and I don't know if some other common location would make sense 🤔
chore: Add towncrier news fragment
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m11s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m18s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
24b90164ef
ginger requested changes 2026-07-14 15:30:45 +00:00
Dismissed
Cargo.toml Outdated
@ -117,3 +117,2 @@
[workspace.dependencies.axum-client-ip]
version = "1.3"
[workspace.dependencies.client-ip]
Owner

where is this dependency used?

where is this dependency used?
Author
Contributor

In src/api/router. It's the actual functionality that was behind axum-client-ip. I guess it can be moved to a lower level maybe?

Can't seem to link to the line within the PR, it's in the for_source function starting on line 62.

In `src/api/router`. It's the actual functionality that was behind `axum-client-ip`. I guess it can be moved to a lower level maybe? Can't seem to link to the line within the PR, it's in the `for_source` function starting on line 62.
ginger marked this conversation as resolved
ginger approved these changes 2026-07-14 15:37:59 +00:00
Omar007 force-pushed feature/ip-src-fallback from 24b90164ef
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m11s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m18s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
to 594f5ea32a
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m14s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m18s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 5m40s
Deploy Element Web / 🏗️ Build and Deploy (pull_request) Failing after 6m30s
2026-07-14 15:42:21 +00:00
Compare
ginger added this to the 26.7.0 milestone 2026-07-14 17:33:26 +00:00
Owner

@Omar007 this fails clippy right now

@Omar007 this fails clippy right now
Author
Contributor

Weird, I'm fairly sure I ran it earlier. I'll do the rebase and run it again 🤔

Weird, I'm fairly sure I ran it earlier. I'll do the rebase and run it again 🤔
Omar007 force-pushed feature/ip-src-fallback from 594f5ea32a
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m14s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m18s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 5m40s
Deploy Element Web / 🏗️ Build and Deploy (pull_request) Failing after 6m30s
to a12805e0c9
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 58s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m12s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 11m8s
2026-07-14 18:29:03 +00:00
Compare
Author
Contributor

Ok turns out after my last set of changes I only ended up doing the +nightly format before I committed, I forgot the clippy run at the very end. Fixed, apologies 🙈

Ok turns out after my last set of changes I only ended up doing the `+nightly` format before I committed, I forgot the clippy run at the very end. Fixed, apologies 🙈
@ -391,2 +395,3 @@
/// default: ["direct"]
#[serde(default)]
pub request_ip_source: Option<String>,
pub request_ip_source: Vec<String>,
Owner

This is a breaking change for people that have already set this to something

This is a breaking change for people that have already set this to something
Owner

Seems to have overlapped with !2003 (cc @nex)

Seems to have overlapped with !2003 (cc @nex)
Author
Contributor

I can probably rebase things on top of the changes made by !2003 no probs, at first glance that does not look to be problematic 🤔

In regards to the config change, I could introduce something like allowed_ip_sources instead (feel free to suggest a better name!) to match the explicitness of this as opposed to the implicitness of !2003, then change the behaviour based on the config option used:

  • If allowed_ip_sources is set, extract from the explicitly supplied options, don't allow anything else.
  • If allowed_ip_sources is NOT set but request_ip_source (the current) is set, extract from the chosen option and always fallback to direct (the behaviour as implemented by !2003).

Setting neither or setting them to 'direct' would be the same and ignores any headers.

EDIT: maybe accepted_ip_sources would be better for the new option?

I can probably rebase things on top of the changes made by !2003 no probs, at first glance that does not look to be problematic 🤔 In regards to the config change, I could introduce something like `allowed_ip_sources` instead (feel free to suggest a better name!) to match the explicitness of this as opposed to the implicitness of !2003, then change the behaviour based on the config option used: - If `allowed_ip_sources` is set, extract from the explicitly supplied options, don't allow anything else. - If `allowed_ip_sources` is NOT set but `request_ip_source` (the current) is set, extract from the chosen option and always fallback to direct (the behaviour as implemented by !2003). Setting neither or setting them to 'direct' would be the same and ignores any headers. EDIT: maybe `accepted_ip_sources` would be better for the new option?
Omar007 force-pushed feature/ip-src-fallback from a12805e0c9
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Checks / Prek / Check changed files (pull_request) Successful in 4s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 58s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m12s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 11m8s
to a655e85dc6
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 5s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m25s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 2m9s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m20s
2026-07-18 18:25:22 +00:00
Compare
Author
Contributor

Rebased against the changes in !2003 and !1973. Also did the config property split for now.

Rebased against the changes in !2003 and !1973. Also did the config property split for now.
Author
Contributor

There is apparently a difference between the clippy run documented in CONTRIBUTING.md and what is being run in the Forgejo workflow. I got nothing locally, picking up the ones returned from the workflow now.

EDIT: looks like the workflow runs with --profile test. Should the CONTRIBUTING.md be updated to use that as well?

There is apparently a difference between the clippy run documented in CONTRIBUTING.md and what is being run in the Forgejo workflow. I got nothing locally, picking up the ones returned from the workflow now. EDIT: looks like the workflow runs with `--profile test`. Should the CONTRIBUTING.md be updated to use that as well?
Omar007 force-pushed feature/ip-src-fallback from a655e85dc6
Some checks failed
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Prek / Check changed files (pull_request) Successful in 5s
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m25s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 2m9s
Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 8m20s
to e0737beb16
All checks were successful
Documentation / Build and Deploy Documentation (pull_request) Has been skipped
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m22s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m15s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 9m2s
2026-07-18 18:48:59 +00:00
Compare
Author
Contributor

Ok w/e that latest failure cause is, is beyond my influence haha. Looks like the server was unreachable so it needs a retry.

Ok w/e that latest failure cause is, is beyond my influence haha. Looks like the server was unreachable so it needs a retry.
Owner

allowed_ip_sources makes sense to me. We might deprecate request_ip_source in the future to avoid having the almost duplicate behaviour, but for now it's fine to avoid breaking anything

`allowed_ip_sources` makes sense to me. We might deprecate `request_ip_source` in the future to avoid having the almost duplicate behaviour, but for now it's fine to avoid breaking anything
nex approved these changes 2026-07-21 14:03:22 +00:00
Jade approved these changes 2026-07-21 14:32:08 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
continuwuation/continuwuity!1985
No description provided.