Skip to content

Conversation

robinnydahl
Copy link

Description

Please see my issue #268. Implement possibility to keep iat unchanged during token refresh.

Checklist:

  • I've added tests for my changes or tests are not applicable
  • I've changed documentations or changes are not required
  • I've added my changes to CHANGELOG.md

@Messhias
Copy link
Collaborator

Messhias commented Nov 7, 2024

@mfn or @eschricker can you review it?

@mfn
Copy link
Contributor

mfn commented Nov 7, 2024

Please allow me to find time, getting back ASAP

@mfn
Copy link
Contributor

mfn commented Nov 12, 2024

I replied over at #268 (comment)

@robinnydahl
Copy link
Author

I replied in #268 as well :)

@specialtactics
Copy link
Member

I am actually keen to set this to false by default in the interests of security, if the other maintainers are game.

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

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

I am actually keen to set this to false by default in the interests of security, if the other maintainers are game.

I can, together with the context of #268 (comment) , agree to this.

I acknowledge:

  • the sudden change in behaviour which next to no proper warning is very unfortunate
  • in the interest of secure-by-default, the default should be false and match the current behaviour
  • the config comment, which likely will serve as the primordial documentation on this for the future, should be more clear about the security implications, more or less the scenario in the comment from @specialtactics I linked to

What do y'all think?

@specialtactics
Copy link
Member

I agree with your thoughts there @mfn, and I guess to your point, we need to work on having some documentation there on that, since we are adding more functionality to this package, and I don't think we control the original docs?

@Messhias
Copy link
Collaborator

Messhias commented Feb 4, 2025

I agree with your thoughts there @mfn, and I guess to your point, we need to work on having some documentation there on that, since we are adding more functionality to this package, and I don't think we control the original docs?

We have some conflicts in the MR right now.

@robinnydahl
Copy link
Author

@Messhias I've gone ahead and fixed the merge conflicts.

@Messhias Messhias requested review from mfn and specialtactics and removed request for mfn February 26, 2025 00:32
@specialtactics
Copy link
Member

@robinnydahl would you mind to set it to false by default both in the config file and the code to get it from config, as per discussion above.

@robinnydahl
Copy link
Author

@specialtactics, sorry for the delay, this one went past me... I have now changed the default behavior of refresh iat to be false. I also merged in the last changes from main.

@mfn, please have a look at the updated code, which reflects the discussions.

Let me know if anything else needs to be adjusted.

@mfn
Copy link
Contributor

mfn commented Jul 3, 2025

I just left for vacation, won't be until August I can check it out

@miszczu
Copy link

miszczu commented Oct 13, 2025

Bumping the PR as we are interested to see this one merged and released soon.

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.

6 participants