[fix][reports] build a report from declared fields, and derive the rest at send time (24.05) - #7918
Open
ar2rsawseen wants to merge 3 commits into
Open
[fix][reports] build a report from declared fields, and derive the rest at send time (24.05)#7918ar2rsawseen wants to merge 3 commits into
ar2rsawseen wants to merge 3 commits into
Conversation
…st at send time /i/reports/create stored the submitted args object as the report document, and /i/reports/update passed it as $set. Only report_type "core" got an app authorization check, so "dashboards" skipped it, and nothing anywhere limited which fields a request could set. That matters because a report document is not only configuration. reports.send prefers report.messages[i].html over rendering the trusted template, and with sendPdf that string is what the pdf renderer opens. A field nobody meant to be writable was therefore renderer input. Three changes, independent of each other: - create and update build the document from REPORT_FIELDS, taken from what the drawer binds plus the metrics map it assembles on submit. _id and user fall out of the list rather than needing their own delete. - getReport strips the fields the generator derives, so a document stored before the allow-list existed cannot supply them either. It goes there rather than in loadReport because /o/reports/preview and /o/reports/pdf run their own findOne and call getReport directly. - the pdf launch no longer passes --disable-web-security. The dashboard template loads its assets from the host it is given, which is same origin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gins Rendering happens on the server, so every request the document makes is made from the server's network position: loopback, the private network, a cloud metadata service. Nothing bounded that. renderPDF now intercepts requests and refuses any origin outside an allow-list: the configured Countly origin, plus whatever the caller passes. data:, blob: and about: pass through, since they carry no network request. Both report call sites pass the origins their templates actually load images from. Origins are compared as scheme, host and port together. Host alone would be too loose, because Countly is often on loopback itself and every other service on that interface would come with it. --disable-web-security is deliberately kept, reversing an earlier commit on this branch that removed it. Measured against real Chromium: the html is opened as a data: url, whose origin is opaque, and Chromium refuses http subresources from an opaque origin, so without the flag the template's own images do not load at all. With a single own-origin image, one request is served with the flag and none without it. What made the flag dangerous was that any request could be made in the first place, which the allow-list now settles. Verified before and after with two loopback listeners standing in for an internal service. Before: the foreign origin is served. After: refused, and the report's own image is still served. web.host and web.port come from the frontend config rather than the api one, which is where they are defined, guarded so pdf.js still loads where that file is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d a TypeError The finally block closed the browser unconditionally, so when puppeteer.launch() threw, browser was still undefined and the close raised "Cannot read properties of undefined (reading 'close')". That replaced the real reason on the way out, and the real reason was logged at debug, so nothing usable reached the log either. Guard the close and log the failure as an error. A missing Chrome now says so. Verified both ways: a normal render still writes its pdf and fires its callback, and a launch that cannot find Chrome logs that and no longer throws the TypeError. Co-Authored-By: Claude Opus 5 <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
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 wrong
/i/reports/createstored the submittedargsobject as the report document, and/i/reports/updatepassed the same object as$set. Onlyreport_type === "core"reached an app authorization check, so"dashboards"walked past it, and nothing limited which fields a request could set.A report document is not only configuration.
reports.sendreadsreport.messages[i].htmland makes the trusted template a fallback:With
sendPdf, that string is whatpdf.renderPDFopens, andapi/utils/pdf.jsnavigates to it as adata:document withwaitUntil: 'networkidle0'andsetBypassCSP(true). So a field nobody intended to be writable was renderer input, and the render happens on the server with its network reach.The dashboards report builder never clears it either:
plugins/dashboards/api/api.jssetsreport.subjectandreport.dataand leavesmessagesalone.Three changes, each sufficient on its own
1. Create and update build the document from a declared list.
REPORT_FIELDSis what the drawer binds (title,report_type,apps,dashboards,date_range,emails,frequency,day,hour,minute,timezone,sendPdf,selectedEvents) plus themetricsmaponSubmitassembles._idanduserfall out of the list rather than needing their owndelete.The direction is deliberate. Removing known-dangerous keys leaves every field added later writable until someone remembers to deny it, and the cost of forgetting is unbounded because these values are consumed, not just stored. Declaring what the drawer sends fails the other way: the cost of forgetting is a setting that stops saving, which someone notices and files.
2.
getReportstrips what the generator derives.messages,data,subject,mailTemplate,properties,period,start,end,date,total_new,universeare all assigned at send time; none needs to survive a database round trip. This closes documents stored before the allow-list existed.It goes in
getReportrather thanloadReportbecause/o/reports/previewand/o/reports/pdfrun their ownfindOneand callgetReportdirectly. That is the one point all three paths share.3. The renderer may only fetch from Countly's own origins.
renderPDFintercepts requests and refuses any origin outside an allow-list: the configured Countly origin plus whatever the caller passes.data:,blob:andabout:pass through, carrying no network request. Both report call sites pass the origins their templates load images from. Origins compare scheme, host and port together, since host alone would be too loose when Countly itself sits on loopback.web.hostandweb.portare read from the frontend config rather than the api one, which is where they are actually defined, guarded sopdf.jsstill loads where that file is absent.--disable-web-security stays, and that is a reversal
An earlier commit on this branch removed it. Measured against real Chromium, that was wrong: the html is opened as a
data:url, whose origin is opaque, and Chromium refuses http subresources from an opaque origin. With a single own-origin image, one request is served with the flag and none without it, so removing it would have shipped dashboard PDFs with every image missing.What made the flag dangerous was that any request could be made at all. The allow-list settles that, and with it the flag can only relax checks for Countly's own origin.
Verified against the unpatched file
test/unit-tests/plugins.reports.public-fields.js(plugins/reports/tests/public-fields.unit.json platform), 3 cases: every derived field is stripped before the generator sees the report, the report's own configuration survives untouched, and noargsarray asks for--disable-web-security.Against these branches all 3 pass. Against the unpatched file 2 of 3 fail, the exception being the case that asserts configuration is preserved, which should pass either way.
Verified against real Chromium
Two loopback listeners, one on the port the config calls Countly's own and one on another port standing in for an internal service, with a document requesting an image from each.