-
Notifications
You must be signed in to change notification settings - Fork 8
Set x-synthetic-id header on integration routes #255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Set x-synthetic-id header on integration routes #255
Conversation
Integration proxy responses were missing the x-synthetic-id header because handle_proxy dispatched to integrations without adding it. This caused identity tracking to break for first-party re-hosted integrations like Permutive's secure-signal endpoint. Centralizing the header logic in handle_proxy ensures all current and future integrations automatically include the synthetic ID, rather than requiring each integration to implement it manually. Fixes #205
aram356
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start but should use funcitons in cookies.rs
Please don't make the feature branch have this long name. Please stick to feature/short-name
Replaces inline cookie parsing with handle_request_cookies() and parse_cookies_to_jar() from cookies.rs for consistency.
Had deleted the comment accidentally but was needed already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 We need a more thorough fix:
publisher.rs:201-211 uses manual string splitting to check for an existing synthetic cookie:
let has_synthetic_cookie = req
.get_header(header::COOKIE)
.and_then(|h| h.to_str().ok())
.map(|cookies| {
cookies.split(';').any(|cookie| {
cookie.trim_start().starts_with(&format!("{}=", COOKIE_SYNTHETIC_ID))
})
})
.unwrap_or(false);
While registry.rs:622-626 uses handle_request_cookies with CookieJar:
let has_synthetic_cookie = handle_request_cookies(&req)
.ok()
.flatten()
.and_then(|jar| jar.get(COOKIE_SYNTHETIC_ID).map(|_| true))
.unwrap_or(false);
These should use the same approach. The CookieJar version is more robust (proper cookie parsing).
Since we already have cookie utilities in cookies.rs, suggest adding a set_synthetic_cookie(settings: &Settings, response: &mut Response, synthetic_id: &str) helper there that handles the append_header call. That way both publisher.rs and registry.rs call the same function, and the append_header fix and cookie creation logic live in one place.
We don't need has_synthetic_cookie check we was primarily for logging we should always set COOKIE_SYNTHETIC_ID
| assert!(!registry.has_route(&Method::GET, "/integrations/test/users")); | ||
| assert!(!registry.has_route(&Method::POST, "/integrations/test/users")); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Duplicate test
These three cookie_jar_* tests are testing parse_cookies_to_jar from cookies.rs — they should live there, not in the registry module. cookies.rs already has equivalent coverage (test_parse_cookies_to_jar, test_parse_cookies_to_jar_emtpy, etc.), so these are duplicates and can be removed.
Integration proxy responses were missing the x-synthetic-id header because handle_proxy dispatched to integrations without adding it. This caused identity tracking to break for first-party re-hosted integrations like Permutive's secure-signal endpoint.
Centralizing the header logic in handle_proxy ensures all current and future integrations automatically include the synthetic ID, rather than requiring each integration to implement it manually.
Fixes #205