GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-65697: Prevent configparser from writing keys it cannot properly read by lincolnj1 · Pull Request #129270 · python/cpython

Conversation

@lincolnj1

@lincolnj1 lincolnj1 commented Jan 24, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Prevents configparser from writing keys containing delimiters and keys beginning with the section header pattern to a file.

Both of those scenarios create .ini files the parser cannot accurately read back. Keys beginning with the section header pattern parse back as new sections. Keys containing delimiters parse back only the first portion as a key and the remainder as a value (i.e. key: 'one=two', value:'three' writes as 'one=two=three' and parses back as key: 'one', value: 'two=three').

Unsure on whether to handle newlines in key/section/values by rejecting them when writing, rejecting them when set, or escaping them when writing.

  • Issue: configparser accepts invalid keys and sections when writing #65697

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
marc-h38 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
lincolnj1 and others added 10 commits January 16, 2025 08:54
@lincolnj1
Added check for delimiters in cfgparser keys to _validate_value_types
454f026
@lincolnj1
Deleted some trailing whitespace
20b24b1
@lincolnj1
Merge branch 'python:main' into fix-issue-128843
5b730b6
@lincolnj1
Added check for section pattern in key and moved
9cc3506 
invalid key checks to seperate function
@lincolnj1
Clarified _validate_key_contents() doc comment
3638e67
@lincolnj1
Merge branch 'python:main' into fix-issue-128843
b161cea
@lincolnj1
Merge branch 'python:main' into fix-issue-128843
f73d17a
@lincolnj1
Clarified new error name/doc comment and improved
5a47f44 
validating check readability
@lincolnj1
Clarified InvalidWriteError doc comment
e12b696
@lincolnj1
Merge branch 'fix-issue-128843' of https://github.com/lincolnj1/cpython…
f5f1cbb 
… into fix-issue-128843
@lincolnj1
lincolnj1 requested a review from jaraco as a code owner January 24, 2025 21:49
@ghost

ghost commented Jan 24, 2025 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting review label Jan 24, 2025
@bedevere-app

bedevere-app Bot commented Jan 24, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot mentioned this pull request Jan 24, 2025
configparser accepts invalid keys and sections when writing #65697
Open
@lincolnj1
Merge branch 'main' into validate-config-writes
3d84347
@bedevere-app

bedevere-app Bot commented Jan 24, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1 lincolnj1 changed the title gh-65697 gh-65697: Prevent configparser from writing keys it cannot properly read Jan 24, 2025
@lincolnj1
lincolnj1 marked this pull request as draft January 24, 2025 22:11
@bedevere-app bedevere-app Bot removed the awaiting review label Jan 24, 2025
jaraco
jaraco requested changes Jan 26, 2025
View reviewed changes

@jaraco jaraco left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks sound and the tests seem to capture the essence of the change. I like it.

The only thing I'd like to see before accepting this - is there any lingering controversy? Who might oppose this change and can we get their consent?

If we can get consent from one or two of the most vocal detractors in the linked issues, then I'd say we could proceed. Otherwise, I'd like to hear their argument for what they recommend instead. Can you help gather that consensus?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app

bedevere-app Bot commented Jan 26, 2025

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting changes label Jan 26, 2025
@lincolnj1

lincolnj1 commented Jan 27, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing, Jason!

The only thing I'd like to see before accepting this - is there any lingering controversy? Who might oppose this change and can we get their consent?

Newlines, \r, and \x00 are still unhandled and it seemed like the consensus was that they should be. I'm working on a fix atm but what I've tried (checking for \n,\r,\x00 in everything when writing) breaks older test cases. I may need to make a second pull request later to fix that.

@terryjreedy Do you have any thoughts on this change? You were pretty active in previous threads on this topic.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1
Remove trailing whitespace
97c8e93
@bedevere-app

bedevere-app Bot commented Jan 27, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1

lincolnj1 commented Jan 31, 2025

Copy link
Copy Markdown
Contributor Author

@bitdancer @vstinner Do you have any thoughts on this change? You both commented in previous threads on the topic.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Jan 31, 2025

Copy link
Copy Markdown
Member

cc @ambv
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jaraco

jaraco commented Feb 2, 2025

Copy link
Copy Markdown
Member

Let's give this until Mon, Feb 24 for comment. If there's no objection raised by then, we'll plan to proceed. If anyone needs more time, that's can be done too.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1
Adding documentation for InvalidWriteError
ea137d0
@bedevere-app

bedevere-app Bot commented Feb 15, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lincolnj1 added 2 commits February 14, 2025 18:17
@lincolnj1
Remove trailing whitespace
7ca33c7
@lincolnj1
Cleaned up documentation on InvalidWriteError
353eaf1
@bedevere-app

bedevere-app Bot commented Feb 21, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1
Merge branch 'main' into validate-config-writes
ccf715e
@bedevere-app

bedevere-app Bot commented Feb 21, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1
Delint of test-configparser.py
a3e8848
@bedevere-app

bedevere-app Bot commented Feb 21, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lincolnj1
Removing trailing whitespace on line 2184 of test_configparser
0e278fd
@bedevere-app

bedevere-app Bot commented Feb 21, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
blurb-it Bot and others added 3 commits February 21, 2025 20:22
@blurb-it
📜🤖 Added by blurb_it.
3da65ae
@lincolnj1
Changed formatting of configparser.py __all__
aa9c92a
@lincolnj1
Merge branch 'validate-config-writes' of github.com:lincolnj1/cpython…
b001df6 
… into validate-config-writes
@lincolnj1
lincolnj1 marked this pull request as ready for review February 21, 2025 21:44
@bedevere-app bedevere-app Bot added awaiting review and removed awaiting changes labels Feb 21, 2025
jaraco
jaraco approved these changes Feb 23, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Feb 23, 2025
@jaraco
jaraco merged commit 25a7ddf into python:main Feb 23, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 23, 2025
@hroncok hroncok mentioned this pull request Jun 11, 2025
osbuild failures with Python 3.14: configparser.InvalidWriteError: Cannot write key that contains delimiters osbuild/osbuild#2109
Closed
@marc-hb marc-hb mentioned this pull request Jun 11, 2025
west config corner cases when setting option foo.bar=baz zephyrproject-rtos/west#779
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaraco jaraco jaraco approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@lincolnj1 @vstinner @jaraco
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.