Support full-object encodedJson input on Send create and edit - #1308
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the replacement of the Code Review Details
Also noted, no action required: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| fn parse_encoded_send_view(raw: &str) -> color_eyre::eyre::Result<SendView> { | ||
| if let Ok(decoded) = STANDARD.decode(raw.trim()) | ||
| && let Ok(text) = std::str::from_utf8(&decoded) | ||
| && let Ok(view) = serde_json::from_str::<SendView>(text) | ||
| { | ||
| return Ok(view); | ||
| } | ||
|
|
||
| serde_json::from_str::<SendView>(raw).wrap_err("Error parsing the encoded request data.") | ||
| } |
There was a problem hiding this comment.
SendView directly rejects the output of bw send template, breaking the documented template → create workflow.
Details and fix
SendView (crates/bitwarden-send/src/send.rs:326-365) is #[serde(rename_all = "camelCase", deny_unknown_fields)] with no #[serde(default)], so a successful parse requires every non-Option field: name, hasPassword, type, accessCount, disabled, hideEmail, revisionDate, deletionDate, emails, authType.
SendTextTemplate (this file, ~line 505) emits only name, notes, type, text, deletionDate. So:
bw send template send.text | jq '.name="x" | .text.text="y" | .deletionDate="2030-01-01T00:00:00Z"' | bw send create
fails with Error parsing the encoded request data (missing field hasPassword). The render_template doc comment in this same file explicitly states the template shapes exist "so round-trips via bw send create are unambiguous" — that round-trip does not work. deny_unknown_fields additionally rejects legacy bw send get output, which carries extra fields such as object and accessUrl.
Consider parsing into a CLI-local input DTO with #[serde(default)] on the server-owned read-only fields (hasPassword, accessCount, revisionDate, authType, emails, disabled, hideEmail) and tolerating unknown keys, then converting to SendView. At minimum, extend SendTextTemplate/SendFileTemplate so their output parses, and add a test covering template output → parse_encoded_send_view.
Secondary: when the input is valid base64 of JSON but the JSON fails to deserialize, the if let chain falls through and re-parses the base64 string as JSON, so the user sees expected value at line 1 column 1 instead of the real cause (missing field 'hasPassword'). Splitting the base64-decode step from the JSON-parse step and surfacing the decoded-JSON error when the input decoded cleanly would make these failures diagnosable.
| fn read_encoded_json_input(positional: Option<String>) -> color_eyre::eyre::Result<Option<String>> { | ||
| if let Some(raw) = positional { | ||
| return Ok(Some(raw)); | ||
| } | ||
| if std::io::stdin().is_terminal() { | ||
| return Ok(None); | ||
| } | ||
| let mut buf = String::new(); | ||
| std::io::stdin().read_to_string(&mut buf)?; | ||
| if buf.trim().is_empty() { | ||
| return Ok(None); | ||
| } | ||
| Ok(Some(buf)) | ||
| } |
There was a problem hiding this comment.
send create/send edit now block on stdin whenever stdin is a non-TTY pipe that stays open.
Details and fix
read_encoded_json_input is called unconditionally for create and edit (lines 296-309), before any check of whether the caller actually supplied JSON-style input. When no positional is given and stdin is not a terminal, read_to_string blocks until EOF.
Previously neither subcommand touched stdin, so this is a behavior change for existing flag-only usage:
ssh host 'bw send edit --itemid <id> --deleteInDays 3' # stdin is an open sshd pipe -> hangs
docker run -i image bw send create --name x --text y # hangs until stdin closes
while read line; do bw send create --name x --text "$line"; done < list.txt # swallows the rest of list.txt
A related case: if stdin happens to carry non-JSON bytes, a fully specified flag-only invocation now fails with Error parsing the encoded request data even though the flags were sufficient.
Suggested fix: only consult stdin when the command has no other input source — for create, when both --text and --file are absent; for edit, when no other override flag was supplied. That keeps the template | jq | bw send create piping story while leaving flag-only invocations independent of stdin.
There was a problem hiding this comment.
❓ Could this impact users that rely on using the CLI via scripts?
There was a problem hiding this comment.
Yes! Should be fixed now :)
There was a problem hiding this comment.
Changes look good to me! I'll approve, but I think it may be worth adding a unit test to cover the stdin gate that's being used now, I'm happy to re-approve if you introduce this.
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1308 +/- ##
==========================================
+ Coverage 85.81% 85.90% +0.09%
==========================================
Files 490 493 +3
Lines 70590 71347 +757
==========================================
+ Hits 60574 61290 +716
- Misses 10016 10057 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mcamirault
left a comment
mcamirault
left a comment
There was a problem hiding this comment.
One suggestion for improving an error message and I'd echo the AI comments, particularly the second one, but overall looks good
|
|
||
| let resolved_name = name.unwrap_or(view.name); | ||
| if resolved_name.is_empty() { | ||
| return Err(eyre!("--name is required for text Sends.")); |
There was a problem hiding this comment.
⛏️ Name should be required for all Send types (though interestingly when I looked at the server it doesn't actually enforce this, something we might want to fix)
| // stdout while the requested file path goes uncreated would be a worse UX than | ||
| // an explicit "not implemented" error. | ||
| Some(SendCommands::Get(args)) if args.output_path.is_some() => Err(eyre!( | ||
| "`--output` on `bw send get` is not yet implemented (tracked under PM-39238)." |
There was a problem hiding this comment.
| "`--output` on `bw send get` is not yet implemented (tracked under PM-39238)." | |
| "`--output` on `bw send get` is not yet implemented" |
| fn read_encoded_json_input(positional: Option<String>) -> color_eyre::eyre::Result<Option<String>> { | ||
| if let Some(raw) = positional { | ||
| return Ok(Some(raw)); | ||
| } | ||
| if std::io::stdin().is_terminal() { | ||
| return Ok(None); | ||
| } | ||
| let mut buf = String::new(); | ||
| std::io::stdin().read_to_string(&mut buf)?; | ||
| if buf.trim().is_empty() { | ||
| return Ok(None); | ||
| } | ||
| Ok(Some(buf)) | ||
| } |
There was a problem hiding this comment.
❓ Could this impact users that rely on using the CLI via scripts?
| "Creating file Sends from JSON is not supported: the CLI needs a local file path \ | ||
| to read and encrypt the contents. Use `--file <path>` instead (file-send \ | ||
| creation is tracked under PM-39238)." |
There was a problem hiding this comment.
| "Creating file Sends from JSON is not supported: the CLI needs a local file path \ | |
| to read and encrypt the contents. Use `--file <path>` instead (file-send \ | |
| creation is tracked under PM-39238)." | |
| "Creating file Sends from JSON is not supported: the CLI needs a local file path \ | |
| to read and encrypt the contents. Use `--file <path>` instead." |
Fixes two IMPORTANT findings from review: - Parse full-object JSON into a CLI-local SendJsonInput DTO instead of SendView directly. SendView's deny_unknown_fields/no-defaults broke the documented `bw send template` -> create round trip and rejected `bw send get` output (which carries extra fields like `object`/`accessUrl`). - Gate the stdin read behind a stdin_eligible check so flag-only create/edit invocations never touch stdin, fixing a hang/data-loss risk for scripted usage (ssh pipes, `docker run -i`, `while read` loops). Also fixes the base64-decode-succeeds-but-JSON-fails case, which previously masked the real deserialize error by silently re-parsing the base64 string as JSON, and applies the wording nits from mcamirault and harr1424 (generalize the --name-required message, drop stale PM-39238 references from user-facing error text).
…ct encodedJson input on Send create and edit (bitwarden/sdk-internal#1308)
4 participants
🎟️ Tracking
PM-39240 (https://bitwarden.atlassian.net/browse/PM-39240)
📔 Objective
Implements full-object encodedJson input support for bw send create and bw send edit, completing the work deferred from PR #1176 (PM-34719).