Copy and Paste of partially selected password returns entire full password,

BizClimb commented 11 months ago

Steps To Reproduce

Windows 11 - Desktop Client.

To reproduce:

Select an item from your vault,
Click the eye icon to view the password in plain text.
Highlight only a portion of the password (not the entire password, e.x. first character only).
Press Ctrl+C (or right click and select Copy).
Paste to notepad.

This is a new bug that started today for me after update.

Expected Result

The selected portion of the password should be copied to clipboard.

Actual Result

The entire password (ignoring selection) gets copied to clipboard.

Screenshots or Videos

No response

Additional Context

No response

Operating System

Windows

Operating System Version

11

Installation method

Direct Download (from bitwarden.com)

Build Version

Version 2023.9.0 - Shell 24.8.3 - Renderer 112.0.5616.204 - Node 18.14.0 - x64

Issue Tracking Info

[X] I understand that work is tracked outside of Github. A PR will be linked to this issue should one be opened to address it, but Bitwarden doesn't use fields like "assigned", "milestone", or "project" to track progress.

bwbug commented 11 months ago

This behavior also occurs in the browser extension (2023.9.1).

Furthermore, it is not even necessary to select any portion of the password — after toggling the password visibility, it is sufficient to left-click anywhere in the password field and the use Ctrl+C. This will transfer the entire password into the clipboard.

micahblut commented 11 months ago

Thanks for reporting this issue! Can you tell me a bit more about your use-case for wanting to copy only portions of the field rather than the whole thing?

BizClimb commented 11 months ago

Often times we use a segment of the full password for an extra layer of security, in case BW access is ever compromised the passwords themselves in their normal self would still be useless to the intruder unless they know our specific password conversion use case.

Example: if my true password is "PASSWORD", I could enter into the bitwarden the password as "PASS-THIS-WORD" and knowing our specific use case, to only use either every other segment or outside segments, etc... In this case I it's simple and quick to get the password copied if I can copy it partially, where as now, I need to copy the entire password then manipulate it where it's copied (if it allows), in many cases it doesn't (such as various command prompt terminals) and I need to copy to a text editor first and manipulate there to re-copy and paste to the correct password field.

This was fast and simple when copying partials was allowed. If it was intentional to disable partial copying universally for passwords, our use case could still work if partial copying was re-enabled when passwords are made "visible" within bitwarden.

-Thanks

elasticspoon commented 8 months ago

Hey, so I looked into this and it seems that this partial copy behavior was removed in this PR to fix extraneous spaces from being copied. Specifically, the copying of a selection to the clipboard was removed here.

Previously, the appCopyText directive looped over all the ranges in the selection and used regex to remove extra spaces.

    for (let i = 0; i < selection.rangeCount; i++) {
      const range = selection.getRangeAt(i);
      const text = range.toString();

      // The selection should only contain one line of text. In some cases however, the
      // selection contains newlines and space characters from the indentation of following
      // sibling nodes. To avoid copying passwords containing trailing newlines and spaces
      // that aren't part of the password, the selection has to be trimmed.
      let stringEndPos = text.length;
      const newLinePos = text.search(/(?:\r\n|\r|\n)/);
      if (newLinePos > -1) {
        const otherPart = text.substr(newLinePos).trim();
        if (otherPart === "") {
          stringEndPos = newLinePos;
        }
      }
      selectCopy += text.substring(0, stringEndPos);
    }

Now it takes the value of the field as an input. For example:

@Input("appSelectCopy") copyText: string;

<bit-color-password
  [password]="h.password"
  class="tw-block tw-font-mono"
  [appCopyText]="h.password"
></bit-color-password>

Given that the change in behavior seem intentional I am not sure what the best approach is:

This issue could be closed
The PR could be reverted
The regex could be returned but improved.

Another potential solution is: copyText could also include the select behavior and only return the selection if it is contained within the input like fullText.includes(selection) ? selection : fullText

BizClimb commented 8 months ago

I think basic trimming of the password isn't the best solution, especially if white space was intentionally added to the password by the user. Otherwise their password would always be trimmed of their intentional white space via regex? So I would say probably no to reverting the PR.

The intentional change does seem to address this issue as it would retain whitespace that is intentional. But it does break standard OS UI expected behavior with selection and copy paste, which as it did in my case, created a bit of confusion and unpredictability.

I'm not sure if this is possible, but really the best solution would involve addressing the root cause of the extraneous whitespace. This fix, coupled with restoring the OS-standard copy/paste functionality, would offer a full resolution, ensuring both password accuracy and user expectations in regards to UI behavior are met. While implementing this might be complex, especially if the rendering of extra whitespace issue is external to Bitwarden's code, it would ideally adhere to OS conventions and remove the issue with user experience without compromising functionality.

elasticspoon commented 8 months ago

the best solution would involve addressing the root cause of the extraneous whitespace.

I agree. The issue is, after reverting PR#5312 I cannot reproduce the behavior that it aimed to solve. No matter how I try to select from the generator I don't get any extra characters. I tried to build Bitwarden to the state before the PR to see if I could reproduce there. But the extension is too out of date for me to connect to a server and access a vault.

I think its either a operating system specific issue (I can only test on Linux) or something else has changed that fixed the issue. I might try to add the behavior back and see if it passes QA.

Also, from what I can tell this still persists, but that is unrelated to the selection issue.

bitwarden / clients