fix(now-policy-api): allow '/' and ':' in PackageIdentifier - #91
Conversation
PackageIdentifier validation rejected '/' and ':' during deserialization, breaking scoped npm/Bun packages (@scope/package), npm aliases (alias:@scope/package@^7.20.0) and vcpkg triplets (curl:x64-windows) over the wire even though the package broker ecosystem advertises support for them. Relax the grammar to allow '/' and ':' while still rejecting genuinely dangerous or ambiguous input: control characters (now including NUL and DEL), backslash, double quote, '<', '>', '|', and the wildcard characters '*' and '?' (policy-side identifier matching is wildcard-based). The schemars regex, parse(), and the generated OpenAPI document are kept in sync, and wire-level (de)serialization tests cover the new grammar. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates package identifier validation to support npm/Bun scopes, aliases, and vcpkg syntax while strengthening control-character rejection.
Changes:
- Allows
/and:in package identifiers. - Adds validation and request-deserialization tests.
- Regenerates the OpenAPI schema.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
policies/rust/now-policy-api/src/lib.rs |
Updates validation and adds tests. |
policies/rust/now-policy-api/openapi/now-policy-api.yaml |
Synchronizes the generated schema. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…wlist Replace the character denylist with an explicit allowlist derived from the identifiers actually used by supported package managers: ASCII alphanumerics plus '. - _ + @ / : ^ ~ = [ ] ,'. This keeps scoped npm/Bun packages, npm aliases, vcpkg triplets/features, homebrew/scoop tap paths, and pip extras/pins working while rejecting whitespace, shell metacharacters, wildcards, control characters, and non-ASCII input. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Per review feedback, additionally allow version range operators and
wildcards for future-proofing: '<' and '>' (npm/pip range specifiers),
'|' ('||' alternation in npm ranges), and the wildcards '*' and '?'.
Whitespace, control characters, double quote, backslash, shell
metacharacters, and non-ASCII input remain rejected.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
policies/rust/now-policy-api/src/lib.rs:425
- This statement says shell metacharacters are rejected, but the allowlist immediately above accepts several shell metacharacters, including
<,>,|,*, and?. Narrow the wording to the unlisted metacharacters so the public API documentation does not overstate the validation guarantee.
/// Everything else — whitespace, control characters, `"`, `\`, shell
/// metacharacters, and non-ASCII — is rejected.
policies/rust/now-policy-api/openapi/now-policy-api.yaml:766
- This statement says shell metacharacters are rejected, although this schema accepts
<,>,|,*, and?, all of which can be shell metacharacters. Qualify the wording so generated API consumers are not given a stronger validation guarantee than the pattern provides.
Everything else — whitespace, control characters, `"`, `\`, shell metacharacters, and non-ASCII — is rejected.
policies/rust/now-policy-api/src/lib.rs:415
- The pip example is not a valid space-free specifier: removing the displayed space produces
>=7<8, but compound PEP 440 constraints require a comma. Using>=7,<8documents a form this allowlist actually accepts and pip can parse.
This issue also appears on line 424 of the same file.
/// - `^`, `~`, `=`, `<`, `>`, `|`: version pins/ranges in npm aliases and pip
/// specifiers (`>=7 <8` without the space, `||` alternation);
policies/rust/now-policy-api/openapi/now-policy-api.yaml:760
- The pip example is not a valid space-free specifier: removing the displayed space produces
>=7<8, but compound PEP 440 constraints require a comma. Use>=7,<8, which both matches this schema and is valid for pip.
This issue also appears on line 766 of the same file.
- `^`, `~`, `=`, `<`, `>`, `|`: version pins/ranges in npm aliases and pip specifiers (`>=7 <8` without the space, `||` alternation);
…entifier docs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Policy-side package identifier matching is wildcard-based, so '*' and '?' in request identifiers would be ambiguous; keep them rejected while retaining '< > |' for version range operators. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
policies/rust/now-policy-api/src/lib.rs:462
- This allowlist still rejects
!, even thoughPipis a supported manager and this API explicitly permits pip version specifiers.!=is a standard PEP 440 operator, so a valid request such asrequests!=2.32.0will still fail deserialization. Please add!consistently to the parser and schema, regenerate the OpenAPI artifact, and add a positive test (or narrow the contract so version specifiers are not accepted as identifiers).
if !s.bytes().all(|b| {
b.is_ascii_alphanumeric()
|| matches!(
b,
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
99347f8 to
9dc9270
Compare
…# $ % { }'
Per user feedback, the final allowlist is ASCII alphanumerics plus
'. - _ + @ / : [ ] , # $ % { }'.
Version range/pin operators ('< > = ! | ^ ~') are rejected: the broker
matches against a specific, exact version carried in the request's
separate Package.Version field, so range expressions do not belong in
the identifier. npm aliases must use exact versions
(e.g. 'alias:pkg@7.20.0'). Wildcards '* ?' remain rejected because
policy-side identifier matching is wildcard-based.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Note in the PackageIdentifier doc comment (and the generated schema description) that these allowlisted characters carry expansion semantics in some shells, so downstream command builders must pass identifiers as discrete process arguments rather than interpolating them into a shell command line. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
0bac944
into
master
Picks up the PackageIdentifier wire-validation rework from Devolutions/now-libraries#91 (explicit allowlist), which allows scoped npm ids, npm aliases with exact versions, and vcpkg triplets to deserialize correctly. Co-authored-by: Vladyslav Nikonov <mail@pacmancoder.xyz> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Problem
now-policy-api0.3.0 rejects/and:inPackageIdentifierduring deserialization, but the package broker ecosystem advertises support for identifiers that require them:@scope/packagebabel-core-legacy:@babel/core@7.20.0curl:x64-windows,curl[ssl]:x64-windowsDownstream unit tests (devolutions-agent) construct the newtype directly and bypass validation, so real HTTP requests with these identifiers were rejected during deserialization before command builders ever ran.
Change
PackageIdentifieris now validated against an explicit allowlist (rather than a denylist, which admitted a wide range of symbols and arbitrary non-control Unicode). The allowlist enumerates the characters used by identifiers of the supported package managers:.-_+— winget (Notepad++.Notepad++), chocolatey, pip, cargo, dotnet, apt/dnf/pacman (g++,libstdc++6), PowerShell modules@/— npm/Bun scopes (@scope/package), homebrew/scooptap/formulapaths, versioned formulas (python@3.11):— npm aliases (alias:@scope/package@1.0.0), vcpkg triplets (curl:x64-windows)[],— vcpkg features (curl[ssl,http2]:x64-windows), pip extras (requests[socks])#$%{}— additional identifier punctuation, included by product decision for forward compatibility. Caveat: these carry expansion semantics in some shells (${VAR},%VAR%, brace expansion), so downstream command builders must pass identifiers as discrete process arguments and never interpolate them into a shell command line (documented in the doc comment / schema description).< > = ! | ^ ~— the broker matches against a specific, exact version carried in the request's separatePackage.Versionfield, so range expressions do not belong in the identifier; npm aliases must use exact versions (alias:pkg@7.20.0)*?— policy-side package identifier matching is wildcard-based, so wildcards in request identifiers would be ambiguous",\, backtick,& ' ( ) ;, and non-ASCIIAn allowlist was chosen because the survey of supported managers shows this covers legitimate identifiers, while a denylist inevitably admits speculative symbols with no known use, leaving needless surface for injection into downstream command builders.
The grammar is aligned across
PackageIdentifier::parse(), the schemars regex, and the regenerated OpenAPI document (cargo run -p now-policy-server-template --bin generate-now-policy-api-openapi --locked).Tests
PackageRequestJSON payloads containing@scope/package,babel-core-legacy:@babel/core@7.20.0,curl:x64-windows,curl[ssl]:x64-windows.PackageIdentifieraccept/reject tests via bothparse()and serde: positives cover winget, npm/Bun, vcpkg, homebrew/scoop, pip, chocolatey, dotnet, cargo, apt-style ids, and# $ % { }; negatives cover range/pin operators (^ ~ = < > ! ||forms), wildcards* ?, whitespace,\ " & ' ( ) ; ````, CR/LF, TAB, NUL, DEL, non-ASCII, empty, and over-length input.cargo test -p now-policy-api --all-features,cargo fmt --check,cargo clippy --all-features --all-targets, andcargo xtask check typospass.Compatibility notes
/and:now accepted); however, symbols that 0.3.0 technically accepted but no supported manager needs (spaces,= ^ ~ & ' ( ) ; !-style punctuation outside the allowlist, non-ASCII, NUL/DEL) are now rejected. No known consumer relies on those.^/~/>=/||, PEP 440==/!=) are not accepted inside identifiers — target versions go in the separatePackage.Versionfield.fix:commit implies a 0.3.1 patch release.