**[#24](https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/24)
– `js/class.js`**
`fnTest` works by serialising a function to a string and checking if
`"xyz"` appears in it - the function is never actually called. The bare
`xyz;` is never executed, so CodeQL is right to flag it. `return xyz;`
makes the intent clear. So this is purely a cosmetic change.
**[#26](https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/26)
– `tests/e2e/helpers/global-setup.js`**
CodeQL flagged `if (exec) exec;` as a useless expression - and it was
right. But the real find was one level deeper.
`startApplication` hardcoded `const port = 8080`, so `MM_PORT` was
always overwritten before the app started. The test named "Set port 8100
on environment variable MM_PORT" was actually testing port 8080 the
whole time - it just happened to pass anyway.
Removed the dead `exec` parameter, made `startApplication` read
`MM_PORT` from the environment, and fixed the test so it actually checks
what it says it checks.
This switches the calendar fetcher from synchronous to asynchronous ICS
parsing.
It does not necessarily make parsing faster overall, but it avoids long
event-loop stalls with large calendar files (especially on slower
devices and with multiple feeds).
So this does not fully solve #4103 on its own, but it clearly mitigates
it.
It should also combine well with a future pre-filter approach discussed
in the issue:
- with pre-filter = less data to parse (future PR)
- with async parsing = less blocking while parsing (this PR)
Together, that is likely the strongest path to fully address #4103.
additional things:
- removed fix chrome-sandbox permissions (runs without)
- added `npx install-electron` which downloads the electron binaries
new in v42:
> Electron now downloads its binary into node_modules dynamically on
first launch instead of running a postinstall script. Added the
install-electron script to manually trigger the download as well.
https://github.com/electron/electron/pull/49328
This PR removes `ajv` from module position validation in `js/utils.js`
and replaces it with straightforward JavaScript checks. The old schema
only covered a few basic structure rules, so using `ajv` here felt
heavier than necessary. The updated validation keeps the same expected
behavior for valid configs and unknown-position warnings, but the code
path is now easier to read and easier to reason about when something
goes wrong.
Outcome: same validation behavior, one less dependency, less lines of
production code and better test coverage.
I noticed that in the System Information output, `used node` and
`installed node` were always identical when starting via Electron. That
usually shouldn't be the case. After digging into it, I found that since
PR #4002, `used node` in the subprocess effectively reported the system
Node version, not the parent process (which runs in Electron) version.
This PR fixes that by passing `used node` from the parent process and
logging it correctly.
**Before:**
`VERSIONS: electron: 41.3.0; used node: 26.0.0; installed node: 26.0.0;
...`
**After:**
`VERSIONS: electron: 41.3.0; used node: 24.15.0; installed node: 26.0.0;
...`
## What does this PR accomplish?
The `postinstall` script runs `git clean -df fonts vendor
modules/default`
unconditionally. When `magicmirror` is installed as an npm dependency
(e.g. `npm install magicmirror` or as a transitive dep in another
project),
npm runs `postinstall` in a directory that is **not** a git repository.
This causes:
```
fatal: not a git repository (or any of the parent directories): .git
```
npm treats the non-zero exit as a failure → the entire installation
aborts
with `code 128`. This makes it impossible to use `magicmirror` as an npm
dependency in any third-party project.
The issue has been present since at least **v2.34.0**. In **v2.35.0**
`modules/default` was added to the clean targets, but the root cause
predates that change.
## Fix
Added `scripts/postinstall.js` — a cross-platform Node.js script that
guards the `git clean` call with a `git rev-parse --git-dir` check.
- When running inside a real git repository: behaviour is identical to
before.
- When running outside one (e.g. installed as an npm package): step is
silently skipped.
`package.json` `postinstall` updated from the bare `git clean` call to:
```json
"postinstall": "node scripts/postinstall.js"
```
This approach is cross-platform (Linux, macOS, Windows) and keeps the
logic readable rather than inlined as a one-liner.
Does this solve a related issue?
No existing issue tracked. Discovered while using magicmirror as a
devDependency in a companion tooling project — installation failed
unconditionally on all platforms.
Checklist
- Targets the develop branch
- No visual changes (script + package.json only)
- node --run lint:prettier run — no changes needed for .js script
The regex captured the full fetch output line including the branch name.
Before #4115, the command was built as a shell string, so the shell
split the arguments correctly. After #4115 switched to `execFile`, the
range and branch name were passed as a single argument
(`60e0377..332e429 develop`) - which git rejects as ambiguous.
The fix replaces the regex with column-based line parsing: find the line
for the current branch, take the first column. Falls back to
`<branch>..origin/<branch>` when fetch reports no changes (same behavior
as before).
Also adds unit tests for the new helper and corrects existing test
snapshots that encoded the broken behavior.
Fixes#4137.
While removing unnecessary conditionals (for `compliments` reported in
https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/27
and for `weather` reported in
https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/28),
I noticed a bug in the imperial conversion path: falsy property checks
like `if (imperialWeatherObject.temperature)` silently skip the
conversion when the value is `0`. So at exactly 0 °C, the result would
show `0 °F` instead of the correct `32 °F`.
I wrote unit tests for `convertWeatherObjectToImperial` first, which
confirmed the bug, then fixed it by replacing the falsy checks with the
`in` operator. Which I also find more intuitive to read.
Weather APIs deliver floating point values, so hitting exactly `0.0`
might be rare in practice - more likely `0.1` or `-0.1`, which are
truthy and convert fine. That should explain why it went unnoticed by
users.
We can rely on PM2's native restart-on-exit behavior instead of the
programmatic pm2 API.
Fixes
https://github.com/MagicMirrorOrg/MagicMirror/security/dependabot/82 by
removing pm2.
Note: Originally this PR was intended to update pm2, but after
discussion, we decided to replace it instead. See the discussion below.
After #4120 was merged, here's a bit of polish on top.
- docs: Optimize JSDoc and comments.
- test: The existing 304 test is moved to the dedicated status code
block and extended with an `errorSpy` to also verify that no `error`
event is emitted.
- refactor: The success branch is moved before the error branch in
`fetch()`. This is more intuitive.
The bottom line is that this PR improves maintainability. For users,
this PR doesn't change anything.
Open-Meteo updates `current_weather` every 15 minutes, but the hourly
array only has entries at full hours. The old code did an exact
timestamp match - so at 14:15, 14:30 or 14:45 it never found a match and
silently fell back to index 0, showing midnight values for humidity,
feels-like temp, precipitation, etc.
Fix: `findLastIndex((hour) => hour.time <= currentMs)` - the last hourly
entry at or before the current time.
While fixing the bug I found several dead branches left over from the
#4032 refactor: a path for pre-transposed hourly data that
`#parseWeatherApiResponse` makes unreachable, an `Array.isArray` guard
that's always true, and a `Log.debug` inside the dead branch. Removing
those accounts for most of the ~40 deleted lines - the actual fix is one
line.
Fixes#4122.
When a client reconnects while the backend is still in its rate-limit
protection phase, the weather module has no data to show and stays on
`Loading...` until the next scheduled API call. This mainly affects
server mode setups, where the server keeps running while a remote client
temporarily loses its connection and reloads. It was [raised in the
forum](https://forum.magicmirror.builders/topic/20218/request-loop-loading...-in-standard-weather-module-open-meteo-after-update/11?_=1777106416020)
and is worthy of a fix to improve the user experience.
With this PR the node helper caches the last successful `WEATHER_DATA`
payload per instance and replays it immediately on reconnect. The client
gets its last known state right away instead of waiting for the next
fetch. The cache is cleaned up when the provider stops.
Tests are included to cover reconnect with and without cached data, and
the cleanup path.
## Summary
This updates `js/http_fetcher.js` so HTTP `304 Not Modified` responses
are emitted through the normal `response` event instead of being treated
as errors.
That aligns the core fetcher with the built-in `yr` weather provider in
`v2.35.0`, which already includes explicit logic to handle `304` and
reuse cached data.
Closes#4119.
## Root cause
`HTTPFetcher` currently treats all non-OK responses as errors. Since
`304` is not an OK response, it never reaches providers listening on the
`response` event.
At the same time, `defaultmodules/weather/providers/yr.js` expects to
receive `304` and handle it by reusing cached data.
## What changed
- special-case HTTP `304` in `HTTPFetcher`
- emit `304` through the normal `response` event
- preserve the existing error path for other non-OK statuses
## Validation
- confirmed `defaultmodules/weather/providers/yr.js` already expects
`response.status === 304`
- compared the unpatched `v2.35.0` `HTTPFetcher` behavior to the
provider logic
- ran `node --check js/http_fetcher.js`
Co-authored-by: sonnyb9 <sonnyb9@users.noreply.github.com>
This fixes CodeQL alert
[#16](https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/16)
by replacing shell-built git commands with `execFile` + `cwd` in
updatenotification’s `git_helper`.
It also includes a small cleanup in `checkUpdates()` (remove unnecessary
async/Promise wrapper) and updates the related unit tests.
As reported in #4109, the weather module retries much more frequently
than expected after network errors. #4092 already fixed the main cause
(duplicate fetchers), but the backoff logic in `HTTPFetcher` still has a
gap: once retries are exhausted, `calculateBackoffDelay` keeps returning
a short fixed delay (60s) instead of falling back to `reloadInterval`.
The same problem existed for 5xx errors, where the delay grew to 8× the
configured interval.
Inspired by #4110 (thanks @CodeLine9), this PR makes both error paths
fall back to `reloadInterval` after retries are exhausted. I also
simplified the catch block, extracted a `#shortenUrl()` helper for log
messages, and added tests for the backoff progression.
While looking at the WeatherFlow provider (to evaluate #4107), I noticed
a few things that weren't quite right.
1. **UV index was broken for most providers in forecast/hourly views.**
The templates read `uv_index`, but only the WeatherAPI provider actually
wrote that key. All other providers (OpenWeatherMap, WeatherFlow,
PirateWeather, etc.) use `uvIndex` - so UV was silently never displayed
for them. This went unnoticed because `showUVIndex` defaults to `false`
and there were no test assertions for it. Standardized everything on
`uvIndex` and added test coverage.
2. **WeatherFlow didn't map precipitation for current weather.** The API
provides `precip_accum_local_day` and `precip_probability`, but they
weren't passed through. While adding them I also noticed the template
used truthiness checks, which hid valid zero values. Fixed both.
3. **`||` vs `??` in WeatherFlow provider.** Several numeric fields used
`|| null`, replacing valid `0` with `null`. Switched to `??` for
correctness.
Fixes#4105
```bash
In JavaScript, standard JSON does not support functions.
If you use JSON.stringify() on an object containing functions,
those functions will be omitted (if they are object properties)
or changed to null (if they are in an array).
```
---------
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Disable secret substitution only for cors=allowAll, the last attempt in
#4102 was to restrictive.
Secret substitution should also work if cors=disabled.
---------
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
After the big weather refactor (#4032), OpenWeatherMap was effectively
hard-wired to One Call v3. One Call 2.5 is deprecated and no longer
available, so it looked like v2.5 support was effectively over — but the
classic `/weather` and `/forecast` endpoints were never actually
dropped. This restores support for those.
Fixes#4100.
## What this PR does
- handles OpenWeatherMap responses by endpoint again (`/onecall`,
`/weather`, `/forecast`)
- restores v2.5 current and forecast support (including hourly via
3-hour forecast slots)
- filters outdated hourly entries centrally while keeping the current
hour visible (if available)
## Screenshot
<img width="768" height="481" alt="bildo"
src="https://github.com/user-attachments/assets/9bce3531-3731-4fd7-b41e-e20603afa725"
/>
### What's the problem?
The `selfSignedCert` option passes an undici `Agent` as `dispatcher` to
`fetch()`. But Node's built-in `fetch()` and undici@8's `Agent` use
different internal handler APIs - passing them together throws:
```
invalid onRequestStart method
```
### What's the fix?
When `selfSignedCert` is enabled (i.e. a `dispatcher` is set), use
undici's own `fetch()` instead of the global one. For all other
requests, keep using `globalThis.fetch`.
```js
const fetchFn = requestOptions.dispatcher ? undiciFetch : globalThis.fetch;
```
### Why not just always use undici's fetch?
That would fix the crash - but it would break some tests. MSW (Mock
Service Worker), which is used in our test suite to intercept HTTP
requests, only hooks into `globalThis.fetch`. Undici's fetch bypasses
those interceptors entirely, so tests would start making real network
requests instead of getting the mocked responses. We could rewrite all
tests to use undici-compatible mocking instead - but that would be a
massive change for no real benefit.
----
Fixes#4093
If a module uses this.io.of() to register a custom socket.io namespace,
connections on that namespace trigger the onAny handler in setSocketIO
before config is set, causing a TypeError.
Fixes#4089
I provide docker images with alpine linux and tested the new cors
approach.
It didn't work because after calling
```js
const dispatcher = new Agent({ connect: { lookup: (_h, _o, cb) => cb(null, address, family) } });
```
the dispatcher variable was undefined.
This PR solves this and I tested this under debian too.
The mix of internal fetch and newer undici did not work and alpine needs
additionally the `process.nextTick`.
---------
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
During the server-side migration (PR #4032), the `instanceId` was built
with `Date.now()`, making it unique per client reload. This approach was
fine in the old browser-based architecture where reloads cleaned up
everything automatically. But now the node_helper persists across
reloads, so each reconnect created a new `HTTPFetcher` while the old one
kept polling - silently multiplying API calls over time.
The fix is simple: use `this.identifier` as the `instanceId`, which is
already stable and unique per module slot. This is the same approach the
Calendar module uses.
On the node_helper side, when a provider already exists for the same
`instanceId`, we now skip re-creation and just resend
`WEATHER_INITIALIZED` so the client picks up where it left off.
Fixes https://forum.magicmirror.builders/topic/20199
PR #4084 blocked SSRF by checking the IP before `fetch()` — but
`fetch()` resolves DNS again on its own. With DNS rebinding (TTL=0,
alternating IPs) an attacker can slip a private IP through between check
and connection.
Fix: resolve DNS once, validate, pin the validated IP for the
connection.
No second DNS query → no rebinding window. `isPrivateTarget()` is gone,
code is shorter than before.
Not a likely attack for a typical MagicMirror setup, but it doesn't add
complexity so there's no reason not to close the gap.
Previously only master was scanned via the default CodeQL setup. Since
development happens on develop, this PR replaces the default setup with
a custom workflow that covers both branches. This gives an overview of
the security status across the current release (master) and the
development branch (develop).
As a result we should also see issues in the develop branch here:
https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning
`node-ical` has released a new version that fixes an issue reported by
one of our users (https://github.com/jens-maus/node-ical/issues/495).
I'd like to have that in the develop branch. Instead of just updating
that, I think it would make sense to include the others as well.
`undici` had a major release but according to the release notes we are
not affected.
Resolve target hostname before proxying and reject any address that is
not globally routable (loopback, RFC 1918, link-local, etc.) using
ipaddr.js and dns.lookup().
In PR #4072 the GitHub bot complained about a missing variable
declaration for `config` in `app.js` and suggested adding `let config`.
Applying that suggestion broke the app because `server_functions.js` was
accessing `config` as an implicit global variable - the `let`
declaration made it unreachable from there.
So instead of the `let` declaration, I replaced all bare `config`
references with explicit `global.config`. This makes the dependency on
the global variable visible without changing runtime behavior,
consistent with how other globals like `global.root_path` and
`global.version` are already handled throughout the codebase.
Related to #4073
In PR #4072 GitHub Bot complained that `newsfeedfetcher.js` used the old
`.pipe()` method to connect streams (HTTP body → iconv decoding → RSS
parser). `.pipe()` has a weakness: errors in one stream are **not**
automatically forwarded to downstream streams. An I/O or decoding error
would silently disappear.
## Solution
Replaced `.pipe()` with `await stream.promises.pipeline()`. The
`pipeline()` API is designed to propagate errors correctly through the
entire chain and to clean up all streams on failure. Errors now reliably
land in the `catch` block and call `fetchFailedCallback` exactly once.
The redundant `parser.on("error")` handler was removed, as it would have
caught the same error again and called the callback a second time.
## Why not the bot's suggested fix?
The GitHub Bot suggested the older callback-based
`stream.pipeline(callback)` variant:
```js
stream.pipeline(nodeStream, iconv.decodeStream(...), parser, (error) => {
if (!error) return;
// error handling...
});
```
This has two drawbacks compared to my approach:
1. It uses the older callback style — `stream.promises.pipeline()` with
`async/await` is the modern, more readable API.
2. The bot's suggestion kept both the `parser.on("error")` handler
**and** the `catch` block, which would not have fixed the
double-callback problem.
----
Related to #4073
## Summary
This PR migrates the SMHI weather provider from the deprecated PMP3gv2
API to the new SNOW1gv1 API.
The old API (pmp3g/version/2) started returning HTTP 404 on 2026-03-31.
## Changes
- Updated API endpoint:
- `pmp3g/version/2` → `snow1g/version/1`
- Updated response parsing:
- `validTime` → `time`
- `parameters[]` → `data` (flat structure)
- Updated parameter names:
- `t` → `air_temperature`
- `ws` → `wind_speed`
- etc.
- Updated precipitation handling to match new
`predominant_precipitation_type_at_surface`
- Updated coordinate parsing (flat `[lon, lat]`)
- Added missing value handling (`9999 → null`)
## Notes
- Maintains backward compatibility for `precipitationValue` config
- No UI changes
- Symbol mapping unchanged (same codes 1–27)
## Testing
- Verified against live SMHI SNOW1gv1 API responses
- Confirmed old API returns HTTP 404
## Impact
Fixes broken SMHI provider due to API deprecation.
In PR #4072 GitHub Bot complained about an unused var. Instead of just
removing that one, I checked why ESLint hadn't complained about it: We
had disabled the rule for it.
So I enabled rule and resolved the issues that ESLint then detected.
Related to #4073
This removes a warning which appeared running a test:
```bash
$ npx vitest --run tests/unit/functions/updatenotification_spec.js
RUN v4.1.2
7:46:46 PM [vite] (ssr) warning: invalid import "../../../${defaults.defaultModulesDir}/updatenotification/git_helper". A file extension must be included in the static part of the import. For example: import(`./foo/${bar}.js`).
```
Related to #4073
Adding a SECURITY.md helps us make two things clearer:
- MagicMirror is not intended for direct public internet exposure.
- There is a clear path to report security concerns responsibly.
Related issue: #4067
---
As always, suggestions for improvement are very welcome.
The URL was built once at startup with a hardcoded start_date. Since
HTTPFetcher reuses the same URL, the forecast never advanced past day
one.
Use forecast_days instead — Open-Meteo resolves it relative to today on
every request. Other providers are not affected as they don't use dates
in their URLs.
Fixes#4063
While looking at all weather providers for #4063, I noticed another
unrelated bug: The weathergov forecast has been off by one day. This has
been since its first commit in 2019 and was not caused by the recent
weather refactor.
The provider built the days array correctly, but then threw away the
first entry (`slice(1)`) because it was considered "incomplete". The
problem: the template uses index position to decide what to label
"Today" and "Tomorrow" — so dropping today made Monday show up as
"Today", Tuesday as "Tomorrow", and one day disappeared entirely.
The fix is a one-liner: remove `slice(1)`. Today's entry now shows
min/max from the remaining hours of the day, which is the right
behaviour anyway. In my understanding it's not a problem at all that the
first day is incomplete.
## Before
Notice in the screenshot that it is Sunday. So after today and tomorrow,
Tuesday should come next, but it says **Wednesday** instead.
<img width="385" height="365" alt="Ekrankopio de 2026-03-22 14-37-55"
src="https://github.com/user-attachments/assets/02295cc6-4421-40a8-929e-6c6721dece97"
/>
## After
<img width="385" height="365" alt="Ekrankopio de 2026-03-22 14-38-34"
src="https://github.com/user-attachments/assets/cb51ca01-7882-4805-8cf4-a78f6721038a"
/>
