Always render plain text in WebauthnVerification#authenticate #3712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3712 +/- ##
=======================================
Coverage 98.75% 98.75%
=======================================
Files 207 207
Lines 5136 5143 +7
=======================================
+ Hits 5072 5079 +7
Misses 64 64
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
3996b66 to
949b2d9
Compare
| format.json { render json: { error: t(:not_found) }, status: :not_found } | ||
| format.yaml { render yaml: { error: t(:not_found) }, status: :not_found } | ||
| format.any(:all) { render text: t(:not_found), status: :not_found } | ||
| format.any(:all) { render plain: t(:not_found), status: :not_found } |
There was a problem hiding this comment.
render text: was removed in Rails 5.1 rails/rails@79a5ea9
| @message = params.permit(:error).fetch(:error, "") | ||
| Rails.logger.info("[webauthn_verification:authenticate] Verification failed: #{@message}") |
There was a problem hiding this comment.
Displaying and logging message as suggested:
can we add a log or something in the catch, rather than completely discarding the error?
Are you thinking of logging it our side (Rails logs) or like displaying a message to the user on why it failed (or both).
Hmmm I think both would be good? We could pass the error in the query param when redirecting to the failure page?
| assert_link_is_expired | ||
| end | ||
|
|
||
| test "when webauthn verification is expired during verification" do |
There was a problem hiding this comment.
Forgot this test case originally :p
What problem are you solving?
There's a couple of TODOs left from #3298. They all refer to how
WebauthnVerification:authenticateshould have a consistent response format.What approach did you choose and why?
Chose to return plain text as the response from the client is plain text. Made sure that not found, and expired token return plain and errors in the authenticate action all return plain.
Since all the responses are plain text, it is simpler to display the error to the user rather than swallowing it. If the result of the verification is not successful, the message with be stored as a param and rendered in the failed verification view.
Testing
RUBYGEMS_HOST=http://localhost:3000 gem signinas a user with a webauthn device. access the webauthn link
4. Do step 1 and authenticate, the success page would look the same.