Project

General

Profile

Actions

action #94901

closed

Simple validation of parameters on /tests/overview

Added by livdywan over 3 years ago. Updated about 3 years 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:

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 1 (1 open0 closed)

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

Actions
Actions #1

Updated by livdywan over 3 years ago

  • Description updated (diff)
Actions #2

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? :)

Actions #3

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.

Actions #4

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.

Actions #5

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/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

Actions #6

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.

Actions #7

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.

Actions #8

Updated by okurz over 3 years ago

  • Copied to action #95030: [easy][beginner] Better, complete validation of parameters on /tests/overview added
Actions #9

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

Actions #10

Updated by kraih about 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

Actions #11

Updated by okurz about 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?

Actions #12

Updated by kraih about 3 years ago

  • Status changed from In Progress to Feedback

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

Actions #13

Updated by ilausuch about 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.

Actions #14

Updated by okurz about 3 years ago

  • Status changed from Feedback to Resolved

No response. I assume this means "Resolved"

Actions

Also available in: Atom PDF