some fixs#3
Open
sm8949752-ux wants to merge 22 commits into
Open
Conversation
…nce Java version support
…pport, enhance FIPS compliance checks
Updated base image to Ubuntu 22.04 and modified package installations.
…d improved logging
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Dev Container support, replaces VS Code tasks with Taskfile-based automation, rewrites FIPS Java buildpack Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
.devcontainer/devcontainer.json (1)
35-35: Security consideration:chmod 666on docker socket is overly permissive.Setting world-read/write permissions on
/var/run/docker.sockallows any user in the container to control Docker. While acceptable for a dev container, be aware this grants significant privileges. The trailing|| truealso silently swallows all errors, which could mask setup failures.Consider using group-based permissions instead:
"postCreateCommand": "sudo chown vscode:docker /var/run/docker.sock && pip3 install rich"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/devcontainer.json at line 35, Replace the insecure and noisy postCreateCommand that sets world-write permissions and swallows errors: locate the "postCreateCommand" entry that runs "chmod 666 /var/run/docker.sock && pip3 install rich && sudo chown vscode:vscode /var/run/docker.sock || true" and change it to use group-based ownership instead of chmod 666 (e.g., set ownership to vscode:docker) and remove the trailing "|| true" so failures are not masked; ensure the command still installs "pip3 install rich" and runs with sudo where needed so the container user (vscode) gains docker group access rather than world RW on /var/run/docker.sock.fips-java-shim/bin/detect (1)
86-94: Consider using a TOML library for plan generation.String concatenation for TOML generation works but is fragile and harder to maintain. Since this is the detect phase and dependencies should be minimal, the current approach is acceptable, but consider using
tomli_wortomlin the future for complex plan structures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/detect` around lines 86 - 94, The current detect script manually concatenates TOML strings when writing to plan_path using clean_version, which is brittle; replace the hand-built strings in the WRITE BUILD PLAN section with structured TOML generation: build a Python dict representing the provides/requires blocks (using keys like "provides" and "requires" and metadata.version set to clean_version) and serialize it with a TOML library such as toml or tomli_w (import and use toml.dumps or tomli_w.dump) before writing to plan_path; ensure you remove the concatenated f.write calls and instead write the serialized TOML string so the plan is robust and easier to maintain..devcontainer/Dockerfile (1)
5-11: Add--no-install-recommendsto reduce image size.Adding
--no-install-recommendsprevents installation of suggested packages, reducing the image size.♻️ Proposed fix
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ python3 \ python3-pip \ curl \ git \ sudo \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/Dockerfile around lines 5 - 11, Edit the Dockerfile RUN line that performs apt-get install so it uses apt-get install -y --no-install-recommends to avoid pulling suggested packages; specifically update the existing RUN command (the one starting with "RUN apt-get update && apt-get install -y \") to insert --no-install-recommends immediately after install -y and keep the trailing "&& rm -rf /var/lib/apt/lists/*" cleanup intact.layers.py (2)
49-50: Consider catching specific exceptions for better error diagnostics.The broad
except Exceptionobscures the root cause. Consider catchingsubprocess.CalledProcessError,FileNotFoundError,json.JSONDecodeError, andKeyErrorseparately, or at minimum re-raise after logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layers.py` around lines 49 - 50, Replace the broad "except Exception as e" block with targeted handlers: add separate except blocks for subprocess.CalledProcessError, FileNotFoundError, json.JSONDecodeError, and KeyError that each log a clear, contextual message to sys.stderr (including the exception details), and for any remaining errors either re-raise or log then raise; specifically change the existing "except Exception as e" to multiple except clauses (subprocess.CalledProcessError, FileNotFoundError, json.JSONDecodeError, KeyError) and a final bare "except Exception" that re-raises after logging to preserve diagnostics.
30-30: Rename ambiguous variableland handle potentialNoneforsha.The variable
lis ambiguous per PEP 8 (can be confused with1orI). Additionally,l['sha'][:12]will raiseTypeErrorifdigestIdisNone.♻️ Proposed fix
- for l in layers: - if "taha/fips-java" in l['cmd']: + for layer in layers: + if "taha/fips-java" in layer['cmd']: layer_type = "JRE/FIPS Layer" - elif "Application Slice" in l['cmd']: + elif "Application Slice" in layer['cmd']: layer_type = "App Code Layer" - elif "paketo-buildpacks" in l['cmd']: + elif "paketo-buildpacks" in layer['cmd']: layer_type = "Buildpack Helper" else: layer_type = "OS/Base Layer" - writer.writerow([layer_type, f"{l['size']:.6f} MB", l['sha'], l['cmd']]) + writer.writerow([layer_type, f"{layer['size']:.6f} MB", layer['sha'], layer['cmd']]) print(f"Success! Detailed report: {csv_path}") print("\n--- Top 5 Largest Layers ---") top_layers = sorted(layers, key=lambda x: x['size'], reverse=True)[:5] - for i, l in enumerate(top_layers, 1): - print(f"{i}. {l['size']:.6f} MB | {l['sha'][:12]}... | {l['cmd'][:40]}") + for i, layer in enumerate(top_layers, 1): + sha_display = (layer['sha'] or "unknown")[:12] + print(f"{i}. {layer['size']:.6f} MB | {sha_display}... | {layer['cmd'][:40]}")Also applies to: 46-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layers.py` at line 30, Rename the ambiguous loop variable l to layer in the for loops over layers and update all uses accordingly, and guard slicing of the digest by checking for None: retrieve the digest with layer.get('sha') or layer.get('digestId') into a variable (e.g., sha), then if sha is truthy take sha[:12] for the short id, otherwise use a safe placeholder (e.g., '<none>' or '') to avoid TypeError; apply the same changes to the similar block around the other loop (previously at lines 46-47) so both loops use layer and a None-safe sha handling.taskfile/app.Taskfile.yml (1)
4-4: Reuse the root builder task/variable here.If these sample builds are meant to use the repo-local builder, wire the root builder task in as a dependency or share the same image variable. Right now
build-maven/build-gradleonly work whentaha/wolfi-fips-builder:latestalready exists locally or can be pulled from a registry.Also applies to: 11-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfile/app.Taskfile.yml` at line 4, The build tasks currently hard-code the image name; update build-maven and build-gradle to reuse the root BUILDER variable or depend on the root builder task instead of referencing taha/wolfi-fips-builder:latest directly — e.g., replace the literal image in build-maven/build-gradle with the shared BUILDER variable or add the root builder task as a dependency so they use the repo-local builder task/image; ensure references to BUILDER, build-maven, and build-gradle in the Taskfile are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/Dockerfile:
- Around line 17-18: The Dockerfile hardcodes pack CLI version v0.36.4; update
the RUN that downloads "pack-v0.36.4-linux.tgz" to use v0.40.2 (i.e., replace
the version token in the URL and filename) or if v0.36.4 is mandatory, add a
comment above that RUN explaining the constraint and why the older version is
required; locate the installation step by the pack download/tar invocation to
make the change.
In `@2-builder/builder.toml`:
- Around line 165-181: The sbt and executable-jar order groups are missing
shared utility buildpacks — add paketo-buildpacks/environment-variables,
paketo-buildpacks/image-labels, and paketo-buildpacks/procfile to the
[[order.group]] entries for id = "paketo-buildpacks/sbt" and id =
"paketo-buildpacks/executable-jar"; additionally restore the observability
helpers (the paketo observability buildpacks present in the Maven/Gradle groups)
into the sbt group so apps retain observability detection. Repeat the same fixes
for the later duplicate section (the ranges noted as also applies to 183-211).
Ensure you insert the same buildpack ids in the same group ordering style as the
other language groups.
In `@4-gradle-app/src/main/java/com/taha/wolfidemo/DemoApplication.java`:
- Line 47: The return string in DemoApplication (the method returning the
payload that currently returns " just change SUCCESS: FIPS is strictly
enforced...") contains an accidental prefix " just change" that breaks the
response contract; update the return expression in that method so the returned
payload begins with "SUCCESS: FIPS is strictly enforced. System blocked insecure
RSA-1024 key generation. Error Type: " (remove the " just change " prefix) so
consumers/assertions that expect the "SUCCESS:" prefix are preserved.
In `@build-image/Dockerfile`:
- Around line 23-25: Remove the duplicate directory creation by keeping a single
RUN mkdir -p /opt/jdks /opt/jres line and deleting the repeated RUN mkdir -p
/opt/jdks /opt/jres; update the Dockerfile so only one RUN instruction performs
the mkdir to avoid the extra image layer (look for the two identical RUN mkdir
entries in the Dockerfile).
In `@fips-java-shim/bin/build`:
- Around line 24-30: The current get_fingerprint function only hashes filenames
and sizes under bin/*, which misses other layer inputs; update get_fingerprint
to also include the contents or at least filenames and sizes (or hashes) of the
BCFIPS jars and the cacerts file under lib/security (e.g., include
directory.glob('lib/**/*.jar') and the specific 'lib/security/cacerts' path)
when building the hasher so changes to those files will change the fingerprint;
ensure you still handle missing paths gracefully (returning "MISSING" only if
the whole directory is absent) and include the unique symbols get_fingerprint,
directory, and the added glob patterns so reviewers can locate the change.
- Around line 64-65: Check that the requested JDK/JRE paths exist before calling
get_fingerprint or writing any layer metadata: validate jdk_src (and jre_src in
the later block) with an existence check and fail fast with a clear error
message (e.g., sys.exit or raise SystemExit) if the directory is missing, and
only call get_fingerprint(jdk_src) after that validation; apply the same pattern
to the JRE handling in the section around get_fingerprint usage (lines ~120-135)
so you never proceed to metadata writes when the requested JVM version is
absent.
- Around line 54-56: The build currently computes MaxRAMPercentage at image
build time (using jvm_type, headroom, ram_percentage and BPL_JVM_HEAD_ROOM) and
writes the concrete value into env.launch/JAVA_TOOL_OPTIONS.append so changes to
BPL_JVM_HEAD_ROOM at container start are ignored; move the
headroom-to-ram_percentage calculation out of bin/build into the launch-time
script that assembles env.launch/JAVA_TOOL_OPTIONS.append (or alternatively
treat BPL_JVM_HEAD_ROOM as a build-time BP_ variable and validate/guard it
before emitting the flag). Concretely: remove the
int(os.getenv("BPL_JVM_HEAD_ROOM", "25")) / ram_percentage calculation from the
build phase and implement the same logic in the launch-phase code that
references env.launch/JAVA_TOOL_OPTIONS.append (or validate BPL_JVM_HEAD_ROOM as
a BP_ var in the build path if you intend it to be immutable).
- Around line 32-37: The download_jma function currently fetches a live GitHub
asset with no timeout and no integrity check; update download_jma to enforce a
network timeout and validate the file's SHA256 (or fail build) after download
(or alternatively skip the network download if a vendored JAR is present).
Concretely: replace the simple urllib.request.urlretrieve call in download_jma
with a download that uses a timeout (e.g., urllib.request.urlopen or requests
with timeout) and write to jma_jar, compute the SHA256 of the saved file and
compare it against a pinned constant digest (add a constant like JMA_SHA256),
and raise an exception or exit if the digest does not match; also allow a
vendoring path by checking for an already-present jma_jar before attempting
network access.
In `@fips-java-shim/bin/detect`:
- Around line 23-27: The main() function reads sys.argv[1] and sys.argv[2] (used
to set platform_dir and plan_path) without validating argument count, which can
raise IndexError; add a defensive check at the start of main() to ensure
len(sys.argv) >= 3, and if not, print a clear usage/error message and exit with
non‑zero status so manual runs fail gracefully (or raise a user-friendly
SystemExit). Ensure the check happens before any use of platform_dir, plan_path,
or app_dir so no unguarded accesses to sys.argv occur.
In `@fips-java-shim/buildpack.toml`:
- Around line 4-5: The buildpack.toml currently advertises universal stack
support via [[stacks]] id = "*" but the shim relies on the Wolfi layout under
fips-java-shim/bin/build (e.g., /opt/jdks/*, /opt/jres/* and bundled FIPS jars),
so change the [[stacks]] id from "*" to the actual custom stack id used by your
Wolfi-based builders (or the specific stack string your CI uses) to avoid
claiming unsupported stacks; alternatively, if you want to keep a broader id,
implement a detect check in the build script (referencing
fips-java-shim/bin/build or its detect entrypoint) that verifies the Wolfi
layout before accepting the stack.
In `@layers.py`:
- Around line 14-15: The subprocess invocation using subprocess.run with
shell=True and an f-string (the call to subprocess.run in layers.py that runs
"dive {image_name} --json image_report.json") is vulnerable to shell injection;
change it to invoke the command as a list of arguments (e.g., ["dive",
image_name, "--json", "image_report.json"]) and remove shell=True so the
image_name is passed as a single argument, keeping check=True and capture_output
as needed; ensure any return or exception handling around that subprocess.run
call still works with the new argument list.
In `@run-image/Dockerfile.run`:
- Around line 3-8: The cnb user/group are created with UID/GID 1000 which
mismatches other images; update the addgroup/adduser invocations so the cnb
group and user are created with --gid 1001 and --uid 1001 (i.e., change the
numeric IDs in the addgroup -S cnb --gid and adduser -S cnb -G cnb --uid calls)
and ensure the USER directive uses 1001 to match build-image/Dockerfile and
run-image/Dockerfile for consistent CNB layer ownership.
In `@taskfile/buildpack.Taskfile.yml`:
- Around line 25-31: The inspect-any task uses Bash process substitution (<(...)
) which fails under /bin/sh; update the Taskfile by setting the task's shell to
Bash (add shell: ["bash","-lc"] to the inspect-any task) or alternatively
rewrite the cmds pipeline to avoid process substitution (e.g., use temporary
files or standard pipelines) so the paste <(...) invocation is removed;
reference the inspect-any task and the paste <(docker inspect ...) <(docker
history ...) command when making the change.
---
Nitpick comments:
In @.devcontainer/devcontainer.json:
- Line 35: Replace the insecure and noisy postCreateCommand that sets
world-write permissions and swallows errors: locate the "postCreateCommand"
entry that runs "chmod 666 /var/run/docker.sock && pip3 install rich && sudo
chown vscode:vscode /var/run/docker.sock || true" and change it to use
group-based ownership instead of chmod 666 (e.g., set ownership to
vscode:docker) and remove the trailing "|| true" so failures are not masked;
ensure the command still installs "pip3 install rich" and runs with sudo where
needed so the container user (vscode) gains docker group access rather than
world RW on /var/run/docker.sock.
In @.devcontainer/Dockerfile:
- Around line 5-11: Edit the Dockerfile RUN line that performs apt-get install
so it uses apt-get install -y --no-install-recommends to avoid pulling suggested
packages; specifically update the existing RUN command (the one starting with
"RUN apt-get update && apt-get install -y \") to insert --no-install-recommends
immediately after install -y and keep the trailing "&& rm -rf
/var/lib/apt/lists/*" cleanup intact.
In `@fips-java-shim/bin/detect`:
- Around line 86-94: The current detect script manually concatenates TOML
strings when writing to plan_path using clean_version, which is brittle; replace
the hand-built strings in the WRITE BUILD PLAN section with structured TOML
generation: build a Python dict representing the provides/requires blocks (using
keys like "provides" and "requires" and metadata.version set to clean_version)
and serialize it with a TOML library such as toml or tomli_w (import and use
toml.dumps or tomli_w.dump) before writing to plan_path; ensure you remove the
concatenated f.write calls and instead write the serialized TOML string so the
plan is robust and easier to maintain.
In `@layers.py`:
- Around line 49-50: Replace the broad "except Exception as e" block with
targeted handlers: add separate except blocks for subprocess.CalledProcessError,
FileNotFoundError, json.JSONDecodeError, and KeyError that each log a clear,
contextual message to sys.stderr (including the exception details), and for any
remaining errors either re-raise or log then raise; specifically change the
existing "except Exception as e" to multiple except clauses
(subprocess.CalledProcessError, FileNotFoundError, json.JSONDecodeError,
KeyError) and a final bare "except Exception" that re-raises after logging to
preserve diagnostics.
- Line 30: Rename the ambiguous loop variable l to layer in the for loops over
layers and update all uses accordingly, and guard slicing of the digest by
checking for None: retrieve the digest with layer.get('sha') or
layer.get('digestId') into a variable (e.g., sha), then if sha is truthy take
sha[:12] for the short id, otherwise use a safe placeholder (e.g., '<none>' or
'') to avoid TypeError; apply the same changes to the similar block around the
other loop (previously at lines 46-47) so both loops use layer and a None-safe
sha handling.
In `@taskfile/app.Taskfile.yml`:
- Line 4: The build tasks currently hard-code the image name; update build-maven
and build-gradle to reuse the root BUILDER variable or depend on the root
builder task instead of referencing taha/wolfi-fips-builder:latest directly —
e.g., replace the literal image in build-maven/build-gradle with the shared
BUILDER variable or add the root builder task as a dependency so they use the
repo-local builder task/image; ensure references to BUILDER, build-maven, and
build-gradle in the Taskfile are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0991f8e2-12ec-44c8-892c-24ed44f7961b
📒 Files selected for processing (15)
.devcontainer/Dockerfile.devcontainer/devcontainer.json.vscode/tasks.json2-builder/builder.toml4-gradle-app/src/main/java/com/taha/wolfidemo/DemoApplication.javaTaskfile.ymlbuild-image/Dockerfilefips-java-shim/bin/buildfips-java-shim/bin/detectfips-java-shim/buildpack.tomllayers.pyrun-image/Dockerfilerun-image/Dockerfile.runtaskfile/app.Taskfile.ymltaskfile/buildpack.Taskfile.yml
💤 Files with no reviewable changes (1)
- .vscode/tasks.json
Comment on lines
+17
to
+18
| RUN curl -sSL "https://github.com/buildpacks/pack/releases/download/v0.36.4/pack-v0.36.4-linux.tgz" \ | ||
| | tar -C /usr/local/bin/ --no-same-owner -xzv pack |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pack CLI latest version buildpacks
💡 Result:
The latest version of the pack CLI, the official command-line interface for Cloud Native Buildpacks, is v0.40.2, released on 2026-03-13. Pack CLI is used to build OCI images from source code using buildpacks. It supports commands like pack build to generate app images, manage builders, buildpacks, and more. Installation instructions from official docs (as of 2026-03-22): - Linux: curl -sSL "https://github.com/buildpacks/pack/releases/download/v0.40.2/pack-v0.40.2-linux.tgz" | sudo tar -C /usr/local/bin/ --strip-components=1 -xzf - - macOS: brew install buildpacks/tap/pack - Or download binaries from https://github.com/buildpacks/pack/releases/tag/v0.40.2 Official docs: https://buildpacks.io/docs/tools/pack/cli/pack/ Source: https://github.com/buildpacks/pack
Citations:
- 1: https://github.com/buildpacks/pack
- 2: https://github.com/buildpacks/pack/releases
- 3: https://buildpacks.io/docs/for-platform-operators/how-to/integrate-ci/pack
- 4: https://www.block.github.io
Update the hardcoded pack CLI version to v0.40.2 or document why v0.36.4 is required.
The pack CLI is pinned to v0.36.4, which is significantly outdated. The latest stable version is v0.40.2 (released 2026-03-13). Either update to the latest version or add a comment explaining the constraint.
Current installation command
RUN curl -sSL "https://github.com/buildpacks/pack/releases/download/v0.36.4/pack-v0.36.4-linux.tgz" \
| tar -C /usr/local/bin/ --no-same-owner -xzv pack
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/Dockerfile around lines 17 - 18, The Dockerfile hardcodes pack
CLI version v0.36.4; update the RUN that downloads "pack-v0.36.4-linux.tgz" to
use v0.40.2 (i.e., replace the version token in the URL and filename) or if
v0.36.4 is mandatory, add a comment above that RUN explaining the constraint and
why the older version is required; locate the installation step by the pack
download/tar invocation to make the change.
Comment on lines
+165
to
+181
| [[order]] | ||
| [[order.group]] | ||
| id = "paketo-buildpacks/ca-certificates" | ||
| optional = true | ||
| [[order.group]] | ||
| id = "taha/fips-java" | ||
| [[order.group]] | ||
| id = "paketo-buildpacks/syft" | ||
| optional = true | ||
| [[order.group]] | ||
| id = "paketo-buildpacks/sbt" | ||
| [[order.group]] | ||
| id = "paketo-buildpacks/executable-jar" | ||
| optional = true | ||
| [[order.group]] | ||
| id = "paketo-buildpacks/spring-boot" | ||
| optional = true |
There was a problem hiding this comment.
The sbt/executable-jar orders dropped shared utility buildpacks.
Unlike the Maven/Gradle groups, these new groups omit paketo-buildpacks/environment-variables, paketo-buildpacks/image-labels, and paketo-buildpacks/procfile; the sbt group also drops the observability helpers entirely. Apps that detect through these groups lose those capabilities even though the builder still bundles the images.
Also applies to: 183-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@2-builder/builder.toml` around lines 165 - 181, The sbt and executable-jar
order groups are missing shared utility buildpacks — add
paketo-buildpacks/environment-variables, paketo-buildpacks/image-labels, and
paketo-buildpacks/procfile to the [[order.group]] entries for id =
"paketo-buildpacks/sbt" and id = "paketo-buildpacks/executable-jar";
additionally restore the observability helpers (the paketo observability
buildpacks present in the Maven/Gradle groups) into the sbt group so apps retain
observability detection. Repeat the same fixes for the later duplicate section
(the ranges noted as also applies to 183-211). Ensure you insert the same
buildpack ids in the same group ordering style as the other language groups.
| if (errorType.contains("FipsUnapprovedOperationError") || | ||
| (message != null && message.toLowerCase().contains("approved only mode"))) { | ||
| return "SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key generation. Error Type: " + errorType; | ||
| return " just change SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key generation. Error Type: " + errorType; |
There was a problem hiding this comment.
Remove accidental prefix in success response
Line 47 includes an unintended " just change " prefix in the success payload. This alters the expected response contract and may break assertions/consumers relying on the "SUCCESS:" start.
Suggested fix
- return " just change SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key generation. Error Type: " + errorType;
+ return "SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key generation. Error Type: " + errorType;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return " just change SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key generation. Error Type: " + errorType; | |
| return "SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key generation. Error Type: " + errorType; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@4-gradle-app/src/main/java/com/taha/wolfidemo/DemoApplication.java` at line
47, The return string in DemoApplication (the method returning the payload that
currently returns " just change SUCCESS: FIPS is strictly enforced...") contains
an accidental prefix " just change" that breaks the response contract; update
the return expression in that method so the returned payload begins with
"SUCCESS: FIPS is strictly enforced. System blocked insecure RSA-1024 key
generation. Error Type: " (remove the " just change " prefix) so
consumers/assertions that expect the "SUCCESS:" prefix are preserved.
Comment on lines
+23
to
25
| RUN mkdir -p /opt/jdks /opt/jres | ||
|
|
||
| RUN mkdir -p /opt/jdks /opt/jres |
There was a problem hiding this comment.
Remove duplicate mkdir command.
Lines 23 and 25 both execute mkdir -p /opt/jdks /opt/jres. This is redundant and adds an unnecessary layer to the image.
🧹 Proposed fix
RUN pip install --no-cache-dir rich --break-system-packages
-RUN mkdir -p /opt/jdks /opt/jres
-
RUN mkdir -p /opt/jdks /opt/jres🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-image/Dockerfile` around lines 23 - 25, Remove the duplicate directory
creation by keeping a single RUN mkdir -p /opt/jdks /opt/jres line and deleting
the repeated RUN mkdir -p /opt/jdks /opt/jres; update the Dockerfile so only one
RUN instruction performs the mkdir to avoid the extra image layer (look for the
two identical RUN mkdir entries in the Dockerfile).
Comment on lines
+24
to
+30
| def get_fingerprint(directory: Path): | ||
| if not directory.exists(): return "MISSING" | ||
| hasher = hashlib.sha256() | ||
| for p in sorted(directory.glob('bin/*')): | ||
| if p.is_file(): | ||
| hasher.update(f"{p.name}{p.stat().st_size}".encode()) | ||
| return hasher.hexdigest()[:12] |
There was a problem hiding this comment.
Strengthen the layer fingerprint before reusing cached JVM bits.
This hash only covers bin/* filenames and sizes, but the launch layer also depends on lib/security/cacerts and the BCFIPS jars. A source update outside bin/* can incorrectly hit the cache and keep stale crypto/runtime contents in the reused layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/bin/build` around lines 24 - 30, The current get_fingerprint
function only hashes filenames and sizes under bin/*, which misses other layer
inputs; update get_fingerprint to also include the contents or at least
filenames and sizes (or hashes) of the BCFIPS jars and the cacerts file under
lib/security (e.g., include directory.glob('lib/**/*.jar') and the specific
'lib/security/cacerts' path) when building the hasher so changes to those files
will change the fingerprint; ensure you still handle missing paths gracefully
(returning "MISSING" only if the whole directory is absent) and include the
unique symbols get_fingerprint, directory, and the added glob patterns so
reviewers can locate the change.
Comment on lines
+23
to
+27
| def main(): | ||
| # Detect arguments: $1 = platform dir, $2 = plan path | ||
| platform_dir = Path(sys.argv[1]) | ||
| plan_path = Path(sys.argv[2]) | ||
| app_dir = Path.cwd() |
There was a problem hiding this comment.
Add argument validation to prevent IndexError.
The function accesses sys.argv[1] and sys.argv[2] without validating that sufficient arguments were passed. While CNB lifecycle should always provide these, defensive coding prevents cryptic errors during manual testing.
🛡️ Proposed fix
def main():
- # Detect arguments: $1 = platform dir, $2 = plan path
- platform_dir = Path(sys.argv[1])
- plan_path = Path(sys.argv[2])
+ # Detect arguments: $1 = platform dir, $2 = plan path
+ if len(sys.argv) < 3:
+ print("Usage: detect <platform_dir> <plan_path>", file=sys.stderr)
+ sys.exit(1)
+ platform_dir = Path(sys.argv[1])
+ plan_path = Path(sys.argv[2])
app_dir = Path.cwd()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/bin/detect` around lines 23 - 27, The main() function reads
sys.argv[1] and sys.argv[2] (used to set platform_dir and plan_path) without
validating argument count, which can raise IndexError; add a defensive check at
the start of main() to ensure len(sys.argv) >= 3, and if not, print a clear
usage/error message and exit with non‑zero status so manual runs fail gracefully
(or raise a user-friendly SystemExit). Ensure the check happens before any use
of platform_dir, plan_path, or app_dir so no unguarded accesses to sys.argv
occur.
Comment on lines
+4
to
+5
| [[stacks]] | ||
| id = "*" |
There was a problem hiding this comment.
Don't claim wildcard stack compatibility here.
This shim depends on the Wolfi layout baked into fips-java-shim/bin/build (/opt/jdks/*, /opt/jres/*, bundled FIPS jars), so id = "*" advertises builders it cannot actually support. Pin this to the custom stack id unless you also add detection for that layout.
🔧 Suggested change
[[stacks]]
- id = "*"
+ id = "taha/wolfi-build-image"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [[stacks]] | |
| id = "*" | |
| [[stacks]] | |
| id = "taha/wolfi-build-image" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/buildpack.toml` around lines 4 - 5, The buildpack.toml
currently advertises universal stack support via [[stacks]] id = "*" but the
shim relies on the Wolfi layout under fips-java-shim/bin/build (e.g.,
/opt/jdks/*, /opt/jres/* and bundled FIPS jars), so change the [[stacks]] id
from "*" to the actual custom stack id used by your Wolfi-based builders (or the
specific stack string your CI uses) to avoid claiming unsupported stacks;
alternatively, if you want to keep a broader id, implement a detect check in the
build script (referencing fips-java-shim/bin/build or its detect entrypoint)
that verifies the Wolfi layout before accepting the stack.
Comment on lines
+14
to
+15
| try: | ||
| subprocess.run(f"dive {image_name} --json image_report.json", shell=True, check=True, capture_output=True) |
There was a problem hiding this comment.
Avoid shell=True with user-provided input.
Using shell=True with string interpolation can lead to command injection if image_name contains shell metacharacters. While this is a dev tool, it's better practice to use a list of arguments.
🔒 Proposed fix
- subprocess.run(f"dive {image_name} --json image_report.json", shell=True, check=True, capture_output=True)
+ subprocess.run(["dive", image_name, "--json", "image_report.json"], check=True, capture_output=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| subprocess.run(f"dive {image_name} --json image_report.json", shell=True, check=True, capture_output=True) | |
| try: | |
| subprocess.run(["dive", image_name, "--json", "image_report.json"], check=True, capture_output=True) |
🧰 Tools
🪛 Ruff (0.15.6)
[error] 15-15: subprocess call with shell=True identified, security issue
(S602)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@layers.py` around lines 14 - 15, The subprocess invocation using
subprocess.run with shell=True and an f-string (the call to subprocess.run in
layers.py that runs "dive {image_name} --json image_report.json") is vulnerable
to shell injection; change it to invoke the command as a list of arguments
(e.g., ["dive", image_name, "--json", "image_report.json"]) and remove
shell=True so the image_name is passed as a single argument, keeping check=True
and capture_output as needed; ensure any return or exception handling around
that subprocess.run call still works with the new argument list.
Comment on lines
+3
to
+8
| RUN addgroup -S cnb --gid 1000 && \ | ||
| adduser -S cnb -G cnb --uid 1000 | ||
|
|
||
| LABEL io.buildpacks.stack.id="taha/wolfi-build-image" | ||
|
|
||
| USER 1000 No newline at end of file |
There was a problem hiding this comment.
UID/GID mismatch will break CNB layer mounting.
This Dockerfile creates the cnb user with UID/GID 1000, but both build-image/Dockerfile and run-image/Dockerfile use UID/GID 1001. Cloud Native Buildpacks requires consistent UID/GID between build and run images for proper layer ownership and permissions.
🐛 Proposed fix to align with other images
FROM cgr.dev/chainguard/wolfi-base:latest
-RUN addgroup -S cnb --gid 1000 && \
- adduser -S cnb -G cnb --uid 1000
+RUN addgroup -S cnb --gid 1001 && \
+ adduser -S cnb -G cnb --uid 1001
LABEL io.buildpacks.stack.id="taha/wolfi-build-image"
-USER 1000
+USER 1001📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RUN addgroup -S cnb --gid 1000 && \ | |
| adduser -S cnb -G cnb --uid 1000 | |
| LABEL io.buildpacks.stack.id="taha/wolfi-build-image" | |
| USER 1000 | |
| RUN addgroup -S cnb --gid 1001 && \ | |
| adduser -S cnb -G cnb --uid 1001 | |
| LABEL io.buildpacks.stack.id="taha/wolfi-build-image" | |
| USER 1001 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@run-image/Dockerfile.run` around lines 3 - 8, The cnb user/group are created
with UID/GID 1000 which mismatches other images; update the addgroup/adduser
invocations so the cnb group and user are created with --gid 1001 and --uid 1001
(i.e., change the numeric IDs in the addgroup -S cnb --gid and adduser -S cnb -G
cnb --uid calls) and ensure the USER directive uses 1001 to match
build-image/Dockerfile and run-image/Dockerfile for consistent CNB layer
ownership.
Comment on lines
+25
to
+31
| inspect-any: | ||
| desc: Show layers for a specific image (usage task app:inspect-any IMG=name) | ||
| vars: | ||
| IMAGE: '{{default "taha/maven-fips-app:latest" .IMG}}' | ||
| cmds: | ||
| - | | ||
| paste <(docker inspect {{.IMAGE}} --format='{{range .RootFS.Layers}}{{.}}{{println}}{{end}}') <(docker history {{.IMAGE}} --format "{{.Size}}" | grep -v SIZE | tac) | column -t > 1.txt |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any Taskfile explicitly switches task execution to bash.
fd 'Taskfile.*\.yml$' -x sh -c '
printf "\n== %s ==\n" "$1"
rg -n -C2 "shell:|inspect-any|<\(" "$1"
' sh {}Repository: taha2samy/custom-wolfi-buildpack-java
Length of output: 769
🏁 Script executed:
# Check for any shell configuration in the root Taskfile and buildpack Taskfile
cat -n taskfile/buildpack.Taskfile.yml | head -50Repository: taha2samy/custom-wolfi-buildpack-java
Length of output: 1218
🏁 Script executed:
# Also check the root Taskfile.yml for global shell settings
cat -n Taskfile.yml | head -50Repository: taha2samy/custom-wolfi-buildpack-java
Length of output: 1589
🏁 Script executed:
# Search for any 'shell:' or 'set:' directives that might affect task execution
rg -n "shell:|set:|cmds:" taskfile/buildpack.Taskfile.yml -B2 -A2Repository: taha2samy/custom-wolfi-buildpack-java
Length of output: 862
inspect-any uses Bash-only syntax without specifying Bash as the shell.
The process substitutions on line 31 (<(...)) require Bash. Since neither the task nor the Taskfile overrides the shell, execution will fall back to /bin/sh and fail. Either add shell: [bash, -lc] to the task definition or rewrite the pipeline without process substitution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@taskfile/buildpack.Taskfile.yml` around lines 25 - 31, The inspect-any task
uses Bash process substitution (<(...) ) which fails under /bin/sh; update the
Taskfile by setting the task's shell to Bash (add shell: ["bash","-lc"] to the
inspect-any task) or alternatively rewrite the cmds pipeline to avoid process
substitution (e.g., use temporary files or standard pipelines) so the paste
<(...) invocation is removed; reference the inspect-any task and the paste
<(docker inspect ...) <(docker history ...) command when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Improvements