check the layer media type before flashing#9
check the layer media type before flashing#9bennyz wants to merge 2 commits intojumpstarter-dev:mainfrom
Conversation
To avoid flashing non disk layers we need to check the media type Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fls/oci/manifest.rs (1)
67-69: Add direct tests for the new selection rules.A couple of focused unit tests for (1)
artifact_typeselection, (2) single non-disk layer rejection, and (3) the no-match error would cover the new branches inget_single_layer(). The current tests in this file only exercise parsing.Also applies to: 149-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/manifest.rs` around lines 67 - 69, Add three focused unit tests for Manifest::get_single_layer covering the new selection branches: (1) create a manifest with artifact_type set and layers such that the correct layer is chosen then assert get_single_layer returns that layer, (2) create a manifest with a single layer whose mediaType is not a disk type and assert get_single_layer returns the expected rejection/error, and (3) create a manifest with no matching layers and assert get_single_layer returns the no-match error. Locate the Manifest struct and the get_single_layer method to construct test Manifests and expected Layer descriptors, use the same error types returned by get_single_layer for assertions, and add these tests adjacent to the existing parsing tests so they exercise lines handling artifact_type and the layer-selection branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fls/oci/manifest.rs`:
- Around line 145-163: The early-return and artifact_type match currently accept
any layer regardless of media_type; update the selection logic in manifest.rs so
every successful path validates the layer.media_type against the allowed
flashable disk types (e.g., check starts_with("application/vnd.automotive.disk")
or an equivalent allowed list) before returning; specifically, when
m.layers.len() == 1 verify m.layers[0].media_type is flashable, when matching
via m.artifact_type ensure the found layer also has a flashable media_type, and
keep the existing fallback that finds the first layer whose media_type
starts_with the disk prefix.
---
Nitpick comments:
In `@src/fls/oci/manifest.rs`:
- Around line 67-69: Add three focused unit tests for Manifest::get_single_layer
covering the new selection branches: (1) create a manifest with artifact_type
set and layers such that the correct layer is chosen then assert
get_single_layer returns that layer, (2) create a manifest with a single layer
whose mediaType is not a disk type and assert get_single_layer returns the
expected rejection/error, and (3) create a manifest with no matching layers and
assert get_single_layer returns the no-match error. Locate the Manifest struct
and the get_single_layer method to construct test Manifests and expected Layer
descriptors, use the same error types returned by get_single_layer for
assertions, and add these tests adjacent to the existing parsing tests so they
exercise lines handling artifact_type and the layer-selection branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55bbc641-f85a-4511-a0be-17bb83c99ad0
📒 Files selected for processing (1)
src/fls/oci/manifest.rs
| if m.layers.len() == 1 { | ||
| return Ok(&m.layers[0]); | ||
| } | ||
|
|
||
| // If artifactType is set, find the layer matching it | ||
| if let Some(ref artifact_type) = m.artifact_type { | ||
| if let Some(layer) = m.layers.iter().find(|l| l.media_type == *artifact_type) { | ||
| return Ok(layer); | ||
| } | ||
| } | ||
|
|
||
| // Fall back to the first disk image layer | ||
| if let Some(layer) = m | ||
| .layers | ||
| .iter() | ||
| .find(|l| l.media_type.starts_with("application/vnd.automotive.disk")) | ||
| { | ||
| return Ok(layer); | ||
| } |
There was a problem hiding this comment.
Validate all successful paths against the flashable media types.
Line 145 returns the only layer before any media-type check, and Lines 149-153 will also accept any artifact_type match. That still lets a non-disk blob through this path, which is exactly what this PR is trying to prevent.
Minimal fix
Manifest::Image(ref m) => {
+ let is_flashable = |layer: &Descriptor| {
+ layer.media_type.starts_with("application/vnd.automotive.disk")
+ };
+
if m.layers.is_empty() {
return Err("Manifest has no layers".to_string());
}
if m.layers.len() == 1 {
- return Ok(&m.layers[0]);
+ let layer = &m.layers[0];
+ return if is_flashable(layer) {
+ Ok(layer)
+ } else {
+ Err(format!("Layer media type '{}' is not flashable", layer.media_type))
+ };
}
// If artifactType is set, find the layer matching it
if let Some(ref artifact_type) = m.artifact_type {
- if let Some(layer) = m.layers.iter().find(|l| l.media_type == *artifact_type) {
+ if let Some(layer) = m.layers.iter().find(|l| {
+ l.media_type == *artifact_type && is_flashable(l)
+ }) {
return Ok(layer);
}
}
// Fall back to the first disk image layer
if let Some(layer) = m
.layers
.iter()
- .find(|l| l.media_type.starts_with("application/vnd.automotive.disk"))
+ .find(|l| is_flashable(l))
{
return Ok(layer);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fls/oci/manifest.rs` around lines 145 - 163, The early-return and
artifact_type match currently accept any layer regardless of media_type; update
the selection logic in manifest.rs so every successful path validates the
layer.media_type against the allowed flashable disk types (e.g., check
starts_with("application/vnd.automotive.disk") or an equivalent allowed list)
before returning; specifically, when m.layers.len() == 1 verify
m.layers[0].media_type is flashable, when matching via m.artifact_type ensure
the found layer also has a flashable media_type, and keep the existing fallback
that finds the first layer whose media_type starts_with the disk prefix.
There was a problem hiding this comment.
@mangelajo, thanks for the confirmation! The fix is straightforward — every successful return path in get_single_layer needs to validate the layer's media type against the flashable disk prefix. Here's the suggested update:
pub fn get_single_layer(&self) -> Result<&Descriptor, String> {
match self {
Manifest::Image(ref m) => {
let is_flashable = |layer: &Descriptor| {
layer.media_type.starts_with("application/vnd.automotive.disk")
};
if m.layers.is_empty() {
return Err("Manifest has no layers".to_string());
}
if m.layers.len() == 1 {
let layer = &m.layers[0];
return if is_flashable(layer) {
Ok(layer)
} else {
Err(format!(
"Layer media type '{}' is not a flashable disk image",
layer.media_type
))
};
}
// If artifactType is set, find a layer matching it that is also flashable
if let Some(ref artifact_type) = m.artifact_type {
if let Some(layer) = m.layers.iter().find(|l| {
l.media_type == *artifact_type && is_flashable(l)
}) {
return Ok(layer);
}
}
// Fall back to the first disk image layer
if let Some(layer) = m.layers.iter().find(|l| is_flashable(l)) {
return Ok(layer);
}
Err(format!(
"No disk image layer found among {} layers",
m.layers.len()
))
}
Manifest::Index(_) => Err(
"Cannot get layer from manifest index - need to resolve platform first".to_string(),
),
}
}Key changes vs the current PR:
- Single-layer path: now rejects the layer if its
media_typedoesn't start withapplication/vnd.automotive.disk. artifact_typepath: the closure additionally requiresis_flashable, so a matching artifact type alone is not enough.- Fallback path: already correct, just uses the shared
is_flashableclosure for consistency.
Would you like me to open this as a plan for code changes?
There was a problem hiding this comment.
Fixed in ab866a4. All three return paths in get_single_layer() now validate the media type using a shared is_flashable closure that checks for the application/vnd.automotive.disk prefix:
- Single layer path: now returns an error if the layer has a non-flashable media type
- artifact_type path: now requires both
media_type == artifact_typeANDis_flashable - Fallback path: uses the same
is_flashableclosure (functionally unchanged, just refactored for consistency)
Also added unit tests covering: single flashable layer acceptance, single non-disk layer rejection, artifact_type-based selection, and no-match error.
There was a problem hiding this comment.
Fixed — but with an important design adjustment: get_single_layer() is shared between the flash and extract paths. The extraction path (extract_files_from_oci_image_to_dir) legitimately needs to work with non-disk media types (e.g. tar+gzip layers), so adding flashable validation inside get_single_layer() broke the 5 oci_extract integration tests.
The fix instead:
- Keeps
get_single_layer()generic — single-layer manifests return the layer directly (callers decide what's valid) - Adds flashable validation in
flash_from_oci()— the flash-specific path now rejects layers that don't start withapplication/vnd.automotive.disk - Multi-layer selection still prefers
artifact_typematch, then falls back to the first disk image layer
Also fixed the &&layer clippy lint (needless borrow).
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| if let Some(layer) = m | ||
| .layers | ||
| .iter() | ||
| .find(|l| l.media_type.starts_with("application/vnd.automotive.disk")) |
There was a problem hiding this comment.
I would probably add some other more generic media types as well, so the oci artifacts could be reused in other areas. applcation/vnd.embedded.disk, or similar.
There was a problem hiding this comment.
@coderabbitai can you open a follow-up issue for this?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo jumpstarter-dev/fls \
--title "Support more generic media types for disk image layer selection" \
--body "## Follow-up: Support more generic media types for disk image layer selection
Currently, the fallback layer selection in \`get_single_layer()\` (in \`src/fls/oci/manifest.rs\`) only matches layers whose \`mediaType\` starts with \`application/vnd.automotive.disk\`.
To allow OCI artifacts to be reused in other areas (e.g. embedded systems beyond automotive), the selection logic should also consider more generic media types such as \`application/vnd.embedded.disk\` or similar conventions.
### Suggested improvement
Expand the media type matching in the fallback layer selection to include additional generic disk image media types, for example:
- \`application/vnd.embedded.disk\`
- Other relevant generic media types as appropriate
This would make \`fls\` more broadly applicable beyond automotive use cases.
---
Raised by `@mangelajo` in `#9`: https://github.com/jumpstarter-dev/fls/pull/9#discussion_r3043976233" \
--assignee mangelajo
Length of output: 48
@mangelajo, I've opened a follow-up issue for this: #12 – Support more generic media types for disk image layer selection.
It covers expanding the fallback layer selection in get_single_layer() to support more generic media types (e.g., application/vnd.embedded.disk) beyond the current application/vnd.automotive.disk prefix, making fls more broadly applicable beyond automotive use cases.
|
@ambient-code please handle existing comments. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fls/oci/manifest.rs (1)
340-365: Make theartifact_typetest proveartifact_typeprecedence.As written, this still passes if the
artifact_typebranch is removed, because the fallback path also returns the same layer. Use two flashable layers with different media types so the test verifies the explicitartifactTypematch wins, and add a regression case for a non-flashableartifactType.🧪 Suggested test shape
fn test_artifact_type_selection() { let json = r#"{ "schemaVersion": 2, - "artifactType": "application/vnd.automotive.disk.raw", + "artifactType": "application/vnd.automotive.disk.qcow2", "config": { "mediaType": "application/vnd.oci.image.config.v1+json", "digest": "sha256:config123", "size": 100 }, "layers": [ { - "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip", - "digest": "sha256:layer1", + "mediaType": "application/vnd.automotive.disk.raw", + "digest": "sha256:disk-raw", "size": 1000 }, { - "mediaType": "application/vnd.automotive.disk.raw", - "digest": "sha256:disk1", + "mediaType": "application/vnd.automotive.disk.qcow2", + "digest": "sha256:disk-qcow2", "size": 9999 } ] }"#; let manifest = Manifest::parse(json.as_bytes(), None).unwrap(); let layer = manifest.get_single_layer().unwrap(); - assert_eq!(layer.digest, "sha256:disk1"); + assert_eq!(layer.digest, "sha256:disk-qcow2"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/manifest.rs` around lines 340 - 365, The test test_artifact_type_selection currently doesn't prove artifactType precedence because the fallback path selects the same layer; update the test to include two flashable layers with distinct mediaTypes (e.g., one with "application/vnd.automotive.disk.raw" and another with a different flashable mediaType) and set artifactType in the manifest JSON to match only one of them so Manifest::parse(... ) and manifest.get_single_layer() must pick the layer whose mediaType equals artifactType; also add a regression case where artifactType is a non-flashable mediaType to assert that the fallback selection logic is used (or an error is returned) to ensure the artifact_type branch is actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fls/oci/manifest.rs`:
- Around line 340-365: The test test_artifact_type_selection currently doesn't
prove artifactType precedence because the fallback path selects the same layer;
update the test to include two flashable layers with distinct mediaTypes (e.g.,
one with "application/vnd.automotive.disk.raw" and another with a different
flashable mediaType) and set artifactType in the manifest JSON to match only one
of them so Manifest::parse(... ) and manifest.get_single_layer() must pick the
layer whose mediaType equals artifactType; also add a regression case where
artifactType is a non-flashable mediaType to assert that the fallback selection
logic is used (or an error is returned) to ensure the artifact_type branch is
actually exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef4d7957-3342-4ec6-ac48-75165781075e
📒 Files selected for processing (1)
src/fls/oci/manifest.rs
Address review feedback: add flashable media type validation (application/vnd.automotive.disk.*) in the flash_from_oci function, which is the correct place for flash-specific checks. Keep get_single_layer() generic since it is also used by the extraction path where any media type is valid. For multi-layer manifests, get_single_layer() prefers artifact_type matches and falls back to the first disk image layer. Add unit tests for: artifact_type selection, single layer (any type) returned, single flashable layer acceptance, and no-match error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ambient-code
bot
force-pushed
the
fix-layer-selection
branch
from
ab866a4 to
5dfbeb5
Compare
2 participants
To avoid flashing non disk layers we need to check the media type