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

Chore: Enable gosec #140

Merged
merged 5 commits into from
Jul 10, 2024
Merged

Chore: Enable gosec #140

merged 5 commits into from
Jul 10, 2024

Conversation

dsonck92
Copy link
Collaborator

Enable the gosec linter to get security related mistakes. It can be a bit prone to false positives, but at least will give us the opportunity to think about our choices.

@dsonck92 dsonck92 force-pushed the chore/enable-gosec branch 2 times, most recently from a318890 to 5f75152 Compare July 10, 2024 18:51
@dsonck92 dsonck92 marked this pull request as ready for review July 10, 2024 19:14
}
return nil
}

func (mt *MailTemplate) getText(path string, variables map[string]string) (string, error) {
f, err := os.Open(path)
f, err := templates.FS.Open(path)
Copy link
Collaborator Author

@dsonck92 dsonck92 Jul 10, 2024

Choose a reason for hiding this comment

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

Please note that this will embed templates. Not sure if it was planned to allow the user to override the templates at some point. But if we talk about cloud native, we might want to bring this into some kind of specialized bucket in the future anyways so it can be edited online. And embed the defaults so it can be populated and/or reset.

But this was the easiest way (other than ignoring) to keep gosec happy. Gosec triggers because you may give .. in this path string, breaking out of the expected "root". One way to fix this is to use path.Join(root, path.Clean(path)) such that it's anchored to root, or the other is to give a virtual FS like done now.

Copy link
Member

Choose a reason for hiding this comment

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

I agree about having the templates editable only, what about like exposing them to some great headless CMS like "Sanity".

Comment on lines -49 to -54
port, err := strconv.Atoi(os.Getenv("PORT"))
if err != nil {
panic(err)
}
config = &Config{
Port: port,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In go, ports are technically not limited to an int (and beyond go). It's also perfectly valid to say :http or other standardized names. At least I think this is the case, although I never tried it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@loboda4450 we still let something slip through 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

eh, that's embarrassing....

@dsonck92 dsonck92 requested review from bouassaba and loboda4450 and removed request for bouassaba July 10, 2024 20:33
@loboda4450 loboda4450 merged commit 3241712 into main Jul 10, 2024
4 checks passed
@loboda4450 loboda4450 deleted the chore/enable-gosec branch July 10, 2024 21:40
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.

3 participants