-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move base64 encoding/decoding to Rust side #13845
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,8 +149,25 @@ fn op_base64_decode( | |
_state: &mut OpState, | ||
input: String, | ||
_: (), | ||
) -> Result<ZeroCopyBuf, AnyError> { | ||
let mut input: &str = &input.replace(|c| char::is_ascii_whitespace(&c), ""); | ||
) -> Result<String, AnyError> { | ||
let mut ws = false; | ||
let len = input.len(); | ||
let invalid = input.char_indices().any(|(i, c)| { | ||
if char::is_ascii_whitespace(&c) { | ||
ws = true; | ||
} else if c == '=' { | ||
if i != len - 1 && i != len - 2 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a bit of a possibility for false negative here: Even if there are '=' characters at the end it does not mean that they'll be removed below since that depends on whitespace removal etc. As such, it would be more appropriate to track '=' markers at end with an enum: None, One, Two. Then compare to those when '=' is being removed and reset the enum appropriately. If reset does not happen, it means that the '=' at the end were invalid characters. |
||
return true; | ||
} | ||
} else if c != '+' && c != '/' && !c.is_alphanumeric() { | ||
return true; | ||
} | ||
false | ||
}); | ||
|
||
// "Remove all ASCII whitespace from data" | ||
let input = if ws { input.replace(|c| char::is_ascii_whitespace(&c), "") } else { input }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be preferable if the replacement could be done on the same iter as the invalid characters but that's probably not possible. |
||
let mut input: &str = &input; | ||
// "If the length of input divides by 4 leaving no remainder, then: | ||
// if input ends with one or two U+003D EQUALS SIGN (=) characters, | ||
// remove them from input." | ||
|
@@ -170,10 +187,8 @@ fn op_base64_decode( | |
); | ||
} | ||
|
||
if input | ||
.chars() | ||
.any(|c| c != '+' && c != '/' && !c.is_alphanumeric()) | ||
{ | ||
// "If data contains a code point that is not '+', '/', or ASCII alphanumeric then return failure." | ||
if invalid { | ||
return Err( | ||
DomExceptionInvalidCharacterError::new( | ||
"Failed to decode base64: invalid character", | ||
|
@@ -190,14 +205,21 @@ fn op_base64_decode( | |
err | ||
)) | ||
})?; | ||
Ok(ZeroCopyBuf::from(out)) | ||
Ok(String::from_utf8(out).unwrap()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if |
||
} | ||
|
||
fn op_base64_encode( | ||
_state: &mut OpState, | ||
s: ZeroCopyBuf, | ||
s: String, | ||
_: (), | ||
) -> Result<String, AnyError> { | ||
let char_count = s.chars().count(); | ||
let s = s.into_bytes(); | ||
if s.len() != char_count { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Properly this should be |
||
// If the byte vec length is longer than the string length, it means that some characters were not respresentable as a single | ||
// byte, ie. some characters were not Latin1. | ||
return Err(DomExceptionInvalidCharacterError::new("The string to be encoded contains characters outside of the Latin1 range.").into()); | ||
} | ||
let cfg = base64::Config::new(base64::CharacterSet::Standard, true) | ||
.decode_allow_trailing_bits(true); | ||
let out = base64::encode_config(&s, cfg); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a shame that the strings (presumably) have to be copied. I wonder if it would be possible to have an optional
&str
serde?