Open hcarty opened 5 years ago
I agree we should redact the passwords by default.
Should we change the behavior of pp
and add a separate, more customizable pretty printer as well? I'd be happy to make a PR for this once an approach is decided on.
Given the existence of GDPR, the best approach is probably to default to not printing the username and password in pp
. How about just a single function with the signature of pp_redacted
above with a ?redact=[Username;Password]
default?
I started looking at this a bit and have a few API design questions/thoughts.
I think `User
and `Password
constructors should be added to the component
type rather than adding the redundant field
type I suggested in the original issue text.
Rather than adding a pp_redacted
function, I think we should make the components to redact a global ref
and always use that ref
's contents to determine the printing behavior in pp
. So pp
would act like pp_redacted
as originally described but ?redact
would be pulled from a (spooky!) global ref
rather than a function argument.
As pure evil as global references may be, the ref
-based approach does have the benefit of being accessible without having to grep
/sed
for all potential uses of Uri.pp
.
The interface additions would be:
(** `User and `Password would be added to the component type definition *)
(** {!pp}'s behavior would silently change to pull from the set of redacted components when printing *)
(** Set of redacted components when printing with {!pp}. Can be changed with {!add_redacted} and {!remove_redacted}. Defaults to [`User] and [`Password]. *)
val redacted : unit -> component list
(** [add_redacted component] adds [component] to the set of redacted components when printing with {!pp}. *)
val add_redacted : component -> unit
(** [remove_redacted component] removes [component] from the set of redacted components when printing with {!pp}. *)
val remove_redacted : component -> unit
It would be better to mark Uri.pp
as deprecated; then the compiler can find all the uses for you. Different callers will need different parts redacted, so having a global for this doesn't seem useful. For example, when a program prints out a message on first run telling you to visit an admin URL to configure the server, the URL printed must include all secret components.
Hiding passwords in a pretty-printer is useful for logging. It may be worth having passwords hidden by default, with a way to opt-in to password printing.
A few possible options: