Skip to content

Conversation

alacroix
Copy link
Contributor

@alacroix alacroix commented Sep 27, 2024

Hey,

First of all, thanks for the project, it's really great.

We found a bug while upgrading to v3, concerning binary content.

It seems that the handling of binary content as response is broken. In convertRes, the parseHeaders function simply stores headers in a Record<string, string> but doesn't take in account the Uppercase / Lowercase thing that we can found in headers. In our case, the headers received are like Content-Type but in the code, it compares to lowercase headers["content-type"]. (Our infra uses lambdas + ApiGateway on top of that like mentioned in the docs)

Here is a small NextJS API route that reproduce the problem in our case.

const IMAGE_BASE64 =
  'data:image/jpeg;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAAAMUlEQVR4nGJRFC5jwAaUQo9iFWfCKooHjGogBrCsOqyCVaLnxEvq2DCqgRgACAAA//+FNgXZnyy3gAAAAABJRU5ErkJggg=='

function handler(req, res) {
  const base64Data = IMAGE_BASE64.split(',')[1]
  const buffer = Buffer.from(base64Data, 'base64')
  res.writeHead(200, {
    'Content-Type': 'image/jpeg',
  })
  res.write(buffer)
  res.end()
}

export default handler

While reaching on browser the route, you should get an error that saying that the navigator can't display the image since it contains errors.

Second problem, which is more specific, is that we use on some routes pre-gzipped content. For example, our sitemap.xml is a pre-gzipped API route. In this case, the header Content-Encoding is already present and the response should take in account the header and be base64encoded for binary content.

I also have simplified the check for isBinaryContentType(headers["content-type"]) since parseHeaders returns a Record<string, string>.

Thanks

Copy link
Contributor

@conico974 conico974 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@conico974 conico974 merged commit b5bfb5d into opennextjs:main Oct 3, 2024
1 check passed
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.

3 participants