Add callout for crypto request/response models #740
Conversation
trmartin4
changed the title
Deploying contributing-docs with
|
| Latest commit: |
ba91ee7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://590d18ff.contributing-docs.pages.dev |
| Branch Preview URL: | https://add-crypto-response-models.contributing-docs.pages.dev |
|
Great job! No new security vulnerabilities introduced in this pull request |
withinfocus
added
ai-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| In order to separate concerns between API contracts and data models, request or response models | ||
| should be used when transmitting `MasterPasswordAuthenticationData` between the server and client. | ||
| For example, | ||
| [`MasterPasswordAuthenticationDataResponseModel`](https://github.com/bitwarden/server/blob/d6528d2b89a160fd8d4a15756df6bbb4e8f58e4e/src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs#L6) |
There was a problem hiding this comment.
Details and fix
The text references MasterPasswordAuthenticationDataResponseModel but the link points to MasterPasswordAuthenticationDataRequestModel.cs in the /Requests/ directory.
Additionally, line 196 states "when the MasterPasswordAuthenticationData is sent in an API response", but Request models are typically sent in API requests, not responses.
Fix needed:
Either:
- Change the link to point to the actual Response model (if one exists)
- Change the text to say "Request" instead of "Response" and update line 196 to say "sent in an API request"
This should match the pattern used on line 164 where MasterPasswordUnlockResponseModel correctly links to a file in the /Response/ directory.
|
Claude finished @trmartin4's task in 3m 43s —— View job Overall Assessment: APPROVE Reviewed documentation updates to the cryptography guide. This PR successfully adds guidance on using request/response models for crypto data structures and strengthens SDK usage requirements. Code Review DetailsNo issues found. The PR accomplishes its stated objectives:
All changes are technically accurate, clearly written, and appropriately address the security concerns raised in the PR objective. |
withinfocus
added
ai-review
|
Overall Assessment: APPROVE Reviewed documentation updates to the cryptography guide. All changes are well-written and technically sound. The previous issue regarding link-text mismatch has been resolved. Code Review DetailsNo issues found. The PR successfully:
|
| outside of the SDK. | ||
| be written in the | ||
| [SDK](https://github.com/bitwarden/sdk-internal/tree/main/crates/bitwarden-crypto), if possible. | ||
| Existing use-cases can be continued in the TypeScript clients for now, but eventually will be |
There was a problem hiding this comment.
ℹ️ Unrelated but I don't like this language at this point because it's still suggesting not using the SDK.
There was a problem hiding this comment.
What exactly is the suggestion here? / What phrasing would be better?
still suggesting not using the SDK.
The phrasing clearly states that you SHOULD write SDK code, and never recommends using TS code, merely allows it for existing use-cases. All new crypto APIs are only available in the SDK, and existing crypto APIs in TypeScript are for a large part deprecated. The clarification here is that you can modify existing use-cases without having to instantly migrate them to the SDK (because doing so is not always immediately possible before a few blockers are resolved. We've seen this with the vault team).
New use-cases SHOULD be written in the SDK.
As a side note, @withinfocus / @MGibson1 if KM gets a review where people are using TS crypto APIs for a new use-case - such as DIRT adding a new type of report, or similar - is the expectation that we block the PR and request the team to re-write their changeset to work through the SDK?
There was a problem hiding this comment.
My comment is merely because it leaves the existing use case's path open-ended, and my preference is to use stronger language about how any use case, if needing modification, really should be a shift to the SDK at that point.
I do also feel that new use cases should be SDK-only, and it's essential that's known up front before any TypeScript work is even considered. Blocking a PR is too late in the process.
There was a problem hiding this comment.
That's fair. Looking at recent PR's, I'm not sure if all teams are aware of that though.
There was a problem hiding this comment.
How does this sound?
I'd like to also confirm (@quexten?) that all the TS APIs are actually deprecated in clients. That should at least give pause to any team trying to use them. I know it did for Auth. In refinement we recognized "hey, these APIs are deprecated, we better reach out to KM to understand how to do it right" when we were designing a new feature.
| Existing attachments are protected using an `EncArrayBuffer`. This is just an `EncString`, but | ||
| encoded slightly differently. Again, a content-encryption-key is usually used, but not enforced. | ||
| When encrypting files for new purposes, a content-encryption-key **MUST** be used. Consider that | ||
| with the current encryption scheme, the entire file must be downloaded and loaded into ram for |
There was a problem hiding this comment.
| with the current encryption scheme, the entire file must be downloaded and loaded into ram for | |
| with the current encryption scheme, the entire file must be downloaded and loaded into memory for |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27084
📔 Objective
In working on bitwarden/server#6715, the Auth team realized that we needed to use additional abstractions around the
MasterPasswordUnlockDataandMasterPasswordAuthenticationDatawhen transmitting that data abstraction between client and server.In order to avoid other teams making this mistake, I've added a sentence to both the unlock and authentication sections highlighting the need for a specific request/response model.
I also made some slight tweaks to include types in
codeblocks to make them stand out more in the text.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes