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

"Maximum depth of replacement has been reached" from unknown placeholder (curly bracket pairs in passwords/notes/...) #1741

Closed
yan12125 opened this issue Mar 16, 2018 · 34 comments · Fixed by #10846

Comments

@yan12125
Copy link
Contributor

yan12125 commented Mar 16, 2018

Expected Behavior

No errors printed in the console

Current Behavior

Several error messages like this are shown:

Maximum depth of replacement has been reached. Entry uuid: <REDACTED>

Possible Solution

Need brainstorming :) Maybe the same unknown placeholder shouldn't be processed more than once?

Steps to Reproduce (for bugs)

  1. Create an entry with password "{foo}"
  2. Click on another entry and click the just created entry

Context

Several long generated passwords contain the pattern {foo}, leading to quite a few error messages in the console, which is somewhat annoying

Debug Info

KeePassXC - 版本 2.3.1-snapshot
Build Type: Snapshot
修訂:46e8e3d

函式庫:

  • Qt 5.10.1
  • libgcrypt 1.8.2

作業系統:Arch Linux
處裡器架構:x86_64
核心:linux 4.15.8-1-ARCH

已啟用的擴充元件:

  • Auto-Type

BTW, I don't remember I got such a message when I was reporting #1137

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Mar 17, 2018

This part of the code will be refactored in 2.4, I will keep the issue open to remember about it

@dminca
Copy link

dminca commented May 18, 2018

This one actually freezed my OS, I was unable to control it unless rebooted. This is the boot log that caused system freeze:

May 18 17:35:58 p0laroid keepassxc[4982]: QObject::startTimer: Timers cannot have negative intervals
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:14 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:
May 18 17:36:19 p0laroid keepassxc[4982]: Maximum depth of replacement has been reached. Entry uuid:

I didn't paste the uuid's as well as I don't think that's ok...

@TheZ3ro
Copy link
Contributor

TheZ3ro commented May 18, 2018

@dminca do some of your entries/fields have a strange pattern of {}?

@dminca
Copy link

dminca commented May 18, 2018

@TheZ3ro yes they have. My db has also been opened with KeeWeb and that one had the option to add references to usernames. I think that's the issue, I gotta hunt the entries down and replace them one by one.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented May 19, 2018

@dminca If you can, paste some example of those reference so I can reproduce your problem (obviously remove sensisitive data)

@dminca
Copy link

dminca commented May 19, 2018

Sure, no problem.

This is a username reference:

{REF:U@I:F268D4F0E0247219A14F9A525474001D}

This is a password reference:

{REF:P@I:E0251A47FF33684BBE4E19113A12FF7F}

@marvinwankersteen
Copy link

I think I have the same problem. I am new to keepassxc and before I used keepass2 with firefox and the addon "Kee". In every entry, which I added from firefox, are fields with {...}.

e.g.:

Name: KPRPC JSON
{"version":1,"formFieldList":[{"name":"data[Account][password]","displayName":"KeePass password","value":"{PASSWORD}","type":"FFTpassword","id":"AccountPassword","page":1},{"name":"data[Account][username]","displayName":"KeePass username","value":"{USERNAME}","type":"FFTusername","id":"AccountUsername","page":1}],"alwaysAutoFill":false,"neverAutoFill":false,"alwaysAutoSubmit":false,"neverAutoSubmit":false,"priority":0,"altURLs":[],"hide":false,"blockHostnameOnlyMatch":false,"blockDomainOnlyMatch":false}

Maybe that helps.

@liar666
Copy link

liar666 commented Aug 29, 2018

Hi,
I also add the error message "Maximum depth of replacement has been reached" reported above even if my entries do not contains {}.

This message disappeared, but I still have this one:

$ keepassxc
QObject::startTimer: Timers cannot have negative intervals
$

@kronenpj
Copy link

For me, this error only occurred on entries that had "Auto-Type:" or "Auto-Type-Window:" lines in the notes. Once I eliminated these from those entries, the errors stopped. It had nothing to do with username or password entries.

@droidmonkey droidmonkey modified the milestones: v2.4.0, v2.5.0 Jan 16, 2019
@rbasoalto
Copy link

I'm still seeing this with the latest Linux AppImage (2.4.3). I do have a bunch of entries that have JSON in the comments field. The annoying part is that the search box clears after a second or so, while spamming the terminal with Maximum depth of replacement has been reached. Entry uuid: {<some uuid>} for several different uuids.

@phoerious phoerious removed this from the v2.5.0 milestone Oct 26, 2019
@adatum
Copy link

adatum commented Nov 5, 2019

I'm seeing the same message Maximum depth of replacement has been reached. Entry uuid: {<uuid>} in journalctl for one specific uuid. It's not causing me any noticeable problems. I haven't figured out the cause though.

@franek
Copy link

franek commented Jan 22, 2021

Same problem here with keepassxc 2.5.2. It is due to an entry with json.

My password or my Notes contain something like that :

'{"http-basic":{"repo.packagist.com":{"username":"fdsf","password":"ffsfdsf"}}}'

Is there a workaround ?

@droidmonkey
Copy link
Member

Don't store json in the notes. Put it in a text atrachment.

@franek
Copy link

franek commented Jan 22, 2021

Don't store json in the notes. Put it in a text atrachment.

Thanks for your answer. It could be an answer but I am using "keepassxc" with chezmoi and it seems that we can't retrieve attachement from keepassxc-cli (cf. #5538).

@yan12125
Copy link
Contributor Author

yan12125 commented Sep 12, 2021

Some other useful tips to workaround this issue:

  • To find out passwords that may trigger this warning, you can search with regex (ex: *password:".*\{.*\}.*")
    I created a script to find all passwords that may trigger this warning. To use it, export the database as XML via a command like keepassxc-cli export --key-file foo.key foo.kdbx > foo.xml (note that the output file contains passwords in plaintexts), and run python issue1741-workaround.py foo.xml
issue1741-workaround.py
import re
import sys
import textwrap
import xml.etree.ElementTree as ET

def main():
    # Find passwords that may cause unwanted warnings
    # https://github.com/keepassxreboot/keepassxc/issues/1741
    dom = ET.parse(sys.argv[1])
    for group in dom.findall('.//Group'):
        for entry in group.findall('./Entry'):
            attrs = {}
            for attr in entry.findall('./String'):
                key = attr.find('Key').text
                value = attr.find('Value').text
                if not value:
                    continue
                attrs[key] = value
            if re.match(r'\{.*\}', attrs.get('Password', '')):
                print(textwrap.dedent(f'''
                    Title: {attrs.get('Title') or '(None)'}
                    Username: {attrs.get('UserName') or '(None)'}
                    Password: {attrs['Password']}
                '''))

if __name__ == '__main__':
    main()
  • If you're as lucky as me and control the code that consumes JSON credentials, you can also use JSON lists instead JSON objects. For example,
[["aws", "token1"],["cloudflare","token2"]]

instead of

{"aws":"token1","cloudflare":"token2"}
  • In Password Generator, there is a useful field "Do not include". I put curly brackets in that field {}, so future passwords will never trigger this warning

@archont00
Copy link

It is a nice coincidence that this very KeePassXC Wiki has entries which look like JSON and trigger the error messages...

Is there a list of forbidden character combinations in Notes?

https://github.com/keepassxreboot/keepassxc.wiki.git

...

Special Key	Code
+	{+}
%	{%}
^	{^}
~	{~}
(, )	{(}, {)}
[, ]	{[}, {]}
{, }	{{}, {}}

...

@droidmonkey
Copy link
Member

droidmonkey commented Sep 13, 2021

Those codes are for Auto-Type

@grassbd
Copy link

grassbd commented Oct 12, 2021

I needed to put JSON into the Password field, and now the logs are flooded with this Error Message.
would be nice if the Password Field is ignored by any replacement Logic.
KeepassXC: 2.6.6

@recolic
Copy link

recolic commented Feb 11, 2022

Don't store json in the notes. Put it in a text atrachment.

Stop storing json in notes is a workaround, not a solution.

Users should be able to store any text in notes. We need to solve the issue instead of giving a workaround.

@xlash123
Copy link

I've also been experiencing this issue. I discovered that deleting everything in the Keyring folder solved the issue. Maybe the autogenerated entries in there created the right conditions for this error?

@goekce
Copy link

goekce commented Sep 5, 2022

Thanks for the hint @xlash123. When searching for certain keywords, Keepassxc would freeze for about 30 seconds and show the following messages on the terminal:

...
Maximum depth of replacement has been reached. Entry uuid: {__SOME_UUID__}

When the freeze ends:

Failed to read proxy message:  "garbage at the end of the document"

Deleting the entry with the corresponding UUID (which had the title ...Production.RETAIL.User...) solved my issue.

@phoerious
Copy link
Member

I recommend you store JSON as file attachments instead of notes.

@michaelk83
Copy link

Why is JSON parsed at all? KPXC should not care what the content of the fields is, JSON or otherwise. If it is needed for REF handling, pass it through a regex first, to make sure it is actually a reference and not JSON data.

Having JSON in fields makes sense in some use cases, particularly passwords stored through Secret Service, where a client application may want to store structured data in some of its secrets.

@phoerious
Copy link
Member

If you want to store structured text fields, you should be using individual string fields for that. For anything else, use custom data.

@michaelk83
Copy link

michaelk83 commented Sep 7, 2022

The Secret Service API does not support multiple (protected) string fields or custom data.
https://specifications.freedesktop.org/secret-service/latest/ch14.html#type-Secret
https://specifications.freedesktop.org/secret-service/latest/re02.html#org.freedesktop.Secret.Collection.CreateItem
There is only one value field in the Secret struct. It does support multiple additional attributes, but those are only intended for lookups. They're not meant to store data, let alone data that should be protected (keep in mind that KPXC isn't the only Secret Service provider; calls made by client applications would be the same as for other providers).

The only spec-compatible way to store structured protected data is either shove it all in the single value field (which I think goes to Password in KPXC), or use multiple entries. This is up to each client application to decide, and is entirely out of the user's control. It is quite likely that some applications will choose to use the structured value approach. I think I recall Chromium doing something like that?

Note also the content_type field of the Secret struct: this may very well be set to "application/json".

And regardless, KPXC should really not try to parse arbitrary JSON, no matter what is the source or use case of it.

@droidmonkey
Copy link
Member

We do not parse JSON. We only detect a reference if the text matches a pretty strict reference pattern.

QRegularExpressionMatch match = EntryAttributes::matchReference(placeholder);

QRegularExpressionMatch EntryAttributes::matchReference(const QString& text)
{
    static QRegularExpression referenceRegExp(
        "\\{REF:(?<WantedField>[TUPANI])@(?<SearchIn>[TUPANIO]):(?<SearchText>[^}]+)\\}",
        QRegularExpression::CaseInsensitiveOption);


    return referenceRegExp.match(text);
}

@akkana
Copy link

akkana commented Dec 27, 2022

This can happen even without Notes and without JSON anywhere. I just started using keepassxc. With a brand-new install (from the Debian sid repo). I created a database, added two test entries with generated passwords and no Notes, then noticed my terminal was full of these lines, and they just kept coming as long as keepassxc was running. Every time I changed desktop back to the desktop with the keepassxc window, I'd notice several more "Maximum depth" lines on the terminal.

But I can't reproduce it. I removed ~/.config/Passwords.kdbx and ~/.config/keepassxc and started over, following the exact same steps, and this time I didn't see anything on the terminal except "QWidget::setWindowModified: The window title does not contain a '[*]' placeholder" (which happens repeatably when clicking Done on the password dialog for creating the database). So I don't know what was causing the "Maximum depth" errors.

Is it possible that they're triggered by something in a generated password, and that's why they only happen sometimes? I didn't save the generated passwords from that first run, unfortunately.

@droidmonkey
Copy link
Member

droidmonkey commented Dec 27, 2022

Ooooh possible that a generated password could cause it, especially if you use curly braces in the password.

@yan12125
Copy link
Contributor Author

I mentioned a tip earlier (#1741 (comment))

In Password Generator, there is a useful field "Do not include". I put curly brackets in that field {}, so future passwords will never trigger this warning

Maybe the password generator can be changed to avoid such passwords by default.

@droidmonkey
Copy link
Member

droidmonkey commented Dec 28, 2022

Nah, the better fix is to overhaul the references code. It is quite primitive and wasteful right now. It also doesn't need to even throw these warnings to the console unless in debug mode since they are meaningless.

@yan12125
Copy link
Contributor Author

yan12125 commented Jan 1, 2023

Ah, ignore my naive comment :)

Looking into reference/placeholder codes, this pull request might be an issue: #1651. It allows resolvePlaceholder to handle placeholders not at the begin or end, but unknown placeholders are also resolved recursively (and fail). As resolveMultiplePlaceholders can already resolve placeholders not at the begin or end, how about reverting that change and replace resolvePlaceholder usage with resolveMultiplePlaceholders?

@dkwo
Copy link

dkwo commented Aug 15, 2023

Same issue here: keepassxc-cli show -sa password passwords.kdbx skype/microsoft-account and a password containing curly braces produces Maximum depth of replacement has been reached. Entry uuid:[redacted] and then prints the password. This does not happen with simpler passwords. This luckily does not affect piping stdout to | qrencode -t utf8.

@jasonkhanlar
Copy link

jasonkhanlar commented Mar 6, 2024

i also see this flooding many dozens to hundreds of times (maybe more) in console, and searching for the relevent code I see two possible matches:

Briefly investigating to figure out how to reproduce, left clicking (default), or using up/down arrows to scroll through password entries (in the right main pane) shows variable 0, 1, or 3 lines in console per selection. I'll have to investigate more later if I have time to figure out what specifically in the entries is responsible for showing the console output, but for now I'm just mentioning here that I see this issue as well.

@yan12125 yan12125 changed the title "Maximum depth of replacement has been reached" from unknown placeholder "Maximum depth of replacement has been reached" from unknown placeholder (curly bracket pairs in passwords/notes/...) Mar 12, 2024
@yan12125
Copy link
Contributor Author

@jasonkhanlar I updated the issue title to make it clear about the cause.

c4rlo added a commit to c4rlo/keepassxc that referenced this issue Jun 2, 2024
When encountering a {brace-enclosed} substring, the placeholder
resolution logic would previously keep recursing until it hit the
recursion depth limit (currently 10). This would lead to "Maximum depth
of replacement has been reached" messages, and was also wasting CPU
cycles.

Fixes keepassxreboot#1741.
@droidmonkey droidmonkey modified the milestones: v2.8.0, v2.7.9 Jun 16, 2024
droidmonkey pushed a commit to c4rlo/keepassxc that referenced this issue Jun 16, 2024
When encountering a {brace-enclosed} substring, the placeholder
resolution logic would previously keep recursing until it hit the
recursion depth limit (currently 10). This would lead to "Maximum depth
of replacement has been reached" messages, and was also wasting CPU
cycles.

Fixes keepassxreboot#1741.
droidmonkey added a commit that referenced this issue Jun 16, 2024
* Entry placeholder resolution: don't overdo it

After resolving placeholders, previously the code would do it all over again if anything had changed, multiple times up to the recursion limit. This would have the effect of applying a much greater recursion limit, which is confusing and unnecessary, and probably undesired.

* Entry tweaks and minor refactoring

- Entry::size(): when computing tag size, use same delimiter set as in other places in the code
- Factor tag delimiter set regex out into global constant
- Placeholder resolution: remove unnecessary special casing for self-referential placeholders (these are taken care of by existing recursion depth limit)
- Placeholder resolution: less wasteful string building loop
- Move some constants from being public static data members of Entry to being local to Entry.cpp (in anonymous namespace)
- Migrate some QRegEx instances to QRegularExpression, the modern alternative
- Miscellanous minor code cleanups

* Entry: fix hitting recursion limit with {braces}

When encountering a {brace-enclosed} substring, the placeholder resolution logic would previously keep recursing until it hit the recursion depth limit (currently 10). This would lead to "Maximum depth of replacement has been reached" messages, and was also wasting CPU cycles.

Fixes #1741

---------

Co-authored-by: Jonathan White <[email protected]>
pull bot pushed a commit to tigerwill90/keepassxc that referenced this issue Jun 17, 2024
* Entry placeholder resolution: don't overdo it

After resolving placeholders, previously the code would do it all over again if anything had changed, multiple times up to the recursion limit. This would have the effect of applying a much greater recursion limit, which is confusing and unnecessary, and probably undesired.

* Entry tweaks and minor refactoring

- Entry::size(): when computing tag size, use same delimiter set as in other places in the code
- Factor tag delimiter set regex out into global constant
- Placeholder resolution: remove unnecessary special casing for self-referential placeholders (these are taken care of by existing recursion depth limit)
- Placeholder resolution: less wasteful string building loop
- Move some constants from being public static data members of Entry to being local to Entry.cpp (in anonymous namespace)
- Migrate some QRegEx instances to QRegularExpression, the modern alternative
- Miscellanous minor code cleanups

* Entry: fix hitting recursion limit with {braces}

When encountering a {brace-enclosed} substring, the placeholder resolution logic would previously keep recursing until it hit the recursion depth limit (currently 10). This would lead to "Maximum depth of replacement has been reached" messages, and was also wasting CPU cycles.

Fixes keepassxreboot#1741

---------

Co-authored-by: Jonathan White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.