action #94901
closedSimple validation of parameters on /tests/overview
Description
Motivation¶
The tests overview page doesn't validate its input. Omitting the group leads to the site picking up some results from the entire db. Invalid values lead to server errors.
Examples:
Acceptance criteria¶
- AC2: Request fails if invalid flavor is specified
Suggestion¶
- Add parameter validation to lib/OpenQA/WebAPI/Controller/Test.pm for simple, obvious cases
Updated by okurz over 3 years ago
- Subject changed from No validation of parameters on /tests/overview to Better validation of parameters on /tests/overview
- Category set to Feature requests
- Priority changed from Normal to Low
- Target version set to future
Sure we can improve the parameter validation. But calling the route without a job group has a purpose. It will still resolve test results, just over all job groups, including the jobs outside any job group. So maybe change AC1 to say /tests/overview without groupid is documented or something or appears somewhere as link in the webui? :)
Updated by livdywan over 3 years ago
- Description updated (diff)
- Status changed from New to Workable
okurz wrote:
Sure we can improve the parameter validation. But calling the route without a job group has a purpose. It will still resolve test results, just over all job groups, including the jobs outside any job group. So maybe change AC1 to say /tests/overview without groupid is documented or something or appears somewhere as link in the webui? :)
Good point. It seemed random to my uninformed eye. We'll need to address the overload differently then but that's okay.
Updated by tinita over 3 years ago
okurz wrote:
So maybe change AC1 to say /tests/overview without groupid is documented or something or appears somewhere as link in the webui? :)
Then there needs to be a configurable limit on the number of jobs.
In the o3 accesslog from today I find a call to /tests/overview that took 300 seconds. 5 minutes.
Updated by livdywan over 3 years ago
tinita wrote:
okurz wrote:
So maybe change AC1 to say /tests/overview without groupid is documented or something or appears somewhere as link in the webui? :)
Then there needs to be a configurable limit on the number of jobs.
In the o3 accesslog from today I find a call to/tests/overviewthat took 300 seconds. 5 minutes.
I didn't include it here because it's an orthogonal problem. Have a link at my PR, though: https://github.com/os-autoinst/openQA/pull/3990
Updated by kraih over 3 years ago
- Assignee set to kraih
This is actually required for #91647. Since an empty flavor value in the filter form results in all jobs getting listed. Which is a big problem.
Updated by okurz over 3 years ago
I understand that it's "clear wrong" but we have likely lived with that going unnoticed for years and no one complained. We have this feature request but please respect the priorization and consider "parameter validation" as out of scope and not in our backlog for now.
Updated by okurz over 3 years ago
- Copied to action #95030: [easy][beginner] Better, complete validation of parameters on /tests/overview added
Updated by okurz over 3 years ago
- Subject changed from Better validation of parameters on /tests/overview to Simple validation of parameters on /tests/overview
- Description updated (diff)
- Target version changed from future to Ready
As discussed in weekly 2021-07-02 here we should restrict ourselves to the "bare minimum" as you stated, e.g. no empty "flavor" parameter value. Split out the advanced further part into #95030
Updated by kraih over 3 years ago
Opened a PR to add input filtering (so we can get rid of dangerous cases like ?flavor=). https://github.com/os-autoinst/openQA/pull/4003
Updated by okurz over 3 years ago
- Status changed from Workable to In Progress
@kraih do you see the ticket resolved with https://github.com/os-autoinst/openQA/pull/4003 merged?
Updated by kraih over 3 years ago
- Status changed from In Progress to Feedback
Technically not validation, but i think it fixes the issue well enough.
Updated by ilausuch over 3 years ago
- Due date set to 2021-07-22
We understand your answer is a yes, and therefore we could resolve the ticket. But before doing that we would like to check with you if there is something else to consider before do that.
We add a remider (due date) to be sure to come back to this ticket if we don't have any change.
Updated by okurz over 3 years ago
- Status changed from Feedback to Resolved
No response. I assume this means "Resolved"