Skip to content

BCH-1150: Validate evidence file type against evidence type on backend#408

Merged
saltpy-cs merged 2 commits into
mainfrom
fix/BCH-1150
May 28, 2026
Merged

BCH-1150: Validate evidence file type against evidence type on backend#408
saltpy-cs merged 2 commits into
mainfrom
fix/BCH-1150

Conversation

@saltpy-cs
Copy link
Copy Markdown
Contributor

@saltpy-cs saltpy-cs commented May 28, 2026

Reject screenshot evidence submissions where the file media type is not an image (png/jpg/jpeg/gif/webp). Adds screenshotAllowedMediaTypes and validates in validateEvidenceRequirements before checking required types.

Summary by CodeRabbit

  • Bug Fixes

    • Screenshot evidence submissions now require valid image formats (PNG, JPEG, GIF, WebP); media type checks are case-insensitive and ignore parameters (e.g., image/png; charset=binary); incompatible media types are rejected.
    • Evidence validation tightened to enforce correct file types.
  • Tests

    • Added tests covering screenshot media-type rules and broader document evidence media-type acceptance (PDF, Word, images).

Review Change Stack

Reject screenshot evidence submissions where the file media type is not
an image (png/jpg/jpeg/gif/webp). Adds screenshotAllowedMediaTypes and
validates in validateEvidenceRequirements before checking required types.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4c0e2e03-033d-4d1d-a524-2168a091c019

📥 Commits

Reviewing files that changed from the base of the PR and between 621a665 and c26f95f.

📒 Files selected for processing (2)
  • internal/workflow/step_transition.go
  • internal/workflow/step_transition_test.go

📝 Walkthrough

Walkthrough

This PR adds media-type validation for screenshot evidence submissions in workflow step transitions. An allowlist of image MIME types is defined, and the evidence validation logic is extended to reject screenshots with incompatible media types. Test coverage validates that screenshots accept only image types while document evidence accepts a broader set.

Changes

Screenshot Evidence Media Type Validation

Layer / File(s) Summary
Media type allowlist and validation logic
internal/workflow/step_transition.go, internal/workflow/step_transition_test.go
screenshotAllowedMediaTypes map defines acceptable image types for screenshot evidence (image/png, image/jpeg, image/jpg, image/gif, image/webp). validateEvidenceRequirements enforces that screenshot evidence uses only allowed media types, rejecting incompatible formats. Test coverage (BCH-1150) verifies screenshots reject non-image types like application/pdf, accept common image formats, handle case-insensitive and parameterized media types, and document evidence accepts PDF, Word, and image types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

📸 A rabbit hops through evidence with care,
Normalizes types, strips parameters fair,
PNG, JPEG, WebP in view,
Rejecting PDFs—no faux pas to rue,
Hooray for clean screenshots! 🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'BCH-1150: Validate evidence file type against evidence type on backend' clearly and specifically describes the main change: adding validation for evidence file types on the backend.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/workflow/step_transition_test.go`:
- Around line 304-309: The test loop in step_transition_test.go is missing the
"image/jpg" media type and thus is out of sync with the production allowlist
used in validateEvidenceRequirements; update the slice in the table-driven loop
that calls svc.validateEvidenceRequirements (which uses EvidenceSubmission) to
include "image/jpg" alongside "image/png", "image/jpeg", "image/gif", and
"image/webp" so the test covers all allowed image types.

In `@internal/workflow/step_transition.go`:
- Around line 304-306: The allowlist lookup for screenshot MIME types rejects
valid variants because evidence.MediaType is matched verbatim; normalize it
before checking by trimming whitespace, splitting off any parameters at the
first ';', and lowercasing the base type (e.g., use strings.TrimSpace,
strings.SplitN(media, ";", 2)[0], strings.ToLower). Replace the direct lookup of
evidence.MediaType against screenshotAllowedMediaTypes in the block that checks
evidence.EvidenceType == "screenshot" with the normalizedMedia value so
screenshotAllowedMediaTypes and the code that references evidence.MediaType use
the normalized base media type.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: aee22940-f085-429e-9b34-dec6dceffe38

📥 Commits

Reviewing files that changed from the base of the PR and between 8e1d435 and 621a665.

📒 Files selected for processing (2)
  • internal/workflow/step_transition.go
  • internal/workflow/step_transition_test.go

Comment thread internal/workflow/step_transition_test.go Outdated
Comment thread internal/workflow/step_transition.go
… test coverage

Add image/jpg to acceptance test loop to match the allowlist. Normalize
media type (lowercase, strip parameters) before allowlist lookup so that
values like Image/PNG or image/png; charset=binary are accepted.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@saltpy-cs saltpy-cs merged commit 89e68ad into main May 28, 2026
5 checks passed
@saltpy-cs saltpy-cs deleted the fix/BCH-1150 branch May 28, 2026 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant