+- CodeIgniter Forums (https://forum.codeigniter.com)
+-- Forum: CodeIgniter 4 (https://forum.codeigniter.com/forumdisplay.php?fid=28)
+--- Forum: CodeIgniter 4 Discussion (https://forum.codeigniter.com/forumdisplay.php?fid=31)
+--- Thread: Model::update() is dangerous (/showthread.php?tid=84833)
RE: Model::update() is dangerous - ikesela - 11-16-2022
maybe can fix this, byass id checking if it is NULL , there should be no if($id), no checking need so WhereIn to be work
Code:
protected function doUpdate($id = null, $data = null): bool
{
$escape = $this->escape;
$this->escape = [];
$builder = $this->builder();
if ($id) {
$builder = $builder->whereIn($this->table . '.' . $this->primaryKey, $id);
}
// Must use the set() method to ensure to set the correct escape flag
foreach ($data as $key => $val) {
$builder->set($key, $val, $escape[$key] ?? null);
}
return $builder->update();
}RE: Model::update() is dangerous - michalsn - 11-16-2022
There is no good solution for this for now. Any change would be a BC break.
Developers may use composite keys with the model.
PHP Code:
$model->where(['key1' => 'value1', 'key2' => 'value2'])->update(null, $data);
I believe this was a design choice.
Whether we like the current model behavior or not, the code presented here and in the repo lacks any basic checks in update() / postUpdate() method:
- why is null/false acceptable as a value at all?
- I can perform an update query on every single row in the table without checking if row exists or not.
Meanwhile, the code responsible for showing the view: getEdit() has these checks. There should be no difference when validating the $id for getEdit() and postUpdate().
I get your reasoning with update(null, $data), but the real danger here is poorly-written code, not the framework. In this case, simple value validation for $id would solve all the problems.
RE: Model::update() is dangerous - kenjis - 11-16-2022
Certainly, the current API cannot be changed without a BC break. However, an API that is not well designed should be changed to something more secure.
All inputs should be validated. Yes, that is correct. But have you guys ever forgotten to validate input values?
With the current API, developers create a vulnerability that if they forget to validate $id even once, all records will be updated.
I can't think of a use case like this sample where you want to update all records at the same time when updating a single record.
UPDATE table and UPDATE table WHERE id = ? are fundamentally different use cases. There is no need to provide both in the same method.
RE: Model::update() is dangerous - michalsn - 11-17-2022
kenjis Wrote:But have you guys ever forgotten to validate input values?
Ok, this is hard to argue. This probably happened to everyone.
kenjis Wrote:I can't think of a use case like this sample where you want to update all records at the same time when updating a single record.
As I said before. We have to think of it from the point of view of the entire API that comes with the Model.
Developers may write queries that don't necessarily update all the records when the null is set, like:
PHP Code:
$model->where(...)->set(...)->update();
So, if we suddenly start to require an $id, this will break people's code. Not sure what is more damaging.
The only workaround I see for this is checking if the where part was set before or not. Something like:
PHP Code:
protected function doUpdate($id = null, $data = null, bool $allowNullId = false): bool
{
$escape = $this->escape;
$this->escape = [];
$builder = $this->builder();
if ($id === null && ! $allowNullId && empty($builder->getCompiledQBWhere())) {
throw new DatabaseException(
'Updates are not allowed unless they contain a "where" or "like" clause.'
);
}
if ($id) {
$builder = $builder->whereIn($this->table . '.' . $this->primaryKey, $id);
}
// Must use the set() method to ensure to set the correct escape flag
foreach ($data as $key => $val) {
$builder->set($key, $val, $escape[$key] ?? null);
}
return $builder->update();
}
It would still be a BC break, but the overall functionality would remain. Still, I'm not sure this is the best thing to do.
RE: Model::update() is dangerous - kenjis - 11-17-2022
I created another controller. I added the validation for id.
But it is still vulnerable.
https://github.com/kenjis/ci4-model-update-danger/blob/78b08c5bb1e7259c3f4863af63df86920fdac9f8/app/Controllers/News3.php#L101-L123
PHP Code:
public function update()
{
$id = $this->request->getPost('id');
// Adds validation rule for id.
$rules = array_merge($this->rules, ['id' => 'required|is_natural_no_zero']);
if ($this->validate($rules)) {
$title = $this->request->getVar('title');
$slug = url_title($title, '-', true);
$data = [
'title' => $title,
'slug' => $slug,
'body' => $this->request->getVar('body'),
];
$this->model->update($id, $data);
return $this->response->redirect(site_url('news3/' . $slug));
}
return $this->edit($id);
}
RE: Model::update() is dangerous - kenjis - 11-17-2022
(11-17-2022, 09:12 AM)michalsn Wrote: The only workaround I see for this is checking if the where part was set before or not. Something like:
PHP Code:protected function doUpdate($id = null, $data = null, bool $allowNullId = false): bool
{
$escape = $this->escape;
$this->escape = [];
$builder = $this->builder();
if ($id === null && ! $allowNullId && empty($builder->getCompiledQBWhere())) {
throw new DatabaseException(
'Updates are not allowed unless they contain a "where" or "like" clause.'
);
}
if ($id) {
$builder = $builder->whereIn($this->table . '.' . $this->primaryKey, $id);
}
// Must use the set() method to ensure to set the correct escape flag
foreach ($data as $key => $val) {
$builder->set($key, $val, $escape[$key] ?? null);
}
return $builder->update();
}
It would still be a BC break, but the overall functionality would remain. Still, I'm not sure this is the best thing to do.
It seems no problem. It prevents unexpected all record updates.
What's your concern?
RE: Model::update() is dangerous - michalsn - 11-18-2022
(11-17-2022, 05:26 PM)kenjis Wrote: I created another controller. I added the validation for id.
But it is still vulnerable.
https://github.com/kenjis/ci4-model-update-danger/blob/78b08c5bb1e7259c3f4863af63df86920fdac9f8/app/Controllers/News3.php#L101-L123
PHP Code:public function update()
{
$id = $this->request->getPost('id');
// Adds validation rule for id.
$rules = array_merge($this->rules, ['id' => 'required|is_natural_no_zero']);
if ($this->validate($rules)) {
$title = $this->request->getVar('title');
$slug = url_title($title, '-', true);
$data = [
'title' => $title,
'slug' => $slug,
'body' => $this->request->getVar('body'),
];
$this->model->update($id, $data);
return $this->response->redirect(site_url('news3/' . $slug));
}
return $this->edit($id);
}
Ok, you got me here. How this is still a vulnerability?
kenjis Wrote:It seems no problem. It prevents unexpected all record updates.
What's your concern?
Not a big fan of the API I came up with.
RE: Model::update() is dangerous - kenjis - 11-19-2022
(11-15-2022, 09:12 PM)iRedds Wrote: And what solution do you propose?
See https://github.com/codeigniter4/CodeIgniter4/pull/6883
RE: Model::update() is dangerous - InsiteFX - 11-19-2022
Nver ever trust user input data!
If it's a bad vulnerability then we need to fix it before it gets out of control.
RE: Model::update() is dangerous - kenjis - 11-28-2022
I would like to change the API:
- [change] update($id, $data) only for updating a specific id or specific ids.
- [add] updateAll($data) for updating without ids.