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

Sanitize field input using different strategies #1222

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@jom
Member

jom commented Nov 1, 2017

Fixes #1205

Changes proposed in this Pull Request:

  • Allows text fields to specify their sanitization strategy. Built-in options: url, email, url_or_email. Callables with a single argument allowed.
  • Changes the application field to use the correct sanitization strategy. This will allow for octets in URLs.
  • Changes the company_website field to use the url sanitization.

Testing instructions:

  • Apply for jobs using standard URLs, URLs with non-Latin characters, emails, bad emails, dangerous strings, etc.

@jom jom changed the title from [WIP] Add/sanitization strategies#1205 to [WIP] Sanitize field input using different strategies Nov 1, 2017

@tripflex

This comment has been minimized.

Show comment
Hide comment
@tripflex

tripflex Nov 1, 2017

Collaborator

The only thing I would bring up is in regards to the preg_match used to check if the value is a URL.

First thing would be if users decide to enter the URL like this:
//google.com/something

I've also had a ton of instances where users have contacted me because a field they are using doesn't have a valid URL (meaning the user entered google.com/something instead of //google.com/something) and is the reason I added a setting and handling in my Field Editor to automatically prepend // to a URL that is missing it

My first thought was to use parse_url to handle this, here's a real quick sandbox to show the differences:
https://glot.io/snippets/ev374vwkbw

Collaborator

tripflex commented Nov 1, 2017

The only thing I would bring up is in regards to the preg_match used to check if the value is a URL.

First thing would be if users decide to enter the URL like this:
//google.com/something

I've also had a ton of instances where users have contacted me because a field they are using doesn't have a valid URL (meaning the user entered google.com/something instead of //google.com/something) and is the reason I added a setting and handling in my Field Editor to automatically prepend // to a URL that is missing it

My first thought was to use parse_url to handle this, here's a real quick sandbox to show the differences:
https://glot.io/snippets/ev374vwkbw

jom added some commits Nov 1, 2017

Add other options for posted field text sanitization
Adds 'sanitizer' parameter for text fields. You can pass a callable or use 'url', 'email', or 'url_or_email'
Sanitize the application and company website fields correctly
Uses the new ability to differentiate between fields for sanitization.
@jom

This comment has been minimized.

Show comment
Hide comment
@jom

jom Nov 10, 2017

Member

I've changed the URL discovery to null !== parse_url( $value, PHP_URL_HOST ). It speaks to a secondary issue with this field: validation. Might be worth another issue.

Member

jom commented Nov 10, 2017

I've changed the URL discovery to null !== parse_url( $value, PHP_URL_HOST ). It speaks to a secondary issue with this field: validation. Might be worth another issue.

@jom jom changed the title from [WIP] Sanitize field input using different strategies to Sanitize field input using different strategies Nov 10, 2017

@jom jom requested review from dbtlr and pgk Nov 10, 2017

@jom jom added this to the 1.29.1 milestone Nov 10, 2017

@dbtlr

dbtlr approved these changes Nov 10, 2017

Looks good to me

@jom jom merged commit 348431a into master Nov 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jom jom deleted the add/sanitization-strategies#1205 branch Nov 13, 2017

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