Skip to content

Commit 8d30ab1

Browse files
ByronByron
authored
Merge pull request #1999 from GitoxideLabs/credential-helper-protocol-fix
fix #1998
2 parents c3f06ae + 6880175 commit 8d30ab1

File tree

15 files changed

+188
-37
lines changed

15 files changed

+188
-37
lines changed

‎Cargo.lock

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎gix-credentials/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ gix-path = { version = "^0.10.18", path = "../gix-path" }
2525
gix-command = { version = "^0.6.0", path = "../gix-command" }
2626
gix-config-value = { version = "^0.15.0", path = "../gix-config-value" }
2727
gix-prompt = { version = "^0.11.0", path = "../gix-prompt" }
28+
gix-date = { version = "^0.10.1", path = "../gix-date" }
2829
gix-trace = { version = "^0.1.12", path = "../gix-trace" }
2930

3031
thiserror = "2.0.0"

‎gix-credentials/src/helper/cascade.rs

+31-9
Original file line numberDiff line numberDiff line change
@@ -88,30 +88,51 @@ impl Cascade {
8888
match helper::invoke::raw(program, &action) {
8989
Ok(None) => {}
9090
Ok(Some(stdout)) => {
91-
let ctx = Context::from_bytes(&stdout)?;
91+
let Context {
92+
protocol,
93+
host,
94+
path,
95+
username,
96+
password,
97+
oauth_refresh_token,
98+
password_expiry_utc,
99+
url: ctx_url,
100+
quit,
101+
} = Context::from_bytes(&stdout)?;
92102
if let Some(dst_ctx) = action.context_mut() {
93-
if let Some(src) = ctx.path {
103+
if let Some(src) = path {
94104
dst_ctx.path = Some(src);
95105
}
106+
if let Some(src) = password_expiry_utc {
107+
dst_ctx.password_expiry_utc = Some(src);
108+
}
96109
for (src, dst) in [
97-
(ctx.protocol, &mut dst_ctx.protocol),
98-
(ctx.host, &mut dst_ctx.host),
99-
(ctx.username, &mut dst_ctx.username),
100-
(ctx.password, &mut dst_ctx.password),
110+
(protocol, &mut dst_ctx.protocol),
111+
(host, &mut dst_ctx.host),
112+
(username, &mut dst_ctx.username),
113+
(password, &mut dst_ctx.password),
114+
(oauth_refresh_token, &mut dst_ctx.oauth_refresh_token),
101115
] {
102116
if let Some(src) = src {
103117
*dst = Some(src);
104118
}
105119
}
106-
if let Some(src) = ctx.url {
120+
if let Some(src) = ctx_url {
107121
dst_ctx.url = Some(src);
108122
url = dst_ctx.destructure_url_in_place(self.use_http_path)?.url.take();
109123
}
124+
if dst_ctx
125+
.password_expiry_utc
126+
.is_some_and(|expiry_date| expiry_date < gix_date::Time::now_utc().seconds)
127+
{
128+
dst_ctx.password_expiry_utc = None;
129+
dst_ctx.clear_secrets();
130+
}
110131
if dst_ctx.username.is_some() && dst_ctx.password.is_some() {
111132
break;
112133
}
113-
if ctx.quit.unwrap_or_default() {
114-
dst_ctx.quit = ctx.quit;
134+
if quit.unwrap_or_default() {
135+
dst_ctx.quit = quit;
115136
break;
116137
}
117138
}
@@ -152,6 +173,7 @@ impl Cascade {
152173
action.context().map(|ctx| helper::Outcome {
153174
username: ctx.username.clone(),
154175
password: ctx.password.clone(),
176+
oauth_refresh_token: ctx.oauth_refresh_token.clone(),
155177
quit: ctx.quit.unwrap_or(false),
156178
next: ctx.to_owned().into(),
157179
}),

‎gix-credentials/src/helper/invoke.rs

+1
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ pub fn invoke(helper: &mut crate::Program, action: &Action) -> Result {
3030
Ok(Some(Outcome {
3131
username: ctx.username,
3232
password: ctx.password,
33+
oauth_refresh_token: ctx.oauth_refresh_token,
3334
quit: ctx.quit.unwrap_or(false),
3435
next: NextAction {
3536
previous_output: stdout.into(),

‎gix-credentials/src/helper/mod.rs

+8-2
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ pub struct Outcome {
2525
pub username: Option<String>,
2626
/// The password to use in the identity, if set.
2727
pub password: Option<String>,
28+
/// An OAuth refresh token that may accompany a password. It is to be treated confidentially, just like the password.
29+
pub oauth_refresh_token: Option<String>,
2830
/// If set, the helper asked to stop the entire process, whether the identity is complete or not.
2931
pub quit: bool,
3032
/// A handle to the action to perform next in another call to [`helper::invoke()`][crate::helper::invoke()].
@@ -42,7 +44,11 @@ impl Outcome {
4244
self.username
4345
.take()
4446
.zip(self.password.take())
45-
.map(|(username, password)| gix_sec::identity::Account { username, password })
47+
.map(|(username, password)| gix_sec::identity::Account {
48+
username,
49+
password,
50+
oauth_refresh_token: self.oauth_refresh_token.take(),
51+
})
4652
}
4753
}
4854

@@ -131,7 +137,7 @@ impl Action {
131137
}
132138
}
133139

134-
/// A handle to [store][NextAction::store()] or [erase][NextAction::erase()] the outcome of the initial action.
140+
/// A handle to [store](NextAction::store()) or [erase](NextAction::erase()) the outcome of the initial action.
135141
#[derive(Clone, Debug, Eq, PartialEq)]
136142
pub struct NextAction {
137143
previous_output: BString,

‎gix-credentials/src/protocol/context/mod.rs

+36
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,42 @@ mod access {
1414
use crate::protocol::Context;
1515

1616
impl Context {
17+
/// Clear all fields that are considered secret.
18+
pub fn clear_secrets(&mut self) {
19+
let Context {
20+
protocol: _,
21+
host: _,
22+
path: _,
23+
username: _,
24+
password,
25+
oauth_refresh_token,
26+
password_expiry_utc: _,
27+
url: _,
28+
quit: _,
29+
} = self;
30+
31+
*password = None;
32+
*oauth_refresh_token = None;
33+
}
34+
/// Replace existing secrets with the word `<redacted>`.
35+
pub fn redacted(mut self) -> Self {
36+
let Context {
37+
protocol: _,
38+
host: _,
39+
path: _,
40+
username: _,
41+
password,
42+
oauth_refresh_token,
43+
password_expiry_utc: _,
44+
url: _,
45+
quit: _,
46+
} = &mut self;
47+
for secret in [password, oauth_refresh_token].into_iter().flatten() {
48+
*secret = "<redacted>".into();
49+
}
50+
self
51+
}
52+
1753
/// Convert all relevant fields into a URL for consumption.
1854
pub fn to_url(&self) -> Option<BString> {
1955
use bstr::{ByteSlice, ByteVec};

‎gix-credentials/src/protocol/context/serde.rs

+49-13
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,46 @@ mod write {
1717
out.write_all(value)?;
1818
out.write_all(b"\n")
1919
}
20-
for (key, value) in [("url", &self.url), ("path", &self.path)] {
20+
let Context {
21+
protocol,
22+
host,
23+
path,
24+
username,
25+
password,
26+
oauth_refresh_token,
27+
password_expiry_utc,
28+
url,
29+
// We only decode quit and interpret it, but won't get to pass it on as it means to stop the
30+
// credential helper invocation chain.
31+
quit: _,
32+
} = self;
33+
for (key, value) in [("url", url), ("path", path)] {
2134
if let Some(value) = value {
2235
validate(key, value.as_slice().into())
2336
.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?;
2437
write_key(&mut out, key, value.as_ref()).ok();
2538
}
2639
}
2740
for (key, value) in [
28-
("protocol", &self.protocol),
29-
("host", &self.host),
30-
("username", &self.username),
31-
("password", &self.password),
41+
("protocol", protocol),
42+
("host", host),
43+
("username", username),
44+
("password", password),
45+
("oauth_refresh_token", oauth_refresh_token),
3246
] {
3347
if let Some(value) = value {
3448
validate(key, value.as_str().into())
3549
.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?;
3650
write_key(&mut out, key, value.as_bytes().as_bstr()).ok();
3751
}
3852
}
53+
if let Some(value) = password_expiry_utc {
54+
let key = "password_expiry_utc";
55+
let value = value.to_string();
56+
validate(key, value.as_str().into())
57+
.map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?;
58+
write_key(&mut out, key, value.as_bytes().as_bstr()).ok();
59+
}
3960
Ok(())
4061
}
4162

@@ -70,6 +91,17 @@ pub mod decode {
7091
/// Decode ourselves from `input` which is the format written by [`write_to()`][Self::write_to()].
7192
pub fn from_bytes(input: &[u8]) -> Result<Self, Error> {
7293
let mut ctx = Context::default();
94+
let Context {
95+
protocol,
96+
host,
97+
path,
98+
username,
99+
password,
100+
oauth_refresh_token,
101+
password_expiry_utc,
102+
url,
103+
quit,
104+
} = &mut ctx;
73105
for res in input.lines().take_while(|line| !line.is_empty()).map(|line| {
74106
let mut it = line.splitn(2, |b| *b == b'=');
75107
match (
@@ -84,23 +116,27 @@ pub mod decode {
84116
}) {
85117
let (key, value) = res?;
86118
match key {
87-
"protocol" | "host" | "username" | "password" => {
119+
"protocol" | "host" | "username" | "password" | "oauth_refresh_token" => {
88120
if !value.is_utf8() {
89121
return Err(Error::IllformedUtf8InValue { key: key.into(), value });
90122
}
91123
let value = value.to_string();
92124
*match key {
93-
"protocol" => &mut ctx.protocol,
94-
"host" => &mut ctx.host,
95-
"username" => &mut ctx.username,
96-
"password" => &mut ctx.password,
125+
"protocol" => &mut *protocol,
126+
"host" => host,
127+
"username" => username,
128+
"password" => password,
129+
"oauth_refresh_token" => oauth_refresh_token,
97130
_ => unreachable!("checked field names in match above"),
98131
} = Some(value);
99132
}
100-
"url" => ctx.url = Some(value),
101-
"path" => ctx.path = Some(value),
133+
"password_expiry_utc" => {
134+
*password_expiry_utc = value.to_str().ok().and_then(|value| value.parse().ok());
135+
}
136+
"url" => *url = Some(value),
137+
"path" => *path = Some(value),
102138
"quit" => {
103-
ctx.quit = gix_config_value::Boolean::try_from(value.as_ref()).ok().map(Into::into);
139+
*quit = gix_config_value::Boolean::try_from(value.as_ref()).ok().map(Into::into);
104140
}
105141
_ => {}
106142
}

‎gix-credentials/src/protocol/mod.rs

+10-8
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ pub struct Context {
4848
pub username: Option<String>,
4949
/// The credential’s password, if we are asking it to be stored.
5050
pub password: Option<String>,
51+
/// An OAuth refresh token that may accompany a password. It is to be treated confidentially, just like the password.
52+
pub oauth_refresh_token: Option<String>,
53+
/// The expiry date of OAuth tokens as seconds from Unix epoch.
54+
pub password_expiry_utc: Option<gix_date::SecondsSinceUnixEpoch>,
5155
/// When this special attribute is read by git credential, the value is parsed as a URL and treated as if its constituent
5256
/// parts were read (e.g., url=<https://example.com> would behave as if
5357
/// protocol=https and host=example.com had been provided). This can help callers avoid parsing URLs themselves.
@@ -59,14 +63,10 @@ pub struct Context {
5963
/// Convert the outcome of a helper invocation to a helper result, assuring that the identity is complete in the process.
6064
#[allow(clippy::result_large_err)]
6165
pub fn helper_outcome_to_result(outcome: Option<helper::Outcome>, action: helper::Action) -> Result {
62-
fn redact(mut ctx: Context) -> Context {
63-
if let Some(pw) = ctx.password.as_mut() {
64-
*pw = "<redacted>".into();
65-
}
66-
ctx
67-
}
6866
match (action, outcome) {
69-
(helper::Action::Get(ctx), None) => Err(Error::IdentityMissing { context: redact(ctx) }),
67+
(helper::Action::Get(ctx), None) => Err(Error::IdentityMissing {
68+
context: ctx.redacted(),
69+
}),
7070
(helper::Action::Get(ctx), Some(mut outcome)) => match outcome.consume_identity() {
7171
Some(identity) => Ok(Some(Outcome {
7272
identity,
@@ -75,7 +75,9 @@ pub fn helper_outcome_to_result(outcome: Option<helper::Outcome>, action: helper
7575
None => Err(if outcome.quit {
7676
Error::Quit
7777
} else {
78-
Error::IdentityMissing { context: redact(ctx) }
78+
Error::IdentityMissing {
79+
context: ctx.redacted(),
80+
}
7981
}),
8082
},
8183
(helper::Action::Store(_) | helper::Action::Erase(_), _ignore) => Ok(None),

‎gix-credentials/tests/fixtures/expired.sh

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
#!/usr/bin/env bash
2+
set -eu
3+
4+
test "$1" = get && \
5+
echo username=user-expired && \
6+
echo password=pass-expired && \
7+
echo password_expiry_utc=1

‎gix-credentials/tests/fixtures/oauth-token.sh

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
#!/usr/bin/env bash
2+
set -eu
3+
4+
test "$1" = get && \
5+
echo oauth_refresh_token=oauth-token

‎gix-credentials/tests/helper/cascade.rs

+20
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,25 @@ mod invoke {
119119
assert_eq!(actual.identity, identity("user", "pass"));
120120
}
121121

122+
#[test]
123+
fn expired_credentials_are_not_returned() {
124+
let actual = invoke_cascade(
125+
["expired", "oauth-token", "custom-helper"],
126+
Action::get_for_url("http://github.com"),
127+
)
128+
.unwrap()
129+
.expect("credentials");
130+
131+
assert_eq!(
132+
actual.identity,
133+
Account {
134+
oauth_refresh_token: Some("oauth-token".into()),
135+
..identity("user-script", "pass-script")
136+
},
137+
"it ignored the expired password, which otherwise would have come first"
138+
);
139+
}
140+
122141
#[test]
123142
fn bogus_password_overrides_any_helper_and_helper_overrides_username_in_url() {
124143
let actual = Cascade::default()
@@ -144,6 +163,7 @@ mod invoke {
144163
Account {
145164
username: user.into(),
146165
password: pass.into(),
166+
oauth_refresh_token: None,
147167
}
148168
}
149169

0 commit comments

Comments
 (0)