[APPSEC-10361] Appsec automated user events check for UUID in safe mode #2952
Conversation
…tract the event information correctly base on the track user mode
github-actions
bot
added
appsec
GustavoCaso
changed the title
| event[:email] = resource_email if resource_email | ||
| event[:username] = resource_username if resource_username | ||
| when SAFE_MODE | ||
| event[:id] = resource_id if resource_id && resource_id.to_s =~ UUID_REGEX |
There was a problem hiding this comment.
Certainly not for this PR but there is an opportunity here to define matchers (e.g as procs) and iterate over them, in order to easily extend the list of known safe bits of info.
There was a problem hiding this comment.
Also, for my understanding, wasn't there a thing specified where known unsafe bits of info would get hashed to pseudonymous information?
There was a problem hiding this comment.
Certainly not for this PR but there is an opportunity here to define matchers (e.g as procs) and iterate over them, in order to easily extend the list of known safe bits of info.
Noted. I don't think there will be more mode in the future, but if the list where to grow I happy to extract the logic into matchers
There was a problem hiding this comment.
Also, for my understanding, wasn't there a thing specified where known unsafe bits of info would get hashed to pseudonymous information?
In the end, for simplify, we decided not to do it on the first iteration. So in safe mode, we only report ID if we are sure they are UUID. If the customers want to report more information they would need to change the mode to extended. Which reports ID no matter the shape
| module Contrib | ||
| module Devise | ||
| # Module to extract event information from the resource | ||
| module EventInformation |
There was a problem hiding this comment.
WDYT of the class name simply be Devise::Event?
| SAFE_MODE = 'safe' | ||
| EXTENDED_MODE = 'extended' | ||
|
|
||
| def self.extract(resource, mode) |
There was a problem hiding this comment.
WDYT of this being .new (or #initialize)?
GustavoCaso
force-pushed
the
appsec-automated-user-events-check-for-uuid-in-safe-mode
branch
from
5d8013e
to
cfe7763
Compare
Codecov Report
@@ Coverage Diff @@
## master #2952 +/- ##
==========================================
+ Coverage 97.96% 98.06% +0.09%
==========================================
Files 1287 1293 +6
Lines 71018 71438 +420
Branches 3285 3289 +4
==========================================
+ Hits 69574 70055 +481
+ Misses 1444 1383 -61
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
GustavoCaso
deleted the
appsec-automated-user-events-check-for-uuid-in-safe-mode
branch
What does this PR do?
For
safeautomated user tracking events, we want to reduce the possibility of reporting PII information as much as possible. The RFC states that we could report any integer that we know 100% has been autogenerated, we could make the assumption that every ID or UUID coming from Rails is autogenerated, but since we can not be 100% sure, insafemode we only report IDs if they are UUID.This PR extracts the logic of gathering event information into a new module,
Devise::EventInformation.Motivation
Additional Notes
I realised the devise specs were not running in CI 🤦
The second commit enables the devise integration specs in CI 0b9c43c.
How to test the change?
CI