Better error messages#645
Closed
DemiMarie wants to merge 4 commits intoQubesOS:mainfrom
Closed
Conversation
Previously qubesd would just terminate the connection.
If the destination of an Admin API method must be dom0 but it is not, raise a useful exception instead of terminating the connection.
BackupCancelledError is the correct exception for this.
Contributor
ben-grande
reviewed
Feb 16, 2026
Contributor
ben-grande
left a comment
ben-grande
left a comment
There was a problem hiding this comment.
Demi, this revision is not for you, it is for me, to remember what to do.
|
|
||
| #: argument | ||
| self.arg = arg.decode("ascii") | ||
| self.arg = arg.decode("ascii", "strict") |
Contributor
There was a problem hiding this comment.
This is already the default:
Was this intended to be explicit, enforce against upstream change of defaults?
| """If the destination is not dom0, raise an exception.""" | ||
| name = self.dest.name | ||
| if name != "dom0": | ||
| raise DestinationNotDom0Error(name) |
Contributor
There was a problem hiding this comment.
I didn't make special exceptions for target error. It is using ProtocolError("Destination is not dom0") for example. I think it is fine. Same thing for the code above regarding prohibiting the use of argument.
| len(untrusted_payload), | ||
| ) | ||
| if self.transport is not None: | ||
| self.send_exception(err) |
| async def vm_volume_revert(self, untrusted_payload): | ||
| self.enforce(self.arg in self.dest.volumes.keys()) | ||
| self._check_volume() | ||
| self.enforce(_snapshot_re.match(untrusted_payload)) |
Contributor
There was a problem hiding this comment.
Copying the regex here so it makes it easier to read:
_snapshot_re = re.compile(rb"\A[A-Za-z0-9][0-9A-Za-z_.-]*\n?\Z")The revision is in the payload. Later on, it is checked if the untrusted_revision is in the snapshots. I believe the regex you set was to allow a sanitized exception message. Although I am not logging the revision name, the check for correct revision in safe set was added:
self.enforce_arg(
wants=self.dest.volumes.keys(),
short_reason=self.EXC_ARG_NOT_IN_DEST_VOLUMES,
)
untrusted_revision = untrusted_payload.decode("ascii").strip()
del untrusted_payload
allowed_chars = string.ascii_letters + string.digits + "-_."
self.enforce(
all(c in allowed_chars for c in untrusted_revision),
reason="Revision name must be in safe set: " + allowed_chars,
)
revision = untrusted_revision
del untrusted_revision| "volatile", | ||
| "kernel", | ||
| ]: | ||
| raise qubes.exc.UnknownVolumeError("<untrusted>") |
Contributor
There was a problem hiding this comment.
This is also done, but specifying the volumes that it is not, rather than what volume it is.
| mode = allowed_values[untrusted_payload] | ||
| except KeyError: | ||
| raise qubes.exc.PermissionDenied() | ||
| raise qubes.exc.QubesAttachmentKindError("invalid attachment kind") |
Contributor
There was a problem hiding this comment.
Changed to ProtocolError("Invalid assignment mode"). Wouldn't hurt to add an specific exception. TODO.
Not related to this PR, but the docstring does not match what allowed_values expect of untrusted_payload.
| raise qubes.exc.PermissionDenied( | ||
| "Backup profile {} does not exist".format(self.arg) | ||
| ) | ||
| raise BackupProfileNotFoundError(self.arg) |
Contributor
There was a problem hiding this comment.
Wouldn't hurt, good idea, TODO.
| raise qubes.exc.PermissionDenied( | ||
| "Backup profile {} does not exist".format(self.arg) | ||
| ) | ||
| raise BackupProfileNotFoundError(self.arg) |
| snapshots = volume.revisions | ||
| self.enforce(untrusted_revision in snapshots) | ||
| if untrusted_revision not in snapshots: | ||
| raise qubes.exc.QubesNoSuchSnapshotError(untrusted_revision) |
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Feb 17, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Feb 17, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 9, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 10, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 11, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 11, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 11, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 11, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 11, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 11, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 12, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 12, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 12, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Mar 29, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Apr 18, 2026
ben-grande
added a commit
to ben-grande/qubes-core-admin
that referenced
this pull request
Apr 30, 2026
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
2 participants
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.
Use better exceptions for various client errors.
This is DRAFT and not ready yet for many reasons.