fix(core): validate and 404 before touching edit_marker / edit_object payloads#916
Open
SAY-5 wants to merge 1 commit intomemeLab:developfrom
Open
fix(core): validate and 404 before touching edit_marker / edit_object payloads#916SAY-5 wants to merge 1 commit intomemeLab:developfrom
SAY-5 wants to merge 1 commit intomemeLab:developfrom
Conversation
… payloads
Both edit views did roughly:
index = request.GET.get("id", "-1")
model = Marker.objects.get(id=index)
model_data = { ... model.source ... }
if not model or model.owner != Profile.objects.get(user=request.user):
raise Http404
Three things are wrong with this ordering:
1. `Marker.objects.get(id=index)` raises DoesNotExist when the id is
unknown (and ValueError / DataError when index is non-numeric junk
like the default "-1" fed to PostgreSQL). Both surface as 500.
2. `model_data = { ... }` runs before the permission check, so we
read source/created/author/patt/title off the row even when the
requesting user has no right to that object. Today the leak is
narrow (nothing is returned to the response until after the
Http404), but it's exactly the kind of ordering that bites the
next refactor (e.g. adding logging or serialisation above the
check).
3. `if not model` is dead code: `.get()` either returns an instance
or raises. The guard never fires.
Coerce index to int first, fetch via get_object_or_404, reject with
Http404 when owner != request.user.profile, and only then build the
model_data dict. Same pattern is used by create_or_edit_exhibit /
edit_artwork in this module, so the two views now match the rest.
Fixes memeLab#846
Signed-off-by: SAY-5 <SAY-5@users.noreply.github.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
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.
What
Both
edit_markerandedit_objectinsrc/core/views/views.pyhad the same bug-shaped logic:Three things are wrong:
Marker.objects.get(id=index)raisesDoesNotExistfor unknown ids andValueError/DataErrorfor non-numeric values. The default"-1"falls back to a DB query that returns nothing and again raisesDoesNotExist. All three surface as HTTP 500.model_databuilt before the permission check. The dict is filled from the row's fields (source,created,author,patt,title) before we know if the requester owns the row. Today the data never escapes becauseHttp404is raised two lines later, but the ordering is exactly the kind that breaks the moment someone adds logging, a metric, or serialisation above the guard.if not modelis dead code:.get()either returns an instance or raises, so the sentinel branch can never fire.Fix
indextointfirst,Http404on failure.get_object_or_404so unknown id → 404.owner != request.user.profilecheck before buildingmodel_data.if not modelbranch.edit_objectgets the same treatment (same bug in the same shape).Matches the pattern already used by
create_or_edit_exhibitandedit_artworkfurther down this file.Fixes #846