fix: email link format for administrator contact#151
Open
odaysec wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Directly writing an uncontrolled stored value (a database field) to a webpage, without properly sanitizing the value first, allows for a cross-site scripting vulnerability. This kind of vulnerability is also called stored cross-site scripting, to distinguish it from other types of cross-site scripting.
CWE-79
CWE-116
In general, to fix this kind of problem, avoid constructing complex HTML or URI strings by interpolating untrusted stored values. Instead, pass untrusted data as separate, escaped arguments to helpers that will handle proper encoding, and/or sanitize or normalize the data before use.
For this specific view, we should stop building the full
mailto:"Name" <email>string ourselves. Instead:mailto:plus the raw email address as thehref, which should be safe when escaped.owner.nameinto thesubjectorbodyquery parameter if desired, but encoded viaRack::Utils.build_queryor similar, not string interpolation.titleattribute usesowner.namedirectly so Rails will HTML-escape it as an attribute value, rather than embedding it inside thehrefstring.Because we can only edit
app/views/accounts/_help_contact.html.erb, the minimal safe change is: replace the interpolated"mailto:\"#{owner.name}\" <#{owner.email_address}>"with a simple"mailto:#{owner.email_address}". This keeps the existing functionality (it still opens a mail client to email the administrator) while removing the untrustedowner.namefrom thehrefstring, eliminating the tainted data in the URI construction. The existingtitle: "Email #{owner.name}"remains; Rails will HTML-escape it.Concretely:
app/views/accounts/_help_contact.html.erb, on line 3, replace the first argument tolink_towith"mailto:#{owner.email_address}".