Permalink
Browse files

Properly encode player urls

  • Loading branch information...
jkelin committed Jun 17, 2018
1 parent 04c6d01 commit 6f22f8c5d0520da803d0115cecdfabfa9e1f93c1
Showing with 4 additions and 4 deletions.
  1. +1 −1 src/matches.ts
  2. +2 −2 src/players.ts
  3. +1 −1 views/match.handlebars
View
@@ -20,7 +20,7 @@ function getPlayersForTeam(data, team) {
ipHash: ip && sha1(ip),
ipHashFirstTwo: ip && sha1(ip.split('.')[0] + ip.split('.')[1]),
ipHashFirstThree: ip && sha1(ip.split('.')[0] + ip.split('.')[1] + ip.split('.')[2]),
url: '/player/' + x.name
url: '/player/' + encodeURIComponent(x.name)
}
})
.sort((a, b) => b.score - a.score)
View
@@ -19,7 +19,7 @@ async function findRelatedNicknames(name) {
}
router.get('/player/:name.json', async function (req, res) {
var name = req.params["name"];
var name = decodeURIComponent(req.params["name"]);
const similar = await findRelatedNicknames(name);
// const data = await Player.where({ _id: name }).findOne();
@@ -30,7 +30,7 @@ router.get('/player/:name.json', async function (req, res) {
})
router.get('/player/:name', async function (req, res) {
var name = req.params["name"];
var name = decodeURIComponent(req.params["name"]);
const similar = await findRelatedNicknames(name);
const data = await Player.where({ _id: name }).findOne();
View
@@ -33,7 +33,7 @@
{{#each stats}}
<tr id="{{id}}" data-href="/matches/{{id}}">
<td>{{translateStatName key}}</td>
<td><a class="team-{{max.team}}" href="/player/{{max.url}}">{{max.name}}</a></td>
<td><a class="team-{{max.team}}" href="/player/{{urlencode max.name}}">{{max.name}}</a></td>
<td>{{max.value}}</td>
<td>{{sum}}</td>
</tr>

3 comments on commit 6f22f8c

@whoppster

This comment has been minimized.

whoppster replied Aug 17, 2018

Hm. Couldn't a properly constructed player name be used as an injection or to cause a crash? Don't see any checks on the names before creating the url there.

@jkelin

This comment has been minimized.

Owner

jkelin replied Aug 18, 2018

Not a crash, but an injection alright. I have assumed that Mongoose (ORM/ODM that is used) would automatically escape queries, but I guess that is too much to ask. At least since stats use MongoDB instead of some SQL variant it is not as trivial to exploit this using automated tools.

I have created an issue for this #2. Thank you!

@whoppster

This comment has been minimized.

whoppster replied Aug 19, 2018

No problem at all, happy to assist!
Btw, I haven't tested this, it was merely a question 😁

Please sign in to comment.