Redact DoNotLog annotated values in toString and introduce dangerousToString#2917
Redact DoNotLog annotated values in toString and introduce dangerousToString#2917mpritham wants to merge 1 commit into
DoNotLog annotated values in toString and introduce dangerousToString#2917Conversation
Generate changelog in
|
mpritham
force-pushed
the
pm/toString-doNotLog
branch
from
f448c7c to
efa625a
Compare
mpritham
force-pushed
the
pm/toString-doNotLog
branch
from
efa625a to
d461018
Compare
| @Override | ||
| @DoNotLog | ||
| public String toString() { | ||
| return "ObjectAllDoNotLog{secret: REDACTED" + ", token: REDACTED" + '}'; | ||
| } |
There was a problem hiding this comment.
This is not in fact DoNotLog anymore
| default boolean redactToStringDoNotLog() { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
There are certain cases where developers are using the toString value of Conjure objects with DoNotLog fields, so the flag is disabled by default, and will be rolled out repo by repo.
Since this will be generated on the producer, but may cause issues on the consumers, I don't think it's safe to simply turn on the flag? We may instead need to start always introducing the dangerousToString methods, so consumers can migrate across the board (and we can then write automation to migrate everything), then get the flag turned on to also redact the main toString?
| @Override | ||
| public String toString() { | ||
| return "UnionWithDoNotLogVariant{value: SecretValueWrapper{value: REDACTED" + "}}"; | ||
| } |
There was a problem hiding this comment.
I wonder if we should still provide a dangerousToString method for unions, because value() is no quite the same here, and I wonder if this is going to make migration more painful. We can also just not do anything for now and wait until we see migration issues, if any
| @Override | ||
| public String toString() { | ||
| return "SafeValueWrapper{value: " + value + '}'; | ||
| } |
There was a problem hiding this comment.
What is the replacement for this/the union's toString method? getValue() is private access, so we can't use it, and the visitor seems overkill 🤔
| CodeBlock.Builder body = CodeBlock.builder().add("return $S\n", prefix + (redactValue ? "REDACTED" : "")); | ||
| if (!redactValue) { | ||
| body.add(" + $N", VALUE_FIELD_NAME); | ||
| } |
There was a problem hiding this comment.
Won't this redact the entire value if it's do-not-log, when we now actually have safe toString for conjure objects, which we could leverage? Probably good enough for a start, but seems like we may want to revisit that approach?
This actually makes me wonder whether the overall object itself should even be marked DoNotLog anymore (iirc, this mostly indicates the stringified log safety?)
2 participants
Before this PR
Third party libraries can call
toStringon objects when constructing exceptions, leading toDoNotLogannotated fields appearing in logs. HavingtoStringnot contain anyDoNotLogdata will prevent this class of error.Conjure generates
toStringmethods on objects containingDoNotLogfield values.This PR adds the
redactToStringDoNotLogflag (default off), that when enabled:DoNotLogfields as<name>: REDACTEDintoStringand contain a new methoddangerousToString(annotatedDoNotLog) that returns the full representation for the rare cases that need it.DoNotLogaliases return the bare literal"REDACTED"from toString.dangerousToStringmethod because they expose an equivalent methodgetDoNotLogredact the value in the wrapper's toString (both sealed and non-sealed paths)dangerousToStringmethod because they expose an equivalent methodgetValueThere are certain cases where developers are using the
toStringvalue of Conjure objects withDoNotLogfields, so the flag is disabled by default, and will be rolled out repo by repo.After this PR
==COMMIT_MSG==
Redact
DoNotLogannotated values intoStringand introducedangerousToString==COMMIT_MSG==
Possible downsides?
This introduces a new method for every generated object. We can't naively generate the
dangerousToStringonly for objects withDoNotLogfields: aDoNotLogfield could be removed from an object definition while a client calls thedangerousToStringmethod which would lead to an ABI break. Solutions that try to take the previous state of a Conjure definition when generated code seem complicated and would be a divergence from Conjure's simple IR based code gen.