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

feat(cloudflare-access): Handle Access organization does not exist and Access not available cases #898

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

G4brym
Copy link
Contributor

@G4brym G4brym commented Dec 20, 2024

In the current version of @hono/cloudflare-access, both of the actions bellow will result into an "internal error" with a stack trace from the middleware:

  • defining an access team name that does not exist
  • fetching the access certificates when access is "generally" unavailable

This pr adds a good error message that correctly describes what the middleware received from Cloudflare Access.

This throws normal JS Error's because this is a fatal exception that the user should handle, but if the Hono maintainers think otherwise, i'm happy to change this into throw new HTTPException(500, { message: '...' })

Copy link

changeset-bot bot commented Dec 20, 2024

🦋 Changeset detected

Latest commit: b368d28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hono/cloudflare-access Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@G4brym G4brym force-pushed the improve-cloudflare-access-middleware branch from 54bf9b2 to 085813d Compare December 20, 2024 18:59
@yusukebe
Copy link
Member

Hi @G4brym

Thank you for the PR. In this case, it should throw a new HTTPException with the proper status number(is this 500?) to indicate the status and the message to the application.

@G4brym G4brym force-pushed the improve-cloudflare-access-middleware branch from 085813d to 757fcf4 Compare December 21, 2024 16:46
@G4brym G4brym force-pushed the improve-cloudflare-access-middleware branch from 757fcf4 to b368d28 Compare December 21, 2024 16:51
@G4brym
Copy link
Contributor Author

G4brym commented Dec 21, 2024

Hey @yusukebe, i've just updated the pr to throw HTTPException, not sure what the proper HTTP status is, but i think it should be 500, because the app cannot recover from this until either the team name is updated or the cloudflare access recovers from a possible outage

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

@G4brym

Thank you for changing it. I also considered which is the best status code. It's not 100% sure, but I think 500 is okay. Looks good. Merging and releasing a new version.

@yusukebe yusukebe merged commit b71d817 into honojs:main Dec 23, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Dec 23, 2024
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