Skip to content

[FIX] Add input modality validation - #21

Merged
pdeziel merged 1 commit into
mainfrom
sc-41423
Nov 3, 2026
Merged

pdeziel merged 1 commit into
mainfrom
sc-41423

Conversation

@pdeziel

@pdeziel pdeziel commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Scope of changes

This adds input modality checking for HTTP horizon routes to fix the bug where horizon was accepting attachments with file types unsupported by the configured task.

Fixes SC-41423

Estimated PR Size

  • Tiny
  • Small
  • Medium
  • Large
  • Huge

Acceptance criteria

[Describe how reviewers can test this change to be sure that it works correctly or copy the requirements from the Shortcut story. Add a checklist below if possible.]

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have added or updated the documentation (README.md, /docs/ directory, package docs, tool docs, etc.)
  • I have run go generate to update generated code (enums, OpenAPI docs, web static assets, etc.)
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)

Comment thread task/task.go
return err
}

// If the task has a modality then there must be an input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm considering it an error if the task has a modality but there's no input. However, if there's no configured modality then that should be valid right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that is right, if there is no modality then no input is expected, otherwise input is expected.

Comment thread handler.go
return
}

// Validate the task against the input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could do this check in a preprocessing step in the runner. However I thought it would be less clunky here since we can detect the error without instantiating the runner and return the 422 directly.

@chris-okuda chris-okuda Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yea that makes sense, we decode it then validate it before we do execution related stuff. We might want to include everything in the handler inside the execution timeout and check for the ctx.Err() between steps, though, in case we have some edge case that takes a long time to do a step like a DDOS style attack.

@pdeziel
pdeziel merged commit f559bb5 into main Nov 3, 2026
5 checks passed
@pdeziel
pdeziel deleted the sc-41423 branch November 3, 2026 14:01
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.

2 participants