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

Remove incorrect ASCII conversion #979

Merged
merged 5 commits into from
Jun 13, 2023

Conversation

eddyashton
Copy link
Contributor

When oav is validating the byte format, it reports errors for many valid base64 strings.

I believe this should permit any valid base64, but the conversion to ASCII is dropping/re-encoding bytes that aren't valid ASCII. The result is that any real base64 in an example (which isn't merely a base-64 encoding of ASCII, as the example generator constructs), will produce a validation error.

Ideally this would return a more precise error message ("here's the character I don't think is valid base64"), as relying on the JS Buffer parser means we only accept url-safe, fully padded values. But that's a larger change, so making this standalone fix for now.

Examples of roundtripping with the code before and after, below

function roundtrip_a(x) {
  const decodedValue = Buffer.from(x, "base64").toString("ascii");
  const reencodedValue = Buffer.from(decodedValue).toString("base64");
  console.log(`${x} -> ${reencodedValue} ${x === reencodedValue ? "" : "(MISMATCH)"}`)
}

function roundtrip_b(x) {
  const decodedValue = Buffer.from(x, "base64");
  const reencodedValue = Buffer.from(decodedValue).toString("base64");
  console.log(`${x} -> ${reencodedValue} ${x === reencodedValue ? "" : "(MISMATCH)"}`)
}


> roundtrip_a('aaY=')
aaY= -> aSY= (MISMATCH)

> roundtrip_b('aaY=')
aaY= -> aaY=

> roundtrip_a('1234')
1234 -> V214 (MISMATCH)

> roundtrip_b('1234')
1234 -> 1234

> roundtrip_a('cQ6T')
cQ6T -> cQ4T (MISMATCH)

> roundtrip_b('cQ6T')
cQ6T -> cQ6T

> roundtrip_a('----')
---- -> e28+ (MISMATCH)

> roundtrip_b('----')
---- -> ++++ (MISMATCH)

@scbedd
Copy link
Member

scbedd commented May 31, 2023

Hey @eddyashton thanks for your PR! For this one I definitely need to try a couple samples in addition, so I'll have this reviewed by EoW.

Copy link
Member

@scbedd scbedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Thanks very much for bringing this up @eddyashton .

Don't worry about merging, my plan is to get the regression-full check that is failing green today, then merge your PR after double checking that run to ensure we're not relying on broken behavior in existing examples 👍

@scbedd
Copy link
Member

scbedd commented Jun 12, 2023

Hey @eddyashton apologies for the delay here! I've been extremely randomized lately. I spent some time digging around, understanding the failure and why it was suddenly cropping up.

Essentially, we "regressed" in that we FIXED a bug. We were expecting an INVALID_FORMAT error on a few examples, but we no longer throw that given that it was actually totally valid. Given that, we just need to patch the known good. Unfortunately I do not have push access to your fork branch, otherwise I would take care of the following steps myself. 👍

To merge this PR, I would ask that you do the following:

  1. Open the latest build run
  2. Click on 2 Published in `artifacts section
    image
  3. Download the patch file
    image
  4. Go to the root of your fork, pull latest to get the upstream merge commit that I commit earlier today.
  5. git apply <path-to-downloaded-patch-file>
  6. Commit the results, push as new commit!
  7. I will merge this PR!

Thank you so much for the original report + fix!

@eddyashton
Copy link
Contributor Author

@scbedd No worries, thanks for the thorough review!

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

Successfully merging this pull request may close these issues.

2 participants