OpenAPI-Specification
OpenAPI-Specification copied to clipboard
OAI
Generalize description of password data type
This new wording completely changes the semantics of the format. Currently the words do not infer that a response payload is going to be changed to not include the password. It only says that tools should not display in UI the value. The new words say that the service should not return the value. If you do not want a service to return the value, then the property needs to be indicated as writeOnly, which existed in 3.0 but I we removed in 3.1 because it caused a host of other challenges.
The new words say that the service should not return the value
I disagree; "obscure" and "redact" have a different definition to "omit".
@darrelmiller writeOnly was only removed from 3.1 because it was added to JSON Schema (alongside readOnly).
I agree that the proposed wording is not quite right, but I think the goal, to generalize away from "UI", is reasonable. I assume "redact" is intended to mean "do not show in plain text to users", whether that's in a UI or whether you store the field in a user-visible database (which might show up in a UI, but without the OpenAPI-provided schema modifiers being available), or wherever.
@darrelmiller , @handrews - if this issue sits for several weeks do we close it with a label (that might indicate some sort of lack of consensus?)
@mgrafl - What about: "A hint to secure sensitive data". (thinking about this a bit more, perhaps one might suggest changing the format value of "password" to something like "sensitive". Anyways, we can tackle this from a the context that the format wants to imply rather than from comments.
I don't have a problem with the proposed wording. We can look at this as:
- dropping "to UIs": I think this is fine since any client should take similar precautions
- changing "obscure" to "obscure/redact": maybe unnecessary but not fundamentally wrong
- changing "input" to "the value": this seems like a good change, since "input" implies some context that isn't necessarily clear
But I also want to point out that any change here should also be reflected in the "password" entry of the OpenAPI format registry
Discussed in the weekly meeting, this is a good change to have, but the wording needs an update as per the suggestions in this thread.
@darrelmiller , @handrews - if this issue sits for several weeks do we close it with a label (that might indicate some sort of lack of consensus?)
@mgrafl - What about: "A hint to secure sensitive data". (thinking about this a bit more, perhaps one might suggest changing the format value of "password" to something like "sensitive". Anyways, we can tackle this from a the context that the format wants to imply rather than from comments.
To add some context: the reason for this PR is that the OpenAPI Generator for Java Spring had passwords included in the toString method (https://github.com/OpenAPITools/openapi-generator/issues/16851), which I consider a security risk.
So, at the very least "to UIs" should be dropped from the specification (as @mikekistler explained above).
@robjtede, it appears that the registry file is in a different branch and I don't know how to include it in this PR. Should I create another PR for that?
Oh, so it is... what an interesting structure. I guess you can't include it in this PR then.
if this issue sits for several weeks do we close it with a label (that might indicate some sort of lack of consensus?)
@miqui As you now know from this week's TDC call the day after you made the above comment, we do not have these sorts of processes well-defined.
For the benefit of others who did not join the call and have the same (totally reasonable) sort of question:
I usually prefer to avoid discussing/proposing processes ad-hoc across different PRs and issues, because many people who might care will miss comments like this. We have many process issues currently filed for such discussions.
We will be working through all of the necessary process (re)definition in the TDC calls as quickly as we can. We even changed the weekly call agenda order to prioritize this work for the foreseeable future.
There is a lot to do right now, and the first thing to do is what we're doing: updating the TSC membership so that we have a fully active governing body. Then we will have the people in place to make the process decisions (and expand the groups to which permissions like labeling and closing issues are delegated).
Until we can get there, we will just keep muddling along as we have been doing. But that should not be the case for too much longer – I hope it is clear from the massive number of notifications anyone subscribed to this repo has been seeing this past month that we are very, very busy working to reduce the backlog and clear out cruft, which is another pre-requisite for defining processes that make sense for where we are now. Right now we can't even see what we're working with. We are also dealing with some cruft in our deployment processes, that makes fully finishing some of these PRs more challenging than one would think.
So let's keep the process discussions in the process issues and wait until we have re-built our governance and cleaned out enough cruft that we can see what we really need. I know it is frustrating. It wasn't that many years ago that I was new here and agitating for more action (resulting in a previous round of closing a lot of stale issues). But there is a lot happening and it should be easier to participate and contribute to this project very soon.
Since we're doing a 3.0.4, should this go into the 3.0.4 branch/version first and then be forward-ported?
Note: I have manually validated that this change passes the validate-markdown check when it is run with the correct environment. This PR can be safely merged without rebasing (which we should do and then backport to 3.0.4, rather than waiting to re-target this).
Thanks for confirming that the tests do pass Henry!
And thanks for the original PR as well. 🙏
