fix(security): validate legacy share-token answers (bogus public scores) - #6
Merged
Merged
Conversation
decodeResultFromURL's legacy base64-JSON branch accepted `parsed.a` after
only checking it was a non-null, non-array object — it never validated the
answer VALUES, and it echoed the token's own `parsed.s` straight into the
score. So a crafted token bypassed the 0–4 invariant that parseAnswerDigits
enforces for the v1/v2 formats:
btoa('{"a":{"acq_channels":50}}') → valid, overall 83
btoa('{"a":{<all 15 ids>:99},"s":999}') → overall 2475/100
These render on the PUBLIC OG image, the page <title>, and shared dashboards
(buildShareModel → decodeShareToken → decodeResultFromURL → scoreDiagnostic),
so anyone could mint a share link showing an absurd score.
Add sanitizeLegacyAnswers(): every answer must be an integer 0–4 keyed by a
known AARRR question id; unknown keys are dropped and any out-of-range value
rejects the whole token (null → the generic, valid:false fallback model).
Recompute the score from the validated answers and stop trusting the token's
`s`. Well-formed legacy tokens still decode.
Adds 2 regression tests (out-of-range rejected; well-formed still decodes);
the existing null-answers regression test still passes.
Co-authored-by: mattia-mamini-gh <281593356+mattia-mamini-gh@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.
Why
decodeResultFromURL's legacy base64-JSON branch (pre-1.1 tokens) acceptedparsed.aafter only checking it was a non-null, non-array object. It never validated the answer values, and it echoed the token's ownparsed.sstraight into the score. So a crafted token bypassed the0–4invariant thatparseAnswerDigitsenforces for the v1/v2 formats:This path is reachable from the public surfaces:
buildShareModel → decodeShareToken → decodeResultFromURL → scoreDiagnostic. The bogus score renders in the public OG image, the page<title>, and shared dashboards — anyone could mint a share link advertising an absurd score.What
sanitizeLegacyAnswers(): every answer must be an integer 0–4 keyed by a known AARRR question id; unknown keys are dropped and any out-of-range value rejects the whole token (returnsnull→ the genericvalid:falsefallback model). This reuses the same invariant the v1/v2 paths already enforce.s.Verification
npm run typecheck✅ ·npm run lint✅ ·npm test→ 230 passed ✅overallScorefalls back to 0; well-formed legacy token still decodes 0–100). Existing null-answers regression still passes.