Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Country field isn't validated properly #15344

Closed
2 tasks done
StarlessNights opened this issue Aug 20, 2024 · 10 comments
Closed
2 tasks done

API: Country field isn't validated properly #15344

StarlessNights opened this issue Aug 20, 2024 · 10 comments

Comments

@StarlessNights
Copy link
Contributor

Debug mode

Describe the bug

When POSTing a location,1 the API will accept any string in the country field. This arbitrary string will also be returned in subsequent GET requests.

However in the UI country is represented as a dropdown. Selecting a country from the UI leads to a 2-letter country code to be returned from the API.

This behavior is inconsistent. The API should enforce the same restriction on the country field.

Reproduction steps

POST /locations with the following body:

{
  "name": "New loc",
  "country": "Nonsense"
}

The response will indicate that the operation was a success, and that the country field was set to "Nonsense":

{
  "status": "success",
  "messages": "Location created successfully.",
  "payload": {
    "id": 19,
    "name": "New loc",
    "image": null,
    "address": null,
    "address2": null,
    "city": null,
    "state": null,
    "country": "Nonsense",
    "zip": null,
    "phone": null,
    "fax": null,
    "assigned_assets_count": 0,
    "assets_count": 0,
    "rtd_assets_count": 0,
    "users_count": 0,
    "currency": null,
    "ldap_ou": null,
    "created_at": {
      "datetime": "2024-08-20 13:10:39",
      "formatted": "20.08.2024 13:10"
    },
    "updated_at": {
      "datetime": "2024-08-20 13:10:39",
      "formatted": "20.08.2024 13:10"
    },
    "parent": null,
    "manager": null,
    "children": [],
    "available_actions": {
      "update": true,
      "delete": false,
      "bulk_selectable": {
        "delete": false
      },
      "clone": true
    }
  }
}

The created location will look like this in the UI:

Snipe-IT location country

Expected behavior

The API should reject nonsense strings, and only accept valid country identifiers.

Screenshots

No response

Snipe-IT Version

7.0.10

Operating System

Ubuntu (Container)

Web Server

Apache

PHP Version

8.1

Operating System

No response

Browser

No response

Version

No response

Device

No response

Operating System

No response

Browser

No response

Version

No response

Error messages

No response

Additional context

Container in Kubernetes.

Footnotes

  1. This likely affects other requests as well. PATCH, PUT and probably /users endpoint too.

@snipe
Copy link
Owner

snipe commented Aug 20, 2024

I'm not sure we want to change that - lots of people use the API in unexpected ways, and it could break integrations if we suddenly introduce enforcement there. Really, it's not a special field, just text, and some folks barely use the UI at all.

@ramanjitsevo
Copy link

ramanjitsevo commented Aug 22, 2024

I believe it should be fixed because, if random text is added to the Country field, it is unable to detect the country name from the dropdown list.
image
image
image

@snipe
Copy link
Owner

snipe commented Aug 22, 2024

I understand that, but breaking existing integrations/imports isn't really an option either.

@StarlessNights
Copy link
Contributor Author

It is an option, and unavoidable in the long run. Deprecation is a normal (and essential) part of software lifecycle.

That said, this particular issue might not be worth it alone. Perhaps queue with other breaking changes in the the future?

@snipe
Copy link
Owner

snipe commented Aug 22, 2024

It is an option, and unavoidable in the long run.

Please don't tell me what's an option and what isn't in my own software. We have 10+ years of knowledge about our user's workflows and challenges. Often times, the existing integrations our users have were created before the current admin was even hired, and they often don't even know where those integrations live or how they work. Most of our users are not developers, they're IT people, so turning those ships can be slow if not impossible.

Additionally, folks on the hosted platform won't have a choice as to whether to upgrade or not, so the next time we roll the fleet, we could end up breaking a lot of people's integrations.

@StarlessNights
Copy link
Contributor Author

Thanks for opening up your perspective.

@snipe
Copy link
Owner

snipe commented Aug 22, 2024

The only way I can see this potentially working (and even then, it's going to cause a ton of support tickets) is if we do a migration to "best guess" the code based on the word. This wouldn't work for typos (which we see a ton of), and definitely seems like the harm it could cause could be worse than the benefit. Since we use model level validation, making this change would mean if an API endpoint is trying to just update a user's name for example, the save would fail if the bad data was already on the user model, since the model itself would no longer validate.

This could potentially cause SCIM failures with unexplained reasons (SCIM does not always do a great job of surfacing failures) and a ton of other issues. And for LDAP sync, if the admin has country mapping turned on, who knows what mess they have in their directory that we're pulling from.

@StarlessNights
Copy link
Contributor Author

Good points. Another angle would be to change the UI element instead. To display, if not input, freeform strings.

@snipe
Copy link
Owner

snipe commented Aug 22, 2024

That's what I'm looking into (via Select2's tagging option)

@snipe
Copy link
Owner

snipe commented Aug 22, 2024

#15367

snipe added a commit that referenced this issue Aug 22, 2024

Verified

This commit was signed with the committer’s verified signature.
neo1973 Markus Härer
…untry_select2

Fixed #15344 - make select2 for countries freeform-ish
@snipe snipe closed this as completed in 3ac0702 Aug 22, 2024
FlorentDotMe pushed a commit to TelecomsSansFrontieres/snipe-it that referenced this issue Sep 9, 2024
Signed-off-by: snipe <snipe@snipe.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants