Skip to content

LG-7306 Redacted logging of ThreatMetrix response#6857

Merged
stevegsa merged 21 commits intomainfrom
stevegsa-whitelist-tmx-response
Aug 30, 2022
Merged

LG-7306 Redacted logging of ThreatMetrix response#6857
stevegsa merged 21 commits intomainfrom
stevegsa-whitelist-tmx-response

Conversation

@stevegsa
Copy link
Contributor

@stevegsa stevegsa commented Aug 28, 2022

Why: To prevent PII from appearing in log files. Fields like first_name appear when their retention is not set to zero. It will be set that way in prod but we want to be doubly sure PII-ish fields will never appear in log files.
How: Create a redacter service that finds fields that should be filtered and puts the response 'redacted' in them. Note: only first level fields will be processed

Example ThreatMetrix response logging event:

{"name":"IdV: doc auth optional verify_wait submitted","properties":{"event_properties":{"success":true,"errors":{},"address_edited":false,"proofing_results":{"messages":[],"exception":null,"transaction_id":"resolution-mock-transaction-id-123","reference":"aaa-bbb-ccc","timed_out":false,"context":{"should_proof_state_id":true,"stages":{"resolution":{"client":"ResolutionMock","errors":{},"exception":null,"success":true,"timed_out":false,"transaction_id":"resolution-mock-transaction-id-123","reference":"aaa-bbb-ccc"},"state_id":{"client":"StateIdMock","errors":{},"success":true,"timed_out":false,"exception":null,"transaction_id":"state-id-mock-transaction-id-456","state":"MT","state_id_jurisdiction":"ND"},
  "threatmetrix":{"client":"lexisnexis:ddp","errors":{},"exception":null,"success":true,"timed_out":false,
    "transaction_id":"7e76104d-448a-4546-851c-199cbe2ab7b1","response_body":{
      "account_address_city": [redacted],
      "account_address_state": "fl",
      "account_address_street1": [redacted],
      "account_address_zip": [redacted],
      "request_duration":"1445",
      "request_id":"7e76104d-448a-4546-851c-199cbe2ab7b1",
      "request_result":"success",
      "review_status":"pass",
      "risk_rating":"low"

@stevegsa stevegsa changed the title Stevegsa whitelist tmx response LG-7306 Redacted logging of ThreatMetrix response Aug 28, 2022
Comment on lines 54 to 57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this not caught as part of earlier refactor? can we add a regression spec that would have caught this?

Copy link
Contributor Author

@stevegsa stevegsa Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this was a bug. it was adding costing for tmx at the resolution step and checking for proofingcomponent in the spec but was using the transaction_id from resolution not tmx. i'll update the spec

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this? haven't seen specs for it yet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do we pass back the alerts for AAMVA, InstantVerify? Is it in this structure?

Copy link
Contributor Author

@stevegsa stevegsa Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. callback_log_data.result[:context][:stages][:resolution and :state_id...

@zachmargolis zachmargolis requested a review from a team August 29, 2022 15:34
@stevegsa stevegsa requested a review from a team August 29, 2022 15:58
@stevegsa stevegsa force-pushed the stevegsa-whitelist-tmx-response branch from 085f9ac to d3ce41c Compare August 29, 2022 16:09
"tmx_risk_rating": "neutral",
"fraudpoint.score": "500"
"fraudpoint.score": "500",
"first_name": "WARNING! YOU SHOULD NEVER SEE THIS PII FIELD IN THE LOGS"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice way to test!

Copy link
Contributor Author

@stevegsa stevegsa Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. if you have suggestions on how to make that message more clear they are welcome. essentially if we see that in our log files we know there is a bug and we'll simply assume that successful responses always contain PII fields

@stevegsa stevegsa requested a review from zachmargolis August 29, 2022 23:10
@stevegsa stevegsa requested a review from zachmargolis August 29, 2022 23:59
@zachmargolis
Copy link
Contributor

@stevegsa before I approve, can you address this comment: #6857 (comment)

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing all this!

@stevegsa
Copy link
Contributor Author

LGTM, thanks for fixing all this!

omg thanks for staying on that one. found some more costing bugs!!! that should have been a separate story. i tried to slip it in...

Copy link
Contributor Author

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Costing fix will be put in a separate pr...

@stevegsa stevegsa merged commit 75c5dbb into main Aug 30, 2022
@stevegsa stevegsa deleted the stevegsa-whitelist-tmx-response branch August 30, 2022 21:43
@aduth aduth mentioned this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants