colinmeinke / ghost-storage-adapter-s3

An AWS S3 storage adapter for Ghost
Other
183 stars 87 forks source link

Potential problem on stripEndingSlash function #62

Open MCTaylor17 opened 4 years ago

MCTaylor17 commented 4 years ago

Problem

Note the following code found in /src/index.js

const stripLeadingSlash = s => s.indexOf('/') === 0 ? s.substring(1) : s
const stripEndingSlash = s => s.indexOf('/') === (s.length - 1) ? s.substring(0, s.length - 1) : s

The expression s.indexOf("/") returns the first index in the string. That means stripEndingSlash will only strip ending slashes from strings with exactly one slash. A string containing two slashes will return with the trailing slash still in place.

Example

Consider the following cases:

const singleSlash = "index.js/"
// singleSlash.indexOf('/') equals 8
// 8 equals (singleSlash.length -1);
// Slash is removed
console.log(stripEndingSlash(singleSlash)); 
// "index.js"
const multiSlash = "/src/index.js/";
// multiSlash.indexOf('/') equals 0
// 0 doesn't equal (multiSlash.length -1)
// Slash is not removed
console.log(stripEndingSlash(multiSlash)); 
// "/src/index.js/"

Suggestion

You could fix this by changing indexOf to lastIndexOf, however, using charAt provides a more readable and performant expression:

const stripLeadingSlash = s => s.charAt(0) === "/" ? s.substring(1) : s
const stripEndingSlash = s => s.charAt(s.length - 1) === "/" ? s.substring(0, s.length - 1) : s