Store addons state as JSON by SaschaCowley · Pull Request #19564 · nvaccess/nvda

SaschaCowley · 2026-02-05T07:04:47Z

Link to issue number:

Closes #19559

Summary of the issue:

Addons state is saved in a pickle file.

Description of user facing changes:

None

Description of developer facing changes:

The add-ons state file is now in JSON.
The fromPickledDict method on AddonsState has been renamed to fromDict, but a shim has been introduced.

Description of development approach:

Switch to using lists instead of sets when populating AddonsState from dict and serializing to dict, as json.dump doesn't support serializing sets.

Added methods to read in a pickled add-ons state file and emit a good JSON representation of that add-ons state. My approach here was to discard invalid values rather than raising an error, so we have the greatest chance of success. This does mean that the conversion may be lossy.

When updating NVDA, convert from pickle to JSON for the system config's add-ons state file. This is done because this is essentially the only context in which we should write to the system config directory and which we know will be executed. User copies perform the migration on first run by migrating if and only if no addonsState.json exists, but addonsState.pickle does. User config migration is done by the installed copy as (a) this is the only time we can guarantee that the user's NVDA config wil be present; and (b) to minimise the amount of pickles we load while elevated. In both cases, a backup of the old state file is kept (as addonsState.pickle.bak).

When running on secure desktops, the pickled state will not be read, even if it is the only addonsState file in the system config. This should never happen, as the installer is responsible for migrating the system config's add-ons state file. In other cases, the fallback of reading the pickled state will attempt to save the JSON state and back up the pickled state, but this is not required in order for the load to succede. This is so when running in secure mode or with access to the user config restricted, add-ons continue to function properly. This does remove the security benefit of migrating away from pickle, but there's no other option other than ripping pickle support out entirely and breaking everyone's add-ons state entirely and having them manually migrate.

Testing strategy:

Unit and system tests.

Manual testing as follows:

Installed 2025.3.3
Installed add-ons (combination of 2026.1 compatible and incompatible, enabled and disabled) and restarted NVDA
Copied user config to system config
Upgraded using self-signed launcher
Ensured that the installer migrated the system addons state, and the installed copy migrated the user addons state on first run

Also "upgraded" my 2026.2 alpha copy with this launcher to ensure that the migration works as expected if updating within the 2026.x series.

Known issues with pull request:

None known (see notes above for some caviats about the approach though)

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

…ickle

seanbudd

Partial review

seanbudd · 2026-02-27T08:29:37Z

source/NVDAState.py

+	@property
+	def _oldAddonStateFile(self) -> str:
+		from addonHandler import _OLD_STATE_FILENAME
+
+		return os.path.join(self.configDir, _OLD_STATE_FILENAME)


Can we make sure there is some record (e.g. issue reference and comment) to remove this vulnerability in 2027.1.
We should drop reading pickles entirely in the next API breaking release.

seanbudd · 2026-02-27T08:31:26Z

source/addonHandler/__init__.py

+					log.error(f"Add-ons state file too large. {size=}B; {_MAX_STATE_FILESIZE_BYTES=}B")
+					return


should we warn the user somehow?

seanbudd · 2026-02-27T08:32:30Z

source/addonHandler/__init__.py

+# Decoding JSON can potentially consume a large amount of memory.
+# To avoid issues caused by this when reading add-on state,
+# limit the maximum size of the file.
+# The 2026.1 state JSON with no add-ons is 244 bytes.
+# As of 2026-02-24, there are 364 add-ons in the Add-on Store,
+# and the longest ID is 40 UTF-8 bytes (mean ~= median ~= 13 bytes).
+# Even if we take the empty state to be 300 bytes,
+# and assume that all add-on IDs are 100 characters long,
+# And those 100 characters are all 4 bytes wide,
+# and factor in the 4 extra bytes required for the enclosing quotes and ", " separator between IDs,
+# 1MiB is still enough space for a state file containing
+# (1024 * 1024) / (4 + 4 * 100) ~= 2594 add-ons.
+_MAX_STATE_FILESIZE_BYTES = 1024 * 1024  # 1MiB


we could stream this?
https://pypi.org/project/json-stream/

seanbudd · 2026-02-27T08:46:56Z

source/addonHandler/__init__.py

+
+
+def _getAddonsStateDictFromPickle(picklePath: os.PathLike) -> dict[str, list[str] | tuple[int, int, int]]:
+	import pickle


why was this import moved?
to isolate usage of pickle?

Copilot

Pull request overview

This PR migrates NVDA’s add-ons state persistence from an unsafe pickle-based format to JSON, including backward-compatible loading/migration paths and updated tests/documentation.

Changes:

Replace add-ons state serialization/deserialization with JSON (addonsState.json) and introduce pickle-to-JSON conversion helpers and fallback loading.
Add installer/system-config migration to convert addonsState.pickle to JSON and back up the old file.
Update tests and changelog entries for the new JSON format and deprecations.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
user_docs/en/changes.md	Documents deprecations related to add-ons state filename and `AddonsState` APIs.
tests/unit/test_addonHandler/test_addonsState.py	Expands unit tests to cover JSON load/save and pickle-to-JSON conversion behavior.
source/installer.py	Adds installer-time migration from pickle to JSON for system config.
source/config/init.py	Updates system config copy behavior to account for JSON state and migration scenarios.
source/addonHandler/init.py	Implements JSON-based add-ons state IO, fallback pickle loading, and conversion helpers.
source/NVDAState.py	Switches state filename to JSON and adds path for legacy pickle state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-27T09:01:19Z

source/installer.py

+			if os.path.exists(jsonPath):
+				tryRemoveFile(jsonPath)
+		except Exception:
+			log.error("Failed to remove existing {jsonPath}.", exc_info=True)


The log message uses "{jsonPath}" but isn't an f-string, so it will log the literal braces instead of the path. Make this an f-string (or pass jsonPath as a %s arg) so the error is actionable.

Suggested change

log.error("Failed to remove existing {jsonPath}.", exc_info=True)

log.error(f"Failed to remove existing {jsonPath}.", exc_info=True)

Copilot · 2026-02-27T09:01:19Z

source/installer.py

+	try:
+		jsonState = addonHandler._getAddonsStateDictFromPickle(pickledPath)
+	except Exception:
+		log.error("Failed to load pickled add-ons state.", exc_info=True)
+	else:
+		jsonPath = os.path.join(configPath, addonHandler.STATE_FILENAME)
+		try:
+			if os.path.exists(jsonPath):
+				tryRemoveFile(jsonPath)
+		except Exception:
+			log.error("Failed to remove existing {jsonPath}.", exc_info=True)
+		else:
+			try:
+				with open(jsonPath, "wt", encoding="utf-8") as file:
+					json.dump(jsonState, file)
+			except Exception:
+				log.error("Failed to dump JSON add-ons state.", exc_info=True)
+	finally:
+		try:
+			os.replace(pickledPath, pickledPath + ".bak")
+		except Exception:
+			log.error("Failed to back up pickled add-ons state.", exc_info=True)


Migration always renames the pickle to .bak in the finally block, even if loading the pickle failed or writing JSON failed. This can leave systemConfig with neither a readable pickle nor a JSON state file, effectively discarding add-on state. Only back up/rename the pickle after a successful JSON write (ideally write JSON to a temp file and atomically replace).

Copilot · 2026-02-27T09:01:20Z

source/addonHandler/__init__.py

+	if not isinstance(pickledState, dict):
+		log.debug("Invalid pickled state: {pickledState!r}")
+		return {}


This debug log message is missing an f-string, so it will print the literal "{pickledState!r}" instead of the value. Use an f-string so logs contain the actual invalid payload type/value.

Copilot · 2026-02-27T09:01:20Z

source/addonHandler/__init__.py

+				jsonState[key] = value
+			else:
+				log.debug(f"Invalid backCompatToAPIVersion: {value!r}. Discarding.")
+		elif key in AddonStateCategory:


elif key in AddonStateCategory will never match for the expected pickled keys because key is a string and EnumMeta.__contains__ only returns True for enum members. As a result, category entries like "disabledAddons"/"blocked" will be discarded during pickle->JSON conversion. Use a value-based check (e.g. try AddonStateCategory(key) in a try/except) before accepting the category.

Suggested change

elif key in AddonStateCategory:

elif any(key == category.value or key == category.name for category in AddonStateCategory):

Copilot · 2026-02-27T09:01:20Z

user_docs/en/changes.md

 * `textInfos.OffsetsTextInfo.allowMoveToOffsetPastEnd` is deprecated.
 Use the `OffsetsTextInfo.allowMoveToUnitOffsetPastEnd` method instead. (#19152, @LeonarddeR)
 * `installer.comparePreviousInstall` is deprecated with no planned replacement. (#19631)
+* Deprecations in `addonHandler: (#19564)


Markdown formatting: the inline code span for addonHandler is missing a closing backtick, which will break rendering for the rest of the section. Close the backtick so only addonHandler is formatted as code.

Suggested change

* Deprecations in `addonHandler: (#19564)

* Deprecations in `addonHandler`: (#19564)

Copilot · 2026-02-27T09:01:20Z

source/config/__init__.py

+			The system config will be in a ``systemConfig`` subdirectory of this directory.
+	:param addonsToCopy: List of add-on IDs to include in the system configuration, defaults to `()`.
+	:param isMigration: Whether this is a migration from one system config location to another, defaults to ``False``.
+		When this is ``True``, the ``addons/`` directory and ``addonsState.pickle`` in ``fromPath`` will be copied as-is, if they exist.


Docstring is now inaccurate: when isMigration is True it says only addonsState.pickle is copied as-is, but the new canonical state file is addonsState.json and may also need to be preserved during migrations. Update the docstring to reflect the current behavior/files.

Suggested change

When this is ``True``, the ``addons/`` directory and ``addonsState.pickle`` in ``fromPath`` will be copied as-is, if they exist.

When this is ``True``, the ``addons/`` directory and add-on state files (for example, ``addonsState.json`` and legacy ``addonsState.pickle``) in ``fromPath`` will be copied as-is, if they exist.

Copilot · 2026-02-27T09:01:21Z

source/config/__init__.py

+					addonHandler.STATE_FILENAME.casefold(),
+					addonHandler._OLD_STATE_FILENAME.casefold(),
+				)
+				files = [filename for filename in files if filename not in stateFilenames]


State file exclusion is casefolding the constants but not the filenames. Since files contains the original casing, this filter can fail and accidentally copy addonsState.json/pickle. Compare using filename.casefold() (or avoid casefolding constants) so the exclusion works reliably.

Suggested change

files = [filename for filename in files if filename not in stateFilenames]

files = [filename for filename in files if filename.casefold() not in stateFilenames]

SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Feb 10, 2026

seanbudd added this to the 2026.1 milestone Feb 16, 2026

SaschaCowley force-pushed the addonsStateJson branch from c457a39 to ed287b4 Compare February 18, 2026 00:13

seanbudd assigned SaschaCowley Feb 19, 2026

SaschaCowley and others added 14 commits February 25, 2026 14:39

Update tests

318ee7b

Fix erroring tests

8a687e6

Fix deserialisation tests

3e21878

Fix up remaining failing tests

6515870

Add migration code and tests (not yet plumbed in)

b1aa821

Slight restructure of helpers

0823742

Load from json

72fa492

Fix installer to copy systemConfig as-is

be3182d

WIP

e04d774

WIP

e729524

Fix issue with defaults not being set

5a1130d

Add __init__ so that we don't have to manually run initialization code

428e7d2

Trim a little dead code

eddb80b

Logic improvements

dd51976

SaschaCowley force-pushed the addonsStateJson branch from 1969677 to dd51976 Compare February 25, 2026 03:41

SaschaCowley added 8 commits February 25, 2026 15:22

Code cleanup

a15dd57

Add more unit tests

e162acc

More tidy

9b86728

Put in installer

19a4b7c

Back compat

92a9bbf

Added changes for developers

7bdb49a

Fix user config backup filename and erroneously copying addonsState.p…

0a6bc82

…ickle

Fix logging

7133257

SaschaCowley marked this pull request as ready for review February 27, 2026 08:20

SaschaCowley requested a review from a team as a code owner February 27, 2026 08:20

SaschaCowley requested a review from seanbudd February 27, 2026 08:20

seanbudd reviewed Feb 27, 2026

View reviewed changes

seanbudd requested a review from Copilot February 27, 2026 08:48

Copilot started reviewing on behalf of seanbudd February 27, 2026 08:48 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

		log.error(f"Add-ons state file too large. {size=}B; {_MAX_STATE_FILESIZE_BYTES=}B")
		return



		def _getAddonsStateDictFromPickle(picklePath: os.PathLike) -> dict[str, list[str] \| tuple[int, int, int]]:
		import pickle

	log.error("Failed to remove existing {jsonPath}.", exc_info=True)
	log.error(f"Failed to remove existing {jsonPath}.", exc_info=True)

	elif key in AddonStateCategory:
	elif any(key == category.value or key == category.name for category in AddonStateCategory):

	* Deprecations in `addonHandler: (#19564)
	* Deprecations in `addonHandler`: (#19564)

	When this is ``True``, the ``addons/`` directory and ``addonsState.pickle`` in ``fromPath`` will be copied as-is, if they exist.
	When this is ``True``, the ``addons/`` directory and add-on state files (for example, ``addonsState.json`` and legacy ``addonsState.pickle``) in ``fromPath`` will be copied as-is, if they exist.

	files = [filename for filename in files if filename not in stateFilenames]
	files = [filename for filename in files if filename.casefold() not in stateFilenames]

Uh oh!

Conversation

SaschaCowley commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes:

Description of developer facing changes:

Description of development approach:

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

seanbudd Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SaschaCowley commented Feb 5, 2026 •

edited

Loading