fix(predictions): sanitize note content on write (closes #1624) - #1789
Open
bilhokista wants to merge 3 commits into
Open
fix(predictions): sanitize note content on write (closes #1624)#1789bilhokista wants to merge 3 commits into
bilhokista wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Closes #1624.
One correction to the issue
The length limit already exists.
UpdatePredictionNoteDtocarries@MaxLength(1000), so notes are already bounded — the storage-bloat half of the issue is covered onmaintoday.What was genuinely missing is sanitization.
PredictionsService.updateNotedidprediction.note = dto.notewith no filtering, so markup went straight to thetextcolumn.The issue also points at
submit-prediction.dto.ts, butSubmitPredictionDtohas nonotefield — notes are set through the separatePATCH :id/noteroute andUpdatePredictionNoteDto. This PR touches that path instead.Why sanitize in the service, not with
@TransformA
@Transformdecorator on the DTO would be the tidier-looking place, but it only runs when the route'sValidationPipehastransform: true. The note route uses a bare@Body() dto: UpdatePredictionNoteDtowith no@UsePipes, unlikeSearchController, which setstransform: trueexplicitly per route.So a decorator here would silently do nothing — the worst kind of security fix, since it reads as protection while providing none. Sanitizing at the write makes it independent of pipe configuration.
Why strip rather than escape
Notes are personal free text and are never rendered as markup, so tags carry no meaning. Stripping keeps the stored value equal to what the user meant to write, and leaves nothing for a downstream consumer to mis-render. Escaping would preserve
<script>in the note, which is safe but is not what anyone typed.Script and style bodies are removed wholesale rather than just their tags — removing only the tags would leave the code sitting in the note as plain text.
Changes
dto/update-prediction-note.dto.ts— exportedsanitizeNote(), following the shape ofescapeLikeWildcards()insearch/dto/search-query.dto.ts.predictions.service.ts—updateNotesanitizes before assigning.predictions.service.spec.ts— six table-driven cases added to the existingupdateNoteblock.Tests
The last case matters as much as the first: it asserts ordinary notes pass through untouched, so the sanitizer cannot quietly start eating legitimate text.
Assertions are on what reaches
predictionsRepository.save, not on the return value, since the stored content is what the issue is about.I also verified
'unclosed <div oops'collapses to'unclosed'— the trailing-?in the tag pattern covers unterminated tags, which is the usual way a naive strip gets bypassed.Verification
All three files parse clean via the TypeScript compiler API, and
sanitizeNotewas exercised against the seven cases above in isolation. I did not run the Jest suite — I worked from the individual files via the API rather than a full clone, so please treat CI as the authority.Behaviour for a note containing no markup is byte-identical to before, apart from trimming surrounding whitespace.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CrfEY1tvXrbeMDAUzxfuk7