Skip to content

Validate Pipfile markers against the PEP 508 grammar - #820

Merged
rowkav09 merged 1 commit into
mainfrom
fix-pipfile-marker-grammar
Oct 2, 2026
Merged

rowkav09 merged 1 commit into
mainfrom
fix-pipfile-marker-grammar

Conversation

@rowkav09

@rowkav09 rowkav09 commented Oct 2, 2026

Copy link
Copy Markdown
Member

Fixes #819.

The operator check from #818 only looked at the start of the value, so python_version = "< 3.11" and sys_platform = "== win32" (unquoted) and an invalid full markers string still produced marker text that does not parse. The combined marker is now checked against the PEP 508 marker grammar and dropped when it fails, as pipenv does. Valid markers, including the sys_platform and platform_machine shorthand, are unchanged. The new Pipfile regression fails before the change; Python adapter suite 249 pass, Prettier and ESLint clean.

@rowkav09

rowkav09 commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

Review at head 32aeb20 (main b6eb246 is in it). CHANGES, one blocker.

Blocker: isValidMarker is recursive and has no depth limit. A Pipfile marker with about 5000 nested parens (about 10 KB, well under MAX_PYPROJECT_BYTES) throws RangeError (stack overflow). It is not caught, so parseManifests rejects for the whole project (checked through parseManifests; 2000 levels is fine, 5000, 8000 and 20000 throw). #818 used regexes and did not have this. The manifest module says malformed files become evidence, never exceptions. Fix: cap depth (e.g. 64) and return false past it, or wrap the call in try/catch so the requirement stays unconditional. Add a test with a deeply nested marker.

Grammar probe: 99 marker strings through isValidMarker vs Python packaging 26.3, and vs the packaging 21.3 vendored in pipenv 2023.12.1. Covered nested parens, in / not in, single, double and mixed quotes, legacy dotted names (python_implementation, platform.version etc.), extra, whitespace, empty, unbalanced, and/or precedence. Only differences:

  • os_name not in 'nt' (several spaces inside not in): TS accepts, 21.3 rejects. Minor.
  • strings containing a newline ("os_name == 'nt'\n"): TS accepts, packaging 26.3 rejects, vendored 21.3 agrees with TS.

Valid input vs #818: py-only, os-only, two-key, sys_platform, platform_machine, full+shorthand, and OR cases give identical markers. Differences are only for invalid input, as intended (unquoted < 3.11, == win32 and garbage markers are now dropped; #818 kept them).

Long and hostile input otherwise fine: 100k-term and chain validates in about 160 ms.

Suite: 250/250 pass in packages/adapters/python. Fail-first: with main's pipfile.ts under the PR tests, "drops a Pipfile marker that does not parse, including unquoted shorthand values" fails.

CI at review time: not CI-green. CodeFactor, typecheck, lint, self-scan, image and the ubuntu consumer jobs passed; corpus, test (22/24) and windows consumer jobs were still running.

Same-account review.

The leading-operator check let unquoted shorthand values and invalid full markers strings through as invalid marker text. Parse the combined marker and leave the requirement unconditional when it does not parse, as pipenv does.
@rowkav09
rowkav09 force-pushed the fix-pipfile-marker-grammar branch from 32aeb20 to dfcbc19 Compare October 2, 2026 23:39
@rowkav09

rowkav09 commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

Re-review at head dfcbc19 (main b6eb246 is in it). PASS with notes.

Blocker fixed: depth cap 64. Probed through isValidMarker: 64 levels valid, 65 invalid, 20000 levels invalid with no throw. The new parsePipfileText test with 20000 levels passes, and the requirement stays unconditional.

Notes (minor, not blocking):

  • The CR/LF change only rejects newlines inside quoted strings ('a\nb' is now invalid). Newlines between tokens or trailing are still accepted by the tokenizer's \s*: "os_name == 'nt'\n", "os_name\n==\n'nt'" and "os_name == 'nt'\r\n" all return true. packaging 26.3 rejects them; pipenv's vendored 21.3 accepts them. Leaving it matches what pipenv does.
  • os_name not in 'nt' (several spaces) is accepted. packaging 26.3 also accepts it (I checked just now). The earlier mismatch was only against vendored 21.3, so keeping it accepted is fine.

Suite: 252/252 pass in packages/adapters/python.

CI at review time: not CI-green. CodeFactor passed; typecheck, lint, test 22/24, image, consumer jobs and self-scan were still running.

Same-account review.

@rowkav09
rowkav09 merged commit 73ce446 into main Oct 2, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pipfile markers: unquoted shorthand values and invalid markers strings still emit invalid marker text

1 participant