Fix handleUnauthorized serving old mashlib when mashlibModule configured - #147
Conversation
handleUnauthorized() didn't check request.mashlibModule, always falling back to generateDatabrowserHtml which references local /mashlib.min.js and /mash.css (404 when not installed locally). Add generateModuleDatabrowserHtml import and mashlibModule check to match the same ternary pattern used in the resource handlers. Fixes #143
There was a problem hiding this comment.
Pull request overview
Fixes unauthorized HTML responses to respect mashlibModule configuration, aligning handleUnauthorized() behavior with the mashlib selection logic used in resource handlers (avoiding broken classic mashlib asset references when only the ES module URL is configured).
Changes:
- Import
generateModuleDatabrowserHtmlinto the auth middleware. - Update
handleUnauthorized()to serve module-based mashlib HTML whenrequest.mashlibModuleis set (with SolidOS UI still taking precedence). - Preserve existing classic mashlib behavior (local/CDN) as the fallback.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| : request.mashlibModule | ||
| ? generateModuleDatabrowserHtml(request.mashlibModule) | ||
| : generateDatabrowserHtml(request.url, request.mashlibCdn ? request.mashlibVersion : null); |
There was a problem hiding this comment.
generateModuleDatabrowserHtml expects a string URL (it calls .replace(/\.js$/, ...)). request.mashlibModule can be a non-string truthy value (e.g., true via JSS_MASHLIB_MODULE=true because env parsing coerces "true" to boolean), which would throw a TypeError here and turn 401/403 into a 500. Consider validating request.mashlibModule is a non-empty string before calling, and/or rejecting invalid config earlier with a clear startup error.
| if (request.mashlibEnabled) { | ||
| // Use SolidOS UI if enabled, otherwise fallback to classic mashlib | ||
| // Use SolidOS UI if enabled, ES module if configured, otherwise classic mashlib | ||
| const html = request.solidosUiEnabled | ||
| ? generateSolidosUiHtml() | ||
| : generateDatabrowserHtml(request.url, request.mashlibCdn ? request.mashlibVersion : null); | ||
| : request.mashlibModule | ||
| ? generateModuleDatabrowserHtml(request.mashlibModule) | ||
| : generateDatabrowserHtml(request.url, request.mashlibCdn ? request.mashlibVersion : null); | ||
| return reply.code(statusCode).type('text/html').send(html); |
There was a problem hiding this comment.
This change adds a new HTML response path for unauthenticated requests when mashlibModule is configured, but there doesn't appear to be test coverage asserting which HTML wrapper is served for 401/403 with Accept: text/html. Adding an integration test would help prevent regressions across mashlibModule / mashlibCdn / solidosUi configurations.
Summary
generateModuleDatabrowserHtmlimport to auth middlewaremashlibModulecheck inhandleUnauthorized()to match the ternary pattern used in resource handlersProblem
When
mashlibModuleis configured, unauthenticated/unauthorized requests still get the classic mashlib HTML (referencing local/mashlib.min.jsand/mash.css) instead of the ES module HTML. This causes 404 errors since the local mashlib files aren't installed.Test plan
mashlibModulein config, visit a resource without auth → mashlib-next module HTML servedmashlibCdnconfig → classic CDN mashlib HTML still workssolidosUiconfig → SolidOS UI HTML still worksmashlib: true(local) → classic local mashlib HTML still worksFixes #143