How to make sef-url field safe

[Jimmi08]

I noticed this for category-sef field

public function beforeCreate($new_data, $old_data)
	{
		if (empty($new_data['category_sef']))
		{
			$new_data['category_sef'] = eHelper::title2sef($new_data['category_name']);
			
		}
		else
		{
			$new_data['category_sef'] = eHelper::secureSef($new_data['category_sef']);
		}

		$sef = e107::getParser()->toDB($new_data['category_sef']);

		if (e107::getDb()->count('news_category', '(*)', "category_sef='{$sef}'"))
		{
			e107::getMessage()->addError(LAN_NEWS_65);
			return false;
		}

		if (empty($new_data['category_order']))
		{
			$c = e107::getDb()->count('news_category');
			$new_data['category_order'] = $c ? $c : 0;
		}

		return $new_data;
	}

Shouldn't be secureSef() used in first option too?

$new_data['category_sef'] = eHelper::title2sef($new_data['category_name']);
$new_data['category_sef'] = eHelper::secureSef($new_data['category_sef']);

And there is toDB() used... is possible to replace it with toFilter()

I don't ask for core changes, I just use this very often and I want make it right,

Thanks

[Jimmi08]

This is in title2Sef:

$title = preg_replace('/[^\w\pL\s.-]/u', '', strip_tags(e107::getParser()->toHTML($title, TRUE)));
$title = trim(preg_replace('/[\s]+/', ' ', str_replace('_', ' ', $title)));

This is in secureSef:
return trim(preg_replace('/[^\w\pL\s\-+.,]+/u', '', strip_tags(e107::getParser()->toHTML($sef, TRUE))));

Maybe it does the same thing.

[Moc]

@CaMer0n @Deltik Your take on this please?

[Deltik]

I'd defer to @CaMer0n, since I don't have much experience with the search engine-friendly helpers.

eHelper::title2sef() does way more processing than eHelper::secureSef(), but perhaps that wasn't the original intention for eHelper::title2sef()?

It looks like eHelper::title2sef() is used for fields that look title-y while eHelper::secureSef() is used to strip undesirable characters for SEF usage.

---

As for the usage of e_parse::toDB(), I believe that that method is one of the worst design decisions made for e107 because it tries to work around both SQL and HTML injections by replacing characters that could be used for either and storing them in the database so that their usages don't need to be escaped where they are actually used. It sounds like a good idea that would solve both kinds of injection in one pass, but it confounds the data type and has led to bugs in the past where we don't know if the string we have had been run through e_parse::toDB().

You should instead store your values in the database with prepared statements and do any necessary frontend escaping at the presenter.

[Jimmi08]

@Deltik Thanks for your answer.

e_parse::toDB() - this is used in the core, so why I was asking if it shouldn't be replaced.

Ad: You should instead store your values in the database...
Admin UI is responsible for saving data in the admin area.
And I thought e_db_pdo is doing this when using new array way with insert() and update() method.