Skip to content

PyNUTClient: decode NUT values and escape authentication arguments - #3629

Open
user01010111 wants to merge 4 commits into
networkupstools:masterfrom
user01010111:codex/issue-3620
Open

PyNUTClient: decode NUT values and escape authentication arguments#3629
user01010111 wants to merge 4 commits into
networkupstools:masterfrom
user01010111:codex/issue-3620

Conversation

@user01010111

@user01010111 user01010111 commented Sep 7, 2026

Copy link
Copy Markdown

Descriptions containing escaped quotes can make GetUPSList() fail, while
other Python readers truncate quoted values or retain wire escapes. Decode
NUT tokens once in a shared private parser, preserving existing byte/string
return types and the command-description error fallback. Use the same
grammar for nutauth.conf values and INCLUDE filenames, and escape literal
usernames and passwords before sending them. Double quotes group tokens;
single quotes remain literal characters.

Encode upsd variable and command descriptions with the existing
pconf_encode() helper before returning DESC and CMDDESC. Keep the existing
response-size limit. Add deterministic tests and a localhost integration
case to the existing test infrastructure. Update NEWS in its client-library
and data-server sections, UPGRADING, the authentication manual and Python
README; add the documented API names to the spelling dictionary.

Closes: #3620

This implements the agreed Python/server scope from the discussion in this
PR. The C quoted-hash correction (#3633), Perl parsing (#3634)
and NDE configuration fixes (#3635) are separate contributions. Command-name formatting reuses the existing
#3628 change; its ownership remains with that PR.

Validation:

  • The new 13-test protocol/configuration suite produces six failures and
    one error on the pre-feedback module. Existing ordinary controls pass.
    The patched module passes all 35 tests: 16 UPS-list tests, 13 shared
    protocol/configuration tests and six existing command tests.
  • Linux ARM64: Python 2.6.9, 2.7.18, 3.4.10, 3.5.10, 3.6.15, 3.7.17,
    3.8.20, 3.9.25, 3.10.21, 3.11.16, 3.12.14, 3.13.15 and 3.14.7 pass.
    Native macOS ARM64 tests pass on Python 3.9.6 and 3.14.7.
  • The Python tokenizer matches the corrected C parser token-for-token on
    352 valid ASCII cases, including 3,304 split-boundary checks and text
    input checks. Python deliberately preserves bytes where C configuration
    parsing filters high bytes; no new Unicode conversion policy is added.
  • Locally built upsd and dummy-ups pass the Python integration group on
    Linux and macOS: eight passed, zero failed or skipped on each. Existing
    TLS checks and the new plaintext punctuation fixture cover decoded
    values/descriptions, INCLUDE credentials, ordinary controls, successful
    secondary LOGIN and incorrect-password rejection. No power commands are
    issued by the new fixture.
  • Configured wheel, source distribution and staged module each pass all
    35 tests on Linux Python 3.11.15; installed module bytes match the
    configured source. The direct CI packaging recipe also builds successfully.
  • Native build and eight Automake tests pass. Linux build, 12 Automake
    tests, documentation, stylecheck, spellcheck and distcheck-light pass.
    The final distribution check uses real manual pages and exercises the
    archive's build/test/install/uninstall/cleanup lifecycle. Changed source
    files are verified against that archive.

No physical UPS hardware was available for testing. Validation covered
deterministic protocol/configuration tests, corrected-C parser comparisons,
package/install tests and localhost upsd with simulated dummy-ups, including
the existing TLS integration checks.
Physical UPS models, firmware versions, and hardware combinations were
not tested.

Windows and other operating systems were not executed. Python 2.6 uses
the plain socket path. Simulated tests do not establish physical-device
compatibility. The existing server response-size ceiling and configuration
include lookup/precedence remain unchanged. Users should remove manual
unescaping workarounds and replace single quotes previously used to group
configuration values with double quotes, as described in UPGRADING.

Relevant checklist:

  • Concrete behavior, compatibility expectations and limitations described.
  • Focused changes; maintained templates and existing test infrastructure used.
  • New source and documentation text uses ASCII.
  • NEWS placement, UPGRADING, authentication manual and Python README reviewed and updated.
  • Build, tests, package/install, spelling/style and distcheck-light pass.
  • AI use and models disclosed.
  • Human review of this revised patch and acceptance of responsibility completed.
  • Human DCO authorization supplied for this revised contribution.

AI assistance: OpenAI Codex with gpt-6-astra (x-high and high reasoning)
and gpt-daybreak-blue-latest (high reasoning).
The human contributor remains responsible for reviewing and submitting
the change.

Parse the quoted LIST UPS description without treating escaped quotes as
field delimiters. Decode NUT escapes once while keeping dictionary keys
and values as bytes and preserving GetUPSNames() conversion of names.

Add deterministic regression coverage to make check, including escaped
byte combinations, fragmented reads, consumers and existing errors.
Reject malformed quoted descriptions with ValueError rather than silently
truncating a description whose closing quote is missing.

Closes: networkupstools#3620

AI assistance: OpenAI Codex with gpt-6-astra (x-high reasoning).
The human contributor remains responsible for reviewing and submitting
the change.

Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

A ZIP file with standard source tarball and another tarball with pre-built docs for commit af77624 is temporarily available: NUT-tarballs-PR-3629.zip.

@jimklimov jimklimov added bug python Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) labels Sep 8, 2026
@jimklimov jimklimov added this to the 2.8.6 milestone Sep 8, 2026
@jimklimov

Copy link
Copy Markdown
Member

Thanks! I think this parser code makes sense as a separate helper method, which should be applied to all strings returned by upsd or read from NUT configuration files which are expected to follow similar escaping semantics (e.g. nutauth.conf).

@jimklimov jimklimov added the AI For good or bad, machine tools are upon us. Humans are still the responsible ones. label Sep 8, 2026
@jimklimov jimklimov added this to NUT QA Sep 8, 2026
@jimklimov jimklimov moved this to In Progress in NUT QA Sep 8, 2026
@user01010111

Copy link
Copy Markdown
Author

Thanks! I think this parser code makes sense as a separate helper method, which should be applied to all strings returned by upsd or read from NUT configuration files which are expected to follow similar escaping semantics (e.g. nutauth.conf).

Thanks, Jim. Happy to extract a shared helper and reuse it across the relevant Python readers. Before expanding this PR, could you clarify a few scope points?

  • Would you prefer all affected PyNUT response readers and nutauth.conf parsing addressed in this PR, or configuration parsing as a follow-up? I’m assuming other language bindings are outside this change.
  • Should Python’s configuration parsing match C parseconf exactly? Python currently strips single quotes, while C treats them literally. The manual’s wording also suggests # inside double quotes is permitted, whereas C requires it to be escaped.
  • If end-to-end validation exposes related outbound credential or server-response escaping problems, would you prefer those fixed here or in separate PRs?

To clarify, I’d preserve the existing return types and keep the changes focused on NUT’s escaping semantics.

@jimklimov

Copy link
Copy Markdown
Member

Fixes for Python (and C if there is indeed some inconsistency) may well be in this PR. Regarding C, I think there was a bug logged about # maybe, and about different outputs of driver internal dumps vs upsc #1306 certainly - maybe that is related to this same pain point.

I think PERL binding may suffer similarly, but can be addressed separately based on lessons learned here.
Wondering now if e.g. shell parsing in NDE is impacted (if it at all touches on values that might be so escaped)?..

@user01010111

Copy link
Copy Markdown
Author

Thanks, Jim. I traced the related paths and checked them with deterministic parser probes, NDE self-test mode, and a localhost upsd/dummy-ups setup. The findings are:

  • Several Python response readers truncate escaped descriptions or values. GetUPSCommands() also formats command names as b'...' under Python 3, so fixing escape parsing alone would leave that path broken.
  • Python's nutauth.conf reader mishandles escapes and trailing comments. INCLUDE filenames need escape handling too. Decoded credentials also need correct outbound encoding to reach the server unchanged.
  • upsd does not re-escape descriptions loaded from cmdvartab when sending DESC and CMDDESC. The localhost test confirmed malformed responses and incorrectly parsed descriptions.
  • Perl retains escape characters in returned values, confirming the separate follow-up you suggested.
  • NDE is affected: a trailing comment on port = "auto" prevents the expected USB classification, and backslash handling can corrupt paths. This is in its ups.conf parsing; it does not consume upsd descriptions.
  • Clarify documentation on conf file parser with respect to quotes and comments #607 appears to be the earlier hash-escaping issue. The nutauth.conf manual's wording about quoted hashes differs from current C behavior, so the documentation should reflect whichever rules we agree on.

I propose keeping the shared Python parsing helper and affected readers, configuration/include parsing, necessary outbound encoding, the Python 3 command-name correction, and the two C response-encoding fixes in this PR, with the relevant tests and documentation updates. Existing Python return types would be preserved. Perl and NDE would follow separately. Does that scope split work for you?

Two compatibility points before implementing:

  1. For Python's nutauth.conf reader, are you happy to retain its existing acceptance of single-quoted values and literal hashes inside quotes while correcting escapes and comments, rather than tightening those forms to current C syntax?
  2. Would you support a separate, focused follow-up allowing literal # inside double quotes in the shared C parser, while retaining \# support and escaped output for older consumers? That would address the surprising quoted-hash behavior you noted in Driver "discovery/dumping" mode escapes some chars that upsc client does not #1306. I would leave upsc's default output unchanged and keep any optional export mode outside this PR.

No physical UPS hardware was available for testing. These checks covered software behavior and simulated/local integration; physical UPS models, firmware versions, and hardware combinations were not tested.

@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5253-master completed (commit 70271d4362 by @user01010111)

@jimklimov

Copy link
Copy Markdown
Member

Yes, splitting the PRs makes sense.

The C parser breaking for a hash inside quotes (the "Outlet #3" example from #1306) is a bug. IIRC that is same code for configs and for values. An escaped character, whether inside or outside quotes, should be handled properly, so "Outlet \#2" and Outlet \#4 should both be internally treated as a string which has a space-hash-number sequence, no backslashes added.

  • It is a separate matter that upsc historically reports values for easier scripted consumption, so the KEY: VALUE lines are not quoted and may include multiple tokens in the VALUE part.
  • The recently added upsc JSON output mode should handle string escape sequences properly.
  • I think the configuration parser insists on use of quotes and other escaping for multi-word single tokens, such as descriptions (maybe it accepts unquoted lines with an escaped space as well?) and that is a correct predictable way to go about it for inputs.
  • For Python/PERL, follow the syntax used by C. Single quotes may be an overlooked artifact of AI generation of the larger-stroke picture for having something working as the first shot, doesn't make it a non-bug still :)

Decode LIST CMD identifiers as ASCII for GET CMDDESC requests and response
offsets, preserving the public bytes dictionary and description fallback.
Add request-sensitive regression coverage to make check and note the fix
in NEWS.adoc.

Validated with Python 2.6, 2.7 and 3.4 through 3.14, localhost upsd with
dummy-ups, generated/installed/packaged modules, make check, spellcheck
and distcheck-light.

AI assistance: OpenAI Codex with gpt-6-astra (x-high reasoning).
The human contributor remains responsible for reviewing and submitting
the change.

Closes: networkupstools#3621

Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
Share NUT token decoding across Python response and configuration readers,
including INCLUDE paths, and encode literal authentication arguments.
Escape server DESC and CMDDESC responses with pconf_encode. Preserve return
types and existing response limits; document configuration compatibility.

Add deterministic and localhost integration coverage to existing tests.
Validated across Python 2.6, 2.7 and 3.4-3.14, package/install paths, real
localhost upsd/dummy-ups, documentation checks and distcheck-light.

Closes: networkupstools#3620

AI assistance: OpenAI Codex with gpt-6-astra (x-high and high reasoning)
and gpt-daybreak-blue-latest (high reasoning).
The human contributor remains responsible for reviewing and submitting
the change.

Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
@user01010111 user01010111 changed the title PyNUTClient: decode escaped UPS descriptions in GetUPSList() PyNUTClient: decode NUT values and escape authentication arguments Sep 8, 2026
@AppVeyorBot

Copy link
Copy Markdown

@AppVeyorBot

Copy link
Copy Markdown

Use portable real filenames and escape complete paths in INCLUDE fixtures.
Check quote, hash and backslash decoding separately through the AuthConf
reader without creating a Windows-invalid filename. Preserve production
behavior and existing escape coverage.

Validated with all 36 tests across the documented macOS/Linux Python matrix
and Windows CPython 3.13.7 under Wine, including a separate shared-test run
with spaces and a hash in the temporary parent path. Build, make check,
spelling/style and distcheck-light pass. Wine does not establish exact
AppVeyor-image or Windows-kernel acceptance.

Related: networkupstools#3629

AI assistance: OpenAI Codex with gpt-6-astra.
The human contributor remains responsible for reviewing and submitting
the change.

Signed-off-by: user01010111 <12504630+user01010111@users.noreply.github.com>
@AppVeyorBot

Copy link
Copy Markdown

@AppVeyorBot

Copy link
Copy Markdown

@jimklimov

Copy link
Copy Markdown
Member

In Appveyor build, this still complains:

================================
Wed Sep  9 02:24:04 UTC 2026 [INFO] [testcase_sandbox_python_escaping] Check escaped data and credentials with PyNUT
Traceback (most recent call last):
  File "<stdin>", line 19, in <module>
AssertionError: Description unavailable
Wed Sep  9 02:24:04 UTC 2026 [ERROR] [testcase_sandbox_python_escaping] Error: PyNUT regression check failed
================================
Wed Sep  9 02:24:05 UTC 2026 [INFO] Stopping test daemons
Read error : 109
================================
Wed Sep  9 02:24:05 UTC 2026 [INFO] OVERALL: PASSED=26 FAILED=1 SKIPPED=0
      1 [F] testcase_sandbox_python_escaping
Wed Sep  9 02:24:05 UTC 2026 [FATAL] Error: Some test scenarios failed!
make[3]: *** [Makefile:782: check-NIT] Error 1
make[2]: *** [Makefile:1790: check-recursive] Error 1
make[1]: *** [Makefile:2216: check] Error 2
make: *** [Makefile:1163: check-recursive] Error 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI For good or bad, machine tools are upon us. Humans are still the responsible ones. bug Incorrect or missing readings On some devices driver-reported values are systemically off (e.g. x10, x0.1, const+Value, etc.) python

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

PyNUT GetUPSList() rejects descriptions containing escaped quotes

3 participants