bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[k.ninev--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

using this Secret Service API code in 'emacs -Q'

(require 'secrets)
(secrets-create-item "session" "my item" "парола" :method "sudo" :user
"joe" :host "remote-host")

Successfully creates an entry but the password "парола" is mangled to
"?0@>;0" and cannot be viewed both with M-x secrets-show-secrets 
and secret-tool cli tool , this does not occur with latin passwords

Tested with both gnome-keyring and keepassxc as API backends.


--text follows this line-
In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.41, cairo version 1.18.0) of 2024-04-08 built on
 arch-nspawn-197981
Repository revision: 600ac35cc6597b63306786bffd0d762f70555322
Repository branch: master
System Description: Arch Linux

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --mandir=/usr/share/man --with-gameuser=:games
 --with-modules --without-m17n-flt --without-gconf
 --with-native-compilation=yes --with-xinput2 --with-pgtk
 --without-xaw3d --with-sound=no --with-tree-sitter --without-gpm
 --without-compress-install
 '--program-transform-name=s/\([ec]tags\)/\1.emacs/'
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions
 -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security
 -fstack-clash-protection -fcf-protection -g
 -ffile-prefix-map=/build/emacs-git/src=/usr/src/debug/emacs-git
 -flto=auto' 'LDFLAGS=-Wl,-O1 -Wl,--sort-common -Wl,--as-needed
 -Wl,-z,relro -Wl,-z,now -Wl,-z,pack-relative-relocs -flto=auto''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LCMS2 LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: bg_BG.UTF-8
  value of $LC_NUMERIC: bg_BG.UTF-8
  value of $LC_TIME: bg_BG.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

-- 
KN

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Eli Zaretskii]

> Date: Tue, 09 Apr 2024 08:18:37 +0000
> From: k.ninev--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 
> using this Secret Service API code in 'emacs -Q'
> 
> (require 'secrets)
> (secrets-create-item "session" "my item" "парола" :method "sudo" :user
> "joe" :host "remote-host")
> 
> Successfully creates an entry but the password "парола" is mangled to
> "?0@>;0" and cannot be viewed both with M-x secrets-show-secrets 
> and secret-tool cli tool , this does not occur with latin passwords

Does the patch below give good results?

Michael, I think we have a similar problem in
dbus-byte-array-to-string: when MULTIBYTE is non-nil, the function
should call decode-coding-string on the unibyte string it produces
instead of converting each byte to multibyte.  Because the bytes in
the argument BYTE-ARRAY are not characters, they are raw bytes of the
UTF-8 sequence, so calling 'string' on them is not TRT.

diff --git a/lisp/net/secrets.el b/lisp/net/secrets.el
index c1a670f..63fc874 100644
--- a/lisp/net/secrets.el
+++ b/lisp/net/secrets.el
@@ -665,7 +665,8 @@ secrets-create-item
 	     ;; Secret.
 	     `(:struct :object-path ,secrets-session-path
 		       (:array :signature "y") ;; No parameters.
-		       ,(dbus-string-to-byte-array password)
+		       ,(dbus-string-to-byte-array
+                         (encode-coding-string password 'utf-8))
                        ,secrets-struct-secret-content-type)
 	     ;; Do not replace. Replace does not seem to work.
 	     nil))

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[k.ninev]

Yes, the patch works for me. Thank you.

On Tuesday, April 9, 2024 12:48 PM (+03:00), Eli Zaretskii wrote:

> > Date: Tue, 09 Apr 2024 08:18:37 +0000
> > From: k.ninev--- via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > 
> > 
> > using this Secret Service API code in 'emacs -Q'
> > 
> > (require 'secrets)
> > (secrets-create-item "session" "my item" "парола" :method "sudo" :user
> > "joe" :host "remote-host")
> > 
> > Successfully creates an entry but the password "парола" is mangled to
> > "?0@>;0" and cannot be viewed both with M-x secrets-show-secrets 
> > and secret-tool cli tool , this does not occur with latin passwords
> 
> Does the patch below give good results?
> 
> Michael, I think we have a similar problem in
> dbus-byte-array-to-string: when MULTIBYTE is non-nil, the function
> should call decode-coding-string on the unibyte string it produces
> instead of converting each byte to multibyte.  Because the bytes in
> the argument BYTE-ARRAY are not characters, they are raw bytes of the
> UTF-8 sequence, so calling 'string' on them is not TRT.
> 
> diff --git a/lisp/net/secrets.el b/lisp/net/secrets.el
> index c1a670f..63fc874 100644
> --- a/lisp/net/secrets.el
> +++ b/lisp/net/secrets.el
> @@ -665,7 +665,8 @@ secrets-create-item
>  	     ;; Secret.
>  	     `(:struct :object-path ,secrets-session-path
>  		       (:array :signature "y") ;; No parameters.
> -		       ,(dbus-string-to-byte-array password)
> +		       ,(dbus-string-to-byte-array
> +                         (encode-coding-string password 'utf-8))
>                         ,secrets-struct-secret-content-type)
>  	     ;; Do not replace. Replace does not seem to work.
>  	     nil))
> 

-- 
KN

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Eli Zaretskii]

> From: k.ninev@cdots.bg
> Cc: 70301@debbugs.gnu.org
> Date: Tue, 09 Apr 2024 10:09:19 +0000
> 
> 
> Yes, the patch works for me. Thank you.

Thanks for testing.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> Does the patch below give good results?

Thanks for the patch. However, I believe we shouldn't fix it in
secrets.el, but in dbus.el.

> Michael, I think we have a similar problem in
> dbus-byte-array-to-string: when MULTIBYTE is non-nil, the function
> should call decode-coding-string on the unibyte string it produces
> instead of converting each byte to multibyte.  Because the bytes in
> the argument BYTE-ARRAY are not characters, they are raw bytes of the
> UTF-8 sequence, so calling 'string' on them is not TRT.

Yes. The following patch works for me. WDYT?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1437 bytes --]

diff --git a/lisp/net/dbus.el b/lisp/net/dbus.el
index 46f85daba24..11c87e72d4e 100644
--- a/lisp/net/dbus.el
+++ b/lisp/net/dbus.el
@@ -997,17 +997,18 @@ dbus-string-to-byte-array
 STRING shall be UTF-8 coded."
   (if (zerop (length string))
       '(:array :signature "y")
-    (cons :array (mapcan (lambda (c) (list :byte c)) string))))
+    (cons :array
+          (mapcan (lambda (c) (list :byte c))
+                  (encode-coding-string string 'utf-8)))))

-(defun dbus-byte-array-to-string (byte-array &optional multibyte)
+(defun dbus-byte-array-to-string (byte-array &optional _multibyte)
   "Transform BYTE-ARRAY into UTF-8 coded string.
-BYTE-ARRAY must be a list of structure (c1 c2 ...), or a byte
-array as produced by `dbus-string-to-byte-array'.  The resulting
-string is unibyte encoded, unless MULTIBYTE is non-nil."
-  (apply
-   (if multibyte #'string #'unibyte-string)
-   (unless (equal byte-array '(:array :signature "y"))
-     (seq-filter #'characterp byte-array))))
+BYTE-ARRAY must be a unibyte list of structure (c1 c2 ...), or a byte
+array as produced by `dbus-string-to-byte-array'."
+   (if-let ((bytes (seq-filter #'characterp byte-array))
+            (string (apply #'unibyte-string bytes)))
+       (decode-coding-string string 'utf-8)
+     ""))

 (defun dbus-escape-as-identifier (string)
   "Escape an arbitrary STRING so it follows the rules for a C identifier.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: k.ninev@cdots.bg,  70301@debbugs.gnu.org
> Date: Wed, 10 Apr 2024 17:32:43 +0200
> 
> > Does the patch below give good results?
> 
> Thanks for the patch. However, I believe we shouldn't fix it in
> secrets.el, but in dbus.el.

Then we ought to update the doc string of dbus-string-to-byte-array,
which currently says the argument should be a UTF-8 encoded string.

> > Michael, I think we have a similar problem in
> > dbus-byte-array-to-string: when MULTIBYTE is non-nil, the function
> > should call decode-coding-string on the unibyte string it produces
> > instead of converting each byte to multibyte.  Because the bytes in
> > the argument BYTE-ARRAY are not characters, they are raw bytes of the
> > UTF-8 sequence, so calling 'string' on them is not TRT.
> 
> Yes. The following patch works for me. WDYT?

If you are okay with changing the behavior of
dbus-byte-array-to-string in backward-incompatible ways, I don't mind.
But maybe this should be called out in NEWS.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Thanks for the patch. However, I believe we shouldn't fix it in
>> secrets.el, but in dbus.el.
>
> Then we ought to update the doc string of dbus-string-to-byte-array,
> which currently says the argument should be a UTF-8 encoded string.

Yep.

>> > Michael, I think we have a similar problem in
>> > dbus-byte-array-to-string: when MULTIBYTE is non-nil, the function
>> > should call decode-coding-string on the unibyte string it produces
>> > instead of converting each byte to multibyte.  Because the bytes in
>> > the argument BYTE-ARRAY are not characters, they are raw bytes of the
>> > UTF-8 sequence, so calling 'string' on them is not TRT.
>>
>> Yes. The following patch works for me. WDYT?
>
> If you are okay with changing the behavior of
> dbus-byte-array-to-string in backward-incompatible ways, I don't mind.
> But maybe this should be called out in NEWS.

I'm aware of this incompatibility. Well, I've played several hours today
with this. Since we have no information from D-Bus about the underlying
byte array, we must assume a given context. A sequence of raw 8-bit
bytes, unibyte, representing an UTF-8 encoded string seems to be aapropriate.

If people want another conversion for a D-Bus byte array, it is still
possible. But they shouldn't use dbus-byte-array-to-string and
dbus-string-to-byte-array then.

I'll puzzle everything together by tomorrow.

Best regards, <<michael.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Albinus <michael.albinus@gmx.de> writes:

> I'll puzzle everything together by tomorrow.

Pushed to master. Could you, pls, check whether it works as expected?

Best regards, <<michael.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[k.ninev]

> Pushed to master. Could you, pls, check whether it works as expected?

It works as expected, thank you.
-- 
KN

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Version: 30.1

k.ninev@cdots.bg writes:

Hi,

>> Pushed to master. Could you, pls, check whether it works as expected?
>
> It works as expected, thank you.

Thanks for the feedback. Closing the bug.

Best regards, Michael.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  70301-done@debbugs.gnu.org
> Date: Fri, 12 Apr 2024 10:59:56 +0200
> 
> >> Pushed to master. Could you, pls, check whether it works as expected?
> >
> > It works as expected, thank you.
> 
> Thanks for the feedback. Closing the bug.

Thanks.  I needed to fix some inaccuracies in the documentation,
please take a look.

bug#70301: 30.0.50; secrets-create-item mangles cyrillic passwords

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> I needed to fix some inaccuracies in the documentation, please take a
> look.

Thanks!

Best regards, Michael.