From 3bb3cf5fcb8027d066f7bae5c62681d1a2bddf5e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 11:13:13 -0700 Subject: [PATCH 01/23] feat: Fix keychain issues and add fs fallback - Add missing features for `keyring` to make it actually work instead of being a no-op (grr) - Allow option for storing token in a file as a fallback for keychain issues (disabled by default) - Add ability to get debug logging --- Cargo.lock | 831 ++++++++++++++++++++++++++++++++++++++++++++++- Cargo.toml | 9 + src/auth/pkce.rs | 113 ++++++- 3 files changed, 941 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3eef7de..fe5e1c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,17 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "aes" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b169f7a6d4742236a0a00c541b845991d0ac43e546831af1249753ab4c3aa3a0" +dependencies = [ + "cfg-if", + "cipher", + "cpufeatures", +] + [[package]] name = "aho-corasick" version = "1.1.4" @@ -76,6 +87,137 @@ version = "1.0.102" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" +[[package]] +name = "async-broadcast" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "435a87a52755b8f27fcf321ac4f04b2802e337c8c4872923137471ec39c37532" +dependencies = [ + "event-listener", + "event-listener-strategy", + "futures-core", + "pin-project-lite", +] + +[[package]] +name = "async-channel" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "924ed96dd52d1b75e9c1a3e6275715fd320f5f9439fb5a4a11fa51f4221158d2" +dependencies = [ + "concurrent-queue", + "event-listener-strategy", + "futures-core", + "pin-project-lite", +] + +[[package]] +name = "async-executor" +version = "1.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c96bf972d85afc50bf5ab8fe2d54d1586b4e0b46c97c50a0c9e71e2f7bcd812a" +dependencies = [ + "async-task", + "concurrent-queue", + "fastrand", + "futures-lite", + "pin-project-lite", + "slab", +] + +[[package]] +name = "async-fs" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8034a681df4aed8b8edbd7fbe472401ecf009251c8b40556b304567052e294c5" +dependencies = [ + "async-lock", + "blocking", + "futures-lite", +] + +[[package]] +name = "async-io" +version = "2.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "456b8a8feb6f42d237746d4b3e9a178494627745c3c56c6ea55d92ba50d026fc" +dependencies = [ + "autocfg", + "cfg-if", + "concurrent-queue", + "futures-io", + "futures-lite", + "parking", + "polling", + "rustix", + "slab", + "windows-sys 0.61.2", +] + +[[package]] +name = "async-lock" +version = "3.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "290f7f2596bd5b78a9fec8088ccd89180d7f9f55b94b0576823bbbdc72ee8311" +dependencies = [ + "event-listener", + "event-listener-strategy", + "pin-project-lite", +] + +[[package]] +name = "async-process" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc50921ec0055cdd8a16de48773bfeec5c972598674347252c0399676be7da75" +dependencies = [ + "async-channel", + "async-io", + "async-lock", + "async-signal", + "async-task", + "blocking", + "cfg-if", + "event-listener", + "futures-lite", + "rustix", +] + +[[package]] +name = "async-recursion" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3b43422f69d8ff38f95f1b2bb76517c91589a924d1559a0e935d7c8ce0274c11" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "async-signal" +version = "0.2.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52b5aaafa020cf5053a01f2a60e8ff5dccf550f0f77ec54a4e47285ac2bab485" +dependencies = [ + "async-io", + "async-lock", + "atomic-waker", + "cfg-if", + "futures-core", + "futures-io", + "rustix", + "signal-hook-registry", + "slab", + "windows-sys 0.61.2", +] + +[[package]] +name = "async-task" +version = "4.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b75356056920673b02621b35afd0f7dda9306d03c79a30f5c56c44cf256e3de" + [[package]] name = "async-trait" version = "0.1.89" @@ -120,18 +262,55 @@ dependencies = [ "generic-array", ] +[[package]] +name = "block-padding" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8894febbff9f758034a5b8e12d87918f56dfc64a8e1fe757d65e29041538d93" +dependencies = [ + "generic-array", +] + +[[package]] +name = "blocking" +version = "1.6.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e83f8d02be6967315521be875afa792a316e28d57b5a2d401897e2a7921b7f21" +dependencies = [ + "async-channel", + "async-task", + "futures-io", + "futures-lite", + "piper", +] + [[package]] name = "bumpalo" version = "3.20.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5d20789868f4b01b2f2caec9f5c4e0213b41e3e5702a50157d699ae31ced2fcb" +[[package]] +name = "byteorder" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" + [[package]] name = "bytes" version = "1.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1e748733b7cbc798e1434b6ac524f0c1ff2ab456fe201501e6497c8417a4fc33" +[[package]] +name = "cbc" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26b52a9543ae338f279b96b0b9fed9c8093744685043739079ce85cd58f289a6" +dependencies = [ + "cipher", +] + [[package]] name = "cc" version = "1.2.62" @@ -166,6 +345,16 @@ dependencies = [ "windows-link", ] +[[package]] +name = "cipher" +version = "0.4.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "773f3b9af64447d2ce9850330c473515014aa235e6a783b02db81ff39e4a3dad" +dependencies = [ + "crypto-common", + "inout", +] + [[package]] name = "clap" version = "4.6.1" @@ -218,7 +407,7 @@ dependencies = [ "keyring", "open", "pretty_assertions", - "rand", + "rand 0.9.4", "regex", "reqwest", "schemars", @@ -239,6 +428,35 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" +[[package]] +name = "concurrent-queue" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ca0197aee26d1ae37445ee532fefce43251d24cc7c166799f4d46817f1d3973" +dependencies = [ + "crossbeam-utils", +] + +[[package]] +name = "core-foundation" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91e195e091a93c46f7102ec7818a2aa394e1e1771c3ab4825963fa03e45afb8f" +dependencies = [ + "core-foundation-sys", + "libc", +] + +[[package]] +name = "core-foundation" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b2a6cd9ae233e7f62ba4e9353e81a88df7fc8a5987b8d445b4d90c879bd156f6" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "core-foundation-sys" version = "0.8.7" @@ -254,6 +472,12 @@ dependencies = [ "libc", ] +[[package]] +name = "crossbeam-utils" +version = "0.8.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" + [[package]] name = "crypto-common" version = "0.1.7" @@ -264,6 +488,35 @@ dependencies = [ "typenum", ] +[[package]] +name = "dbus" +version = "0.9.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b942602992bb7acfd1f51c49811c58a610ef9181b6e66f3e519d79b540a3bf73" +dependencies = [ + "libc", + "libdbus-sys", + "windows-sys 0.61.2", +] + +[[package]] +name = "dbus-secret-service" +version = "4.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "708b509edf7889e53d7efb0ffadd994cc6c2345ccb62f55cfd6b0682165e4fa6" +dependencies = [ + "aes", + "block-padding", + "cbc", + "dbus", + "fastrand", + "hkdf", + "num", + "once_cell", + "sha2", + "zeroize", +] + [[package]] name = "deunicode" version = "1.6.2" @@ -284,6 +537,7 @@ checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" dependencies = [ "block-buffer", "crypto-common", + "subtle", ] [[package]] @@ -303,6 +557,33 @@ version = "1.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d0881ea181b1df73ff77ffaaf9c7544ecc11e82fba9b5f27b262a3c73a332555" +[[package]] +name = "endi" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "66b7e2430c6dff6a955451e2cfc438f09cea1965a9d6f87f7e3b90decc014099" + +[[package]] +name = "enumflags2" +version = "0.7.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1027f7680c853e056ebcec683615fb6fbbc07dbaa13b4d5d9442b146ded4ecef" +dependencies = [ + "enumflags2_derive", + "serde", +] + +[[package]] +name = "enumflags2_derive" +version = "0.7.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "67c78a4d8fdf9953a5c9d458f9efe940fd97a0cab0941c075a813ac594733827" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "equivalent" version = "1.0.2" @@ -319,6 +600,27 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "event-listener" +version = "5.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e13b66accf52311f30a0db42147dadea9850cb48cd070028831ae5f5d4b856ab" +dependencies = [ + "concurrent-queue", + "parking", + "pin-project-lite", +] + +[[package]] +name = "event-listener-strategy" +version = "0.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8be9f3dfaaffdae2972880079a491a1a8bb7cbed0b8dd7a347f668b4150a3b93" +dependencies = [ + "event-listener", + "pin-project-lite", +] + [[package]] name = "fastrand" version = "2.4.1" @@ -361,6 +663,42 @@ version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7e3450815272ef58cec6d564423f6e755e25379b217b0bc688e295ba24df6b1d" +[[package]] +name = "futures-io" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cecba35d7ad927e23624b22ad55235f2239cfa44fd10428eecbeba6d6a717718" + +[[package]] +name = "futures-lite" +version = "2.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f78e10609fe0e0b3f4157ffab1876319b5b0db102a2c60dc4626306dc46b44ad" +dependencies = [ + "fastrand", + "futures-core", + "futures-io", + "parking", + "pin-project-lite", +] + +[[package]] +name = "futures-macro" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e835b70203e41293343137df5c0664546da5745f82ec9b84d40be8336958447b" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "futures-sink" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c39754e157331b013978ec91992bde1ac089843443c49cbc7f46150b0fad0893" + [[package]] name = "futures-task" version = "0.3.32" @@ -374,7 +712,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "389ca41296e6190b48053de0321d02a77f32f8a5d2461dd38762c0593805c6d6" dependencies = [ "futures-core", + "futures-io", + "futures-macro", + "futures-sink", "futures-task", + "memchr", "pin-project-lite", "slab", ] @@ -450,6 +792,36 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hermit-abi" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" + +[[package]] +name = "hex" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" + +[[package]] +name = "hkdf" +version = "0.12.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b5f8eb2ad728638ea2c7d47a21db23b7b58a72ed6a38256b8a1849f15fbbdf7" +dependencies = [ + "hmac", +] + +[[package]] +name = "hmac" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" +dependencies = [ + "digest", +] + [[package]] name = "http" version = "1.4.0" @@ -693,6 +1065,16 @@ dependencies = [ "serde_core", ] +[[package]] +name = "inout" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "879f10e63c20629ecabbb64a8010319738c66a5cd0c29b02d63d272b03751d01" +dependencies = [ + "block-padding", + "generic-array", +] + [[package]] name = "ipnet" version = "2.12.0" @@ -759,7 +1141,14 @@ version = "3.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eebcc3aff044e5944a8fbaf69eb277d11986064cba30c468730e8b9909fb551c" dependencies = [ + "byteorder", + "dbus-secret-service", "log", + "secret-service", + "security-framework 2.11.1", + "security-framework 3.7.0", + "windows-sys 0.60.2", + "zbus", "zeroize", ] @@ -775,6 +1164,15 @@ version = "0.2.186" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66" +[[package]] +name = "libdbus-sys" +version = "0.2.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "328c4789d42200f1eeec05bd86c9c13c7f091d2ba9a6ea35acdf51f31bc0f043" +dependencies = [ + "pkg-config", +] + [[package]] name = "linux-raw-sys" version = "0.12.1" @@ -805,6 +1203,15 @@ version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" +[[package]] +name = "memoffset" +version = "0.9.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "488016bfae457b036d996092f6cb448677611ce4449e970ceaf42695203f218a" +dependencies = [ + "autocfg", +] + [[package]] name = "mime" version = "0.3.17" @@ -832,6 +1239,83 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "nix" +version = "0.29.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71e2746dc3a24dd78b3cfcb7be93368c6de9963d30f43a6a73998a9cf4b17b46" +dependencies = [ + "bitflags", + "cfg-if", + "cfg_aliases", + "libc", + "memoffset", +] + +[[package]] +name = "num" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "35bd024e8b2ff75562e5f34e7f4905839deb4b22955ef5e73d2fea1b9813cb23" +dependencies = [ + "num-bigint", + "num-complex", + "num-integer", + "num-iter", + "num-rational", + "num-traits", +] + +[[package]] +name = "num-bigint" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a5e44f723f1133c9deac646763579fdb3ac745e418f2a7af9cd0c431da1f20b9" +dependencies = [ + "num-integer", + "num-traits", +] + +[[package]] +name = "num-complex" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73f88a1307638156682bada9d7604135552957b7818057dcef22705b4d509495" +dependencies = [ + "num-traits", +] + +[[package]] +name = "num-integer" +version = "0.1.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7969661fd2958a5cb096e56c8e1ad0444ac2bbcd0061bd28660485a44879858f" +dependencies = [ + "num-traits", +] + +[[package]] +name = "num-iter" +version = "0.1.45" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1429034a0490724d0075ebb2bc9e875d6503c3cf69e235a8941aa757d83ef5bf" +dependencies = [ + "autocfg", + "num-integer", + "num-traits", +] + +[[package]] +name = "num-rational" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f83d14da390562dca69fc84082e73e548e1ad308d24accdedd2720017cb37824" +dependencies = [ + "num-bigint", + "num-integer", + "num-traits", +] + [[package]] name = "num-traits" version = "0.2.19" @@ -864,6 +1348,22 @@ dependencies = [ "pathdiff", ] +[[package]] +name = "ordered-stream" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9aa2b01e1d916879f73a53d01d1d6cee68adbb31d6d9177a8cfce093cced1d50" +dependencies = [ + "futures-core", + "pin-project-lite", +] + +[[package]] +name = "parking" +version = "2.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f38d5652c16fde515bb1ecef450ab0f6a219d619a7274976324d5e377f7dceba" + [[package]] name = "pathdiff" version = "0.2.3" @@ -882,6 +1382,37 @@ version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" +[[package]] +name = "piper" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c835479a4443ded371d6c535cbfd8d31ad92c5d23ae9770a61bc155e4992a3c1" +dependencies = [ + "atomic-waker", + "fastrand", + "futures-io", +] + +[[package]] +name = "pkg-config" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7edddbd0b52d732b21ad9a5fab5c704c14cd949e5e9a1ec5929a24fded1b904c" + +[[package]] +name = "polling" +version = "3.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d0e4f59085d47d8241c88ead0f274e8a0cb551f3625263c05eb8dd897c34218" +dependencies = [ + "cfg-if", + "concurrent-queue", + "hermit-abi", + "pin-project-lite", + "rustix", + "windows-sys 0.61.2", +] + [[package]] name = "potential_utf" version = "0.1.5" @@ -920,6 +1451,15 @@ dependencies = [ "syn", ] +[[package]] +name = "proc-macro-crate" +version = "3.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e67ba7e9b2b56446f1d419b1d807906278ffa1a658a8a5d8a39dcb1f5a78614f" +dependencies = [ + "toml_edit", +] + [[package]] name = "proc-macro2" version = "1.0.106" @@ -958,7 +1498,7 @@ dependencies = [ "bytes", "getrandom 0.3.4", "lru-slab", - "rand", + "rand 0.9.4", "ring", "rustc-hash", "rustls", @@ -1005,14 +1545,35 @@ version = "6.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8dcc9c7d52a811697d2151c701e0d08956f92b0e24136cf4cf27b57a6a0d9bf" +[[package]] +name = "rand" +version = "0.8.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5ca0ecfa931c29007047d1bc58e623ab12e5590e8c7cc53200d5202b69266d8a" +dependencies = [ + "libc", + "rand_chacha 0.3.1", + "rand_core 0.6.4", +] + [[package]] name = "rand" version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "44c5af06bb1b7d3216d91932aed5265164bf384dc89cd6ba05cf59a35f5f76ea" dependencies = [ - "rand_chacha", - "rand_core", + "rand_chacha 0.9.0", + "rand_core 0.9.5", +] + +[[package]] +name = "rand_chacha" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" +dependencies = [ + "ppv-lite86", + "rand_core 0.6.4", ] [[package]] @@ -1022,7 +1583,16 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" dependencies = [ "ppv-lite86", - "rand_core", + "rand_core 0.9.5", +] + +[[package]] +name = "rand_core" +version = "0.6.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c" +dependencies = [ + "getrandom 0.2.17", ] [[package]] @@ -1228,6 +1798,61 @@ dependencies = [ "syn", ] +[[package]] +name = "secret-service" +version = "4.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e4d35ad99a181be0a60ffcbe85d680d98f87bdc4d7644ade319b87076b9dbfd4" +dependencies = [ + "aes", + "cbc", + "futures-util", + "generic-array", + "hkdf", + "num", + "once_cell", + "rand 0.8.6", + "serde", + "sha2", + "zbus", +] + +[[package]] +name = "security-framework" +version = "2.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "897b2245f0b511c87893af39b033e5ca9cce68824c4d7e7630b5a1d339658d02" +dependencies = [ + "bitflags", + "core-foundation 0.9.4", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + +[[package]] +name = "security-framework" +version = "3.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b7f4bc775c73d9a02cde8bf7b2ec4c9d12743edf609006c7facc23998404cd1d" +dependencies = [ + "bitflags", + "core-foundation 0.10.1", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + +[[package]] +name = "security-framework-sys" +version = "2.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ce2691df843ecc5d231c0b14ece2acc3efb62c0a398c7e1d875f3983ce020e3" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "semver" version = "1.0.28" @@ -1288,6 +1913,17 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_repr" +version = "0.1.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "175ee3e80ae9982737ca543e96133087cbd9a485eecc3bc4de9c1a37b47ea59c" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -1300,6 +1936,17 @@ dependencies = [ "serde", ] +[[package]] +name = "sha1" +version = "0.10.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3bf829a2d51ab4a5ddf1352d8470c140cadc8301b2ae1789db023f01cedd6ba" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "sha2" version = "0.10.9" @@ -1365,6 +2012,12 @@ version = "1.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6ce2be8dc25455e1f91df71bfa12ad37d7af1092ae736f3a6cd0e37bc7810596" +[[package]] +name = "static_assertions" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" + [[package]] name = "strsim" version = "0.11.1" @@ -1503,6 +2156,36 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_edit" +version = "0.25.8+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "16bff38f1d86c47f9ff0647e6838d7bb362522bdf44006c7068c2b1e606f1f3c" +dependencies = [ + "indexmap", + "toml_datetime", + "toml_parser", + "winnow", +] + +[[package]] +name = "toml_parser" +version = "1.1.0+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2334f11ee363607eb04df9b8fc8a13ca1715a72ba8662a26ac285c98aabb4011" +dependencies = [ + "winnow", +] + [[package]] name = "tower" version = "0.5.3" @@ -1591,6 +2274,17 @@ version = "1.20.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40ce102ab67701b8526c123c1bab5cbe42d7040ccfd0f64af1a385808d2f43de" +[[package]] +name = "uds_windows" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e" +dependencies = [ + "memoffset", + "tempfile", + "windows-sys 0.60.2", +] + [[package]] name = "unicase" version = "2.9.0" @@ -1864,6 +2558,15 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "windows-sys" +version = "0.59.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e38bc4d79ed67fd075bcc251a1c39b32a1776bbe92e5bef1f0bf1f8c531853b" +dependencies = [ + "windows-targets 0.52.6", +] + [[package]] name = "windows-sys" version = "0.60.2" @@ -2011,6 +2714,15 @@ version = "0.53.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" +[[package]] +name = "winnow" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0592e1c9d151f854e6fd382574c3a0855250e1d9b2f99d9281c6e6391af352f1" +dependencies = [ + "memchr", +] + [[package]] name = "wit-bindgen" version = "0.51.0" @@ -2111,6 +2823,16 @@ version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1ffae5123b2d3fc086436f8834ae3ab053a283cfac8fe0a0b8eaae044768a4c4" +[[package]] +name = "xdg-home" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ec1cdab258fb55c0da61328dc52c8764709b249011b2cad0454c72f0bf10a1f6" +dependencies = [ + "libc", + "windows-sys 0.59.0", +] + [[package]] name = "yansi" version = "1.0.1" @@ -2140,6 +2862,68 @@ dependencies = [ "synstructure", ] +[[package]] +name = "zbus" +version = "4.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bb97012beadd29e654708a0fdb4c84bc046f537aecfde2c3ee0a9e4b4d48c725" +dependencies = [ + "async-broadcast", + "async-executor", + "async-fs", + "async-io", + "async-lock", + "async-process", + "async-recursion", + "async-task", + "async-trait", + "blocking", + "enumflags2", + "event-listener", + "futures-core", + "futures-sink", + "futures-util", + "hex", + "nix", + "ordered-stream", + "rand 0.8.6", + "serde", + "serde_repr", + "sha1", + "static_assertions", + "tracing", + "uds_windows", + "windows-sys 0.52.0", + "xdg-home", + "zbus_macros", + "zbus_names", + "zvariant", +] + +[[package]] +name = "zbus_macros" +version = "4.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "267db9407081e90bbfa46d841d3cbc60f59c0351838c4bc65199ecd79ab1983e" +dependencies = [ + "proc-macro-crate", + "proc-macro2", + "quote", + "syn", + "zvariant_utils", +] + +[[package]] +name = "zbus_names" +version = "3.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b9b1fef7d021261cc16cba64c351d291b715febe0fa10dc3a443ac5a5022e6c" +dependencies = [ + "serde", + "static_assertions", + "zvariant", +] + [[package]] name = "zerocopy" version = "0.8.48" @@ -2239,3 +3023,40 @@ name = "zmij" version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" + +[[package]] +name = "zvariant" +version = "4.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2084290ab9a1c471c38fc524945837734fbf124487e105daec2bb57fd48c81fe" +dependencies = [ + "endi", + "enumflags2", + "serde", + "static_assertions", + "zvariant_derive", +] + +[[package]] +name = "zvariant_derive" +version = "4.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73e2ba546bda683a90652bac4a279bc146adad1386f25379cf73200d2002c449" +dependencies = [ + "proc-macro-crate", + "proc-macro2", + "quote", + "syn", + "zvariant_utils", +] + +[[package]] +name = "zvariant_utils" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c51bcff7cc3dbb5055396bcf774748c3dab426b4b8659046963523cee4808340" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] diff --git a/Cargo.toml b/Cargo.toml index 724bfe7..fe848cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,6 +37,15 @@ thiserror = "2.0.17" tokio = { version = "1.48.0", features = ["fs", "io-std", "io-util", "macros", "net", "process", "rt-multi-thread", "signal", "sync", "time"] } tracing = "0.1.43" +[target.'cfg(target_os = "linux")'.dependencies] +keyring = { version = "3.6.1", optional = true, features = ["async-secret-service", "async-io", "crypto-rust"] } + +[target.'cfg(target_os = "macos")'.dependencies] +keyring = { version = "3.6.1", optional = true, features = ["apple-native"] } + +[target.'cfg(windows)'.dependencies] +keyring = { version = "3.6.1", optional = true, features = ["windows-native"] } + [features] pkce-auth = ["dep:keyring", "dep:open", "dep:rand", "dep:sha2", "dep:url", "dep:zeroize"] diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index d0478d5..ff67c9b 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -99,6 +99,7 @@ pub struct PkceAuthProvider { redirect_uri: Option, app_id: String, env_prefix: String, + allow_file_fallback: bool, /// In-process token cache keyed by env. cache: Arc>>, } @@ -131,6 +132,7 @@ impl PkceAuthProvider { redirect_uri: None, app_id: String::new(), env_prefix, + allow_file_fallback: false, cache: Arc::new(RwLock::new(HashMap::new())), } } @@ -169,6 +171,18 @@ impl PkceAuthProvider { self } + /// Enables an encrypted-file fallback when the system keychain is unavailable + /// (e.g. headless Linux / WSL without a running secret-service daemon). + /// + /// Disabled by default. The original TypeScript CLI had no file fallback; + /// enable only when you have confirmed the deployment environment lacks a + /// reliable keychain and you accept unencrypted credentials on disk. + #[must_use] + pub fn with_file_fallback(mut self, enabled: bool) -> Self { + self.allow_file_fallback = enabled; + self + } + fn effective_client_id(&self) -> String { let key = format!("{}_OAUTH_CLIENT_ID", self.env_prefix); std::env::var(&key).unwrap_or_else(|_| self.client_id.clone()) @@ -217,19 +231,99 @@ impl PkceAuthProvider { "token" } + /// Returns the path to the fallback credential file for this provider/env. + /// + /// Used when the system keychain is unavailable (e.g. WSL, headless Linux). + fn credential_file_path(&self, env: &str) -> Option { + let app = if self.app_id.is_empty() { + &self.name + } else { + &self.app_id + }; + let base = std::env::var("XDG_CONFIG_HOME") + .map(std::path::PathBuf::from) + .or_else(|_| { + std::env::var("HOME").map(|h| std::path::PathBuf::from(h).join(".config")) + }) + .ok()?; + Some( + base.join(app) + .join("credentials") + .join(format!("{}-{}.json", self.name, env)), + ) + } + fn load_token_from_keychain(&self, env: &str) -> Option { - let entry = keyring::Entry::new(&self.keychain_service(env), self.keychain_user()).ok()?; - let json = entry.get_password().ok()?; + let service = self.keychain_service(env); + match keyring::Entry::new(&service, self.keychain_user()) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + } + Ok(entry) => match entry.get_password() { + Err(e) => { + tracing::warn!(service, error = %e, "keychain read failed"); + } + Ok(json) => match serde_json::from_str(&json) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain token JSON invalid"); + } + Ok(token) => return Some(token), + }, + }, + } + if !self.allow_file_fallback { + return None; + } + let path = self.credential_file_path(env)?; + let json = std::fs::read_to_string(&path).ok()?; + tracing::debug!(path = %path.display(), "loaded token from file fallback"); serde_json::from_str(&json).ok() } fn save_token_to_keychain(&self, env: &str, token: &StoredToken) -> Result<()> { - let entry = keyring::Entry::new(&self.keychain_service(env), self.keychain_user()) - .map_err(|err| CliCoreError::message(format!("keychain access failed: {err}")))?; let json = serde_json::to_string(token).map_err(CliCoreError::from)?; - entry - .set_password(&json) - .map_err(|err| CliCoreError::message(format!("keychain write failed: {err}")))?; + let service = self.keychain_service(env); + match keyring::Entry::new(&service, self.keychain_user()) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + } + Ok(entry) => match entry.set_password(&json) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain write failed"); + } + Ok(()) => { + tracing::debug!(service, "token saved to keychain"); + return Ok(()); + } + }, + } + if !self.allow_file_fallback { + return Err(CliCoreError::message( + "keychain is unavailable and file fallback is disabled — \ + ensure your system keychain (e.g. gnome-keyring, macOS Keychain) \ + is running and unlocked", + )); + } + let path = self + .credential_file_path(env) + .ok_or_else(|| CliCoreError::message("could not determine credential file path"))?; + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).map_err(|e| { + CliCoreError::message(format!("failed to create credential directory: {e}")) + })?; + } + std::fs::write(&path, &json).map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + path.display() + )) + })?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).ok(); + } + tracing::debug!(path = %path.display(), "token saved to file fallback"); Ok(()) } @@ -237,6 +331,11 @@ impl PkceAuthProvider { if let Ok(entry) = keyring::Entry::new(&self.keychain_service(env), self.keychain_user()) { drop(entry.delete_credential()); } + if self.allow_file_fallback { + if let Some(path) = self.credential_file_path(env) { + drop(std::fs::remove_file(path)); + } + } } async fn cached_token(&self, env: &str) -> Option { From 827a8bf412ccd5e7d8d4f5b3a9f87fbbe86e316c Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 12:02:35 -0700 Subject: [PATCH 02/23] Address pull request issues --- src/auth/pkce.rs | 71 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 13 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index ff67c9b..d659bd6 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -171,7 +171,7 @@ impl PkceAuthProvider { self } - /// Enables an encrypted-file fallback when the system keychain is unavailable + /// Enables a file-based fallback when the system keychain is unavailable /// (e.g. headless Linux / WSL without a running secret-service daemon). /// /// Disabled by default. The original TypeScript CLI had no file fallback; @@ -242,9 +242,8 @@ impl PkceAuthProvider { }; let base = std::env::var("XDG_CONFIG_HOME") .map(std::path::PathBuf::from) - .or_else(|_| { - std::env::var("HOME").map(|h| std::path::PathBuf::from(h).join(".config")) - }) + .or_else(|_| std::env::var("HOME").map(|h| std::path::PathBuf::from(h).join(".config"))) + .or_else(|_| std::env::var("APPDATA").map(std::path::PathBuf::from)) .ok()?; Some( base.join(app) @@ -260,6 +259,9 @@ impl PkceAuthProvider { tracing::warn!(service, error = %e, "keychain entry creation failed"); } Ok(entry) => match entry.get_password() { + Err(keyring::Error::NoEntry) => { + tracing::debug!(service, "no stored token in keychain"); + } Err(e) => { tracing::warn!(service, error = %e, "keychain read failed"); } @@ -312,17 +314,36 @@ impl PkceAuthProvider { CliCoreError::message(format!("failed to create credential directory: {e}")) })?; } + #[cfg(unix)] + { + use std::io::Write as _; + use std::os::unix::fs::OpenOptionsExt as _; + let mut file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(&path) + .map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + path.display() + )) + })?; + file.write_all(json.as_bytes()).map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + path.display() + )) + })?; + } + #[cfg(not(unix))] std::fs::write(&path, &json).map_err(|e| { CliCoreError::message(format!( "failed to write credentials to {}: {e}", path.display() )) })?; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt as _; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).ok(); - } tracing::debug!(path = %path.display(), "token saved to file fallback"); Ok(()) } @@ -331,10 +352,8 @@ impl PkceAuthProvider { if let Ok(entry) = keyring::Entry::new(&self.keychain_service(env), self.keychain_user()) { drop(entry.delete_credential()); } - if self.allow_file_fallback { - if let Some(path) = self.credential_file_path(env) { - drop(std::fs::remove_file(path)); - } + if let Some(path) = self.credential_file_path(env) { + drop(std::fs::remove_file(path)); } } @@ -807,6 +826,32 @@ mod tests { ); } + #[test] + fn credential_file_path_uses_xdg_config_home() { + let dir = std::env::temp_dir().join("cli-engine-test-xdg-pkce"); + std::env::set_var("XDG_CONFIG_HOME", &dir); + let provider = test_provider(); + let path = provider.credential_file_path("prod"); + std::env::remove_var("XDG_CONFIG_HOME"); + assert_eq!( + path, + Some(dir.join("test").join("credentials").join("test-prod.json")) + ); + } + + #[test] + fn credential_file_path_with_app_id_uses_app_id_as_dir() { + let dir = std::env::temp_dir().join("cli-engine-test-xdg-pkce-appid"); + std::env::set_var("XDG_CONFIG_HOME", &dir); + let provider = test_provider().with_app_id("myapp"); + let path = provider.credential_file_path("prod"); + std::env::remove_var("XDG_CONFIG_HOME"); + assert_eq!( + path, + Some(dir.join("myapp").join("credentials").join("test-prod.json")) + ); + } + /// resolve_token must return a pre-seeded in-memory token without /// triggering the PKCE browser flow (which would require a port and browser). /// This also exercises the cache-hit path that follows token persistence. From b079d33812eb3066955d8121a46e91464797828f Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 12:21:31 -0700 Subject: [PATCH 03/23] Path safety --- Cargo.toml | 8 ++--- src/auth/pkce.rs | 85 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 73 insertions(+), 20 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fe848cd..560a375 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ path = "src/lib.rs" [dependencies] async-trait = "0.1.89" base64 = "0.22.1" -keyring = { version = "3.6.1", optional = true } +keyring = { version = "3.6.1", optional = true, default-features = false } open = { version = "5.3.2", optional = true } rand = { version = "0.9", optional = true } sha2 = { version = "0.10.9", optional = true } @@ -38,13 +38,13 @@ tokio = { version = "1.48.0", features = ["fs", "io-std", "io-util", "macros", " tracing = "0.1.43" [target.'cfg(target_os = "linux")'.dependencies] -keyring = { version = "3.6.1", optional = true, features = ["async-secret-service", "async-io", "crypto-rust"] } +keyring = { version = "3.6.1", optional = true, default-features = false, features = ["async-secret-service", "async-io", "crypto-rust"] } [target.'cfg(target_os = "macos")'.dependencies] -keyring = { version = "3.6.1", optional = true, features = ["apple-native"] } +keyring = { version = "3.6.1", optional = true, default-features = false, features = ["apple-native"] } [target.'cfg(windows)'.dependencies] -keyring = { version = "3.6.1", optional = true, features = ["windows-native"] } +keyring = { version = "3.6.1", optional = true, default-features = false, features = ["windows-native"] } [features] pkce-auth = ["dep:keyring", "dep:open", "dep:rand", "dep:sha2", "dep:url", "dep:zeroize"] diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index d659bd6..7319e1c 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -240,6 +240,20 @@ impl PkceAuthProvider { } else { &self.app_id }; + // Reject components that contain path separators or are dot-sequences to + // prevent directory traversal when env or app_id comes from untrusted input. + if !is_safe_path_component(app) + || !is_safe_path_component(&self.name) + || !is_safe_path_component(env) + { + tracing::warn!( + app, + name = self.name, + env, + "refusing credential path with unsafe component" + ); + return None; + } let base = std::env::var("XDG_CONFIG_HOME") .map(std::path::PathBuf::from) .or_else(|_| std::env::var("HOME").map(|h| std::path::PathBuf::from(h).join(".config"))) @@ -336,6 +350,9 @@ impl PkceAuthProvider { path.display() )) })?; + // Correct permissions even when the file pre-existed with broader mode. + use std::os::unix::fs::PermissionsExt as _; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).ok(); } #[cfg(not(unix))] std::fs::write(&path, &json).map_err(|e| { @@ -561,6 +578,14 @@ fn random_state() -> String { URL_SAFE_NO_PAD.encode(bytes) } +/// Returns true only when `s` is a single, non-traversal path component. +/// +/// Rejects empty strings, dot-sequences (`..`, `.`), and strings that contain +/// any path separator (both `/` and `\` so the check is portable). +fn is_safe_path_component(s: &str) -> bool { + !s.is_empty() && s != ".." && s != "." && !s.contains('/') && !s.contains('\\') +} + /// Waits for the OAuth callback on the given listener, validates state and path. /// /// Accepts connections in a loop so that stray connections (port scanners, @@ -686,6 +711,21 @@ async fn parse_token_response(response: reqwest::Response, _env: &str) -> Result mod tests { use super::*; + /// Serialises access to XDG_CONFIG_HOME (and restores it) so env-var tests + /// cannot race each other when the test runner spawns multiple threads. + static XDG_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn with_xdg_config_home(value: &std::path::Path, f: F) { + let _guard = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let prev = std::env::var("XDG_CONFIG_HOME").ok(); + std::env::set_var("XDG_CONFIG_HOME", value); + f(); + match prev { + Some(v) => std::env::set_var("XDG_CONFIG_HOME", v), + None => std::env::remove_var("XDG_CONFIG_HOME"), + } + } + fn test_provider() -> PkceAuthProvider { PkceAuthProvider::new( "test", @@ -829,27 +869,40 @@ mod tests { #[test] fn credential_file_path_uses_xdg_config_home() { let dir = std::env::temp_dir().join("cli-engine-test-xdg-pkce"); - std::env::set_var("XDG_CONFIG_HOME", &dir); - let provider = test_provider(); - let path = provider.credential_file_path("prod"); - std::env::remove_var("XDG_CONFIG_HOME"); - assert_eq!( - path, - Some(dir.join("test").join("credentials").join("test-prod.json")) - ); + with_xdg_config_home(&dir, || { + let path = test_provider().credential_file_path("prod"); + assert_eq!( + path, + Some(dir.join("test").join("credentials").join("test-prod.json")) + ); + }); } #[test] fn credential_file_path_with_app_id_uses_app_id_as_dir() { let dir = std::env::temp_dir().join("cli-engine-test-xdg-pkce-appid"); - std::env::set_var("XDG_CONFIG_HOME", &dir); - let provider = test_provider().with_app_id("myapp"); - let path = provider.credential_file_path("prod"); - std::env::remove_var("XDG_CONFIG_HOME"); - assert_eq!( - path, - Some(dir.join("myapp").join("credentials").join("test-prod.json")) - ); + with_xdg_config_home(&dir, || { + let path = test_provider() + .with_app_id("myapp") + .credential_file_path("prod"); + assert_eq!( + path, + Some(dir.join("myapp").join("credentials").join("test-prod.json")) + ); + }); + } + + #[test] + fn credential_file_path_rejects_traversal_in_env() { + let dir = std::env::temp_dir().join("cli-engine-test-xdg-traversal"); + with_xdg_config_home(&dir, || { + assert_eq!( + test_provider().credential_file_path("../../etc/passwd"), + None + ); + assert_eq!(test_provider().credential_file_path("dev/subdir"), None); + assert_eq!(test_provider().credential_file_path(".."), None); + }); } /// resolve_token must return a pre-seeded in-memory token without From 9de8dd4e82452564331daeff4cb454ef3819587b Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 12:59:15 -0700 Subject: [PATCH 04/23] Address various home/XDG things --- src/auth/pkce.rs | 51 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 7319e1c..0fe07d9 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -255,10 +255,21 @@ impl PkceAuthProvider { return None; } let base = std::env::var("XDG_CONFIG_HOME") + .ok() + .filter(|v| !v.is_empty()) .map(std::path::PathBuf::from) - .or_else(|_| std::env::var("HOME").map(|h| std::path::PathBuf::from(h).join(".config"))) - .or_else(|_| std::env::var("APPDATA").map(std::path::PathBuf::from)) - .ok()?; + .or_else(|| { + std::env::var("HOME") + .ok() + .filter(|v| !v.is_empty()) + .map(|h| std::path::PathBuf::from(h).join(".config")) + }) + .or_else(|| { + std::env::var("APPDATA") + .ok() + .filter(|v| !v.is_empty()) + .map(std::path::PathBuf::from) + })?; Some( base.join(app) .join("credentials") @@ -352,7 +363,14 @@ impl PkceAuthProvider { })?; // Correct permissions even when the file pre-existed with broader mode. use std::os::unix::fs::PermissionsExt as _; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).ok(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).map_err( + |e| { + CliCoreError::message(format!( + "failed to set credentials file permissions on {}: {e}", + path.display() + )) + }, + )?; } #[cfg(not(unix))] std::fs::write(&path, &json).map_err(|e| { @@ -715,15 +733,30 @@ mod tests { /// cannot race each other when the test runner spawns multiple threads. static XDG_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// RAII guard that restores an env var when dropped, including on panic. + struct EnvVarGuard { + key: &'static str, + prev: Option, + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + match self.prev.take() { + Some(v) => std::env::set_var(self.key, v), + None => std::env::remove_var(self.key), + } + } + } + fn with_xdg_config_home(value: &std::path::Path, f: F) { - let _guard = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let _lock = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); let prev = std::env::var("XDG_CONFIG_HOME").ok(); std::env::set_var("XDG_CONFIG_HOME", value); + let _restore = EnvVarGuard { + key: "XDG_CONFIG_HOME", + prev, + }; f(); - match prev { - Some(v) => std::env::set_var("XDG_CONFIG_HOME", v), - None => std::env::remove_var("XDG_CONFIG_HOME"), - } } fn test_provider() -> PkceAuthProvider { From 69c560cf0ddd76e679d48006233fc35ef690440f Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 14:10:35 -0700 Subject: [PATCH 05/23] Pure rust keychain --- Cargo.lock | 2 + Cargo.toml | 2 +- src/auth/pkce.rs | 140 +++++++++++++++++++++++++++++++++-------------- 3 files changed, 102 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fe5e1c0..46dcfc2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2132,6 +2132,7 @@ dependencies = [ "signal-hook-registry", "socket2", "tokio-macros", + "tracing", "windows-sys 0.61.2", ] @@ -2891,6 +2892,7 @@ dependencies = [ "serde_repr", "sha1", "static_assertions", + "tokio", "tracing", "uds_windows", "windows-sys 0.52.0", diff --git a/Cargo.toml b/Cargo.toml index 560a375..9c69135 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,7 +38,7 @@ tokio = { version = "1.48.0", features = ["fs", "io-std", "io-util", "macros", " tracing = "0.1.43" [target.'cfg(target_os = "linux")'.dependencies] -keyring = { version = "3.6.1", optional = true, default-features = false, features = ["async-secret-service", "async-io", "crypto-rust"] } +keyring = { version = "3.6.1", optional = true, default-features = false, features = ["async-secret-service", "tokio", "crypto-rust"] } [target.'cfg(target_os = "macos")'.dependencies] keyring = { version = "3.6.1", optional = true, default-features = false, features = ["apple-native"] } diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 0fe07d9..ce18749 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -277,52 +277,103 @@ impl PkceAuthProvider { ) } - fn load_token_from_keychain(&self, env: &str) -> Option { + async fn load_token_from_keychain(&self, env: &str) -> Option { let service = self.keychain_service(env); - match keyring::Entry::new(&service, self.keychain_user()) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed"); - } - Ok(entry) => match entry.get_password() { - Err(keyring::Error::NoEntry) => { - tracing::debug!(service, "no stored token in keychain"); + let user = self.keychain_user().to_owned(); + + // Run keyring I/O on a blocking thread. zbus's tokio transport internally + // calls block_on, which panics if invoked from within an active runtime. + let json_opt = tokio::task::spawn_blocking({ + let service = service.clone(); + move || -> Option { + match keyring::Entry::new(&service, &user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + None + } + Ok(entry) => match entry.get_password() { + Err(keyring::Error::NoEntry) => { + tracing::debug!(service, "no stored token in keychain"); + None + } + Err(e) => { + tracing::warn!(service, error = %e, "keychain read failed"); + None + } + Ok(json) => Some(json), + }, } + } + }) + .await + .ok() + .flatten(); + + if let Some(json) = json_opt { + match serde_json::from_str::(&json) { + Ok(token) => return Some(token), Err(e) => { - tracing::warn!(service, error = %e, "keychain read failed"); + tracing::warn!(service, error = %e, "keychain token JSON invalid"); } - Ok(json) => match serde_json::from_str(&json) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain token JSON invalid"); - } - Ok(token) => return Some(token), - }, - }, + } } + if !self.allow_file_fallback { return None; } let path = self.credential_file_path(env)?; - let json = std::fs::read_to_string(&path).ok()?; - tracing::debug!(path = %path.display(), "loaded token from file fallback"); - serde_json::from_str(&json).ok() + let json = match std::fs::read_to_string(&path) { + Ok(s) => s, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "file fallback read failed"); + return None; + } + }; + match serde_json::from_str(&json) { + Ok(token) => { + tracing::debug!(path = %path.display(), "loaded token from file fallback"); + Some(token) + } + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "file fallback token JSON invalid"); + None + } + } } - fn save_token_to_keychain(&self, env: &str, token: &StoredToken) -> Result<()> { + async fn save_token_to_keychain(&self, env: &str, token: &StoredToken) -> Result<()> { let json = serde_json::to_string(token).map_err(CliCoreError::from)?; let service = self.keychain_service(env); - match keyring::Entry::new(&service, self.keychain_user()) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed"); - } - Ok(entry) => match entry.set_password(&json) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain write failed"); - } - Ok(()) => { - tracing::debug!(service, "token saved to keychain"); - return Ok(()); + let user = self.keychain_user().to_owned(); + + let keychain_saved = tokio::task::spawn_blocking({ + let service = service.clone(); + let json = json.clone(); + move || -> bool { + match keyring::Entry::new(&service, &user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + false + } + Ok(entry) => match entry.set_password(&json) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain write failed"); + false + } + Ok(()) => { + tracing::debug!(service, "token saved to keychain"); + true + } + }, } - }, + } + }) + .await + .unwrap_or(false); + + if keychain_saved { + return Ok(()); } if !self.allow_file_fallback { return Err(CliCoreError::message( @@ -383,10 +434,17 @@ impl PkceAuthProvider { Ok(()) } - fn delete_token_from_keychain(&self, env: &str) { - if let Ok(entry) = keyring::Entry::new(&self.keychain_service(env), self.keychain_user()) { - drop(entry.delete_credential()); - } + async fn delete_token_from_keychain(&self, env: &str) { + let service = self.keychain_service(env); + let user = self.keychain_user().to_owned(); + drop( + tokio::task::spawn_blocking(move || { + if let Ok(entry) = keyring::Entry::new(&service, &user) { + drop(entry.delete_credential()); + } + }) + .await, + ); if let Some(path) = self.credential_file_path(env) { drop(std::fs::remove_file(path)); } @@ -406,7 +464,7 @@ impl PkceAuthProvider { if let Some(token) = self.cached_token(env).await { return Ok(token); } - if let Some(token) = self.load_token_from_keychain(env) { + if let Some(token) = self.load_token_from_keychain(env).await { if token.is_valid() { self.store_cached_token(env, token.clone()).await; return Ok(token); @@ -417,13 +475,13 @@ impl PkceAuthProvider { if refreshed.refresh_token.is_none() { refreshed.refresh_token = Some(refresh_token.to_owned()); } - self.save_token_to_keychain(env, &refreshed)?; + self.save_token_to_keychain(env, &refreshed).await?; self.store_cached_token(env, refreshed.clone()).await; return Ok(refreshed); } } let token = self.run_pkce_flow(env).await?; - self.save_token_to_keychain(env, &token)?; + self.save_token_to_keychain(env, &token).await?; self.store_cached_token(env, token.clone()).await; Ok(token) } @@ -551,7 +609,7 @@ impl AuthProvider for PkceAuthProvider { } async fn status(&self, env: &str) -> Result { - let Some(token) = self.load_token_from_keychain(env) else { + let Some(token) = self.load_token_from_keychain(env).await else { return Err(CliCoreError::message(format!( "not logged in for environment {env:?}" ))); @@ -568,7 +626,7 @@ impl AuthProvider for PkceAuthProvider { } async fn logout(&self, env: &str) -> Result<()> { - self.delete_token_from_keychain(env); + self.delete_token_from_keychain(env).await; let mut cache = self.cache.write().await; cache.remove(env); Ok(()) From 2c80367157f354f849c30856d04dd9ef3f86e65d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 14:19:33 -0700 Subject: [PATCH 06/23] Async fixes --- src/auth/pkce.rs | 112 ++++++++++++++++++++++++++++------------------- 1 file changed, 66 insertions(+), 46 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index ce18749..d421f67 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -283,7 +283,7 @@ impl PkceAuthProvider { // Run keyring I/O on a blocking thread. zbus's tokio transport internally // calls block_on, which panics if invoked from within an active runtime. - let json_opt = tokio::task::spawn_blocking({ + let json_opt = match tokio::task::spawn_blocking({ let service = service.clone(); move || -> Option { match keyring::Entry::new(&service, &user) { @@ -306,8 +306,13 @@ impl PkceAuthProvider { } }) .await - .ok() - .flatten(); + { + Ok(opt) => opt, + Err(e) => { + tracing::warn!(service, error = %e, "keychain read task panicked"); + None + } + }; if let Some(json) = json_opt { match serde_json::from_str::(&json) { @@ -322,7 +327,7 @@ impl PkceAuthProvider { return None; } let path = self.credential_file_path(env)?; - let json = match std::fs::read_to_string(&path) { + let json = match tokio::fs::read_to_string(&path).await { Ok(s) => s, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, Err(e) => { @@ -347,7 +352,7 @@ impl PkceAuthProvider { let service = self.keychain_service(env); let user = self.keychain_user().to_owned(); - let keychain_saved = tokio::task::spawn_blocking({ + let keychain_saved = match tokio::task::spawn_blocking({ let service = service.clone(); let json = json.clone(); move || -> bool { @@ -370,7 +375,13 @@ impl PkceAuthProvider { } }) .await - .unwrap_or(false); + { + Ok(saved) => saved, + Err(e) => { + tracing::warn!(service, error = %e, "keychain write task panicked"); + false + } + }; if keychain_saved { return Ok(()); @@ -385,51 +396,60 @@ impl PkceAuthProvider { let path = self .credential_file_path(env) .ok_or_else(|| CliCoreError::message("could not determine credential file path"))?; - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).map_err(|e| { - CliCoreError::message(format!("failed to create credential directory: {e}")) - })?; - } - #[cfg(unix)] - { - use std::io::Write as _; - use std::os::unix::fs::OpenOptionsExt as _; - let mut file = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(&path) - .map_err(|e| { + tokio::task::spawn_blocking({ + let path = path.clone(); + move || -> Result<()> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).map_err(|e| { + CliCoreError::message(format!("failed to create credential directory: {e}")) + })?; + } + #[cfg(unix)] + { + use std::io::Write as _; + use std::os::unix::fs::OpenOptionsExt as _; + use std::os::unix::fs::PermissionsExt as _; + let mut file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(&path) + .map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + path.display() + )) + })?; + file.write_all(json.as_bytes()).map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + path.display() + )) + })?; + // Correct permissions even when the file pre-existed with broader mode. + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .map_err(|e| { + CliCoreError::message(format!( + "failed to set credentials file permissions on {}: {e}", + path.display() + )) + })?; + } + #[cfg(not(unix))] + std::fs::write(&path, &json).map_err(|e| { CliCoreError::message(format!( "failed to write credentials to {}: {e}", path.display() )) })?; - file.write_all(json.as_bytes()).map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - path.display() - )) - })?; - // Correct permissions even when the file pre-existed with broader mode. - use std::os::unix::fs::PermissionsExt as _; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).map_err( - |e| { - CliCoreError::message(format!( - "failed to set credentials file permissions on {}: {e}", - path.display() - )) - }, - )?; - } - #[cfg(not(unix))] - std::fs::write(&path, &json).map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - path.display() - )) - })?; + Ok(()) + } + }) + .await + .map_err(|e| { + CliCoreError::message(format!("credential file write task panicked: {e}")) + })??; tracing::debug!(path = %path.display(), "token saved to file fallback"); Ok(()) } From df56382940a9eca1bddfd155a8ceaf03655fbc50 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 14:31:08 -0700 Subject: [PATCH 07/23] Address Copilot feedback --- src/auth/pkce.rs | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index d421f67..f1cafe8 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -404,42 +404,40 @@ impl PkceAuthProvider { CliCoreError::message(format!("failed to create credential directory: {e}")) })?; } + let tmp_path = path.with_extension("tmp"); #[cfg(unix)] { use std::io::Write as _; use std::os::unix::fs::OpenOptionsExt as _; - use std::os::unix::fs::PermissionsExt as _; let mut file = std::fs::OpenOptions::new() .write(true) .create(true) .truncate(true) .mode(0o600) - .open(&path) + .open(&tmp_path) .map_err(|e| { CliCoreError::message(format!( "failed to write credentials to {}: {e}", - path.display() + tmp_path.display() )) })?; file.write_all(json.as_bytes()).map_err(|e| { CliCoreError::message(format!( "failed to write credentials to {}: {e}", - path.display() + tmp_path.display() )) })?; - // Correct permissions even when the file pre-existed with broader mode. - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .map_err(|e| { - CliCoreError::message(format!( - "failed to set credentials file permissions on {}: {e}", - path.display() - )) - })?; } #[cfg(not(unix))] - std::fs::write(&path, &json).map_err(|e| { + std::fs::write(&tmp_path, &json).map_err(|e| { CliCoreError::message(format!( "failed to write credentials to {}: {e}", + tmp_path.display() + )) + })?; + std::fs::rename(&tmp_path, &path).map_err(|e| { + CliCoreError::message(format!( + "failed to finalize credential file {}: {e}", path.display() )) })?; @@ -466,7 +464,13 @@ impl PkceAuthProvider { .await, ); if let Some(path) = self.credential_file_path(env) { - drop(std::fs::remove_file(path)); + match tokio::fs::remove_file(&path).await { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "failed to delete credential file"); + } + } } } @@ -679,7 +683,9 @@ fn random_state() -> String { /// Rejects empty strings, dot-sequences (`..`, `.`), and strings that contain /// any path separator (both `/` and `\` so the check is portable). fn is_safe_path_component(s: &str) -> bool { - !s.is_empty() && s != ".." && s != "." && !s.contains('/') && !s.contains('\\') + let mut components = std::path::Path::new(s).components(); + matches!(components.next(), Some(std::path::Component::Normal(_))) + && components.next().is_none() } /// Waits for the OAuth callback on the given listener, validates state and path. From d9121a4bfa13f7f0385b6e6e9ecc2d4e2100028d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 14:42:23 -0700 Subject: [PATCH 08/23] Better temp file implementation --- src/auth/pkce.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index f1cafe8..7c8b8d6 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -404,7 +404,11 @@ impl PkceAuthProvider { CliCoreError::message(format!("failed to create credential directory: {e}")) })?; } - let tmp_path = path.with_extension("tmp"); + let rand_id = rand::random::(); + let tmp_path = path.with_file_name(format!( + "{}.{rand_id:08x}.tmp", + path.file_stem().and_then(|s| s.to_str()).unwrap_or("cred"), + )); #[cfg(unix)] { use std::io::Write as _; From 5108f4d4cfad2bb815009c9a26e71d7a5ce2e7ba Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 15:00:27 -0700 Subject: [PATCH 09/23] Slash fix --- src/auth/pkce.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 7c8b8d6..a5a5de2 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -684,9 +684,14 @@ fn random_state() -> String { /// Returns true only when `s` is a single, non-traversal path component. /// -/// Rejects empty strings, dot-sequences (`..`, `.`), and strings that contain -/// any path separator (both `/` and `\` so the check is portable). +/// Rejects empty strings, dot-sequences (`..`, `.`), strings that contain `/`, +/// and strings that contain `\` (rejected explicitly because on Unix `\\` is a +/// valid filename character and `Path::components()` would accept it as a single +/// `Normal` component, defeating the traversal guard on Windows-style inputs). fn is_safe_path_component(s: &str) -> bool { + if s.contains('\\') { + return false; + } let mut components = std::path::Path::new(s).components(); matches!(components.next(), Some(std::path::Component::Normal(_))) && components.next().is_none() @@ -1022,6 +1027,7 @@ mod tests { None ); assert_eq!(test_provider().credential_file_path("dev/subdir"), None); + assert_eq!(test_provider().credential_file_path("dev\\subdir"), None); assert_eq!(test_provider().credential_file_path(".."), None); }); } From 7c5d808c6a6cd6c36b6530cd2773f91dee58382e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 15:48:32 -0700 Subject: [PATCH 10/23] Address the legitimate concern --- src/auth/pkce.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index a5a5de2..c2d3c2f 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -309,7 +309,12 @@ impl PkceAuthProvider { { Ok(opt) => opt, Err(e) => { - tracing::warn!(service, error = %e, "keychain read task panicked"); + let reason = if e.is_cancelled() { + "cancelled" + } else { + "panicked" + }; + tracing::warn!(service, error = %e, reason, "keychain read task failed"); None } }; @@ -378,7 +383,12 @@ impl PkceAuthProvider { { Ok(saved) => saved, Err(e) => { - tracing::warn!(service, error = %e, "keychain write task panicked"); + let reason = if e.is_cancelled() { + "cancelled" + } else { + "panicked" + }; + tracing::warn!(service, error = %e, reason, "keychain write task failed"); false } }; @@ -450,7 +460,14 @@ impl PkceAuthProvider { }) .await .map_err(|e| { - CliCoreError::message(format!("credential file write task panicked: {e}")) + CliCoreError::message(format!( + "credential file write task {}: {e}", + if e.is_cancelled() { + "cancelled" + } else { + "panicked" + } + )) })??; tracing::debug!(path = %path.display(), "token saved to file fallback"); Ok(()) From a33e4bbe22fa032b62e443f6da181c18e0e44e52 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 16:09:03 -0700 Subject: [PATCH 11/23] Copilot suggestions --- src/auth/pkce.rs | 54 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index c2d3c2f..9b414fd 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -259,16 +259,36 @@ impl PkceAuthProvider { .filter(|v| !v.is_empty()) .map(std::path::PathBuf::from) .or_else(|| { - std::env::var("HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(|h| std::path::PathBuf::from(h).join(".config")) - }) - .or_else(|| { - std::env::var("APPDATA") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) + // On Windows prefer APPDATA over HOME/.config: HOME is often set + // by Git Bash/MSYS shells and would place credentials in a + // non-standard location. On all other platforms keep the + // XDG-conventional HOME/.config first. + #[cfg(windows)] + { + std::env::var("APPDATA") + .ok() + .filter(|v| !v.is_empty()) + .map(std::path::PathBuf::from) + .or_else(|| { + std::env::var("HOME") + .ok() + .filter(|v| !v.is_empty()) + .map(|h| std::path::PathBuf::from(h).join(".config")) + }) + } + #[cfg(not(windows))] + { + std::env::var("HOME") + .ok() + .filter(|v| !v.is_empty()) + .map(|h| std::path::PathBuf::from(h).join(".config")) + .or_else(|| { + std::env::var("APPDATA") + .ok() + .filter(|v| !v.is_empty()) + .map(std::path::PathBuf::from) + }) + } })?; Some( base.join(app) @@ -476,14 +496,12 @@ impl PkceAuthProvider { async fn delete_token_from_keychain(&self, env: &str) { let service = self.keychain_service(env); let user = self.keychain_user().to_owned(); - drop( - tokio::task::spawn_blocking(move || { - if let Ok(entry) = keyring::Entry::new(&service, &user) { - drop(entry.delete_credential()); - } - }) - .await, - ); + let _ = tokio::task::spawn_blocking(move || { + if let Ok(entry) = keyring::Entry::new(&service, &user) { + let _ = entry.delete_credential(); + } + }) + .await; if let Some(path) = self.credential_file_path(env) { match tokio::fs::remove_file(&path).await { Ok(()) => {} From fb21b423d38010df0b458ea6a35abc43d6792fdc Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 16:34:21 -0700 Subject: [PATCH 12/23] More FS junk --- src/auth/pkce.rs | 298 ++++++++++++++++++++++++----------------------- 1 file changed, 154 insertions(+), 144 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 9b414fd..c6ab985 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -240,8 +240,6 @@ impl PkceAuthProvider { } else { &self.app_id }; - // Reject components that contain path separators or are dot-sequences to - // prevent directory traversal when env or app_id comes from untrusted input. if !is_safe_path_component(app) || !is_safe_path_component(&self.name) || !is_safe_path_component(env) @@ -254,42 +252,7 @@ impl PkceAuthProvider { ); return None; } - let base = std::env::var("XDG_CONFIG_HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - // On Windows prefer APPDATA over HOME/.config: HOME is often set - // by Git Bash/MSYS shells and would place credentials in a - // non-standard location. On all other platforms keep the - // XDG-conventional HOME/.config first. - #[cfg(windows)] - { - std::env::var("APPDATA") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - std::env::var("HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(|h| std::path::PathBuf::from(h).join(".config")) - }) - } - #[cfg(not(windows))] - { - std::env::var("HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(|h| std::path::PathBuf::from(h).join(".config")) - .or_else(|| { - std::env::var("APPDATA") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - }) - } - })?; + let base = config_base_dir()?; Some( base.join(app) .join("credentials") @@ -301,29 +264,9 @@ impl PkceAuthProvider { let service = self.keychain_service(env); let user = self.keychain_user().to_owned(); - // Run keyring I/O on a blocking thread. zbus's tokio transport internally - // calls block_on, which panics if invoked from within an active runtime. let json_opt = match tokio::task::spawn_blocking({ let service = service.clone(); - move || -> Option { - match keyring::Entry::new(&service, &user) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed"); - None - } - Ok(entry) => match entry.get_password() { - Err(keyring::Error::NoEntry) => { - tracing::debug!(service, "no stored token in keychain"); - None - } - Err(e) => { - tracing::warn!(service, error = %e, "keychain read failed"); - None - } - Ok(json) => Some(json), - }, - } - } + move || keychain_read_blocking(&service, &user) }) .await { @@ -352,24 +295,7 @@ impl PkceAuthProvider { return None; } let path = self.credential_file_path(env)?; - let json = match tokio::fs::read_to_string(&path).await { - Ok(s) => s, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "file fallback read failed"); - return None; - } - }; - match serde_json::from_str(&json) { - Ok(token) => { - tracing::debug!(path = %path.display(), "loaded token from file fallback"); - Some(token) - } - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "file fallback token JSON invalid"); - None - } - } + load_token_from_file(&path).await } async fn save_token_to_keychain(&self, env: &str, token: &StoredToken) -> Result<()> { @@ -380,24 +306,7 @@ impl PkceAuthProvider { let keychain_saved = match tokio::task::spawn_blocking({ let service = service.clone(); let json = json.clone(); - move || -> bool { - match keyring::Entry::new(&service, &user) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed"); - false - } - Ok(entry) => match entry.set_password(&json) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain write failed"); - false - } - Ok(()) => { - tracing::debug!(service, "token saved to keychain"); - true - } - }, - } - } + move || keychain_write_blocking(&service, &user, &json) }) .await { @@ -428,55 +337,7 @@ impl PkceAuthProvider { .ok_or_else(|| CliCoreError::message("could not determine credential file path"))?; tokio::task::spawn_blocking({ let path = path.clone(); - move || -> Result<()> { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).map_err(|e| { - CliCoreError::message(format!("failed to create credential directory: {e}")) - })?; - } - let rand_id = rand::random::(); - let tmp_path = path.with_file_name(format!( - "{}.{rand_id:08x}.tmp", - path.file_stem().and_then(|s| s.to_str()).unwrap_or("cred"), - )); - #[cfg(unix)] - { - use std::io::Write as _; - use std::os::unix::fs::OpenOptionsExt as _; - let mut file = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(&tmp_path) - .map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - tmp_path.display() - )) - })?; - file.write_all(json.as_bytes()).map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - tmp_path.display() - )) - })?; - } - #[cfg(not(unix))] - std::fs::write(&tmp_path, &json).map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - tmp_path.display() - )) - })?; - std::fs::rename(&tmp_path, &path).map_err(|e| { - CliCoreError::message(format!( - "failed to finalize credential file {}: {e}", - path.display() - )) - })?; - Ok(()) - } + move || write_token_file_blocking(path, json) }) .await .map_err(|e| { @@ -717,6 +578,155 @@ fn random_state() -> String { URL_SAFE_NO_PAD.encode(bytes) } +/// Resolves the base config directory from environment variables. +fn config_base_dir() -> Option { + std::env::var("XDG_CONFIG_HOME") + .ok() + .filter(|v| !v.is_empty()) + .map(std::path::PathBuf::from) + .or_else(|| { + // On Windows prefer APPDATA over HOME/.config: HOME is often set by + // Git Bash/MSYS shells and would place credentials in a non-standard + // location. On all other platforms keep XDG-conventional HOME/.config. + #[cfg(windows)] + { + std::env::var("APPDATA") + .ok() + .filter(|v| !v.is_empty()) + .map(std::path::PathBuf::from) + .or_else(|| { + std::env::var("HOME") + .ok() + .filter(|v| !v.is_empty()) + .map(|h| std::path::PathBuf::from(h).join(".config")) + }) + } + #[cfg(not(windows))] + { + std::env::var("HOME") + .ok() + .filter(|v| !v.is_empty()) + .map(|h| std::path::PathBuf::from(h).join(".config")) + .or_else(|| { + std::env::var("APPDATA") + .ok() + .filter(|v| !v.is_empty()) + .map(std::path::PathBuf::from) + }) + } + }) +} + +/// Reads a token JSON string from the system keychain. Sync; call inside `spawn_blocking`. +fn keychain_read_blocking(service: &str, user: &str) -> Option { + match keyring::Entry::new(service, user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + None + } + Ok(entry) => match entry.get_password() { + Err(keyring::Error::NoEntry) => { + tracing::debug!(service, "no stored token in keychain"); + None + } + Err(e) => { + tracing::warn!(service, error = %e, "keychain read failed"); + None + } + Ok(json) => Some(json), + }, + } +} + +/// Writes a token JSON string to the system keychain. Sync; call inside `spawn_blocking`. +fn keychain_write_blocking(service: &str, user: &str, json: &str) -> bool { + match keyring::Entry::new(service, user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + false + } + Ok(entry) => match entry.set_password(json) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain write failed"); + false + } + Ok(()) => { + tracing::debug!(service, "token saved to keychain"); + true + } + }, + } +} + +/// Reads and parses a [`StoredToken`] from the file fallback path. +async fn load_token_from_file(path: &std::path::Path) -> Option { + let json = match tokio::fs::read_to_string(path).await { + Ok(s) => s, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "file fallback read failed"); + return None; + } + }; + match serde_json::from_str(&json) { + Ok(token) => { + tracing::debug!(path = %path.display(), "loaded token from file fallback"); + Some(token) + } + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "file fallback token JSON invalid"); + None + } + } +} + +/// Writes `json` atomically to `path` via a uniquely-named temp file. +/// Sync; call inside `spawn_blocking`. +fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<()> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).map_err(|e| { + CliCoreError::message(format!("failed to create credential directory: {e}")) + })?; + } + let rand_id = rand::random::(); + let tmp_path = path.with_file_name(format!( + "{}.{rand_id:08x}.tmp", + path.file_stem().and_then(|s| s.to_str()).unwrap_or("cred"), + )); + write_token_tmp(&tmp_path, &json)?; + std::fs::rename(&tmp_path, &path).map_err(|e| { + CliCoreError::message(format!( + "failed to finalize credential file {}: {e}", + path.display() + )) + }) +} + +/// Opens `tmp_path` with `O_CREAT|O_EXCL` and writes `json`. +/// Sets mode `0o600` on Unix so credentials are never world-readable. +fn write_token_tmp(tmp_path: &std::path::Path, json: &str) -> Result<()> { + use std::io::Write as _; + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + opts.mode(0o600); + } + let mut file = opts.open(tmp_path).map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + tmp_path.display() + )) + })?; + file.write_all(json.as_bytes()).map_err(|e| { + CliCoreError::message(format!( + "failed to write credentials to {}: {e}", + tmp_path.display() + )) + }) +} + /// Returns true only when `s` is a single, non-traversal path component. /// /// Rejects empty strings, dot-sequences (`..`, `.`), strings that contain `/`, From a39162e67d31928b1f2e83834bdc47dfaec348f2 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 16:47:13 -0700 Subject: [PATCH 13/23] More --- src/auth/pkce.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index c6ab985..a4836d2 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -680,8 +680,10 @@ async fn load_token_from_file(path: &std::path::Path) -> Option { } } -/// Writes `json` atomically to `path` via a uniquely-named temp file. -/// Sync; call inside `spawn_blocking`. +/// Writes `json` to `path` via a uniquely-named temp file then renames it into place. +/// On Unix the rename is atomic. On Windows it is best-effort (`MoveFileExW` with +/// `MOVEFILE_REPLACE_EXISTING`): it replaces an existing destination but is not +/// crash-atomic. Sync; call inside `spawn_blocking`. fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<()> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent).map_err(|e| { @@ -727,14 +729,17 @@ fn write_token_tmp(tmp_path: &std::path::Path, json: &str) -> Result<()> { }) } -/// Returns true only when `s` is a single, non-traversal path component. +/// Returns true only when `s` is a single, non-traversal path component that is +/// valid on all supported platforms. /// -/// Rejects empty strings, dot-sequences (`..`, `.`), strings that contain `/`, -/// and strings that contain `\` (rejected explicitly because on Unix `\\` is a -/// valid filename character and `Path::components()` would accept it as a single -/// `Normal` component, defeating the traversal guard on Windows-style inputs). +/// Rejects: +/// - empty strings, `.`, and `..` +/// - strings containing `/` or `\` (path separators on any platform) +/// - Windows-forbidden filename characters: `: * ? " < > |` +/// - ASCII control characters (bytes 0x00–0x1F) fn is_safe_path_component(s: &str) -> bool { - if s.contains('\\') { + const WINDOWS_FORBIDDEN: &[char] = &['\\', ':', '*', '?', '"', '<', '>', '|']; + if s.contains(WINDOWS_FORBIDDEN) || s.bytes().any(|b| b < 0x20) { return false; } let mut components = std::path::Path::new(s).components(); From ac1f5c802e128fb5ae43d63eb62ad2aab7ac5b43 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 16:57:12 -0700 Subject: [PATCH 14/23] Yeesh, more --- src/auth/pkce.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index a4836d2..af382b4 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -689,6 +689,11 @@ fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<( std::fs::create_dir_all(parent).map_err(|e| { CliCoreError::message(format!("failed to create credential directory: {e}")) })?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let _ = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)); + } } let rand_id = rand::random::(); let tmp_path = path.with_file_name(format!( @@ -738,8 +743,11 @@ fn write_token_tmp(tmp_path: &std::path::Path, json: &str) -> Result<()> { /// - Windows-forbidden filename characters: `: * ? " < > |` /// - ASCII control characters (bytes 0x00–0x1F) fn is_safe_path_component(s: &str) -> bool { - const WINDOWS_FORBIDDEN: &[char] = &['\\', ':', '*', '?', '"', '<', '>', '|']; - if s.contains(WINDOWS_FORBIDDEN) || s.bytes().any(|b| b < 0x20) { + // '/' is listed explicitly because Path::components() silently strips trailing + // slashes — "prod/" parses as a single Normal("prod") component and would + // otherwise pass the components check below. + const FORBIDDEN: &[char] = &['/', '\\', ':', '*', '?', '"', '<', '>', '|']; + if s.contains(FORBIDDEN) || s.bytes().any(|b| b < 0x20) { return false; } let mut components = std::path::Path::new(s).components(); From 2b80edd97e8f7dc035dd58181d12f25a586db99c Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 17:08:39 -0700 Subject: [PATCH 15/23] Build pkce-auth in CI --- .github/workflows/ci.yml | 6 +++--- src/auth/pkce.rs | 36 +++++++++++++++++++++++++----------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d6e5afa..acf9b02 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,13 +23,13 @@ jobs: run: cargo fmt --all --check - name: Check - run: cargo check --all-targets + run: cargo check --all-targets && cargo check --all-targets --features pkce-auth - name: Clippy - run: cargo clippy --all-targets -- -D warnings + run: cargo clippy --all-targets -- -D warnings && cargo clippy --all-targets --features pkce-auth -- -D warnings - name: Test - run: cargo test --all-targets + run: cargo test --all-targets && cargo test --all-targets --features pkce-auth - name: Docs run: RUSTDOCFLAGS='-D warnings' cargo doc --no-deps diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index af382b4..4ee8b0a 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -357,12 +357,14 @@ impl PkceAuthProvider { async fn delete_token_from_keychain(&self, env: &str) { let service = self.keychain_service(env); let user = self.keychain_user().to_owned(); - let _ = tokio::task::spawn_blocking(move || { - if let Ok(entry) = keyring::Entry::new(&service, &user) { - let _ = entry.delete_credential(); - } - }) - .await; + drop( + tokio::task::spawn_blocking(move || { + if let Ok(entry) = keyring::Entry::new(&service, &user) { + drop(entry.delete_credential()); + } + }) + .await, + ); if let Some(path) = self.credential_file_path(env) { match tokio::fs::remove_file(&path).await { Ok(()) => {} @@ -692,7 +694,10 @@ fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<( #[cfg(unix)] { use std::os::unix::fs::PermissionsExt as _; - let _ = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)); + drop(std::fs::set_permissions( + parent, + std::fs::Permissions::from_mode(0o700), + )); } } let rand_id = rand::random::(); @@ -877,6 +882,9 @@ async fn parse_token_response(response: reqwest::Response, _env: &str) -> Result } #[cfg(test)] +// set_var/remove_var are unsafe in Rust 2024 edition. The XDG_MUTEX in this +// module serialises all access so usage here is data-race-free. +#[allow(unsafe_code)] mod tests { use super::*; @@ -892,9 +900,14 @@ mod tests { impl Drop for EnvVarGuard { fn drop(&mut self) { - match self.prev.take() { - Some(v) => std::env::set_var(self.key, v), - None => std::env::remove_var(self.key), + // SAFETY: the XDG_MUTEX held by with_xdg_config_home serialises all + // env-var access in this test module; no other thread touches these + // variables while the mutex is held. + unsafe { + match self.prev.take() { + Some(v) => std::env::set_var(self.key, v), + None => std::env::remove_var(self.key), + } } } } @@ -902,7 +915,8 @@ mod tests { fn with_xdg_config_home(value: &std::path::Path, f: F) { let _lock = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); let prev = std::env::var("XDG_CONFIG_HOME").ok(); - std::env::set_var("XDG_CONFIG_HOME", value); + // SAFETY: same as EnvVarGuard::drop — mutex held for the duration. + unsafe { std::env::set_var("XDG_CONFIG_HOME", value) }; let _restore = EnvVarGuard { key: "XDG_CONFIG_HOME", prev, From 069362cadc7e8e37064cebe275a0567095bfd3e2 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 17:21:46 -0700 Subject: [PATCH 16/23] fix: log directory chmod failures in write_token_file_blocking Replaces `drop(set_permissions(...))` with an explicit debug log so chmod failures on unusual filesystems are observable. The file itself is always created with 0o600 via O_EXCL, so a directory chmod failure is a defence-in-depth miss, not a confidentiality breach. Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 4ee8b0a..40394ee 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -694,10 +694,17 @@ fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<( #[cfg(unix)] { use std::os::unix::fs::PermissionsExt as _; - drop(std::fs::set_permissions( - parent, - std::fs::Permissions::from_mode(0o700), - )); + if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) + { + // The credential file itself is always 0o600; failing to + // restrict the parent directory is a defence-in-depth miss, + // not a confidentiality breach. + tracing::debug!( + path = %parent.display(), + error = %e, + "could not restrict credential directory permissions" + ); + } } } let rand_id = rand::random::(); From 1313f05c382b2556e050cd1d2ce6f52e91c49fee Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 17:48:11 -0700 Subject: [PATCH 17/23] fix: reject relative base dirs and log keychain delete JoinError - config_base_dir: add .filter(|p| p.is_absolute()) so a relative XDG_CONFIG_HOME/APPDATA/HOME does not silently place credentials relative to the current working directory - delete_token_from_keychain: handle JoinError with a WARN log, consistent with the read/write paths - Add test: credential_file_path_rejects_relative_base_dir Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 40394ee..b590286 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -357,14 +357,20 @@ impl PkceAuthProvider { async fn delete_token_from_keychain(&self, env: &str) { let service = self.keychain_service(env); let user = self.keychain_user().to_owned(); - drop( - tokio::task::spawn_blocking(move || { - if let Ok(entry) = keyring::Entry::new(&service, &user) { - drop(entry.delete_credential()); - } - }) - .await, - ); + if let Err(e) = tokio::task::spawn_blocking(move || { + if let Ok(entry) = keyring::Entry::new(&service, &user) { + drop(entry.delete_credential()); + } + }) + .await + { + let reason = if e.is_cancelled() { + "cancelled" + } else { + "panicked" + }; + tracing::warn!(error = %e, reason, "keychain delete task failed"); + } if let Some(path) = self.credential_file_path(env) { match tokio::fs::remove_file(&path).await { Ok(()) => {} @@ -617,6 +623,9 @@ fn config_base_dir() -> Option { }) } }) + // Reject relative paths: a relative XDG_CONFIG_HOME/APPDATA/HOME would + // silently place credentials relative to the current working directory. + .filter(|p| p.is_absolute()) } /// Reads a token JSON string from the system keychain. Sync; call inside `spawn_blocking`. @@ -1111,6 +1120,23 @@ mod tests { }); } + #[test] + fn credential_file_path_rejects_relative_base_dir() { + let _lock = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let prev = std::env::var("XDG_CONFIG_HOME").ok(); + // SAFETY: mutex held for the duration. + unsafe { std::env::set_var("XDG_CONFIG_HOME", ".") }; + let _restore = EnvVarGuard { + key: "XDG_CONFIG_HOME", + prev, + }; + assert_eq!( + test_provider().credential_file_path("prod"), + None, + "relative XDG_CONFIG_HOME should be rejected" + ); + } + /// resolve_token must return a pre-seeded in-memory token without /// triggering the PKCE browser flow (which would require a port and browser). /// This also exercises the cache-hit path that follows token persistence. From 9774289f7447a8065f12a5007ba35027defdbecd Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 18:12:05 -0700 Subject: [PATCH 18/23] fix: improve keychain error message and add file fallback tests - save_token_to_keychain: change "keychain is unavailable" to "failed to save token to keychain" so the message is accurate for all failure modes (permissions, locked, truly absent) - Add file_fallback_round_trip_write_then_read: exercises write_token_file_blocking + load_token_from_file together - Add file_fallback_invalid_json_returns_none: verifies corrupted credential files are handled gracefully Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index b590286..cd2c891 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -327,9 +327,9 @@ impl PkceAuthProvider { } if !self.allow_file_fallback { return Err(CliCoreError::message( - "keychain is unavailable and file fallback is disabled — \ - ensure your system keychain (e.g. gnome-keyring, macOS Keychain) \ - is running and unlocked", + "failed to save token to keychain and file fallback is disabled — \ + check logs for the underlying error, or ensure your system keychain \ + (e.g. gnome-keyring, macOS Keychain) is running and unlocked", )); } let path = self @@ -1137,6 +1137,31 @@ mod tests { ); } + #[tokio::test] + async fn file_fallback_round_trip_write_then_read() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("test-prod.json"); + let token = valid_token("file-token"); + let json = serde_json::to_string(&token).expect("serialize"); + + write_token_file_blocking(path.clone(), json).expect("write"); + + let loaded = load_token_from_file(&path).await; + assert_eq!(loaded.expect("token present").access_token, "file-token"); + } + + #[tokio::test] + async fn file_fallback_invalid_json_returns_none() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("bad.json"); + std::fs::write(&path, b"not-valid-json").expect("write"); + + assert!( + load_token_from_file(&path).await.is_none(), + "invalid JSON should return None" + ); + } + /// resolve_token must return a pre-seeded in-memory token without /// triggering the PKCE browser flow (which would require a port and browser). /// This also exercises the cache-hit path that follows token persistence. From 380b409a936c15e86515eab2fba45f5d82789731 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 18:24:19 -0700 Subject: [PATCH 19/23] fix: reject Windows reserved names and trailing dot/space in path components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_safe_path_component now also rejects: - trailing '.' or ' ' (valid on Unix but rejected by Windows) - Windows reserved device names (CON, NUL, COM1-9, LPT1-9 with or without extension) — opening e.g. NUL.json on Windows writes to the null device rather than a file Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index cd2c891..3b5e479 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -763,6 +763,8 @@ fn write_token_tmp(tmp_path: &std::path::Path, json: &str) -> Result<()> { /// - strings containing `/` or `\` (path separators on any platform) /// - Windows-forbidden filename characters: `: * ? " < > |` /// - ASCII control characters (bytes 0x00–0x1F) +/// - trailing `.` or space (valid on Unix but rejected by Windows) +/// - Windows reserved device names (`CON`, `NUL`, `COM1`, etc.) with or without extension fn is_safe_path_component(s: &str) -> bool { // '/' is listed explicitly because Path::components() silently strips trailing // slashes — "prod/" parses as a single Normal("prod") component and would @@ -771,6 +773,23 @@ fn is_safe_path_component(s: &str) -> bool { if s.contains(FORBIDDEN) || s.bytes().any(|b| b < 0x20) { return false; } + if s.ends_with('.') || s.ends_with(' ') { + return false; + } + // Windows treats these device names as special regardless of extension, + // e.g. opening "NUL.json" writes to the null device, not a file. + const RESERVED: &[&str] = &[ + "CON", "PRN", "AUX", "NUL", "COM0", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", + "COM8", "COM9", "LPT0", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", + "LPT9", + ]; + let stem = std::path::Path::new(s) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(s); + if RESERVED.iter().any(|r| stem.eq_ignore_ascii_case(r)) { + return false; + } let mut components = std::path::Path::new(s).components(); matches!(components.next(), Some(std::path::Component::Normal(_))) && components.next().is_none() @@ -1120,6 +1139,31 @@ mod tests { }); } + #[test] + fn is_safe_path_component_rejects_windows_reserved_names() { + for name in &[ + "CON", "con", "NUL", "nul", "COM1", "LPT9", "CON.txt", "NUL.json", + ] { + assert!( + !is_safe_path_component(name), + "{name:?} should be rejected as a Windows reserved name" + ); + } + } + + #[test] + fn is_safe_path_component_rejects_trailing_dot_and_space() { + assert!(!is_safe_path_component("prod.")); + assert!(!is_safe_path_component("prod ")); + } + + #[test] + fn is_safe_path_component_accepts_normal_values() { + for name in &["dev", "prod", "staging", "my-app", "my_app", "app.v2"] { + assert!(is_safe_path_component(name), "{name:?} should be accepted"); + } + } + #[test] fn credential_file_path_rejects_relative_base_dir() { let _lock = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); From 32c3da882a9c7b24a42097492e08034199fddc1a Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 18:30:53 -0700 Subject: [PATCH 20/23] fix: clean up temp credential file if rename fails If std::fs::rename fails the token data would otherwise sit in an orphaned *.tmp file. Now best-effort deletes the temp file before returning the error. Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 3b5e479..57e6fcb 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -722,12 +722,14 @@ fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<( path.file_stem().and_then(|s| s.to_str()).unwrap_or("cred"), )); write_token_tmp(&tmp_path, &json)?; - std::fs::rename(&tmp_path, &path).map_err(|e| { - CliCoreError::message(format!( + if let Err(e) = std::fs::rename(&tmp_path, &path) { + std::fs::remove_file(&tmp_path).ok(); + return Err(CliCoreError::message(format!( "failed to finalize credential file {}: {e}", path.display() - )) - }) + ))); + } + Ok(()) } /// Opens `tmp_path` with `O_CREAT|O_EXCL` and writes `json`. From 3684aae6011d23cd063fa7af586d3f626df8fa81 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 18:37:23 -0700 Subject: [PATCH 21/23] fix: clean up stale file fallback on keychain write success; fix comment - save_token_to_keychain: when keychain write succeeds, best-effort delete any leftover file-fallback token (NotFound is silently ignored; other errors logged at debug) - config_base_dir: update comment to accurately describe the non-Windows fallback order (HOME/.config first, then APPDATA as last resort) Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 57e6fcb..621507c 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -323,6 +323,19 @@ impl PkceAuthProvider { }; if keychain_saved { + // Best-effort: remove any stale file-fallback token now that the + // keychain is working. Ignore NotFound; the file may never have existed. + if let Some(path) = self.credential_file_path(env) { + match tokio::fs::remove_file(&path).await { + Ok(()) => { + tracing::debug!(path = %path.display(), "removed stale file fallback after keychain write"); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + tracing::debug!(path = %path.display(), error = %e, "could not remove stale file fallback"); + } + } + } return Ok(()); } if !self.allow_file_fallback { @@ -595,7 +608,8 @@ fn config_base_dir() -> Option { .or_else(|| { // On Windows prefer APPDATA over HOME/.config: HOME is often set by // Git Bash/MSYS shells and would place credentials in a non-standard - // location. On all other platforms keep XDG-conventional HOME/.config. + // location. On all other platforms prefer XDG-conventional HOME/.config, + // falling back to APPDATA as a last resort if HOME is unset. #[cfg(windows)] { std::env::var("APPDATA") From 3a03984915997fccbf8f502ab765865551579323 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 18:52:08 -0700 Subject: [PATCH 22/23] fix: log keychain delete errors; self-heal corrupt file fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - delete_token_from_keychain: replace drop(delete_credential()) with a match that logs WARN on unexpected errors (NoEntry is silently ignored — token already gone is fine during logout) - load_token_from_file: best-effort delete the file after logging invalid JSON so the system self-heals instead of forcing re-auth on every subsequent run Co-Authored-By: Claude Sonnet 4.6 --- src/auth/pkce.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 621507c..5998188 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -372,7 +372,12 @@ impl PkceAuthProvider { let user = self.keychain_user().to_owned(); if let Err(e) = tokio::task::spawn_blocking(move || { if let Ok(entry) = keyring::Entry::new(&service, &user) { - drop(entry.delete_credential()); + match entry.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => {} + Err(e) => { + tracing::warn!(service, error = %e, "keychain delete failed"); + } + } } }) .await @@ -700,6 +705,9 @@ async fn load_token_from_file(path: &std::path::Path) -> Option { } Err(e) => { tracing::warn!(path = %path.display(), error = %e, "file fallback token JSON invalid"); + // Best-effort delete: a permanently corrupt file causes repeated + // warnings and PKCE flows on every run until manually removed. + tokio::fs::remove_file(path).await.ok(); None } } From 41df0820290da48bd87134c2597571eedc168b9b Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 29 May 2026 19:06:28 -0700 Subject: [PATCH 23/23] fix: harden delete path diagnostics, self-heal corrupt keychain entry, fix CI doc tests - delete_token_from_keychain: clone service before closure so the JoinError warning includes the service name; log WARN when Entry::new fails (consistent with read/write helpers) - load_token_from_keychain: best-effort delete the keychain entry when its JSON is corrupt, preventing repeated warnings on every run - ci.yml: add --features pkce-auth to the Doc Tests step so the pkce module's no_run example is compile-checked in CI Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/ci.yml | 2 +- src/auth/pkce.rs | 31 +++++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index acf9b02..da19be3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,7 +35,7 @@ jobs: run: RUSTDOCFLAGS='-D warnings' cargo doc --no-deps - name: Doc Tests - run: cargo test --doc + run: cargo test --doc && cargo test --doc --features pkce-auth - name: Publish dry run run: cargo publish --dry-run diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 5998188..a5f53f6 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -287,6 +287,18 @@ impl PkceAuthProvider { Ok(token) => return Some(token), Err(e) => { tracing::warn!(service, error = %e, "keychain token JSON invalid"); + // Best-effort delete the corrupt entry so subsequent runs + // don't repeat the warning and fall through to re-auth. + let svc = service.clone(); + let usr = self.keychain_user().to_owned(); + drop( + tokio::task::spawn_blocking(move || { + if let Ok(entry) = keyring::Entry::new(&svc, &usr) { + drop(entry.delete_credential()); + } + }) + .await, + ); } } } @@ -370,24 +382,27 @@ impl PkceAuthProvider { async fn delete_token_from_keychain(&self, env: &str) { let service = self.keychain_service(env); let user = self.keychain_user().to_owned(); - if let Err(e) = tokio::task::spawn_blocking(move || { - if let Ok(entry) = keyring::Entry::new(&service, &user) { - match entry.delete_credential() { + let service_for_warn = service.clone(); + if let Err(e) = + tokio::task::spawn_blocking(move || match keyring::Entry::new(&service, &user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed on delete"); + } + Ok(entry) => match entry.delete_credential() { Ok(()) | Err(keyring::Error::NoEntry) => {} Err(e) => { tracing::warn!(service, error = %e, "keychain delete failed"); } - } - } - }) - .await + }, + }) + .await { let reason = if e.is_cancelled() { "cancelled" } else { "panicked" }; - tracing::warn!(error = %e, reason, "keychain delete task failed"); + tracing::warn!(service = service_for_warn, error = %e, reason, "keychain delete task failed"); } if let Some(path) = self.credential_file_path(env) { match tokio::fs::remove_file(&path).await {