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

Update web.go #63

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Update web.go #63

wants to merge 1 commit into from

Conversation

samascience
Copy link

made changes to truncate to 70 characters

made changes to truncate to 70 characters
@Nevexo
Copy link
Contributor

Nevexo commented Jan 12, 2025

Thanks for your PR! Just a couple of things:

Your editor has altered the spacing of some lines, some that aren't affected by your change. Please run "go fmt" before committing code to make sure the style aligns with the rest of the project, it's always work looking at the diff before pushing it, just to make sure your editor hasn't snaked in a bad change.

Also, please add some more info to the commit message, giving a description of what the change is makes bisecting and troubleshooting much easier in the future. It's also worth linking to the original issue (#60) in your PR, just so everyone knows what's going on.

Finally, this kind of change needs some form of messaging on the front end explaining to the user why the password is limited to 72 bytes, it's probably worth limiting the text box field size to the limit within the backend. But don't worry if you're not comfortable altering the front end, we can do that in a different PR.

Thanks again!

@Nevexo Nevexo added bug Something isn't working reviewed/needs-rework labels Jan 12, 2025
Copy link
Author

@samascience samascience left a comment

Choose a reason for hiding this comment

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

edited

Copy link
Author

@samascience samascience left a comment

Choose a reason for hiding this comment

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

added truncated changes to truncate the password to 70 characters !

@samascience samascience closed this Feb 2, 2025
@samascience samascience reopened this Feb 2, 2025
@samascience
Copy link
Author

truncatedPassword - added truncated password in the code to limit to 70 characters
a

Comment on lines +250 to +253
truncatedPassword := req.Password
if len(truncatedPassword) > 70 {
truncatedPassword = truncatedPassword[:70]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is generally the wrong way to go about this.

Rather than truncating silently, the user should get feedback that the password they entered is too long. A http.StatusBadRequest or http.StatusRequestEntityTooLarge should be returned.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reviewed/needs-rework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants