Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect comment lines. #1234

Merged
merged 3 commits into from
Mar 20, 2021
Merged

Fix incorrect comment lines. #1234

merged 3 commits into from
Mar 20, 2021

Conversation

iredmail
Copy link
Contributor

@iredmail iredmail commented Mar 20, 2021

Fix incorrect comment lines in 2 files:

  • middleware/csrf/config.go
  • middleware/session/config.go

One question left: in csrf middleware, default value is Strict, but in session, its Lax (see line 163-170). Should we set it to Strict too in session by default?

@kiyonlin
Copy link
Contributor

Thanks!

One question left: in csrf middleware, default value is Strict, but in session, its Lax (see line 163-170). Should we set it to Strict too in session by default?

We should not cuz it'll cause a breaking change.

@iredmail
Copy link
Contributor Author

Reasonable. But better set the default value to “strict” and mention this breaking change in release notes, so that all users get better security. :)

@kiyonlin
Copy link
Contributor

kiyonlin commented Mar 20, 2021

Reasonable. But better set the default value to “strict” and mention this breaking change in release notes, so that all users get better security. :)

Then we should release fiber v3 according to Semantic versioning 😂

Users can override the value if they wish. So don't worry about the different default values.

@iredmail
Copy link
Contributor Author

Understand. Let’s fix the comment lines first. :)

where should we put a TODO to remind fiber developers to change it to “strict” when preparing v3?

@kiyonlin
Copy link
Contributor

where should we put a TODO to remind fiber developers to change it to “strict” when preparing v3?

This is really a good point. Could you please add this?

@iredmail
Copy link
Contributor Author

I’d like to, but where should I put this remind? A new issue? Or add a comment line in source file?

@kiyonlin
Copy link
Contributor

Just push another commit about the TODO stuff in this PR.

@iredmail
Copy link
Contributor Author

Added a TODO item in comment line, hope it's ok for you. :)

@kiyonlin kiyonlin merged commit bcd10dd into gofiber:master Mar 20, 2021
@kiyonlin
Copy link
Contributor

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants