Skip to content

Import failure text reaches the History API unfiltered #975

Description

@m4bard

Filed from the batch G review of PRs 916 and 930. Pre-existing on canary a630572e;
both of those PRs widen it, neither creates it.

What happens

A failed file import writes its message straight into a History row, and History rows are
returned whole:

  • listenarr.infrastructure/Downloads/Processing/DownloadProcessingJobProcessor.cs:346
    sets Message = result.Message ?? $"{result.Action} completed".
  • DownloadProcessingJobProcessor.History.cs:65-76 puts result.Message, result.SourcePath
    and result.FinalPath into details["FailedResults"], serialized into History.Data.
  • listenarr.api/Features/ActivityHistory/HistoryController.cs returns the entities with
    Ok(history) and Ok(new { entry, related }), so History.Message and History.Data
    are response fields.
    ImportResult.ImportFailure builds its message as
    $"Unable to perform {action} on {sourcePath} to {finalPath}", so the absolute source and
    destination paths are already in that response today. ImportResult.Exception passes the
    caught exception's message through, and an IOException from the file layer normally quotes
    the path again and adds the platform error text.
    Two adjacent log sites interpolate the same path into the message template rather than
    passing it as a structured argument, so neither is sanitized and neither can be filtered by
    field:
  • listenarr.application/Downloads/Import/DownloadImportService.cs:256
  • listenarr.application/Downloads/Import/DownloadImportService.cs:483
    Everywhere else in the same area the repository already routes this text through
    LogRedaction.SanitizeFilePath and LogRedaction.SanitizeText, including FileMover's own
    LogMutation and DownloadImportService.DirectoryOwnership.cs:277-280. These call sites are
    the exception rather than the policy.

Why it matters

The History API is how the web UI draws the activity list, so this text is rendered to anyone
who can reach the instance. On a shared or reverse-proxied deployment the response describes
the host's directory layout, and on Windows the paths usually contain the account name.

How Readarr handles the same split

Readarr keeps the two audiences apart deliberately. ImportApprovedBooks.cs:271-306 logs the
exception object, so the log keeps everything, and then records a fixed classified sentence in
the user-visible result: "Failed to import book, permissions error", "Failed to import book,
root folder missing", "Failed to import book, destination already exists". The raw exception
text never reaches ImportResult.Errors (ImportResult.cs:30-37).

Suggested change

  1. Classify at the boundary. Map the failure to a short stable sentence for History.Message,
    the way Readarr does, and keep the exception object for the log.
  2. If the raw text is kept, sanitize it on the way into the row and cap its length, matching
    what LogRedaction.SanitizeText already does for logs.
  3. Independently, fix the two interpolated templates at DownloadImportService.cs:256 and
    :483 to pass the path as a structured argument through LogRedaction.SanitizeFilePath.
    Item 3 is small and self-contained and could land on its own.
    Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions