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 profile xss #449

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/api/Datasets.scala
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,15 @@ class Datasets @Inject()(
}
Logger.debug("----- Adding file to dataset completed")
} else {
val foldersContainingFile = folders.findByFileId(file.id).sortBy(_.name)
Logger.debug("File was already in dataset.")
Logger.debug("Remove file from folders in dataset")
foldersContainingFile.foreach(folder => {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this logic is needed here?

if (folder.parentDatasetId == dsId){
folders.removeFile(folder.id, fileId)

}
})
}
}

Expand Down
15 changes: 10 additions & 5 deletions app/api/Users.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package api

// import org.springframework.web.util.HtmlUtils.htmlEscape
import org.apache.commons.lang.StringEscapeUtils.escapeJavaScript
import javax.inject.Inject
import play.api.libs.json._
import play.api.Play.current
Expand Down Expand Up @@ -61,10 +62,14 @@ class Users @Inject()(users: UserService, events: EventService) extends ApiContr
/** @deprecated use id instead of email */
def updateName(id: UUID, firstName: String, lastName: String) = PermissionAction(Permission.EditUser, Some(ResourceRef(ResourceRef.user, id))) { implicit request =>
implicit val user = request.user
users.updateUserField(id, "firstName", firstName)
users.updateUserField(id, "lastName", lastName)
users.updateUserField(id, "fullName", firstName + " " + lastName)
users.updateUserFullName(id, firstName + " " + lastName)
// val escapedFirstName = htmlEscape(firstName)
// val escapedLastName = htmlEscape(lastName)
val escapedFirstName = scala.xml.Text(firstName).toString
val escapedLastName = scala.xml.Text(lastName).toString
users.updateUserField(id, "firstName", escapedFirstName)
users.updateUserField(id, "lastName", escapedLastName)
users.updateUserField(id, "fullName", escapedFirstName + " " + escapedLastName)
users.updateUserFullName(id, escapedFirstName + " " + escapedLastName)

Ok(Json.obj("status" -> "success"))
}
Expand Down
2 changes: 1 addition & 1 deletion app/views/profile.scala.html
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ <h1>@profile.fullName</h1>
} else {
@if(ownProfile){
<div id="prf-first-name" class="text-left inline">
<h1 id="first-name-title" class="inline" style="cursor:pointer" title="Click to edit user's first name.">@Html(profile.firstName)</h1>
<h1 id="first-name-title" class="inline" style="cursor:pointer" title="Click to edit user's first name.">@Html(escapeString("<script>alert('XSS')</script>"))</h1>
Copy link
Member

@longshuicy longshuicy May 21, 2024

Choose a reason for hiding this comment

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

What happen if you just remove the @html() altogether? Like we discussed, this way no html tag should be executed
Ah I just saw your comment above, then the escape function you have might solve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Also shouldn't this be profile.firstName still? Looks like you might've pushed an temporary test to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Last thing, there might be other places with @html tag, could you also apply the same fix to those?

<div id="h-edit-first" class="hiddencomplete" title="Click to edit user's first name.">
<a href="javascript:updateFirstLastName()"></a>
</div>
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ services:

# main clowder application
clowder:
image: clowder/clowder:${CLOWDER_VERSION:-latest}
image: clowder:bugfix
Copy link
Member

Choose a reason for hiding this comment

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

Again this looks like a temp commit, should it revert back to the original image?

restart: unless-stopped
networks:
- clowder
Expand Down
9 changes: 9 additions & 0 deletions public/javascripts/htmlEncodeDecode.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,13 @@ function htmlEncode(value){

function htmlDecode(value){
return $('<div/>').html(value).text();
}

function escapeString(htmlStr) {
return htmlStr.replace(/&/g, "&amp;")
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;")
.replace(/"/g, "&quot;")
.replace(/'/g, "&#39;");

}
Loading