Merge pull request #2345 from hongwei1/develop

refactor/remove the basicUrlValidation, use basicUriAndQueryStringVal
This commit is contained in:
Simon Redfern 2023-12-05 15:33:05 +01:00 committed by GitHub
commit a467ed163c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 66 additions and 33 deletions

View File

@ -777,20 +777,6 @@ object APIUtil extends MdcLoggable with CustomJsonFormats{
}
}
def basicUrlValidation(urlString: String): Boolean = {
//in scala test - org.scalatest.FeatureSpecLike.scenario:
// redirectUrl = http%3A%2F%2Flocalhost%3A8016%3Foauth_token%3DEBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK%26oauth_verifier%3D63461
// URLDecoder.decode(urlString,"UTF-8")-->http://localhost:8016?oauth_token=EBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK&oauth_verifier=63461
val regex =
"""((([A-Za-z]{3,9}:(?:\/\/)?)(?:[-;:&=\+\$,\w]+@)?[A-Za-z0-9.-]+(:[0-9]+)?|(?:www.|[-;:&=\+\$,\w]+@)[A-Za-z0-9.-]+)((?:\/[\+~%\/.\w-_]*)?\??(?:[-\+=&;%@.\w_\/]*)#?(?:[\w]*))?)""".r
val decodeUrlValue = URLDecoder.decode(urlString, "UTF-8").trim()
decodeUrlValue match {
case regex(_*) if (decodeUrlValue.length <= 2048) => true
case _ => false
}
}
/** only A-Z, a-z, 0-9,-,_,. =, & and max length <= 2048 */
def basicUriAndQueryStringValidation(urlString: String): Boolean = {
val regex =

View File

@ -48,8 +48,10 @@ class OAuthWorkedThanks extends MdcLoggable {
val redirectUrl = ObpS.param("redirectUrl").map(urlDecode(_))
logger.debug(s"OAuthWorkedThanks.thanks.redirectUrl $redirectUrl")
//extract the clean(omit the parameters) redirect url from request url
val requestedRedirectURL = Helper.extractCleanRedirectURL(redirectUrl.openOr("invalidRequestedRedirectURL")) openOr("invalidRequestedRedirectURL")
logger.debug(s"OAuthWorkedThanks.thanks.requestedRedirectURL $requestedRedirectURL")
val staticPortionOfRedirectUrl = Helper.getStaticPortionOfRedirectURL(redirectUrl.openOr("invalidRequestedRedirectURL")) openOr("invalidRequestedRedirectURL")
val hostOnlyOfRedirectUrlLegacy = Helper.getHostOnlyOfRedirectURL(staticPortionOfRedirectUrl) openOr("invalidRequestedRedirectURL")
logger.debug(s"OAuthWorkedThanks.thanks.staticPortionOfRedirectUrl $staticPortionOfRedirectUrl")
logger.debug(s"OAuthWorkedThanks.thanks.hostOnlyOfRedirectUrlLegacy $hostOnlyOfRedirectUrlLegacy")
val requestedOauthToken = Helper.extractOauthToken(redirectUrl.openOr("No Oauth Token here")) openOr("No Oauth Token here")
logger.debug(s"OAuthWorkedThanks.thanks.requestedOauthToken $requestedOauthToken")
@ -62,13 +64,10 @@ class OAuthWorkedThanks extends MdcLoggable {
redirectUrl match {
case Full(url) =>
//this redirect url is checked by following, no open redirect issue.
// TODO maybe handle case of extra trailing / on the url ?
val incorrectRedirectUrlMessage = s"The validRedirectURL is $validRedirectURL but the staticPortionOfRedirectUrl was $staticPortionOfRedirectUrl"
val incorrectRedirectUrlMessage = s"The validRedirectURL is $validRedirectURL but the requestedRedirectURL was $requestedRedirectURL"
if(validRedirectURL.equals(requestedRedirectURL)) {
//hostOnlyOfRedirectUrlLegacy is deprecated now, we use the staticPortionOfRedirectUrl stead,
if(validRedirectURL.equals(staticPortionOfRedirectUrl)|| validRedirectURL.equals(hostOnlyOfRedirectUrlLegacy)) {
"#redirect-link [href]" #> url &
".app-name"#> appName //there may be several places to be modified in html, so here use the class, not the id.
}else{

View File

@ -1,9 +1,8 @@
package code.util
import java.net.{Socket, SocketException}
import java.net.{Socket, SocketException, URL}
import java.util.UUID.randomUUID
import java.util.{Date, GregorianCalendar}
import code.api.util.{APIUtil, CallContext, CallContextLight, CustomJsonFormats}
import code.api.{APIFailureNewStyle, Constant}
import code.api.util.APIUtil.fullBoxOrException
@ -20,7 +19,9 @@ import com.openbankproject.commons.util.{ReflectUtils, RequiredFieldValidation,
import com.tesobe.CacheKeyFromArguments
import net.liftweb.http.S
import net.liftweb.util.Helpers
import net.liftweb.util.Helpers.tryo
import net.sf.cglib.proxy.{Enhancer, MethodInterceptor, MethodProxy}
import java.lang.reflect.Method
import scala.concurrent.Future
import scala.util.Random
@ -167,8 +168,30 @@ object Helper extends Loggable {
prettyRender(decompose(input))
}
def extractCleanRedirectURL(input: String): Box[String] = {
Full(input.split("\\?oauth_token=")(0))
/**
*
* @param redirectUrl eg: http://localhost:8082/oauthcallback?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018
* @return http://localhost:8082/oauthcallback
*/
def getStaticPortionOfRedirectURL(redirectUrl: String): Box[String] = {
tryo(redirectUrl.split("\\?")(0)) //return everything before the "?"
}
/**
* extract clean redirect url from input value, because input may have some parameters, such as the following examples <br/>
* eg1: http://localhost:8082/oauthcallback?....--> http://localhost:8082 <br/>
* eg2: http://localhost:8016?oautallback?=3NLMGV ...--> http://localhost:8016
*
* @param redirectUrl -> http://localhost:8082/oauthcallback?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018
* @return hostOnlyOfRedirectURL-> http://localhost:8082
*/
@deprecated("We can not only use hostname as the redirectUrl, now add new method `getStaticPortionOfRedirectURL` ","05.12.2023")
def getHostOnlyOfRedirectURL(redirectUrl: String): Box[String] = {
val url = new URL(redirectUrl) //eg: http://localhost:8082/oauthcallback?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018
val protocol = url.getProtocol() // http
val authority = url.getAuthority()// localhost:8082, this will contain the port.
tryo(s"$protocol://$authority") // http://localhost:8082
}
/**
@ -461,7 +484,7 @@ object Helper extends Loggable {
}else if((args.length>0) && args.apply(0).toString.equalsIgnoreCase("consumer_key")){
result.asInstanceOf[Box[String]].filter(APIUtil.basicConsumerKeyValidation(_)==SILENCE_IS_GOLDEN)
}else if((args.length>0) && args.apply(0).toString.equalsIgnoreCase("redirectUrl")){
result.asInstanceOf[Box[String]].filter(APIUtil.basicUrlValidation(_))
result.asInstanceOf[Box[String]].filter(APIUtil.basicUriAndQueryStringValidation(_))
} else{
result.asInstanceOf[Box[String]].filter(APIUtil.checkMediumString(_)==SILENCE_IS_GOLDEN)
}

View File

@ -698,12 +698,18 @@ class APIUtilTest extends FeatureSpec with Matchers with GivenWhenThen with Prop
APIUtil.getObpFormatOperationId("xxx") should be ("xxx")
}
feature("test APIUtil.basicUrlValidation method") {
feature("test APIUtil.basicUriAndQueryStringValidation method") {
val testString1 = "https%3A%2F%2Fapisandbox.openbankproject.com%2Foauth%2Fauthorize%3Fnext%3D%2Fen%2Fusers%2Fmyuser%26oauth_token%3DWTOBT2YRCTMI1BCCF4XAIKRXPLLZDZPFAIL5K03Z%26oauth_verifier%3D45381"
val testString2 = "http%3A%2F%2Flocalhost%3A8016%3Foauth_token%3DEBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK%26oauth_verifier%3D63461"
val testString3 = "myapp://callback?oauth_token=%3DEBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK%26oauth_verifier%3D63461"
val testString4 = "fb00000000:://callback?oauth_token=%3DEBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK%26oauth_verifier%3D63461"
val testString5 = "http://127.0.0.1:8000/oauth/authorize?next=/en/metrics/api/&oauth_token=TN0124OCPRCL4KUJRF5LNLVMRNHTVZPJDBS2PNWU&oauth_verifier=10470"
APIUtil.basicUrlValidation(testString1) should be (true)
APIUtil.basicUrlValidation(testString2) should be (true)
APIUtil.basicUriAndQueryStringValidation(testString1) should be (true)
APIUtil.basicUriAndQueryStringValidation(testString2) should be (true)
APIUtil.basicUriAndQueryStringValidation(testString3) should be (true)
APIUtil.basicUriAndQueryStringValidation(testString4) should be (true)
APIUtil.basicUriAndQueryStringValidation(testString5) should be (true)
}

View File

@ -35,13 +35,32 @@ import org.scalatest.{FeatureSpec, GivenWhenThen, Matchers}
class HelperTest extends FeatureSpec with Matchers with GivenWhenThen with PropsReset {
feature("test APIUtil.basicUrlValidation method") {
feature("test APIUtil.getStaticPortionOfRedirectURL method") {
// The redirectURl is `http://localhost:8082/oauthcallback`
val testString1 = "http://localhost:8082/oauthcallback?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018"
val testString2 = "http://localhost:8082?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018"
val testString3 = "myapp://callback?oauth_token=%3DEBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK%26oauth_verifier%3D63461"
val testString4 = "fb00000000:://callback?oauth_token=%3DEBRZBMOPDXEUGGJP421FPFGK01IY2DGM5O3TLVSK%26oauth_verifier%3D63461"
val testString5 = "http://127.0.0.1:8000/oauth/authorize?next=/en/metrics/api/&oauth_token=TN0124OCPRCL4KUJRF5LNLVMRNHTVZPJDBS2PNWU&oauth_verifier=10470"
Helper.extractCleanRedirectURL(testString1).head should be("http://localhost:8082/oauthcallback")
Helper.extractCleanRedirectURL(testString2).head should be("http://localhost:8082")
Helper.getStaticPortionOfRedirectURL(testString1).head should be("http://localhost:8082/oauthcallback")
Helper.getStaticPortionOfRedirectURL(testString2).head should be("http://localhost:8082")
Helper.getStaticPortionOfRedirectURL(testString3).head should be("myapp://callback")
Helper.getStaticPortionOfRedirectURL(testString4).head should be("fb00000000:://callback")
Helper.getStaticPortionOfRedirectURL(testString5).head should be("http://127.0.0.1:8000/oauth/authorize")
}
feature("test APIUtil.getHostOnlyOfRedirectURL method") {
// The redirectURl is `http://localhost:8082/oauthcallback`
val testString1 = "http://localhost:8082/oauthcallback?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018"
val testString2 = "http://localhost:8082/oauthcallback"
val testString3 = "http://localhost:8082?oauth_token=G5AEA2U1WG404EGHTIGBHKRR4YJZAPPHWKOMNEEV&oauth_verifier=53018"
val testString4 = "http://localhost:8082"
Helper.getHostOnlyOfRedirectURL(testString1).head should be("http://localhost:8082")
Helper.getHostOnlyOfRedirectURL(testString2).head should be("http://localhost:8082")
Helper.getHostOnlyOfRedirectURL(testString3).head should be("http://localhost:8082")
Helper.getHostOnlyOfRedirectURL(testString4).head should be("http://localhost:8082")
}
}