fix(certificates): view in-app instead of forcing a background download - #215
Merged
Conversation
Reported from the Android app (all vendors): "certificates are now viewable
but user is redirected to browser first and the certificate is downloading in
the background. Expected: the certificates should be accessible directly."
Two independent causes, and fixing either alone leaves the report standing:
1. The presigned R2 url signed `Content-Disposition: attachment`, so whatever
opened it saved the file instead of rendering it. The bot's presigner
supported attachment and NOTHING else — buildPresignOverrides returned {}
for any other disposition, so there was no way to ask for a viewable url,
even though the dashboard's presigner has defaulted to inline all along.
Certificates were the one artefact on the wrong side of that split, which
is why they were the one artefact that downloaded.
2. The portal anchor carried target="_blank". Inside the Capacitor WebView
that hands the url to external Chrome, so the teacher left the app before
Content-Disposition was ever read.
The teacher now gets two controls per certificate: View (inline, navigates in
place) and Download (attachment, unchanged). Both are kept because a
certificate really is something teachers save and print — and leaving that to
the PDF viewer's own save button would bet on the Android WebView PDF toolbar
being present and findable, which varies by Android version.
`inline` deliberately asserts a Content-Type and emits nothing for an
extension it cannot type: inline + application/octet-stream downloads anyway,
so a lone inline header would look like a fix and change nothing.
WhatsApp delivery is untouched — it passes no disposition and so takes the
unchanged `attachment` default. One route with `?view=1` rather than two
routes, so the auth check, ownership filter, mint and 404/502 split cannot
drift apart.
Tests: red-first, and the target="_blank" guard was mutation-checked by
reintroducing the attribute to confirm it actually fails. 10 certificate
suites / 123 tests green; full-suite failures unchanged from the develop
baseline (which is red for unrelated pre-existing reasons).
Closes: bd-2676
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.
What was reported
ICT priority sheet, row 9 — Medium: App, all vendors:
Follow-up to the earlier "error viewing certificates on app" row: the certificate now loads, but the app hands it off to an external browser and it downloads instead of rendering.
Root cause — two of them
The report names two symptoms in one sentence, and they have two different causes. Fixing either alone leaves the other standing.
1.
Content-Disposition: attachmentwas signed into the presigned URL.certificate-pdf.service.jsasked forattachment, deliberately ("a certificate is a file a teacher saves and prints"). The bot's presigner supportedattachmentand nothing else —buildPresignOverridesreturned{}for any other value, so there was no way to ask for a viewable URL.Notably the dashboard has a second presigner that defaults to
inline, which is why videos and reading assessments render in place. Certificates were the one artefact routed through the attachment-only presigner — the whole reason they were the one artefact that downloaded.2.
target="_blank"on the portal anchor.Inside the Capacitor WebView that's a hand-off to external Chrome. The teacher leaves the app before
Content-Dispositionis even read.What changed
Two controls per certificate instead of one:
inline(new)_blankattachment(unchanged)_blank, as beforeBoth are kept on purpose. A certificate genuinely is something teachers save and print, which is what the original
attachmentdecision was protecting. Relying on the PDF viewer's own save button would bet on the Android WebView PDF toolbar being present and discoverable — that varies by Android version and WebView build and isn't something this codebase can assert. The explicit Download button reuses the attachment path already live in production.inlinemust assert a Content-Type to do anything:inline+application/octet-streamdownloads anyway. So an extension it cannot type gets no override at all rather than a lone inline header that looks like a fix and changes nothing.One route with
?view=1rather than two routes — the auth check, ownership filter, mint, and 404/502 split are identical, and duplicating them is how two paths drift into disagreeing about who may read a file.Blast radius
deliverCertificateByCodepasses no disposition, so it takes the unchangedattachmentdefault. It also never hands out a presigned URL (server-side fetch + re-upload to Meta), so disposition is irrelevant there.?view=1→attachment, exactly as before.fetchOrMintCertificatePdf; updated.bad_request) rather than silently forwarding a value the presigner would ignore.Tests
Red-first. The
target="_blank"guard was mutation-checked — the attribute was reintroduced to confirm the assertion actually fails, then reverted. (My first version of that assertion passed for the wrong reason: its source window caught the neighbouring comment, which quotestarget="_blank"while explaining its absence.)tests/training/bd-2676-certificate-inline-view.test.js— presigner inline mode + service disposition threadingtests/portal/bd-2676-certificate-view-and-download.test.js— UI contract + route contracttests/training/portal-certificates.test.js— updated for the new 3rd arg; added inline + auth-on-view cases10 certificate suites / 123 tests green. Full-suite failures are unchanged from the
developbaseline (68 suites, red for unrelated pre-existing reasons — missingexpressin a fresh root install, the 756-violation ticket-ref hygiene guard, etc.). Verified by stash-and-measure rather than assumption.The causal chain is read off the code, not watched on an Android build — and per bd-2675 NIETE staging has no dashboard/portal-API service, so the portal half may not be QA-able on staging as things stand.
What QA must confirm on the app: tapping View renders the PDF inside the app (no jump to Chrome, nothing landing in Downloads), and Download still saves.
Closes: bd-2676
🤖 Generated with Claude Code