Skip to content

Add status codes to crypto/credential failures#836

Merged
bplatz merged 2 commits intomainfrom
feature/crypto-exceptions
Jul 16, 2024
Merged

Add status codes to crypto/credential failures#836
bplatz merged 2 commits intomainfrom
feature/crypto-exceptions

Conversation

@bplatz
Copy link
Contributor

@bplatz bplatz commented Jul 16, 2024

Add error message status code to crypto verification exceptions

@bplatz bplatz requested a review from a team July 16, 2024 13:44
(when (not (crypto/verify-signature pubkey signing-input signature))
(throw (ex-info "Verification failed." {:error :credential/invalid-signature :credential credential})))
(throw (ex-info "Verification failed, invalid credential."
{:status 400
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider using a 403 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did at first but then reconsidered as the request itself is invalid but definitely a gray area.

I did a quick google search and found other tools doing 400 so that was enough to nudge in this direction. Certainly open for debate though.

Copy link

@Jackamus29 Jackamus29 Jul 16, 2024

Choose a reason for hiding this comment

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

Yeah, according to MDN:

The HTTP 403 Forbidden response status code indicates that the server understands the request but refuses to authorize it.

I would expect a 400 for malformed requests (which includes an invalid credential) and a 403 for a valid credential, but insufficient permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes perfect sense, I'm happy with the 400.

Copy link
Contributor

@dpetran dpetran left a comment

Choose a reason for hiding this comment

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

🏟️

Base automatically changed from fix/policy-modify to main July 16, 2024 19:48
@bplatz bplatz merged commit 9697f89 into main Jul 16, 2024
@bplatz bplatz deleted the feature/crypto-exceptions branch July 16, 2024 19:48
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