honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
17.16k stars 484 forks source link

Basic auth not working when using app.onError #952

Closed JustJoostNL closed 1 year ago

JustJoostNL commented 1 year ago

Since version 3.0.0 the basic auth doesn't work anymore when also using app.onError

Example:


app.use('/test-auth', basicAuth(
    {
        username: 'test',
        password: 'test',
    }
))

app.get('/test-auth', async (c) => {
    return c.json({
        message: 'You are authorized!',
        status: 200
    })
})

app.onError((err, c) => {
    console.log("An error has been thrown: " + err)
    return c.json({
        message: 'Internal Server Error',
        status: 500
    }, 500);
})

In this example, when you use the /test-auth route, it just returns an "Internal Server Error" and the error it returns is: "Error: Unauthorized"

When I am not using the app.onError (I comment that out), it does work normally. This issue occurs since version 3.0.0

I hope someone can help 🙂 Thanks in advance!

yusukebe commented 1 year ago

Hi @JustJoostNL !

Since version 3.0.0, a basic auth middleware throws the HTTPException if it does not authorize. This makes our appliation more secure. So, we have to handle it in app.onError(). For example, to customize the message, write like the following:

app.onError((err, c) => {
  if (err instanceof HTTPException) {
    const res = err.getResponse()
    return new Response(
      JSON.stringify({
        message: 'Unauthorized',
        status: 401,
      }),
      res
    )
  }
  return c.text('Internal Server Error', 500)
})

This is a bit confusing. If you can work it fine, I will put it in the documentation. Thank you.

P.S:

@usualoma @ThatOneBro and others

If you have any smart ideas to handle the exception, please share these. But I think this way is better. Or it's trouble to make a response from the Response object, so, I think it will be convenient like this API that overrides c.json():

if (err instanceof HTTPException) {
  const res = err.getResponse()
  return c.json(
    {
      message: 'Unauthorized',
      status: 401,
    },
    res
  )
}

But it may be unnecessary because the cord getting fat.

JustJoostNL commented 1 year ago

Hey @yusukebe! Thanks for your answer!

I assume I need to use an import to get the HTTPException?

I did try:

import { HTTPException } from "hono/dist/types/http-exception";

But I get an error when using that.

yudai-nkt commented 1 year ago

I myself have never used HTTPException, but I suppose you need to import from hono/http-exception. https://github.com/honojs/hono/blob/64956cb57ded4f8b7833afd9a9059e6dac3bfaac/package.json#L36-L40

JustJoostNL commented 1 year ago

So using the HTTPException does work. It will return Unauthorized.

But when using it doesn't even ask for login details. (It doesn't ask for a username and password). While when I comment the app.onError out (I remove the app.onError), it does ask for login details.

As I said earlier, this occurs since 3.0.0 and later.

I hope someone can help! Thanks in advance!

yusukebe commented 1 year ago

To make it ask for a username and password, we have to set the appropriate WWW-Authenticate header:

'WWW-Authenticate': 'Basic realm="Secure Area"'

The Response gotten from err.getResponse() has the headers including it, so this will work well:

const res = err.getResponse()
return new Response(
  JSON.stringify({
    message: 'Unauthorized',
    status: 401,
  }),
  res
)

If you don't add WWW-Authenticate header, it does not ask for these.

usualoma commented 1 year ago

@yusukebe

I thought about this for a while. Although it is a bit confusing, the current specification is reasonable to ensure security and flexibility.

yusukebe commented 1 year ago

@usualoma

Okay! Let's continue the discussion in #959 .

JustJoostNL commented 1 year ago

To make it ask for a username and password, we have to set the appropriate WWW-Authenticate header:

'WWW-Authenticate': 'Basic realm="Secure Area"'

The Response gotten from err.getResponse() has the headers including it, so this will work well:

const res = err.getResponse()
return new Response(
  JSON.stringify({
    message: 'Unauthorized',
    status: 401,
  }),
  res
)

If you don't add WWW-Authenticate header, it does not ask for these.

Thanks, this fixed it! Can we close this, or wait for #959?

yusukebe commented 1 year ago

Good!

Let's leave the open.

charnould commented 2 weeks ago

@yusukebe I'm missing something here. I don't know/understand where to put WWW-Authenticate header to make it work in this very simple code (using Bun). It does not ask for id/pwd. Any guidance? Thanks.

import { Hono } from 'hono'
import { basicAuth } from 'hono/basic-auth'

const app = new Hono()

app.use(
  '/auth/page',
  basicAuth({
    username: 'user',
    password: 'pwd',
  })
)

app.get('/auth/page', (c) => {
  console.log("Inside auth/page")
  c.header('WWW-Authenticate', 'Basic realm="Protected Area"'); /// ??????
  return c.text('You are authorized')
})

app.get('/ai/:id', (c) => c.text('You are inside AI route'))

app.notFound(async (c) => c.redirect(`/ai/${crypto.randomUUID()}`))
app.onError((_err, c) => c.notFound())

export default app
yusukebe commented 2 weeks ago

Hi @charnould

You have to handle a case where the user is unauthorized. Your code always returns a 404 response. To handle the reauthorization error, check the err object and if it's HTTPException and return the response in the HTTPException object:

app.onError((err, c) => {
  if (err instanceof HTTPException) {
    return err.getResponse()
  }
  return c.notFound()
})

Or you might not return c.notFound() in app.onError() in any case.

charnould commented 1 day ago

Hi @charnould

You have to handle a case where the user is unauthorized. Your code always returns a 404 response. To handle the reauthorization error, check the err object and if it's HTTPException and return the response in the HTTPException object:

app.onError((err, c) => {
  if (err instanceof HTTPException) {
    return err.getResponse()
  }
  return c.notFound()
})

Or you might not return c.notFound() in app.onError() in any case.

Thanks a lot @yusukebe ! It works perfectly.