Skip to content
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

Regression in at_hash behavior from Version 0.10.1 #45

Closed
issackelly opened this issue Aug 15, 2023 · 4 comments · Fixed by #46
Closed

Regression in at_hash behavior from Version 0.10.1 #45

issackelly opened this issue Aug 15, 2023 · 4 comments · Fixed by #46
Assignees
Milestone

Comments

@issackelly
Copy link

issackelly commented Aug 15, 2023

In the upgrade from base64 from 0.13 to 0.21 the behavior was lost where both unpadded or correctly padded strings were accepted. I am using dex which is currently sending me unpadded strings. I have not checked if this behavior is allowed as per the spec or not.


at_hash on  master [?] is 📦 v0.1.0 via 🐍 v3.10.12 (env) via 🦀 v1.70.0 
// base64 0.13.1

fn main() {
    let x = "zglPCMCEP7ilF3LP_NExow";
    match base64::decode_config(x, base64::URL_SAFE) {
        Ok(_) => println!("ok"),
        Err(e) => println!("error {}", e),
    };
}


❯ cargo run
   Compiling at_hash v0.1.0 (/home/issac/src/at_hash)
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running `target/debug/at_hash`
ok




// base64 0.21
use base64::{engine::general_purpose::URL_SAFE, Engine as _};
fn main() {
    let x = "zglPCMCEP7ilF3LP_NExow";
    match URL_SAFE.decode(x) {
        Ok(_) => println!("ok"),
        Err(e) => println!("error {}", e),
    };
}

at_hash on  master [?] is 📦 v0.1.0 via 🐍 v3.10.12 (env) via 🦀 v1.70.0 
❯ cargo run             
   Compiling at_hash v0.1.0 (/home/issac/src/at_hash)
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running `target/debug/at_hash`
error Invalid padding




@issackelly
Copy link
Author

issackelly commented Aug 16, 2023

It appears that the spec is not thorough on this point, simply saying "base64url" which itself suggests that since we're always dealing with the first 128 bits, we may drop the padding. Would it be best to have the old behavior, where both methods are tried?

@teohhanhui
Copy link

teohhanhui commented Aug 16, 2023

Implementations MUST include appropriate pad characters at the end of
encoded data unless the specification referring to this document
explicitly states otherwise.

https://www.rfc-editor.org/rfc/rfc4648#section-3.2

Read together with that, it seems like the spec is clear. Padding is always required.

EDIT: lol - https://github.com/marshallpierce/rust-base64#i-want-canonical-base64-encodingdecoding

@kilork
Copy link
Owner

kilork commented Aug 18, 2023

As change in the behavior was not intended, I consider this as regression.

@kilork kilork self-assigned this Sep 17, 2023
@kilork kilork added this to the 0.12.1 milestone Sep 17, 2023
@kilork kilork linked a pull request Sep 17, 2023 that will close this issue
@issackelly
Copy link
Author

Thank you! I had been meaning to get around to this but we have an upstream patch so it was not pressing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants