legal: complete third-party notices and optional-agent policy - #43
legal: complete third-party notices and optional-agent policy#43eXPerience83 wants to merge 187 commits into
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThe change adds a maintained third-party inventory, source locks, runtime notice collection, validation tools, optional-agent policy documentation, scoped image metadata, SBOM reconciliation, and release verification requirements. ChangesThird-party notices and licensing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildWorkflow
participant BaseDockerfile
participant CopyRuntimeNotices
participant RemoteDevNotices
participant SBOMReconciler
BuildWorkflow->>BaseDockerfile: build candidate images
BaseDockerfile->>CopyRuntimeNotices: stage runtime and component notices
CopyRuntimeNotices-->>BaseDockerfile: populate canonical notice root
BaseDockerfile->>RemoteDevNotices: run --check
RemoteDevNotices-->>BaseDockerfile: validate notices and manifests
BuildWorkflow->>SBOMReconciler: reconcile base and final SPDX SBOMs
SBOMReconciler-->>BuildWorkflow: report inventory coverage
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc8f416042
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b07ed402d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| install = normalized.index("install", 1) | ||
| except ValueError: | ||
| return None |
There was a problem hiding this comment.
When a Dockerfile uses RUN apt-get download bash or RUN apt-get source bash, this search returns None, and neither the download nor installer scanner claims the command, so an archive can be retained or unpacked into the image without inventory ownership. Checked apt-get --help, which describes download as “Download the binary package into the current directory” and source as “Download source archives”; classify these retrieval commands or reject unsupported APT subcommands.
Useful? React with 👍 / 👎.
| if executable == "node" and any(option in tokens for option in {"-e", "--eval"}): | ||
| option = "-e" if "-e" in tokens else "--eval" | ||
| code = tokens[tokens.index(option) + 1] if tokens.index(option) + 1 < len(tokens) else "" | ||
| if NODE_NETWORK_RE.search(code): | ||
| return "Node.js inline network acquisition" |
There was a problem hiding this comment.
When a context-backed shell helper runs node -p 'fetch("https://vendor.example/tool")', this separate helper scanner accepts it because it recognizes only -e and --eval. Fresh evidence beyond the previously fixed Dockerfile scanner is that _acquisition_reason() still omits both -p and --print; checked node --help, which defines them as evaluating a script and printing the result, so these modes can still download unowned content during a build.
Useful? React with 👍 / 👎.
| "requests.get", | ||
| "requests.post", | ||
| "requests.put", | ||
| "requests.request", |
There was a problem hiding this comment.
When an invoked Python build helper uses requests.Session().get(...) or assigns the session before calling get, call_name() resolves the method target to just get, which is absent from this literal set, and validation accepts the helper. The returned content can then be written into the image without download ownership checks; track client construction and assignments or conservatively reject networking methods reached through imported client libraries.
Useful? React with 👍 / 👎.
|
@codex review |
|
@coderabbitai resume |
|
@coderabbitai review |
✅ Action performedReviews resumed. |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc451f4a02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mapping: dict[str, Path] = {} | ||
| for instruction in instructions: | ||
| payload = instruction_payload(instruction, "COPY") | ||
| if payload is None or payload.startswith("[") or "--from" in payload: |
There was a problem hiding this comment.
When an extensionless helper is copied with COPY ["scripts/fetch", "/usr/local/bin/fetch"] and invoked by RUN /usr/local/bin/fetch, this branch discards the entire COPY mapping; because the unresolved destination has no .sh/.py suffix, _invoked_scripts() also accepts it, so a curl inside the executed helper bypasses acquisition validation. Fresh evidence beyond the previously fixed extensionless-helper case is that the exact reviewed tree still accepts this JSON-form reproduction; parse JSON-form COPY sources and destinations before resolving helpers.
Useful? React with 👍 / 👎.
| if any(token in {"--help", "-h", "--version", "-V", "help", "version"} for token in tokens[1:]): | ||
| return False |
There was a problem hiding this comment.
When a Dockerfile uses cargo install --version 14.1.1 ripgrep, this unconditional query check returns false before the Cargo branch, so the package acquisition receives no inventory owner. Fresh evidence beyond the earlier verbose-flag fix is that the exact scanner returns no instruction for this valid command; Cargo documents --version <VERSION> as specifying the version to install in the official cargo install reference, so only recognize version-query flags according to each command's position and semantics.
Useful? React with 👍 / 👎.
| if resolved_command not in NPM_INSTALL_COMMANDS: | ||
| return None |
There was a problem hiding this comment.
When a Dockerfile runs npm exec --package=cowsay@1.6.0 -- cowsay hi, the resolved exec command is ignored here, and neither installer nor download discovery claims it, allowing the remotely fetched package and any build side effects to pass without an inventory owner. The official npm exec reference describes it as running a command from a local or remote npm package and installing specified packages into the npm cache; classify exec/npx acquisitions or reject them fail-closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48d8f22b4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return executable == "git" and ( | ||
| _git_subcommand(tokens, path) in {"clone", "fetch", "pull"} | ||
| or ("lfs" in tokens[1:] and any(x in tokens[1:] for x in {"fetch", "pull"})) |
There was a problem hiding this comment.
When a Dockerfile runs git archive --remote=https://vendor.example/repository HEAD | tar -x ..., this predicate ignores the command and docker_download_urls() returns no URLs, allowing the extracted remote content to bypass inventory ownership. The checked git archive -h explicitly describes --remote <repo> as retrieving an archive from a remote repository; classify this operation as an acquisition or reject unsupported URL-bearing Git subcommands.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/legal_inventory/build_scripts.py (1)
88-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClustered short options like
-lc/-ucstill get treated as unresolved script candidates.The break condition at line 100 correctly detects a cluster containing
corm(for example-lc,-uc). But the check at line 103 (tokens[index] not in {"-c", "-m"}) only excludes the exact tokens"-c"and"-m". A cluster such as"-lc"or"-uc"is not equal to either literal string, so it is still appended tocandidates._resolve_script("-lc", ...)then returnsNone, and sinceexecutableis in{"bash", "sh", "dash", "python", "python3"}, line 111-115 raisesInventoryError("build invokes a script whose context source cannot be resolved...").This means
RUN bash -lc "…"orRUN python3 -uc "…"in a Dockerfile now fails discovery with a misleading "unresolved helper" error, even though a prior review explicitly requested that such clusters be treated as terminal (inline code) without raising this error.Use the same cluster test as the break condition instead of exact-string membership.
🐛 Proposed fix
- if index < len(tokens) and tokens[index] not in {"-c", "-m"}: - candidates.append(tokens[index]) + if index < len(tokens): + is_terminal_cluster = ( + short_option_value(tokens[index], "c")[0] or short_option_value(tokens[index], "m")[0] + ) + if not is_terminal_cluster: + candidates.append(tokens[index])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/legal_inventory/build_scripts.py` around lines 88 - 104, Update the candidate guard in the token-processing loop around short_option_value so it uses the same cluster-aware detection for c or m as the preceding break condition, excluding tokens such as -lc and -uc from candidates while preserving normal script-token handling.scripts/legal_inventory/installer_scan.py (1)
67-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRecognize bundled Python -m pip invocations.
python3 -mpip install pkgis valid;tokens.index("-m", 1)returnsNonefor this form, so_is_installerreports no installer instead of classification aspython -m pip install. Use the existingshort_option_value(token, "m")helper here, handling attached modules and the next token case, then classifypip install/pip download.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/legal_inventory/installer_scan.py` around lines 67 - 77, The Python pip detection in _is_installer must support both attached and separate -m arguments. Replace the direct tokens.index("-m", 1) lookup with short_option_value(token, "m"), handling an attached “pip” module and the next-token module form, then continue through _skip_options and classify “install” or “download” as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/legal_inventory/build_scripts.py`:
- Around line 334-341: Replace the regex-based substitution extraction in the
build-helper checks with docker_parse._command_substitutions, then run
_command_segments and _acquisition_reason on each parsed substitution as before.
Preserve the existing InventoryError message and Dockerfile ownership guidance
while ensuring nested and literal-parenthesis substitutions are detected.
- Around line 265-292: Update _heredoc_acquisition_reason to track whether the
matched heredoc uses <<- versus plain <<, and recognize terminators accordingly:
plain << must match the delimiter exactly, while <<- may remove only leading
tabs before comparison. Do not use strip(), so indentation spaces remain part of
heredoc content and acquisition code after an incorrectly indented terminator is
still analyzed.
In `@scripts/legal_inventory/docker_parse.py`:
- Around line 18-25: The short-option parser in short_option_value must stop
scanning a token when it encounters an earlier value-taking option letter.
Update the search logic to recognize interpreter-specific value-taking letters
(`c`/`m` for Python, `e` for Perl/Ruby, and `r` for PHP), returning only the
value for the first applicable option and never treating later letters inside
that value as flags.
In `@scripts/test-legal-discovery-hardening.py`:
- Around line 424-430: Rename the three tests to reflect both their positive
discovery coverage and rejection assertions:
scripts/test-legal-discovery-hardening.py:424-430
test_known_package_manager_forms_fail_closed, :464-471
test_latest_acquisition_forms_fail_closed, and :492-495
test_verbose_installs_and_custom_shell_fail_closed. Alternatively, move each
rejection assertion into its own separately named test while preserving the
existing form-discovery coverage.
- Around line 398-422: Extract the repeated temporary discovery setup from
test_python_local_imports_are_scanned and
test_shell_helper_invoked_python_helper_is_scanned into a shared test helper.
Have it create the images/base, images/codex, and scripts directories, write the
supplied helper contents and Dockerfile bodies, and invoke validate_discovery;
update both tests and the other matching tests to use it while preserving their
distinct helper content and base-image behavior.
- Around line 489-490: Update the assertRaisesRegex call around
legal_inventory.validate_discovery to match the specific “build helper.*curl”
rejection message, consistent with the other tests, rather than the broad “curl”
pattern.
---
Outside diff comments:
In `@scripts/legal_inventory/build_scripts.py`:
- Around line 88-104: Update the candidate guard in the token-processing loop
around short_option_value so it uses the same cluster-aware detection for c or m
as the preceding break condition, excluding tokens such as -lc and -uc from
candidates while preserving normal script-token handling.
In `@scripts/legal_inventory/installer_scan.py`:
- Around line 67-77: The Python pip detection in _is_installer must support both
attached and separate -m arguments. Replace the direct tokens.index("-m", 1)
lookup with short_option_value(token, "m"), handling an attached “pip” module
and the next-token module form, then continue through _skip_options and classify
“install” or “download” as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdcf7309-c701-4510-a834-bd11f2719129
📒 Files selected for processing (5)
scripts/legal_inventory/build_scripts.pyscripts/legal_inventory/docker_parse.pyscripts/legal_inventory/download_scan.pyscripts/legal_inventory/installer_scan.pyscripts/test-legal-discovery-hardening.py
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
scripts/test-legal-discovery-hardening.py
[warning] 393-393: Do not make http calls without encryption
Context: "http://vendor.example/tool"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
scripts/legal_inventory/docker_parse.py
[warning] 21-21: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: token.find(letter, 1)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🪛 Ruff (0.16.0)
scripts/legal_inventory/installer_scan.py
[warning] 65-65: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 77-77: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 84-84: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 88-88: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 95-95: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 101-101: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 119-119: Avoid specifying long messages outside the exception class
(TRY003)
scripts/legal_inventory/download_scan.py
[warning] 155-155: Avoid specifying long messages outside the exception class
(TRY003)
scripts/test-legal-discovery-hardening.py
[warning] 390-390: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 395-395: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 408-408: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 421-421: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 429-429: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 446-446: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 461-461: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 469-469: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 489-489: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 496-496: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
scripts/legal_inventory/docker_parse.py
[warning] 56-56: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 58-61: Avoid specifying long messages outside the exception class
(TRY003)
scripts/legal_inventory/build_scripts.py
[warning] 96-99: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 311-314: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 318-321: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 338-341: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
scripts/test-legal-discovery-hardening.py (4)
372-391: LGTM!
393-396: LGTM!
432-462: LGTM!
588-588: LGTM!scripts/legal_inventory/build_scripts.py (1)
156-199: LGTM!Also applies to: 350-350
scripts/legal_inventory/docker_parse.py (1)
36-74: LGTM!scripts/legal_inventory/download_scan.py (1)
87-96: LGTM!Also applies to: 99-123, 126-138, 141-175
scripts/legal_inventory/installer_scan.py (1)
52-65: LGTM!Also applies to: 79-119
| for substitution in re.findall(r"\$\(([^()]*)\)", text, re.DOTALL): | ||
| for tokens in _command_segments(substitution): | ||
| reason = _acquisition_reason(tokens) | ||
| if reason is not None: | ||
| raise InventoryError( | ||
| f"build helper {script.relative_to(root)} performs command substitution {reason}; " | ||
| "declare acquisition directly in the Dockerfile so legal inventory ownership can be verified" | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '/build_scripts.py/p;/docker_parse.py/p;/legal_inventory/p' | head -200
echo "== build_scripts outline/context =="
if [ -f scripts/legal_inventory/build_scripts.py ]; then
wc -l scripts/legal_inventory/build_scripts.py
ast-grep outline scripts/legal_inventory/build_scripts.py --view compact || true
sed -n '300,360p' scripts/legal_inventory/build_scripts.py
fi
echo "== docker_parse substitution parser =="
if [ -f scripts/legal_inventory/docker_parse.py ]; then
wc -l scripts/legal_inventory/docker_parse.py
rg -n "_command_substitutions|command_substitutions|backtick|quote|escape|\\$\\(" scripts/legal_inventory/docker_parse.py -C 4
sed -n '1,260p' scripts/legal_inventory/docker_parse.py
fi
echo "== tests/usages =="
rg -n "build helper|command substitution|command_substitutions|_command_substitutions|acquisition_reason|legal_inventory" scripts/legal_inventory tests . | head -300Repository: eXPerience83/remote-dev-containers
Length of output: 39974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports/top of build_scripts =="
sed -n '1,70p' scripts/legal_inventory/build_scripts.py
echo "== deterministic regex behavior for reported patterns =="
python3 - <<'PY'
import re
cases = [
"echo $(curl $(hostname))",
"echo $(echo abc)",
'echo $(echo "a(b)c")',
"echo $(echo 'a(b)c')",
"echo $(echo a\\(b\\)",
]
pattern = re.compile(r"\$\(([^()]*)\)", re.DOTALL)
for text in cases:
print(f"text={text!r}")
print(f"findall={pattern.findall(text)!r}")
PY
echo "== tests around build helper command substitution =="
sed -n '300,400p' scripts/test-legal-discovery-hardening.py | sed -n '1,140p'Repository: eXPerience83/remote-dev-containers
Length of output: 9788
Use the Dockerfile substitution parser for build-helper command substitution checks.
re.findall(r"\$\(([^()]*)\)", text, re.DOTALL) drops balanced $(...) contents that include any parenthesis, so nested substitutions like $(curl $(hostname)) stop at the inner $(hostname) and can miss acquisitions. The same pattern also skips substitutions that contain literal parentheses, such as echo "$(printf 'a(b)c')". Reuse docker_parse._command_substitutions so build helpers use the same hardening as Dockerfile RUN instructions.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 338-341: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/legal_inventory/build_scripts.py` around lines 334 - 341, Replace the
regex-based substitution extraction in the build-helper checks with
docker_parse._command_substitutions, then run _command_segments and
_acquisition_reason on each parsed substitution as before. Preserve the existing
InventoryError message and Dockerfile ownership guidance while ensuring nested
and literal-parenthesis substitutions are detected.
| def short_option_value(token: str, letter: str) -> tuple[bool, str | None]: | ||
| """Return whether a short-option cluster contains a value-taking letter.""" | ||
| if not token.startswith("-") or token.startswith("--"): | ||
| return False, None | ||
| position = token.find(letter, 1) | ||
| if position < 0: | ||
| return False, None | ||
| return True, token[position + 1 :] or None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(docker_parse|build_scripts|download_scan)\.py$|scripts/legal_inventory/'
echo
echo "== docker_parse outline =="
ast-grep outline scripts/legal_inventory/docker_parse.py || true
echo
sed -n '1,80p' scripts/legal_inventory/docker_parse.py | cat -n
echo
echo "== build_scripts relevant =="
ast-grep outline scripts/legal_inventory/build_scripts.py || true
echo
sed -n '1,230p' scripts/legal_inventory/build_scripts.py | cat -n
echo
echo "== download_scan parser usages =="
ast-grep outline scripts/legal_inventory/download_scan.py || true
echo
rg -n "short_option_value|download_scan|perl|ruby|php|python3" scripts/legal_inventory -SRepository: eXPerience83/remote-dev-containers
Length of output: 22982
🌐 Web query:
Python command line -c option short option bundle value consumes rest -m flag
💡 Result:
In Python's command-line parsing libraries, "bundling" short options together is a common feature, but how they interact with values depends on the library used. Argparse: When using argparse, multiple short options can be grouped together behind a single hyphen (e.g., -abc) [1][2]. If one of those options requires a value, the remainder of that specific command-line argument is consumed as the value for that option [1][3]. For example, if -a and -b are flags (no value) and -c requires a value, then -abcXYZ will parse -a as True, -b as True, and -c as "XYZ" [1][4]. It does not consume "the rest" of all command-line arguments, only the remainder of the single string in which the short option was bundled [1][5]. Getopt: The getopt module also allows short option bundling [6]. Similar to traditional Unix getopt, if a short option that requires an argument is present, it will consume the remainder of the current argument string if concatenated (e.g., -cXYZ) [7]. If no value is concatenated, it will consume the next item in the argument list [7]. It does not have a "consume the rest" flag that absorbs all remaining command-line arguments into a single option [6][8]. Consuming the Rest: If you need a flag or option to consume all remaining command-line arguments, neither library uses a standard "short option bundle" flag for this purpose. Instead: - In argparse, the nargs=argparse.REMAINDER argument can be used for a positional argument to capture all remaining command-line tokens [9][10]. - Alternatively, the -- (double dash) separator is the standard way to signify the end of option parsing, allowing the remaining items to be treated as positional arguments or extras [9][10][6]. [1][6]
Citations:
- 1: https://docs.python.org/3.11/library/argparse.html
- 2: https://docs.python.org/3.8/library/argparse.html
- 3: https://docs.python.org/3.10/library/argparse.html?highlight=argparse
- 4: https://docs.python.org/3/library/argparse.html
- 5: /Lib/argparse.py incorrect argument parser python/cpython#105804
- 6: https://docs.python.org/3/library/getopt.html
- 7: https://github.com/python/cpython/blob/3.13/Lib/getopt.py
- 8: https://docs.python.org/3.11/library/getopt.html
- 9: https://stackoverflow.com/questions/15480062/python-argparse-remainder-is-not-clear
- 10: Argparse: improve parse_known_args() doc python/cpython#106235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sys
sys.path.insert(0, "scripts/legal_inventory")
from docker_parse import short_option_value
cases = [
("-cimport", "m"),
("-ucimport", "m"),
("-ucimport", "c"),
("-c", "m"),
("-cm", "m"),
("-mi", "m"),
("-mi", "i"),
("-r", "e"),
("-er", "e"),
("-er", "r"),
]
print("short_option_value current behavior:")
for token, letter in cases:
print(f" {token!r}, {letter!r} -> {short_option_value(token, letter)}")
PY
echo
echo "== download_scan relevant =="
sed -n '1,180p' scripts/legal_inventory/download_scan.py | cat -nRepository: eXPerience83/remote-dev-containers
Length of output: 437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def short_option_value(token: str, letter: str):
if not token.startswith("-") or token.startswith("--"):
return False, None
position = token.find(letter, 1)
if position < 0:
return False, None
return True, token[position + 1 :] or None
cases = [
("-cimport", "m"),
("-ucimport", "m"),
("-ucimport", "c"),
("-c", "m"),
("-cm", "m"),
("-mi", "m"),
("-mi", "i"),
("-r", "e"),
("-er", "e"),
("-er", "r"),
("-e123ruby", "e"),
("-rvalue", "r"),
("-e123rvalue", "r"),
]
print("Current short_option_value behavior:")
for token, letter in cases:
print(f"{token!r}, {letter!r} -> {short_option_value(token, letter)}")
PY
echo
sed -n '1,180p' scripts/legal_inventory/download_scan.py | cat -nRepository: eXPerience83/remote-dev-containers
Length of output: 9837
Stop treating letters inside a value-taking option as additional flags.
short_option_value should stop searching once it finds any value-taking letter before the target letter. For Python, -c and -m are value-taking; for perl/ruby, -e is value-taking; for php, -r is value-taking.
This makes bundled usage like python3 -cimport incorrectly trigger Python -m build helpers are unsupported...; the m is inside -c's value, not a separate -m flag.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 21-21: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: token.find(letter, 1)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/legal_inventory/docker_parse.py` around lines 18 - 25, The
short-option parser in short_option_value must stop scanning a token when it
encounters an earlier value-taking option letter. Update the search logic to
recognize interpreter-specific value-taking letters (`c`/`m` for Python, `e` for
Perl/Ruby, and `r` for PHP), returning only the value for the first applicable
option and never treating later letters inside that value as flags.
| def test_python_local_imports_are_scanned(self) -> None: | ||
| with tempfile.TemporaryDirectory() as temporary: | ||
| root = Path(temporary) | ||
| (root / "images/base").mkdir(parents=True) | ||
| (root / "images/codex").mkdir(parents=True) | ||
| (root / "scripts").mkdir() | ||
| (root / "scripts/a.py").write_text("import b\n", encoding="utf-8") | ||
| (root / "scripts/b.py").write_text("import urllib.request\nurllib.request.urlopen('https://vendor.example/tool')\n", encoding="utf-8") | ||
| (root / "images/base/Dockerfile").write_text("FROM ubuntu:${UBUNTU_VERSION}@${UBUNTU_DIGEST}\nCOPY scripts/a.py /a.py\nRUN python3 /a.py\n", encoding="utf-8") | ||
| (root / "images/codex/Dockerfile").write_text("FROM ${BASE_IMAGE}\n", encoding="utf-8") | ||
| with self.assertRaisesRegex(legal_inventory.InventoryError, "build helper.*urlopen"): | ||
| legal_inventory.validate_discovery(root, {"components": [{"id": "project"}]}) | ||
|
|
||
| def test_shell_helper_invoked_python_helper_is_scanned(self) -> None: | ||
| with tempfile.TemporaryDirectory() as temporary: | ||
| root = Path(temporary) | ||
| (root / "images/base").mkdir(parents=True) | ||
| (root / "images/codex").mkdir(parents=True) | ||
| (root / "scripts").mkdir() | ||
| (root / "scripts/a.sh").write_text("python3 /fetch.py\n", encoding="utf-8") | ||
| (root / "scripts/fetch.py").write_text("import urllib.request\nurllib.request.urlopen('https://vendor.example/tool')\n", encoding="utf-8") | ||
| (root / "images/base/Dockerfile").write_text("FROM ubuntu:${UBUNTU_VERSION}@${UBUNTU_DIGEST}\nCOPY scripts/a.sh /a.sh\nCOPY scripts/fetch.py /fetch.py\nRUN sh /a.sh\n", encoding="utf-8") | ||
| (root / "images/codex/Dockerfile").write_text("FROM ${BASE_IMAGE}\n", encoding="utf-8") | ||
| with self.assertRaisesRegex(legal_inventory.InventoryError, "build helper.*urlopen"): | ||
| legal_inventory.validate_discovery(root, {"components": [{"id": "project"}]}) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the repeated discovery fixture into one helper.
These two tests repeat the same setup that at least ten other tests in this file also use: create images/base, images/codex, and scripts, write helper files, write both Dockerfiles, then call validate_discovery. Only the helper contents and the base-image body differ.
A single helper reduces the duplication and shortens the long inline write_text calls at Lines 405, 406, 418, and 419.
♻️ Proposed helper and call sites
+ def discovery_root(self, temporary: str, files: dict[str, str], base_body: str) -> Path:
+ root = Path(temporary)
+ (root / "images/base").mkdir(parents=True)
+ (root / "images/codex").mkdir(parents=True)
+ (root / "scripts").mkdir()
+ for relative, content in files.items():
+ (root / relative).write_text(content, encoding="utf-8")
+ (root / "images/base/Dockerfile").write_text(
+ "FROM ubuntu:${UBUNTU_VERSION}@${UBUNTU_DIGEST}\n" + base_body,
+ encoding="utf-8",
+ )
+ (root / "images/codex/Dockerfile").write_text("FROM ${BASE_IMAGE}\n", encoding="utf-8")
+ return root
+
def test_python_local_imports_are_scanned(self) -> None:
with tempfile.TemporaryDirectory() as temporary:
- root = Path(temporary)
- (root / "images/base").mkdir(parents=True)
- (root / "images/codex").mkdir(parents=True)
- (root / "scripts").mkdir()
- (root / "scripts/a.py").write_text("import b\n", encoding="utf-8")
- (root / "scripts/b.py").write_text("import urllib.request\nurllib.request.urlopen('https://vendor.example/tool')\n", encoding="utf-8")
- (root / "images/base/Dockerfile").write_text("FROM ubuntu:${UBUNTU_VERSION}@${UBUNTU_DIGEST}\nCOPY scripts/a.py /a.py\nRUN python3 /a.py\n", encoding="utf-8")
- (root / "images/codex/Dockerfile").write_text("FROM ${BASE_IMAGE}\n", encoding="utf-8")
+ root = self.discovery_root(
+ temporary,
+ {
+ "scripts/a.py": "import b\n",
+ "scripts/b.py": (
+ "import urllib.request\n"
+ "urllib.request.urlopen('https://vendor.example/tool')\n"
+ ),
+ },
+ "COPY scripts/a.py /a.py\nRUN python3 /a.py\n",
+ )
with self.assertRaisesRegex(legal_inventory.InventoryError, "build helper.*urlopen"):
legal_inventory.validate_discovery(root, {"components": [{"id": "project"}]})🧰 Tools
🪛 Ruff (0.16.0)
[warning] 408-408: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
[warning] 421-421: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-legal-discovery-hardening.py` around lines 398 - 422, Extract
the repeated temporary discovery setup from
test_python_local_imports_are_scanned and
test_shell_helper_invoked_python_helper_is_scanned into a shared test helper.
Have it create the images/base, images/codex, and scripts directories, write the
supplied helper contents and Dockerfile bodies, and invoke validate_discovery;
update both tests and the other matching tests to use it while preserving their
distinct helper content and base-image behavior.
| with self.assertRaisesRegex(legal_inventory.InventoryError, "curl"): | ||
| legal_inventory.validate_discovery(root, {"components": [{"id": "project"}]}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the assertion to pin the rejection reason.
Line 489 matches only curl. Other tests in this file use build helper.*curl for the same rejection, for example Lines 301, 369, and 558. The broad regex here also matches unrelated errors that quote the command, so a regression in helper discovery can keep this test green.
💚 Proposed fix
- with self.assertRaisesRegex(legal_inventory.InventoryError, "curl"):
+ with self.assertRaisesRegex(legal_inventory.InventoryError, "build helper.*curl"):
legal_inventory.validate_discovery(root, {"components": [{"id": "project"}]})🧰 Tools
🪛 Ruff (0.16.0)
[warning] 489-489: Use pytest.raises instead of unittest-style assertRaisesRegex
Replace assertRaisesRegex with pytest.raises
(PT027)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-legal-discovery-hardening.py` around lines 489 - 490, Update the
assertRaisesRegex call around legal_inventory.validate_discovery to match the
specific “build helper.*curl” rejection message, consistent with the other
tests, rather than the broad “curl” pattern.
|
Closing this PR because its scope expanded far beyond issue #26 into a general-purpose, fail-closed Docker/shell/package-manager acquisition analyzer. That approach is not bounded or maintainable: every new parser rule exposes another equivalent syntax to support. The useful legal-compliance work will be rebuilt from a clean
The replacement will deliberately exclude the custom parser for arbitrary Docker, shell, Git, npm, APT, Python, Cargo and other installation forms. PR #43 remains available as historical reference, but no further findings should be addressed here. |
1 participant
Summary
licensesarrays;remote-dev-noticesand fail image builds when required notices, versions, package copyrights or provenance records are missing or inconsistent;ADDsources and externalFROM/COPY --fromimages that are not explicitly inventoried.Important boundaries
org.opencontainers.image.licensesvalue. Custom metadata identifies the project license and points to the complete bundled notice set.maincontent./usr/share/docpaths and are verified in the built images.astral-sh/python-build-standaloneartifact pinned inmise.lock, not inaccurately as a direct python.org download.Validation
remote-dev-notices --checkin both images;b235307129a80d6da8efcf94587978839b9ba1b0without new actionable findings;b235307129a80d6da8efcf94587978839b9ba1b0without new actionable findings;Latest complete AMD64 run:
30644061727— validation, hardening regressions, builds, smoke tests, both SPDX SBOMs, inventory reconciliation, both Trivy scans and the fixable-critical gate succeeded.Refs #26