Skip to content

Harden App Viewer metadata generation - #797

Merged
tavdog merged 2 commits into
tronbyt:app-viewer-sourcefrom
saltedlolly:fix/app-viewer-hardening
Oct 4, 2026
Merged

tavdog merged 2 commits into
tronbyt:app-viewer-sourcefrom
saltedlolly:fix/app-viewer-hardening

Conversation

@saltedlolly

@saltedlolly saltedlolly commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Overview

This is the first App Viewer hardening step in a larger, multi-part improvement
to the Tronbyt app discovery and installation experience.

The complete work spans both the tronbyt/apps and tronbyt/server
repositories, so it is being divided into smaller PRs that can be reviewed,
tested and merged independently.

When all parts are released, users will be able to:

  1. Browse a more responsive App Viewer with display-size filters, tags, author
    pages, improved app details and README presentation.
  2. Connect the Viewer to their own Tronbyt Server.
  3. Sign in to the server when required.
  4. Verify that the Viewer and server use the same community-app repository.
  5. Select “Add to Tronbyt” from the catalogue or app details.
  6. Have the server refresh a stale local app catalogue when necessary.
  7. Review compatible, incompatible and already-installed devices.
  8. Explicitly choose a device and continue through the existing app
    configuration flow.

The Viewer will not send app source code, passwords, session cookies or API
tokens to the server. The server will install only from its own configured
system-app checkout.

Why this PR comes first

The App Viewer generates static detail and author pages from app manifests in
this repository.

Some manifest-derived values, including titles and descriptions, were inserted
into generated <title> and metadata elements with only partial quote
handling. Although manifests are reviewed before merge, generated HTML should
still treat repository metadata as untrusted input and encode it safely at the
output boundary.

This issue already existed before the new catalogue and server-integration
work. Fixing it separately:

  • prevents the larger UI changes from building on an unsafe output boundary;
  • keeps the security fix small and independently reviewable;
  • avoids mixing dependency and CI changes into the later presentation PR;
  • gives subsequent Viewer PRs regression coverage for generated metadata.

The installed js-yaml version also had a published security advisory, so this
PR updates it before additional manifest fields and catalogue provenance are
processed.

Changes in this PR

  • Escape manifest-derived values inserted into generated app detail metadata.
  • Cover text, attribute and URL contexts at the generation boundary.
  • Add regression tests for tag and attribute injection attempts, including
    scripts, closing tags, quotes, ampersands and Unicode.
  • Update js-yaml from 4.1.1 to 4.3.2 and refresh compatible transitive
    dependencies.
  • Make the generator importable for focused unit tests without automatically
    running a complete build.
  • Run the App Viewer test suite in pull-request CI.

What this PR intentionally does not include

This PR does not add or change:

  • the catalogue or app-details design;
  • display-size filters, tags or author navigation;
  • README image handling;
  • Tronbyt Server verification;
  • “Add to Tronbyt” controls;
  • app installation endpoints;
  • deployment provenance or repository matching.

Those changes will be introduced in separate PRs so their behaviour and
security boundaries can be reviewed independently.

Planned follow-up work

Separate PRs will provide:

  1. GitHub Pages build support for the new Viewer modules and catalogue
    provenance.
  2. The responsive catalogue, app details, author pages, display filtering,
    broken-app presentation and performance improvements.
  3. Tronbyt Server connection, repository matching, coordinated catalogue
    refresh and device-selection support.
  4. The Viewer-side verification and installation controls.

The final Viewer/server integration will remain a draft until the supporting
Tronbyt Server version has been released.

Security impact

This PR closes an existing generated-HTML injection weakness and updates a
dependency with a known vulnerability.

It does not add a network endpoint, change authentication, broaden permissions,
or transmit information to a Tronbyt Server.

Testing

  • npm test
  • npm audit — 0 vulnerabilities
  • Generated all 1,095 app detail pages and 597 author pages successfully
  • Pull-request CI checks
  • git diff --check

@saltedlolly
saltedlolly requested a review from tavdog as a code owner October 4, 2026 00:20
@tavdog
tavdog merged commit a2afd31 into tronbyt:app-viewer-source Oct 4, 2026
2 checks passed
@tavdog

tavdog commented Oct 4, 2026

Copy link
Copy Markdown
Member

I'm wondering what to do about this separate branch though. does it need to be kept in sync with main for any reason ?

@saltedlolly

saltedlolly commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor Author

I don't know why this is in a separate branch? Is there a good reason for that, other than slight obscuring the app viewer code from visitors to the main branch?

Perhaps the app viewer website could be moved to its own repo, then the app's repo is only for apps themselves? Though there may be a good reason for doing it this way that I have not discovered...

@saltedlolly

Copy link
Copy Markdown
Contributor Author

@tavdog I did some digging and found the original rationale in #41 and the later repository history.
The apps repository is shallow-cloned as a single branch by every Tronbyt Server installation. Keeping the Viewer source off main therefore prevents website-only files from being included in each server’s app checkout.
I hadn’t realised until now that @brombomb originally built the App Viewer—I had assumed it was you. My understanding is that Rob later introduced the app-viewer-source branch for this reason. The Pages workflow takes the current app data from main and the website source from app-viewer-source, so the Viewer branch should not need to be kept in sync with ordinary app changes.

@brombomb, have I understood the reasoning correctly? The arrangement makes sense, although the permanent parallel branch does add some maintenance and discoverability overhead. Do you think it should remain this way, or might moving the App Viewer to its own repository eventually be clearer? I don’t want to propose changing the structure without understanding the original considerations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants