Soft validating image upload

[benjaminleonard]

This PR contains some small validations that help nudge the user if they're uploading an image unlikely to boot.

We're checking for:

1. The absence of CD001 at 0x8001(bootable CD), or a UEFI header (EFI PART) at offsets 512, 2048, 4096 are a strong indicator it's not bootable
2. File extensions that imply a compressed or unsuitable image, e.g. qcow2, vmdk or ones that end in gz or 7z
3. That the selected block size is the correct one for bootable CDs or the one that matches the location of the UEFI header

Feedback wanted especially on whether:
The UX validates softly enough so that we're not adding friction on users who have an image that would boot but doesn't meet what we expect, and for users that may be uploading an image that does not need to boot at all.

Note that this does not prevent form submission, it's merely a notice.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	May 20, 2024 4:18pm

[david-crespo]

Weird flash was indeed fixed by our shuffling.

[benjaminleonard]

I think this is about ready to go – and fairly low risk since it does not prevent the user from uploading an image even if it doesn't think its bootable. @iliana have you got a link to a bootable CD image I could tst?

[david-crespo]

I think this all looks good, my main concern was getting the contents of a list like this right. @iliana ?

[iliana]

have you got a link to a bootable CD image I could tst?

well the ISO I downloaded to learn that we don't need a GPT was the Windows 10 retail ISO, so you could grab that off of Microsoft's site. although it's 6+ GiB :( but all the other CD images I'm aware of are hybrid ISO+GPT

I have been meaning to craft some small testing images for Propolis testing, might be worth having those for playwright tests here?

[david-crespo]

Might be simpler to pass in more elaborate fake data here for buffer:

console/test/e2e/utils.ts

Lines 178 to 189 in 9b20b7c

	export async function chooseFile(page: Page, inputLocator: Locator, size = 15 * MiB) {
	const fileChooserPromise = page.waitForEvent('filechooser')
	await inputLocator.click()
	const fileChooser = await fileChooserPromise
	await fileChooser.setFiles({
	name: 'my-image.iso',
	mimeType: 'application/octet-stream',
	// fill with nonzero content, otherwise we'll skip the whole thing, which
	// makes the test too fast for playwright to catch anything
	buffer: Buffer.alloc(size, 'a'),
	})
	}

On the other hand, it's kind of question-begging when you craft the perfect fake data to fit your logic, so it might be better to use test images that were not designed specifically for this.

[iliana]

So this doesn't quite implement the decision tree of "if no EFI PART found, look for CD001". This is likely problematic in terms of what we tell customers for bootable hybrid GPT+ISO, which a lot of Linux distro ISOs are so that you can dd them directly to a flash drive (512-byte blocks!) and have them still boot. In this particular case, I think it's better to prefer the GPT and tell the user they should use 512-byte blocks.

My expectation is that one of these would return {efiPartOffset: 512, isBootableCd: true, ...} — would that always show a "this image might not be bootable" message of one kind or another?

[iliana]

it might be better to use test images that were not designed specifically for this

except for the fact that those are giant because they're operating systems :(

[iliana]

Oh, I just read this line. That should cover my concern.

However I think if a user has 4096 selected and uploads a hybrid GPT+MBR it will suggest they use 2048, when they should probably follow the GPT suggestion instead.

[david-crespo]

It would probably help to move this logic over into validateImage so you can see everything up front. When you say "follow the GPT suggestion", you mean 4096 is correct and we should avoid warning if possible? How can we detect hybrid GPT+MBR?

[iliana]

oh sorry I meant hybrid GPT+ISO above. (hybrid GPT+MBR is a whole 'nother rabbit hole to fall down that we should ignore for this PR, and probably forever)

[iliana]

What I mean is if a user uploads a hybrid GPT+ISO, and the GPT is at offset 512 (i.e., {efiPartOffset: 512, isBootableCd: true, ...}, then the suggested block device size should be 512. If the user has 2048 selected, we maybe don't bother displaying a suggestion, because it might still boot?, but if the user has 4096 selected, that definitely won't boot and we should suggest 512.

[benjaminleonard]

@iliana you had mentioned in chat you might write a separate library to check if an image is bootable, want to hold this PR for that?

[iliana]

I want to verify that we aren't suggesting something weird with hybrid GPT+ISO, but other than that we should merge this. I'll either push a commit tweaking the behavior or report back that it's fine.

[iliana]

Tested my commit with:

• Alpine nocloud qcow2: BootableNotice displays as expected
• Alpine nocloud raw: Only 512 is correct, other options display "EFI PART" message
• Alpine ISO (hybrid GPT+ISO): Only 512 is correct, other options display "EFI PART" message
• A random Windows Server ISO I partially downloaded from archive.org: Only 2048 is correct, other options display "Bootable CDs" message

[david-crespo]

Awesome, thank you!

[benjaminleonard]

Lovely stuff! Thanks for humouring me @iliana