Repository: google/OpenSK
File: src/ctap/ctap1.rs (Rust)

Code before:
    Register {
        challenge: [u8; 32],
        application: [u8; 32],
    },
    #[allow(dead_code)]
    Authenticate {
        challenge: [u8; 32],
        application: [u8; 32],
        key_handle: Vec<u8>,
        flags: Ctap1Flags,
    },
    Version,
    #[allow(dead_code)]
    VendorSpecific {
        payload: Vec<u8>,
    },
}

impl TryFrom<&[u8]> for U2fCommand {
    type Error = Ctap1StatusCode;

    fn try_from(message: &[u8]) -> Result<Self, Ctap1StatusCode> {
        let apdu: APDU = match APDU::try_from(message) {
            Ok(apdu) => apdu,
            // Todo: Better conversion between ApduStatusCode and Ctap1StatusCode
            // Maybe use TryFrom?
            Err(apdu_status_code) => match apdu_status_code {
                ApduStatusCode::SW_WRONG_LENGTH => return Err(Ctap1StatusCode::SW_WRONG_LENGTH),
                ApduStatusCode::SW_WRONG_DATA => return Err(Ctap1StatusCode::SW_WRONG_DATA),
                _ => return Err(Ctap1StatusCode::SW_COMMAND_ABORTED),
            },
        };

        // ISO7816 APDU Header format. Each cell is 1 byte. Note that the CTAP flavor always
        // encodes the length on 3 bytes and doesn't use the field "Le" (Length Expected).
        // We keep the 2 byte of "Le" for the packet length in mind, but always ignore its value.
        // Lc is using big-endian encoding
        // +-----+-----+----+----+-----+-----+-----+
        // | CLA | INS | P1 | P2 | Lc1 | Lc2 | Lc3 |
        // +-----+-----+----+----+-----+-----+-----+
        if apdu.header.cla != Ctap1Command::CTAP1_CLA {
            return Err(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED);
        }

        // Since there is always request data, the expected length is either omitted or
        // encoded in 2 bytes.
        // Todo: support extended APDUs now that the new parser can work with those
        if apdu.lc as usize != apdu.data.len() && (apdu.lc as usize) + 2 != apdu.data.len() {
            return Err(Ctap1StatusCode::SW_WRONG_LENGTH);
        }


Review comment (suggestion):
Our `TODO` style would be
> // TODO(kaczmarczyck)

or 
> // TODO(#123456)

for a GitHub issue number. In this case, why not resolve it? By merging these two status codes? I don't think we need both.

Code after:
        // +-----+-----+----+----+-----+-----+-----+
        if apdu.header.cla != Ctap1Command::CTAP1_CLA {
            return Err(Ctap1StatusCode::SW_CLA_INVALID);
        }

        // Since there is always request data, the expected length is either omitted or
        // encoded in 2 bytes.
        if lc != apdu.data.len() && lc + 2 != apdu.data.len() {
            return Err(Ctap1StatusCode::SW_WRONG_LENGTH);
        }

        match apdu.header.ins {
            // U2F raw message format specification, Section 4.1
            // +-----------------+-------------------+
            // + Challenge (32B) | Application (32B) |
            // +-----------------+-------------------+
            Ctap1Command::U2F_REGISTER => {
                if lc != 64 {
                    return Err(Ctap1StatusCode::SW_WRONG_LENGTH);
                }
                Ok(Self::Register {
                    challenge: *array_ref!(apdu.data, 0, 32),
                    application: *array_ref!(apdu.data, 32, 32),
                })
            }

            // U2F raw message format specification, Section 5.1
            // +-----------------+-------------------+---------------------+------------+
            // + Challenge (32B) | Application (32B) | key handle len (1B) | key handle |
            // +-----------------+-------------------+---------------------+------------+
            Ctap1Command::U2F_AUTHENTICATE => {
                if lc < 65 {
                    return Err(Ctap1StatusCode::SW_WRONG_LENGTH);
                }
                let handle_length = apdu.data[64] as usize;
                if lc != 65 + handle_length {
                    return Err(Ctap1StatusCode::SW_WRONG_LENGTH);
                }
                let flag = Ctap1Flags::try_from(apdu.header.p1)?;
                Ok(Self::Authenticate {
                    challenge: *array_ref!(apdu.data, 0, 32),
                    application: *array_ref!(apdu.data, 32, 32),
                    key_handle: apdu.data[65..].to_vec(),
                    flags: flag,
                })
            }

            // U2F raw message format specification, Section 6.1
            Ctap1Command::U2F_VERSION => {
                if lc != 0 {
                    return Err(Ctap1StatusCode::SW_WRONG_LENGTH);