fix: surface 403 instead of 404 on download permission errors#5
Merged
Conversation
The download endpoint now looks up the artefact unfiltered and lets assertDownloadAccess decide between 200 and 403. Listing still hides private artefacts behind 404, but on /download the caller already knows the id — surfacing 403 makes the permission error actionable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
GET /:id/downloadused to apply the listing access filter before lookup, whichmade every "not allowed" response indistinguishable from "doesn't exist" (404).
Look the artefact up unfiltered and let
assertDownloadAccessdecide between200 and 403 instead.
Why: callers of
/downloadalready know the id — a 404 there is unhelpfuland made client-side error handling guess at the cause.
Regression risks:
plugin registry but a deliberate trade-off worth noting.
404from/:id/downloadwill now see403. Internallib-node client only checks 200/304 so it's unaffected; existing read-keys
test that relied on the old 404 is updated in this PR.
404(artefact has no uploadedpath) is preserved —it still runs after
assertDownloadAccess, so a non-authorized caller gets403, not 404, even when the artefact has no content.