Skip to content

fix(tasks): return 403 instead of 500 for unauthorised submission access - #111

Closed
jmirchh75 wants to merge 1517 commits into
thoth-tech:developmentfrom
ontrack-features-t2-2026:fix/submission-access-403
Closed

fix(tasks): return 403 instead of 500 for unauthorised submission access#111
jmirchh75 wants to merge 1517 commits into
thoth-tech:developmentfrom
ontrack-features-t2-2026:fix/submission-access-403

Conversation

@jmirchh75

Copy link
Copy Markdown

Description

submission_details and submission_files in tasks_api.rb were correctly denying access to unauthorised callers (the authorise? check itself was working), but the denial was returned as 500 Internal Server Error instead of 403 Forbidden. This is because error!() was called without an explicit status code, so Grape fell back to its default of 500.

This is not a security vulnerability — no unauthorised caller ever received submission data or files. It's a correctness issue: a 500 looks like a server crash to monitoring/alerting tools and to any client code that checks specifically for 403 to distinguish "not allowed" from "something broke."

Fix: added the explicit 403 status to both error!() calls. No other logic changed.

Fixes # (no tracked issue — found while adding test coverage for these endpoints)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added test/api/submission_access_test.rb — 14 new tests covering both endpoints:

  • Owning student, unit convenor, and unit tutor can access (200)
  • A different student in the same unit is blocked (403)
  • Staff from a different unit are blocked (403)
  • Unauthenticated requests are blocked (419)
  • A task definition from another unit returns 404
  • Staff-only fields (e.g. claimed_by_unit_role_id) aren't exposed to students
  • A blocked caller's response never contains the owning student's filename/username

Verified before/after:

  • Before the fix: ran the new test suite against the unmodified code — 5 tests failed, all Expected: 403, Actual: 500:
image
  • After the fix: all 14 tests pass.
image

To reproduce: bundle exec rails test test/api/submission_access_test.rb

  • Test 1: Full access-control matrix for submission_details
  • Test 2: Full access-control matrix for submission_files

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if appropriate
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have created or extended unit tests to address my new additions
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

If you have any questions, please contact @macite or @jakerenzella.

maplefoxgit and others added 30 commits August 22, 2026 14:59
Simplified condition for triggering Teams notifications.
…n-click-contract

feat(notifications): add safe push notification click contract
Bumps [ruby-lsp](https://github.com/Shopify/ruby-lsp) from 0.23.13 to 0.26.9.
- [Release notes](https://github.com/Shopify/ruby-lsp/releases)
- [Commits](Shopify/ruby-lsp@v0.23.13...v0.26.9)

---
updated-dependencies:
- dependency-name: ruby-lsp
  dependency-version: 0.26.9
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
…er/ruby-lsp-0.26.9

chore(deps-dev): bump ruby-lsp from 0.23.13 to 0.26.9
…urce-map

docs: publish PPI backend data-source and field-ownership map
…g-guide

docs(notifications): add the notifications contribution guide
maplefoxgit and others added 27 commits August 28, 2026 19:58
…s-status-authorisation

fix(scorm): authorise the success status wherever it is written
…sition-403

fix(tasks): return 403 when a status transition is refused
…okup-on-asserted-identity

fix(auth): match a federated assertion only on what it asserts
…en-to-subject

fix(lti): bind an enrolment token to its subject and make it single use
…n-set-per-recipient-delivery-errors

fix(communications): keep sending when one recipient's delivery fails
…ification-route-20260828

fix(notifications): open feedback alerts in the feedback pane
…parallel-test-workers-20260828

fix(security): resolve CodeQL alerts and parallelize API tests
@jmirchh75 jmirchh75 closed this Aug 30, 2026
@jmirchh75 jmirchh75 reopened this Aug 30, 2026
@jmirchh75 jmirchh75 closed this Aug 30, 2026
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.

4 participants