Project

General

Profile

action #94901

Simple validation of parameters on /tests/overview

Added by cdywan 3 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
Feature requests
Target version:
Start date:
2021-06-29
Due date:
2021-07-22
% Done:

0%

Estimated time:
Difficulty:

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

Related issues

Copied to openQA Project - action #95030: Better, complete validation of parameters on /tests/overviewNew2021-06-29

History

#1 Updated by cdywan 3 months ago

  • Description updated (diff)

#2 Updated by okurz 3 months 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? :)

#3 Updated by cdywan 3 months 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.

#4 Updated by tinita 3 months 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.

#5 Updated by cdywan 3 months 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/overview that 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

#6 Updated by kraih 3 months 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.

#7 Updated by okurz 3 months 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.

#8 Updated by okurz 3 months ago

  • Copied to action #95030: Better, complete validation of parameters on /tests/overview added

#9 Updated by okurz 3 months 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

#10 Updated by kraih 2 months 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

#11 Updated by okurz 2 months 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?

#12 Updated by kraih 2 months ago

  • Status changed from In Progress to Feedback

Technically not validation, but i think it fixes the issue well enough.

#13 Updated by ilausuch 2 months 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.

#14 Updated by okurz about 2 months ago

  • Status changed from Feedback to Resolved

No response. I assume this means "Resolved"

Also available in: Atom PDF