Skip to content

bug(auth): Fix bad config for otp code expiration#19454

Merged
dschom merged 1 commit intomainfrom
FXA-12400
Sep 16, 2025
Merged

bug(auth): Fix bad config for otp code expiration#19454
dschom merged 1 commit intomainfrom
FXA-12400

Conversation

@dschom
Copy link
Copy Markdown
Contributor

@dschom dschom commented Sep 16, 2025

Because

  • The email wasn't taking into account that the time in the config was specified in seconds
  • The 'step' and 'window' were swapped in the config

This pull request

  • Introduces the correct calculation for when the code actually expires
  • Specifies the step and window size correctly. Note that the old config still resulted in a code that was valid for 5 minutes, it's just not how the library expects it to be defined.

Issue that this pull request solves

Closes: FXA-12400

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

image

Other information (Optional)

Any other information that is important to this pull request.

@dschom dschom requested a review from a team as a code owner September 16, 2025 18:57
Because:
- The email wasn't taking into account that the time in the config was specified in seconds

This commit:
- Introduces the correct calculation for when the code actually expires.
Copy link
Copy Markdown
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

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

Looks good, just checking it out locally now. Only one docs nit, non-blocking

// Number of steps contained in the window. In this case
// 5 minutes worth of steps
default: 5 * 60,
doc: 'Overrides window otp options',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is working differently from the window/step defined in otp config in this file, unless that config is unrelated? Maybe specifying that this overrides otplib options would be clearer for anyone checking back on this later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested locally 👍

image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They are both valid configs. Neither is wrong... I agree normalizing how we define this makes sense though. More importantly we should double check how we calculate the actual expiration time and communicate this to end users.

I have a feeling the expiration time we communicate might not be correct in all cases. i.e. We don't take window and step into account, when doing the calculation, and this could lead to inaccuracies depending on how it is configured. I'll file a separate ticket for this.

@dschom dschom merged commit 42b85b1 into main Sep 16, 2025
19 checks passed
@dschom dschom deleted the FXA-12400 branch September 16, 2025 20:20
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