Fix/security OIDC open redirect#1653
Conversation
WalkthroughAdd canonical-origin helpers; replace regex validator; validate and normalize redirects in login, callback, and logout; refactor logout signature and OIDC logout URL building; canonicalize local dev origins; update Helm ChangesOIDC Redirect Validation Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/values.yaml`:
- Around line 53-55: The P_ORIGIN_URI example in the env documentation is
indented too far and will break YAML if uncommented; move the commented example
key "P_ORIGIN_URI: \"https://parseable.example.com\"" out to the same
indentation level as other env keys (align with the "env" entries), removing the
extra leading spaces so the comment preserves valid YAML structure when
uncommented and visually matches related env entries.
In `@src/handlers/http/oidc.rs`:
- Around line 666-669: The function is_valid_redirect_url_safe currently rejects
an empty redirect string (due to serde defaulting missing redirect to ""), which
breaks optional redirect behavior; change the empty-check to treat an empty
redirect as "no redirect provided" by returning
Ok(canonical.as_str().to_string()) (i.e., use the canonical URL as the effective
redirect) instead of Err(()). Update the branch that checks redirect.is_empty()
in is_valid_redirect_url_safe to return the canonical URL string and keep the
rest of the validation logic for non-empty redirect values unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c39d35ca-5b88-4d8c-bfa7-cfc685108411
📒 Files selected for processing (3)
helm/values.yamlsrc/cli.rssrc/handlers/http/oidc.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/oidc.rs`:
- Around line 77-96: Add unit tests for canonical_local_origin to cover the IPv6
wildcard and the unwrap_or_else fallback: 1) assert that
canonical_local_origin("::", false) and canonical_local_origin(":::8000", false)
canonicalize to "http://localhost:8000/" (or with provided port), 2) assert that
an invalid socket string like canonical_local_origin("not_a_valid_address",
false) falls back to "http://localhost:8000/", and 3) assert that the https
boolean is respected (e.g., canonical_local_origin("127.0.0.1:9000", true) ->
"https://127.0.0.1:9000/"); add these tests in the same tests module alongside
the existing tests referencing canonical_local_origin.
- Around line 67-75: canonical_local_origin fails for unbracketed IPv6 and
doesn't canonicalize bracketed wildcard "[::]"; update canonical_local_origin to
(1) detect IPv6 literal inputs (contain ':' and not already bracketed) and wrap
the address portion in brackets before building the URL so Url::parse accepts
forms like "::1:8000" or ":::8000", (2) preserve and use any port from
socket_addr rather than falling back to hardcoded localhost:8000, and (3) treat
both "::" and "[::]" (and "0.0.0.0") as wildcard hosts and replace them with
"localhost" using url.set_host(Some("localhost")). Also add unit tests covering
unbracketed IPv6 with port (e.g. "::1:PORT"), bracketed wildcard "[::]:PORT",
and IPv4 wildcard "0.0.0.0:PORT" to prevent regressions; reference
canonical_local_origin in src/oidc.rs and its tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 95bd2126-b944-4eb1-8786-4c64bcc246fa
📒 Files selected for processing (2)
src/handlers/http/oidc.rssrc/oidc.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/oidc.rs
| pub fn canonical_local_origin(socket_addr: &str, https: bool) -> Url { | ||
| let scheme = if https { "https" } else { "http" }; | ||
| let mut url = Url::parse(&format!("{scheme}://{socket_addr}")) | ||
| .unwrap_or_else(|_| Url::parse(&format!("{scheme}://localhost:8000")).unwrap()); | ||
| if matches!(url.host_str(), Some("0.0.0.0") | Some("::")) { | ||
| url.set_host(Some("localhost")).expect("localhost is valid"); | ||
| } | ||
| url | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Test url crate IPv6 parsing with various socket address formats
cat > /tmp/test_ipv6_url.rs << 'EOF'
use url::Url;
fn main() {
let test_cases = vec![
("http://[::1]:8000", "bracketed IPv6 loopback"),
("http://[::]:8000", "bracketed IPv6 wildcard"),
("http://:::8000", "unbracketed IPv6 wildcard with port"),
("http://::1:8000", "unbracketed IPv6 loopback with port"),
("http://0.0.0.0:8000", "IPv4 wildcard"),
];
for (input, desc) in test_cases {
match Url::parse(input) {
Ok(url) => {
println!("✓ {}: parsed", desc);
println!(" host_str: {:?}", url.host_str());
println!(" port: {:?}", url.port());
}
Err(e) => println!("✗ {}: parse error: {}", desc, e),
}
}
}
EOF
# Compile and run the test
rustc --edition 2021 /tmp/test_ipv6_url.rs -o /tmp/test_ipv6_url --extern url=$(fd -t f liburl.*\.rlib | head -1 | sed 's/.*\(\/.*\)/\1/') 2>/dev/null || \
(cd /tmp && cargo init --name test_ipv6 && \
echo 'url = "2.4.0"' >> Cargo.toml && \
cat test_ipv6_url.rs > src/main.rs && \
cargo run --quiet)Repository: parseablehq/parseable
Length of output: 312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "rustc:"; (command -v rustc && rustc --version) || true
echo "cargo:"; (command -v cargo && cargo --version) || true
echo "rustup:"; (command -v rustup && rustup --version) || true
echo
echo "rustup show:"
rustup show || true
echo
echo "Try toolchain-specific versions:"
(rustc +stable --version && cargo +stable --version) || true
(rustc +1.77.0 --version && cargo +1.77.0 --version) || true
(rustc +nightly --version && cargo +nightly --version) || trueRepository: parseablehq/parseable
Length of output: 1803
🌐 Web query:
url crate Url::parse unbracketed IPv6 format '::1:8000' or ':::8000'
💡 Result:
The url crate in Rust does not support unbracketed IPv6 addresses in URLs. According to the URL Standard, which the crate implements, IPv6 literals must be enclosed in square brackets (e.g., [::1]) [1][2][3]. Attempting to parse an invalid format like::1:8000 or:::8000 using Url::parse will result in a ParseError::InvalidIpv6Address [1][4]. The documentation and examples explicitly state that IPv6 addresses are required to be given between '[' and ']' brackets [2][4][5]. For example: assert!(Url::parse("http://[:::1]") == Err(ParseError::InvalidIpv6Address)); [1] If you need to handle IPv6 addresses, ensure they are properly bracketed, such as [2001:db8::1]:8080/path [3].
Citations:
- 1: https://docs.rs/urls/latest/url/
- 2: https://docs.rs/url/latest/url/struct.Url.html
- 3: https://oneuptime.com/blog/post/2026-03-20-parse-ipv6-addresses-rust/view
- 4: https://docs.rs/url/latest/src/url/host.rs.html
- 5: https://docs.rs/url/1.6.1/url/struct.Url.html
🌐 Web query:
url crate IPv6 parsing rules host_str for bracketed '[::]' and wildcard '::'
💡 Result:
In the Rust url crate: 1) Url::host_str() for bracketed IPv6 - Url::host_str() returns the serialization of an IPv6 host address in square brackets ([ and ]), in RFC 5952 format (lowercase hex with maximal :: compression) [1][2]. - The crate’s host parsing rules state that the host is either an IPv6 address in [...] brackets or a domain name [2]. 2) How bracketed [::] is handled - [::] (inside brackets) is an IPv6 literal host form; since host_str() brackets IPv6 and uses the IPv6 textual representation rules, bracketed [::] will be treated as an IPv6 unspecified-address literal and serialized back with brackets [1][2]. - Invalid bracketed IPv6 literals are rejected; e.g. Url::parse("http://[:::1]") returns ParseError::InvalidIpv6Address, demonstrating the parser enforces IPv6 literal syntax inside [...] [3]. 3) Wildcard :: - The url crate does not document or implement a “wildcard ::” host grammar. The host parser rule is explicitly “IPv6 address in [...] square brackets, or a domain” [2]. - Therefore, :: is treated as just an (invalid) host string unless it appears in the required bracketed IPv6-literal form [...] [2]. Practical guidance - Use bracketed form for IPv6 hosts in URLs: http://[::1]/ and http://[::]/. - Do not use :: as a wildcard host value with the url crate; it’s not part of the documented host parsing model [2].
Citations:
- 1: https://docs.rs/url/latest/src/url/host.rs.html
- 2: https://docs.rs/url/latest/url/enum.Host.html
- 3: https://docs.rs/url/%5E1.2.0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Run with explicit toolchain since rustup has no default configured.
TOOLCHAIN="+1.95.0"
cat > /tmp/test_ipv6_url.rs <<'EOF'
use url::Url;
fn main() {
let test_cases = vec![
("http://[::1]:8000", "bracketed IPv6 loopback"),
("http://[::]:8000", "bracketed IPv6 wildcard"),
("http://:::8000", "unbracketed IPv6 wildcard with port"),
("http://::1:8000", "unbracketed IPv6 loopback with port"),
("http://0.0.0.0:8000", "IPv4 wildcard"),
];
for (input, desc) in test_cases {
match Url::parse(input) {
Ok(url) => {
println!("✓ {}: parsed => host_str={:?} host={:?} port={:?}", desc, url.host_str(), url.host(), url.port());
}
Err(e) => println!("✗ {}: parse error: {}", desc, e),
}
}
}
EOF
# Use cargo to fetch the url crate (no manual rlib path hunting).
rm -rf /tmp/test_ipv6_url_proj
mkdir -p /tmp/test_ipv6_url_proj/src
cp /tmp/test_ipv6_url.rs /tmp/test_ipv6_url_proj/src/main.rs
cat > /tmp/test_ipv6_url_proj/Cargo.toml <<'EOF'
[package]
name = "test_ipv6_url"
version = "0.1.0"
edition = "2021"
[dependencies]
url = "2.5.2"
EOF
(cd /tmp/test_ipv6_url_proj && cargo $TOOLCHAIN run --quiet)Repository: parseablehq/parseable
Length of output: 667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' src/oidc.rs | nl -ba | sed -n '40,110p'Repository: parseablehq/parseable
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/oidc.rs | sed -n '40,120p'Repository: parseablehq/parseable
Length of output: 2716
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Origin::Local|socket_addr" srcRepository: parseablehq/parseable
Length of output: 710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/option.rs | sed -n '90,170p'Repository: parseablehq/parseable
Length of output: 3116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/cli.rs | sed -n '640,760p'Repository: parseablehq/parseable
Length of output: 5048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "address:|validation::socket_addr|P_.*ADDRESS" src/cli.rsRepository: parseablehq/parseable
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/cli.rs | sed -n '110,240p'Repository: parseablehq/parseable
Length of output: 4405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat > /tmp/test_to_socket_addrs.rs <<'EOF'
use std::net::ToSocketAddrs;
fn main() {
let cases = vec![
("0.0.0.0:8000", "ipv4 wildcard"),
("[::]:8000", "ipv6 wildcard bracketed"),
("[::1]:8000", "ipv6 loopback bracketed"),
("::1:8000", "ipv6 loopback unbracketed"),
(":::8000", "ipv6 wildcard+port unbracketed"),
];
for (s, desc) in cases {
let ok = s.to_socket_addrs().is_ok();
println!("{} => {}", desc, ok);
}
}
EOF
rm -rf /tmp/test_to_socket_addrs_proj
mkdir -p /tmp/test_to_socket_addrs_proj/src
cp /tmp/test_to_socket_addrs.rs /tmp/test_to_socket_addrs_proj/src/main.rs
cat > /tmp/test_to_socket_addrs_proj/Cargo.toml <<'EOF'
[package]
name = "test_to_socket_addrs"
version = "0.1.0"
edition = "2021"
[dependencies]
EOF
(cd /tmp/test_to_socket_addrs_proj && cargo +1.95.0 run --quiet)Repository: parseablehq/parseable
Length of output: 224
Fix IPv6 wildcard + parsing behavior in canonical_local_origin (src/oidc.rs)
url::Url::parseonly accepts bracketed IPv6 in URLs; unbracketed inputs like::1:8000/:::8000fail and hit the fallback tohttp(s)://localhost:8000(hardcoded port).- For bracketed wildcard inputs (
[::]:PORT),url.host_str()is"[::]", so the current matchSome("::")never fires—redirects may still use the wildcard IPv6 host. - Update to (a) bracket-wrap IPv6
socket_addrbefore building the URL, (b) canonicalize whenhost_str()is"[::]", and (c) add IPv6-focused tests (including configured ports) to prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/oidc.rs` around lines 67 - 75, canonical_local_origin fails for
unbracketed IPv6 and doesn't canonicalize bracketed wildcard "[::]"; update
canonical_local_origin to (1) detect IPv6 literal inputs (contain ':' and not
already bracketed) and wrap the address portion in brackets before building the
URL so Url::parse accepts forms like "::1:8000" or ":::8000", (2) preserve and
use any port from socket_addr rather than falling back to hardcoded
localhost:8000, and (3) treat both "::" and "[::]" (and "0.0.0.0") as wildcard
hosts and replace them with "localhost" using url.set_host(Some("localhost")).
Also add unit tests covering unbracketed IPv6 with port (e.g. "::1:PORT"),
bracketed wildcard "[::]:PORT", and IPv4 wildcard "0.0.0.0:PORT" to prevent
regressions; reference canonical_local_origin in src/oidc.rs and its tests.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn wildcard_ipv4_origin_uses_localhost_with_configured_port() { | ||
| assert_eq!( | ||
| canonical_local_origin("0.0.0.0:8000", false).as_str(), | ||
| "http://localhost:8000/" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn loopback_ipv4_origin_is_preserved() { | ||
| assert_eq!( | ||
| canonical_local_origin("127.0.0.1:9000", false).as_str(), | ||
| "http://127.0.0.1:9000/" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add test coverage for IPv6 wildcard and fallback behavior.
The test suite has the following gaps:
-
Missing test for
::IPv6 wildcard canonicalization: Line 71 explicitly handles"::"in the match pattern, but there's no test verifying thatcanonical_local_origin("::", false)orcanonical_local_origin(":::8000", false)correctly rewrites tolocalhost. -
Missing test for fallback behavior: The
unwrap_or_elsefallback on line 69-70 is not tested. Consider adding a test with an invalid socket address (e.g.,"invalid_address") to verify it falls back tolocalhost:8000.
🧪 Suggested test additions
#[test]
fn wildcard_ipv6_origin_uses_localhost_with_configured_port() {
// Test :: wildcard with port
assert_eq!(
canonical_local_origin(":::8000", false).as_str(),
"http://localhost:8000/"
);
}
#[test]
fn invalid_socket_addr_falls_back_to_localhost_8000() {
assert_eq!(
canonical_local_origin("not_a_valid_address", false).as_str(),
"http://localhost:8000/"
);
}
#[test]
fn https_scheme_is_respected() {
assert_eq!(
canonical_local_origin("127.0.0.1:9000", true).as_str(),
"https://127.0.0.1:9000/"
);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/oidc.rs` around lines 77 - 96, Add unit tests for canonical_local_origin
to cover the IPv6 wildcard and the unwrap_or_else fallback: 1) assert that
canonical_local_origin("::", false) and canonical_local_origin(":::8000", false)
canonicalize to "http://localhost:8000/" (or with provided port), 2) assert that
an invalid socket string like canonical_local_origin("not_a_valid_address",
false) falls back to "http://localhost:8000/", and 3) assert that the https
boolean is respected (e.g., canonical_local_origin("127.0.0.1:9000", true) ->
"https://127.0.0.1:9000/"); add these tests in the same tests module alongside
the existing tests referencing canonical_local_origin.
1 participant
Fixes [#XXXX]
Description
This PR fixes a Host-header trust issue in OIDC redirect validation that could allow open
redirects. Previously, OIDC login/logout redirects could derive the allowed origin from request
host information, so a spoofed
Hostheader plus a matching external redirect URL could betreated as same-origin.
The likely intent of the old behavior was convenience: redirects could work automatically across
local, proxied, and multi-host deployments without a configured public URL. The tradeoff was
unsafe for OIDC, because request
Hostis not a reliable trust boundary behind proxies, ingresscontrollers, load balancers, or TLS terminators.
This patch validates OIDC redirect targets against a configured canonical origin instead of the
request
Host. Relative redirects are still allowed, and absolute redirects must match thecanonical Parseable origin. Deployments behind proxies/TLS terminators should set
P_ORIGIN_URIto the public Parseable URL, for example
https://parseable.example.com.The
redirectquery parameter is now optional for OIDC login/logoutto prevent deserialization failures when the frontend omits it.
Key changes:
stateredirects against the canonical origin./oidc-not-configuredpath.redirectquery parameter optional in OIDC login/logout (via#[serde(default)])P_ORIGIN_URIusage for proxied/TLS-terminated deployments.This PR has:
obvious for an unfamiliar reader.
Summary by CodeRabbit
Bug Fixes
Documentation