-
Notifications
You must be signed in to change notification settings - Fork 263
feat(core): add masked_connector_response with per-connector key allowlist #2050
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?
Changes from 8 commits
6e2166f
b4431bc
cb64e5c
d00b920
0cd38c0
6909fa0
73ee3ff
95ecc2d
c8ac48f
709b850
d2eeb6a
52de180
e5169b4
4452706
2a98da5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -327,6 +327,48 @@ fn flow_status_label(flow_status: &domain_types::router_data::FlowStatus) -> Str | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// Build the selectively-masked view of a connector response and stash it on the flow data. | ||||||||||
| /// | ||||||||||
| /// Reads the untouched response bytes, so this works whether or not `raw_connector_response` | ||||||||||
| /// is being captured — the safe view can be on in production with raw capture off. | ||||||||||
| fn record_unmasked_connector_response<ResourceCommonData>( | ||||||||||
| resource_common_data: &mut ResourceCommonData, | ||||||||||
| body: &Response, | ||||||||||
| connector_name: &str, | ||||||||||
| config: &domain_types::connector_response_masking::ConnectorResponseMaskingConfig, | ||||||||||
| ) where | ||||||||||
| ResourceCommonData: RawConnectorRequestResponse, | ||||||||||
| { | ||||||||||
| use std::str::FromStr; | ||||||||||
|
|
||||||||||
| // `connector_name` came from `ConnectorEnum::get_connector_name()`, and the enum derives | ||||||||||
| // `EnumString` with snake_case, so this always round-trips. | ||||||||||
| let Ok(connector) = domain_types::connector_types::ConnectorEnum::from_str(connector_name) | ||||||||||
| else { | ||||||||||
| return; | ||||||||||
| }; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add debug log here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a log here would have papered over a live bug — the comment above this branch is wrong.
The branch is reachable — Same root cause hit config load: Rather than logging the silent exit, I removed the re-parse. Ingress already validates the name per flow family ( Verified: with |
||||||||||
|
|
||||||||||
| // By name: this HeaderMap is reqwest 0.11 (http 0.2), not the http 1.x in scope. | ||||||||||
| let content_type = body | ||||||||||
| .headers | ||||||||||
| .as_ref() | ||||||||||
| .and_then(|headers| headers.get("content-type")) | ||||||||||
| .and_then(|value| value.to_str().ok()); | ||||||||||
|
|
||||||||||
| let masked = domain_types::connector_response_masking::mask_connector_response( | ||||||||||
| &body.response, | ||||||||||
| content_type, | ||||||||||
| &connector, | ||||||||||
| config, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| if let Some(masked) = masked.as_deref() { | ||||||||||
| tracing::Span::current().record("response.unmasked_body", tracing::field::display(masked)); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| resource_common_data.set_unmasked_connector_response(masked); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// Handles the connector response, processing both successful and error responses | ||||||||||
| #[allow(clippy::too_many_arguments)] | ||||||||||
| pub fn handle_connector_response<F, ResourceCommonData, Req, Resp>( | ||||||||||
|
|
@@ -367,6 +409,18 @@ where | |||||||||
| .set_connector_response_headers(body.headers.clone()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Independent of `return_raw_connector_data`: this view is already sanitized. | ||||||||||
| if let Some(params) = | ||||||||||
| event_params.filter(|p| p.connector_response_masking.enabled) | ||||||||||
| { | ||||||||||
| record_unmasked_connector_response( | ||||||||||
| &mut updated_router_data.resource_common_data, | ||||||||||
| &body, | ||||||||||
| params.connector_name, | ||||||||||
| params.connector_response_masking, | ||||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| let handle_response_result = connector.handle_response_v2( | ||||||||||
| &updated_router_data, | ||||||||||
| event.as_deref_mut(), | ||||||||||
|
|
@@ -424,6 +478,18 @@ where | |||||||||
| .set_connector_response_headers(body.headers.clone()); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // A 4xx/5xx body is exactly when the masked view is most useful. | ||||||||||
| if let Some(params) = | ||||||||||
| event_params.filter(|p| p.connector_response_masking.enabled) | ||||||||||
| { | ||||||||||
| record_unmasked_connector_response( | ||||||||||
| &mut updated_router_data.resource_common_data, | ||||||||||
| &body, | ||||||||||
| params.connector_name, | ||||||||||
| params.connector_response_masking, | ||||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| let error_response = match body.status_code { | ||||||||||
| 500..=511 => connector.get_5xx_error_response( | ||||||||||
| body.clone(), | ||||||||||
|
|
@@ -533,6 +599,10 @@ pub struct EventProcessingParams<'a> { | |||||||||
| pub tenant_id: &'a str, | ||||||||||
| pub merchant_id: &'a str, | ||||||||||
| pub return_raw_connector_data: bool, | ||||||||||
| /// Per-connector key lists driving `unmasked_connector_response`. Gated by its own | ||||||||||
| /// `enabled` flag, deliberately independent of `return_raw_connector_data`. | ||||||||||
| pub connector_response_masking: | ||||||||||
| &'a domain_types::connector_response_masking::ConnectorResponseMaskingConfig, | ||||||||||
| pub connector_latency: ConnectorLatencyTracker, | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -546,6 +616,7 @@ pub struct EventProcessingParams<'a> { | |||||||||
| request.url = Empty, | ||||||||||
| request.method = Empty, | ||||||||||
| response.body = Empty, | ||||||||||
| response.unmasked_body = Empty, | ||||||||||
| response.headers = Empty, | ||||||||||
| response.error_message = Empty, | ||||||||||
| response.status_code = Empty, | ||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
enabled = truehere and insandbox.toml, gated independently ofreturn_raw_connector_data, means the masked view reaches callers from the first deploy — which makes the bypasses I've flagged onconnector_response_masking.rslive rather than latent. Can we default itfalseuntil those are closed and covered? The structDefaultis alreadyfalse, so it's config-only.The comment block above also promises PAN/CVV/expiry stay masked regardless — true for keyed fields, not for the unkeyed paths.
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.
Taking it.
enabled = falsein bothsandbox.tomlandproduction.toml,development.tomlstaystrue. Config-only, as you note — the structDefaultwas alreadyfalse.I'd have argued the containment case is weaker now that the bypasses are closed in this same PR, but that's the wrong way round: the reason to ship it off is that turning it on returns connector response bytes to the caller, so a deployment should opt in once it has chosen its key lists — independent of whether any particular bypass is open. Added that as the comment above the flag.
On the comment block — you're right that it was a claim about keyed fields dressed up as an unconditional one. Rewritten to say what naming a key actually grants, and to name the unkeyed case explicitly rather than leave it implied:
Applied to all three config files. Fixed in 52de180.