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

Basic auth not working when using app.onError #952

Closed
JustJoostNL opened this issue Mar 3, 2023 · 12 comments
Closed

Basic auth not working when using app.onError #952

JustJoostNL opened this issue Mar 3, 2023 · 12 comments

Comments

@JustJoostNL
Copy link

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
Copy link
Member

yusukebe commented Mar 3, 2023

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
Copy link
Author

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
Copy link
Contributor

I myself have never used HTTPException, but I suppose you need to import from hono/http-exception.

hono/package.json

Lines 36 to 40 in 64956cb

"./http-exception": {
"types": "./dist/types/http-exception.d.ts",
"import": "./dist/http-exception.js",
"require": "./dist/cjs/http-exception.js"
},

@JustJoostNL
Copy link
Author

JustJoostNL commented Mar 4, 2023

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
Copy link
Member

yusukebe commented Mar 4, 2023

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
Copy link
Member

usualoma commented Mar 8, 2023

@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
Copy link
Member

yusukebe commented Mar 8, 2023

@usualoma

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

@JustJoostNL
Copy link
Author

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
Copy link
Member

yusukebe commented Mar 9, 2023

Good!

Let's leave the open.

@charnould
Copy link

@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
Copy link
Member

yusukebe commented Jul 3, 2024

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
Copy link

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.

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

No branches or pull requests

5 participants