Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/src/instant/model/app.clj (1)
365-376: Race condition handling is sound, butensure-totp-secret!return value is unused.The three-step fallback pattern (read → ensure → read) correctly handles concurrent initialization. However,
ensure-totp-secret!returns the result ofsql/execute-one!(row count metadata), not the encrypted key, so Line 372's(:totp_secret_key_enc ...)will always be nil from that branch. The logic still works because the third read (Line 374) catches this case.Consider simplifying by removing the unused middle extraction:
♻️ Optional refactor
(defn get-totp-secret-key ([params] (get-totp-secret-key (aurora/conn-pool :read) (aurora/conn-pool :write) params)) ([read-conn write-conn {:keys [id]}] - (let [secret-key-enc (or (:totp_secret_key_enc (get-by-id! read-conn {:id id})) - (:totp_secret_key_enc (ensure-totp-secret! write-conn {:id id})) - ;; 2nd read in case two txes tried to initialize the key at the same time - (:totp_secret_key_enc (get-by-id! read-conn {:id id})))] + (let [secret-key-enc (or (:totp_secret_key_enc (get-by-id! read-conn {:id id})) + (do (ensure-totp-secret! write-conn {:id id}) + ;; Re-read after ensure, handles both success and race condition + (:totp_secret_key_enc (get-by-id! read-conn {:id id}))))] (crypt-util/aead-decrypt {:ciphertext secret-key-enc :associated-data (uuid-util/->bytes id)}))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/model/app.clj` around lines 365 - 376, The middle branch of the fallback is trying to extract :totp_secret_key_enc from the return of ensure-totp-secret!, but ensure-totp-secret! returns row/metadata (not the row), so that extraction is always nil; update the get-totp-secret-key function to call ensure-totp-secret! for its side-effect (i.e., keep the read → ensure → read pattern) and remove the unused (:totp_secret_key_enc (ensure-totp-secret! ...)) extraction — either replace that branch with a plain (ensure-totp-secret! write-conn {:id id}) call or drop it and rely on the final get-by-id! call; keep references to get-totp-secret-key, ensure-totp-secret!, and get-by-id! so the logic and the second read remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/flags.clj`:
- Around line 373-388: The validate-with-totp? toggle can enable validation
before generation, causing all verifications to fail; update validate-with-totp?
(or replace both flags with a single mode) so it only returns true when both the
validate toggle and generate toggle are enabled (i.e., require
generate-with-totp? to be true as a precondition), or refactor
generate-with-totp? and validate-with-totp? into a single rollout state (e.g.,
:totp-mode) and use that single source in validate-with-totp?,
generate-with-totp?, and dual-write-totp? to prevent the invalid
validate⇒generate ordering.
In `@server/src/instant/model/app_user_magic_code.clj`:
- Around line 47-60: The DB write that records issuance/consumption must not be
skipped when TOTP flags vary; currently the (when (or (not
(flags/validate-with-totp?)) ...)) around update-op causes create! to omit the
issuance record and consume! to allow replay. Remove or adjust that conditional
so update-op is always invoked for the issuance path in create! (and similarly
for the consume!/mark-consumed path), keeping the same update-op call that
writes {:id id :etype etype :codeHash ... :email email} so verify_magic_code and
consume! can check prior issuance/one-time use even when validate-with-totp? or
dual-write-totp? differ; ensure the same fix is applied to the other mirrored
block later in the file (the 84-103 equivalent).
- Around line 66-76: The expiry window is being unintentionally extended because
(inc expiry-periods) adds an extra period while instant.totp/valid-totp? already
includes the current period; change the call to totp/valid-totp? to pass
expiry-periods (not (inc expiry-periods)) and update the adjacent comment to
reflect that no additional increment is needed. Locate the expiry-periods
binding and the totp/valid-totp? invocation (the secret-key, expiry-periods,
code usage) and remove the inc usage so the configured expiry is honored.
In `@server/src/instant/runtime/routes.clj`:
- Around line 119-125: The test setup calls app-user-magic-code-model/create!
with the wrong keys: it passes :user-id and omits the required :email; update
the create! call to remove :user-id and include :email (e.g., :email
"stopa@instantdb.com") so it matches the function signature {:keys [app-id email
id]} and still binds the returned code to m for the subsequent
verify-magic-code-post calls; ensure the create! invocation remains
(app-user-magic-code-model/create! ...) and that verify-magic-code-post uses m
as the :code value.
---
Nitpick comments:
In `@server/src/instant/model/app.clj`:
- Around line 365-376: The middle branch of the fallback is trying to extract
:totp_secret_key_enc from the return of ensure-totp-secret!, but
ensure-totp-secret! returns row/metadata (not the row), so that extraction is
always nil; update the get-totp-secret-key function to call ensure-totp-secret!
for its side-effect (i.e., keep the read → ensure → read pattern) and remove the
unused (:totp_secret_key_enc (ensure-totp-secret! ...)) extraction — either
replace that branch with a plain (ensure-totp-secret! write-conn {:id id}) call
or drop it and rely on the final get-by-id! call; keep references to
get-totp-secret-key, ensure-totp-secret!, and get-by-id! so the logic and the
second read remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 11ef3dd5-c954-4acf-8e1b-0b1ced1994ad
📒 Files selected for processing (13)
server/resources/migrations/100_totp_magic_code.down.sqlserver/resources/migrations/100_totp_magic_code.up.sqlserver/src/instant/admin/routes.cljserver/src/instant/flags.cljserver/src/instant/model/app.cljserver/src/instant/model/app_user_magic_code.cljserver/src/instant/runtime/magic_code_auth.cljserver/src/instant/runtime/routes.cljserver/src/instant/totp.cljserver/src/instant/util/crypt.cljserver/test/instant/reactive/session_test.cljserver/test/instant/runtime/routes_test.cljserver/test/instant/totp_test.clj
| (defn generate-with-totp? | ||
| "Returns true if we should generate codes with totp. Need to run this for 24 hours before | ||
| turning on validate-with-totp" | ||
| [] | ||
| (toggled? :generate-with-totp true)) | ||
|
|
||
| (defn validate-with-totp? | ||
| "Returns true if we should validate codes with totp. Need to generate-with-totp for 24 hours before | ||
| turning on validate-with-totp" | ||
| [] | ||
| (toggled? :validate-with-totp false)) | ||
|
|
||
| (defn dual-write-totp? | ||
| "Returns true if we should write our totp codes to the magic codes table." | ||
| [] | ||
| (toggled? :dual-write-totp true)) |
There was a problem hiding this comment.
These rollout flags still allow a broken auth state.
If validate-with-totp? is flipped on while generate-with-totp? is still off, create! sends a random DB-backed code but consume! still requires a TOTP match, so every verification fails. Please enforce validate ⇒ generate or collapse these booleans into a single rollout mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/flags.clj` around lines 373 - 388, The validate-with-totp?
toggle can enable validation before generation, causing all verifications to
fail; update validate-with-totp? (or replace both flags with a single mode) so
it only returns true when both the validate toggle and generate toggle are
enabled (i.e., require generate-with-totp? to be true as a precondition), or
refactor generate-with-totp? and validate-with-totp? into a single rollout state
(e.g., :totp-mode) and use that single source in validate-with-totp?,
generate-with-totp?, and dual-write-totp? to prevent the invalid
validate⇒generate ordering.
| (when (or (not (flags/validate-with-totp?)) | ||
| (not (flags/generate-with-totp?)) | ||
| (flags/dual-write-totp?)) | ||
| (update-op | ||
| conn | ||
| {:app-id app-id | ||
| :etype etype} | ||
| (fn [{:keys [transact!]}] | ||
| (transact! [{:id id | ||
| :etype etype | ||
| :codeHash (-> code | ||
| crypt-util/str->sha256 | ||
| crypt-util/bytes->hex-string) | ||
| :email email}])))) |
There was a problem hiding this comment.
Cutting off the DB write removes both issuance tracking and one-time use.
When validate-with-totp? is true and dual-write-totp? is false, create! no longer records that a code was issued and consume! accepts any matching current TOTP for (app-id,email) without checking prior issuance or prior use. That means /verify_magic_code can succeed even if /send_magic_code was never called, and the same code can be replayed until it expires.
Also applies to: 84-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/model/app_user_magic_code.clj` around lines 47 - 60, The
DB write that records issuance/consumption must not be skipped when TOTP flags
vary; currently the (when (or (not (flags/validate-with-totp?)) ...)) around
update-op causes create! to omit the issuance record and consume! to allow
replay. Remove or adjust that conditional so update-op is always invoked for the
issuance path in create! (and similarly for the consume!/mark-consumed path),
keeping the same update-op call that writes {:id id :etype etype :codeHash ...
:email email} so verify_magic_code and consume! can check prior
issuance/one-time use even when validate-with-totp? or dual-write-totp? differ;
ensure the same fix is applied to the other mirrored block later in the file
(the 84-103 equivalent).
| expiry-periods (or (some-> (app-model/get-by-id! {:id app-id}) | ||
| :totp_expiry_minutes | ||
| (max 5) ;; Minimum of 5 minutes | ||
| (min 1440) ;; Maximum of 1 day | ||
| (* 60) | ||
| (/ totp/default-time-step) | ||
| (Math/ceil)) | ||
| (/ (flags/totp-default-expiry-seconds) totp/default-time-step))] | ||
| ;; Have to add 1 extra period in case the code was generated near the | ||
| ;; end of a period | ||
| (when-not (totp/valid-totp? secret-key (inc expiry-periods) code) |
There was a problem hiding this comment.
inc extends the validity window by one extra 5-minute period.
instant.totp/valid-totp? already counts the current period as one of the checked windows, so (inc expiry-periods) turns a 3-period policy into 4 and a 24-hour/288-period policy into 289. That keeps codes valid longer than the configured expiry.
Suggested fix
- (when-not (totp/valid-totp? secret-key (inc expiry-periods) code)
+ (when-not (totp/valid-totp? secret-key expiry-periods code)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expiry-periods (or (some-> (app-model/get-by-id! {:id app-id}) | |
| :totp_expiry_minutes | |
| (max 5) ;; Minimum of 5 minutes | |
| (min 1440) ;; Maximum of 1 day | |
| (* 60) | |
| (/ totp/default-time-step) | |
| (Math/ceil)) | |
| (/ (flags/totp-default-expiry-seconds) totp/default-time-step))] | |
| ;; Have to add 1 extra period in case the code was generated near the | |
| ;; end of a period | |
| (when-not (totp/valid-totp? secret-key (inc expiry-periods) code) | |
| expiry-periods (or (some-> (app-model/get-by-id! {:id app-id}) | |
| :totp_expiry_minutes | |
| (max 5) ;; Minimum of 5 minutes | |
| (min 1440) ;; Maximum of 1 day | |
| (* 60) | |
| (/ totp/default-time-step) | |
| (Math/ceil)) | |
| (/ (flags/totp-default-expiry-seconds) totp/default-time-step))] | |
| ;; Have to add 1 extra period in case the code was generated near the | |
| ;; end of a period | |
| (when-not (totp/valid-totp? secret-key expiry-periods code) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/model/app_user_magic_code.clj` around lines 66 - 76, The
expiry window is being unintentionally extended because (inc expiry-periods)
adds an extra period while instant.totp/valid-totp? already includes the current
period; change the call to totp/valid-totp? to pass expiry-periods (not (inc
expiry-periods)) and update the adjacent comment to reflect that no additional
increment is needed. Locate the expiry-periods binding and the totp/valid-totp?
invocation (the secret-key, expiry-periods, code usage) and remove the inc usage
so the configured expiry is honored.
| (def m (app-user-magic-code-model/create! | ||
| {:id (random-uuid) :user-id (:id runtime-user) :code (app-user-magic-code-model/rand-code) | ||
| {:id (random-uuid) :user-id (:id runtime-user) | ||
| :app-id (:id app)})) | ||
| (verify-magic-code-post {:body {:email "stopainstantdb.com" :code (:code m)}}) | ||
| (verify-magic-code-post {:body {:email "stopa@instantdb.com" :code (:code m)}}) | ||
| (verify-magic-code-post {:body {:email "stopainstantdb.com" :code m}}) | ||
| (verify-magic-code-post {:body {:email "stopa@instantdb.com" :code m}}) | ||
| (verify-magic-code-post {:body {:email "stopa@instantdb.com" :code "0" :app-id (:id app)}}) | ||
| (verify-magic-code-post {:body {:email "stopa@instantdb.com" :code (:code m) :app-id (:id app)}})) | ||
| (verify-magic-code-post {:body {:email "stopa@instantdb.com" :code m :app-id (:id app)}})) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current signature of app-user-magic-code-model/create!
rg -n "defn create!" server/src/instant/model/app_user_magic_code.clj -A 10Repository: instantdb/instant
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Get the complete create! function implementation
rg -n "defn create!" server/src/instant/model/app_user_magic_code.clj -A 30Repository: instantdb/instant
Length of output: 1276
🏁 Script executed:
#!/bin/bash
# Search for other usages of create! in the test files to see correct parameter usage
rg "app-user-magic-code-model/create!" server/src/instant/runtime/routes.clj -B 2 -A 5Repository: instantdb/instant
Length of output: 517
🏁 Script executed:
#!/bin/bash
# Look for test files that might show how create! should be used
fd "test.*\.clj$" | xargs rg "app-user-magic-code-model/create!" -l 2>/dev/null | head -5Repository: instantdb/instant
Length of output: 43
The comment block test code has incorrect parameters for create!.
The create! function signature expects {:keys [app-id email id]}, but the call on line 120 includes :user-id (which does not exist) and omits :email (which is required). Remove :user-id and add :email to the call. The use of m as :code in the subsequent verify calls is correct since create! returns the generated code string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/runtime/routes.clj` around lines 119 - 125, The test setup
calls app-user-magic-code-model/create! with the wrong keys: it passes :user-id
and omits the required :email; update the create! call to remove :user-id and
include :email (e.g., :email "stopa@instantdb.com") so it matches the function
signature {:keys [app-id email id]} and still binds the returned code to m for
the subsequent verify-magic-code-post calls; ensure the create! invocation
remains (app-user-magic-code-model/create! ...) and that verify-magic-code-post
uses m as the :code value.
1 participant
Uses a TOTP for magic codes instead of generating a random code.
To generate a TOTP, you need a secret key. We want to have a separate key per app, per email. Each app gets its own secret key that we generate on demand and store encrypted in the database. Then we use hmac-sha256 with that secret key and the email for the code to generate the secret key for the TOTP code.
When you generate a totp, you have to pick an interval. We set the interval to 5 minutes and then we check across multiple past periods to get the desired interval. I think we'll want to make the default 3 periods, which will let every code expire between 10 and 15 minutes. But if you want it to last for 24 hours, then we'll just check your code against the last 288 periods.
While we roll out the change, we'll write the totp codes into the database like they were regular magic codes.
The full rollout will look like this:
We'll want to announce that we're reducing the expiry (that's in a feature flag and is currently set to 24 hours) and give users a chance to modify their totp expiration.
I made a couple of changes to make the testing easier:
*toggle-overrides*and*flag-overrides*dynamic vars so that you can override the config for individual testsStill TODO: