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 #2539: Super Referrer Improvements (Clipboard) #2535

Merged
merged 16 commits into from May 25, 2020
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Changed ref code sanitzation to use regex.

  • Loading branch information
jhreis committed May 18, 2020
commit 8214d99dbc457f76e3ff9578b6064b383f7d7d20
@@ -194,28 +194,16 @@ public class UserReferralProgram {
/// Uses very strict matching.
/// Returns the sanitized code, or nil if no code was found
public class func sanitize(input: String?) -> String? {
guard var code = input, code.hasPrefix(self.clipboardPrefix) else { return nil }
guard var input = input, input.hasPrefix(self.clipboardPrefix) else { return nil }

// +1 to strip off `:` that proceeds the defined prefix
code.removeFirst(self.clipboardPrefix.count + 1)
input.removeFirst(self.clipboardPrefix.count + 1)
This conversation was marked as resolved by jhreis

This comment has been minimized.

Copy link
@kylehickinson

kylehickinson May 11, 2020

Contributor

Understand this probably set in a spec somewhere, but maybe a comment about why + 1?

This comment has been minimized.

Copy link
@jhreis

jhreis May 11, 2020

Author Contributor

Nah, lol, yeah, will do!

This conversation was marked as resolved by Brandon-T

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T May 20, 2020

Collaborator

If input is exactly the same length as clipboardPrefix, then removing count + 1 is undefined behaviour and could potentially cause a crash.

The number of elements to remove from the collection. k must be greater than or equal to zero and must not exceed the number of elements in the collection.

We can add to the guard statement that input.count > self.clipboardPrefix.count + 1
Is it possible that the ref-code after the prefix could potentially be blank? I'm not sure, but I think some people might try to manipulate it :D

This comment has been minimized.

Copy link
@jhreis

jhreis May 20, 2020

Author Contributor

Good catch, thanks @Brandon-T

This comment has been minimized.

Copy link
@jhreis

jhreis May 20, 2020

Author Contributor

I added a unit test to cover this too, thanks for pointing out.

// Add any other potential validation here, e.g. validating the actual ref code string
This conversation was marked as resolved by jhreis

This comment has been minimized.

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T May 11, 2020

Collaborator

The ref code is guaranteed to be 6 upper-case, 0-9 codes?

This comment has been minimized.

Copy link
@BrendanEich

BrendanEich May 11, 2020

Member

Refcode must be ABC123 format, 3 uppercase alphabetic and 3 decimal digits.

This comment has been minimized.

Copy link
@jhreis

jhreis May 12, 2020

Author Contributor

@jumde Please finalize this on the spec first.

This comment has been minimized.

Copy link
@jhreis

jhreis May 18, 2020

Author Contributor

Updated regex per spec.

if code.count != 6 { return nil }
let letters = code.prefix(3)
let numbers = code.suffix(3)

// Cannot use `isLetters` or `isUppercase` b/inc Æ and the like
let validLetters = letters.allSatisfy(("A"..."Z").contains)

// Cannot use `isNumber` b/inc 万 and the like
let validNumbers = numbers.allSatisfy(("0"..."9").contains)

// Both conditions must be met
return validLetters && validNumbers ? code : nil

// Regex solution
// let valid = code.range(of: #"\b[A-Z]{3}[0-9]{3}\b"#, options: .regularExpression) != nil // true
let valid = input.range(of: #"\b[A-Z]{3}[0-9]{3}\b"#, options: .regularExpression) != nil // true

This comment has been minimized.

Copy link
@kylehickinson

kylehickinson May 20, 2020

Contributor

Leftover // true comment? 😄

This comment has been minimized.

Copy link
@jhreis

jhreis May 20, 2020

Author Contributor

lol, yup, used for testing in a playground, thanks for pointing out.

// Both conditions must be met
return valid ? input : nil
}

/// Same as `customHeaders` only blocking on result, to gaurantee data is available
@@ -0,0 +1,30 @@
// Copyright 2020 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.
import XCTest

class UserReferralProgramTests: XCTestCase {

override func setUpWithError() throws {
// Put setup code here. This method is called before the invocation of each test method in the class.
}

override func tearDownWithError() throws {
// Put teardown code here. This method is called after the invocation of each test method in the class.
}

func testExample() throws {
// This is an example of a functional test case.
// Use XCTAssert and related functions to verify your tests produce the correct results.
}

func testPerformanceExample() throws {
// This is an example of a performance test case.
self.measure {
// Put the code you want to measure the time of here.
}
}

}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.