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

Remove bluemonday #1

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Remove bluemonday #1

merged 1 commit into from
Oct 24, 2023

Conversation

david-bezero
Copy link

This was previously used:

  • (wrongly) for some JSON escaping (fixed by replacing with real JSON serialisation), and
  • for building an email template (which also had a vulnerability because it didn't apply to the site parameter).

Both have been fixed properly here. Fortunately we're not using the email capability at all. Having seen this code makes me super uncomfortable about trusting these authors with anything authentication-related though…

github.com/nullrocks/identicon v0.0.0-20180626043057-7875f45b0022
github.com/pkg/errors v0.9.1
Copy link
Author

Choose a reason for hiding this comment

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

this appeared when running go mod tidy - seems some of the oauth changes brought it in

Comment on lines +166 to +168
ID: userID,
Name: userID,
Picture: fmt.Sprintf(c.URL+"/avatar?user=%s", userID),
Copy link
Author

@david-bezero david-bezero Oct 23, 2023

Choose a reason for hiding this comment

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

vulnerability fixed: backslashes and double quotes in userID were not being escaped here

Comment on lines -185 to +182
Site: r.URL.Query().Get("site"),
User: safeHTML(user),
Address: safeHTML(address),
Token: template.HTMLEscapeString(tkn),
Site: template.HTMLEscapeString(site),
Copy link
Author

Choose a reason for hiding this comment

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

vulnerability fixed: site comes from a URL parameter and was not being HTML escaped at all. Token is less of an issue, but I added HTML escaping just-in-case. The user and address also had some "prettifying" and truncation logic (presumably for plain-text viewing), which I have preserved in safeHTML, though I don't really see the point.

Comment on lines -153 to +150
Audience: e.sanitize(r.URL.Query().Get("site")),
Audience: site,
Copy link
Author

Choose a reason for hiding this comment

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

this was incorrectly being HTML encoded, but never appears in any HTML document

@david-bezero david-bezero merged commit 572dcaa into master Oct 24, 2023
@david-bezero david-bezero deleted the remove-bluemonday branch October 24, 2023 09:52
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