Skip to content

Conversation

@Mat-Sedkowski
Copy link

This small change excludes the phone number starts with + sign from sanitisation.

Changes:

  • Added phone_number? method that compares input with regexp to determine if input is a phone number → may require some future improvements
  • Added conditional return in starts_with_special_character method if given input starts with + and is a phone number
  • Added tests

@zvory
Copy link
Owner

zvory commented Jul 25, 2024

Thanks for making this change. This would be useful to add.

However, I only want to add special case support for phone numbers if we actually support phone numbers well. Otherwise, I'd rather keep prefixing them to guarantee security.

What I mean by supporting phone numbers well means either using phonelib's .valid? method, or reworking this PR so that:

  • the tests show support for a variety of international numbers following E.164
  • the regex is simple and understandable and clearly doesn't introduce a security vulnerability
  • the tests as much as possible prove that we haven't weakened the security guarantees this library is supposed to provide

TBH, I'm scared even of this. Unless other people comment on this PR or issue saying they would like this functionality, I'd rather keep this gem as secure and slightly less useful, than have a niggling doubt in my mind that the regex is wrong (or that phonelib might get compromised).

@adamzapasnik
Copy link

@zvory Maybe we could add a "callback" via a configuration value? This way, end users could provide any logic that they want to starts_with_special_character? For example, my use case is that I'd like to not sanitise single character strings like - or +.

some really ugly pseudo code to explain what I mean:

def starts_with_special_character?(str)
  return false if CSVSafe.skip_sanitisation?(str)

  str.start_with?("-", "=", "+", "@", "%", "|", "\r", "\t")
end
  

def skip_sanitisation?(str)
  CSVSafe.skip_sanitisation(str)
end

@skip_sanitisation = -> (_str) { false }
def skip_sanitisation=(&block)
  @skip_sanitisation=&block
end
  
  

@zvory
Copy link
Owner

zvory commented Aug 2, 2024

That option seems to make sense to me.

Could you call it skip_sanitization_if or something like that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants