Handle malformed section & field IDs

Logotrop commented 1 year ago

When trying to Item Get a Login, OPItemFieldCollisionException is raised. Using Biometric OP integration Item_list along with other item commands works fine op item get "LoginName" --vault "Private" works ok in terminal

op -v 2.12.0

pyonepassword 3.3.1 -3.4.1.post0 pyonepassword 3.2.0 works just fine

from pyonepassword import OP opass = OP() opass.item_get("LoginName",vault="Private") Traceback (most recent call last): File "", line 1, in File "C:\Python38\lib\site-packages\pyonepassword\pyonepassword.py", line 150, in item_get op_item = OPItemFactory.op_item(output) File "C:\Python38\lib\site-packages\pyonepassword\op_items_op_item_type_registry.py", line 43, in op_item obj = cls._item_from_dict(item_dict) File "C:\Python38\lib\site-packages\pyonepassword\op_items_op_item_type_registry.py", line 34, in _item_from_dict return item_cls(item_dict) File "C:\Python38\lib\site-packages\pyonepassword\op_items\login.py", line 84, in init super().init(item_dict_or_json) File "C:\Python38\lib\site-packages\pyonepassword\op_items_op_items_base.py", line 29, in init self._field_map = self._initialize_fields() File "C:\Python38\lib\site-packages\pyonepassword\op_items_op_items_base.py", line 142, in _initialize_fields raise OPItemFieldCollisionException( pyonepassword.op_items.item_section.OPItemFieldCollisionException: Field already registered

zcutlip commented 1 year ago

Thanks for the report! I'll have a look to see what I broke. In the mean time, a couple questions:

I don't suppose you could dump that 1Password item using --format json, and share it?
Does the problem reproduce on a variety of items, or just that specific one?

zcutlip commented 1 year ago

Pasting in the above exception using a code fence for readability

>>> from pyonepassword import OP
>>> opass = OP()
>>> opass.item_get("LoginName",vault="Private")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python38\lib\site-packages\pyonepassword\pyonepassword.py", line 150, in item_get
    op_item = OPItemFactory.op_item(output)
  File "C:\Python38\lib\site-packages\pyonepassword\op_items\_op_item_type_registry.py", line 43, in op_item
    obj = cls._item_from_dict(item_dict)
  File "C:\Python38\lib\site-packages\pyonepassword\op_items\_op_item_type_registry.py", line 34, in _item_from_dict
    return item_cls(item_dict)
  File "C:\Python38\lib\site-packages\pyonepassword\op_items\login.py", line 84, in __init__
    super().__init__(item_dict_or_json)
  File "C:\Python38\lib\site-packages\pyonepassword\op_items\_op_items_base.py", line 29, in __init__
    self._field_map = self._initialize_fields()
  File "C:\Python38\lib\site-packages\pyonepassword\op_items\_op_items_base.py", line 142, in _initialize_fields
    raise OPItemFieldCollisionException(
pyonepassword.op_items.item_section.OPItemFieldCollisionException: Field  already registered

zcutlip commented 1 year ago

I've taken a look at this and the only way I can see this happening is if somehow there are two fields with the same ID. I don't think that's supposed to be possible, but either:

I'm wrong and and field IDs are not guaranteed to be unique (I don't think so, and I think this is documented somewhere)
There's something I'm doing subtly wrong to inadvertently cause a field ID collision

This doesn't reproduce on any of my test data, so @Logotrop, if you can get me some JSON samples (at least one, but more is better) that reproduce this issue, that will be super helpful

Fwiw, the reason this problem doesn't happen with 3.2.0, is I wasn't checking for field collisions. I added that additional checking in 3.3.x with item creation to help users avoid trying to create an item that 1Password won't accept.

zcutlip commented 1 year ago

bump @Logotrop

Let me know if you're able to provide some more context. I'm really curious about what could be causing this!

Logotrop commented 1 year ago

made powershell work, to get more info: I wanted to see if there is possibility that my vault had a duplicate ID, but doesn't seem like it I tried duplicating the login Item to a separate vault to see if it would raise the OPItemFieldCollisionException even when only one item exists in the vault, and yes, exception is still raised.

PS C:\Op> .\op.exe item list --tags "TOX"
ID                            TITLE               VAULT                 EDITED
t77otsb7punttclmrpy575xloi    TOX / Citrix        Test Vault            2 weeks ago
eni3hcwhaj26wakagg7klelyuy    TOX / Citrix        Private               2 weeks ago

PS C:\Op> .\op.exe item list --tags "TOX" --vault "Test Vault"
ID                            TITLE               VAULT                 EDITED
t77otsb7punttclmrpy575xloi    TOX / Citrix        Test Vault            2 weeks ago

PS C:\Op> .\op.exe item get eni3hcwhaj26wakagg7klelyuy
ID:          eni3hcwhaj26wakagg7klelyuy
Title:       TOX / Citrix
Vault:       Private (yvtufj6x4spnnubqevkrpd2dpa)

Part of the Code I am using, to recreate:

from pyonepassword import OP
from pyonepassword.api.exceptions import *

def OPSign(masterpassword = "", account = "Company", domain = "https://Company.ent.1password.com", email = "",secret = ""):
    global opass

    if OP.uses_biometric():
        try:
            #pass
            opass = OP()
        except:
            pass
OPSign()

if 'opass' in locals():
    isthereanytox = opass.item_list(tags=["tox"],vault="Test Vault")
    print(isthereanytox)
    if len(isthereanytox) > 0:
        tox = opass.item_get(isthereanytox[0].title,vault="Test Vault")

Logotrop commented 1 year ago

The login item I am using (renamed all fields, but still raises Exception for Field Collision):

{
  "id": "t77otsb7punttclmrpy575xloi",
  "title": "TOX / Citrix",
  "tags": ["TOX"],
  "version": 2,
  "vault": {
    "id": "yggwsslkxgzzke63636rzbni3y",
    "name": "Test Vault"
  },
  "category": "LOGIN",
  "last_edited_by": "WVDCMNOEHZBTDOLX6LJUVCHNT4",
  "created_at": "2020-06-11T14:46:19Z",
  "updated_at": "2023-01-26T16:51:02Z",
  "additional_information": "czzzuserid",
  "urls": [
    {
      "label": "website",
      "primary": true,
      "href": "https://420.0.0.0:69/"
    },
    {
      "label": "website 2",
      "href": "https://toxwebsite.com/Citrix/"
    }
  ],
  "sections": [
    {
      "id": "linked items",
      "label": "Related Items"
    },
    {
      "id": "Section_lwxu3ea3jtblonif5m3vjjtfmm",
      "label": "Saved on passwordwebsite.com"
    }
  ],
  "fields": [
    {
      "id": "password",
      "type": "CONCEALED",
      "purpose": "PASSWORD",
      "label": "DATA",
      "value": "SuperSecretPassword",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/DATA",
      "password_details": {
        "strength": "WEAK",
        "history": [
          "PreviousPassword"
        ]
      }
    },
    {
      "id": "username",
      "type": "STRING",
      "purpose": "USERNAME",
      "label": "username",
      "value": "czzzuserid",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/username"
    },
    {
      "id": "notesPlain",
      "type": "STRING",
      "purpose": "NOTES",
      "label": "notesPlain",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/notesPlain"
    },
    {
      "id": "gpotynowwnpcikkxpnnejrfasu",
      "section": {
        "id": "Section_lwxu3ea3jtblonif5m3vjjtfmm",
        "label": "Saved on passwordwebsite.com"
      },
      "type": "STRING",
      "label": "Page Number:",
      "value": "1",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/Saved on passwordwebsite.com/gpotynowwnpcikkxpnnejrfasu"
    },
    {
      "id": "2zrzt6gihao4k25n5e5cjv5lg4",
      "section": {
        "id": "Section_lwxu3ea3jtblonif5m3vjjtfmm",
        "label": "Saved on passwordwebsite.com"
      },
      "type": "STRING",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/Saved on passwordwebsite.com/2zrzt6gihao4k25n5e5cjv5lg4"
    }
  ]
}

zcutlip commented 1 year ago

Hmm, when I load that JSON it loads fine for me, however I've found several items in my own 1Password that generate field collisions.

The most common case is evidently it's possible to have field IDs that are an empty string. I'm wondering if your login item had one of those before you modified it?

To make your scenario as simple as possible, you can dump your item's JSON to a file and do something like:

from pyonepassword.api.object_types import OPLoginItem

gh_85_file = "gh-85.json"
gh_85_json_text = open(gh_85_file, "r").read()

op_item = OPLoginItem(gh_85_json_text)

This should produce the same exception

In the mean time I'll proceed under the assumption that you're encountering one or more fields where the ID is blank. I have a ton of those in my own account and I need to handle that properly.

zcutlip commented 1 year ago

here's an example where I have a field that has an empty string for its ID:

{
    "id": "",
    "type": "STRING",
    "label": "receiveNewsletter",
    "value": "✓",
    "reference": "op://Private/Example/receiveNewsletter"
}

When there is more than one field in the same item with an empty field ID, that's when the collision happens.

zcutlip commented 1 year ago

A couple more anomalies I found:

An item with a section that is duplicated, including its "unique" section ID
An item with a section that lacks any ID at all, not even an empty string

Here's the section list with the duplicated section. Not the "security code" sections.

"sections": [
    {
        "id": "linked items",
        "label": "Related Items"
    },
    {
        "id": "Section_453C35FE21C643AEAFAFA53BA89D7CF5",
        "label": "Security Code"
    },
    {
        "id": "Section_453C35FE21C643AEAFAFA53BA89D7CF5",
        "label": "Security Code"
    },
    {
        "id": "Section_4BF92D24E2C54E16A4D24AD9F2EBCABE",
        "label": "Security Question"
    }
]

And here's the item with a section completely lacking section ID, the "VPN Cert" section:

"sections": [
    {
      "id": "admin_console",
      "label": "Admin Console"
    },
    {
      "id": "hosting_provider_details",
      "label": "Hosting Provider"
    },
    {
      "id": "linked items",
      "label": "Related Items"
    },
    {
      "label": "VPN Cert"
    }
]

zcutlip commented 1 year ago

A couple interesting observations:

Note that if you use dump the first one and use that as a template to create a duplicate it fails due to the duplicate sections.
If you dump the second one and use it as a template, the "VPN Cert" section gets assigned a randomized section ID
If you dump one of the items with one or more "empty string" field IDs and use that as a template, the field IDs get populated with randomized IDs

I suspect all of these items were created with earlier versions of the app that didn't enforce things particularly strictly, and the backend didn't either at the time (though with end to end encryption, I'm not sure how much enforcement can happen on the backend). I think everything I've created in the past year or so appears to be well-formed.

I'm going to broaden this bug to address three observed instances of malformed item objects, and hopefully that addresses the original issue.

Logotrop commented 1 year ago

Yes! I tried duplicating the original again and I have this in the json. What is weird is that the app even when duplicating in newer version, keeps all the empty fields as well.

I have 44 of these on the original and the duplicated one.

{
      "id": "",
      "type": "STRING",
      "label": "accountTable.selectAllChecked",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/accountTable.selectAllChecked"
    },
    {
      "id": "",
      "type": "STRING",
      "label": "accountTable.multiSelectValues",
      "reference": "op://Test Vault/t77otsb7punttclmrpy575xloi/accountTable.multiSelectValues"
    },

zcutlip commented 1 year ago

Okay, thanks! This shouldn't be too troublesome to fix, but I need to think through it a bit.

This is the same code path as when you're creating an item. On creation, the enforcement is important because I want to catch errors as early as possible so I can raise a specific and meaningful exception (rather than letting op fail and having to scrape its error message).

However, it's now clear that on item retrieval this strict enforcement can be a problem. I need to think through how to know programmatically when enforcement is and isn't desired.

It is also possible, by design, to just instantiate a pyonepassword item object from JSON or a dictionary without using OP to retrieve it. In that case the origin of the dictionary is unknown, so knowing whether to enforce proper structure may be hard to determine.

zcutlip commented 1 year ago

I have more or less confirmation in the 1Password developers slack that these non-conforming item objects are likely anomalies where some version of the 1Password app had a bug affecting entry creation.

So in terms of understanding the structure of a well formed item object, these anomalies don't represent a misunderstanding.

That said pyonepassword still needs to handle anomalous data gracefully, so on we go

zcutlip commented 1 year ago

Sorry this has taken so long. Given the discovery that it is expected behavior for op to return non-conforming items on occasion, I wanted to come up with a solution to optionally handle that gracefully, while keeping strict validation as the default.

I've added an API to control item validation policy that lets you optionally enable relaxed validation for a few different scopes.

You can test that out in the dev/85-handle-malformed-section-field-ids branch, and it's documented here: https://github.com/zcutlip/pyonepassword/blob/dev/85-handle-malformed-section-field-ids/ITEM_VALIDATION.md

I believe this API should be stable at this point; I don't for see making any changes before release.

I hope to cut a release soon, but this has been a much larger set of changes than anticipated. So I want to make sure everything's thoroughly tested, and there's nothing I'm overlooking.

zcutlip commented 1 year ago

This should be addressed by PR #88 which has been merged into development.

I've got one more set of changes in testing (#86) which I plan to merge soon, then I can merge it all into main and cut a new release

zcutlip commented 1 year ago

This should be addressed in 3.5.0 which is now available in PyPI via pip

You can relax validation so malformed item dictionaries hopefully won't raise an exception. For example you can call op.item_get(item_name, relaxed_validation=True).

Be sure to check out ITEM_VALIDATION.md for more details about item validation policy API

zcutlip / pyonepassword

Handle malformed section & field IDs #85